KEP-2885: Beta Graduation Criteria for Field Validation

[kevindelgado]

• One-line PR description: Update PR for beta milestone targeting 1.24 (kubectl support and alpha follow-ups)

• Issue link: Server Side Unknown Field Validation #2885

[sftim]

One question, one minor and optional tweak.

[wojtek-t]

Why do we need it then?

[kevindelgado]

"Strict", "Warn", and "Ignore" are the semantics used by the fieldValidation query param and will be permanent.

"Server" and "Client" are transitionary semantics for now since we want to give the caller the ability to choose until client side validation is no longer an option. It makes sense to offer both "server" and "strict" for now so that the user sees the expected behavior regardless of which set of semantics they are using.

[kevindelgado]

cc @liggitt

[kevindelgado]

This should be ready for a prod-readiness review now @deads2k

[apelisse]

The conditional behavior based on the servers ability sounds confusing to me. Also, what do you do on a "client-dry-run"? i.e. kubectl create --dry-run=client --validate? Does that automatically fall back to client-side validation? How do you explain that? "Why are the errors looking so different"?

[kevindelgado]

Added a note that we will treat --dry-run=client as not being connected to any server at-all and thus will trigger a fallback to CS validation

[liggitt]

doesn't dry-run=client trigger local mode and prevent getting the schema from the server at all, and therefore do no validation?

[kevindelgado]

@apelisse do you know if there's some mechanism under the hood that using a cached schema or something.

FWIW I can definitely see that validation is being performed with --dry-run=client when running a simple experiment on a generic 1.23 cluster

[apelisse]

Yes, client dry-run still asks the server for the openapi. It has pros and cons ...

[liggitt]

@apelisse:
I'm not usually a big fan of flags whose behavior depends on some hard-to-assess logic (in this case, looking at the existence of a query param defined in the openapi). I'd rather want the behavior to be strictly defined and unconditional, with a proper error when used incorrectly (rather than the query param being merely ignored).

kubectl already inspects published openapi to determine support for server side dry run. Since client side validation doesn't work well and there's an end goal of dropping the current implementation from create/replace/apply, I don't think we should do anything to increase the CLI flag surface to explicitly depend on it

[apelisse]

kubectl already inspects published openapi to determine support for server side dry run.

It does, but it doesn't implicitly change the behavior of flags based on that, it just stops you from doing something broken, no?

[apelisse]

Since my main concern is changing the behavior without the person knowing, I suggest we go with that sort of messaging:

client-side validation is deprecated and server-side validation will be used instead by default if available. We will fallback to client-side validation in the mean time if ..., along with a warning to let users know what's going on."

[kevindelgado]

Since my main concern is changing the behavior without the person knowing, I suggest we go with that sort of messaging:

client-side validation is deprecated and server-side validation will be used instead by default if available. We will fallback to client-side validation in the mean time if ..., along with a warning to let users know what's going on."

Modified the proposal to include this messaging

[k8s-triage-robot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[deads2k]

PRR lgtm and the beta criteria lgtm too, but I'll leave time for additional reviewers there.

/approve

[apelisse]

LGTM for me, thank you Kevin!

[apelisse]

Not asking for a change here, but using the same yaml/json library client side would already be an improvement (even if it doesn't work with openapi support, it would at least detect duplicates for example?)

[apelisse]

Yes, client dry-run still asks the server for the openapi. It has pros and cons ...

[soltysh]

this lgtm from sig-cli pov

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, kevindelgado, soltysh

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:

• keps/prod-readiness/OWNERS [deads2k]
• keps/sig-api-machinery/OWNERS [deads2k]

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

[kevindelgado]

Anything I'm missing @liggitt or @apelisse? We have lgtms from PRR and sig-cli

[liggitt]

no, lgtm

[liggitt]

/lgtm
per #3081 (review)