From beb17fe10bba63c50ef35b8b78a468051dfa9da2 Mon Sep 17 00:00:00 2001 From: saad-ali Date: Fri, 17 Sep 2021 00:34:39 -0700 Subject: [PATCH] Remove VolumeSubpath feature gate Remove the VolumeSubpath feature gate. Feature gate convention has been updated since this was introduced to indicate that they "are intended to be deprecated and removed after a feature becomes GA or is dropped.". --- pkg/api/pod/util.go | 59 ------ pkg/api/pod/util_test.go | 188 -------------------- pkg/apis/core/validation/validation_test.go | 103 ----------- pkg/features/kube_features.go | 8 - pkg/kubelet/kubelet_pods.go | 8 - pkg/kubelet/kubelet_pods_test.go | 57 ------ 6 files changed, 423 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 1a61f970175..be52beb85ec 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -544,29 +544,10 @@ func dropDisabledFields( } } - if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) && !subpathInUse(oldPodSpec) { - // drop subpath from the pod if the feature is disabled and the old spec did not specify subpaths - VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool { - for i := range c.VolumeMounts { - c.VolumeMounts[i].SubPath = "" - } - return true - }) - } if !utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) && !ephemeralContainersInUse(oldPodSpec) { podSpec.EphemeralContainers = nil } - if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) && !subpathExprInUse(oldPodSpec) { - // drop subpath env expansion from the pod if subpath feature is disabled and the old spec did not specify subpath env expansion - VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool { - for i := range c.VolumeMounts { - c.VolumeMounts[i].SubPathExpr = "" - } - return true - }) - } - if !utilfeature.DefaultFeatureGate.Enabled(features.ProbeTerminationGracePeriod) && !probeGracePeriodInUse(oldPodSpec) { // Set pod-level terminationGracePeriodSeconds to nil if the feature is disabled and it is not used VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool { @@ -728,26 +709,6 @@ func fsGroupPolicyInUse(podSpec *api.PodSpec) bool { return false } -// subpathInUse returns true if the pod spec is non-nil and has a volume mount that makes use of the subPath feature -func subpathInUse(podSpec *api.PodSpec) bool { - if podSpec == nil { - return false - } - - var inUse bool - VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool { - for i := range c.VolumeMounts { - if len(c.VolumeMounts[i].SubPath) > 0 { - inUse = true - return false - } - } - return true - }) - - return inUse -} - // overheadInUse returns true if the pod spec is non-nil and has Overhead set func overheadInUse(podSpec *api.PodSpec) bool { if podSpec == nil { @@ -816,26 +777,6 @@ func emptyDirSizeLimitInUse(podSpec *api.PodSpec) bool { return false } -// 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 { - return false - } - - var inUse bool - VisitContainers(podSpec, AllContainers, func(c *api.Container, containerType ContainerType) bool { - for i := range c.VolumeMounts { - if len(c.VolumeMounts[i].SubPathExpr) > 0 { - inUse = true - return false - } - } - return true - }) - - return inUse -} - // probeGracePeriodInUse returns true if the pod spec is non-nil and has a probe that makes use // of the probe-level terminationGracePeriodSeconds feature func probeGracePeriodInUse(podSpec *api.PodSpec) bool { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index c13458fdc77..c66b738b98c 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -652,100 +652,6 @@ func TestDropFSGroupFields(t *testing.T) { } -func TestDropSubPath(t *testing.T) { - podWithSubpaths := func() *api.Pod { - return &api.Pod{ - Spec: api.PodSpec{ - RestartPolicy: api.RestartPolicyNever, - Containers: []api.Container{{Name: "container1", Image: "testimage", VolumeMounts: []api.VolumeMount{{Name: "a", SubPath: "foo"}, {Name: "a", SubPath: "foo2"}, {Name: "a", SubPath: "foo3"}}}}, - InitContainers: []api.Container{{Name: "container1", Image: "testimage", VolumeMounts: []api.VolumeMount{{Name: "a", SubPath: "foo"}, {Name: "a", SubPath: "foo2"}}}}, - Volumes: []api.Volume{{Name: "a", VolumeSource: api.VolumeSource{HostPath: &api.HostPathVolumeSource{Path: "/dev/xvdc"}}}}, - }, - } - } - podWithoutSubpaths := func() *api.Pod { - return &api.Pod{ - Spec: api.PodSpec{ - RestartPolicy: api.RestartPolicyNever, - Containers: []api.Container{{Name: "container1", Image: "testimage", VolumeMounts: []api.VolumeMount{{Name: "a", SubPath: ""}, {Name: "a", SubPath: ""}, {Name: "a", SubPath: ""}}}}, - InitContainers: []api.Container{{Name: "container1", Image: "testimage", VolumeMounts: []api.VolumeMount{{Name: "a", SubPath: ""}, {Name: "a", SubPath: ""}}}}, - Volumes: []api.Volume{{Name: "a", VolumeSource: api.VolumeSource{HostPath: &api.HostPathVolumeSource{Path: "/dev/xvdc"}}}}, - }, - } - } - - podInfo := []struct { - description string - hasSubpaths bool - pod func() *api.Pod - }{ - { - description: "has subpaths", - hasSubpaths: true, - pod: podWithSubpaths, - }, - { - description: "does not have subpaths", - hasSubpaths: false, - pod: podWithoutSubpaths, - }, - { - description: "is nil", - hasSubpaths: false, - pod: func() *api.Pod { return nil }, - }, - } - - for _, enabled := range []bool{true, false} { - for _, oldPodInfo := range podInfo { - for _, newPodInfo := range podInfo { - oldPodHasSubpaths, oldPod := oldPodInfo.hasSubpaths, oldPodInfo.pod() - newPodHasSubpaths, newPod := newPodInfo.hasSubpaths, 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.VolumeSubpath, 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", cmp.Diff(oldPod, oldPodInfo.pod())) - } - - switch { - case enabled || oldPodHasSubpaths: - // new pod should not be changed if the feature is enabled, or if the old pod had subpaths - if !reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) - } - case newPodHasSubpaths: - // new pod should be changed - if reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod was not changed") - } - // new pod should not have subpaths - if !reflect.DeepEqual(newPod, podWithoutSubpaths()) { - t.Errorf("new pod had subpaths: %v", cmp.Diff(newPod, podWithoutSubpaths())) - } - default: - // new pod should not need to be changed - if !reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) - } - } - }) - } - } - } -} - func TestDropProcMount(t *testing.T) { procMount := api.UnmaskedProcMount defaultProcMount := api.DefaultProcMount @@ -1046,100 +952,6 @@ func TestDropAppArmor(t *testing.T) { } } -func TestDropSubPathExpr(t *testing.T) { - podWithSubpaths := func() *api.Pod { - return &api.Pod{ - Spec: api.PodSpec{ - RestartPolicy: api.RestartPolicyNever, - Containers: []api.Container{{Name: "container1", Image: "testimage", VolumeMounts: []api.VolumeMount{{Name: "a", SubPathExpr: "foo"}, {Name: "a", SubPathExpr: "foo2"}, {Name: "a", SubPathExpr: "foo3"}}}}, - InitContainers: []api.Container{{Name: "container1", Image: "testimage", VolumeMounts: []api.VolumeMount{{Name: "a", SubPathExpr: "foo"}, {Name: "a", SubPathExpr: "foo2"}}}}, - Volumes: []api.Volume{{Name: "a", VolumeSource: api.VolumeSource{HostPath: &api.HostPathVolumeSource{Path: "/dev/xvdc"}}}}, - }, - } - } - podWithoutSubpaths := func() *api.Pod { - return &api.Pod{ - Spec: api.PodSpec{ - RestartPolicy: api.RestartPolicyNever, - Containers: []api.Container{{Name: "container1", Image: "testimage", VolumeMounts: []api.VolumeMount{{Name: "a", SubPathExpr: ""}, {Name: "a", SubPathExpr: ""}, {Name: "a", SubPathExpr: ""}}}}, - InitContainers: []api.Container{{Name: "container1", Image: "testimage", VolumeMounts: []api.VolumeMount{{Name: "a", SubPathExpr: ""}, {Name: "a", SubPathExpr: ""}}}}, - Volumes: []api.Volume{{Name: "a", VolumeSource: api.VolumeSource{HostPath: &api.HostPathVolumeSource{Path: "/dev/xvdc"}}}}, - }, - } - } - - podInfo := []struct { - description string - hasSubpaths bool - pod func() *api.Pod - }{ - { - description: "has subpaths", - hasSubpaths: true, - pod: podWithSubpaths, - }, - { - description: "does not have subpaths", - hasSubpaths: false, - pod: podWithoutSubpaths, - }, - { - description: "is nil", - hasSubpaths: false, - pod: func() *api.Pod { return nil }, - }, - } - - for _, enabled := range []bool{true, false} { - for _, oldPodInfo := range podInfo { - for _, newPodInfo := range podInfo { - oldPodHasSubpaths, oldPod := oldPodInfo.hasSubpaths, oldPodInfo.pod() - newPodHasSubpaths, newPod := newPodInfo.hasSubpaths, 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.VolumeSubpath, 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", cmp.Diff(oldPod, oldPodInfo.pod())) - } - - switch { - case enabled || oldPodHasSubpaths: - // new pod should not be changed if the feature is enabled, or if the old pod had subpaths - if !reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) - } - case newPodHasSubpaths: - // new pod should be changed - if reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod was not changed") - } - // new pod should not have subpaths - if !reflect.DeepEqual(newPod, podWithoutSubpaths()) { - t.Errorf("new pod had subpaths: %v", cmp.Diff(newPod, podWithoutSubpaths())) - } - default: - // new pod should not need to be changed - if !reflect.DeepEqual(newPod, newPodInfo.pod()) { - t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) - } - } - }) - } - } - } -} - func TestDropProbeGracePeriod(t *testing.T) { podWithProbeGracePeriod := func() *api.Pod { livenessGracePeriod := int64(10) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 64cdf96bd7a..cc2c52845c8 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -5586,71 +5586,7 @@ func TestValidateVolumeMounts(t *testing.T) { } } -func TestValidateDisabledSubpath(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpath, false)() - - volumes := []core.Volume{ - {Name: "abc", VolumeSource: core.VolumeSource{PersistentVolumeClaim: &core.PersistentVolumeClaimVolumeSource{ClaimName: "testclaim1"}}}, - {Name: "abc-123", VolumeSource: core.VolumeSource{PersistentVolumeClaim: &core.PersistentVolumeClaimVolumeSource{ClaimName: "testclaim2"}}}, - {Name: "123", VolumeSource: core.VolumeSource{HostPath: &core.HostPathVolumeSource{Path: "/foo/baz", Type: newHostPathType(string(core.HostPathUnset))}}}, - } - vols, v1err := ValidateVolumes(volumes, nil, field.NewPath("field"), PodValidationOptions{}) - if len(v1err) > 0 { - t.Errorf("Invalid test volume - expected success %v", v1err) - return - } - - container := core.Container{ - SecurityContext: nil, - } - - goodVolumeDevices := []core.VolumeDevice{ - {Name: "xyz", DevicePath: "/foofoo"}, - {Name: "uvw", DevicePath: "/foofoo/share/test"}, - } - - cases := map[string]struct { - mounts []core.VolumeMount - expectError bool - }{ - "subpath not specified": { - []core.VolumeMount{ - { - Name: "abc-123", - MountPath: "/bab", - }, - }, - false, - }, - "subpath specified": { - []core.VolumeMount{ - { - Name: "abc-123", - MountPath: "/bab", - SubPath: "baz", - }, - }, - false, // validation should not fail, dropping the field is handled in PrepareForCreate/PrepareForUpdate - }, - } - - for name, test := range cases { - errs := ValidateVolumeMounts(test.mounts, GetVolumeDeviceMap(goodVolumeDevices), vols, &container, field.NewPath("field")) - - if len(errs) != 0 && !test.expectError { - t.Errorf("test %v failed: %+v", name, errs) - } - - if len(errs) == 0 && test.expectError { - t.Errorf("test %v failed, expected error", name) - } - } -} - func TestValidateSubpathMutuallyExclusive(t *testing.T) { - // Enable feature VolumeSubpath - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpath, true)() - volumes := []core.Volume{ {Name: "abc", VolumeSource: core.VolumeSource{PersistentVolumeClaim: &core.PersistentVolumeClaimVolumeSource{ClaimName: "testclaim1"}}}, {Name: "abc-123", VolumeSource: core.VolumeSource{PersistentVolumeClaim: &core.PersistentVolumeClaimVolumeSource{ClaimName: "testclaim2"}}}, @@ -5788,45 +5724,6 @@ func TestValidateDisabledSubpathExpr(t *testing.T) { t.Errorf("test %v failed, expected error", name) } } - - // Repeat with subpath feature gate off - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpath, false)() - cases = map[string]struct { - mounts []core.VolumeMount - expectError bool - }{ - "subpath expr not specified": { - []core.VolumeMount{ - { - Name: "abc-123", - MountPath: "/bab", - }, - }, - false, - }, - "subpath expr specified": { - []core.VolumeMount{ - { - Name: "abc-123", - MountPath: "/bab", - SubPathExpr: "$(POD_NAME)", - }, - }, - false, // validation should not fail, dropping the field is handled in PrepareForCreate/PrepareForUpdate - }, - } - - for name, test := range cases { - errs := ValidateVolumeMounts(test.mounts, GetVolumeDeviceMap(goodVolumeDevices), vols, &container, field.NewPath("field")) - - if len(errs) != 0 && !test.expectError { - t.Errorf("test %v failed: %+v", name, errs) - } - - if len(errs) == 0 && test.expectError { - t.Errorf("test %v failed, expected error", name) - } - } } func TestValidateMountPropagation(t *testing.T) { diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index a36ef8c5cbe..16268891653 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -156,13 +156,6 @@ const ( // to the API server. BoundServiceAccountTokenVolume featuregate.Feature = "BoundServiceAccountTokenVolume" - // owner: @saad-ali - // ga: v1.10 - // - // Allow mounting a subpath of a volume in a container - // Do not remove this feature gate even though it's GA - VolumeSubpath featuregate.Feature = "VolumeSubpath" - // owner: @pohly // alpha: v1.14 // beta: v1.16 @@ -814,7 +807,6 @@ 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}, - VolumeSubpath: {Default: true, PreRelease: featuregate.GA}, ConfigurableFSGroupPolicy: {Default: true, PreRelease: featuregate.Beta}, CSIInlineVolume: {Default: true, PreRelease: featuregate.Beta}, CSIStorageCapacity: {Default: true, PreRelease: featuregate.Beta}, diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index a6eb12e8f28..d68fa45f9be 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -186,10 +186,6 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h subPath := mount.SubPath if mount.SubPathExpr != "" { - if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) { - return nil, cleanupAction, fmt.Errorf("volume subpaths are disabled") - } - subPath, err = kubecontainer.ExpandContainerVolumeMounts(mount, expandEnvs) if err != nil { @@ -198,10 +194,6 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h } if subPath != "" { - if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) { - return nil, cleanupAction, fmt.Errorf("volume subpaths are disabled") - } - if filepath.IsAbs(subPath) { return nil, cleanupAction, fmt.Errorf("error SubPath `%s` must not be an absolute path", subPath) } diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index 1dd1da19744..561eead7fae 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -56,65 +56,8 @@ import ( "k8s.io/kubernetes/pkg/kubelet/cri/streaming/remotecommand" "k8s.io/kubernetes/pkg/kubelet/prober/results" kubetypes "k8s.io/kubernetes/pkg/kubelet/types" - "k8s.io/kubernetes/pkg/volume/util/hostutil" - "k8s.io/kubernetes/pkg/volume/util/subpath" ) -func TestDisabledSubpath(t *testing.T) { - fhu := hostutil.NewFakeHostUtil(nil) - fsp := &subpath.FakeSubpath{} - pod := v1.Pod{ - Spec: v1.PodSpec{ - HostNetwork: true, - }, - } - podVolumes := kubecontainer.VolumeMap{ - "disk": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}}, - } - - cases := map[string]struct { - container v1.Container - expectError bool - }{ - "subpath not specified": { - v1.Container{ - VolumeMounts: []v1.VolumeMount{ - { - MountPath: "/mnt/path3", - Name: "disk", - ReadOnly: true, - }, - }, - }, - false, - }, - "subpath specified": { - v1.Container{ - VolumeMounts: []v1.VolumeMount{ - { - MountPath: "/mnt/path3", - SubPath: "/must/not/be/absolute", - Name: "disk", - ReadOnly: true, - }, - }, - }, - true, - }, - } - - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpath, false)() - for name, test := range cases { - _, _, err := makeMounts(&pod, "/pod", &test.container, "fakepodname", "", []string{}, podVolumes, fhu, fsp, nil, false) - if err != nil && !test.expectError { - t.Errorf("test %v failed: %v", name, err) - } - if err == nil && test.expectError { - t.Errorf("test %v failed: expected error", name) - } - } -} - func TestNodeHostsFileContent(t *testing.T) { testCases := []struct { hostsFileName string