✨ Implement `RulesFor` for `GlobalAuthorizer` and `LocalAuthorizer` to enable `SelfSubjectRulesReview` #3097

embik · 2024-03-25T13:17:17Z

Summary

This PR implements the stub RulesFor methods for both GlobalAuthorizer and LocalAuthorizer to enable usage of the SelfSubjectRulesReview API. I'll be completely honest, this felt "too easy", so I hope I'm doing the correct thing here. Output of kubectl auth can-i --list looked sensible to me.

Basically, I've aligned the way both the global and the local authorizer generate their cluster-specific authorizer and just called RulesFor on the scoped authorizer created from it. After PR review, I've also added two e2e test cases that make sure that the SelfSubjectRulesReview API gives expected responses.

On a minor note, this PR cleans up the OWNERS file because it's from the before times (unless we want to keep access to this package limited, which might be fair).

Related issue(s)

Fixes #1924

Release Notes

Implement `SelfSubjectRulesReview` API, enabling usage of e.g. `kubectl auth can-i --list`

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

sttts · 2024-03-28T22:04:37Z

pkg/authorization/local_authorizer.go

-	roleLister               rbacv1listers.RoleClusterLister
-	roleBindingLister        rbacv1listers.RoleBindingClusterLister
-	clusterRoleBindingLister rbacv1listers.ClusterRoleBindingClusterLister
-	clusterRoleLister        rbacv1listers.ClusterRoleClusterLister


these were intentional I believe to ensure the informers are started. If you move them into the hot path, the Lister() call might come to late after the shared informer factory has been started. Creates ugly races.

code comment would be good if this is the case

Yeah, let me bring this back and add a note. I wasn't aware of this and thought they were just written at different times and there was room to unify the code.

I'll do a little PR history search to see if I can find some additional context.

we do similar things in the controllers. They all store listers in their struct to enforce creation of the informers at constructor time.

Deleted my previous comment by mistake. I copied this pattern over from the GlobalAuthorizer, which also takes SharedInformerFactory input parameters. Should we adjust that one as well, or is there something special with it that will prevent those races mentioned?

pkg/authorization/global_authorizer.go

embik · 2024-04-03T09:48:00Z

@sttts @mjudeikis ready for re-review.

embik · 2024-04-03T11:43:11Z

/retest

looks like flakes ...? Let's see. Tests are not related I think, but maybe I broke something.

sttts · 2024-04-04T09:51:41Z

pkg/authorization/global_authorizer.go

-				&rbac.ClusterRoleBindingLister{Lister: globalKubeInformers.Rbac().V1().ClusterRoleBindings().Lister().Cluster(clusterName)},
-			)
-		},
+		globalRoleLister: globalKubeInformers.Rbac().V1().Roles().Lister(),


nit: globalRoles

Would be less noisy.

I tried to keep it consistent with LocalAuthorizer, which looks like this:

a := &LocalAuthorizer{ // listers are saved in the struct here to ensure that informers are started and we do not encounter race conditions with them. roleLister: versionedInformers.Rbac().V1().Roles().Lister(), roleBindingLister: versionedInformers.Rbac().V1().RoleBindings().Lister(), clusterRoleLister: versionedInformers.Rbac().V1().ClusterRoles().Lister(), clusterRoleBindingLister: versionedInformers.Rbac().V1().ClusterRoleBindings().Lister(), }

sttts · 2024-04-04T09:53:21Z

pkg/authorization/local_authorizer.go

@@ -41,6 +42,7 @@ type LocalAuthorizer struct {

 func NewLocalAuthorizer(versionedInformers kcpkubernetesinformers.SharedInformerFactory) (authorizer.Authorizer, authorizer.RuleResolver) {
 	a := &LocalAuthorizer{
+		// listers are saved in the struct here to ensure that informers are started and we do not encounter race conditions with them.


nit: are instantiated early and ... with starting them

Much clearer, thanks. Updated the comments.

sttts · 2024-04-04T09:53:56Z

Some nits.

/lgtm
/approve
/hold

if you want to address the nits.

kcp-ci-bot · 2024-04-04T09:54:01Z

LGTM label has been added.

Git tree hash: f0636f9bf7e16fa0a836af78aa11003cf66051b8

kcp-ci-bot · 2024-04-04T09:54:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/authorization/OWNERS~~ [sttts]
~~test/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ment RulesFor Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

embik · 2024-04-04T09:59:21Z

/hold cancel

Addressed comment nit. I'm not 100% sure how to proceed with the lister fields, but from my perspective it's okay, so this can go in with an lgtm.

sttts · 2024-04-04T12:24:34Z

/lgtm

kcp-ci-bot · 2024-04-04T12:24:38Z

LGTM label has been added.

Git tree hash: 7b33ee050e76409f80b7eff394e2ae23daa6a498

Remove pkg/authorization/OWNERS

291d75c

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

embik requested a review from sttts March 25, 2024 13:17

kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the DCO. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 25, 2024

sttts reviewed Mar 28, 2024

View reviewed changes

pkg/authorization/global_authorizer.go Show resolved Hide resolved

embik force-pushed the authorizer-rules-for branch from 644da14 to 8d784e6 Compare April 3, 2024 09:43

kcp-ci-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 3, 2024

embik force-pushed the authorizer-rules-for branch from 8d784e6 to 3789416 Compare April 3, 2024 09:47

embik requested review from sttts and mjudeikis April 3, 2024 09:48

embik force-pushed the authorizer-rules-for branch from 6068c34 to 5b159c0 Compare April 3, 2024 10:33

embik force-pushed the authorizer-rules-for branch from 5b159c0 to 3ffeccb Compare April 3, 2024 12:13

sttts reviewed Apr 4, 2024

View reviewed changes

kcp-ci-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2024

kcp-ci-bot assigned sttts Apr 4, 2024

kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2024

kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2024

embik added 3 commits April 4, 2024 11:57

Align generating authorizers in local and global authorizer and imple…

6391528

…ment RulesFor Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

e2e: add authorizer test cases for SelfSubjectRulesReview API

ecfdad4

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

Make linter happy

6b55a8e

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>

embik force-pushed the authorizer-rules-for branch from 3ffeccb to 6b55a8e Compare April 4, 2024 09:57

kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2024

kcp-ci-bot requested a review from sttts April 4, 2024 09:57

kcp-ci-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2024

kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2024

kcp-ci-bot merged commit ca53991 into kcp-dev:main Apr 4, 2024
16 checks passed

embik deleted the authorizer-rules-for branch April 4, 2024 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Implement `RulesFor` for `GlobalAuthorizer` and `LocalAuthorizer` to enable `SelfSubjectRulesReview` #3097

✨ Implement `RulesFor` for `GlobalAuthorizer` and `LocalAuthorizer` to enable `SelfSubjectRulesReview` #3097

embik commented Mar 25, 2024 •

edited

Loading

sttts Mar 28, 2024 •

edited

Loading

mjudeikis Mar 30, 2024

embik Mar 30, 2024

embik Mar 30, 2024 •

edited

Loading

sttts Mar 31, 2024

embik Apr 2, 2024

embik commented Apr 3, 2024

embik commented Apr 3, 2024

sttts Apr 4, 2024

embik Apr 4, 2024

sttts Apr 4, 2024

embik Apr 4, 2024

sttts commented Apr 4, 2024

kcp-ci-bot commented Apr 4, 2024

kcp-ci-bot commented Apr 4, 2024

embik commented Apr 4, 2024

sttts commented Apr 4, 2024

kcp-ci-bot commented Apr 4, 2024

✨ Implement RulesFor for GlobalAuthorizer and LocalAuthorizer to enable SelfSubjectRulesReview #3097

✨ Implement RulesFor for GlobalAuthorizer and LocalAuthorizer to enable SelfSubjectRulesReview #3097

Conversation

embik commented Mar 25, 2024 • edited Loading

Summary

Related issue(s)

Release Notes

sttts Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embik Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embik commented Apr 3, 2024

embik commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Apr 4, 2024

kcp-ci-bot commented Apr 4, 2024

kcp-ci-bot commented Apr 4, 2024

embik commented Apr 4, 2024

sttts commented Apr 4, 2024

kcp-ci-bot commented Apr 4, 2024

✨ Implement `RulesFor` for `GlobalAuthorizer` and `LocalAuthorizer` to enable `SelfSubjectRulesReview` #3097

✨ Implement `RulesFor` for `GlobalAuthorizer` and `LocalAuthorizer` to enable `SelfSubjectRulesReview` #3097

embik commented Mar 25, 2024 •

edited

Loading

sttts Mar 28, 2024 •

edited

Loading

embik Mar 30, 2024 •

edited

Loading