diff --git a/plugin/pkg/admission/security/podsecuritypolicy/admission.go b/plugin/pkg/admission/security/podsecuritypolicy/admission.go index da6ec45695d..fa6f62e36e7 100644 --- a/plugin/pkg/admission/security/podsecuritypolicy/admission.go +++ b/plugin/pkg/admission/security/podsecuritypolicy/admission.go @@ -119,11 +119,16 @@ func (c *PodSecurityPolicyPlugin) Admit(a admission.Attributes) error { return nil } + // only mutate if this is a CREATE request. On updates we only validate. + // TODO(liggitt): allow spec mutation during initializing updates? + if a.GetOperation() != admission.Create { + return nil + } + pod := a.GetObject().(*api.Pod) - // compute the context. If the current security context is valid, this call won't change it. - allowMutation := a.GetOperation() == admission.Create // TODO(liggitt): allow spec mutation during initializing updates? - allowedPod, pspName, validationErrs, err := c.computeSecurityContext(a, pod, allowMutation) + // compute the context + allowedPod, pspName, validationErrs, err := c.computeSecurityContext(a, pod, true) if err != nil { return admission.NewForbidden(a, err) } @@ -134,10 +139,6 @@ func (c *PodSecurityPolicyPlugin) Admit(a admission.Attributes) error { if pod.ObjectMeta.Annotations == nil { pod.ObjectMeta.Annotations = map[string]string{} } - // set annotation to mark this as passed. Note, that the actual value is not important, the - // validating PSP might even change later-on. Also not that pspName can be the empty string - // if failOnNoPolicies is false. - // TODO: if failOnNoPolicies is toggled from false to true, we will never update the annotation anymore. Is this desired? pod.ObjectMeta.Annotations[psputil.ValidatedPSPAnnotation] = pspName return nil } @@ -156,7 +157,7 @@ func (c *PodSecurityPolicyPlugin) Validate(a admission.Attributes) error { pod := a.GetObject().(*api.Pod) - // compute the context. If the current security context is valid, this call won't change it. + // compute the context. Mutation is not allowed. allowedPod, _, validationErrs, err := c.computeSecurityContext(a, pod, false) if err != nil { return admission.NewForbidden(a, err) diff --git a/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go b/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go index b64bf59e160..bf043b75232 100644 --- a/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go +++ b/plugin/pkg/admission/security/podsecuritypolicy/admission_test.go @@ -329,6 +329,11 @@ func TestAdmitPreferNonmutating(t *testing.T) { changedPod := unprivilegedRunAsAnyPod.DeepCopy() changedPod.Spec.Containers[0].Image = "myimage2" + podWithSC := unprivilegedRunAsAnyPod.DeepCopy() + podWithSC.Annotations = map[string]string{psputil.ValidatedPSPAnnotation: privilegedPSP.Name} + changedPodWithSC := changedPod.DeepCopy() + changedPodWithSC.Annotations = map[string]string{psputil.ValidatedPSPAnnotation: privilegedPSP.Name} + gcChangedPod := unprivilegedRunAsAnyPod.DeepCopy() gcChangedPod.OwnerReferences = []metav1.OwnerReference{{Kind: "Foo", Name: "bar"}} gcChangedPod.Finalizers = []string{"foo"} @@ -336,7 +341,7 @@ func TestAdmitPreferNonmutating(t *testing.T) { tests := map[string]struct { operation kadmission.Operation pod *kapi.Pod - oldPod *kapi.Pod + podBeforeUpdate *kapi.Pod psps []*extensions.PodSecurityPolicy shouldPassAdmit bool shouldPassValidate bool @@ -380,8 +385,8 @@ func TestAdmitPreferNonmutating(t *testing.T) { }, "pod should prefer non-mutating PSP on update": { operation: kadmission.Update, - pod: unprivilegedRunAsAnyPod.DeepCopy(), - oldPod: changedPod.DeepCopy(), + pod: changedPodWithSC.DeepCopy(), + podBeforeUpdate: podWithSC.DeepCopy(), psps: []*extensions.PodSecurityPolicy{mutating2, mutating1, privilegedPSP}, shouldPassAdmit: true, shouldPassValidate: true, @@ -390,12 +395,12 @@ func TestAdmitPreferNonmutating(t *testing.T) { expectedContainerUser: nil, expectedPSP: privilegedPSP.Name, }, - "pod should not allow mutation on update": { + "pod should not mutate on update, but fail validation": { operation: kadmission.Update, - pod: unprivilegedRunAsAnyPod.DeepCopy(), - oldPod: changedPod.DeepCopy(), + pod: changedPod.DeepCopy(), + podBeforeUpdate: unprivilegedRunAsAnyPod.DeepCopy(), psps: []*extensions.PodSecurityPolicy{mutating2, mutating1}, - shouldPassAdmit: false, + shouldPassAdmit: true, shouldPassValidate: false, expectMutation: false, expectedPodUser: nil, @@ -405,7 +410,7 @@ func TestAdmitPreferNonmutating(t *testing.T) { "pod should be allowed if completely unchanged on update": { operation: kadmission.Update, pod: unprivilegedRunAsAnyPod.DeepCopy(), - oldPod: unprivilegedRunAsAnyPod.DeepCopy(), + podBeforeUpdate: unprivilegedRunAsAnyPod.DeepCopy(), psps: []*extensions.PodSecurityPolicy{mutating2, mutating1}, shouldPassAdmit: true, shouldPassValidate: true, @@ -416,8 +421,8 @@ func TestAdmitPreferNonmutating(t *testing.T) { }, "pod should be allowed if unchanged on update except finalizers,ownerrefs": { operation: kadmission.Update, - pod: unprivilegedRunAsAnyPod.DeepCopy(), - oldPod: gcChangedPod.DeepCopy(), + pod: gcChangedPod.DeepCopy(), + podBeforeUpdate: unprivilegedRunAsAnyPod.DeepCopy(), psps: []*extensions.PodSecurityPolicy{mutating2, mutating1}, shouldPassAdmit: true, shouldPassValidate: true, @@ -429,7 +434,7 @@ func TestAdmitPreferNonmutating(t *testing.T) { } for k, v := range tests { - testPSPAdmitAdvanced(k, v.operation, v.psps, v.pod, v.oldPod, v.shouldPassAdmit, v.shouldPassValidate, v.expectMutation, v.expectedPSP, t) + testPSPAdmitAdvanced(k, v.operation, v.psps, v.pod, v.podBeforeUpdate, v.shouldPassAdmit, v.shouldPassValidate, v.expectMutation, v.expectedPSP, t) if v.shouldPassAdmit { actualPodUser := (*int64)(nil)