From 5de2d7694dfdeea4252231d88f00a17a73e561e4 Mon Sep 17 00:00:00 2001 From: Rajath Agasthya Date: Wed, 9 Jan 2019 14:17:50 -0800 Subject: [PATCH] Remove Sysctls feature gate from validation --- pkg/api/pod/util.go | 16 ++++ pkg/api/pod/util_test.go | 100 +++++++++++++++++++++++ pkg/api/podsecuritypolicy/util.go | 14 ++++ pkg/api/podsecuritypolicy/util_test.go | 89 ++++++++++++++++++++ pkg/apis/core/validation/validation.go | 6 +- pkg/apis/policy/validation/BUILD | 2 - pkg/apis/policy/validation/validation.go | 7 -- 7 files changed, 220 insertions(+), 14 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 999d3f6e3c8..a40d1ac6849 100644 --- a/pkg/api/pod/util.go +++ b/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 { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 4b9e06e8a6c..80c527171d3 100644 --- a/pkg/api/pod/util_test.go +++ b/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())) + } + } + }) + } + } + } +} diff --git a/pkg/api/podsecuritypolicy/util.go b/pkg/api/podsecuritypolicy/util.go index 4aecb5a16ad..10ce15093c9 100644 --- a/pkg/api/podsecuritypolicy/util.go +++ b/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 +} diff --git a/pkg/api/podsecuritypolicy/util_test.go b/pkg/api/podsecuritypolicy/util_test.go index 7ec614c1719..1527fdb67a0 100644 --- a/pkg/api/podsecuritypolicy/util_test.go +++ b/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())) + } + } + }) + } + } + } +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 6bdc69e244c..1e2cec0ffd0 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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"))...) - } else { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("sysctls"), "Sysctls are disabled by Sysctls feature-gate")) - } + allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"))...) } } diff --git a/pkg/apis/policy/validation/BUILD b/pkg/apis/policy/validation/BUILD index a32bc3f9ca4..0b60688425e 100644 --- a/pkg/apis/policy/validation/BUILD +++ b/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", ], ) diff --git a/pkg/apis/policy/validation/validation.go b/pkg/apis/policy/validation/validation.go index 18108e2e526..cb9e92864f3 100644 --- a/pkg/apis/policy/validation/validation.go +++ b/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 {