From 9a6f1e807e4680f4a646e4aa73c648e02b084594 Mon Sep 17 00:00:00 2001 From: Mayank Kumar Date: Wed, 9 Sep 2020 01:08:06 -0700 Subject: [PATCH] Promote RunAsGroup to GA --- pkg/api/pod/util.go | 40 ------ pkg/api/pod/util_test.go | 137 --------------------- pkg/api/podsecuritypolicy/util.go | 3 - pkg/api/podsecuritypolicy/util_test.go | 80 ------------ pkg/features/kube_features.go | 5 +- pkg/security/podsecuritypolicy/factory.go | 11 +- pkg/security/podsecuritypolicy/provider.go | 25 ++-- 7 files changed, 16 insertions(+), 285 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 27398b53fc4..787a238eaae 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -484,8 +484,6 @@ func dropDisabledFields( }) } - dropDisabledRunAsGroupField(podSpec, oldPodSpec) - dropDisabledFSGroupFields(podSpec, oldPodSpec) if !utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) && !overheadInUse(oldPodSpec) { @@ -512,22 +510,6 @@ func dropDisabledFields( } -// dropDisabledRunAsGroupField removes disabled fields from PodSpec related -// to RunAsGroup -func dropDisabledRunAsGroupField(podSpec, oldPodSpec *api.PodSpec) { - if !utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) && !runAsGroupInUse(oldPodSpec) { - if podSpec.SecurityContext != nil { - podSpec.SecurityContext.RunAsGroup = nil - } - VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool { - if c.SecurityContext != nil { - c.SecurityContext.RunAsGroup = nil - } - return true - }) - } -} - // dropDisabledProcMountField removes disabled fields from PodSpec related // to ProcMount only if it is not already used by the old spec func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) { @@ -691,28 +673,6 @@ func emptyDirSizeLimitInUse(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 - } - - var inUse bool - VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool { - if c.SecurityContext != nil && c.SecurityContext.RunAsGroup != nil { - inUse = true - return false - } - return true - }) - - return inUse -} - // subpathExprInUse returns true if the pod spec is non-nil and has a volume mount that makes use of the subPathExpr feature func subpathExprInUse(podSpec *api.PodSpec) bool { if podSpec == nil { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 74866f239af..47be8fd5e62 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -1017,143 +1017,6 @@ func TestDropAppArmor(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 featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RunAsGroup, 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 || 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())) - } - } - }) - } - } - } -} - func TestDropPodSysctls(t *testing.T) { podWithSysctls := func() *api.Pod { return &api.Pod{ diff --git a/pkg/api/podsecuritypolicy/util.go b/pkg/api/podsecuritypolicy/util.go index 6dbcdb95b6f..43de1b835c8 100644 --- a/pkg/api/podsecuritypolicy/util.go +++ b/pkg/api/podsecuritypolicy/util.go @@ -28,9 +28,6 @@ func DropDisabledFields(pspSpec, oldPSPSpec *policy.PodSecurityPolicySpec) { if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) && !allowedProcMountTypesInUse(oldPSPSpec) { pspSpec.AllowedProcMountTypes = nil } - 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 diff --git a/pkg/api/podsecuritypolicy/util_test.go b/pkg/api/podsecuritypolicy/util_test.go index 4ab7f6fc288..e20ca3795a6 100644 --- a/pkg/api/podsecuritypolicy/util_test.go +++ b/pkg/api/podsecuritypolicy/util_test.go @@ -108,86 +108,6 @@ 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 featuregatetesting.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())) - } - } - }) - } - } - } -} - func TestDropSysctls(t *testing.T) { scWithSysctls := func() *policy.PodSecurityPolicySpec { return &policy.PodSecurityPolicySpec{ diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 8e7e9dbd195..68022e3f4cf 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -216,7 +216,8 @@ const ( CRIContainerLogRotation featuregate.Feature = "CRIContainerLogRotation" // owner: @krmayankk - // beta: v1.14 + // beta: v1.14 + // ga: v1.21 // // Enables control over the primary group ID of containers' init processes. RunAsGroup featuregate.Feature = "RunAsGroup" @@ -730,7 +731,6 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS CSIMigrationvSphere: {Default: false, PreRelease: featuregate.Beta}, // Off by default (requires vSphere CSI driver) CSIMigrationvSphereComplete: {Default: false, PreRelease: featuregate.Beta}, // remove in 1.22 InTreePluginvSphereUnregister: {Default: false, PreRelease: featuregate.Alpha}, - RunAsGroup: {Default: true, PreRelease: featuregate.Beta}, CSIMigrationOpenStack: {Default: false, PreRelease: featuregate.Beta}, // Off by default (requires OpenStack Cinder CSI driver) InTreePluginOpenStackUnregister: {Default: false, PreRelease: featuregate.Alpha}, VolumeSubpath: {Default: true, PreRelease: featuregate.GA}, @@ -785,6 +785,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS ServiceLBNodePortControl: {Default: false, PreRelease: featuregate.Alpha}, MixedProtocolLBService: {Default: false, PreRelease: featuregate.Alpha}, PreferNominatedNode: {Default: false, PreRelease: featuregate.Alpha}, + RunAsGroup: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.22 // inherited features from generic apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: diff --git a/pkg/security/podsecuritypolicy/factory.go b/pkg/security/podsecuritypolicy/factory.go index e68cf647dd2..ed019b223e3 100644 --- a/pkg/security/podsecuritypolicy/factory.go +++ b/pkg/security/podsecuritypolicy/factory.go @@ -19,9 +19,6 @@ package podsecuritypolicy import ( "fmt" - utilfeature "k8s.io/apiserver/pkg/util/feature" - "k8s.io/kubernetes/pkg/features" - corev1 "k8s.io/api/core/v1" policy "k8s.io/api/policy/v1beta1" "k8s.io/apimachinery/pkg/util/errors" @@ -51,11 +48,9 @@ func (f *simpleStrategyFactory) CreateStrategies(psp *policy.PodSecurityPolicy, } var groupStrat group.GroupStrategy - if utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) { - groupStrat, err = createRunAsGroupStrategy(psp.Spec.RunAsGroup) - if err != nil { - errs = append(errs, err) - } + groupStrat, err = createRunAsGroupStrategy(psp.Spec.RunAsGroup) + if err != nil { + errs = append(errs, err) } seLinuxStrat, err := createSELinuxStrategy(&psp.Spec.SELinux) diff --git a/pkg/security/podsecuritypolicy/provider.go b/pkg/security/podsecuritypolicy/provider.go index d3ee1dcbc75..eda587533c9 100644 --- a/pkg/security/podsecuritypolicy/provider.go +++ b/pkg/security/podsecuritypolicy/provider.go @@ -140,15 +140,12 @@ func (s *simpleProvider) mutateContainer(pod *api.Pod, container *api.Container) sc.SetRunAsUser(uid) } - if utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) { - if sc.RunAsGroup() == nil { - gid, err := s.strategies.RunAsGroupStrategy.GenerateSingle(pod) - if err != nil { - return err - } - sc.SetRunAsGroup(gid) + if sc.RunAsGroup() == nil { + gid, err := s.strategies.RunAsGroupStrategy.GenerateSingle(pod) + if err != nil { + return err } - + sc.SetRunAsGroup(gid) } if sc.SELinuxOptions() == nil { @@ -337,14 +334,12 @@ func (s *simpleProvider) validateContainer(pod *api.Pod, container *api.Containe scPath := containerPath.Child("securityContext") allErrs = append(allErrs, s.strategies.RunAsUserStrategy.Validate(scPath, pod, container, sc.RunAsNonRoot(), sc.RunAsUser())...) - - if utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) { - var runAsGroups []int64 - if sc.RunAsGroup() != nil { - runAsGroups = []int64{*sc.RunAsGroup()} - } - allErrs = append(allErrs, s.strategies.RunAsGroupStrategy.Validate(scPath, pod, runAsGroups)...) + var runAsGroups []int64 + if sc.RunAsGroup() != nil { + runAsGroups = []int64{*sc.RunAsGroup()} } + allErrs = append(allErrs, s.strategies.RunAsGroupStrategy.Validate(scPath, pod, runAsGroups)...) + allErrs = append(allErrs, s.strategies.SELinuxStrategy.Validate(scPath.Child("seLinuxOptions"), pod, container, sc.SELinuxOptions())...) allErrs = append(allErrs, s.strategies.AppArmorStrategy.Validate(pod, container)...) allErrs = append(allErrs, s.strategies.SeccompStrategy.ValidateContainer(pod, container)...)