From f4ddf15c1fb0f7e21c2ca828250f3fe23bd260ab Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 25 Oct 2021 12:21:43 -0400 Subject: [PATCH 1/4] Move ConfigurableFSGroupPolicy feature to GA --- pkg/features/kube_features.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index df440c7e651..8678c79ba2e 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -333,6 +333,7 @@ const ( // owner: @gnufied // alpha: v1.18 // beta: v1.20 + // GA: v1.23 // Allows user to configure volume permission change policy for fsGroups when mounting // a volume in a Pod. ConfigurableFSGroupPolicy featuregate.Feature = "ConfigurableFSGroupPolicy" @@ -816,7 +817,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS InTreePluginvSphereUnregister: {Default: false, PreRelease: featuregate.Alpha}, CSIMigrationOpenStack: {Default: true, PreRelease: featuregate.Beta}, InTreePluginOpenStackUnregister: {Default: false, PreRelease: featuregate.Alpha}, - ConfigurableFSGroupPolicy: {Default: true, PreRelease: featuregate.Beta}, + ConfigurableFSGroupPolicy: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.24 CSIInlineVolume: {Default: true, PreRelease: featuregate.Beta}, CSIStorageCapacity: {Default: true, PreRelease: featuregate.Beta}, CSIServiceAccountToken: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.23 From 481068c0d27f3f63808d97c5244f5f8869e30662 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 25 Oct 2021 12:27:53 -0400 Subject: [PATCH 2/4] rename volume_fsgroup_recursive_apply metric to volume_apply_access_control --- pkg/volume/util/metrics.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/volume/util/metrics.go b/pkg/volume/util/metrics.go index 356566227bd..df5a2f23265 100644 --- a/pkg/volume/util/metrics.go +++ b/pkg/volume/util/metrics.go @@ -132,7 +132,7 @@ func OperationCompleteHook(plugin, operationName string) func(types.CompleteFunc // FSGroupCompleteHook returns a hook to call when volume recursive permission is changed func FSGroupCompleteHook(plugin volume.VolumePlugin, spec *volume.Spec) func(types.CompleteFuncParam) { - return OperationCompleteHook(GetFullQualifiedPluginNameForVolume(plugin.GetPluginName(), spec), "volume_fsgroup_recursive_apply") + return OperationCompleteHook(GetFullQualifiedPluginNameForVolume(plugin.GetPluginName(), spec), "volume_apply_access_control") } // GetFullQualifiedPluginNameForVolume returns full qualified plugin name for From 9d9c3000b015d653915ef4977cb74f2c25bdce3a Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Thu, 28 Oct 2021 08:10:08 -0400 Subject: [PATCH 3/4] Remove unnecessary unit tests that exercised disabling the feature gate --- pkg/api/pod/util_test.go | 33 --------------------------------- pkg/features/kube_features.go | 2 +- 2 files changed, 1 insertion(+), 34 deletions(-) diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index e9b5c494554..9b588f67a06 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -562,56 +562,24 @@ func TestDropFSGroupFields(t *testing.T) { } podInfos := []struct { description string - featureEnabled bool newPodHasFSGroupChangePolicy bool pod func() *api.Pod expectPolicyInPod bool }{ { description: "oldPod.FSGroupChangePolicy=nil, feature=true, newPod.FSGroupChangePolicy=true", - featureEnabled: true, pod: nofsGroupPod, newPodHasFSGroupChangePolicy: true, expectPolicyInPod: true, }, - { - description: "oldPod=nil, feature=false, newPod.FSGroupChangePolicy=true", - featureEnabled: false, - pod: func() *api.Pod { return nil }, - newPodHasFSGroupChangePolicy: true, - expectPolicyInPod: false, - }, { description: "oldPod=nil, feature=true, newPod.FSGroupChangePolicy=true", - featureEnabled: true, pod: func() *api.Pod { return nil }, newPodHasFSGroupChangePolicy: true, expectPolicyInPod: true, }, - { - description: "oldPod.FSGroupChangePolicy=nil, feature=false, newPod.FSGroupChangePolicy=true", - featureEnabled: false, - pod: nofsGroupPod, - newPodHasFSGroupChangePolicy: true, - expectPolicyInPod: false, - }, - { - description: "oldPod.FSGroupChangePolicy=true, feature=false, newPod.FSGroupChangePolicy=true", - featureEnabled: false, - pod: fsGroupPod, - newPodHasFSGroupChangePolicy: true, - expectPolicyInPod: true, - }, - { - description: "oldPod.FSGroupChangePolicy=true, feature=false, newPod.FSGroupChangePolicy=false", - featureEnabled: false, - pod: fsGroupPod, - newPodHasFSGroupChangePolicy: false, - expectPolicyInPod: false, - }, { description: "oldPod.FSGroupChangePolicy=true, feature=true, newPod.FSGroupChangePolicy=false", - featureEnabled: true, pod: fsGroupPod, newPodHasFSGroupChangePolicy: false, expectPolicyInPod: false, @@ -619,7 +587,6 @@ func TestDropFSGroupFields(t *testing.T) { } for _, podInfo := range podInfos { t.Run(podInfo.description, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConfigurableFSGroupPolicy, podInfo.featureEnabled)() oldPod := podInfo.pod() newPod := oldPod.DeepCopy() if oldPod == nil && podInfo.newPodHasFSGroupChangePolicy { diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 8678c79ba2e..3d0ddc0554b 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -817,7 +817,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS InTreePluginvSphereUnregister: {Default: false, PreRelease: featuregate.Alpha}, CSIMigrationOpenStack: {Default: true, PreRelease: featuregate.Beta}, InTreePluginOpenStackUnregister: {Default: false, PreRelease: featuregate.Alpha}, - ConfigurableFSGroupPolicy: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.24 + ConfigurableFSGroupPolicy: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25 CSIInlineVolume: {Default: true, PreRelease: featuregate.Beta}, CSIStorageCapacity: {Default: true, PreRelease: featuregate.Beta}, CSIServiceAccountToken: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.23 From 27d1e9a4e26b800c0253c63a03fe2a23a9647750 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Wed, 10 Nov 2021 14:24:08 -0500 Subject: [PATCH 4/4] Remove all references to ConfigurableFSGroupPolicy feature gate --- pkg/api/pod/util.go | 23 ----------------------- pkg/volume/volume_linux.go | 25 ------------------------- pkg/volume/volume_linux_test.go | 8 -------- 3 files changed, 56 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index d1614240e2e..c786c7b190b 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -546,8 +546,6 @@ func dropDisabledFields( }) } - dropDisabledFSGroupFields(podSpec, oldPodSpec) - if !utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) && !overheadInUse(oldPodSpec) { // Set Overhead to nil only if the feature is disabled and it is not used podSpec.Overhead = nil @@ -585,16 +583,6 @@ func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) { } } -func dropDisabledFSGroupFields(podSpec, oldPodSpec *api.PodSpec) { - if !utilfeature.DefaultFeatureGate.Enabled(features.ConfigurableFSGroupPolicy) && !fsGroupPolicyInUse(oldPodSpec) { - // if oldPodSpec had no FSGroupChangePolicy set then we should prevent new pod from having this field - // if ConfigurableFSGroupPolicy feature is disabled - if podSpec.SecurityContext != nil { - podSpec.SecurityContext.FSGroupChangePolicy = nil - } - } -} - // dropDisabledCSIVolumeSourceAlphaFields removes disabled alpha fields from []CSIVolumeSource. // This should be called from PrepareForCreate/PrepareForUpdate for all pod specs resources containing a CSIVolumeSource func dropDisabledCSIVolumeSourceAlphaFields(podSpec, oldPodSpec *api.PodSpec) { @@ -682,17 +670,6 @@ func ephemeralContainersInUse(podSpec *api.PodSpec) bool { return len(podSpec.EphemeralContainers) > 0 } -func fsGroupPolicyInUse(podSpec *api.PodSpec) bool { - if podSpec == nil { - return false - } - securityContext := podSpec.SecurityContext - if securityContext != nil && securityContext.FSGroupChangePolicy != nil { - return true - } - return false -} - // overheadInUse returns true if the pod spec is non-nil and has Overhead set func overheadInUse(podSpec *api.PodSpec) bool { if podSpec == nil { diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index ac2300f8132..c55eac77b33 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -27,9 +27,7 @@ import ( "time" v1 "k8s.io/api/core/v1" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/klog/v2" - "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/volume/util/types" ) @@ -47,25 +45,11 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1 return nil } - fsGroupPolicyEnabled := utilfeature.DefaultFeatureGate.Enabled(features.ConfigurableFSGroupPolicy) - timer := time.AfterFunc(30*time.Second, func() { klog.Warningf("Setting volume ownership for %s and fsGroup set. If the volume has a lot of files then setting volume ownership could be slow, see https://github.com/kubernetes/kubernetes/issues/69699", mounter.GetPath()) }) defer timer.Stop() - // This code exists for legacy purposes, so as old behaviour is entirely preserved when feature gate is disabled - // TODO: remove this when ConfigurableFSGroupPolicy turns GA. - if !fsGroupPolicyEnabled { - err := legacyOwnershipChange(mounter, fsGroup) - if completeFunc != nil { - completeFunc(types.CompleteFuncParam{ - Err: &err, - }) - } - return err - } - if skipPermissionChange(mounter, fsGroup, fsGroupChangePolicy) { klog.V(3).InfoS("Skipping permission and ownership change for volume", "path", mounter.GetPath()) return nil @@ -85,15 +69,6 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64, fsGroupChangePolicy *v1 return err } -func legacyOwnershipChange(mounter Mounter, fsGroup *int64) error { - return filepath.Walk(mounter.GetPath(), func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - return changeFilePermission(path, fsGroup, mounter.GetAttributes().ReadOnly, info) - }) -} - func changeFilePermission(filename string, fsGroup *int64, readonly bool, info os.FileInfo) error { err := os.Lchown(filename, -1, int(*fsGroup)) if err != nil { diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go index e0524a3d9a2..8c8f4430aa3 100644 --- a/pkg/volume/volume_linux_test.go +++ b/pkg/volume/volume_linux_test.go @@ -27,10 +27,7 @@ import ( "testing" v1 "k8s.io/api/core/v1" - utilfeature "k8s.io/apiserver/pkg/util/feature" utiltesting "k8s.io/client-go/util/testing" - featuregatetesting "k8s.io/component-base/featuregate/testing" - "k8s.io/kubernetes/pkg/features" ) type localFakeMounter struct { @@ -192,12 +189,10 @@ func TestSetVolumeOwnershipMode(t *testing.T) { fsGroupChangePolicy *v1.PodFSGroupChangePolicy setupFunc func(path string) error assertFunc func(path string) error - featureGate bool }{ { description: "featuregate=on, fsgroupchangepolicy=always", fsGroupChangePolicy: &always, - featureGate: true, setupFunc: func(path string) error { info, err := os.Lstat(path) if err != nil { @@ -230,7 +225,6 @@ func TestSetVolumeOwnershipMode(t *testing.T) { { description: "featuregate=on, fsgroupchangepolicy=onrootmismatch,rootdir=validperm", fsGroupChangePolicy: &onrootMismatch, - featureGate: true, setupFunc: func(path string) error { info, err := os.Lstat(path) if err != nil { @@ -262,7 +256,6 @@ func TestSetVolumeOwnershipMode(t *testing.T) { { description: "featuregate=on, fsgroupchangepolicy=onrootmismatch,rootdir=invalidperm", fsGroupChangePolicy: &onrootMismatch, - featureGate: true, setupFunc: func(path string) error { // change mode of root folder to be right err := os.Chmod(path, 0770) @@ -291,7 +284,6 @@ func TestSetVolumeOwnershipMode(t *testing.T) { for _, test := range tests { t.Run(test.description, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConfigurableFSGroupPolicy, test.featureGate)() tmpDir, err := utiltesting.MkTmpdir("volume_linux_ownership") if err != nil { t.Fatalf("error creating temp dir: %v", err)