Add configuration to permissions pre-flight check to use SelfSubjectAccessReview or SelfSubjectRulesReview
#931
Conversation
|
@rashmigottipati @tmshort Do y'all want to review this first? In case you already have and it looks good to you, please drop an LGTM 😄 |
|
@praveenrewar that sounds good, I'll make sure to review it today/tomorrow. :) |
| // An error is returned if there are any issues creating a SelfSubjectRulesReview (i.e can't determine permissions) | ||
| // or if the SelfSubjectRulesReview is evaluated and the caller does not have the permission to perform the actions | ||
| // identified in the provided ResourceAttributes. | ||
| func (rv *SelfSubjectRulesReviewValidator) ValidatePermissions(ctx context.Context, resourceAttrib *authv1.ResourceAttributes) error { |
There was a problem hiding this comment.
(optional) nit: resourceAttributes instead of resourceAttrib? (Also applicable to ValidatePermissions on SelfSubjectAccessReviewValidator)
There was a problem hiding this comment.
Ah, I missed making this change. I can make this change if needed, but functionally it shouldn't make a difference
I think that is reasonable. I think this might be possible via adding a kapp Config resource to the appropriate tests and changing the value there, which I hadn't thought of before. I'll try to have a change made soon-ish for this |
|
@everettraven Just checking if you are still working on it or your forgot to push your changes. (No rush) |
|
@praveenrewar thanks for following up! I was working on the changes a bit yesterday. I'll finish them up and push today |
to allow configuring whether it uses SelfSubjectAccessReview or SelfSubjectRulesReview to determine if a user has the appropriate permissions. Defaults to SelfSubjectAccessReview for backwards compatibility Signed-off-by: everettraven <[email protected]>
Signed-off-by: everettraven <[email protected]>
everettraven
force-pushed
the
enhancement/permissions-ssrr
branch
from
cb68443
to
0505c5f
Compare
|
@praveenrewar @100mik rebased and pushed. I can squash the commits if needed |
What this PR does / why we need it:
SelfSubjectAccessReviews are great for this pre-flight check's use case, but can a bit intense on the API server (for every resource we are sending at least one request to the API server). For users who are concerned about the load placed on the API server, it would be nice to allow the option to use aSelfSubjectRulesReviewinstead as it will give the set of permissions a user has for a namespace. Using this API, and caching the results per namespace, results in less requests sent directly to the API server.This PR:
permissionspackage to have a pluggablePermissionValidatorinterface that has aValidatePermissionsfunction instead of using a single, inextensible function as existed previously.PermissionValidatorimplementations that validates permissions usingSelfSubjectAccessReviewandSelfSubjectRulesReviewPermissionValidatorinterfaceWhich issue(s) this PR fixes:
Fixes #930
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: