Merge pull request #72351 from ceshihao/featuregate_RunAsGroup

RunAsGroup - Update DropDisabled[Alpha]Fields behaviour

pkg/api/pod/util.go

@@ -279,7 +279,7 @@ func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) {
 // dropDisabledRunAsGroupField removes disabled fields from PodSpec related
 // to RunAsGroup
 func dropDisabledRunAsGroupField(podSpec, oldPodSpec *api.PodSpec) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) && !runAsGroupInUse(oldPodSpec) {
 		if podSpec.SecurityContext != nil {
 			podSpec.SecurityContext.RunAsGroup = nil
 		}
@@ -293,22 +293,6 @@ func dropDisabledRunAsGroupField(podSpec, oldPodSpec *api.PodSpec) {
 				podSpec.InitContainers[i].SecurityContext.RunAsGroup = nil
 			}
 		}
-
-		if oldPodSpec != nil {
-			if oldPodSpec.SecurityContext != nil {
-				oldPodSpec.SecurityContext.RunAsGroup = nil
-			}
-			for i := range oldPodSpec.Containers {
-				if oldPodSpec.Containers[i].SecurityContext != nil {
-					oldPodSpec.Containers[i].SecurityContext.RunAsGroup = nil
-				}
-			}
-			for i := range oldPodSpec.InitContainers {
-				if oldPodSpec.InitContainers[i].SecurityContext != nil {
-					oldPodSpec.InitContainers[i].SecurityContext.RunAsGroup = nil
-				}
-			}
-		}
 	}
 }
 
@@ -445,3 +429,25 @@ func volumeDevicesInUse(podSpec *api.PodSpec) bool {
 	}
 	return false
 }
+
+// runAsGroupInUse returns true if the pod spec is non-nil and has a SecurityContext's RunAsGroup field set
+func runAsGroupInUse(podSpec *api.PodSpec) bool {
+	if podSpec == nil {
+		return false
+	}
+
+	if podSpec.SecurityContext != nil && podSpec.SecurityContext.RunAsGroup != nil {
+		return true
+	}
+	for i := range podSpec.Containers {
+		if podSpec.Containers[i].SecurityContext != nil && podSpec.Containers[i].SecurityContext.RunAsGroup != nil {
+			return true
+		}
+	}
+	for i := range podSpec.InitContainers {
+		if podSpec.InitContainers[i].SecurityContext != nil && podSpec.InitContainers[i].SecurityContext.RunAsGroup != nil {
+			return true
+		}
+	}
+	return false
+}

pkg/api/pod/util_test.go

@@ -911,3 +911,140 @@ func TestDropEmptyDirSizeLimit(t *testing.T) {
 		}
 	}
 }
+
+func TestDropRunAsGroup(t *testing.T) {
+	group := func() *int64 {
+		testGroup := int64(1000)
+		return &testGroup
+	}
+	defaultProcMount := api.DefaultProcMount
+	defaultSecurityContext := func() *api.SecurityContext {
+		return &api.SecurityContext{ProcMount: &defaultProcMount}
+	}
+	securityContextWithRunAsGroup := func() *api.SecurityContext {
+		return &api.SecurityContext{ProcMount: &defaultProcMount, RunAsGroup: group()}
+	}
+	podWithoutRunAsGroup := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				RestartPolicy:   api.RestartPolicyNever,
+				SecurityContext: &api.PodSecurityContext{},
+				Containers:      []api.Container{{Name: "container1", Image: "testimage", SecurityContext: defaultSecurityContext()}},
+				InitContainers:  []api.Container{{Name: "initContainer1", Image: "testimage", SecurityContext: defaultSecurityContext()}},
+			},
+		}
+	}
+	podWithRunAsGroupInPod := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				RestartPolicy:   api.RestartPolicyNever,
+				SecurityContext: &api.PodSecurityContext{RunAsGroup: group()},
+				Containers:      []api.Container{{Name: "container1", Image: "testimage", SecurityContext: defaultSecurityContext()}},
+				InitContainers:  []api.Container{{Name: "initContainer1", Image: "testimage", SecurityContext: defaultSecurityContext()}},
+			},
+		}
+	}
+	podWithRunAsGroupInContainers := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				RestartPolicy:   api.RestartPolicyNever,
+				SecurityContext: &api.PodSecurityContext{},
+				Containers:      []api.Container{{Name: "container1", Image: "testimage", SecurityContext: securityContextWithRunAsGroup()}},
+				InitContainers:  []api.Container{{Name: "initContainer1", Image: "testimage", SecurityContext: defaultSecurityContext()}},
+			},
+		}
+	}
+	podWithRunAsGroupInInitContainers := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				RestartPolicy:   api.RestartPolicyNever,
+				SecurityContext: &api.PodSecurityContext{},
+				Containers:      []api.Container{{Name: "container1", Image: "testimage", SecurityContext: defaultSecurityContext()}},
+				InitContainers:  []api.Container{{Name: "initContainer1", Image: "testimage", SecurityContext: securityContextWithRunAsGroup()}},
+			},
+		}
+	}
+
+	podInfo := []struct {
+		description   string
+		hasRunAsGroup bool
+		pod           func() *api.Pod
+	}{
+		{
+			description:   "have RunAsGroup in Pod",
+			hasRunAsGroup: true,
+			pod:           podWithRunAsGroupInPod,
+		},
+		{
+			description:   "have RunAsGroup in Container",
+			hasRunAsGroup: true,
+			pod:           podWithRunAsGroupInContainers,
+		},
+		{
+			description:   "have RunAsGroup in InitContainer",
+			hasRunAsGroup: true,
+			pod:           podWithRunAsGroupInInitContainers,
+		},
+		{
+			description:   "does not have RunAsGroup",
+			hasRunAsGroup: false,
+			pod:           podWithoutRunAsGroup,
+		},
+		{
+			description:   "is nil",
+			hasRunAsGroup: false,
+			pod:           func() *api.Pod { return nil },
+		},
+	}
+
+	for _, enabled := range []bool{true, false} {
+		for _, oldPodInfo := range podInfo {
+			for _, newPodInfo := range podInfo {
+				oldPodHasRunAsGroup, oldPod := oldPodInfo.hasRunAsGroup, oldPodInfo.pod()
+				newPodHasRunAsGroup, newPod := newPodInfo.hasRunAsGroup, 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.RunAsGroup, 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 || oldPodHasRunAsGroup:
+						// new pod should not be changed if the feature is enabled, or if the old pod had RunAsGroup
+						if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
+						}
+					case newPodHasRunAsGroup:
+						// new pod should be changed
+						if reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("%v", oldPod)
+							t.Errorf("%v", newPod)
+							t.Errorf("new pod was not changed")
+						}
+						// new pod should not have RunAsGroup
+						if !reflect.DeepEqual(newPod, podWithoutRunAsGroup()) {
+							t.Errorf("new pod had RunAsGroup: %v", diff.ObjectReflectDiff(newPod, podWithoutRunAsGroup()))
+						}
+					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/api/podsecuritypolicy/util.go

@@ -28,11 +28,8 @@ func DropDisabledFields(pspSpec, oldPSPSpec *policy.PodSecurityPolicySpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) && !allowedProcMountTypesInUse(oldPSPSpec) {
 		pspSpec.AllowedProcMountTypes = nil
 	}
-	if !utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) && (oldPSPSpec == nil || oldPSPSpec.RunAsGroup == nil) {
 		pspSpec.RunAsGroup = nil
-		if oldPSPSpec != nil {
-			oldPSPSpec.RunAsGroup = nil
-		}
 	}
 }
 

pkg/api/podsecuritypolicy/util_test.go

@@ -107,3 +107,83 @@ func TestDropAllowedProcMountTypes(t *testing.T) {
 		}
 	}
 }
+
+func TestDropRunAsGroup(t *testing.T) {
+	group := func() *policy.RunAsGroupStrategyOptions {
+		return &policy.RunAsGroupStrategyOptions{}
+	}
+	scWithoutRunAsGroup := func() *policy.PodSecurityPolicySpec {
+		return &policy.PodSecurityPolicySpec{}
+	}
+	scWithRunAsGroup := func() *policy.PodSecurityPolicySpec {
+		return &policy.PodSecurityPolicySpec{
+			RunAsGroup: group(),
+		}
+	}
+	scInfo := []struct {
+		description   string
+		hasRunAsGroup bool
+		sc            func() *policy.PodSecurityPolicySpec
+	}{
+		{
+			description:   "PodSecurityPolicySpec Without RunAsGroup",
+			hasRunAsGroup: false,
+			sc:            scWithoutRunAsGroup,
+		},
+		{
+			description:   "PodSecurityPolicySpec With RunAsGroup",
+			hasRunAsGroup: true,
+			sc:            scWithRunAsGroup,
+		},
+		{
+			description:   "is nil",
+			hasRunAsGroup: false,
+			sc:            func() *policy.PodSecurityPolicySpec { return nil },
+		},
+	}
+
+	for _, enabled := range []bool{true, false} {
+		for _, oldPSPSpecInfo := range scInfo {
+			for _, newPSPSpecInfo := range scInfo {
+				oldPSPSpecHasRunAsGroup, oldPSPSpec := oldPSPSpecInfo.hasRunAsGroup, oldPSPSpecInfo.sc()
+				newPSPSpecHasRunAsGroup, newPSPSpec := newPSPSpecInfo.hasRunAsGroup, newPSPSpecInfo.sc()
+				if newPSPSpec == nil {
+					continue
+				}
+
+				t.Run(fmt.Sprintf("feature enabled=%v, old PodSecurityPolicySpec %v, new PodSecurityPolicySpec %v", enabled, oldPSPSpecInfo.description, newPSPSpecInfo.description), func(t *testing.T) {
+					defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RunAsGroup, enabled)()
+
+					DropDisabledFields(newPSPSpec, oldPSPSpec)
+
+					// old PodSecurityPolicySpec should never be changed
+					if !reflect.DeepEqual(oldPSPSpec, oldPSPSpecInfo.sc()) {
+						t.Errorf("old PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(oldPSPSpec, oldPSPSpecInfo.sc()))
+					}
+
+					switch {
+					case enabled || oldPSPSpecHasRunAsGroup:
+						// new PodSecurityPolicySpec should not be changed if the feature is enabled, or if the old PodSecurityPolicySpec had RunAsGroup
+						if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.sc()) {
+							t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.sc()))
+						}
+					case newPSPSpecHasRunAsGroup:
+						// new PodSecurityPolicySpec should be changed
+						if reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.sc()) {
+							t.Errorf("new PodSecurityPolicySpec was not changed")
+						}
+						// new PodSecurityPolicySpec should not have RunAsGroup
+						if !reflect.DeepEqual(newPSPSpec, scWithoutRunAsGroup()) {
+							t.Errorf("new PodSecurityPolicySpec had RunAsGroup: %v", diff.ObjectReflectDiff(newPSPSpec, scWithoutRunAsGroup()))
+						}
+					default:
+						// new PodSecurityPolicySpec should not need to be changed
+						if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.sc()) {
+							t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.sc()))
+						}
+					}
+				})
+			}
+		}
+	}
+}