Refactor PSP provider

pkg/security/podsecuritypolicy/provider.go

@@ -59,10 +59,10 @@ func NewSimpleProvider(psp *policy.PodSecurityPolicy, namespace string, strategy
 	}, nil
 }
 
-// DefaultPodSecurityContext sets the default values of the required but not filled fields.
+// MutatePod sets the default values of the required but not filled fields.
-// It modifies the SecurityContext and annotations of the provided pod. Validation should be
+// Validation should be used after the context is defaulted to ensure it
-// used after the context is defaulted to ensure it complies with the required restrictions.
+// complies with the required restrictions.
-func (s *simpleProvider) DefaultPodSecurityContext(pod *api.Pod) error {
+func (s *simpleProvider) MutatePod(pod *api.Pod) error {
 	sc := securitycontext.NewPodSecurityContextMutator(pod.Spec.SecurityContext)
 
 	if sc.SupplementalGroups() == nil {
@@ -104,13 +104,25 @@ func (s *simpleProvider) DefaultPodSecurityContext(pod *api.Pod) error {
 
 	pod.Spec.SecurityContext = sc.PodSecurityContext()
 
+	for i := range pod.Spec.InitContainers {
+		if err := s.mutateContainer(pod, &pod.Spec.InitContainers[i]); err != nil {
+			return err
+		}
+	}
+
+	for i := range pod.Spec.Containers {
+		if err := s.mutateContainer(pod, &pod.Spec.Containers[i]); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
-// DefaultContainerSecurityContext sets the default values of the required but not filled fields.
+// mutateContainer sets the default values of the required but not filled fields.
 // It modifies the SecurityContext of the container and annotations of the pod. Validation should
 // be used after the context is defaulted to ensure it complies with the required restrictions.
-func (s *simpleProvider) DefaultContainerSecurityContext(pod *api.Pod, container *api.Container) error {
+func (s *simpleProvider) mutateContainer(pod *api.Pod, container *api.Container) error {
 	sc := securitycontext.NewEffectiveContainerSecurityContextMutator(
 		securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext),
 		securitycontext.NewContainerSecurityContextMutator(container.SecurityContext),
@@ -282,11 +294,22 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList {
 			}
 		}
 	}
+
+	fldPath := field.NewPath("spec", "initContainers")
+	for i := range pod.Spec.InitContainers {
+		allErrs = append(allErrs, s.validateContainer(pod, &pod.Spec.InitContainers[i], fldPath.Index(i))...)
+	}
+
+	fldPath = field.NewPath("spec", "containers")
+	for i := range pod.Spec.Containers {
+		allErrs = append(allErrs, s.validateContainer(pod, &pod.Spec.Containers[i], fldPath.Index(i))...)
+	}
+
 	return allErrs
 }
 
 // Ensure a container's SecurityContext is in compliance with the given constraints
-func (s *simpleProvider) ValidateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList {
+func (s *simpleProvider) validateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	podSC := securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext)

pkg/security/podsecuritypolicy/provider_test.go

@@ -31,7 +31,6 @@ import (
 	policy "k8s.io/api/policy/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
-	"k8s.io/apimachinery/pkg/util/validation/field"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	k8s_api_v1 "k8s.io/kubernetes/pkg/apis/core/v1"
 	"k8s.io/kubernetes/pkg/security/apparmor"
@@ -41,7 +40,7 @@ import (
 
 const defaultContainerName = "test-c"
 
-func TestDefaultPodSecurityContextNonmutating(t *testing.T) {
+func TestMutatePodNonmutating(t *testing.T) {
 	// Create a pod with a security context that needs filling in
 	createPod := func() *api.Pod {
 		return &api.Pod{
@@ -89,7 +88,7 @@ func TestDefaultPodSecurityContextNonmutating(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unable to create provider %v", err)
 	}
-	err = provider.DefaultPodSecurityContext(pod)
+	err = provider.MutatePod(pod)
 	if err != nil {
 		t.Fatalf("unable to create psc %v", err)
 	}
@@ -98,14 +97,14 @@ func TestDefaultPodSecurityContextNonmutating(t *testing.T) {
 	// since all the strategies were permissive
 	if !reflect.DeepEqual(createPod(), pod) {
 		diffs := diff.ObjectDiff(createPod(), pod)
-		t.Errorf("pod was mutated by DefaultPodSecurityContext. diff:\n%s", diffs)
+		t.Errorf("pod was mutated by MutatePod. diff:\n%s", diffs)
 	}
 	if !reflect.DeepEqual(createPSP(), psp) {
-		t.Error("psp was mutated by DefaultPodSecurityContext")
+		t.Error("psp was mutated by MutatePod")
 	}
 }
 
-func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
+func TestMutateContainerNonmutating(t *testing.T) {
 	untrue := false
 	tests := []struct {
 		security *api.SecurityContext
@@ -134,7 +133,6 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
 					Name: "psp-sa",
 					Annotations: map[string]string{
 						seccomp.AllowedProfilesAnnotationKey: "*",
-						seccomp.DefaultProfileAnnotationKey:  "foo",
 					},
 				},
 				Spec: policy.PodSecurityPolicySpec{
@@ -165,7 +163,7 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
 		if err != nil {
 			t.Fatalf("unable to create provider %v", err)
 		}
-		err = provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[0])
+		err = provider.MutatePod(pod)
 		if err != nil {
 			t.Fatalf("unable to create container security context %v", err)
 		}
@@ -174,15 +172,15 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
 		// since all the strategies were permissive
 		if !reflect.DeepEqual(createPod(), pod) {
 			diffs := diff.ObjectDiff(createPod(), pod)
-			t.Errorf("pod was mutated by DefaultContainerSecurityContext. diff:\n%s", diffs)
+			t.Errorf("pod was mutated. diff:\n%s", diffs)
 		}
 		if !reflect.DeepEqual(createPSP(), psp) {
-			t.Error("psp was mutated by DefaultContainerSecurityContext")
+			t.Error("psp was mutated")
 		}
 	}
 }
 
-func TestValidatePodSecurityContextFailures(t *testing.T) {
+func TestValidatePodFailures(t *testing.T) {
 	failHostNetworkPod := defaultPod()
 	failHostNetworkPod.Spec.SecurityContext.HostNetwork = true
 
@@ -504,6 +502,7 @@ func TestValidateContainerFailures(t *testing.T) {
 		},
 	}
 	failSELinuxPod := defaultPod()
+	failSELinuxPod.Spec.SecurityContext.SELinuxOptions = &api.SELinuxOptions{Level: "foo"}
 	failSELinuxPod.Spec.Containers[0].SecurityContext.SELinuxOptions = &api.SELinuxOptions{
 		Level: "bar",
 	}
@@ -620,22 +619,24 @@ func TestValidateContainerFailures(t *testing.T) {
 	}
 
 	for k, v := range errorCases {
-		provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory())
+		t.Run(k, func(t *testing.T) {
-		if err != nil {
+			provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory())
-			t.Fatalf("unable to create provider %v", err)
+			if err != nil {
-		}
+				t.Fatalf("unable to create provider %v", err)
-		errs := provider.ValidateContainer(v.pod, &v.pod.Spec.Containers[0], field.NewPath(""))
+			}
-		if len(errs) == 0 {
+			errs := provider.ValidatePod(v.pod)
-			t.Errorf("%s expected validation failure but did not receive errors", k)
+			if len(errs) == 0 {
-			continue
+				t.Errorf("expected validation failure but did not receive errors")
-		}
+				return
-		if !strings.Contains(errs[0].Error(), v.expectedError) {
+			}
-			t.Errorf("%s received unexpected error %v\nexpected: %s", k, errs, v.expectedError)
+			if !strings.Contains(errs[0].Error(), v.expectedError) {
-		}
+				t.Errorf("unexpected error %v\nexpected: %s", errs, v.expectedError)
+			}
+		})
 	}
 }
 
-func TestValidatePodSecurityContextSuccess(t *testing.T) {
+func TestValidatePodSuccess(t *testing.T) {
 	hostNetworkPSP := defaultPSP()
 	hostNetworkPSP.Spec.HostNetwork = true
 	hostNetworkPod := defaultPod()
@@ -941,6 +942,7 @@ func TestValidateContainerSuccess(t *testing.T) {
 		},
 	}
 	seLinuxPod := defaultPod()
+	seLinuxPod.Spec.SecurityContext.SELinuxOptions = &api.SELinuxOptions{Level: "foo"}
 	seLinuxPod.Spec.Containers[0].SecurityContext.SELinuxOptions = &api.SELinuxOptions{
 		Level: "foo",
 	}
@@ -1007,6 +1009,7 @@ func TestValidateContainerSuccess(t *testing.T) {
 
 	seccompPod := defaultPod()
 	seccompPod.Annotations = map[string]string{
+		api.SeccompPodAnnotationKey: "foo",
 		api.SeccompContainerAnnotationKeyPrefix + seccompPod.Spec.Containers[0].Name: "foo",
 	}
 
@@ -1074,15 +1077,16 @@ func TestValidateContainerSuccess(t *testing.T) {
 	}
 
 	for k, v := range successCases {
-		provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory())
+		t.Run(k, func(t *testing.T) {
-		if err != nil {
+			provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory())
-			t.Fatalf("unable to create provider %v", err)
+			if err != nil {
-		}
+				t.Fatalf("unable to create provider %v", err)
-		errs := provider.ValidateContainer(v.pod, &v.pod.Spec.Containers[0], field.NewPath(""))
+			}
-		if len(errs) != 0 {
+			errs := provider.ValidatePod(v.pod)
-			t.Errorf("%s expected validation pass but received errors %v\n%s", k, errs, spew.Sdump(v.pod.ObjectMeta))
+			if len(errs) != 0 {
-			continue
+				t.Errorf("%s expected validation pass but received errors %v\n%s", k, errs, spew.Sdump(v.pod.ObjectMeta))
-		}
+			}
+		})
 	}
 }
 
@@ -1146,7 +1150,7 @@ func TestGenerateContainerSecurityContextReadOnlyRootFS(t *testing.T) {
 			t.Errorf("%s unable to create provider %v", k, err)
 			continue
 		}
-		err = provider.DefaultContainerSecurityContext(v.pod, &v.pod.Spec.Containers[0])
+		err = provider.MutatePod(v.pod)
 		if err != nil {
 			t.Errorf("%s unable to create container security context %v", k, err)
 			continue
@@ -1351,10 +1355,10 @@ func TestAllowPrivilegeEscalation(t *testing.T) {
 			provider, err := NewSimpleProvider(psp, "namespace", NewSimpleStrategyFactory())
 			require.NoError(t, err)
 
-			err = provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[0])
+			err = provider.MutatePod(pod)
 			require.NoError(t, err)
 
-			errs := provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath(""))
+			errs := provider.ValidatePod(pod)
 			if test.expectErr {
 				assert.NotEmpty(t, errs, "expected validation error")
 			} else {

pkg/security/podsecuritypolicy/types.go

@@ -32,16 +32,12 @@ import (
 // Provider provides the implementation to generate a new security
 // context based on constraints or validate an existing security context against constraints.
 type Provider interface {
-	// DefaultPodSecurityContext sets the default values of the required but not filled fields.
+	// MutatePod sets the default values of the required but not filled fields of the pod and all
-	// It modifies the SecurityContext and annotations of the provided pod.
+	// containers in the pod.
-	DefaultPodSecurityContext(pod *api.Pod) error
+	MutatePod(pod *api.Pod) error
-	// DefaultContainerSecurityContext sets the default values of the required but not filled fields.
+	// ValidatePod ensures a pod and all its containers are in compliance with the given constraints.
-	// It modifies the SecurityContext of the container and annotations of the pod.
+	// ValidatePod MUST NOT mutate the pod.
-	DefaultContainerSecurityContext(pod *api.Pod, container *api.Container) error
-	// Ensure a pod is in compliance with the given constraints.
 	ValidatePod(pod *api.Pod) field.ErrorList
-	// Ensure a container's SecurityContext is in compliance with the given constraints.
-	ValidateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList
 	// Get the name of the PSP that this provider was initialized with.
 	GetPSPName() string
 }

plugin/pkg/admission/security/podsecuritypolicy/admission.go

@@ -306,35 +306,14 @@ func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes,
 func assignSecurityContext(provider psp.Provider, pod *api.Pod) field.ErrorList {
 	errs := field.ErrorList{}
 
-	err := provider.DefaultPodSecurityContext(pod)
+	if err := provider.MutatePod(pod); err != nil {
-	if err != nil {
+		// TODO(tallclair): MutatePod should return a field.ErrorList
-		errs = append(errs, field.Invalid(field.NewPath("spec", "securityContext"), pod.Spec.SecurityContext, err.Error()))
+		errs = append(errs, field.Invalid(field.NewPath(""), pod, err.Error()))
 	}
 
 	errs = append(errs, provider.ValidatePod(pod)...)
 
-	for i := range pod.Spec.InitContainers {
+	return errs
-		err := provider.DefaultContainerSecurityContext(pod, &pod.Spec.InitContainers[i])
-		if err != nil {
-			errs = append(errs, field.Invalid(field.NewPath("spec", "initContainers").Index(i).Child("securityContext"), "", err.Error()))
-			continue
-		}
-		errs = append(errs, provider.ValidateContainer(pod, &pod.Spec.InitContainers[i], field.NewPath("spec", "initContainers").Index(i))...)
-	}
-
-	for i := range pod.Spec.Containers {
-		err := provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[i])
-		if err != nil {
-			errs = append(errs, field.Invalid(field.NewPath("spec", "containers").Index(i).Child("securityContext"), "", err.Error()))
-			continue
-		}
-		errs = append(errs, provider.ValidateContainer(pod, &pod.Spec.Containers[i], field.NewPath("spec", "containers").Index(i))...)
-	}
-
-	if len(errs) > 0 {
-		return errs
-	}
-	return nil
 }
 
 // createProvidersFromPolicies creates providers from the constraints supplied.