Move VolumeSnapshotDataSource feature gate check from validation

pkg/api/persistentvolumeclaim/util.go

@@ -28,6 +28,9 @@ func DropDisabledFields(pvcSpec, oldPVCSpec *core.PersistentVolumeClaimSpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && !volumeModeInUse(oldPVCSpec) {
 		pvcSpec.VolumeMode = nil
 	}
+	if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSnapshotDataSource) && !volumeSnapshotDataSourceInUse(oldPVCSpec) {
+		pvcSpec.DataSource = nil
+	}
 }
 
 func volumeModeInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool {
@@ -39,3 +42,13 @@ func volumeModeInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool {
 	}
 	return false
 }
+
+func volumeSnapshotDataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool {
+	if oldPVCSpec == nil {
+		return false
+	}
+	if oldPVCSpec.DataSource != nil {
+		return true
+	}
+	return false
+}

pkg/api/persistentvolumeclaim/util_test.go

@@ -117,3 +117,96 @@ func TestDropAlphaPVCVolumeMode(t *testing.T) {
 		}
 	}
 }
+
+func TestDropDisabledDataSource(t *testing.T) {
+	pvcWithoutDataSource := func() *core.PersistentVolumeClaim {
+		return &core.PersistentVolumeClaim{
+			Spec: core.PersistentVolumeClaimSpec{
+				DataSource: nil,
+			},
+		}
+	}
+	apiGroup := "snapshot.storage.k8s.io"
+	pvcWithDataSource := func() *core.PersistentVolumeClaim {
+		return &core.PersistentVolumeClaim{
+			Spec: core.PersistentVolumeClaimSpec{
+				DataSource: &core.TypedLocalObjectReference{
+					APIGroup: &apiGroup,
+					Kind:     "VolumeSnapshot",
+					Name:     "test_snapshot",
+				},
+			},
+		}
+	}
+
+	pvcInfo := []struct {
+		description   string
+		hasDataSource bool
+		pvc           func() *core.PersistentVolumeClaim
+	}{
+		{
+			description:   "pvc without DataSource",
+			hasDataSource: false,
+			pvc:           pvcWithoutDataSource,
+		},
+		{
+			description:   "pvc with DataSource",
+			hasDataSource: true,
+			pvc:           pvcWithDataSource,
+		},
+		{
+			description:   "is nil",
+			hasDataSource: false,
+			pvc:           func() *core.PersistentVolumeClaim { return nil },
+		},
+	}
+
+	for _, enabled := range []bool{true, false} {
+		for _, oldpvcInfo := range pvcInfo {
+			for _, newpvcInfo := range pvcInfo {
+				oldPvcHasDataSource, oldpvc := oldpvcInfo.hasDataSource, oldpvcInfo.pvc()
+				newPvcHasDataSource, newpvc := newpvcInfo.hasDataSource, newpvcInfo.pvc()
+				if newpvc == nil {
+					continue
+				}
+
+				t.Run(fmt.Sprintf("feature enabled=%v, old pvc %v, new pvc %v", enabled, oldpvcInfo.description, newpvcInfo.description), func(t *testing.T) {
+					defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSnapshotDataSource, enabled)()
+
+					var oldpvcSpec *core.PersistentVolumeClaimSpec
+					if oldpvc != nil {
+						oldpvcSpec = &oldpvc.Spec
+					}
+					DropDisabledFields(&newpvc.Spec, oldpvcSpec)
+
+					// old pvc should never be changed
+					if !reflect.DeepEqual(oldpvc, oldpvcInfo.pvc()) {
+						t.Errorf("old pvc changed: %v", diff.ObjectReflectDiff(oldpvc, oldpvcInfo.pvc()))
+					}
+
+					switch {
+					case enabled || oldPvcHasDataSource:
+						// new pvc should not be changed if the feature is enabled, or if the old pvc had DataSource
+						if !reflect.DeepEqual(newpvc, newpvcInfo.pvc()) {
+							t.Errorf("new pvc changed: %v", diff.ObjectReflectDiff(newpvc, newpvcInfo.pvc()))
+						}
+					case newPvcHasDataSource:
+						// new pvc should be changed
+						if reflect.DeepEqual(newpvc, newpvcInfo.pvc()) {
+							t.Errorf("new pvc was not changed")
+						}
+						// new pvc should not have DataSource
+						if !reflect.DeepEqual(newpvc, pvcWithoutDataSource()) {
+							t.Errorf("new pvc had DataSource: %v", diff.ObjectReflectDiff(newpvc, pvcWithoutDataSource()))
+						}
+					default:
+						// new pvc should not need to be changed
+						if !reflect.DeepEqual(newpvc, newpvcInfo.pvc()) {
+							t.Errorf("new pvc changed: %v", diff.ObjectReflectDiff(newpvc, newpvcInfo.pvc()))
+						}
+					}
+				})
+			}
+		}
+	}
+}

pkg/apis/core/validation/validation.go

@@ -1834,9 +1834,7 @@ func ValidatePersistentVolumeClaimSpec(spec *core.PersistentVolumeClaimSpec, fld
 		allErrs = append(allErrs, field.NotSupported(fldPath.Child("volumeMode"), *spec.VolumeMode, supportedVolumeModes.List()))
 	}
 
-	if spec.DataSource != nil && !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSnapshotDataSource) {
+	if spec.DataSource != nil {
-		allErrs = append(allErrs, field.Forbidden(fldPath.Child("dataSource"), "VolumeSnapshotDataSource is disabled by feature-gate"))
-	} else if spec.DataSource != nil {
 		if len(spec.DataSource.Name) == 0 {
 			allErrs = append(allErrs, field.Required(fldPath.Child("dataSource", "name"), ""))
 		}

pkg/apis/core/validation/validation_test.go

@@ -749,8 +749,6 @@ func TestAlphaVolumeSnapshotDataSource(t *testing.T) {
 		*testVolumeSnapshotDataSourceInSpec("test_snapshot", "VolumeSnapshot", "storage.k8s.io"),
 	}
 
-	// Enable alpha feature VolumeSnapshotDataSource
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSnapshotDataSource, true)()
 	for _, tc := range successTestCases {
 		if errs := ValidatePersistentVolumeClaimSpec(&tc, field.NewPath("spec")); len(errs) != 0 {
 			t.Errorf("expected success: %v", errs)
@@ -761,13 +759,6 @@ func TestAlphaVolumeSnapshotDataSource(t *testing.T) {
 			t.Errorf("expected failure: %v", errs)
 		}
 	}
-	// Disable alpha feature VolumeSnapshotDataSource
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSnapshotDataSource, false)()
-	for _, tc := range successTestCases {
-		if errs := ValidatePersistentVolumeClaimSpec(&tc, field.NewPath("spec")); len(errs) == 0 {
-			t.Errorf("expected failure: %v", errs)
-		}
-	}
 }
 
 func testVolumeClaimStorageClassInAnnotationAndSpec(name, namespace, scNameInAnn, scName string, spec core.PersistentVolumeClaimSpec) *core.PersistentVolumeClaim {