Merge pull request kubernetes#70490 from liggitt/tolerate-existing-subpath

tolerate existing subpath on pod update when feature is disabled

Author: k8s-ci-robot

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",

pkg/api/pod/util.go

@@ -232,12 +232,16 @@ func UpdatePodCondition(status *api.PodStatus, condition *api.PodCondition) bool
 	return !isEqual
 }
 
-// DropDisabledAlphaFields removes disabled fields from the pod spec.
+// DropDisabledFields removes disabled fields from the pod spec.
 // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pod spec.
-func DropDisabledAlphaFields(podSpec *api.PodSpec) {
+func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) {
 		podSpec.Priority = nil
 		podSpec.PriorityClassName = ""
+		if oldPodSpec != nil {
+			oldPodSpec.Priority = nil
+			oldPodSpec.PriorityClassName = ""
+		}
 	}
 
 	if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) {
@@ -246,22 +250,46 @@ func DropDisabledAlphaFields(podSpec *api.PodSpec) {
 				podSpec.Volumes[i].EmptyDir.SizeLimit = nil
 			}
 		}
+		if oldPodSpec != nil {
+			for i := range oldPodSpec.Volumes {
+				if oldPodSpec.Volumes[i].EmptyDir != nil {
+					oldPodSpec.Volumes[i].EmptyDir.SizeLimit = nil
+				}
+			}
+		}
+	}
+
+	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)
+	dropDisabledVolumeDevicesAlphaFields(podSpec, oldPodSpec)
 
-	DropDisabledRunAsGroupField(podSpec)
+	dropDisabledRunAsGroupField(podSpec, oldPodSpec)
 
-	if !utilfeature.DefaultFeatureGate.Enabled(features.RuntimeClass) && podSpec.RuntimeClassName != nil {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.RuntimeClass) {
 		podSpec.RuntimeClassName = nil
+		if oldPodSpec != nil {
+			oldPodSpec.RuntimeClassName = nil
+		}
 	}
 
-	DropDisabledProcMountField(podSpec)
+	dropDisabledProcMountField(podSpec, oldPodSpec)
 }
 
-// DropDisabledRunAsGroupField removes disabled fields from PodSpec related
+// dropDisabledRunAsGroupField removes disabled fields from PodSpec related
 // to RunAsGroup
-func DropDisabledRunAsGroupField(podSpec *api.PodSpec) {
+func dropDisabledRunAsGroupField(podSpec, oldPodSpec *api.PodSpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.RunAsGroup) {
 		if podSpec.SecurityContext != nil {
 			podSpec.SecurityContext.RunAsGroup = nil
@@ -276,12 +304,28 @@ func DropDisabledRunAsGroupField(podSpec *api.PodSpec) {
 				podSpec.InitContainers[i].SecurityContext.RunAsGroup = nil
 			}
 		}
+
+		if oldPodSpec != nil {
+			if oldPodSpec.SecurityContext != nil {
+				oldPodSpec.SecurityContext.RunAsGroup = nil
+			}
+			for i := range oldPodSpec.Containers {
+				if oldPodSpec.Containers[i].SecurityContext != nil {
+					oldPodSpec.Containers[i].SecurityContext.RunAsGroup = nil
+				}
+			}
+			for i := range oldPodSpec.InitContainers {
+				if oldPodSpec.InitContainers[i].SecurityContext != nil {
+					oldPodSpec.InitContainers[i].SecurityContext.RunAsGroup = nil
+				}
+			}
+		}
 	}
 }
 
-// DropDisabledProcMountField removes disabled fields from PodSpec related
+// dropDisabledProcMountField removes disabled fields from PodSpec related
 // to ProcMount
-func DropDisabledProcMountField(podSpec *api.PodSpec) {
+func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.ProcMountType) {
 		defProcMount := api.DefaultProcMount
 		for i := range podSpec.Containers {
@@ -294,18 +338,62 @@ func DropDisabledProcMountField(podSpec *api.PodSpec) {
 				podSpec.InitContainers[i].SecurityContext.ProcMount = &defProcMount
 			}
 		}
+
+		if oldPodSpec != nil {
+			for i := range oldPodSpec.Containers {
+				if oldPodSpec.Containers[i].SecurityContext != nil {
+					oldPodSpec.Containers[i].SecurityContext.ProcMount = &defProcMount
+				}
+			}
+			for i := range oldPodSpec.InitContainers {
+				if oldPodSpec.InitContainers[i].SecurityContext != nil {
+					oldPodSpec.InitContainers[i].SecurityContext.ProcMount = &defProcMount
+				}
+			}
+		}
 	}
 }
 
-// DropDisabledVolumeDevicesAlphaFields removes disabled fields from []VolumeDevice.
+// dropDisabledVolumeDevicesAlphaFields removes disabled fields from []VolumeDevice.
 // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a VolumeDevice
-func DropDisabledVolumeDevicesAlphaFields(podSpec *api.PodSpec) {
+func dropDisabledVolumeDevicesAlphaFields(podSpec, oldPodSpec *api.PodSpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
 		for i := range podSpec.Containers {
 			podSpec.Containers[i].VolumeDevices = nil
 		}
 		for i := range podSpec.InitContainers {
 			podSpec.InitContainers[i].VolumeDevices = nil
 		}
+
+		if oldPodSpec != nil {
+			for i := range oldPodSpec.Containers {
+				oldPodSpec.Containers[i].VolumeDevices = nil
+			}
+			for i := range oldPodSpec.InitContainers {
+				oldPodSpec.InitContainers[i].VolumeDevices = nil
+			}
+		}
+	}
+}
+
+// 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
 }

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"
@@ -307,7 +309,7 @@ func TestDropAlphaVolumeDevices(t *testing.T) {
 	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, true)()
 
 	// now test dropping the fields - should not be dropped
-	DropDisabledAlphaFields(&testPod.Spec)
+	DropDisabledFields(&testPod.Spec, nil)
 
 	// check to make sure VolumeDevices is still present
 	// if featureset is set to true
@@ -322,14 +324,108 @@ func TestDropAlphaVolumeDevices(t *testing.T) {
 	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, false)()
 
 	// now test dropping the fields
-	DropDisabledAlphaFields(&testPod.Spec)
+	DropDisabledFields(&testPod.Spec, nil)
 
 	// check to make sure VolumeDevices is nil
 	// if featureset is set to false
 	if testPod.Spec.Containers[0].VolumeDevices != nil {
-		t.Error("DropDisabledAlphaFields for Containers failed")
+		t.Error("DropDisabledFields for Containers failed")
 	}
 	if testPod.Spec.InitContainers[0].VolumeDevices != nil {
-		t.Error("DropDisabledAlphaFields for InitContainers failed")
+		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()))
+						}
+					}
+				})
+			}
+		}
 	}
 }

pkg/apis/core/validation/validation.go

@@ -2299,11 +2299,7 @@ func ValidateVolumeMounts(mounts []core.VolumeMount, voldevices map[string]strin
 		}
 
 		if len(mnt.SubPath) > 0 {
-			if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) {
-				allErrs = append(allErrs, field.Forbidden(fldPath.Child("subPath"), "subPath is disabled by feature-gate"))
-			} else {
-				allErrs = append(allErrs, validateLocalDescendingPath(mnt.SubPath, fldPath.Child("subPath"))...)
-			}
+			allErrs = append(allErrs, validateLocalDescendingPath(mnt.SubPath, fldPath.Child("subPath"))...)
 		}
 
 		if mnt.MountPropagation != nil {

pkg/apis/core/validation/validation_test.go

@@ -4931,7 +4931,7 @@ func TestValidateDisabledSubpath(t *testing.T) {
 					SubPath:   "baz",
 				},
 			},
-			true,
+			false, // validation should not fail, dropping the field is handled in PrepareForCreate/PrepareForUpdate
 		},
 	}
 

pkg/registry/apps/daemonset/strategy.go

@@ -75,16 +75,15 @@ func (daemonSetStrategy) PrepareForCreate(ctx context.Context, obj runtime.Objec
 		daemonSet.Spec.TemplateGeneration = 1
 	}
 
-	pod.DropDisabledAlphaFields(&daemonSet.Spec.Template.Spec)
+	pod.DropDisabledFields(&daemonSet.Spec.Template.Spec, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
 func (daemonSetStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
 	newDaemonSet := obj.(*apps.DaemonSet)
 	oldDaemonSet := old.(*apps.DaemonSet)
 
-	pod.DropDisabledAlphaFields(&newDaemonSet.Spec.Template.Spec)
-	pod.DropDisabledAlphaFields(&oldDaemonSet.Spec.Template.Spec)
+	pod.DropDisabledFields(&newDaemonSet.Spec.Template.Spec, &oldDaemonSet.Spec.Template.Spec)
 
 	// update is not allowed to set status
 	newDaemonSet.Status = oldDaemonSet.Status

pkg/registry/apps/deployment/strategy.go

@@ -73,7 +73,7 @@ func (deploymentStrategy) PrepareForCreate(ctx context.Context, obj runtime.Obje
 	deployment.Status = apps.DeploymentStatus{}
 	deployment.Generation = 1
 
-	pod.DropDisabledAlphaFields(&deployment.Spec.Template.Spec)
+	pod.DropDisabledFields(&deployment.Spec.Template.Spec, nil)
 }
 
 // Validate validates a new deployment.
@@ -97,8 +97,7 @@ func (deploymentStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime
 	oldDeployment := old.(*apps.Deployment)
 	newDeployment.Status = oldDeployment.Status
 
-	pod.DropDisabledAlphaFields(&newDeployment.Spec.Template.Spec)
-	pod.DropDisabledAlphaFields(&oldDeployment.Spec.Template.Spec)
+	pod.DropDisabledFields(&newDeployment.Spec.Template.Spec, &oldDeployment.Spec.Template.Spec)
 
 	// Spec updates bump the generation so that we can distinguish between
 	// scaling events and template changes, annotation updates bump the generation

pkg/registry/apps/replicaset/strategy.go

@@ -80,7 +80,7 @@ func (rsStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
 
 	rs.Generation = 1
 
-	pod.DropDisabledAlphaFields(&rs.Spec.Template.Spec)
+	pod.DropDisabledFields(&rs.Spec.Template.Spec, nil)
 }
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
@@ -90,8 +90,7 @@ func (rsStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object)
 	// update is not allowed to set status
 	newRS.Status = oldRS.Status
 
-	pod.DropDisabledAlphaFields(&newRS.Spec.Template.Spec)
-	pod.DropDisabledAlphaFields(&oldRS.Spec.Template.Spec)
+	pod.DropDisabledFields(&newRS.Spec.Template.Spec, &oldRS.Spec.Template.Spec)
 
 	// Any changes to the spec increment the generation number, any changes to the
 	// status should reflect the generation number of the corresponding object. We push