Remove Sysctls feature gate from validation

pkg/api/pod/util.go

@@ -310,6 +310,12 @@ func dropDisabledFields(
 		podSpec.ReadinessGates = nil
 	}
 
+	if !utilfeature.DefaultFeatureGate.Enabled(features.Sysctls) && !sysctlsInUse(oldPodSpec) {
+		if podSpec.SecurityContext != nil {
+			podSpec.SecurityContext.Sysctls = nil
+		}
+	}
+
 	if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) && !emptyDirSizeLimitInUse(oldPodSpec) {
 		for i := range podSpec.Volumes {
 			if podSpec.Volumes[i].EmptyDir != nil {
@@ -496,6 +502,16 @@ func podReadinessGatesInUse(podSpec *api.PodSpec) bool {
 	return false
 }
 
+func sysctlsInUse(podSpec *api.PodSpec) bool {
+	if podSpec == nil {
+		return false
+	}
+	if podSpec.SecurityContext != nil && podSpec.SecurityContext.Sysctls != nil {
+		return true
+	}
+	return false
+}
+
 // emptyDirSizeLimitInUse returns true if any pod's EptyDir volumes use SizeLimit.
 func emptyDirSizeLimitInUse(podSpec *api.PodSpec) bool {
 	if podSpec == nil {

pkg/api/pod/util_test.go

@@ -1327,3 +1327,103 @@ func TestDropRunAsGroup(t *testing.T) {
 		}
 	}
 }
+
+func TestDropPodSysctls(t *testing.T) {
+	podWithSysctls := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				SecurityContext: &api.PodSecurityContext{
+					Sysctls: []api.Sysctl{{Name: "test", Value: "value"}},
+				},
+			},
+		}
+	}
+	podWithoutSysctls := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{
+				SecurityContext: &api.PodSecurityContext{},
+			},
+		}
+	}
+	podWithoutSecurityContext := func() *api.Pod {
+		return &api.Pod{
+			Spec: api.PodSpec{},
+		}
+	}
+
+	podInfo := []struct {
+		description string
+		hasSysctls  bool
+		pod         func() *api.Pod
+	}{
+		{
+			description: "has Sysctls",
+			hasSysctls:  true,
+			pod:         podWithSysctls,
+		},
+		{
+			description: "does not have Sysctls",
+			hasSysctls:  false,
+			pod:         podWithoutSysctls,
+		},
+		{
+			description: "does not have SecurityContext",
+			hasSysctls:  false,
+			pod:         podWithoutSecurityContext,
+		},
+		{
+			description: "is nil",
+			hasSysctls:  false,
+			pod:         func() *api.Pod { return nil },
+		},
+	}
+
+	for _, enabled := range []bool{true, false} {
+		for _, oldPodInfo := range podInfo {
+			for _, newPodInfo := range podInfo {
+				oldPodHasSysctls, oldPod := oldPodInfo.hasSysctls, oldPodInfo.pod()
+				newPodHasSysctls, newPod := newPodInfo.hasSysctls, 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.Sysctls, enabled)()
+
+					var oldPodSpec *api.PodSpec
+					if oldPod != nil {
+						oldPodSpec = &oldPod.Spec
+					}
+					dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)
+
+					// 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 || oldPodHasSysctls:
+						// new pod should not be changed if the feature is enabled, or if the old pod had Sysctls set
+						if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
+						}
+					case newPodHasSysctls:
+						// new pod should be changed
+						if reflect.DeepEqual(newPod, newPodInfo.pod()) {
+							t.Errorf("new pod was not changed")
+						}
+						// new pod should not have Sysctls
+						if !reflect.DeepEqual(newPod, podWithoutSysctls()) {
+							t.Errorf("new pod had Sysctls: %v", diff.ObjectReflectDiff(newPod, podWithoutSysctls()))
+						}
+					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

@@ -31,6 +31,10 @@ func DropDisabledFields(pspSpec, oldPSPSpec *policy.PodSecurityPolicySpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) && (oldPSPSpec == nil || oldPSPSpec.RunAsGroup == nil) {
 		pspSpec.RunAsGroup = nil
 	}
+	if !utilfeature.DefaultFeatureGate.Enabled(features.Sysctls) && !sysctlsInUse(oldPSPSpec) {
+		pspSpec.AllowedUnsafeSysctls = nil
+		pspSpec.ForbiddenSysctls = nil
+	}
 }
 
 func allowedProcMountTypesInUse(oldPSPSpec *policy.PodSecurityPolicySpec) bool {
@@ -45,3 +49,13 @@ func allowedProcMountTypesInUse(oldPSPSpec *policy.PodSecurityPolicySpec) bool {
 	return false
 
 }
+
+func sysctlsInUse(oldPSPSpec *policy.PodSecurityPolicySpec) bool {
+	if oldPSPSpec == nil {
+		return false
+	}
+	if oldPSPSpec.AllowedUnsafeSysctls != nil || oldPSPSpec.ForbiddenSysctls != nil {
+		return true
+	}
+	return false
+}

pkg/api/podsecuritypolicy/util_test.go

@@ -187,3 +187,92 @@ func TestDropRunAsGroup(t *testing.T) {
 		}
 	}
 }
+
+func TestDropSysctls(t *testing.T) {
+	scWithSysctls := func() *policy.PodSecurityPolicySpec {
+		return &policy.PodSecurityPolicySpec{
+			AllowedUnsafeSysctls: []string{"foo/*"},
+			ForbiddenSysctls:     []string{"bar.*"},
+		}
+	}
+	scWithOneSysctls := func() *policy.PodSecurityPolicySpec {
+		return &policy.PodSecurityPolicySpec{
+			AllowedUnsafeSysctls: []string{"foo/*"},
+		}
+	}
+	scWithoutSysctls := func() *policy.PodSecurityPolicySpec {
+		return &policy.PodSecurityPolicySpec{}
+	}
+
+	scInfo := []struct {
+		description string
+		hasSysctls  bool
+		sc          func() *policy.PodSecurityPolicySpec
+	}{
+		{
+			description: "has Sysctls",
+			hasSysctls:  true,
+			sc:          scWithSysctls,
+		},
+		{
+			description: "has one Sysctl",
+			hasSysctls:  true,
+			sc:          scWithOneSysctls,
+		},
+		{
+			description: "does not have Sysctls",
+			hasSysctls:  false,
+			sc:          scWithoutSysctls,
+		},
+		{
+			description: "is nil",
+			hasSysctls:  false,
+			sc:          func() *policy.PodSecurityPolicySpec { return nil },
+		},
+	}
+
+	for _, enabled := range []bool{true, false} {
+		for _, oldPSPSpecInfo := range scInfo {
+			for _, newPSPSpecInfo := range scInfo {
+				oldPSPSpecHasSysctls, oldPSPSpec := oldPSPSpecInfo.hasSysctls, oldPSPSpecInfo.sc()
+				newPSPSpecHasSysctls, newPSPSpec := newPSPSpecInfo.hasSysctls, 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.Sysctls, 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 || oldPSPSpecHasSysctls:
+						// new PodSecurityPolicySpec should not be changed if the feature is enabled, or if the old PodSecurityPolicySpec had Sysctls
+						if !reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.sc()) {
+							t.Errorf("new PodSecurityPolicySpec changed: %v", diff.ObjectReflectDiff(newPSPSpec, newPSPSpecInfo.sc()))
+						}
+					case newPSPSpecHasSysctls:
+						// new PodSecurityPolicySpec should be changed
+						if reflect.DeepEqual(newPSPSpec, newPSPSpecInfo.sc()) {
+							t.Errorf("new PodSecurityPolicySpec was not changed")
+						}
+						// new PodSecurityPolicySpec should not have Sysctls
+						if !reflect.DeepEqual(newPSPSpec, scWithoutSysctls()) {
+							t.Errorf("new PodSecurityPolicySpec had Sysctls: %v", diff.ObjectReflectDiff(newPSPSpec, scWithoutSysctls()))
+						}
+					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()))
+						}
+					}
+				})
+			}
+		}
+	}
+}

pkg/apis/core/validation/validation.go

@@ -3436,11 +3436,7 @@ func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec *
 		}
 
 		if len(securityContext.Sysctls) != 0 {
-			if utilfeature.DefaultFeatureGate.Enabled(features.Sysctls) {
+			allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"))...)
-				allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"))...)
-			} else {
-				allErrs = append(allErrs, field.Forbidden(fldPath.Child("sysctls"), "Sysctls are disabled by Sysctls feature-gate"))
-			}
 		}
 	}
 

pkg/apis/policy/validation/BUILD

@@ -15,7 +15,6 @@ go_library(
         "//pkg/apis/core:go_default_library",
         "//pkg/apis/core/validation:go_default_library",
         "//pkg/apis/policy:go_default_library",
-        "//pkg/features:go_default_library",
         "//pkg/security/apparmor:go_default_library",
         "//pkg/security/podsecuritypolicy/seccomp:go_default_library",
         "//pkg/security/podsecuritypolicy/util:go_default_library",
@@ -23,7 +22,6 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
-        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
     ],
 )
 

pkg/apis/policy/validation/validation.go

@@ -31,12 +31,9 @@ import (
 	core "k8s.io/kubernetes/pkg/apis/core"
 	apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"
 	"k8s.io/kubernetes/pkg/apis/policy"
-	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/security/apparmor"
 	"k8s.io/kubernetes/pkg/security/podsecuritypolicy/seccomp"
 	psputil "k8s.io/kubernetes/pkg/security/podsecuritypolicy/util"
-
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 )
 
 func ValidatePodDisruptionBudget(pdb *policy.PodDisruptionBudget) field.ErrorList {
@@ -401,10 +398,6 @@ func validatePodSecurityPolicySysctls(fldPath *field.Path, sysctls []string) fie
 		return allErrs
 	}
 
-	if !utilfeature.DefaultFeatureGate.Enabled(features.Sysctls) {
-		return append(allErrs, field.Forbidden(fldPath, "Sysctls are disabled by Sysctls feature-gate"))
-	}
-
 	coversAll := false
 	for i, s := range sysctls {
 		if len(s) == 0 {