ProcMount validation and testing

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

pkg/api/pod/util.go

@@ -322,31 +322,18 @@ func dropDisabledRunAsGroupField(podSpec, oldPodSpec *api.PodSpec) {
 }
 
 // dropDisabledProcMountField removes disabled fields from PodSpec related
-// to ProcMount
+// to ProcMount only if it is not already used by the old spec
 func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) && !procMountInUse(oldPodSpec) {
-		defProcMount := api.DefaultProcMount
+		defaultProcMount := api.DefaultProcMount
 		for i := range podSpec.Containers {
 			if podSpec.Containers[i].SecurityContext != nil {
-				podSpec.Containers[i].SecurityContext.ProcMount = &defProcMount
+				podSpec.Containers[i].SecurityContext.ProcMount = &defaultProcMount
 			}
 		}
 		for i := range podSpec.InitContainers {
 			if podSpec.InitContainers[i].SecurityContext != nil {
-				podSpec.InitContainers[i].SecurityContext.ProcMount = &defProcMount
+				podSpec.InitContainers[i].SecurityContext.ProcMount = &defaultProcMount
-			}
-		}
-
-		if oldPodSpec != nil {
-			for i := range oldPodSpec.Containers {
-				if oldPodSpec.Containers[i].SecurityContext != nil {
-					oldPodSpec.Containers[i].SecurityContext.ProcMount = &defProcMount
-				}
-			}
-			for i := range oldPodSpec.InitContainers {
-				if oldPodSpec.InitContainers[i].SecurityContext != nil {
-					oldPodSpec.InitContainers[i].SecurityContext.ProcMount = &defProcMount
-				}
 			}
 		}
 	}
@@ -406,3 +393,29 @@ func runtimeClassInUse(podSpec *api.PodSpec) bool {
 	}
 	return false
 }
+
+// procMountInUse returns true if the pod spec is non-nil and has a SecurityContext's ProcMount field set
+func procMountInUse(podSpec *api.PodSpec) bool {
+	if podSpec == nil {
+		return false
+	}
+	for i := range podSpec.Containers {
+		if podSpec.Containers[i].SecurityContext != nil {
+			if podSpec.Containers[i].SecurityContext.ProcMount != nil {
+				if *podSpec.Containers[i].SecurityContext.ProcMount != api.DefaultProcMount {
+					return true
+				}
+			}
+		}
+	}
+	for i := range podSpec.InitContainers {
+		if podSpec.InitContainers[i].SecurityContext != nil {
+			if podSpec.InitContainers[i].SecurityContext.ProcMount != nil {
+				if *podSpec.InitContainers[i].SecurityContext.ProcMount != api.DefaultProcMount {
+					return true
+				}
+			}
+		}
+	}
+	return false
+}

pkg/api/pod/util_test.go

@@ -518,3 +518,97 @@ func TestDropRuntimeClass(t *testing.T) {
 		}
 	}
 }
+
+func TestDropProcMount(t *testing.T) {
+	procMount := api.UnmaskedProcMount
+	defaultProcMount := api.DefaultProcMount
+	podWithProcMount := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				RestartPolicy:  api.RestartPolicyNever,
+				Containers:     []api.Container{{Name: "container1", Image: "testimage", SecurityContext: &api.SecurityContext{ProcMount: &procMount}}},
+				InitContainers: []api.Container{{Name: "container1", Image: "testimage", SecurityContext: &api.SecurityContext{ProcMount: &procMount}}},
+			},
+		}
+	}
+	podWithoutProcMount := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				RestartPolicy:  api.RestartPolicyNever,
+				Containers:     []api.Container{{Name: "container1", Image: "testimage", SecurityContext: &api.SecurityContext{ProcMount: &defaultProcMount}}},
+				InitContainers: []api.Container{{Name: "container1", Image: "testimage", SecurityContext: &api.SecurityContext{ProcMount: &defaultProcMount}}},
+			},
+		}
+	}
+
+	podInfo := []struct {
+		description  string
+		hasProcMount bool
+		pod          func() *api.Pod
+	}{
+		{
+			description:  "has ProcMount",
+			hasProcMount: true,
+			pod:          podWithProcMount,
+		},
+		{
+			description:  "does not have ProcMount",
+			hasProcMount: false,
+			pod:          podWithoutProcMount,
+		},
+		{
+			description:  "is nil",
+			hasProcMount: false,
+			pod:          func() *api.Pod { return nil },
+		},
+	}
+
+	for _, enabled := range []bool{true, false} {
+		for _, oldPodInfo := range podInfo {
+			for _, newPodInfo := range podInfo {
+				oldPodHasProcMount, oldPod := oldPodInfo.hasProcMount, oldPodInfo.pod()
+				newPodHasProcMount, newPod := newPodInfo.hasProcMount, newPodInfo.pod()
+				if newPod == nil {
+					continue
+				}
+
+				t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
+					defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProcMountType, enabled)()
+
+					var oldPodSpec *api.PodSpec
+					if oldPod != nil {
+						oldPodSpec = &oldPod.Spec
+					}
+					DropDisabledFields(&newPod.Spec, oldPodSpec)
+
+					// old pod should never be changed
+					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
+						t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod()))
+					}
+
+					switch {
+					case enabled || oldPodHasProcMount:
+						// new pod should not be changed if the feature is enabled, or if the old pod had ProcMount
+						if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
+						}
+					case newPodHasProcMount:
+						// new pod should be changed
+						if reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod was not changed")
+						}
+						// new pod should not have ProcMount
+						if !reflect.DeepEqual(newPod, podWithoutProcMount()) {
+							t.Errorf("new pod had ProcMount: %v", diff.ObjectReflectDiff(newPod, podWithoutProcMount()))
+						}
+					default:
+						// new pod should not need to be changed
+						if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
+						}
+					}
+				})
+			}
+		}
+	}
+}

pkg/apis/core/fuzzer/fuzzer.go

@@ -354,6 +354,10 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} {
 				c.Fuzz(&sc.Capabilities.Add)
 				c.Fuzz(&sc.Capabilities.Drop)
 			}
+			if sc.ProcMount == nil {
+				defProcMount := core.DefaultProcMount
+				sc.ProcMount = &defProcMount
+			}
 		},
 		func(s *core.Secret, c fuzz.Continue) {
 			c.FuzzNoCustom(s) // fuzz self without calling this function again

pkg/apis/core/v1/defaults.go

@@ -423,3 +423,10 @@ func SetDefaults_HostPathVolumeSource(obj *v1.HostPathVolumeSource) {
 		obj.Type = &typeVol
 	}
 }
+
+func SetDefaults_SecurityContext(obj *v1.SecurityContext) {
+	if obj.ProcMount == nil {
+		defProcMount := v1.DefaultProcMount
+		obj.ProcMount = &defProcMount
+	}
+}

pkg/apis/core/validation/validation.go

@@ -3468,7 +3468,6 @@ func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec *
 				allErrs = append(allErrs, field.Invalid(fldPath.Child("runAsGroup"), *(securityContext.RunAsGroup), msg))
 			}
 		}
-
 		for g, gid := range securityContext.SupplementalGroups {
 			for _, msg := range validation.IsValidGroupID(gid) {
 				allErrs = append(allErrs, field.Invalid(fldPath.Child("supplementalGroups").Index(g), gid, msg))
@@ -5272,6 +5271,12 @@ func ValidateSecurityContext(sc *core.SecurityContext, fldPath *field.Path) fiel
 		}
 	}
 
+	if sc.ProcMount != nil {
+		if err := IsValidProcMount(*sc.ProcMount); err != nil {
+			allErrs = append(allErrs, field.NotSupported(fldPath.Child("procMount"), *sc.ProcMount, []string{string(core.DefaultProcMount), string(core.UnmaskedProcMount)}))
+		}
+	}
+
 	if sc.AllowPrivilegeEscalation != nil && !*sc.AllowPrivilegeEscalation {
 		if sc.Privileged != nil && *sc.Privileged {
 			allErrs = append(allErrs, field.Invalid(fldPath, sc, "cannot set `allowPrivilegeEscalation` to false and `privileged` to true"))
@@ -5372,3 +5377,14 @@ func IsDecremented(update, old *int32) bool {
 	}
 	return *update < *old
 }
+
+// IsValidProcMount tests that the argument is a valid ProcMountType.
+func IsValidProcMount(procMountType core.ProcMountType) error {
+	switch procMountType {
+	case core.DefaultProcMount:
+	case core.UnmaskedProcMount:
+	default:
+		return fmt.Errorf("unsupported ProcMount type %s", procMountType)
+	}
+	return nil
+}

pkg/securitycontext/fake.go

@@ -25,9 +25,11 @@ import (
 // empty container defaults.  Used for testing.
 func ValidSecurityContextWithContainerDefaults() *v1.SecurityContext {
 	priv := false
+	defProcMount := v1.DefaultProcMount
 	return &v1.SecurityContext{
 		Capabilities: &v1.Capabilities{},
 		Privileged:   &priv,
+		ProcMount:    &defProcMount,
 	}
 }