From 082f23ab1b648c5015a7969f8d14907c79c99da7 Mon Sep 17 00:00:00 2001 From: Serguei Bezverkhi Date: Thu, 20 Dec 2018 13:51:53 -0500 Subject: [PATCH] AllowVolumeExpansion validation and tests Signed-off-by: Serguei Bezverkhi --- pkg/apis/storage/util/BUILD | 1 + pkg/apis/storage/util/util.go | 13 +++ pkg/apis/storage/util/util_test.go | 81 +++++++++++++++++++ pkg/apis/storage/validation/validation.go | 11 --- .../storage/validation/validation_test.go | 26 ------ pkg/registry/storage/storageclass/BUILD | 2 - pkg/registry/storage/storageclass/strategy.go | 10 --- 7 files changed, 95 insertions(+), 49 deletions(-) diff --git a/pkg/apis/storage/util/BUILD b/pkg/apis/storage/util/BUILD index 1b37c7f631d..629d7cfa2fe 100644 --- a/pkg/apis/storage/util/BUILD +++ b/pkg/apis/storage/util/BUILD @@ -42,6 +42,7 @@ go_test( "//pkg/apis/core:go_default_library", "//pkg/apis/storage:go_default_library", "//pkg/features:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature/testing:go_default_library", ], diff --git a/pkg/apis/storage/util/util.go b/pkg/apis/storage/util/util.go index f4321ad6999..0eaf9a8f70c 100644 --- a/pkg/apis/storage/util/util.go +++ b/pkg/apis/storage/util/util.go @@ -32,4 +32,17 @@ func DropDisabledFields(class, oldClass *storage.StorageClass) { oldClass.AllowedTopologies = nil } } + if !utilfeature.DefaultFeatureGate.Enabled(features.ExpandPersistentVolumes) && !allowVolumeExpansionInUse(oldClass) { + class.AllowVolumeExpansion = nil + } +} + +func allowVolumeExpansionInUse(oldClass *storage.StorageClass) bool { + if oldClass == nil { + return false + } + if oldClass.AllowVolumeExpansion != nil { + return true + } + return false } diff --git a/pkg/apis/storage/util/util_test.go b/pkg/apis/storage/util/util_test.go index 43dada5bb81..5fe8820d87b 100644 --- a/pkg/apis/storage/util/util_test.go +++ b/pkg/apis/storage/util/util_test.go @@ -17,9 +17,11 @@ limitations under the License. package util import ( + "fmt" "reflect" "testing" + "k8s.io/apimachinery/pkg/util/diff" utilfeature "k8s.io/apiserver/pkg/util/feature" utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing" api "k8s.io/kubernetes/pkg/apis/core" @@ -68,3 +70,82 @@ func TestDropAlphaFields(t *testing.T) { t.Errorf("AllowedTopologies field got unexpectantly modified: %+v", class.AllowedTopologies) } } + +func TestDropAllowVolumeExpansion(t *testing.T) { + allowVolumeExpansion := false + scWithoutAllowVolumeExpansion := func() *storage.StorageClass { + return &storage.StorageClass{} + } + scWithAllowVolumeExpansion := func() *storage.StorageClass { + return &storage.StorageClass{ + AllowVolumeExpansion: &allowVolumeExpansion, + } + } + + scInfo := []struct { + description string + hasAllowVolumeExpansion bool + sc func() *storage.StorageClass + }{ + { + description: "StorageClass Without AllowVolumeExpansion", + hasAllowVolumeExpansion: false, + sc: scWithoutAllowVolumeExpansion, + }, + { + description: "StorageClass With AllowVolumeExpansion", + hasAllowVolumeExpansion: true, + sc: scWithAllowVolumeExpansion, + }, + { + description: "is nil", + hasAllowVolumeExpansion: false, + sc: func() *storage.StorageClass { return nil }, + }, + } + + for _, enabled := range []bool{true, false} { + for _, oldStorageClassInfo := range scInfo { + for _, newStorageClassInfo := range scInfo { + oldStorageClassHasAllowVolumeExpansion, oldStorageClass := oldStorageClassInfo.hasAllowVolumeExpansion, oldStorageClassInfo.sc() + newStorageClassHasAllowVolumeExpansion, newStorageClass := newStorageClassInfo.hasAllowVolumeExpansion, newStorageClassInfo.sc() + if newStorageClass == nil { + continue + } + + t.Run(fmt.Sprintf("feature enabled=%v, old StorageClass %v, new StorageClass %v", enabled, oldStorageClassInfo.description, newStorageClassInfo.description), func(t *testing.T) { + defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ExpandPersistentVolumes, enabled)() + + DropDisabledFields(newStorageClass, oldStorageClass) + + // old StorageClass should never be changed + if !reflect.DeepEqual(oldStorageClass, oldStorageClassInfo.sc()) { + t.Errorf("old StorageClass changed: %v", diff.ObjectReflectDiff(oldStorageClass, oldStorageClassInfo.sc())) + } + + switch { + case enabled || oldStorageClassHasAllowVolumeExpansion: + // new StorageClass should not be changed if the feature is enabled, or if the old StorageClass had AllowVolumeExpansion + if !reflect.DeepEqual(newStorageClass, newStorageClassInfo.sc()) { + t.Errorf("new StorageClass changed: %v", diff.ObjectReflectDiff(newStorageClass, newStorageClassInfo.sc())) + } + case newStorageClassHasAllowVolumeExpansion: + // new StorageClass should be changed + if reflect.DeepEqual(newStorageClass, newStorageClassInfo.sc()) { + t.Errorf("new StorageClass was not changed") + } + // new StorageClass should not have AllowVolumeExpansion + if !reflect.DeepEqual(newStorageClass, scWithoutAllowVolumeExpansion()) { + t.Errorf("new StorageClass had StorageClassAllowVolumeExpansion: %v", diff.ObjectReflectDiff(newStorageClass, scWithoutAllowVolumeExpansion())) + } + default: + // new StorageClass should not need to be changed + if !reflect.DeepEqual(newStorageClass, newStorageClassInfo.sc()) { + t.Errorf("new StorageClass changed: %v", diff.ObjectReflectDiff(newStorageClass, newStorageClassInfo.sc())) + } + } + }) + } + } + } +} diff --git a/pkg/apis/storage/validation/validation.go b/pkg/apis/storage/validation/validation.go index 48d9ecde476..7a738186f03 100644 --- a/pkg/apis/storage/validation/validation.go +++ b/pkg/apis/storage/validation/validation.go @@ -46,7 +46,6 @@ func ValidateStorageClass(storageClass *storage.StorageClass) field.ErrorList { allErrs = append(allErrs, validateProvisioner(storageClass.Provisioner, field.NewPath("provisioner"))...) allErrs = append(allErrs, validateParameters(storageClass.Parameters, field.NewPath("parameters"))...) allErrs = append(allErrs, validateReclaimPolicy(storageClass.ReclaimPolicy, field.NewPath("reclaimPolicy"))...) - allErrs = append(allErrs, validateAllowVolumeExpansion(storageClass.AllowVolumeExpansion, field.NewPath("allowVolumeExpansion"))...) allErrs = append(allErrs, validateVolumeBindingMode(storageClass.VolumeBindingMode, field.NewPath("volumeBindingMode"))...) allErrs = append(allErrs, validateAllowedTopologies(storageClass.AllowedTopologies, field.NewPath("allowedTopologies"))...) @@ -123,16 +122,6 @@ func validateReclaimPolicy(reclaimPolicy *api.PersistentVolumeReclaimPolicy, fld return allErrs } -// validateAllowVolumeExpansion tests that if ExpandPersistentVolumes feature gate is disabled, whether the AllowVolumeExpansion filed -// of storage class is set -func validateAllowVolumeExpansion(allowExpand *bool, fldPath *field.Path) field.ErrorList { - allErrs := field.ErrorList{} - if allowExpand != nil && !utilfeature.DefaultFeatureGate.Enabled(features.ExpandPersistentVolumes) { - allErrs = append(allErrs, field.Forbidden(fldPath, "field is disabled by feature-gate ExpandPersistentVolumes")) - } - return allErrs -} - // ValidateVolumeAttachment validates a VolumeAttachment. This function is common for v1 and v1beta1 objects, func ValidateVolumeAttachment(volumeAttachment *storage.VolumeAttachment) field.ErrorList { allErrs := apivalidation.ValidateObjectMeta(&volumeAttachment.ObjectMeta, false, apivalidation.ValidateClassName, field.NewPath("metadata")) diff --git a/pkg/apis/storage/validation/validation_test.go b/pkg/apis/storage/validation/validation_test.go index d9940300c98..b72d0db89df 100644 --- a/pkg/apis/storage/validation/validation_test.go +++ b/pkg/apis/storage/validation/validation_test.go @@ -140,32 +140,6 @@ func TestValidateStorageClass(t *testing.T) { } } -func TestAlphaExpandPersistentVolumesFeatureValidation(t *testing.T) { - deleteReclaimPolicy := api.PersistentVolumeReclaimPolicy("Delete") - falseVar := false - testSC := &storage.StorageClass{ - // empty parameters - ObjectMeta: metav1.ObjectMeta{Name: "foo"}, - Provisioner: "kubernetes.io/foo-provisioner", - Parameters: map[string]string{}, - ReclaimPolicy: &deleteReclaimPolicy, - AllowVolumeExpansion: &falseVar, - VolumeBindingMode: &immediateMode1, - } - - // Enable feature ExpandPersistentVolumes - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ExpandPersistentVolumes, true)() - if errs := ValidateStorageClass(testSC); len(errs) != 0 { - t.Errorf("expected success: %v", errs) - } - // Disable feature ExpandPersistentVolumes - defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ExpandPersistentVolumes, false)() - if errs := ValidateStorageClass(testSC); len(errs) == 0 { - t.Errorf("expected failure, but got no error") - } - -} - func TestVolumeAttachmentValidation(t *testing.T) { volumeName := "pv-name" empty := "" diff --git a/pkg/registry/storage/storageclass/BUILD b/pkg/registry/storage/storageclass/BUILD index 31ab232a746..6dc98c39d42 100644 --- a/pkg/registry/storage/storageclass/BUILD +++ b/pkg/registry/storage/storageclass/BUILD @@ -18,11 +18,9 @@ go_library( "//pkg/apis/storage:go_default_library", "//pkg/apis/storage/util:go_default_library", "//pkg/apis/storage/validation:go_default_library", - "//pkg/features:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", ], ) diff --git a/pkg/registry/storage/storageclass/strategy.go b/pkg/registry/storage/storageclass/strategy.go index d9b17bebfe3..58744efeb43 100644 --- a/pkg/registry/storage/storageclass/strategy.go +++ b/pkg/registry/storage/storageclass/strategy.go @@ -22,12 +22,10 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/storage/names" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" "k8s.io/kubernetes/pkg/apis/storage" storageutil "k8s.io/kubernetes/pkg/apis/storage/util" "k8s.io/kubernetes/pkg/apis/storage/validation" - "k8s.io/kubernetes/pkg/features" ) // storageClassStrategy implements behavior for StorageClass objects @@ -48,10 +46,6 @@ func (storageClassStrategy) NamespaceScoped() bool { func (storageClassStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { class := obj.(*storage.StorageClass) - if !utilfeature.DefaultFeatureGate.Enabled(features.ExpandPersistentVolumes) { - class.AllowVolumeExpansion = nil - } - storageutil.DropDisabledFields(class, nil) } @@ -73,10 +67,6 @@ func (storageClassStrategy) PrepareForUpdate(ctx context.Context, obj, old runti newClass := obj.(*storage.StorageClass) oldClass := old.(*storage.StorageClass) - if !utilfeature.DefaultFeatureGate.Enabled(features.ExpandPersistentVolumes) { - newClass.AllowVolumeExpansion = nil - oldClass.AllowVolumeExpansion = nil - } storageutil.DropDisabledFields(oldClass, newClass) }