AllowVolumeExpansion validation and tests

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",
     ],

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
 }

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()))
+						}
+					}
+				})
+			}
+		}
+	}
+}

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"))

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 := ""

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",
     ],
 )
 

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)
 }