KEP-2885 Field Validation 1.27 GA graduation

[kevindelgado]

• One-line PR description: Update Field Validation to GA 1.27

• Issue link: Server Side Unknown Field Validation #2885

• Other comments: /cc @apelisse @liggitt @deads2k

[apelisse]

Generally looks good, asked a few clarification questions, thanks!

[apelisse]

To be fair, we might want to be a little more involved with that project. We've done a lot of work with regards to performance and openapi v3 that could benefit that project. I guess what I'm saying is that your sentence makes it sound like we just have to use kubeconform and that's it, but there is actually quite some work to do there to have parity with what we have today, but I think there is also a lot of potential to do a much better job than what we have.

[kevindelgado]

Reworded to hopefully capture what you are saying.

[apelisse]

That's good. I think in practice it's still very early to say anything about 1.26, but it's probably in the process of being rolled-out in different places. In the meantime, we must have some data for 1.25. Do we have metrics about how this is going? Can we know if it's working properly?

[liggitt]

As a data point, we (GKE) are seeing 1.25 API servers use less CPU and memory overall in the fleet than 1.24 API servers did at the same point in the 1.24 rollout. That at least means that any added cost of this feature was offset by other improvements in 1.25 in our use.

[kevindelgado]

I've summarized your comment inline. Let me know if there's a better place to put it or if we want/are able to provide more concrete metrics, to further support the claim I'm making that existing releases are not seeing issues in the wild.

[apelisse]

Why 2-3? Officially, kubectl is only supposed to support 1 version skew.

[liggitt]

if the cost is low, I'd be open to keeping the client-side validation in place until 1.26 goes out of support, even if it's not strictly required

[liggitt]

though... server-side validation has been on by default since 1.25, and I can't find evidence anyone has disabled it, so dropping client-side validation in 1.28 would be 1-version-skew-compatible with GA in 1.27 and practically would mean all OSS-supported versions as of August 2023 (1.25+) would have server-side validation enabled by default.

[apelisse]

August 20232022

[apelisse]

Yeah, as I mentioned below, there are two things:

1. do we stop checking if server-side strict validation is enabled on the server (I'd like that to stop as early as possible because it forces a pull on the openapi every single time).
2. do we stop support client-side validation at all? That is probably up to sig-cli to decide whether they are happy with the alternatives. That still requires a pull on openapi v2 but would only happen when someone specifically asks for it (which should be much more occasional).

[liggitt]

all OSS-supported versions as of August 2023 (1.25+)

August 20232022

August 20232022 :)

1.28 is estimated to release in August 2023, at which point 1.25+ will be still in support :)

[kevindelgado]

I've attempted to summarize this here. Let me know if it should be worded differently or if I should add anything about potentitally keeping it until 1.26 goes out of support

[apelisse]

The implications in this sentence seem incorrect.

Until version skew makes it incompatible for a client to hit a server
with field validation disabled, kubectl will need to continue to have
client-side validation available.

1. Until version skew makes it incompatible for a client to hit a server with field validation disabled, kubectl MUST continue to check if the feature is enabled server-side.
2. As long as the feature can be disabled server-side, kubectl MUST (SHOULD?) continue to have client-side validation
3. When the feature can't be disabled server-side, kubectl SHOULD (MAY?) remove client-side validation

[kevindelgado]

thanks, updated it.

[apelisse]

Side note, can you remind me where the benchmarks are for that?

[kevindelgado]

The benchmarks are here
https://github.com/kubernetes/kubernetes/blob/master/test/integration/apiserver/field_validation_test.go#L2936-L2996

and the analysis of it is here kubernetes/kubernetes#107848 (comment)

Updated the kep to more accurately reflect the findings of the benchmarking

[liggitt]

nit: maybe drop the "more rigorous" bit, since the new test will not actually exercise anything that is currently untested, it will just do it directly and in conformance

Suggested change

	- [] More rigorous e2e/conformance testing added to invoke field validation directly
	- [] Add conformance testing to invoke field validation directly

[kevindelgado]

done

[liggitt]

though... server-side validation has been on by default since 1.25, and I can't find evidence anyone has disabled it, so dropping client-side validation in 1.28 would be 1-version-skew-compatible with GA in 1.27 and practically would mean all OSS-supported versions as of August 2023 (1.25+) would have server-side validation enabled by default.

[liggitt]

I think this meant "could the gate be disabled during feature development", not "can the gate be disabled when locked to true at GA"

[kevindelgado]

Gotcha, I've reverted this for now.

It looks like based on this one example we remove the feature gate entirely once it's GA. This one did it three releases after going GA, but I can't tell if that wait is required, or if we can just remove the gate in the release that it goes GA. Do you know?

If so, I imagine we can just keep the gate in the kep.yaml file but adda comment that the gate was removed in release 1.x like I've done here?

[kevindelgado]

All feedback addressed. PTAL @apelisse @liggitt

[kevindelgado]

The benchmarks are here
https://github.com/kubernetes/kubernetes/blob/master/test/integration/apiserver/field_validation_test.go#L2936-L2996

and the analysis of it is here kubernetes/kubernetes#107848 (comment)

Updated the kep to more accurately reflect the findings of the benchmarking

[kevindelgado]

Reworded to hopefully capture what you are saying.

[kevindelgado]

I've summarized your comment inline. Let me know if there's a better place to put it or if we want/are able to provide more concrete metrics, to further support the claim I'm making that existing releases are not seeing issues in the wild.

[kevindelgado]

done

[kevindelgado]

thanks, updated it.

[kevindelgado]

I've attempted to summarize this here. Let me know if it should be worded differently or if I should add anything about potentitally keeping it until 1.26 goes out of support

[kevindelgado]

Gotcha, I've reverted this for now.

It looks like based on this one example we remove the feature gate entirely once it's GA. This one did it three releases after going GA, but I can't tell if that wait is required, or if we can just remove the gate in the release that it goes GA. Do you know?

If so, I imagine we can just keep the gate in the kep.yaml file but adda comment that the gate was removed in release 1.x like I've done here?

[liggitt]

Suggested change

	- Note: feature gate removed upon graduation to GA (1.27)
	- Note: feature gate locked to true upon graduation to GA (1.27) and removed two releases later (1.29)

[liggitt]

https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecation

Rule #8: Feature gates must be deprecated when the corresponding feature they control transitions a lifecycle stage as follows. Feature gates must function for no less than:

• Beta feature to GA: 6 months or 2 releases (whichever is longer)
  ...

we lock to true and give 2 releases for folks to update their invocations to drop explicitly setting the gate

[liggitt]

other than the gate removal timeline, lgtm

[kevindelgado]

Thanks, all feedback addressed.

/assign @deads2k for prod-readiness

[k8s-ci-robot]

@kevindelgado: GitHub didn't allow me to assign the following users: for, prod-readiness.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Thanks, all feedback addressed.

/assign @deads2k for prod-readiness

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.

[apelisse]

lgtm. thanks

[deads2k]

I'm ready to commit to reviewing designs for this purpose, but I'm not aware of something concrete at this moment. There's nothing concrete and this is a statement that we want to help the client leverage the server, not facilitate an attempt to move server responsibility client-side, correct?

[deads2k]

cc @lavalamp

for statement of future intent.

[kevindelgado]

You are correct that the future work is to have any client side validation tooling better leverage the information published by the server.

I guess, the goal of this statement is not to commit API machinery to working on client side validation tooling but rather suggest that it would behoove those looking to use/build client side validation tools to understand and incorporate what the API server offers (via open API, etc) to most accurately reflect the validation that the server is doing (i.e. the source of truth), and that sig-api-machinery is in a good position to do this work if it wants too

Let me know if I should word it better to reflect this goal

[deads2k]

Good, I'm happy with that future intent. I'd like to give daniel a couple days to have a comment prior to merge.

[lavalamp]

Can we make this clearer or just remove it? If we have to say something, I suggest something more like: SIG API Machinery's effort will go to producing clear and complete (and improving over time) API spec documents instead of making client validation. We strongly recommend that third party integrators make use of such documents.

[kevindelgado]

Sounds good, added

[lavalamp]

thanks, LGTM if @deads2k likes this wording!

[deads2k]

This lgtm for apimachinery and PRR

/lgtm
/approve
/hold

I'm holding so that @lavalamp gets a chance to comment on the statement of future intent: #3725 (comment)

Feel free to release when he's happy with that statement.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

[deads2k]

/tide merge-method-squash

[deads2k]

updated wording is ok with me.

/lgtm
/hold cancel