Convert subjectchecker to use rbac.Subject #16286
Conversation
openshift-ci-robot
added
the
size/L
|
@enj PTAL |
|
I added #16259 as a fixed bug as well. |
|
@enj oh right, thanks! |
| @@ -18,7 +18,7 @@ import ( | |||
| // SubjectChecker determines whether rolebindings on a subject (user, group, or | |||
| // service account) are allowed in a project. | |||
| type SubjectChecker interface { | |||
| Allowed(kapi.ObjectReference, *RoleBindingRestrictionContext) (bool, error) | |||
| Allowed(rbac.Subject, *RoleBindingRestrictionContext) (bool, error) | |||
There was a problem hiding this comment.
Maybe this should be Allowed(subject *rbac.Subject, ctx *RoleBindingRestrictionContext) (allowed bool, err error)
*rbac.Subject instead of rbac.Subject?
There was a problem hiding this comment.
makes it impossible to pass in nil, and basically just maintaining the previous interface, not a hard reason, but also saw no reason to change it, Subjects are pretty small
|
LGTM just a minor thought. /approve @deads2k sanity check please. |
openshift-merge-robot
added
the
approved
|
/retest |
|
Flaked on #16273 |
|
/retest |
| @@ -167,7 +154,7 @@ func (checker UserSubjectChecker) Allowed(subject kapi.ObjectReference, ctx *Rol | |||
| } | |||
|
|
|||
| // System users can match only by name. | |||
| if subject.Kind != authorizationapi.UserKind { | |||
| if subject.Kind != rbac.UserKind { | |||
There was a problem hiding this comment.
The entire if block and comment should be deleted. Its purpose was to skip checking groups and selectors for system users, but now it is redundant.
| @@ -233,7 +219,7 @@ func (checker GroupSubjectChecker) Allowed(subject kapi.ObjectReference, ctx *Ro | |||
| } | |||
|
|
|||
| // System groups can match only by name. | |||
| if subject.Kind != authorizationapi.GroupKind { | |||
| if subject.Kind != rbac.GroupKind { | |||
There was a problem hiding this comment.
This entire if block and comment are now redundant and should be deleted.
| } | ||
|
|
||
| if subject.Kind != authorizationapi.UserKind { | ||
| func (ctx *RoleBindingRestrictionContext) labelSetForUser(subject rbac.Subject) (labels.Set, error) { |
There was a problem hiding this comment.
Are NotFoundErrors well tolerated by callers?
There was a problem hiding this comment.
Seem like all errors are tolerated equally as they are all printed and gathered in a composed error
|
SystemUsers will fail user resource lookups. I think its fine since it fails tighter instead of looser, but there should probably be a test making sure that still works as expected. |
|
@deads2k can you propose a test that would check it ? (pseudo code is fine) |
binding a user without any user object (like system:anything-here) should not fail admission if it is specifically listed as an allowed user. I don't remember what the code actually does, it may actually do that and only return the NotFoundError if it fails |
This also removes any distinction between System and regular User/Groups, as that distinction is gone with RBAC RoleBindings. Signed-off-by: Simo Sorce <simo@redhat.com>
|
@deads2k can you check if the additional test is ok and lgtm ? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, enj, simo5 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 16384, 16327, 16199, 16286, 16378) |
This also removes any distinction between System and regular User/Groups, as that distinction is gone with RBAC RoleBindings.
Fixes #16032
Fixes #16259