Fix empty string reclaim policy being considered valid in StorageClass

[wongma7]

What this PR does / why we need it: currently, storage classes are allowed to be created with reclaimPolicy=="" which has no documented meaning. Our docs say that if storageclass.reclaimPolicy is unspecified/nil, the default reclaimPolicy is Delete but they say nothing about empty string/"". So we should make "" invalid.

If the empty string from storageClass.ReclaimPolicy gets passed to a PV, the PV defaulting resets it to Retain. IMO this is okay since PV.Spec.PersistentVolumeReclaimPolicy is not a pointer and we can consider this PV empty string behaviour "legacy" behaviour. But storageclasses should know better and should not be able to have reclaimPolicy=="" when we have documented that it must be either "nil" or one of the 2 valid policies.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Special notes for your reviewer: commit 1 fixes an issue where the error validation tests would succeed no matter what because none of them had volumeBindingMode set.

Release note:

NONE

[wongma7]

@kubernetes/sig-storage-pr-reviews @kubernetes/sig-storage-api-reviews
/kind bug

[childsb]

/approve

[jsafrane]

/lgtm
for the code

/assign @liggitt
for approval, validation will reject StorageClasses that were working before, but IMO it's a good idea here.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: childsb, jsafrane, wongma7
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

If they are not already assigned, you can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

The pull request process is described 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

[liggitt]

/hold

Tightening validation in a way that causes existing persisted data to be considered invalid is a breaking API change.

What is the current behavior of a storage class with reclaimPolicy:""?

[wongma7]

The current behaviour is that PVs provisioned for the storageclass are created with reclaimPolicy Retain

[liggitt]

A possible approach to improve validation for objects which are fatally flawed is being explored in #64907, but I'm not sure that applies here (the produced objects do work, they just behave in potentially unexpected ways).

[liggitt]

The options I see:

1. making "" invalid breaks existing data, which isn't acceptable
2. changing "" to default to "Delete" in API defaulting means a storage class read from a v1.12 and written to a v1.11 server (HA cluster version skew) would be rejected by the v1.11 server as trying to mutate the retainPolicy on update.
3. making the PV controller turn a storage class with retainPolicy:"" into a PV with retainPolicy: Delete is an API-invisible behavior change. It would not affect existing PVs, but would mean a storage class from 1.11 would start to work differently in 1.12
4. update documentation to indicate that a storage class with retainPolicy:"" produces PVs with retainPolicy:"Retain"

[wongma7]

@liggitt
I would add 1 more option which I like the best:

5. Similar to 2 but we change "" to default to "Retain". The argument for this is that this is what PVs do (I neglected to say in comment above, but the reason storageClass.reclaimPolicy=="" results in PV.spec.reclaimPolicy is because of pv defaulting code https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/v1/defaults.go#L238). And we avoid the "a storage class from 1.11 would start to work differently in 1.12" problem. Argument against this is that users typically think of "" as equivalent to nil/"unspecified" (see: confusion over "" vs nil pvc.spec.storageClassName). But I would rather they be surprised that their "" has become Retain than surprised that their "" has become Delete, because the latter case might result in unexpected data loss.

3 is also okay I think. Our docs say the default provisioning policy is Delete and again, users think of nil=="". In other words, instead of blindly passing "" to the pv defaulting code, we change it to Delete first.

4 I guess you mean "Retain" in the last word there. I know the "" vs nil pvc.spec.storageClassName documentation sets the precedent that it's possible (but not easy) to teach users ""!=nil.

Also, I reeeally wish 1 were possible. Isn't there some way to automatically update all storageClasses with "" to "Retain" during an upgrade?

[wongma7]

On second thought the “unexpected data loss” risk might be overstated :) so options2 and 3 get some more points from me, but I still want 5 the most

[k8s-github-robot]

/test all

Tests are more than 96 hours old. Re-running tests.

[k8s-ci-robot]

@wongma7: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	64e63e2	link	/test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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/test-infra repository.