Extract mount option pv validation in its own package

[gnufied]

This PR implements #42707

We can do creation time validation of PVs without putting everything in pkg/api/validation.go

[k8s-reviewable]

This change is 

[gnufied]

/assign @saad-ali

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: gnufied

Needs approval from an approver in each of these OWNERS Files:

• cmd/kubelet/OWNERS
• pkg/api/OWNERS
• pkg/registry/OWNERS
• pkg/volume/OWNERS

We suggest the following people:
cc @lavalamp
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[gnufied]

I have verified that this works as expected - https://gist.github.com/gnufied/ace932d6fa9e20fb0b90053ea6a078b7

Also size of kubectl binary doesn't increase.

[gnufied]

@k8s-bot non-cri e2e test this
@k8s-bot cvm gce e2e test this

[saad-ali]

@liggitt @smarterclayton @justinsb This PR addresses #42573 by moving the validation logic from the pkg/api/validation to a a strategy (pkg/registry/core/persistentvolume/strategy.go). kubectl does not reference strategies so the size of kubectl with this PR is back to normal. However, api-server does, and therefore it's size is still larger than previous releases:

-rwxrwxr-x. 1 hekumar hekumar 120M Mar  8 20:19 kube-apiserver <--- 1.5
and now it is 142M

If you guys are ok with this increase in size of the api-server binary, I'm fine with this PR. If not, then we can move forward with #42706 instead. And revisit a better approach for mount options validation in 1.7.

[smarterclayton]

So when I was talking to you today, what I was assuming would be that all mount options validation would be split out into it's own package, with no dependency on the probe plugins mechanism. I don't see why you need to probe plugins - the API defines the structs, those always exist, and the validation for those should be totally deterministic.

I'd be ok with that change - I don't think this is the correct way to include validation. SupportsMountOption should be code that is in a separate package (no dependencies), and then referenced both by each mount plugin and referenced by this validation code. Does that make sense?

[smarterclayton]

SupportsMountOption should be an API decision, not a plugin decision. The plugin decision can inherit from the api decision code, but the converse should not be true.

[gnufied]

I think I am getting vague tingling of what you are proposing, but it would be very helpful if you can show what you mean by some code snippet just for illustration.

The volume API defines interfaces which dictate what they support and what they don't. We need to initialize concrete implementation of those interfaces to find out, if they support particular feature or not. That is what ProbeVolumePlugins does - it initializes concrete implementation of volume plugins to figure out what features they support. I don't like having to initialize the plugin to find out its capabilities and have opened a separate issue for that - #42386 but I am still thinking about how to implement that properly. May be this will help with that issue as well.

[gnufied]

Okay, so what you are saying is - it shouldn't be upto plugin author(such as NFS) to decide if it supports mount options or not? Plugin should inherit the api decision but not override it?

[smarterclayton]

As an example:

func ValidateNFSSource(source *api.NFSVolumeSource) .. {
  // check all valid nfs sources
}

Validation is an API concern - not a plugin concern. If we expose an API, we must decide (at the time of the api) what mount options to accept. If we are going to validate, the validation should be part of the api (if a future version of NFS adds more mount options, that's an API change). If we don't do anything specific for a mount version, then it's not an API change.

The NFS author must specify all accepted mount options for a particular version of kubernetes, and if they want to validate them, follow the rules for introducing and deprecating mount options (we only loosen validation, we rarely tighten it).

I.e. if in 1.6 we support 3 options on NFS: "squash", "root", "uid", and validation checks those, when we later add support for "noroot", we'll allow it to be passed. But a 1.6 cluster would not allow it unless we patched the validation.

Validation code referenced by the API needs to be completely static - the API server either decides what is valid, or checks a few options and passes the rest through, but the plugin author should be adding their validation rules into a package that only calls validation, and then importing the validation into API validation and also reusing it in the plugin (or doing more extensive validation in the plugin, and letting simpler validation happen in the API code).

[gnufied]

Just so I understand this, the pseudo code of this package will look like:

func Validate(pv *api.PersistentVolume) Errorlist {

case pv.Spec.VolumeSource.(type) {
when api.NFSSource:
  validateNFSSource(pv)
when api.EBSElasticStore:
  validateEBSSource(pv).
...


return erroList
}

Each plugin author can go ahead and add their own validation there and this package can be then imported from pkg/api/validation and chosen to validate the PV during creation.

I think we can do that, need to think bit more.

[calebamiles]

moved into the 1.6 milestone as one of the possible fixes for #42573.

cc: @ethernetdan, @kubernetes/release-team

[justinsb]

If you guys are ok with this increase in size of the api-server binary

I reported the original issue about kubectl, and I think that while the size increase is unfortunate, we don't need to fix it to release 1.6.0. I think the same applies to kube-apiserver (more so, because it is only downloaded onto masters).

I reported it in case it was either a silly mistake (we were passing odd compiler options) or in case it was something deeper. It feels like it is something deeper which I don't know a lot about, so I am happy to leave it up to y'all to decide which option - if any - makes sense.

[gnufied]

Closing this in favour of #42811