Merge pull request #111467 from RomanBednar/retro-sc-assignment

Allow retroactive storage class assigment to PVCs

pkg/apis/core/validation/validation.go

@@ -2027,12 +2027,15 @@ type PersistentVolumeClaimSpecValidationOptions struct {
 	AllowReadWriteOncePod bool
 	// Allow users to recover from previously failing expansion operation
 	EnableRecoverFromExpansionFailure bool
+	// Allow assigning StorageClass to unbound PVCs retroactively
+	EnableRetroactiveDefaultStorageClass bool
 }
 
 func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolumeClaim) PersistentVolumeClaimSpecValidationOptions {
 	opts := PersistentVolumeClaimSpecValidationOptions{
-		AllowReadWriteOncePod:             utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod),
+		AllowReadWriteOncePod:                utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod),
-		EnableRecoverFromExpansionFailure: utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure),
+		EnableRecoverFromExpansionFailure:    utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure),
+		EnableRetroactiveDefaultStorageClass: utilfeature.DefaultFeatureGate.Enabled(features.RetroactiveDefaultStorageClass),
 	}
 	if oldPvc == nil {
 		// If there's no old PVC, use the options based solely on feature enablement
@@ -2168,7 +2171,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl
 		oldPvcClone.Spec.VolumeName = newPvcClone.Spec.VolumeName // +k8s:verify-mutation:reason=clone
 	}
 
-	if validateStorageClassUpgrade(oldPvcClone.Annotations, newPvcClone.Annotations,
+	if validateStorageClassUpgradeFromAnnotation(oldPvcClone.Annotations, newPvcClone.Annotations,
 		oldPvcClone.Spec.StorageClassName, newPvcClone.Spec.StorageClassName) {
 		newPvcClone.Spec.StorageClassName = nil
 		metav1.SetMetaDataAnnotation(&newPvcClone.ObjectMeta, core.BetaStorageClassAnnotation, oldPvcClone.Annotations[core.BetaStorageClassAnnotation])
@@ -2176,6 +2179,13 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl
 		// storageclass annotation should be immutable after creation
 		// TODO: remove Beta when no longer needed
 		allErrs = append(allErrs, ValidateImmutableAnnotation(newPvc.ObjectMeta.Annotations[v1.BetaStorageClassAnnotation], oldPvc.ObjectMeta.Annotations[v1.BetaStorageClassAnnotation], v1.BetaStorageClassAnnotation, field.NewPath("metadata"))...)
+
+		// If update from annotation to attribute failed we can attempt try to validate update from nil value.
+		if validateStorageClassUpgradeFromNil(oldPvc.Annotations, oldPvc.Spec.StorageClassName, newPvc.Spec.StorageClassName, opts) {
+			newPvcClone.Spec.StorageClassName = oldPvcClone.Spec.StorageClassName // +k8s:verify-mutation:reason=clone
+		}
+		// TODO: add a specific error with a hint that storage class name can not be changed
+		// (instead of letting spec comparison below return generic field forbidden error)
 	}
 
 	// lets make sure storage values are same.
@@ -2216,7 +2226,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl
 // 2. The old pvc's StorageClassName is not set
 // 3. The new pvc's StorageClassName is set and equal to the old value in annotation
 // 4. If the new pvc's StorageClassAnnotation is set,it must be equal to the old pv/pvc's StorageClassAnnotation
-func validateStorageClassUpgrade(oldAnnotations, newAnnotations map[string]string, oldScName, newScName *string) bool {
+func validateStorageClassUpgradeFromAnnotation(oldAnnotations, newAnnotations map[string]string, oldScName, newScName *string) bool {
 	oldSc, oldAnnotationExist := oldAnnotations[core.BetaStorageClassAnnotation]
 	newScInAnnotation, newAnnotationExist := newAnnotations[core.BetaStorageClassAnnotation]
 	return oldAnnotationExist /* condition 1 */ &&
@@ -2225,6 +2235,20 @@ func validateStorageClassUpgrade(oldAnnotations, newAnnotations map[string]strin
 		(!newAnnotationExist || newScInAnnotation == oldSc) /* condition 4 */
 }
 
+// Provide an upgrade path from PVC with nil storage class. We allow update of
+// StorageClassName only if following four conditions are met at the same time:
+// 1. RetroactiveDefaultStorageClass FeatureGate is enabled
+// 2. The new pvc's StorageClassName is not nil
+// 3. The old pvc's StorageClassName is nil
+// 4. The old pvc either does not have beta annotation set, or the beta annotation matches new pvc's StorageClassName
+func validateStorageClassUpgradeFromNil(oldAnnotations map[string]string, oldScName, newScName *string, opts PersistentVolumeClaimSpecValidationOptions) bool {
+	oldAnnotation, oldAnnotationExist := oldAnnotations[core.BetaStorageClassAnnotation]
+	return opts.EnableRetroactiveDefaultStorageClass /* condition 1 */ &&
+		newScName != nil /* condition 2 */ &&
+		oldScName == nil /* condition 3 */ &&
+		(!oldAnnotationExist || *newScName == oldAnnotation) /* condition 4 */
+}
+
 var resizeStatusSet = sets.NewString(string(core.PersistentVolumeClaimNoExpansionInProgress),
 	string(core.PersistentVolumeClaimControllerExpansionInProgress),
 	string(core.PersistentVolumeClaimControllerExpansionFailed),

pkg/apis/core/validation/validation_test.go

@@ -1192,6 +1192,17 @@ func testVolumeClaimStorageClassInSpec(name, namespace, scName string, spec core
 	}
 }
 
+func testVolumeClaimStorageClassNilInSpec(name, namespace string, spec core.PersistentVolumeClaimSpec) *core.PersistentVolumeClaim {
+	spec.StorageClassName = nil
+	return &core.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+		Spec: spec,
+	}
+}
+
 func testVolumeSnapshotDataSourceInSpec(name string, kind string, apiGroup string) *core.PersistentVolumeClaimSpec {
 	scName := "csi-plugin"
 	dataSourceInSpec := core.PersistentVolumeClaimSpec{
@@ -1249,6 +1260,18 @@ func testVolumeClaimStorageClassInAnnotationAndSpec(name, namespace, scNameInAnn
 	}
 }
 
+func testVolumeClaimStorageClassInAnnotationAndNilInSpec(name, namespace, scNameInAnn string, spec core.PersistentVolumeClaimSpec) *core.PersistentVolumeClaim {
+	spec.StorageClassName = nil
+	return &core.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        name,
+			Namespace:   namespace,
+			Annotations: map[string]string{v1.BetaStorageClassAnnotation: scNameInAnn},
+		},
+		Spec: spec,
+	}
+}
+
 func testValidatePVC(t *testing.T, ephemeral bool) {
 	invalidClassName := "-invalid-"
 	validClassName := "valid"
@@ -1972,6 +1995,28 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
 		},
 	})
 
+	validClaimStorageClassInSpecChanged := testVolumeClaimStorageClassInSpec("foo", "ns", "fast2", core.PersistentVolumeClaimSpec{
+		AccessModes: []core.PersistentVolumeAccessMode{
+			core.ReadOnlyMany,
+		},
+		Resources: core.ResourceRequirements{
+			Requests: core.ResourceList{
+				core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+			},
+		},
+	})
+
+	validClaimStorageClassNil := testVolumeClaimStorageClassNilInSpec("foo", "ns", core.PersistentVolumeClaimSpec{
+		AccessModes: []core.PersistentVolumeAccessMode{
+			core.ReadOnlyMany,
+		},
+		Resources: core.ResourceRequirements{
+			Requests: core.ResourceList{
+				core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+			},
+		},
+	})
+
 	invalidClaimStorageClassInSpec := testVolumeClaimStorageClassInSpec("foo", "ns", "fast2", core.PersistentVolumeClaimSpec{
 		AccessModes: []core.PersistentVolumeAccessMode{
 			core.ReadOnlyMany,
@@ -1995,6 +2040,18 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
 			},
 		})
 
+	validClaimStorageClassInAnnotationAndNilInSpec := testVolumeClaimStorageClassInAnnotationAndNilInSpec(
+		"foo", "ns", "fast", core.PersistentVolumeClaimSpec{
+			AccessModes: []core.PersistentVolumeAccessMode{
+				core.ReadOnlyMany,
+			},
+			Resources: core.ResourceRequirements{
+				Requests: core.ResourceList{
+					core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+				},
+			},
+		})
+
 	invalidClaimStorageClassInAnnotationAndSpec := testVolumeClaimStorageClassInAnnotationAndSpec(
 		"foo", "ns", "fast2", "fast", core.PersistentVolumeClaimSpec{
 			AccessModes: []core.PersistentVolumeAccessMode{
@@ -2116,10 +2173,11 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
 	})
 
 	scenarios := map[string]struct {
-		isExpectedFailure          bool
+		isExpectedFailure                    bool
-		oldClaim                   *core.PersistentVolumeClaim
+		oldClaim                             *core.PersistentVolumeClaim
-		newClaim                   *core.PersistentVolumeClaim
+		newClaim                             *core.PersistentVolumeClaim
-		enableRecoverFromExpansion bool
+		enableRecoverFromExpansion           bool
+		enableRetroactiveDefaultStorageClass bool
 	}{
 		"valid-update-volumeName-only": {
 			isExpectedFailure: false,
@@ -2230,6 +2288,69 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
 			oldClaim:          validClaimStorageClass,
 			newClaim:          validClaimStorageClassInSpec,
 		},
+		"valid-upgrade-nil-storage-class-spec-to-spec": {
+			isExpectedFailure:                    false,
+			oldClaim:                             validClaimStorageClassNil,
+			newClaim:                             validClaimStorageClassInSpec,
+			enableRetroactiveDefaultStorageClass: true,
+			// Feature enabled - change from nil sc name is valid if there is no beta annotation.
+		},
+		"invalid-upgrade-nil-storage-class-spec-to-spec": {
+			isExpectedFailure:                    true,
+			oldClaim:                             validClaimStorageClassNil,
+			newClaim:                             validClaimStorageClassInSpec,
+			enableRetroactiveDefaultStorageClass: false,
+			// Feature disabled - change from nil sc name is invalid if there is no beta annotation.
+		},
+		"invalid-upgrade-not-nil-storage-class-spec-to-spec": {
+			isExpectedFailure:                    true,
+			oldClaim:                             validClaimStorageClassInSpec,
+			newClaim:                             validClaimStorageClassInSpecChanged,
+			enableRetroactiveDefaultStorageClass: true,
+			// Feature enablement must not allow non nil value change.
+		},
+		"invalid-upgrade-to-nil-storage-class-spec-to-spec": {
+			isExpectedFailure:                    true,
+			oldClaim:                             validClaimStorageClassInSpec,
+			newClaim:                             validClaimStorageClassNil,
+			enableRetroactiveDefaultStorageClass: true,
+			// Feature enablement must not allow change to nil value change.
+		},
+		"valid-upgrade-storage-class-annotation-and-nil-spec-to-spec": {
+			isExpectedFailure:                    false,
+			oldClaim:                             validClaimStorageClassInAnnotationAndNilInSpec,
+			newClaim:                             validClaimStorageClassInAnnotationAndSpec,
+			enableRetroactiveDefaultStorageClass: false,
+			// Change from nil sc name is valid if annotations match.
+		},
+		"valid-upgrade-storage-class-annotation-and-nil-spec-to-spec-retro": {
+			isExpectedFailure:                    false,
+			oldClaim:                             validClaimStorageClassInAnnotationAndNilInSpec,
+			newClaim:                             validClaimStorageClassInAnnotationAndSpec,
+			enableRetroactiveDefaultStorageClass: true,
+			// Change from nil sc name is valid if annotations match, feature enablement must not break this old behavior.
+		},
+		"invalid-upgrade-storage-class-annotation-and-spec-to-spec": {
+			isExpectedFailure:                    true,
+			oldClaim:                             validClaimStorageClassInAnnotationAndSpec,
+			newClaim:                             validClaimStorageClassInSpecChanged,
+			enableRetroactiveDefaultStorageClass: false,
+			// Change from non nil sc name is invalid if annotations don't match.
+		},
+		"invalid-upgrade-storage-class-annotation-and-spec-to-spec-retro": {
+			isExpectedFailure:                    true,
+			oldClaim:                             validClaimStorageClassInAnnotationAndSpec,
+			newClaim:                             validClaimStorageClassInSpecChanged,
+			enableRetroactiveDefaultStorageClass: true,
+			// Change from non nil sc name is invalid if annotations don't match, feature enablement must not break this old behavior.
+		},
+		"invalid-upgrade-storage-class-annotation-and-no-spec": {
+			isExpectedFailure:                    true,
+			oldClaim:                             validClaimStorageClassInAnnotationAndNilInSpec,
+			newClaim:                             validClaimStorageClassInSpecChanged,
+			enableRetroactiveDefaultStorageClass: true,
+			// Change from nil sc name is invalid if annotations don't match, feature enablement must not break this old behavior.
+		},
 		"invalid-upgrade-storage-class-annotation-to-spec": {
 			isExpectedFailure: true,
 			oldClaim:          validClaimStorageClass,
@@ -2294,6 +2415,7 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
 	for name, scenario := range scenarios {
 		t.Run(name, func(t *testing.T) {
 			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecoverVolumeExpansionFailure, scenario.enableRecoverFromExpansion)()
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RetroactiveDefaultStorageClass, scenario.enableRetroactiveDefaultStorageClass)()
 			scenario.oldClaim.ResourceVersion = "1"
 			scenario.newClaim.ResourceVersion = "1"
 			opts := ValidationOptionsForPersistentVolumeClaim(scenario.newClaim, scenario.oldClaim)

pkg/controller/volume/persistentvolume/pv_controller.go

@@ -350,6 +350,14 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(ctx context.Context, cl
 			klog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: no volume found", claimToClaimKey(claim))
 			// No PV could be found
 			// OBSERVATION: pvc is "Pending", will retry
+
+			if utilfeature.DefaultFeatureGate.Enabled(features.RetroactiveDefaultStorageClass) {
+				klog.V(4).Infof("FeatureGate[%s] is enabled, attempting to assign storage class to unbound PersistentVolumeClaim[%s]", features.RetroactiveDefaultStorageClass, claimToClaimKey(claim))
+				if claim, err = ctrl.assignDefaultStorageClass(claim); err != nil {
+					return fmt.Errorf("can't update PersistentVolumeClaim[%q]: %w", claimToClaimKey(claim), err)
+				}
+			}
+
 			switch {
 			case delayBinding && !storagehelpers.IsDelayBindingProvisioning(claim):
 				if err = ctrl.emitEventForUnboundDelayBindingClaim(claim); err != nil {
@@ -918,6 +926,35 @@ func (ctrl *PersistentVolumeController) updateVolumePhaseWithEvent(volume *v1.Pe
 	return newVol, nil
 }
 
+// assignDefaultStorageClass updates the claim storage class if there is any, the claim is updated to the API server.
+// Ignores claims that already have a storage class.
+// TODO: if resync is ever changed to a larger period, we might need to change how we set the default class on existing unbound claims
+func (ctrl *PersistentVolumeController) assignDefaultStorageClass(claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) {
+	if claim.Spec.StorageClassName != nil {
+		return claim, nil
+	}
+
+	class, err := util.GetDefaultClass(ctrl.classLister)
+	if err != nil {
+		// It is safe to ignore errors here because it means we either could not list SCs or there is more than one default.
+		// TODO: do not ignore errors after this PR is merged: https://github.com/kubernetes/kubernetes/pull/110559
+		klog.V(4).Infof("failed to get default storage class: %v", err)
+		return claim, nil
+	} else if class == nil {
+		klog.V(4).Infof("can not assign storage class to PersistentVolumeClaim[%s]: default storage class not found", claimToClaimKey(claim))
+		return claim, nil
+	}
+
+	klog.V(4).Infof("assigning StorageClass[%s] to PersistentVolumeClaim[%s]", class.Name, claimToClaimKey(claim))
+	claim.Spec.StorageClassName = &class.Name
+	newClaim, err := ctrl.kubeClient.CoreV1().PersistentVolumeClaims(claim.GetNamespace()).Update(context.TODO(), claim, metav1.UpdateOptions{})
+	if err != nil {
+		return claim, err
+	}
+
+	return newClaim, nil
+}
+
 // bindVolumeToClaim modifies given volume to be bound to a claim and saves it to
 // API server. The claim is not modified in this method!
 func (ctrl *PersistentVolumeController) bindVolumeToClaim(volume *v1.PersistentVolume, claim *v1.PersistentVolumeClaim) (*v1.PersistentVolume, error) {

pkg/controller/volume/persistentvolume/pv_controller_test.go

@@ -465,6 +465,20 @@ func makeStorageClass(scName string, mode *storagev1.VolumeBindingMode) *storage
 		ObjectMeta: metav1.ObjectMeta{
 			Name: scName,
 		},
+		Provisioner:       "kubernetes.io/no-provisioner",
+		VolumeBindingMode: mode,
+	}
+}
+
+func makeDefaultStorageClass(scName string, mode *storagev1.VolumeBindingMode) *storagev1.StorageClass {
+	return &storagev1.StorageClass{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: scName,
+			Annotations: map[string]string{
+				util.IsDefaultStorageClassAnnotation: "true",
+			},
+		},
+		Provisioner:       "kubernetes.io/no-provisioner",
 		VolumeBindingMode: mode,
 	}
 }
@@ -728,3 +742,121 @@ func TestModifyDeletionFinalizers(t *testing.T) {
 		})
 	}
 }
+
+func TestRetroactiveStorageClassAssignment(t *testing.T) {
+	// Enable RetroactiveDefaultStorageClass feature gate.
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RetroactiveDefaultStorageClass, true)()
+	tests := []struct {
+		storageClasses []*storagev1.StorageClass
+		tests          []controllerTest
+	}{
+		// [Unit test set 15] - retroactive storage class assignment tests
+		{
+			storageClasses: []*storagev1.StorageClass{},
+			tests: []controllerTest{
+				{
+					name:            "15-1 - pvc storage class is not assigned retroactively if there are no default storage classes",
+					initialVolumes:  novolumes,
+					expectedVolumes: novolumes,
+					initialClaims:   newClaimArray("claim15-1", "uid15-1", "1Gi", "", v1.ClaimPending, nil),
+					expectedClaims:  newClaimArray("claim15-1", "uid15-1", "1Gi", "", v1.ClaimPending, nil),
+					expectedEvents:  noevents,
+					errors:          noerrors,
+					test:            testSyncClaim,
+				},
+			},
+		},
+		{
+			storageClasses: []*storagev1.StorageClass{
+				makeDefaultStorageClass(classGold, &modeImmediate),
+				makeDefaultStorageClass(classSilver, &modeImmediate)},
+			tests: []controllerTest{
+				{
+					name:            "15-2 - pvc storage class is not assigned retroactively if there are multiple default storage classes",
+					initialVolumes:  novolumes,
+					expectedVolumes: novolumes,
+					initialClaims:   newClaimArray("claim15-2", "uid15-2", "1Gi", "", v1.ClaimPending, nil),
+					expectedClaims:  newClaimArray("claim15-2", "uid15-2", "1Gi", "", v1.ClaimPending, nil),
+					expectedEvents:  noevents,
+					errors:          noerrors,
+					test:            testSyncClaim,
+				},
+			},
+		},
+		{
+			storageClasses: []*storagev1.StorageClass{
+				makeDefaultStorageClass(classGold, &modeImmediate),
+				makeStorageClass(classSilver, &modeImmediate),
+			},
+			tests: []controllerTest{
+				{
+					name:            "15-3 - pvc storage class is not assigned retroactively if claim is already bound",
+					initialVolumes:  novolumes,
+					expectedVolumes: novolumes,
+					initialClaims:   newClaimArray("claim15-3", "uid15-3", "1Gi", "test", v1.ClaimBound, &classCopper, volume.AnnBoundByController, volume.AnnBindCompleted),
+					expectedClaims:  newClaimArray("claim15-3", "uid15-3", "1Gi", "test", v1.ClaimLost, &classCopper, volume.AnnBoundByController, volume.AnnBindCompleted),
+					expectedEvents:  noevents,
+					errors:          noerrors,
+					test:            testSyncClaim,
+				},
+			},
+		},
+		{
+			storageClasses: []*storagev1.StorageClass{
+				makeDefaultStorageClass(classGold, &modeImmediate),
+				makeStorageClass(classSilver, &modeImmediate),
+			},
+			tests: []controllerTest{
+				{
+					name:            "15-4 - pvc storage class is not assigned retroactively if claim is already bound but annotations are missing",
+					initialVolumes:  novolumes,
+					expectedVolumes: novolumes,
+					initialClaims:   newClaimArray("claim15-4", "uid15-4", "1Gi", "test", v1.ClaimBound, &classCopper),
+					expectedClaims:  newClaimArray("claim15-4", "uid15-4", "1Gi", "test", v1.ClaimPending, &classCopper),
+					expectedEvents:  noevents,
+					errors:          noerrors,
+					test:            testSyncClaim,
+				},
+			},
+		},
+		{
+			storageClasses: []*storagev1.StorageClass{
+				makeDefaultStorageClass(classGold, &modeImmediate),
+				makeStorageClass(classSilver, &modeImmediate),
+			},
+			tests: []controllerTest{
+				{
+					name:            "15-5 - pvc storage class is assigned retroactively if there is a default",
+					initialVolumes:  novolumes,
+					expectedVolumes: novolumes,
+					initialClaims:   newClaimArray("claim15-5", "uid15-5", "1Gi", "", v1.ClaimPending, nil),
+					expectedClaims:  newClaimArray("claim15-5", "uid15-5", "1Gi", "", v1.ClaimPending, &classGold),
+					expectedEvents:  noevents,
+					errors:          noerrors,
+					test:            testSyncClaim,
+				},
+			},
+		},
+		{
+			storageClasses: []*storagev1.StorageClass{
+				makeDefaultStorageClass(classGold, &modeImmediate),
+				makeStorageClass(classCopper, &modeImmediate),
+			},
+			tests: []controllerTest{
+				{
+					name:            "15-6 - pvc storage class is not changed if claim is not bound but already has a storage class",
+					initialVolumes:  novolumes,
+					expectedVolumes: novolumes,
+					initialClaims:   newClaimArray("claim15-6", "uid15-6", "1Gi", "", v1.ClaimPending, &classCopper),
+					expectedClaims:  newClaimArray("claim15-6", "uid15-6", "1Gi", "", v1.ClaimPending, &classCopper),
+					expectedEvents:  noevents,
+					errors:          noerrors,
+					test:            testSyncClaim,
+				},
+			},
+		},
+	}
+	for _, test := range tests {
+		runSyncTests(t, test.tests, test.storageClasses, nil)
+	}
+}

pkg/features/kube_features.go

@@ -720,6 +720,13 @@ const (
 	// Allow users to recover from volume expansion failure
 	RecoverVolumeExpansionFailure featuregate.Feature = "RecoverVolumeExpansionFailure"
 
+	// owner: @RomanBednar
+	// kep: http://kep.k8s.io/3333
+	// alpha: v1.25
+	//
+	// Allow assigning StorageClass to unbound PVCs retroactively
+	RetroactiveDefaultStorageClass featuregate.Feature = "RetroactiveDefaultStorageClass"
+
 	// owner: @mikedanese
 	// alpha: v1.7
 	// beta: v1.12
@@ -1045,6 +1052,8 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
 
 	RecoverVolumeExpansionFailure: {Default: false, PreRelease: featuregate.Alpha},
 
+	RetroactiveDefaultStorageClass: {Default: false, PreRelease: featuregate.Alpha},
+
 	RotateKubeletServerCertificate: {Default: true, PreRelease: featuregate.Beta},
 
 	SeccompDefault: {Default: true, PreRelease: featuregate.Beta},

pkg/volume/util/storageclass.go

@@ -0,0 +1,76 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"fmt"
+	storagev1 "k8s.io/api/storage/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	storagev1listers "k8s.io/client-go/listers/storage/v1"
+	"k8s.io/klog/v2"
+)
+
+const (
+	// isDefaultStorageClassAnnotation represents a StorageClass annotation that
+	// marks a class as the default StorageClass
+	IsDefaultStorageClassAnnotation = "storageclass.kubernetes.io/is-default-class"
+
+	// betaIsDefaultStorageClassAnnotation is the beta version of IsDefaultStorageClassAnnotation.
+	// TODO: remove Beta when no longer used
+	BetaIsDefaultStorageClassAnnotation = "storageclass.beta.kubernetes.io/is-default-class"
+)
+
+// GetDefaultClass returns the default StorageClass from the store, or nil.
+func GetDefaultClass(lister storagev1listers.StorageClassLister) (*storagev1.StorageClass, error) {
+	list, err := lister.List(labels.Everything())
+	if err != nil {
+		return nil, err
+	}
+
+	defaultClasses := []*storagev1.StorageClass{}
+	for _, class := range list {
+		if IsDefaultAnnotation(class.ObjectMeta) {
+			defaultClasses = append(defaultClasses, class)
+			klog.V(4).Infof("GetDefaultClass added: %s", class.Name)
+		}
+	}
+
+	if len(defaultClasses) == 0 {
+		return nil, nil
+	}
+	if len(defaultClasses) > 1 {
+		klog.V(4).Infof("GetDefaultClass %d defaults found", len(defaultClasses))
+		return nil, errors.NewInternalError(fmt.Errorf("%d default StorageClasses were found", len(defaultClasses)))
+	}
+	return defaultClasses[0], nil
+}
+
+// IsDefaultAnnotation returns a boolean if the default storage class
+// annotation is set
+// TODO: remove Beta when no longer needed
+func IsDefaultAnnotation(obj metav1.ObjectMeta) bool {
+	if obj.Annotations[IsDefaultStorageClassAnnotation] == "true" {
+		return true
+	}
+	if obj.Annotations[BetaIsDefaultStorageClassAnnotation] == "true" {
+		return true
+	}
+
+	return false
+}

plugin/pkg/admission/storage/storageclass/setdefault/admission.go

@@ -20,19 +20,16 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"k8s.io/kubernetes/pkg/volume/util"
 
 	"k8s.io/klog/v2"
 
-	storagev1 "k8s.io/api/storage/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apiserver/pkg/admission"
 	genericadmissioninitializer "k8s.io/apiserver/pkg/admission/initializer"
 	"k8s.io/client-go/informers"
 	storagev1listers "k8s.io/client-go/listers/storage/v1"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/core/helper"
-	storageutil "k8s.io/kubernetes/pkg/apis/storage/util"
 )
 
 const (
@@ -108,7 +105,7 @@ func (a *claimDefaulterPlugin) Admit(ctx context.Context, attr admission.Attribu
 
 	klog.V(4).Infof("no storage class for claim %s (generate: %s)", pvc.Name, pvc.GenerateName)
 
-	def, err := getDefaultClass(a.lister)
+	def, err := util.GetDefaultClass(a.lister)
 	if err != nil {
 		return admission.NewForbidden(attr, err)
 	}
@@ -121,28 +118,3 @@ func (a *claimDefaulterPlugin) Admit(ctx context.Context, attr admission.Attribu
 	pvc.Spec.StorageClassName = &def.Name
 	return nil
 }
-
-// getDefaultClass returns the default StorageClass from the store, or nil.
-func getDefaultClass(lister storagev1listers.StorageClassLister) (*storagev1.StorageClass, error) {
-	list, err := lister.List(labels.Everything())
-	if err != nil {
-		return nil, err
-	}
-
-	defaultClasses := []*storagev1.StorageClass{}
-	for _, class := range list {
-		if storageutil.IsDefaultAnnotation(class.ObjectMeta) {
-			defaultClasses = append(defaultClasses, class)
-			klog.V(4).Infof("getDefaultClass added: %s", class.Name)
-		}
-	}
-
-	if len(defaultClasses) == 0 {
-		return nil, nil
-	}
-	if len(defaultClasses) > 1 {
-		klog.V(4).Infof("getDefaultClass %d defaults found", len(defaultClasses))
-		return nil, errors.NewInternalError(fmt.Errorf("%d default StorageClasses were found", len(defaultClasses)))
-	}
-	return defaultClasses[0], nil
-}

test/e2e/framework/pv/pv.go

@@ -34,6 +34,7 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	clientset "k8s.io/client-go/kubernetes"
+	"k8s.io/kubernetes/pkg/volume/util"
 	"k8s.io/kubernetes/test/e2e/framework"
 	e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper"
 )
@@ -45,14 +46,6 @@ const (
 	// VolumeSelectorKey is the key for volume selector.
 	VolumeSelectorKey = "e2e-pv-pool"
 
-	// isDefaultStorageClassAnnotation represents a StorageClass annotation that
-	// marks a class as the default StorageClass
-	isDefaultStorageClassAnnotation = "storageclass.kubernetes.io/is-default-class"
-
-	// betaIsDefaultStorageClassAnnotation is the beta version of IsDefaultStorageClassAnnotation.
-	// TODO: remove Beta when no longer used
-	betaIsDefaultStorageClassAnnotation = "storageclass.beta.kubernetes.io/is-default-class"
-
 	// volumeGidAnnotationKey is the of the annotation on the PersistentVolume
 	// object that specifies a supplemental GID.
 	// it is copied from k8s.io/kubernetes/pkg/volume/util VolumeGidAnnotationKey
@@ -826,7 +819,7 @@ func GetDefaultStorageClassName(c clientset.Interface) (string, error) {
 	}
 	var scName string
 	for _, sc := range list.Items {
-		if isDefaultAnnotation(sc.ObjectMeta) {
+		if util.IsDefaultAnnotation(sc.ObjectMeta) {
 			if len(scName) != 0 {
 				return "", fmt.Errorf("Multiple default storage classes found: %q and %q", scName, sc.Name)
 			}
@@ -840,20 +833,6 @@ func GetDefaultStorageClassName(c clientset.Interface) (string, error) {
 	return scName, nil
 }
 
-// isDefaultAnnotation returns a boolean if the default storage class
-// annotation is set
-// TODO: remove Beta when no longer needed
-func isDefaultAnnotation(obj metav1.ObjectMeta) bool {
-	if obj.Annotations[isDefaultStorageClassAnnotation] == "true" {
-		return true
-	}
-	if obj.Annotations[betaIsDefaultStorageClassAnnotation] == "true" {
-		return true
-	}
-
-	return false
-}
-
 // SkipIfNoDefaultStorageClass skips tests if no default SC can be found.
 func SkipIfNoDefaultStorageClass(c clientset.Interface) {
 	_, err := GetDefaultStorageClassName(c)