Merge pull request #99158 from wgahnagl/lock-sysctls

Graduate sysctls to GA

pkg/api/pod/util.go

@@ -447,12 +447,6 @@ func dropDisabledFields(
 		}
 	}
 
-	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 {
@@ -648,16 +642,6 @@ func podPriorityInUse(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 EmptyDir volumes use SizeLimit.
 func emptyDirSizeLimitInUse(podSpec *api.PodSpec) bool {
 	if podSpec == nil {

pkg/api/pod/util_test.go

@@ -1017,106 +1017,6 @@ func TestDropAppArmor(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 featuregatetesting.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()))
-						}
-					}
-				})
-			}
-		}
-	}
-}
-
 func TestDropSubPathExpr(t *testing.T) {
 	podWithSubpaths := func() *api.Pod {
 		return &api.Pod{

pkg/api/podsecuritypolicy/util.go

@@ -28,10 +28,6 @@ func DropDisabledFields(pspSpec, oldPSPSpec *policy.PodSecurityPolicySpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) && !allowedProcMountTypesInUse(oldPSPSpec) {
 		pspSpec.AllowedProcMountTypes = nil
 	}
-	if !utilfeature.DefaultFeatureGate.Enabled(features.Sysctls) && !sysctlsInUse(oldPSPSpec) {
-		pspSpec.AllowedUnsafeSysctls = nil
-		pspSpec.ForbiddenSysctls = nil
-	}
 	if !utilfeature.DefaultFeatureGate.Enabled(features.CSIInlineVolume) {
 		pspSpec.AllowedCSIDrivers = nil
 	}
@@ -49,13 +45,3 @@ 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

@@ -107,92 +107,3 @@ func TestDropAllowedProcMountTypes(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 featuregatetesting.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/features/kube_features.go

@@ -130,7 +130,9 @@ const (
 	MemoryManager featuregate.Feature = "MemoryManager"
 
 	// owner: @sjenning
+	// alpha: v1.4
 	// beta: v1.11
+	// ga: v1.21
 	//
 	// Enable pods to set sysctls on a pod
 	Sysctls featuregate.Feature = "Sysctls"
@@ -677,7 +679,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
 	DevicePlugins:                                  {Default: true, PreRelease: featuregate.Beta},
 	RotateKubeletServerCertificate:                 {Default: true, PreRelease: featuregate.Beta},
 	LocalStorageCapacityIsolation:                  {Default: true, PreRelease: featuregate.Beta},
-	Sysctls:                                        {Default: true, PreRelease: featuregate.Beta},
+	Sysctls:                                        {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.23
 	EphemeralContainers:                            {Default: false, PreRelease: featuregate.Alpha},
 	QOSReserved:                                    {Default: false, PreRelease: featuregate.Alpha},
 	ExpandPersistentVolumes:                        {Default: true, PreRelease: featuregate.Beta},

pkg/kubelet/kubelet.go

@@ -775,16 +775,14 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
 	klet.evictionManager = evictionManager
 	klet.admitHandlers.AddPodAdmitHandler(evictionAdmitHandler)
 
-	if utilfeature.DefaultFeatureGate.Enabled(features.Sysctls) {
+	// Safe, whitelisted sysctls can always be used as unsafe sysctls in the spec.
-		// Safe, whitelisted sysctls can always be used as unsafe sysctls in the spec.
+	// Hence, we concatenate those two lists.
-		// Hence, we concatenate those two lists.
+	safeAndUnsafeSysctls := append(sysctlwhitelist.SafeSysctlWhitelist(), allowedUnsafeSysctls...)
-		safeAndUnsafeSysctls := append(sysctlwhitelist.SafeSysctlWhitelist(), allowedUnsafeSysctls...)
+	sysctlsWhitelist, err := sysctl.NewWhitelist(safeAndUnsafeSysctls)
-		sysctlsWhitelist, err := sysctl.NewWhitelist(safeAndUnsafeSysctls)
+	if err != nil {
-		if err != nil {
+		return nil, err
-			return nil, err
-		}
-		klet.admitHandlers.AddPodAdmitHandler(sysctlsWhitelist)
 	}
+	klet.admitHandlers.AddPodAdmitHandler(sysctlsWhitelist)
 
 	// enable active deadline handler
 	activeDeadlineHandler, err := newActiveDeadlineHandler(klet.statusManager, kubeDeps.Recorder, klet.clock)

pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

@@ -25,10 +25,8 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	kubetypes "k8s.io/apimachinery/pkg/types"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 	"k8s.io/klog/v2"
-	"k8s.io/kubernetes/pkg/features"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 	"k8s.io/kubernetes/pkg/kubelet/types"
 	"k8s.io/kubernetes/pkg/kubelet/util"
@@ -166,11 +164,9 @@ func (m *kubeGenericRuntimeManager) generatePodSandboxLinuxConfig(pod *v1.Pod) (
 	}
 
 	sysctls := make(map[string]string)
-	if utilfeature.DefaultFeatureGate.Enabled(features.Sysctls) {
+	if pod.Spec.SecurityContext != nil {
-		if pod.Spec.SecurityContext != nil {
+		for _, c := range pod.Spec.SecurityContext.Sysctls {
-			for _, c := range pod.Spec.SecurityContext.Sysctls {
+			sysctls[c.Name] = c.Value
-				sysctls[c.Name] = c.Value
-			}
 		}
 	}