Merge pull request #55486 from sttts/sttts-psp-admission-annotation

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)

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)