PersistentLocalVolumes validation and tests

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

pkg/api/persistentvolume/util.go

@@ -28,7 +28,9 @@ func DropDisabledFields(pvSpec *api.PersistentVolumeSpec, oldPVSpec *api.Persist
 	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && !volumeModeInUse(oldPVSpec) {
 		pvSpec.VolumeMode = nil
 	}
-
+	if !utilfeature.DefaultFeatureGate.Enabled(features.PersistentLocalVolumes) && !persistentLocalVolumesInUse(oldPVSpec) {
+		pvSpec.PersistentVolumeSource.Local = nil
+	}
 	if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
 		// if this is a new PV, or the old PV didn't already have the CSI field, clear it
 		if oldPVSpec == nil || oldPVSpec.PersistentVolumeSource.CSI == nil {
@@ -46,3 +48,13 @@ func volumeModeInUse(oldPVSpec *api.PersistentVolumeSpec) bool {
 	}
 	return false
 }
+
+func persistentLocalVolumesInUse(oldPVSpec *api.PersistentVolumeSpec) bool {
+	if oldPVSpec == nil {
+		return false
+	}
+	if oldPVSpec.PersistentVolumeSource.Local != nil {
+		return true
+	}
+	return false
+}

pkg/api/persistentvolume/util_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package persistentvolume
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 
@@ -152,3 +153,99 @@ func TestDropDisabledFields(t *testing.T) {
 		})
 	}
 }
+
+func TestDropDisabledFieldsPersistentLocalVolume(t *testing.T) {
+	pvWithoutLocalVolume := func() *api.PersistentVolume {
+		return &api.PersistentVolume{
+			Spec: api.PersistentVolumeSpec{
+				PersistentVolumeSource: api.PersistentVolumeSource{
+					Local: nil,
+				},
+			},
+		}
+	}
+	pvWithLocalVolume := func() *api.PersistentVolume {
+		fsType := "ext4"
+		return &api.PersistentVolume{
+			Spec: api.PersistentVolumeSpec{
+				PersistentVolumeSource: api.PersistentVolumeSource{
+					Local: &api.LocalVolumeSource{
+						Path:   "/a/b/c",
+						FSType: &fsType,
+					},
+				},
+			},
+		}
+	}
+
+	pvInfo := []struct {
+		description    string
+		hasLocalVolume bool
+		pv             func() *api.PersistentVolume
+	}{
+		{
+			description:    "pv without LocalVolume",
+			hasLocalVolume: false,
+			pv:             pvWithoutLocalVolume,
+		},
+		{
+			description:    "pv with LocalVolume",
+			hasLocalVolume: true,
+			pv:             pvWithLocalVolume,
+		},
+		{
+			description:    "is nil",
+			hasLocalVolume: false,
+			pv:             func() *api.PersistentVolume { return nil },
+		},
+	}
+
+	for _, enabled := range []bool{true, false} {
+		for _, oldpvInfo := range pvInfo {
+			for _, newpvInfo := range pvInfo {
+				oldpvHasLocalVolume, oldpv := oldpvInfo.hasLocalVolume, oldpvInfo.pv()
+				newpvHasLocalVolume, newpv := newpvInfo.hasLocalVolume, newpvInfo.pv()
+				if newpv == nil {
+					continue
+				}
+
+				t.Run(fmt.Sprintf("feature enabled=%v, old pvc %v, new pvc %v", enabled, oldpvInfo.description, newpvInfo.description), func(t *testing.T) {
+					defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PersistentLocalVolumes, enabled)()
+
+					var oldpvSpec *api.PersistentVolumeSpec
+					if oldpv != nil {
+						oldpvSpec = &oldpv.Spec
+					}
+					DropDisabledFields(&newpv.Spec, oldpvSpec)
+
+					// old pv should never be changed
+					if !reflect.DeepEqual(oldpv, oldpvInfo.pv()) {
+						t.Errorf("old pv changed: %v", diff.ObjectReflectDiff(oldpv, oldpvInfo.pv()))
+					}
+
+					switch {
+					case enabled || oldpvHasLocalVolume:
+						// new pv should not be changed if the feature is enabled, or if the old pv had LocalVolume source
+						if !reflect.DeepEqual(newpv, newpvInfo.pv()) {
+							t.Errorf("new pv changed: %v", diff.ObjectReflectDiff(newpv, newpvInfo.pv()))
+						}
+					case newpvHasLocalVolume:
+						// new pv should be changed
+						if reflect.DeepEqual(newpv, newpvInfo.pv()) {
+							t.Errorf("new pv was not changed")
+						}
+						// new pv should not have LocalVolume
+						if !reflect.DeepEqual(newpv, pvWithoutLocalVolume()) {
+							t.Errorf("new pv had LocalVolume source: %v", diff.ObjectReflectDiff(newpv, pvWithoutLocalVolume()))
+						}
+					default:
+						// new pv should not need to be changed
+						if !reflect.DeepEqual(newpv, newpvInfo.pv()) {
+							t.Errorf("new pv changed: %v", diff.ObjectReflectDiff(newpv, newpvInfo.pv()))
+						}
+					}
+				})
+			}
+		}
+	}
+}

pkg/apis/core/validation/validation.go

@@ -1714,9 +1714,6 @@ func ValidatePersistentVolume(pv *core.PersistentVolume) field.ErrorList {
 			allErrs = append(allErrs, field.Forbidden(specPath.Child("local"), "may not specify more than 1 volume type"))
 		} else {
 			numVolumes++
-			if !utilfeature.DefaultFeatureGate.Enabled(features.PersistentLocalVolumes) {
-				allErrs = append(allErrs, field.Forbidden(specPath.Child("local"), "Local volumes are disabled by feature-gate"))
-			}
 			allErrs = append(allErrs, validateLocalVolumeSource(pv.Spec.Local, specPath.Child("local"))...)
 
 			// NodeAffinity is required

pkg/apis/core/validation/validation_test.go

@@ -553,32 +553,6 @@ func TestValidateLocalVolumes(t *testing.T) {
 	}
 }
 
-func TestValidateLocalVolumesDisabled(t *testing.T) {
-	scenarios := map[string]struct {
-		isExpectedFailure bool
-		volume            *core.PersistentVolume
-	}{
-		"feature disabled valid local volume": {
-			isExpectedFailure: true,
-			volume: testVolume("valid-local-volume", "",
-				testLocalVolume("/foo", simpleVolumeNodeAffinity("foo", "bar"))),
-		},
-	}
-
-	for name, scenario := range scenarios {
-		t.Run(name+" PersistentLocalVolumes disabled", func(t *testing.T) {
-			defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PersistentLocalVolumes, false)()
-			errs := ValidatePersistentVolume(scenario.volume)
-			if len(errs) == 0 && scenario.isExpectedFailure {
-				t.Errorf("Unexpected success for scenario: %s", name)
-			}
-			if len(errs) > 0 && !scenario.isExpectedFailure {
-				t.Errorf("Unexpected failure for scenario: %s - %+v", name, errs)
-			}
-		})
-	}
-}
-
 func testVolumeWithNodeAffinity(affinity *core.VolumeNodeAffinity) *core.PersistentVolume {
 	return testVolume("test-affinity-volume", "",
 		core.PersistentVolumeSpec{