From 123f1bac35a45ac26bcaed9d51f4f3d56e28a752 Mon Sep 17 00:00:00 2001 From: j-griffith Date: Thu, 11 Apr 2019 13:45:03 -0600 Subject: [PATCH 1/6] Enable PVC as DataSource for PVC creation This enables the ability to specify and existing PVC as a DataSource in a new PVC Spec (eg "clone" and existing volume). --- pkg/api/persistentvolumeclaim/util.go | 18 ++++++++- pkg/apis/core/types.go | 13 +++--- pkg/apis/core/validation/validation.go | 1 + pkg/apis/core/validation/validation_test.go | 45 +++++++++++++++++++++ 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go index 0c7116de5ec..095c1d82192 100644 --- a/pkg/api/persistentvolumeclaim/util.go +++ b/pkg/api/persistentvolumeclaim/util.go @@ -28,7 +28,7 @@ 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) { + if !dataSourceIsEnabled(pvcSpec) && !dataSourceInUse(oldPVCSpec) { pvcSpec.DataSource = nil } } @@ -43,7 +43,7 @@ func volumeModeInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool { return false } -func volumeSnapshotDataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool { +func dataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool { if oldPVCSpec == nil { return false } @@ -52,3 +52,17 @@ func volumeSnapshotDataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) b } return false } + +func dataSourceIsEnabled(pvcSpec *core.PersistentVolumeClaimSpec) bool { + if pvcSpec.DataSource != nil { + if pvcSpec.DataSource.Kind == "PersistentVolumeClaim" && utilfeature.DefaultFeatureGate.Enabled(features.VolumeDataSource) { + return true + + } + if pvcSpec.DataSource.Kind == "VolumeSnapshot" && utilfeature.DefaultFeatureGate.Enabled(features.VolumeSnapshotDataSource) { + return true + + } + } + return false +} diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 1f0ab9dea2d..978de4ce1ee 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -413,11 +413,14 @@ type PersistentVolumeClaimSpec struct { // This is a beta feature. // +optional VolumeMode *PersistentVolumeMode - // This field requires the VolumeSnapshotDataSource alpha feature gate to be - // enabled and currently VolumeSnapshot is the only supported data source. - // If the provisioner can support VolumeSnapshot data source, it will create - // a new volume and data will be restored to the volume at the same time. - // If the provisioner does not support VolumeSnapshot data source, volume will + // This field can be used to specify either: + // * An existing VolumeSnapshot + // * An existing Volume (PVC, Clone operation) + // In order to use either of these DataSource types, the appropriate feature gate + // must be enabled (VolumeSnapshotDataSource, VolumeDataSource) + // 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 // not be created and the failure will be reported as an event. // In the future, we plan to support more data source types and the behavior // of the provisioner may change. diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index ead5c29c9cf..b64184c5fba 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -1537,6 +1537,7 @@ var supportedVolumeModes = sets.NewString(string(core.PersistentVolumeBlock), st var supportedDataSourceAPIGroupKinds = map[schema.GroupKind]bool{ {Group: "snapshot.storage.k8s.io", Kind: "VolumeSnapshot"}: true, + {Group: "", Kind: "PersistentVolumeClaim"}: true, } func ValidatePersistentVolume(pv *core.PersistentVolume) field.ErrorList { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 07ef601f50b..2a06e283740 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -13288,3 +13288,48 @@ func TestValidateWindowsSecurityContextOptions(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) + } + } +} From a66bb3c63d3b99bd31ff30615d5d54c3f31309fd Mon Sep 17 00:00:00 2001 From: j-griffith Date: Mon, 22 Apr 2019 16:22:19 -0600 Subject: [PATCH 2/6] 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: From ae4c2a18589453ff91d7a3c94192967541d595d1 Mon Sep 17 00:00:00 2001 From: j-griffith Date: Thu, 11 Apr 2019 13:45:03 -0600 Subject: [PATCH 3/6] Enable PVC as DataSource for PVC creation This enables the ability to specify and existing PVC as a DataSource in a new PVC Spec (eg "clone" and existing volume). --- pkg/api/persistentvolumeclaim/util.go | 1 - pkg/apis/core/validation/validation_test.go | 45 +++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go index 3612881107c..2fbb207a32e 100644 --- a/pkg/api/persistentvolumeclaim/util.go +++ b/pkg/api/persistentvolumeclaim/util.go @@ -65,7 +65,6 @@ func dataSourceIsEnabled(pvcSpec *core.PersistentVolumeClaimSpec) bool { *pvcSpec.DataSource.APIGroup == "snapshot.storage.k8s.io" && utilfeature.DefaultFeatureGate.Enabled(features.VolumeSnapshotDataSource) { return true - } } return false diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 4e4cf4dbdb6..66258259f5b 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -13333,3 +13333,48 @@ 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) + } + } +} From 54154f8ebbeb3c1b71e468c805313de0659f977c Mon Sep 17 00:00:00 2001 From: j-griffith Date: Fri, 17 May 2019 14:30:22 -0600 Subject: [PATCH 4/6] rework pvc datasource filter tests --- pkg/api/persistentvolumeclaim/util_test.go | 187 +++++++------------- pkg/apis/core/validation/validation_test.go | 45 ----- 2 files changed, 61 insertions(+), 171 deletions(-) diff --git a/pkg/api/persistentvolumeclaim/util_test.go b/pkg/api/persistentvolumeclaim/util_test.go index 171dc56e3f7..b5c310964d9 100644 --- a/pkg/api/persistentvolumeclaim/util_test.go +++ b/pkg/api/persistentvolumeclaim/util_test.go @@ -211,138 +211,73 @@ func TestDropDisabledSnapshotDataSource(t *testing.T) { } } -func TestDropDisabledPVCDataSource(t *testing.T) { - pvcWithoutDataSource := func() *core.PersistentVolumeClaim { - return &core.PersistentVolumeClaim{ - Spec: core.PersistentVolumeClaimSpec{ - DataSource: nil, - }, - } - } +// TestPVCDataSourceSpecFilter checks to ensure the DropDisabledFields function behaves correctly for PVCDataSource featuregate +func TestPVCDataSourceSpecFilter(t *testing.T) { 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, + validSpec := core.PersistentVolumeClaimSpec{ + DataSource: &core.TypedLocalObjectReference{ + APIGroup: &apiGroup, + Kind: "PersistentVolumeClaim", + Name: "test_clone", }, - { - 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) + invalidSpec := core.PersistentVolumeClaimSpec{ + DataSource: &core.TypedLocalObjectReference{ + APIGroup: &invalidAPIGroup, + Kind: "PersistentVolumeClaim", + Name: "test_clone_invalid", + }, } - 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) + var tests = map[string]struct { + spec core.PersistentVolumeClaimSpec + gateEnabled bool + want *core.TypedLocalObjectReference + }{ + "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) + } + + }) + + } + } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 66258259f5b..4e4cf4dbdb6 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/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) - } - } -} From 60d991e59a86a413191cabb7495701e58ed34900 Mon Sep 17 00:00:00 2001 From: j-griffith Date: Thu, 30 May 2019 16:28:09 -0600 Subject: [PATCH 5/6] add comments to validation testcases, and use const in util.go --- pkg/api/persistentvolumeclaim/util.go | 9 +++- pkg/apis/core/validation/validation_test.go | 56 +++++++++++++++------ 2 files changed, 47 insertions(+), 18 deletions(-) diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go index 2fbb207a32e..34de30e65d5 100644 --- a/pkg/api/persistentvolumeclaim/util.go +++ b/pkg/api/persistentvolumeclaim/util.go @@ -22,6 +22,11 @@ import ( "k8s.io/kubernetes/pkg/features" ) +const ( + pvc string = "PersistentVolumeClaim" + volumeSnapshot string = "VolumeSnapshot" +) + // 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) { @@ -55,13 +60,13 @@ func dataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool { func dataSourceIsEnabled(pvcSpec *core.PersistentVolumeClaimSpec) bool { if pvcSpec.DataSource != nil { - if pvcSpec.DataSource.Kind == "PersistentVolumeClaim" && + if pvcSpec.DataSource.Kind == pvc && *pvcSpec.DataSource.APIGroup == "" && utilfeature.DefaultFeatureGate.Enabled(features.VolumePVCDataSource) { return true } - if pvcSpec.DataSource.Kind == "VolumeSnapshot" && + if pvcSpec.DataSource.Kind == volumeSnapshot && *pvcSpec.DataSource.APIGroup == "snapshot.storage.k8s.io" && utilfeature.DefaultFeatureGate.Enabled(features.VolumeSnapshotDataSource) { return true diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 4e4cf4dbdb6..d12862a89bd 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -13312,24 +13312,48 @@ func testDataSourceInSpec(name string, kind string, apiGroup string) *core.Persi } func TestAlphaVolumePVCDataSource(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"), + + testCases := []struct { + testName string + claimSpec core.PersistentVolumeClaimSpec + expectedFail bool + }{ + { + testName: "test create from valid snapshot source", + claimSpec: *testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "snapshot.storage.k8s.io"), + }, + { + testName: "test create from valid pvc source", + claimSpec: *testDataSourceInSpec("test_pvc", "PersistentVolumeClaim", ""), + }, + { + testName: "test missing name in snapshot datasource should fail", + claimSpec: *testDataSourceInSpec("", "VolumeSnapshot", "snapshot.storage.k8s.io"), + expectedFail: true, + }, + { + testName: "test specifying pvc with snapshot api group should fail", + claimSpec: *testDataSourceInSpec("test_snapshot", "PersistentVolumeClaim", "snapshot.storage.k8s.io"), + expectedFail: true, + }, + { + testName: "test invalid group name in snapshot datasource should fail", + claimSpec: *testDataSourceInSpec("test_snapshot", "VolumeSnapshot", "storage.k8s.io"), + expectedFail: true, + }, } - 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) + for _, tc := range testCases { + if tc.expectedFail { + if errs := ValidatePersistentVolumeClaimSpec(&tc.claimSpec, field.NewPath("spec")); len(errs) == 0 { + t.Errorf("expected failure: %v", errs) + } + + } else { + if errs := ValidatePersistentVolumeClaimSpec(&tc.claimSpec, field.NewPath("spec")); len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } + } } } From 62a4861c9bfa0aa7b422eb29ba21c09cbb20f87e Mon Sep 17 00:00:00 2001 From: j-griffith Date: Thu, 30 May 2019 16:42:59 -0600 Subject: [PATCH 6/6] fix typo in types.go comment --- pkg/apis/core/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 408f7daeec3..497b9924c63 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -415,7 +415,7 @@ type PersistentVolumeClaimSpec struct { VolumeMode *PersistentVolumeMode // This field can be used to specify either: // * An existing VolumeSnapshot object (snapshot.storage.k8s.io/VolumeSnapshot) - // * An existing PVC (""/PersistentVolumeClaim) + // * An existing PVC (PersistentVolumeClaim) // In order to use either of these DataSource types, the appropriate feature gate // must be enabled (VolumeSnapshotDataSource, VolumePVCDataSource) // If the provisioner can support the specified data source, it will create