rework pvc datasource filter tests

pkg/api/persistentvolumeclaim/util_test.go

@@ -211,138 +211,73 @@ func TestDropDisabledSnapshotDataSource(t *testing.T) {
 	}
 }
 
-func TestDropDisabledPVCDataSource(t *testing.T) {
+// TestPVCDataSourceSpecFilter checks to ensure the DropDisabledFields function behaves correctly for PVCDataSource featuregate
-	pvcWithoutDataSource := func() *core.PersistentVolumeClaim {
+func TestPVCDataSourceSpecFilter(t *testing.T) {
-		return &core.PersistentVolumeClaim{
-			Spec: core.PersistentVolumeClaimSpec{
-				DataSource: nil,
-			},
-		}
-	}
 	apiGroup := ""
-	pvcWithDataSource := func() *core.PersistentVolumeClaim {
+	validSpec := core.PersistentVolumeClaimSpec{
-		return &core.PersistentVolumeClaim{
-			Spec: core.PersistentVolumeClaimSpec{
 		DataSource: &core.TypedLocalObjectReference{
 			APIGroup: &apiGroup,
 			Kind:     "PersistentVolumeClaim",
 			Name:     "test_clone",
 		},
-			},
-		}
-	}
-
-	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.VolumePVCDataSource, 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()))
-						}
-					}
-				})
-			}
-		}
-	}
-}
-
-// TestDropInvalidPVCDataSource checks specifically for invalid APIGroup settings for PVCDataSource
-func TestDropInvalidPVCDataSource(t *testing.T) {
-	apiGroup := ""
-	pvcWithDataSource := func() *core.PersistentVolumeClaim {
-		return &core.PersistentVolumeClaim{
-			Spec: core.PersistentVolumeClaimSpec{
-				DataSource: &core.TypedLocalObjectReference{
-					APIGroup: &apiGroup,
-					Kind:     "PersistentVolumeClaim",
-					Name:     "test_clone",
-				},
-			},
-		}
 	}
 
 	invalidAPIGroup := "invalid.pvc.api.group"
-	pvcWithInvalidDataSource := func() *core.PersistentVolumeClaim {
+	invalidSpec := core.PersistentVolumeClaimSpec{
-		return &core.PersistentVolumeClaim{
-			Spec: core.PersistentVolumeClaimSpec{
 		DataSource: &core.TypedLocalObjectReference{
 			APIGroup: &invalidAPIGroup,
 			Kind:     "PersistentVolumeClaim",
 			Name:     "test_clone_invalid",
 		},
-			},
-		}
-	}
-	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumePVCDataSource, true)()
-	validPvc := pvcWithDataSource()
-	originalDS := validPvc.Spec.DataSource
-	DropDisabledFields(&validPvc.Spec, nil)
-	if validPvc.Spec.DataSource == nil {
-		t.Errorf("valid PVC DataSource was dropped: Kind: %s, APIGroup: %s, Name: %s\n", originalDS.Kind, *originalDS.APIGroup, originalDS.Name)
 	}
 
-	invalidPvc := pvcWithInvalidDataSource()
+	var tests = map[string]struct {
-	originalDS = invalidPvc.Spec.DataSource
+		spec        core.PersistentVolumeClaimSpec
-	DropDisabledFields(&invalidPvc.Spec, nil)
+		gateEnabled bool
-	if invalidPvc.Spec.DataSource != nil {
+		want        *core.TypedLocalObjectReference
-		t.Errorf("invalid PVC DataSource was not dropped: Kind: %s, APIGroup: %s, Name: %s\n", originalDS.Kind, *originalDS.APIGroup, originalDS.Name)
+	}{
+		"enabled with empty ds": {
+			spec:        core.PersistentVolumeClaimSpec{},
+			gateEnabled: true,
+			want:        nil,
+		},
+		"enabled with invalid spec": {
+			spec:        invalidSpec,
+			gateEnabled: true,
+			want:        nil,
+		},
+		"enabled with valid spec": {
+			spec:        validSpec,
+			gateEnabled: true,
+			want:        validSpec.DataSource,
+		},
+		"disabled with invalid spec": {
+			spec:        invalidSpec,
+			gateEnabled: false,
+			want:        nil,
+		},
+		"disabled with valid spec": {
+			spec:        validSpec,
+			gateEnabled: false,
+			want:        nil,
+		},
+		"diabled with empty ds": {
+			spec:        core.PersistentVolumeClaimSpec{},
+			gateEnabled: false,
+			want:        nil,
+		},
 	}
+
+	for testName, test := range tests {
+		t.Run(testName, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumePVCDataSource, test.gateEnabled)()
+			DropDisabledFields(&test.spec, nil)
+			if test.spec.DataSource != test.want {
+				t.Errorf("expected drop datasource condition was not met, test: %s, gateEnabled: %v, spec: %v, expected: %v", testName, test.gateEnabled, test.spec, test.want)
+			}
+
+		})
+
+	}
+
 }

pkg/apis/core/validation/validation_test.go

@@ -13333,48 +13333,3 @@ func TestAlphaVolumePVCDataSource(t *testing.T) {
 		}
 	}
 }
-
-func testDataSourceInSpec(name string, kind string, apiGroup string) *core.PersistentVolumeClaimSpec {
-	scName := "csi-plugin"
-	dataSourceInSpec := core.PersistentVolumeClaimSpec{
-		AccessModes: []core.PersistentVolumeAccessMode{
-			core.ReadOnlyMany,
-		},
-		Resources: core.ResourceRequirements{
-			Requests: core.ResourceList{
-				core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
-			},
-		},
-		StorageClassName: &scName,
-		DataSource: &core.TypedLocalObjectReference{
-			APIGroup: &apiGroup,
-			Kind:     kind,
-			Name:     name,
-		},
-	}
-
-	return &dataSourceInSpec
-}
-
-func TestAlphaVolumeDataSource(t *testing.T) {
-	successTestCases := []core.PersistentVolumeClaimSpec{
-		*testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "snapshot.storage.k8s.io"),
-		*testDataSourceInSpec("test_pvc", "PersistentVolumeClaim", ""),
-	}
-	failedTestCases := []core.PersistentVolumeClaimSpec{
-		*testDataSourceInSpec("", "VolumeSnapshot", "snapshot.storage.k8s.io"),
-		*testDataSourceInSpec("test_snapshot", "PersistentVolumeClaim", "snapshot.storage.k8s.io"),
-		*testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "storage.k8s.io"),
-	}
-
-	for _, tc := range successTestCases {
-		if errs := ValidatePersistentVolumeClaimSpec(&tc, field.NewPath("spec")); len(errs) != 0 {
-			t.Errorf("expected success: %v", errs)
-		}
-	}
-	for _, tc := range failedTestCases {
-		if errs := ValidatePersistentVolumeClaimSpec(&tc, field.NewPath("spec")); len(errs) == 0 {
-			t.Errorf("expected failure: %v", errs)
-		}
-	}
-}