Allow PVC VACName to go from non-nil to nil #132106

AndrewSirenko · 2025-06-04T20:56:00Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

PR kubernetes-csi/external-resizer#487 will allow users using ModifyVolume feature via VolumeAttributesClass to rollback from an infeasible pvc.spec.VacName to no VAC.

However, kube-api-server includes validations to prevent going from non-nil pvc/pv VACName to nil VACName.

This PR loosens those PVC/PV Update validations. External-resizer sidecar will ensure user can only rollback infeasible modifications to nil.

For additional context see slack thread: https://kubernetes.slack.com/archives/C8EJ01Z46/p1748628161127689

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

/cc @gnufied
/cc @sunnylovestiramisu
/cc @msau42
/cc @xing-yang

Tested on an AWS kOps cluster alongside aws-ebs-csi-driver. See K/K VAC Validation changes + Sunny's resizer PR.

Does this PR introduce a user-facing change?

Allow pvc.spec.VolumeAttributesClassName to go from non-nil to nil

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-06-04T20:56:04Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2025-06-04T20:56:11Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2025-06-04T20:56:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndrewSirenko
Once this PR has been reviewed and has the lgtm label, please assign msau42 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/apis/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

AndrewSirenko · 2025-06-04T20:58:01Z

pkg/apis/core/validation/validation.go

@@ -2155,11 +2155,6 @@ func ValidatePersistentVolumeUpdate(newPv, oldPv *core.PersistentVolume, opts Pe
 		if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass) {
 			allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "volumeAttributesClassName"), "update is forbidden when the VolumeAttributesClass feature gate is disabled"))
 		}
-		if opts.EnableVolumeAttributesClass {


Presume we want to remove the validation for PV as well so that we avoid situation where PVC can be nil but PV somehow stuck non-nil.

AndrewSirenko · 2025-06-04T20:58:30Z

pkg/apis/core/validation/validation.go

@@ -2427,7 +2422,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl
 		newPvcClone.Spec.Resources.Requests["storage"] = oldPvc.Spec.Resources.Requests["storage"] // +k8s:verify-mutation:reason=clone
 	}
 	// lets make sure volume attributes class name is same.
-	if newPvc.Status.Phase == core.ClaimBound && newPvcClone.Spec.VolumeAttributesClassName != nil {
+	if newPvc.Status.Phase == core.ClaimBound {


Was required to prevent this err:

❯ k patch pvc ebs-claim --patch '{"spec": {"volumeAttributesClassName": null}}' The PersistentVolumeClaim "ebs-claim" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims core.PersistentVolumeClaimSpec{ ... // 6 identical fields DataSource: nil, DataSourceRef: nil, - VolumeAttributesClassName: &"io3-class", + VolumeAttributesClassName: nil, }

xing-yang · 2025-06-04T21:24:59Z

pkg/apis/core/validation/validation_test.go

 			enableVolumeAttributesClass: true,
 			isExpectedFailure:           true,
 		},
 		"invalid-update-volume-attributes-class-to-nil": {
 			oldClaim:                    validClaimVolumeAttributesClass1,
 			newClaim:                    validClaimNilVolumeAttributesClass,
 			enableVolumeAttributesClass: true,
-			isExpectedFailure:           true,
+			isExpectedFailure:           false,
 		},
 		"invalid-update-volume-attributes-class-to-empty": {


This looks like the same test case as line 3035 "invalid-update-volume-attributes-class"?

Yes there existed duplicate tests with different names.

I presume the intent of "invalid-update-volume-attributes-class" is to catch some invalid update, while the existence of "invalid-update-volume-attributes-class-to-empty" AND "invalid-update-volume-attributes-class-to-nil" was to test these specific cases. But happy to delete one of the duplicate tests.

Either way I realize I need to change the name of this test to valid-update-vac-to-nil now that it is valid. Thanks!

xing-yang · 2025-06-04T21:27:55Z

pkg/apis/core/validation/validation_test.go

@@ -3034,15 +3034,15 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
 		},
 		"invalid-update-volume-attributes-class": {
 			oldClaim:                    validClaimVolumeAttributesClass1,
-			newClaim:                    validClaimNilVolumeAttributesClass,
+			newClaim:                    validClaimEmptyVolumeAttributesClass,
 			enableVolumeAttributesClass: true,
 			isExpectedFailure:           true,
 		},
 		"invalid-update-volume-attributes-class-to-nil": {


invalid -> valid

Will refactor this helper's name next revision.

Can we have a test case for string to empty-string and expect error?

gnufied · 2025-06-05T15:23:59Z

Can you make this a real PR so as tests are running? I am not sure if we run tests on draft PRs.

AndrewSirenko · 2025-06-05T20:49:55Z

Can you make this a real PR so as tests are running?

@gnufied Jinx!

Looks like tests all passed.

Allow PVC VACName to go from non-nil to nil

26be46e

k8s-ci-robot requested a review from gnufied June 4, 2025 20:56

k8s-ci-robot requested a review from msau42 June 4, 2025 20:56

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2025

k8s-ci-robot requested a review from sunnylovestiramisu June 4, 2025 20:56

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 4, 2025

k8s-ci-robot requested a review from xing-yang June 4, 2025 20:56

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 4, 2025

github-project-automation bot added this to SIG Apps Jun 4, 2025

github-project-automation bot moved this to Needs Triage in SIG Apps Jun 4, 2025

AndrewSirenko mentioned this pull request Jun 4, 2025

Enable Rollback Support of VAC with Infeasible Errors kubernetes-csi/external-resizer#487

Open

AndrewSirenko commented Jun 4, 2025

View reviewed changes

xing-yang reviewed Jun 4, 2025

View reviewed changes

AndrewSirenko marked this pull request as ready for review June 5, 2025 15:22

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow PVC VACName to go from non-nil to nil #132106

Allow PVC VACName to go from non-nil to nil #132106

AndrewSirenko commented Jun 4, 2025 •

edited

Loading

Uh oh!

k8s-ci-robot commented Jun 4, 2025

Uh oh!

k8s-ci-robot commented Jun 4, 2025

Uh oh!

k8s-ci-robot commented Jun 4, 2025

Uh oh!

AndrewSirenko Jun 4, 2025

Uh oh!

AndrewSirenko Jun 4, 2025

Uh oh!

xing-yang Jun 4, 2025

Uh oh!

AndrewSirenko Jun 5, 2025 •

edited

Loading

Uh oh!

xing-yang Jun 4, 2025

Uh oh!

AndrewSirenko Jun 5, 2025

Uh oh!

sunnylovestiramisu Jun 6, 2025

Uh oh!

gnufied commented Jun 5, 2025

Uh oh!

AndrewSirenko commented Jun 5, 2025

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

Allow PVC VACName to go from non-nil to nil #132106

Are you sure you want to change the base?

Allow PVC VACName to go from non-nil to nil #132106

Conversation

AndrewSirenko commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Jun 4, 2025

Uh oh!

k8s-ci-robot commented Jun 4, 2025

Uh oh!

k8s-ci-robot commented Jun 4, 2025

Uh oh!

AndrewSirenko Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

AndrewSirenko Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

xing-yang Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

AndrewSirenko Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xing-yang Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

AndrewSirenko Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

sunnylovestiramisu Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

gnufied commented Jun 5, 2025

Uh oh!

AndrewSirenko commented Jun 5, 2025

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

AndrewSirenko commented Jun 4, 2025 •

edited

Loading

AndrewSirenko Jun 5, 2025 •

edited

Loading