Update RoleBindingRestriction admission to handle RBAC #16163
Conversation
openshift-ci-robot
added
the
size/L
|
/retest |
| @@ -98,7 +99,12 @@ func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) { | |||
| // We only care about rolebindings and policybindings; ignore anything else. | |||
| gr := a.GetResource().GroupResource() | |||
| switch { | |||
| case authorizationapi.IsResourceOrLegacy("policybindings", gr), authorizationapi.IsResourceOrLegacy("rolebindings", gr): | |||
| case authorizationapi.IsResourceOrLegacy("policybindings", gr): | |||
There was a problem hiding this comment.
Hmm maybe we do not need this anymore. During HA upgrade if you talk with a new master about policy bindings it will simply say it does not know what that is. Old masters will continue to work for policy bindings and origin role bindings. Old masters also do not allow messing with RBAC by normal users. @deads2k do you agree that we are safe in dropping this case?
There was a problem hiding this comment.
We don't care about the policybinding case because new masters (where this admission plugin will be running) will 404 before they get to admission.
| for _, tc := range testCases { | ||
| switch tc.object.(type) { | ||
| case *authorizationapi.RoleBinding: | ||
| newObj := rbac.RoleBinding{} |
| // object | ||
| convTestCases := []testCase{} | ||
| for _, tc := range testCases { | ||
| switch tc.object.(type) { |
There was a problem hiding this comment.
x := tc.object.(type) to save you one of the type casts.
| case *authorizationapi.RoleBinding: | ||
| newObj := rbac.RoleBinding{} | ||
| oldObj := rbac.RoleBinding{} | ||
| if err := authorizationapi.Convert_authorization_RoleBinding_To_rbac_RoleBinding(tc.object.(*authorizationapi.RoleBinding), &newObj, nil); err != nil { |
There was a problem hiding this comment.
You probably want to use the helpers to have deep copies just in case.
| @@ -569,6 +572,40 @@ func TestAdmission(t *testing.T) { | |||
| stopCh := make(chan struct{}) | |||
| defer close(stopCh) | |||
|
|
|||
| // for each testCase that involves an authorizationapi rolebinding also | |||
There was a problem hiding this comment.
I would like one command line based test for each type (i.e. I want this verified with a real server).
| @@ -127,7 +130,7 @@ func TestAdmission(t *testing.T) { | |||
| Name: "rolebinding", | |||
| }, | |||
| Subjects: []kapi.ObjectReference{userAliceRef}, | |||
| RoleRef: kapi.ObjectReference{Namespace: authorizationapi.PolicyName}, | |||
| RoleRef: kapi.ObjectReference{Namespace: "namespace"}, | |||
| } | ||
| } | ||
|
|
||
| glog.V(4).Infof("Handling rolebinding %s/%s", |
| case authorizationapi.IsResourceOrLegacy("policybindings", gr), authorizationapi.IsResourceOrLegacy("rolebindings", gr): | ||
| case authorizationapi.IsResourceOrLegacy("policybindings", gr): | ||
| // legacy policy bindings | ||
| case authorizationapi.IsResourceOrLegacy("rolebindings", gr): |
There was a problem hiding this comment.
Since you're proxying through to RBAC rolebindings as the original user, yoiu probably don't care about this case, right? Admission will run on the RBAC resource.
|
|
||
| glog.V(4).Infof("Handling rolebinding %s/%s", | ||
| rolebinding.Namespace, rolebinding.Name) | ||
|
|
||
| case authorizationapi.IsResourceOrLegacy("rolebindings", gr): |
There was a problem hiding this comment.
Why not just enforce on the RBAC resource you proxied to?
There was a problem hiding this comment.
That is probably what we want to do, though at that point we may need to rewrite most of the logic in this file and the tests to be RBAC specific. We may want to take a half step and have this working using conversions so that it no longer blocks deployment to online. Then we can continue to work on this for 3.7 release without rushing.
There was a problem hiding this comment.
ok I think it's fair, I'll go ahead and actually drop all authorization api related stuff in RBR and tests, and only base on RBAC.
There was a problem hiding this comment.
@enj, I am still using the conversion functions to convert subjects, as I am not rewriting subjectchecker at this time, we'll probably open a followup issue for that one for now
| @@ -22,6 +23,19 @@ import ( | |||
| userapi "github.com/openshift/origin/pkg/user/apis/user" | |||
| ) | |||
|
|
|||
| type testCase struct { | |||
| @@ -114,66 +113,39 @@ func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) { | |||
| return nil | |||
| } | |||
|
|
|||
| var subjects, oldSubjects []kapi.ObjectReference | |||
| var subjects, oldSubjects []rbac.Subject | |||
There was a problem hiding this comment.
Use rolebinding.Subjects and remove the subjects var.
|
|
||
| systemuserRef = kapi.ObjectReference{ | ||
| Kind: authorizationapi.SystemUserKind, | ||
| Name: "system user", |
| name: "prohibit a user in a policybinding without a matching selector", | ||
| expectedErr: fmt.Sprintf("rolebindings to %s %q are not allowed", | ||
| userBobRef.Kind, userBobRef.Name), | ||
| object: &authorizationapi.PolicyBinding{ |
There was a problem hiding this comment.
I do enjoy deleting policy code.
|
@simo5 please add command level tests for this (both proxied and direct RBAC). I want confirmation this works in a real setup. |
simo5
force-pushed
the
updateRBR
branch
2 times, most recently
from
33cc2ac to
479f306
Compare
|
@enj added a native rbac test in integration, other "real" tests were already there and working. PTAL |
Signed-off-by: Simo Sorce <[email protected]>
|
/lgtm @simo5 please tag this will the followup issue to remove the conversions and use RBAC directly (and drop all the system kind bits). @deads2k feel free to block if #16163 (comment) is an issue. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-merge-robot
added
the
approved
|
Automatic merge from submit-queue |
Fixes #16042