Fix oc policy remove-user to remove rolebindings too #18550
Conversation
openshift-ci-robot
added
the
size/XS
|
@soltysh this lgtm |
|
/test crio |
|
Please fix #18102 (comment) |
| @@ -90,7 +91,7 @@ func TestPolicyCommands(t *testing.T) { | |||
| } | |||
|
|
|||
| viewers, err = haroldAuthorizationClient.RoleBindings(projectName).Get("view", metav1.GetOptions{}) | |||
| if err != nil { | |||
| if !errors.IsNotFound(err) { | |||
There was a problem hiding this comment.
A comment explaining this would be helpful.
| if len(currBinding.Subjects) > 0 { | ||
| _, err = o.Client.RoleBindings(o.BindingNamespace).Update(&currBinding) | ||
| } else { | ||
| err = o.Client.RoleBindings(o.BindingNamespace).Delete(currBinding.Name, &metav1.DeleteOptions{}) |
There was a problem hiding this comment.
These delete options should include the UID as a precondition (same with the other delete added in the last PR).
There was a problem hiding this comment.
Hmm it is actually UID + RV that we want here. @liggitt do we have a way to specify that via delete options?
There was a problem hiding this comment.
Preconditions doesn't support resource version today... no particular reason it couldn't
There was a problem hiding this comment.
I do not see why we want either UID or RV, most of these commands are not "safe" that way and they do not look they are meant to be or can be.
There was a problem hiding this comment.
Pinning uid and rv as preconditions would ensure you only deleted the object in its current state (another modification that added another subject would change the rv and make your delete fail on a conflict error, notifying you you were deleting data added by some other process)
There was a problem hiding this comment.
I know what UID and RV do, but no other command in oc adm policy does that fore removal, so I do not think it makes sense to try to shoehorn that in this PR, which is just a followup to fix stuff for 3.9
We can open an Issue to make all these commands "safer" under this aspect if we think it makes sense to do so.
There was a problem hiding this comment.
no other command in oc adm policy does that fore removal
no other command injects data into found resources either
so I do not think it makes sense to try to shoehorn that in this PR
agree it isn't required for this PR
Followup to openshift#18102 Signed-off-by: Simo Sorce <simo@redhat.com>
simo5
force-pushed
the
fix_rmfromproj
branch
from
d70e210
to
df98bdd
Compare
openshift-ci-robot
added
size/S
|
Fixed the first 2 nits, not the last on the Delete() as I do not think we need that. |
| @@ -513,6 +513,12 @@ func (o *RoleModificationOptions) RemoveRole() error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| // Check that we update the rolebinding for the intended role. | |||
| if existingRoleBinding.RoleRef.Name != o.RoleName || existingRoleBinding.RoleRef.Namespace != o.RoleNamespace { | |||
| return fmt.Errorf("rolebinding %s contains role %s in namespace %s, instead of role %s in namespace %s", | |||
There was a problem hiding this comment.
Add a test that fails when this case is hit, otherwise LGTM.
There was a problem hiding this comment.
Please open a separate issue with all the nits in the original code that you'd like fixed.
I want this PR to unblock QA for 3.9, if time will permit I'll bring down tech debt in a separate PR that closes your issue
|
/test gcp |
|
[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-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Automatic merge from submit-queue (batch tested with PRs 18437, 18546, 18550, 18579). |
Followup to #18102