VolumeDevices validation and tests

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

pkg/api/pod/util.go

@@ -264,7 +264,7 @@ func DropDisabledFields(podSpec, oldPodSpec *api.PodSpec) {
 		}
 	}
 
-	dropDisabledVolumeDevicesAlphaFields(podSpec, oldPodSpec)
+	dropDisabledVolumeDevicesFields(podSpec, oldPodSpec)
 
 	dropDisabledRunAsGroupField(podSpec, oldPodSpec)
 
@@ -330,25 +330,16 @@ func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) {
 	}
 }
 
-// dropDisabledVolumeDevicesAlphaFields removes disabled fields from []VolumeDevice.
+// dropDisabledVolumeDevicesFields removes disabled fields from []VolumeDevice if it has not been already populated.
 // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a VolumeDevice
-func dropDisabledVolumeDevicesAlphaFields(podSpec, oldPodSpec *api.PodSpec) {
+func dropDisabledVolumeDevicesFields(podSpec, oldPodSpec *api.PodSpec) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && !volumeDevicesInUse(oldPodSpec) {
 		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
-			}
-		}
 	}
 }
 
@@ -436,3 +427,21 @@ func emptyDirSizeLimitInUse(podSpec *api.PodSpec) bool {
 	}
 	return false
 }
+
+// volumeDevicesInUse returns true if the pod spec is non-nil and has VolumeDevices set.
+func volumeDevicesInUse(podSpec *api.PodSpec) bool {
+	if podSpec == nil {
+		return false
+	}
+	for i := range podSpec.Containers {
+		if podSpec.Containers[i].VolumeDevices != nil {
+			return true
+		}
+	}
+	for i := range podSpec.InitContainers {
+		if podSpec.InitContainers[i].VolumeDevices != nil {
+			return true
+		}
+	}
+	return false
+}

pkg/api/pod/util_test.go

@@ -266,74 +266,146 @@ func TestPodConfigmaps(t *testing.T) {
 }
 
 func TestDropAlphaVolumeDevices(t *testing.T) {
-	testPod := api.Pod{
+	podWithVolumeDevices := func() *api.Pod {
-		Spec: api.PodSpec{
+		return &api.Pod{
-			RestartPolicy: api.RestartPolicyNever,
+			Spec: api.PodSpec{
-			Containers: []api.Container{
+				RestartPolicy: api.RestartPolicyNever,
-				{
+				Containers: []api.Container{
-					Name:  "container1",
+					{
-					Image: "testimage",
+						Name:  "container1",
-					VolumeDevices: []api.VolumeDevice{
+						Image: "testimage",
-						{
+						VolumeDevices: []api.VolumeDevice{
-							Name:       "myvolume",
+							{
-							DevicePath: "/usr/test",
+								Name:       "myvolume",
+								DevicePath: "/usr/test",
+							},
+						},
+					},
+				},
+				InitContainers: []api.Container{
+					{
+						Name:  "container1",
+						Image: "testimage",
+						VolumeDevices: []api.VolumeDevice{
+							{
+								Name:       "myvolume",
+								DevicePath: "/usr/test",
+							},
+						},
+					},
+				},
+				Volumes: []api.Volume{
+					{
+						Name: "myvolume",
+						VolumeSource: api.VolumeSource{
+							HostPath: &api.HostPathVolumeSource{
+								Path: "/dev/xvdc",
+							},
 						},
 					},
 				},
 			},
-			InitContainers: []api.Container{
+		}
-				{
+	}
-					Name:  "container1",
+	podWithoutVolumeDevices := func() *api.Pod {
-					Image: "testimage",
+		return &api.Pod{
-					VolumeDevices: []api.VolumeDevice{
+			Spec: api.PodSpec{
-						{
+				RestartPolicy: api.RestartPolicyNever,
-							Name:       "myvolume",
+				Containers: []api.Container{
-							DevicePath: "/usr/test",
+					{
-						},
+						Name:  "container1",
-					},
+						Image: "testimage",
-				},
+					},
-			},
+				},
-			Volumes: []api.Volume{
+				InitContainers: []api.Container{
-				{
+					{
-					Name: "myvolume",
+						Name:  "container1",
-					VolumeSource: api.VolumeSource{
+						Image: "testimage",
-						HostPath: &api.HostPathVolumeSource{
+					},
-							Path: "/dev/xvdc",
+				},
+				Volumes: []api.Volume{
+					{
+						Name: "myvolume",
+						VolumeSource: api.VolumeSource{
+							HostPath: &api.HostPathVolumeSource{
+								Path: "/dev/xvdc",
+							},
 						},
 					},
 				},
 			},
+		}
+	}
+
+	podInfo := []struct {
+		description      string
+		hasVolumeDevices bool
+		pod              func() *api.Pod
+	}{
+		{
+			description:      "has VolumeDevices",
+			hasVolumeDevices: true,
+			pod:              podWithVolumeDevices,
+		},
+		{
+			description:      "does not have VolumeDevices",
+			hasVolumeDevices: false,
+			pod:              podWithoutVolumeDevices,
+		},
+		{
+			description:      "is nil",
+			hasVolumeDevices: false,
+			pod:              func() *api.Pod { return nil },
 		},
 	}
 
-	// Enable alpha feature BlockVolume
+	for _, enabled := range []bool{true, false} {
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, true)()
+		for _, oldPodInfo := range podInfo {
+			for _, newPodInfo := range podInfo {
+				oldPodHasVolumeDevices, oldPod := oldPodInfo.hasVolumeDevices, oldPodInfo.pod()
+				newPodHasVolumeDevices, newPod := newPodInfo.hasVolumeDevices, newPodInfo.pod()
+				if newPod == nil {
+					continue
+				}
 
-	// now test dropping the fields - should not be dropped
+				t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
-	DropDisabledFields(&testPod.Spec, nil)
+					defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, enabled)()
 
-	// check to make sure VolumeDevices is still present
+					var oldPodSpec *api.PodSpec
-	// if featureset is set to true
+					if oldPod != nil {
-	if testPod.Spec.Containers[0].VolumeDevices == nil {
+						oldPodSpec = &oldPod.Spec
-		t.Error("VolumeDevices in Container should not have been dropped based on feature-gate")
+					}
-	}
+					DropDisabledFields(&newPod.Spec, oldPodSpec)
-	if testPod.Spec.InitContainers[0].VolumeDevices == nil {
-		t.Error("VolumeDevices in InitContainers should not have been dropped based on feature-gate")
-	}
 
-	// Disable alpha feature BlockVolume
+					// old pod should never be changed
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, false)()
+					if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
+						t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod()))
+					}
 
-	// now test dropping the fields
+					switch {
-	DropDisabledFields(&testPod.Spec, nil)
+					case enabled || oldPodHasVolumeDevices:
-
+						// new pod should not be changed if the feature is enabled, or if the old pod had VolumeDevices
-	// check to make sure VolumeDevices is nil
+						if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
-	// if featureset is set to false
+							t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
-	if testPod.Spec.Containers[0].VolumeDevices != nil {
+						}
-		t.Error("DropDisabledFields for Containers failed")
+					case newPodHasVolumeDevices:
-	}
+						// new pod should be changed
-	if testPod.Spec.InitContainers[0].VolumeDevices != nil {
+						if reflect.DeepEqual(newPod, newPodInfo.pod()) {
-		t.Error("DropDisabledFields for InitContainers failed")
+							t.Errorf("new pod was not changed")
+						}
+						// new pod should not have VolumeDevices
+						if !reflect.DeepEqual(newPod, podWithoutVolumeDevices()) {
+							t.Errorf("new pod had VolumeDevices: %v", diff.ObjectReflectDiff(newPod, podWithoutVolumeDevices()))
+						}
+					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

@@ -2298,10 +2298,6 @@ func ValidateVolumeDevices(devices []core.VolumeDevice, volmounts map[string]str
 	devicepath := sets.NewString()
 	devicename := sets.NewString()
 
-	if devices != nil && !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
-		allErrs = append(allErrs, field.Forbidden(fldPath.Child("volumeDevices"), "Container volumeDevices is disabled by feature-gate"))
-		return allErrs
-	}
 	if devices != nil {
 		for i, dev := range devices {
 			idxPath := fldPath.Index(i)

pkg/apis/core/validation/validation_test.go

@@ -4974,10 +4974,6 @@ func TestAlphaValidateVolumeDevices(t *testing.T) {
 		return
 	}
 
-	disabledAlphaVolDevice := []core.VolumeDevice{
-		{Name: "abc", DevicePath: "/foo"},
-	}
-
 	successCase := []core.VolumeDevice{
 		{Name: "abc", DevicePath: "/foo"},
 		{Name: "abc-123", DevicePath: "/usr/share/test"},
@@ -5005,8 +5001,6 @@ func TestAlphaValidateVolumeDevices(t *testing.T) {
 		{Name: "abc-123", MountPath: "/this/path/exists"},
 	}
 
-	// enable BlockVolume
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, true)()
 	// Success Cases:
 	// Validate normal success cases - only PVC volumeSource
 	if errs := ValidateVolumeDevices(successCase, GetVolumeMountMap(goodVolumeMounts), vols, field.NewPath("field")); len(errs) != 0 {
@@ -5020,12 +5014,6 @@ func TestAlphaValidateVolumeDevices(t *testing.T) {
 			t.Errorf("expected failure for %s", k)
 		}
 	}
-
-	// disable BlockVolume
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, false)()
-	if errs := ValidateVolumeDevices(disabledAlphaVolDevice, GetVolumeMountMap(goodVolumeMounts), vols, field.NewPath("field")); len(errs) == 0 {
-		t.Errorf("expected failure: %v", errs)
-	}
 }
 
 func TestValidateProbe(t *testing.T) {