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.

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

KEP-2876: CRD Validation Expression Language #2877

Merged
merged 34 commits into from
Sep 9, 2021

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Aug 18, 2021

Primary goal is to allow the majority of the validation use cases that currently must be handled by a webhook, to instead be handled by adding inline validation expressions directly into the schema of a CRD.

cc @cici37 @leilajal @DangerOnTheRanger @deads2k @lavalamp @fedebongio @apelisse
/sig api-machinery

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 18, 2021
@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. labels Aug 18, 2021
@jpbetz jpbetz mentioned this pull request Aug 18, 2021
21 tasks
@negz
Copy link

negz commented Aug 20, 2021

This is exciting; we've been holding out for something like this (or considering building it) for @crossplane.

@kikisdeliveryservice kikisdeliveryservice changed the title KEP: CRD Validation Expression Language KEP-2876: CRD Validation Expression Language Aug 30, 2021
Update status to be implementable.
operation, it should be safe to integrate.

Additional limits we can put in place, as needed, include:
- A max execution time limit to but could bound running time of CEL programs. This would require
Copy link
Member

@jiahuif jiahuif Sep 8, 2021

Choose a reason for hiding this comment

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

which means a CEL program may timeout, and thus, may fail. Similar to webhook, there may be need of a policy about whether to deny or allow it when the validation fails.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we'd ever allow if a validation rule timed out.

Copy link
Member

Choose a reason for hiding this comment

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

Which means failure policy will not be implemented here

Copy link
Member

Choose a reason for hiding this comment

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

CEL evaluations are multiple orders of magnitude faster than typical webhook invocations... I suspect we'd bound the CEL expression complexity and handle timeouts the same way we would if etcd caused a write operation to timeout by returning a generic server timeout error

Copy link
Member

Choose a reason for hiding this comment

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

It make perfect sense to treat runtime CEL failures as an internal error, just like the write timeout case. I think it would still be helpful to mention that in the KEP.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 to using complexity estimations. I think I also agree that a server timeout error is appropriate. I'll make some notes in the KEP.

@jiahuif
Copy link
Member

jiahuif commented Sep 8, 2021

There is no mention of how a user should test their embedded expressions. Something that runs the validation with a given set of input/output could help.

With this feature, working on full-stack YAML can be so complicated that we may need a testing framework for that.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 9, 2021
@deads2k
Copy link
Contributor

deads2k commented Sep 9, 2021

/label tide/merge-method-squash
/lgtm
/approve
/hold

holding to prevent instant merge. I think we've got all blocking comments in, but I'll quickly solicit to be sure.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. labels Sep 9, 2021
…/README.md

Co-authored-by: David Eads <deads2k@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 9, 2021
@liggitt
Copy link
Member

liggitt commented Sep 9, 2021

no blocking comments remaining from me

@lavalamp
Copy link
Member

lavalamp commented Sep 9, 2021

/hold cancel

I saw an earlier version and I don't think this needs to wait for another look from me. 👍

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2021
@deads2k
Copy link
Contributor

deads2k commented Sep 9, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 085480e into kubernetes:master Sep 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 9, 2021
rikatz pushed a commit to rikatz/enhancements that referenced this pull request Feb 1, 2022
* WIP

* WIP

* add alternatives

* Add WebAssembly alternative section.

* Update the KEP for validation

* update title to focus on validation, expand on alternatives

* Replace TODOs with UNRESOLVED and flesh out risks

* Update KEP

* Update KEP based on feedback received.

* Address SIG bi-weekly feedback

* Add PRR review.
Update status to be implementable.

* Remove security review.

* Apply feedback

* Fill out PRR questionare

* Clarify semantic equality

* Address unsolved section.

* Move type field into future work

* Fix toc checking.

* Address need for test for rollback

* Add graduation criteria for beta

* Add test plan and graduation criteria

* Address liggitts feedback

* Update keps/sig-api-machinery/2876-crd-validation-expression-language/README.md

Co-authored-by: David Eads <deads2k@users.noreply.github.com>

* Add comments about CEL expression complexity

Co-authored-by: Kermit Alexander <kermitalexandr@google.com>
Co-authored-by: cici37 <cicih@google.com>
Co-authored-by: David Eads <deads2k@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants