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

Remove Webhooks in Favor of OpenAPI/CEL CRD Validation #103

Closed
dudicoco opened this issue Dec 5, 2022 · 8 comments · Fixed by #821
Closed

Remove Webhooks in Favor of OpenAPI/CEL CRD Validation #103

dudicoco opened this issue Dec 5, 2022 · 8 comments · Fixed by #821
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. operational-excellence

Comments

@dudicoco
Copy link

dudicoco commented Dec 5, 2022

Hi,

I'm trying to understand the benefits which the Karpenter webhooks provide.

Looking at the validating webhook code it seems that the same validation could be done via the CRD schema.

The defaulting webhook doesn't seem to do anything at all. In addition, you can add default values to the CRD schema.

@ellistarn
Copy link
Contributor

ellistarn commented Dec 5, 2022

We haven't gone through the exercise of using builtin CRD validation. The defaults are provider specific. See: https://github.com/aws/karpenter/blob/main/pkg/apis/v1alpha5/provisioner.go#L45. I'd be curious to see if you could replace the validation webhooks.

@dudicoco
Copy link
Author

dudicoco commented Dec 6, 2022

We haven't gone through the exercise of using builtin CRD validation.

@ellistarn can you please elaborate on that?
It is much easier to add some fields to the CRD schema than writing additional code to validate it.
For example the following code could be easily replaced with a required: ["evictionSoftGracePeriod"] block within the schema.

I believe the defaulting webhook could also be avoided by adding default fields within the CRD schema.

@ellistarn
Copy link
Contributor

Interesting ideas. Would you be interested in contributing this?

@dudicoco
Copy link
Author

dudicoco commented Dec 6, 2022

@ellistarn sure.
Should I open a PR for the CRDs changes?

@ellistarn
Copy link
Contributor

Sure thing!

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Labeled for closure due to inactivity in 10 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 5, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 16, 2023
@tzneal tzneal removed lifecycle/closed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 17, 2023
@tzneal tzneal reopened this Apr 17, 2023
@github-actions
Copy link

github-actions bot commented May 8, 2023

Labeled for closure due to inactivity in 10 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 8, 2023
@ellistarn ellistarn added operational-excellence and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 8, 2023
@jonathan-innis jonathan-innis changed the title Why are the webhooks necessary? Remove Webhooks in Favor of CEL CRD Validation May 30, 2023
@jonathan-innis jonathan-innis added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. api labels May 30, 2023
@jonathan-innis
Copy link
Member

Kubernetes supports CEL for CRD validation. We should investigate this along with the other validations that are supported natively by the Open API specs (like defaults, minimum, maximums, etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. operational-excellence
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants