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

feat(crds): Migrate from a webhook to CEL validation for aws nodetemplates #4586

Closed

Conversation

knechtionscoding
Copy link

@knechtionscoding knechtionscoding commented Sep 8, 2023

Fixes kubernetes-sigs/karpenter#677

Description

Replaces the webhook validation with CEL validation.

Starting in 1.25 CEL validation is available instead of maintaining a separate webhook.

This starts the process of moving from webhook validation to CEL validation.

Discussed in kubernetes-sigs/karpenter#103 as well.

How was this change tested?

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 17ba116
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/64fa6a85bbd796000811925b

@knechtionscoding
Copy link
Author

I have a couple of questions that I'm not sure about:

I'm not sure of the intent behind:

I also don't love how CEL handles nested fields. It would be better if we could provide the validation closer to the actual spec of the AWSTemplate. Right now I have to write a bunch of rules at the top level which feels very wrong and like I should be able to write them at the spec level. Not sure where that's defined though or if the spec is pulled in from a different package.

@github-actions
Copy link
Contributor

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

@njtran
Copy link
Contributor

njtran commented Sep 28, 2023

Hey @knechtionscoding, thanks for the PR. Would you be able to make changes to this to enable this only for the v1beta1 APIs? We're actively migrating the APIs over, where the alpha APIs will be soon deprecated.

@knechtionscoding
Copy link
Author

Sure! Is there a good way to test this change? And I'm still not sure this is the right place for some of the validation. Some the aws parts might be better in karpenter-core?

@engedaam
Copy link
Contributor

engedaam commented Sep 29, 2023

Sure! Is there a good way to test this change? And I'm still not sure this is the right place for some of the validation. Some the aws parts might be better in karpenter-core?

@knechtionscoding Which aws parts are thinking would be better in karpenter-core?

@engedaam
Copy link
Contributor

engedaam commented Oct 3, 2023

@knechtionscoding Have you made any progress on CEL for v1beta1 APIs? I have some work going toward the effort and just wanted to know your progress

@knechtionscoding
Copy link
Author

@knechtionscoding Have you made any progress on CEL for v1beta1 APIs? I have some work going toward the effort and just wanted to know your progress

I have not, I've been swamped and haven't had a chance. Feel free to take it.

@github-actions
Copy link
Contributor

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

@ellistarn
Copy link
Contributor

This is being worked on by @engedaam

@ellistarn ellistarn closed this Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for Usage of Cert-Manager in helm chart for webhook certificate
4 participants