From b1f29601605fe59caa26769d76a75c5255d7b577 Mon Sep 17 00:00:00 2001 From: Skyler Clark Date: Tue, 16 Feb 2021 15:09:58 -0500 Subject: [PATCH 1/2] locks sysctls to on --- pkg/api/pod/util_test.go | 100 ------------------------- pkg/api/podsecuritypolicy/util_test.go | 89 ---------------------- pkg/features/kube_features.go | 4 +- 3 files changed, 3 insertions(+), 190 deletions(-) diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 47be8fd5e62..2e6b24b17b0 100644 --- a/pkg/api/pod/util_test.go +++ b/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{ diff --git a/pkg/api/podsecuritypolicy/util_test.go b/pkg/api/podsecuritypolicy/util_test.go index e20ca3795a6..1535b04128c 100644 --- a/pkg/api/podsecuritypolicy/util_test.go +++ b/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())) - } - } - }) - } - } - } -} diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 4f2ce5417e2..42d8752b02f 100644 --- a/pkg/features/kube_features.go +++ b/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" @@ -676,7 +678,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}, From 3de4dd841fd6cba942e85d84171a97728e1cf21a Mon Sep 17 00:00:00 2001 From: pacoxu Date: Mon, 22 Feb 2021 14:00:17 +0800 Subject: [PATCH 2/2] remove featuregate for sysctl Co-authored-by: Skyler Clark --- pkg/api/pod/util.go | 16 ---------------- pkg/api/podsecuritypolicy/util.go | 14 -------------- pkg/kubelet/kubelet.go | 16 +++++++--------- pkg/kubelet/kuberuntime/kuberuntime_sandbox.go | 10 +++------- 4 files changed, 10 insertions(+), 46 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 787a238eaae..dd79c4831b9 100644 --- a/pkg/api/pod/util.go +++ b/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 { diff --git a/pkg/api/podsecuritypolicy/util.go b/pkg/api/podsecuritypolicy/util.go index 43de1b835c8..a0df4f83ac4 100644 --- a/pkg/api/podsecuritypolicy/util.go +++ b/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 -} diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 6513882d8e1..55b5f0ce62d 100644 --- a/pkg/kubelet/kubelet.go +++ b/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. - // Hence, we concatenate those two lists. - safeAndUnsafeSysctls := append(sysctlwhitelist.SafeSysctlWhitelist(), allowedUnsafeSysctls...) - sysctlsWhitelist, err := sysctl.NewWhitelist(safeAndUnsafeSysctls) - if err != nil { - return nil, err - } - klet.admitHandlers.AddPodAdmitHandler(sysctlsWhitelist) + // Safe, whitelisted sysctls can always be used as unsafe sysctls in the spec. + // Hence, we concatenate those two lists. + safeAndUnsafeSysctls := append(sysctlwhitelist.SafeSysctlWhitelist(), allowedUnsafeSysctls...) + sysctlsWhitelist, err := sysctl.NewWhitelist(safeAndUnsafeSysctls) + if err != nil { + return nil, err } + klet.admitHandlers.AddPodAdmitHandler(sysctlsWhitelist) // enable active deadline handler activeDeadlineHandler, err := newActiveDeadlineHandler(klet.statusManager, kubeDeps.Recorder, klet.clock) diff --git a/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go b/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go index b09edd08d7f..db0bdd7a0dc 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go +++ b/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 { - for _, c := range pod.Spec.SecurityContext.Sysctls { - sysctls[c.Name] = c.Value - } + if pod.Spec.SecurityContext != nil { + for _, c := range pod.Spec.SecurityContext.Sysctls { + sysctls[c.Name] = c.Value } }