Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KEP-5073: Declarative Validation of Kubernetes Native Types With validation-gen #5074

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

KEP-5073: Declarative Validation of Kubernetes Native Types With validation-gen #5074

wants to merge 13 commits into from

Conversation

aaron-prindle
Copy link

One-line PR description: Implement declarative validation of Kubernetes native types using validation-gen to generate validation code.
Issue link: #5073

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 22, 2025
@k8s-ci-robot k8s-ci-robot requested review from jpbetz and sttts January 22, 2025 19:33
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 22, 2025
@aaron-prindle aaron-prindle mentioned this pull request Jan 22, 2025
Open
6 tasks
@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch 3 times, most recently from 73f9dcd to fdcec1b Compare January 22, 2025 19:56
@aaron-prindle aaron-prindle mentioned this pull request Jan 22, 2025
Open
15 tasks
@jpbetz
Copy link
Contributor

jpbetz commented Jan 22, 2025

/label lead-opted-in
/milestone v1.33

@k8s-ci-robot k8s-ci-robot added this to the v1.33 milestone Jan 22, 2025
@k8s-ci-robot k8s-ci-robot added the lead-opted-in Denotes that an issue has been opted in to a release label Jan 22, 2025
@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch 2 times, most recently from 996751a to b31c75f Compare January 22, 2025 20:55
@aaron-prindle
Copy link
Author

aaron-prindle commented Jan 22, 2025 •
edited
Loading

/cc @deads2k @liggitt @msau42 @thockin

NOTE: adding @msau42 and @thockin from api-reviewers given a goal of the KEP is to streamline API reviews (not for an API review on the KEP itself)

@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch from b31c75f to c1bce5a Compare January 23, 2025 01:13
@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch 5 times, most recently from ab4fe70 to 68fe8c5 Compare January 23, 2025 22:55
@bowei
Copy link
Member

bowei commented Jan 29, 2025

I haven't looked in detail at the specifics but this is great direction to go!

@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch 4 times, most recently from 115aabe to 1d7e6e6 Compare January 29, 2025 23:58
@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch from 1d7e6e6 to abd9ff7 Compare January 30, 2025 00:03
@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch from 051733d to 07f5524 Compare January 30, 2025 20:44
robscott
robscott reviewed Feb 4, 2025
View reviewed changes
keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md

#### `validation-gen` Has No Plans To Use CEL Validation Directly

The previous Declarative Validation proposal ([KEP-4153](https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4153-declarative-validation)) proposed using CEL for a number of the complex validations present in the current Kubernetes validation logic for native types (cross-field, transition, etc.). The `validation-gen` solution presented here uses go code directly for the validations which means we do not plan on evaluating CEL server side as we can write arbitrary go code to perform server side validations. This allows the `validation-gen` solution to be highly flexible and performant. If we reach a point where CEL makes sense, we can evaluate it at that time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something here, but if an eventual goal is to make this available for CRDs, it seems like the move away from CEL would complicate that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can get to a place where you don't even need CEL for most validation, and the code that was invoked was LITERALLY the same code as is run for built-in types, that sounds pretty good, no?

Also, and I know it's not for you, but imagine you could specifically say "and then invoke the pod validation logic" without inlining it all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something here, but if an eventual goal is to make this available for CRDs

I see this project largely as build-in types catching up with what CRDs already have-- a way to declare validation directly in the schema. In the case of CRDs this is via OpenAPI and x-kubernetes-* extensions. In the case of built-ins, it's via // + tags.

The benefit to CRDs will be a follow on as move toward parity. The two at top of mind for me are:

  • CRDs become able to compose-in built-in types while retaining validation fidelity. The approach today of copy-pasting OpenAPI from built-in types into CRD schemas is extremely lossy.
  • As we bring built-ins to parity with CRDs, we add capabilities to CRDs to bring the systems into alignment. I expect that to include a mix of new OpenAPI capabilities and CEL capabilities on the CRD side.

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md
* Ability to give users direct access to the actual API validation rules (via publishing OpenAPI rules for native types), which are currently only available to developers willing and able to find and read the hand written validation rules.
* Ability to enable clients to perform validation of native types earlier in development workflows ("shift-left"), such as with a Git pre-submit linter.
* Ability to improve API composition. In particular CRDs that embed native types (such as PodTemplate), which gain validation of the native type automatically. This has the potential to simplify controller development and improve end user experiences when using CRDs.
* (Eventually) bring CRD validation and built-in validation together with the same set of server-side validations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong on this, but I thought there was an overall architectural goal to push a large portion of new API development out of k/k and into CRDs. If I'm understanding that correctly, why aren't CRDs an immediate goal?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most immediate goal is to lessen the burden and risk of hand-written validation code and to simultaneously lay pavement for CRDs to call the same code, with ~0 perf hit.

@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch from 3f616dd to 432fe22 Compare February 4, 2025 19:23
@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch 2 times, most recently from fa57b1f to cacfe64 Compare February 4, 2025 21:01
@jpbetz
Copy link
Contributor

jpbetz commented Feb 5, 2025

/approve
For PRR. The deadline is tomorrow and while we still have lots of specifics to discuss, I think we're good for PRR for 1.33 given the current plan and the fact that we're not migrating at scale in 1.33

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaron-prindle, jpbetz, thockin

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2025
@thockin
Copy link
Member

thockin commented Feb 6, 2025

Do we feel like this is converging enough to merge and iterate? Basically, in or out for 33?

@aaron-prindle aaron-prindle force-pushed the declarative-validation-with-validation-gen branch from cacfe64 to 65f989d Compare February 7, 2025 18:34
aaron-prindle
aaron-prindle commented Feb 7, 2025
View reviewed changes
keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md

## Future Work
### "New K8s Native APIs" Design Partner For Declarative Validation in 1.34
For 1.34 the Declarative Validation working group is looking for a design partner to collaborate with aid in the UX, "validator" creation, and developer process of using Declarative Validation. The partnership would be focused around the new k8s native API creation context of declarative validation (vs the migrating of current k8s native APIs).
Copy link
Author

@aaron-prindle aaron-prindle Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke offline w/ @thockin and @jpbetz regarding the idea of having a design partner for Declarative Validation specifically circle the 1.34 release timeline around the users story of "new k8s API creation".

@pohly - would DRA be interested in pursuing being a design partner for Declarative Validation? (re: from discussion in #5034)

Happy to discuss more here if so or via slack, etc. Thanks

@dom4ha dom4ha mentioned this pull request Feb 7, 2025
Open
@yongruilin yongruilin mentioned this pull request Feb 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@robscott robscott robscott left review comments

@jpbetz jpbetz jpbetz left review comments

@thockin thockin thockin approved these changes

@deads2k deads2k deads2k left review comments

@yongruilin yongruilin yongruilin left review comments

@sttts sttts Awaiting requested review from sttts

@liggitt liggitt Awaiting requested review from liggitt

@msau42 msau42 Awaiting requested review from msau42

Assignees

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lead-opted-in Denotes that an issue has been opted in to a release sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.33
Development

Successfully merging this pull request may close these issues.
9 participants
@aaron-prindle @jpbetz @bowei @k8s-ci-robot @thockin @pohly @robscott @deads2k @yongruilin