Merge pull request #55504 from php-coder/cleanup_create_sc

Automatic merge from submit-queue (batch tested with PRs 55557, 55504, 56269, 55604, 56202). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Create{Container,Pod}SecurityContext: modify a pod and don't return the annotations

**What this PR does / why we need it**:
Prior https://github.com/kubernetes/kubernetes/pull/52849 we couldn't modify a pod and had to return annotations from the methods. But now, as we always working with a copy of a pod, we can modify it directly and we don't need to copy&return annotations separately.

This PR simplifies the code by modifying a pod directly. Also it renames these methods and replaces returning of the `SecurityContext` by in-place modification.

In fact it reverts the changes from https://github.com/kubernetes/kubernetes/pull/30257

**Release note**:
```release-note
NONE
```

PTAL @liggitt @timstclair 
CC @simo5

pkg/security/podsecuritypolicy/BUILD

@@ -27,7 +27,6 @@ go_library(
         "//pkg/security/podsecuritypolicy/user:go_default_library",
         "//pkg/security/podsecuritypolicy/util:go_default_library",
         "//pkg/securitycontext:go_default_library",
-        "//pkg/util/maps:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
     ],

pkg/security/podsecuritypolicy/provider.go

@@ -25,7 +25,6 @@ import (
 	"k8s.io/kubernetes/pkg/apis/extensions"
 	psputil "k8s.io/kubernetes/pkg/security/podsecuritypolicy/util"
 	"k8s.io/kubernetes/pkg/securitycontext"
-	"k8s.io/kubernetes/pkg/util/maps"
 )
 
 // used to pass in the field being validated for reusable group strategies so they
@@ -64,17 +63,16 @@ func NewSimpleProvider(psp *extensions.PodSecurityPolicy, namespace string, stra
 	}, nil
 }
 
-// Create a PodSecurityContext based on the given constraints.  If a setting is already set
+// DefaultPodSecurityContext sets the default values of the required but not filled fields.
-// on the PodSecurityContext it will not be changed.  Validate should be used after the context
+// It modifies the SecurityContext and annotations of the provided pod. Validation should be
-// is created to ensure it complies with the required restrictions.
+// used after the context is defaulted to ensure it complies with the required restrictions.
-func (s *simpleProvider) CreatePodSecurityContext(pod *api.Pod) (*api.PodSecurityContext, map[string]string, error) {
+func (s *simpleProvider) DefaultPodSecurityContext(pod *api.Pod) error {
 	sc := securitycontext.NewPodSecurityContextMutator(pod.Spec.SecurityContext)
-	annotations := maps.CopySS(pod.Annotations)
 
 	if sc.SupplementalGroups() == nil {
 		supGroups, err := s.strategies.SupplementalGroupStrategy.Generate(pod)
 		if err != nil {
-			return nil, nil, err
+			return err
 		}
 		sc.SetSupplementalGroups(supGroups)
 	}
@@ -82,7 +80,7 @@ func (s *simpleProvider) CreatePodSecurityContext(pod *api.Pod) (*api.PodSecurit
 	if sc.FSGroup() == nil {
 		fsGroup, err := s.strategies.FSGroupStrategy.GenerateSingle(pod)
 		if err != nil {
-			return nil, nil, err
+			return err
 		}
 		sc.SetFSGroup(fsGroup)
 	}
@@ -90,41 +88,42 @@ func (s *simpleProvider) CreatePodSecurityContext(pod *api.Pod) (*api.PodSecurit
 	if sc.SELinuxOptions() == nil {
 		seLinux, err := s.strategies.SELinuxStrategy.Generate(pod, nil)
 		if err != nil {
-			return nil, nil, err
+			return err
 		}
 		sc.SetSELinuxOptions(seLinux)
 	}
 
 	// This is only generated on the pod level.  Containers inherit the pod's profile.  If the
 	// container has a specific profile set then it will be caught in the validation step.
-	seccompProfile, err := s.strategies.SeccompStrategy.Generate(annotations, pod)
+	seccompProfile, err := s.strategies.SeccompStrategy.Generate(pod.Annotations, pod)
 	if err != nil {
-		return nil, nil, err
+		return err
 	}
 	if seccompProfile != "" {
-		if annotations == nil {
+		if pod.Annotations == nil {
-			annotations = map[string]string{}
+			pod.Annotations = map[string]string{}
 		}
-		annotations[api.SeccompPodAnnotationKey] = seccompProfile
+		pod.Annotations[api.SeccompPodAnnotationKey] = seccompProfile
-	}
-	return sc.PodSecurityContext(), annotations, nil
 	}
 
-// Create a SecurityContext based on the given constraints.  If a setting is already set on the
+	pod.Spec.SecurityContext = sc.PodSecurityContext()
-// container's security context then it will not be changed.  Validation should be used after
+
-// the context is created to ensure it complies with the required restrictions.
+	return nil
-func (s *simpleProvider) CreateContainerSecurityContext(pod *api.Pod, container *api.Container) (*api.SecurityContext, map[string]string, error) {
+}
+
+// DefaultContainerSecurityContext 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 {
 	sc := securitycontext.NewEffectiveContainerSecurityContextMutator(
 		securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext),
 		securitycontext.NewContainerSecurityContextMutator(container.SecurityContext),
 	)
 
-	annotations := maps.CopySS(pod.Annotations)
-
 	if sc.RunAsUser() == nil {
 		uid, err := s.strategies.RunAsUserStrategy.Generate(pod, container)
 		if err != nil {
-			return nil, nil, err
+			return err
 		}
 		sc.SetRunAsUser(uid)
 	}
@@ -132,14 +131,14 @@ func (s *simpleProvider) CreateContainerSecurityContext(pod *api.Pod, container
 	if sc.SELinuxOptions() == nil {
 		seLinux, err := s.strategies.SELinuxStrategy.Generate(pod, container)
 		if err != nil {
-			return nil, nil, err
+			return err
 		}
 		sc.SetSELinuxOptions(seLinux)
 	}
 
-	annotations, err := s.strategies.AppArmorStrategy.Generate(annotations, container)
+	annotations, err := s.strategies.AppArmorStrategy.Generate(pod.Annotations, container)
 	if err != nil {
-		return nil, nil, err
+		return err
 	}
 
 	// if we're using the non-root strategy set the marker that this container should not be
@@ -152,7 +151,7 @@ func (s *simpleProvider) CreateContainerSecurityContext(pod *api.Pod, container
 
 	caps, err := s.strategies.CapabilitiesStrategy.Generate(pod, container)
 	if err != nil {
-		return nil, nil, err
+		return err
 	}
 	sc.SetCapabilities(caps)
 
@@ -174,7 +173,10 @@ func (s *simpleProvider) CreateContainerSecurityContext(pod *api.Pod, container
 		sc.SetAllowPrivilegeEscalation(&s.psp.Spec.AllowPrivilegeEscalation)
 	}
 
-	return sc.ContainerSecurityContext(), annotations, nil
+	pod.Annotations = annotations
+	container.SecurityContext = sc.ContainerSecurityContext()
+
+	return nil
 }
 
 // Ensure a pod's SecurityContext is in compliance with the given constraints.

pkg/security/podsecuritypolicy/provider_test.go

@@ -38,7 +38,7 @@ import (
 
 const defaultContainerName = "test-c"
 
-func TestCreatePodSecurityContextNonmutating(t *testing.T) {
+func TestDefaultPodSecurityContextNonmutating(t *testing.T) {
 	// Create a pod with a security context that needs filling in
 	createPod := func() *api.Pod {
 		return &api.Pod{
@@ -82,7 +82,7 @@ func TestCreatePodSecurityContextNonmutating(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unable to create provider %v", err)
 	}
-	_, _, err = provider.CreatePodSecurityContext(pod)
+	err = provider.DefaultPodSecurityContext(pod)
 	if err != nil {
 		t.Fatalf("unable to create psc %v", err)
 	}
@@ -91,14 +91,14 @@ func TestCreatePodSecurityContextNonmutating(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 CreatePodSecurityContext. diff:\n%s", diffs)
+		t.Errorf("pod was mutated by DefaultPodSecurityContext. diff:\n%s", diffs)
 	}
 	if !reflect.DeepEqual(createPSP(), psp) {
-		t.Error("psp was mutated by CreatePodSecurityContext")
+		t.Error("psp was mutated by DefaultPodSecurityContext")
 	}
 }
 
-func TestCreateContainerSecurityContextNonmutating(t *testing.T) {
+func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
 	untrue := false
 	tests := []struct {
 		security *api.SecurityContext
@@ -154,7 +154,7 @@ func TestCreateContainerSecurityContextNonmutating(t *testing.T) {
 		if err != nil {
 			t.Fatalf("unable to create provider %v", err)
 		}
-		_, _, err = provider.CreateContainerSecurityContext(pod, &pod.Spec.Containers[0])
+		err = provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[0])
 		if err != nil {
 			t.Fatalf("unable to create container security context %v", err)
 		}
@@ -163,10 +163,10 @@ func TestCreateContainerSecurityContextNonmutating(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 CreateContainerSecurityContext. diff:\n%s", diffs)
+			t.Errorf("pod was mutated by DefaultContainerSecurityContext. diff:\n%s", diffs)
 		}
 		if !reflect.DeepEqual(createPSP(), psp) {
-			t.Error("psp was mutated by CreateContainerSecurityContext")
+			t.Error("psp was mutated by DefaultContainerSecurityContext")
 		}
 	}
 }
@@ -965,12 +965,13 @@ func TestGenerateContainerSecurityContextReadOnlyRootFS(t *testing.T) {
 			t.Errorf("%s unable to create provider %v", k, err)
 			continue
 		}
-		sc, _, err := provider.CreateContainerSecurityContext(v.pod, &v.pod.Spec.Containers[0])
+		err = provider.DefaultContainerSecurityContext(v.pod, &v.pod.Spec.Containers[0])
 		if err != nil {
 			t.Errorf("%s unable to create container security context %v", k, err)
 			continue
 		}
 
+		sc := v.pod.Spec.Containers[0].SecurityContext
 		if v.expected == nil && sc.ReadOnlyRootFilesystem != nil {
 			t.Errorf("%s expected a nil ReadOnlyRootFilesystem but got %t", k, *sc.ReadOnlyRootFilesystem)
 		}

pkg/security/podsecuritypolicy/types.go

@@ -32,12 +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 {
-	// Create a PodSecurityContext based on the given constraints. Also returns an updated set
+	// DefaultPodSecurityContext sets the default values of the required but not filled fields.
-	// of Pod annotations for alpha feature support.
+	// It modifies the SecurityContext and annotations of the provided pod.
-	CreatePodSecurityContext(pod *api.Pod) (*api.PodSecurityContext, map[string]string, error)
+	DefaultPodSecurityContext(pod *api.Pod) error
-	// Create a container SecurityContext based on the given constraints. Also returns an updated set
+	// DefaultContainerSecurityContext sets the default values of the required but not filled fields.
-	// of Pod annotations for alpha feature support.
+	// It modifies the SecurityContext of the container and annotations of the pod.
-	CreateContainerSecurityContext(pod *api.Pod, container *api.Container) (*api.SecurityContext, map[string]string, error)
+	DefaultContainerSecurityContext(pod *api.Pod, container *api.Container) error
 	// Ensure a pod's SecurityContext is in compliance with the given constraints.
 	ValidatePodSecurityContext(pod *api.Pod, fldPath *field.Path) field.ErrorList
 	// Ensure a container's SecurityContext is in compliance with the given constraints

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

@@ -287,35 +287,28 @@ func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes,
 func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.Path) field.ErrorList {
 	errs := field.ErrorList{}
 
-	psc, pscAnnotations, err := provider.CreatePodSecurityContext(pod)
+	err := provider.DefaultPodSecurityContext(pod)
 	if err != nil {
 		errs = append(errs, field.Invalid(field.NewPath("spec", "securityContext"), pod.Spec.SecurityContext, err.Error()))
 	}
-	pod.Spec.SecurityContext = psc
-	pod.Annotations = pscAnnotations
 
 	errs = append(errs, provider.ValidatePodSecurityContext(pod, field.NewPath("spec", "securityContext"))...)
 
 	for i := range pod.Spec.InitContainers {
-		sc, scAnnotations, err := provider.CreateContainerSecurityContext(pod, &pod.Spec.InitContainers[i])
+		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
 		}
-		pod.Spec.InitContainers[i].SecurityContext = sc
-		pod.Annotations = scAnnotations
 		errs = append(errs, provider.ValidateContainerSecurityContext(pod, &pod.Spec.InitContainers[i], field.NewPath("spec", "initContainers").Index(i).Child("securityContext"))...)
 	}
 
 	for i := range pod.Spec.Containers {
-		sc, scAnnotations, err := provider.CreateContainerSecurityContext(pod, &pod.Spec.Containers[i])
+		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
 		}
-
-		pod.Spec.Containers[i].SecurityContext = sc
-		pod.Annotations = scAnnotations
 		errs = append(errs, provider.ValidateContainerSecurityContext(pod, &pod.Spec.Containers[i], field.NewPath("spec", "containers").Index(i).Child("securityContext"))...)
 	}