split scc admission into mutating and validating #20605
Conversation
openshift-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
| break loop | ||
| default: | ||
| glog.V(6).Infof("pod %s (generate: %s) validated against provider %s, but required mutation, skipping", pod.Name, pod.GenerateName, provider.GetSCCName()) | ||
| glog.V(4).Infof("pod %s (generate: %s) validated against provider %s, but required mutation, skipping", pod.Name, pod.GenerateName, provider.GetSCCName()) |
There was a problem hiding this comment.
V(5), maybe? this one could be noisy
| pod.ObjectMeta.Annotations[allocator.ValidatedSCCAnnotation] = allowingProvider.GetSCCName() | ||
| return nil | ||
| if allowedPod == nil || allowingProvider == nil { | ||
| return nil, "", validationErrs, nil |
There was a problem hiding this comment.
if the pod wasn't allowed, why is this returning a nil error?
edit: returning nil is ok as long as all callers check for a nil allowedPod and reject otherwise
| if err != nil { | ||
| return admission.NewForbidden(a, err) | ||
| } | ||
| if apiequality.Semantic.DeepEqual(pod, allowedPod) { |
There was a problem hiding this comment.
need to make sure allowedPod is non-nil here
There was a problem hiding this comment.
need to make sure allowedPod is non-nil here
Just in case a deep equals of nil matches an actual pod?
| // 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 && rbacregistry.IsOnlyMutatingGCFields(a.GetObject(), a.GetOldObject(), kapihelper.Semantic) { |
There was a problem hiding this comment.
should we also be checking that the operation is UPDATE? I vaguely remember a recent change around this
There was a problem hiding this comment.
should we also be checking that the operation is UPDATE?
This is a straight move. I didn't check. We have concerns about existing behavior?
There was a problem hiding this comment.
tracked it down to a 1.12-era PR fixing admission for create-on-update cases (kubernetes/kubernetes#65572 (comment)). this is fine for 1.11, but we need to sweep before 1.12 to make sure we're not using GetOldObject() != nil when we really mean GetOperation() == UPDATE
|
nit on checking nil pod on return, lgtm otherwise |
deads2k
force-pushed
the
admission-01-scc
branch
from
b82d450
to
6bff3d0
Compare
|
comments addressed. |
|
forgot about the log level one, sorry |
deads2k
force-pushed
the
admission-01-scc
branch
from
6bff3d0
to
5b289fa
Compare
Uh who exactly is cc @openshift/sig-security |
|
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
We have not split our admission chain into validating and mutating. Because of this we disable the webhook admission plugins by default and don't claim support. When they are turned on, they bypass our admission chain checks. There are more to do, but SCC stands out as one that is super important and we don't want people to get used to bypassing.
@openshift/sig-master
@liggitt as discussed