diff --git a/pkg/api/pod/BUILD b/pkg/api/pod/BUILD index 2da65ee0c57..00c07c3b549 100644 --- a/pkg/api/pod/BUILD +++ b/pkg/api/pod/BUILD @@ -38,6 +38,7 @@ go_test( deps = [ "//pkg/apis/core:go_default_library", "//pkg/features:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/diff: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/api/pod/util.go b/pkg/api/pod/util.go index 8f9a50cc8a7..b57d9145511 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -259,6 +259,20 @@ func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) { } } + 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 + for i := range podSpec.Containers { + for j := range podSpec.Containers[i].VolumeMounts { + podSpec.Containers[i].VolumeMounts[j].SubPath = "" + } + } + for i := range podSpec.InitContainers { + for j := range podSpec.InitContainers[i].VolumeMounts { + podSpec.InitContainers[i].VolumeMounts[j].SubPath = "" + } + } + } + dropDisabledVolumeDevicesAlphaFields(podSpec, oldPodSpec) dropDisabledRunAsGroupField(podSpec, oldPodSpec) @@ -361,3 +375,25 @@ func dropDisabledVolumeDevicesAlphaFields(podSpec, oldPodSpec *api.PodSpec) { } } } + +// 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 + } + for i := range podSpec.Containers { + for j := range podSpec.Containers[i].VolumeMounts { + if len(podSpec.Containers[i].VolumeMounts[j].SubPath) > 0 { + return true + } + } + } + for i := range podSpec.InitContainers { + for j := range podSpec.InitContainers[i].VolumeMounts { + if len(podSpec.InitContainers[i].VolumeMounts[j].SubPath) > 0 { + return true + } + } + } + return false +} diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 264e8451cf5..112a61a2cc0 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -17,10 +17,12 @@ limitations under the License. package pod import ( + "fmt" "reflect" "strings" "testing" + "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -333,3 +335,97 @@ func TestDropAlphaVolumeDevices(t *testing.T) { t.Error("DropDisabledFields for InitContainers failed") } } + +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 utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpath, enabled)() + + var oldPodSpec *api.PodSpec + if oldPod != nil { + oldPodSpec = &oldPod.Spec + } + DropDisabledFields(&newPod.Spec, oldPodSpec) + + // 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 || 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", diff.ObjectReflectDiff(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", diff.ObjectReflectDiff(newPod, podWithoutSubpaths())) + } + 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())) + } + } + }) + } + } + } +}