Add DataSourceRef field to PVC spec

Modify the behavior of the AnyVolumeDataSource alpha feature gate to enable
a new field, DataSourceRef, rather than modifying the behavior of the
existing DataSource field. This allows addition Volume Populators in a way
that doesn't risk breaking backwards compatibility, although it will
result in eventually deprecating the DataSource field.

pkg/api/persistentvolumeclaim/util.go

@@ -29,8 +29,47 @@ const (
 
 // DropDisabledFields removes disabled fields from the pvc spec.
 // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pvc spec.
-func DropDisabledFields(pvcSpec, oldPVCSpec *core.PersistentVolumeClaimSpec) {
-	if !dataSourceIsEnabled(pvcSpec) && !dataSourceInUse(oldPVCSpec) {
+func DropDisabledFields(pvcSpec *core.PersistentVolumeClaimSpec) {
+	// Drop the contents of the dataSourceRef field if the AnyVolumeDataSource
+	// feature gate is disabled.
+	if !utilfeature.DefaultFeatureGate.Enabled(features.AnyVolumeDataSource) {
+		pvcSpec.DataSourceRef = nil
+	}
+}
+
+// EnforceDataSourceBackwardsCompatibility drops the data source field under certain conditions
+// to maintain backwards compatibility with old behavior.
+// See KEP 1495 for details.
+// Specifically, if this is an update of a PVC with no data source, or a creation of a new PVC,
+// and the dataSourceRef field is not filled in, then we will drop "invalid" data sources
+// (anything other than a PVC or a VolumeSnapshot) from this request as if an empty PVC had
+// been requested.
+// This should be called after DropDisabledFields so that if the AnyVolumeDataSource feature
+// gate is disabled, dataSourceRef will be forced to empty, ensuring pre-1.22 behavior.
+// This should be called before NormalizeDataSources, so that data sources other than PVCs
+// and VolumeSnapshots can only be set through the dataSourceRef field and not the dataSource
+// field.
+func EnforceDataSourceBackwardsCompatibility(pvcSpec, oldPVCSpec *core.PersistentVolumeClaimSpec) {
+	// Check if the old PVC has a data source here is so that on updates from old clients
+	// that omit dataSourceRef, we preserve the data source, even if it would have been
+	// invalid to specify it at using the dataSource field at create.
+	if dataSourceInUse(oldPVCSpec) {
+		return
+	}
+
+	// Check if dataSourceRef is empty is because if it's not empty, then there is
+	// definitely a newer client and it definitely either wants to create a non-empty
+	// volume, or it wants to update a PVC that has a data source. Whether the
+	// specified data source is valid or satisfiable is a matter for validation and
+	// the volume populator code, but we can say with certainty that the client is
+	// not expecting the legacy behavior of ignoring invalid data sources.
+	if pvcSpec.DataSourceRef != nil {
+		return
+	}
+
+	// Historically, we only allow PVCs and VolumeSnapshots in the dataSource field.
+	// All other values are silently dropped.
+	if !dataSourceIsPvcOrSnapshot(pvcSpec.DataSource) {
 		pvcSpec.DataSource = nil
 	}
 }
@@ -39,31 +78,43 @@ func dataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool {
 	if oldPVCSpec == nil {
 		return false
 	}
-	if oldPVCSpec.DataSource != nil {
+	if oldPVCSpec.DataSource != nil || oldPVCSpec.DataSourceRef != nil {
 		return true
 	}
 	return false
 }
 
-func dataSourceIsEnabled(pvcSpec *core.PersistentVolumeClaimSpec) bool {
-	if pvcSpec.DataSource != nil {
-		if utilfeature.DefaultFeatureGate.Enabled(features.AnyVolumeDataSource) {
-			return true
-		}
-
+func dataSourceIsPvcOrSnapshot(dataSource *core.TypedLocalObjectReference) bool {
+	if dataSource != nil {
 		apiGroup := ""
-		if pvcSpec.DataSource.APIGroup != nil {
-			apiGroup = *pvcSpec.DataSource.APIGroup
+		if dataSource.APIGroup != nil {
+			apiGroup = *dataSource.APIGroup
 		}
-		if pvcSpec.DataSource.Kind == pvc &&
+		if dataSource.Kind == pvc &&
 			apiGroup == "" {
 			return true
-
 		}
 
-		if pvcSpec.DataSource.Kind == volumeSnapshot && apiGroup == "snapshot.storage.k8s.io" {
+		if dataSource.Kind == volumeSnapshot && apiGroup == "snapshot.storage.k8s.io" {
 			return true
 		}
 	}
 	return false
 }
+
+// NormalizeDataSources ensures that DataSource and DataSourceRef have the same contents
+// as long as both are not explicitly set.
+// This should be used by creates/gets of PVCs, but not updates
+func NormalizeDataSources(pvcSpec *core.PersistentVolumeClaimSpec) {
+	// Don't enable this behavior if the feature gate is not on
+	if !utilfeature.DefaultFeatureGate.Enabled(features.AnyVolumeDataSource) {
+		return
+	}
+	if pvcSpec.DataSource != nil && pvcSpec.DataSourceRef == nil {
+		// Using the old way of setting a data source
+		pvcSpec.DataSourceRef = pvcSpec.DataSource.DeepCopy()
+	} else if pvcSpec.DataSourceRef != nil && pvcSpec.DataSource == nil {
+		// Using the new way of setting a data source
+		pvcSpec.DataSource = pvcSpec.DataSourceRef.DeepCopy()
+	}
+}

pkg/api/persistentvolumeclaim/util_test.go

@@ -68,9 +68,6 @@ func TestDropDisabledSnapshotDataSource(t *testing.T) {
 		},
 	}
 
-	// Ensure that any data sources aren't enabled for this test
-	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AnyVolumeDataSource, false)()
-
 	for _, oldpvcInfo := range pvcInfo {
 		for _, newpvcInfo := range pvcInfo {
 			oldpvc := oldpvcInfo.pvc()
@@ -80,11 +77,7 @@ func TestDropDisabledSnapshotDataSource(t *testing.T) {
 			}
 
 			t.Run(fmt.Sprintf("old pvc %v, new pvc %v", oldpvcInfo.description, newpvcInfo.description), func(t *testing.T) {
-				var oldpvcSpec *core.PersistentVolumeClaimSpec
-				if oldpvc != nil {
-					oldpvcSpec = &oldpvc.Spec
-				}
-				DropDisabledFields(&newpvc.Spec, oldpvcSpec)
+				EnforceDataSourceBackwardsCompatibility(&newpvc.Spec, nil)
 
 				// old pvc should never be changed
 				if !reflect.DeepEqual(oldpvc, oldpvcInfo.pvc()) {
@@ -148,20 +141,15 @@ func TestPVCDataSourceSpecFilter(t *testing.T) {
 		},
 	}
 
-	// Ensure that any data sources aren't enabled for this test
-	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AnyVolumeDataSource, false)()
-
 	for testName, test := range tests {
 		t.Run(testName, func(t *testing.T) {
-			DropDisabledFields(&test.spec, nil)
+			EnforceDataSourceBackwardsCompatibility(&test.spec, nil)
 			if test.spec.DataSource != test.want {
 				t.Errorf("expected drop datasource condition was not met, test: %s, spec: %v, expected: %v", testName, test.spec, test.want)
 			}
 
 		})
-
 	}
-
 }
 
 // TestAnyDataSourceFilter checks to ensure the AnyVolumeDataSource feature gate works
@@ -175,59 +163,127 @@ func TestAnyDataSourceFilter(t *testing.T) {
 	}
 
 	volumeDataSource := makeDataSource("", "PersistentVolumeClaim", "my-vol")
-	snapshotDataSource := makeDataSource("snapshot.storage.k8s.io", "VolumeSnapshot", "my-snap")
-	genericDataSource := makeDataSource("generic.storage.k8s.io", "Generic", "my-foo")
 
 	var tests = map[string]struct {
 		spec       core.PersistentVolumeClaimSpec
 		anyEnabled bool
 		want       *core.TypedLocalObjectReference
+		wantRef    *core.TypedLocalObjectReference
 	}{
 		"any disabled with empty ds": {
 			spec: core.PersistentVolumeClaimSpec{},
-			want: nil,
 		},
 		"any disabled with volume ds": {
 			spec: core.PersistentVolumeClaimSpec{DataSource: volumeDataSource},
 			want: volumeDataSource,
 		},
-		"any disabled with snapshot ds": {
-			spec: core.PersistentVolumeClaimSpec{DataSource: snapshotDataSource},
-			want: snapshotDataSource,
+		"any disabled with volume ds ref": {
+			spec: core.PersistentVolumeClaimSpec{DataSourceRef: volumeDataSource},
 		},
-		"any disabled with generic ds": {
-			spec: core.PersistentVolumeClaimSpec{DataSource: genericDataSource},
-			want: nil,
+		"any disabled with both data sources": {
+			spec: core.PersistentVolumeClaimSpec{DataSource: volumeDataSource, DataSourceRef: volumeDataSource},
+			want: volumeDataSource,
 		},
 		"any enabled with empty ds": {
 			spec:       core.PersistentVolumeClaimSpec{},
 			anyEnabled: true,
-			want:       nil,
 		},
 		"any enabled with volume ds": {
 			spec:       core.PersistentVolumeClaimSpec{DataSource: volumeDataSource},
 			anyEnabled: true,
 			want:       volumeDataSource,
 		},
-		"any enabled with snapshot ds": {
-			spec:       core.PersistentVolumeClaimSpec{DataSource: snapshotDataSource},
+		"any enabled with volume ds ref": {
+			spec:       core.PersistentVolumeClaimSpec{DataSourceRef: volumeDataSource},
 			anyEnabled: true,
-			want:       snapshotDataSource,
+			wantRef:    volumeDataSource,
 		},
-		"any enabled with generic ds": {
-			spec:       core.PersistentVolumeClaimSpec{DataSource: genericDataSource},
+		"any enabled with both data sources": {
+			spec:       core.PersistentVolumeClaimSpec{DataSource: volumeDataSource, DataSourceRef: volumeDataSource},
 			anyEnabled: true,
-			want:       genericDataSource,
+			want:       volumeDataSource,
+			wantRef:    volumeDataSource,
 		},
 	}
 
 	for testName, test := range tests {
 		t.Run(testName, func(t *testing.T) {
 			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AnyVolumeDataSource, test.anyEnabled)()
-			DropDisabledFields(&test.spec, nil)
-			if test.spec.DataSource != test.want {
-				t.Errorf("expected condition was not met, test: %s, anyEnabled: %v, spec: %v, expected: %v",
-					testName, test.anyEnabled, test.spec, test.want)
+			DropDisabledFields(&test.spec)
+			if test.spec.DataSource != test.want || test.spec.DataSourceRef != test.wantRef {
+				t.Errorf("expected condition was not met, test: %s, anyEnabled: %v, spec: %v, expected: %v %v",
+					testName, test.anyEnabled, test.spec, test.want, test.wantRef)
+			}
+		})
+	}
+}
+
+// TestDataSourceRef checks to ensure the DataSourceRef field handles backwards
+// compatibility with the DataSource field
+func TestDataSourceRef(t *testing.T) {
+	makeDataSource := func(apiGroup, kind, name string) *core.TypedLocalObjectReference {
+		return &core.TypedLocalObjectReference{
+			APIGroup: &apiGroup,
+			Kind:     kind,
+			Name:     name,
+		}
+	}
+
+	volumeDataSource := makeDataSource("", "PersistentVolumeClaim", "my-vol")
+	snapshotDataSource := makeDataSource("snapshot.storage.k8s.io", "VolumeSnapshot", "my-snap")
+	genericDataSource := makeDataSource("generic.storage.k8s.io", "Generic", "my-foo")
+	coreDataSource := makeDataSource("", "Pod", "my-pod")
+
+	var tests = map[string]struct {
+		spec core.PersistentVolumeClaimSpec
+		want *core.TypedLocalObjectReference
+	}{
+		"empty ds": {
+			spec: core.PersistentVolumeClaimSpec{},
+		},
+		"volume ds": {
+			spec: core.PersistentVolumeClaimSpec{DataSource: volumeDataSource},
+			want: volumeDataSource,
+		},
+		"snapshot ds": {
+			spec: core.PersistentVolumeClaimSpec{DataSource: snapshotDataSource},
+			want: snapshotDataSource,
+		},
+		"generic ds": {
+			spec: core.PersistentVolumeClaimSpec{DataSource: genericDataSource},
+			want: genericDataSource,
+		},
+		"core ds": {
+			spec: core.PersistentVolumeClaimSpec{DataSource: coreDataSource},
+			want: coreDataSource,
+		},
+		"volume ds ref": {
+			spec: core.PersistentVolumeClaimSpec{DataSourceRef: volumeDataSource},
+			want: volumeDataSource,
+		},
+		"snapshot ds ref": {
+			spec: core.PersistentVolumeClaimSpec{DataSourceRef: snapshotDataSource},
+			want: snapshotDataSource,
+		},
+		"generic ds ref": {
+			spec: core.PersistentVolumeClaimSpec{DataSourceRef: genericDataSource},
+			want: genericDataSource,
+		},
+		"core ds ref": {
+			spec: core.PersistentVolumeClaimSpec{DataSourceRef: coreDataSource},
+			want: coreDataSource,
+		},
+	}
+
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AnyVolumeDataSource, true)()
+
+	for testName, test := range tests {
+		t.Run(testName, func(t *testing.T) {
+			NormalizeDataSources(&test.spec)
+			if !reflect.DeepEqual(test.spec.DataSource, test.want) ||
+				!reflect.DeepEqual(test.spec.DataSourceRef, test.want) {
+				t.Errorf("expected condition was not met, test: %s, spec: %v, expected: %v",
+					testName, test.spec, test.want)
 			}
 		})
 	}