don't prevent updates that only touch ownerrefs #14816
Conversation
|
[test] |
|
Evaluated for origin test up to 15766a1 |
| @@ -46,6 +46,14 @@ func (a *buildByStrategy) Admit(attr admission.Attributes) error { | |||
| if buildapi.IsResourceOrLegacy("builds", gr) && attr.GetSubresource() == "details" { | |||
| return nil | |||
| } | |||
|
|
|||
| // if this is an update, see if we are only updating the ownerRef. Garbage collection does this | |||
There was a problem hiding this comment.
s/if/If/
2 spaces before Garbage
There was a problem hiding this comment.
Two spaces after a dot is an Americanism ;) Not to worry about.
| } | ||
|
|
||
| for _, tc := range tests { | ||
| actual := IsOnlyMutatingOwnerRefs(tc.obj(), tc.old()) |
There was a problem hiding this comment.
@deads2k any reason not to call t.Run since we have go 1.7 (https://blog.golang.org/subtests)
There was a problem hiding this comment.
@deads2k any reason not to call t.Run since we have go 1.7 (https://blog.golang.org/subtests)
Just existing style. I'll keep it in mind moving forward.
| @@ -49,7 +49,7 @@ func (d *sccExecRestrictions) Admit(a admission.Attributes) (err error) { | |||
|
|
|||
| // TODO, if we want to actually limit who can use which service account, then we'll need to add logic here to make sure that | |||
| // we're allowed to use the SA the pod is using. Otherwise, user-A creates pod and user-B (who can't use the SA) can exec into it. | |||
| createAttributes := admission.NewAttributesRecord(pod, pod, kapi.Kind("Pod").WithVersion(""), a.GetNamespace(), a.GetName(), a.GetResource(), "", admission.Create, a.GetUserInfo()) | |||
| createAttributes := admission.NewAttributesRecord(pod, nil, kapi.Kind("Pod").WithVersion(""), a.GetNamespace(), a.GetName(), a.GetResource(), "", admission.Create, a.GetUserInfo()) | |||
| // if this is an update, see if we are only updating the ownerRef. Garbage collection does this | ||
| // and we should allow it in general, since you had the power to update and the power to delete. | ||
| // The worst that happens is that you delete something, but you aren't controlling the privileged object itself | ||
| if a.GetOldObject() != nil && oadmission.IsOnlyMutatingOwnerRefs(a.GetObject(), a.GetOldObject()) { |
There was a problem hiding this comment.
@deads2k any chance we could check this only once for different objects? I am not optimistic but asking to be sure
There was a problem hiding this comment.
@deads2k any chance we could check this only once for different objects? I am not optimistic but asking to be sure
I don't understand the question. I think this only happens once per admission plugin which is restricting updates with special/additional permissions.
There was a problem hiding this comment.
I meant if ownerRefs changes could be whitelisted for all objects without injecting it into specific admission plugins. First thing that comes to my mind is that we will add/modify admission an restrict it for some objects without knowing about it. We don't have any tests to make sure ownerRefs are editable.
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2503/) (Base Commit: 0fba3e5) (PR Branch Commit: 15766a1) |
Probably integration in openshift. Perhaps part of a larger GC test once we have more of the behavior locked down. [merge] |
|
re[merge][severity:blocker] |
|
Evaluated for origin merge up to 15766a1 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1098/) (Base Commit: 4917a3e) (PR Branch Commit: 15766a1) (Extended Tests: blocker) (Image: devenv-rhel7_6392) |
fixes #14149