From a66bb3c63d3b99bd31ff30615d5d54c3f31309fd Mon Sep 17 00:00:00 2001 From: j-griffith Date: Mon, 22 Apr 2019 16:22:19 -0600 Subject: [PATCH] Update unit tests and feature name Update the unit tests to include checks for incorrect APIGroup type in PVC DataSource and change the name of the feature gate to be more clear: s/VolumeDataSource/VolumePVCDataSource/ --- pkg/api/persistentvolumeclaim/util.go | 8 +- pkg/api/persistentvolumeclaim/util_test.go | 138 +++++++++++++++++++- pkg/apis/core/types.go | 6 +- pkg/apis/core/validation/validation_test.go | 2 +- pkg/features/kube_features.go | 7 + 5 files changed, 154 insertions(+), 7 deletions(-) diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go index 095c1d82192..3612881107c 100644 --- a/pkg/api/persistentvolumeclaim/util.go +++ b/pkg/api/persistentvolumeclaim/util.go @@ -55,11 +55,15 @@ func dataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool { func dataSourceIsEnabled(pvcSpec *core.PersistentVolumeClaimSpec) bool { if pvcSpec.DataSource != nil { - if pvcSpec.DataSource.Kind == "PersistentVolumeClaim" && utilfeature.DefaultFeatureGate.Enabled(features.VolumeDataSource) { + if pvcSpec.DataSource.Kind == "PersistentVolumeClaim" && + *pvcSpec.DataSource.APIGroup == "" && + utilfeature.DefaultFeatureGate.Enabled(features.VolumePVCDataSource) { return true } - if pvcSpec.DataSource.Kind == "VolumeSnapshot" && utilfeature.DefaultFeatureGate.Enabled(features.VolumeSnapshotDataSource) { + if pvcSpec.DataSource.Kind == "VolumeSnapshot" && + *pvcSpec.DataSource.APIGroup == "snapshot.storage.k8s.io" && + utilfeature.DefaultFeatureGate.Enabled(features.VolumeSnapshotDataSource) { return true } diff --git a/pkg/api/persistentvolumeclaim/util_test.go b/pkg/api/persistentvolumeclaim/util_test.go index e970dce4c88..171dc56e3f7 100644 --- a/pkg/api/persistentvolumeclaim/util_test.go +++ b/pkg/api/persistentvolumeclaim/util_test.go @@ -118,7 +118,7 @@ func TestDropAlphaPVCVolumeMode(t *testing.T) { } } -func TestDropDisabledDataSource(t *testing.T) { +func TestDropDisabledSnapshotDataSource(t *testing.T) { pvcWithoutDataSource := func() *core.PersistentVolumeClaim { return &core.PersistentVolumeClaim{ Spec: core.PersistentVolumeClaimSpec{ @@ -210,3 +210,139 @@ func TestDropDisabledDataSource(t *testing.T) { } } } + +func TestDropDisabledPVCDataSource(t *testing.T) { + pvcWithoutDataSource := func() *core.PersistentVolumeClaim { + return &core.PersistentVolumeClaim{ + Spec: core.PersistentVolumeClaimSpec{ + DataSource: nil, + }, + } + } + apiGroup := "" + pvcWithDataSource := func() *core.PersistentVolumeClaim { + 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 { + 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() + originalDS = invalidPvc.Spec.DataSource + DropDisabledFields(&invalidPvc.Spec, nil) + if invalidPvc.Spec.DataSource != nil { + t.Errorf("invalid PVC DataSource was not dropped: Kind: %s, APIGroup: %s, Name: %s\n", originalDS.Kind, *originalDS.APIGroup, originalDS.Name) + } +} diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 978de4ce1ee..408f7daeec3 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -414,10 +414,10 @@ type PersistentVolumeClaimSpec struct { // +optional VolumeMode *PersistentVolumeMode // This field can be used to specify either: - // * An existing VolumeSnapshot - // * An existing Volume (PVC, Clone operation) + // * An existing VolumeSnapshot object (snapshot.storage.k8s.io/VolumeSnapshot) + // * An existing PVC (""/PersistentVolumeClaim) // In order to use either of these DataSource types, the appropriate feature gate - // must be enabled (VolumeSnapshotDataSource, VolumeDataSource) + // must be enabled (VolumeSnapshotDataSource, VolumePVCDataSource) // If the provisioner can support the specified data source, it will create // a new volume based on the contents of the specified PVC or Snapshot. // If the provisioner does not support the specified data source, the volume will diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 2a06e283740..4e4cf4dbdb6 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -13311,7 +13311,7 @@ func testDataSourceInSpec(name string, kind string, apiGroup string) *core.Persi return &dataSourceInSpec } -func TestAlphaVolumeDataSource(t *testing.T) { +func TestAlphaVolumePVCDataSource(t *testing.T) { successTestCases := []core.PersistentVolumeClaimSpec{ *testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "snapshot.storage.k8s.io"), *testDataSourceInSpec("test_pvc", "PersistentVolumeClaim", ""), diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 20fc26e2cb8..0dd42697f04 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -465,6 +465,12 @@ const ( // // Enables NonPreempting option for priorityClass and pod. NonPreemptingPriority featuregate.Feature = "NonPreemptingPriority" + + // owner: @j-griffith + // alpha: v1.15 + // + // Enable support for specifying an existing PVC as a DataSource + VolumePVCDataSource featuregate.Feature = "VolumePVCDataSource" ) func init() { @@ -543,6 +549,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS WindowsGMSA: {Default: false, PreRelease: featuregate.Alpha}, LocalStorageCapacityIsolationFSQuotaMonitoring: {Default: false, PreRelease: featuregate.Alpha}, NonPreemptingPriority: {Default: false, PreRelease: featuregate.Alpha}, + VolumePVCDataSource: {Default: false, PreRelease: featuregate.Alpha}, // inherited features from generic apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: