Add RBAC-controlled StorageClass modification via /storageclass subre…

pkg/apis/core/validation/validation.go

@@ -17,6 +17,7 @@ limitations under the License.
 package validation
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"math"
@@ -2363,8 +2364,48 @@ func isDataSourceEqualDataSourceRef(dataSource *core.TypedLocalObjectReference,
 	return reflect.DeepEqual(dataSource.APIGroup, dataSourceRef.APIGroup) && dataSource.Kind == dataSourceRef.Kind && dataSource.Name == dataSourceRef.Name
 }
 
+// validateStorageClassChange checks if storage class change is permitted
+// Authorization is handled at the REST layer via RBAC on the subresource
+func validateStorageClassChange(ctx context.Context, newPvc, oldPvc *core.PersistentVolumeClaim) *field.Error {
+	// Check if storage class is actually changing
+	oldStorageClass := ""
+	if oldPvc.Spec.StorageClassName != nil {
+		oldStorageClass = *oldPvc.Spec.StorageClassName
+	}
+
+	newStorageClass := ""
+	if newPvc.Spec.StorageClassName != nil {
+		newStorageClass = *newPvc.Spec.StorageClassName
+	}
+
+	// If storage class is not changing, allow the update (no special authorization needed)
+	if oldStorageClass == newStorageClass {
+		return nil
+	}
+
+	// Allow the special case of setting a storage class when it was previously nil
+	// This is an existing legitimate upgrade path that should be preserved
+	if oldPvc.Spec.StorageClassName == nil && newPvc.Spec.StorageClassName != nil {
+		return nil
+	}
+
+	// Other storage class changes require the user to have proper RBAC permissions
+	// on the persistentvolumeclaims/storageclass subresource. The authorization check
+	// is performed at the REST layer, not here in validation.
+
+	// For regular PVC updates, other storage class changes are forbidden
+	// Users must use the /storageclass subresource endpoint for these changes
+	return field.Forbidden(field.NewPath("spec", "storageClassName"),
+		"storage class changes are only allowed via the persistentvolumeclaims/storageclass subresource")
+}
+
 // ValidatePersistentVolumeClaimUpdate validates an update to a PersistentVolumeClaim
 func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeClaim, opts PersistentVolumeClaimSpecValidationOptions) field.ErrorList {
+	return ValidatePersistentVolumeClaimUpdateWithContext(context.TODO(), newPvc, oldPvc, opts)
+}
+
+// ValidatePersistentVolumeClaimUpdateWithContext validates an update to a PersistentVolumeClaim with context
+func ValidatePersistentVolumeClaimUpdateWithContext(ctx context.Context, newPvc, oldPvc *core.PersistentVolumeClaim, opts PersistentVolumeClaimSpecValidationOptions) field.ErrorList {
 	allErrs := ValidateObjectMetaUpdate(&newPvc.ObjectMeta, &oldPvc.ObjectMeta, field.NewPath("metadata"))
 	allErrs = append(allErrs, ValidatePersistentVolumeClaim(newPvc, opts)...)
 	newPvcClone := newPvc.DeepCopy()
@@ -2382,16 +2423,21 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl
 		newPvcClone.Spec.StorageClassName = nil
 		metav1.SetMetaDataAnnotation(&newPvcClone.ObjectMeta, core.BetaStorageClassAnnotation, oldPvcClone.Annotations[core.BetaStorageClassAnnotation])
 	} else {
-		// 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"))...)
+		// Check for storage class change with RBAC validation
+		if err := validateStorageClassChange(ctx, newPvc, oldPvc); err != nil {
+			allErrs = append(allErrs, err)
+		} else {
+			// 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
+			// 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)
 		}
-		// 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.

pkg/apis/core/validation/validation_test.go

@@ -18,6 +18,7 @@ package validation
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"math"
 	"reflect"
@@ -3054,6 +3055,151 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
 	}
 }
 
+func TestValidateStorageClassChange(t *testing.T) {
+	tests := map[string]struct {
+		oldPvc      *core.PersistentVolumeClaim
+		newPvc      *core.PersistentVolumeClaim
+		expectError bool
+		errorPath   string
+	}{
+		"no storage class change": {
+			oldPvc: testVolumeClaimStorageClassInSpec("foo", "ns", "fast", core.PersistentVolumeClaimSpec{
+				AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+				Resources: core.VolumeResourceRequirements{
+					Requests: core.ResourceList{
+						core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+					},
+				},
+			}),
+			newPvc: testVolumeClaimStorageClassInSpec("foo", "ns", "fast", core.PersistentVolumeClaimSpec{
+				AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+				Resources: core.VolumeResourceRequirements{
+					Requests: core.ResourceList{
+						core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+					},
+				},
+			}),
+			expectError: false,
+		},
+		"storage class change from fast to slow": {
+			oldPvc: testVolumeClaimStorageClassInSpec("foo", "ns", "fast", core.PersistentVolumeClaimSpec{
+				AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+				Resources: core.VolumeResourceRequirements{
+					Requests: core.ResourceList{
+						core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+					},
+				},
+			}),
+			newPvc: testVolumeClaimStorageClassInSpec("foo", "ns", "slow", core.PersistentVolumeClaimSpec{
+				AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+				Resources: core.VolumeResourceRequirements{
+					Requests: core.ResourceList{
+						core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+					},
+				},
+			}),
+			expectError: true,
+			errorPath:   "spec.storageClassName",
+		},
+		"storage class change from nil to fast": {
+			oldPvc: testVolumeClaimStorageClassNilInSpec("foo", "ns", core.PersistentVolumeClaimSpec{
+				AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+				Resources: core.VolumeResourceRequirements{
+					Requests: core.ResourceList{
+						core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+					},
+				},
+			}),
+			newPvc: testVolumeClaimStorageClassInSpec("foo", "ns", "fast", core.PersistentVolumeClaimSpec{
+				AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+				Resources: core.VolumeResourceRequirements{
+					Requests: core.ResourceList{
+						core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+					},
+				},
+			}),
+			expectError: false, // This should be allowed as it's an existing upgrade path
+		},
+		"storage class change from fast to nil": {
+			oldPvc: testVolumeClaimStorageClassInSpec("foo", "ns", "fast", core.PersistentVolumeClaimSpec{
+				AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+				Resources: core.VolumeResourceRequirements{
+					Requests: core.ResourceList{
+						core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+					},
+				},
+			}),
+			newPvc: testVolumeClaimStorageClassNilInSpec("foo", "ns", core.PersistentVolumeClaimSpec{
+				AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+				Resources: core.VolumeResourceRequirements{
+					Requests: core.ResourceList{
+						core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+					},
+				},
+			}),
+			expectError: true,
+			errorPath:   "spec.storageClassName",
+		},
+	}
+
+	for testName, test := range tests {
+		t.Run(testName, func(t *testing.T) {
+			ctx := context.TODO()
+			err := validateStorageClassChange(ctx, test.newPvc, test.oldPvc)
+
+			if test.expectError {
+				if err == nil {
+					t.Errorf("expected error but got none")
+				} else if err.Field != test.errorPath {
+					t.Errorf("expected error at field %s, got %s", test.errorPath, err.Field)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("expected no error but got: %v", err)
+				}
+			}
+		})
+	}
+}
+
+func TestValidatePersistentVolumeClaimUpdateWithStorageClassChange(t *testing.T) {
+	// Test that regular PVC updates reject storage class changes
+	oldPvc := testVolumeClaimStorageClassInSpec("foo", "ns", "fast", core.PersistentVolumeClaimSpec{
+		AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+		Resources: core.VolumeResourceRequirements{
+			Requests: core.ResourceList{
+				core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+			},
+		},
+	})
+
+	newPvc := testVolumeClaimStorageClassInSpec("foo", "ns", "slow", core.PersistentVolumeClaimSpec{
+		AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce},
+		Resources: core.VolumeResourceRequirements{
+			Requests: core.ResourceList{
+				core.ResourceName(core.ResourceStorage): resource.MustParse("10G"),
+			},
+		},
+	})
+
+	opts := ValidationOptionsForPersistentVolumeClaim(newPvc, oldPvc)
+	ctx := context.TODO()
+	errs := ValidatePersistentVolumeClaimUpdateWithContext(ctx, newPvc, oldPvc, opts)
+
+	// Should contain error about storage class changes being forbidden
+	found := false
+	for _, err := range errs {
+		if strings.Contains(err.Error(), "storage class changes are only allowed via the persistentvolumeclaims/storageclass subresource") {
+			found = true
+			break
+		}
+	}
+
+	if !found {
+		t.Errorf("Expected error about storage class changes being forbidden, got: %v", errs)
+	}
+}
+
 func TestValidationOptionsForPersistentVolumeClaim(t *testing.T) {
 	invaildAPIGroup := "^invalid"
 

pkg/registry/core/persistentvolumeclaim/storage/storage.go

@@ -33,13 +33,20 @@ import (
 	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
 )
 
+// PersistentVolumeClaimStorage includes storage for PersistentVolumeClaims and related subresources.
+type PersistentVolumeClaimStorage struct {
+	PersistentVolumeClaim *REST
+	Status                *StatusREST
+	StorageClass          *StorageClassREST
+}
+
 // REST implements a RESTStorage for persistent volume claims.
 type REST struct {
 	*genericregistry.Store
 }
 
 // NewREST returns a RESTStorage object that will work against persistent volume claims.
-func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) {
+func NewREST(optsGetter generic.RESTOptionsGetter) (PersistentVolumeClaimStorage, error) {
 	store := &genericregistry.Store{
 		NewFunc:                   func() runtime.Object { return &api.PersistentVolumeClaim{} },
 		NewListFunc:               func() runtime.Object { return &api.PersistentVolumeClaimList{} },
@@ -57,17 +64,26 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) {
 	}
 	options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: persistentvolumeclaim.GetAttrs}
 	if err := store.CompleteWithOptions(options); err != nil {
-		return nil, nil, err
+		return PersistentVolumeClaimStorage{}, err
 	}
 
 	statusStore := *store
 	statusStore.UpdateStrategy = persistentvolumeclaim.StatusStrategy
 	statusStore.ResetFieldsStrategy = persistentvolumeclaim.StatusStrategy
 
+	storageClassStore := *store
+	storageClassStore.UpdateStrategy = persistentvolumeclaim.StorageClassStrategy
+	storageClassStore.ResetFieldsStrategy = persistentvolumeclaim.StorageClassStrategy
+
 	rest := &REST{store}
 	store.Decorator = rest.defaultOnRead
 
-	return rest, &StatusREST{store: &statusStore}, nil
+	storageClassREST := &StorageClassREST{store: &storageClassStore}
+	return PersistentVolumeClaimStorage{
+		PersistentVolumeClaim: rest,
+		Status:                &StatusREST{store: &statusStore},
+		StorageClass:          storageClassREST,
+	}, nil
 }
 
 // Implement ShortNamesProvider
@@ -154,3 +170,40 @@ func (r *StatusREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
 func (r *StatusREST) ConvertToTable(ctx context.Context, object runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) {
 	return r.store.ConvertToTable(ctx, object, tableOptions)
 }
+
+// StorageClassREST implements the REST endpoint for changing the storage class of a persistentvolumeclaim.
+type StorageClassREST struct {
+	store *genericregistry.Store
+}
+
+// New creates a new PersistentVolumeClaim object.
+func (r *StorageClassREST) New() runtime.Object {
+	return &api.PersistentVolumeClaim{}
+}
+
+// Destroy cleans up resources on shutdown.
+func (r *StorageClassREST) Destroy() {
+	// Given that underlying store is shared with REST,
+	// we don't destroy it here explicitly.
+}
+
+// Get retrieves the object from the storage. It is required to support Patch.
+func (r *StorageClassREST) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
+	return r.store.Get(ctx, name, options)
+}
+
+// Update alters the storage class of an object.
+func (r *StorageClassREST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) {
+	// We are explicitly setting forceAllowCreate to false in the call to the underlying storage because
+	// subresources should never allow create on update.
+	return r.store.Update(ctx, name, objInfo, createValidation, updateValidation, false, options)
+}
+
+// GetResetFields implements rest.ResetFieldsStrategy
+func (r *StorageClassREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
+	return r.store.GetResetFields()
+}
+
+func (r *StorageClassREST) ConvertToTable(ctx context.Context, object runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) {
+	return r.store.ConvertToTable(ctx, object, tableOptions)
+}