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

VPA: Migrate admission webhook validations to CEL where possible #7665

Open
omerap12 opened this issue Jan 7, 2025 · 7 comments · May be fixed by #7690
Open

VPA: Migrate admission webhook validations to CEL where possible #7665

omerap12 opened this issue Jan 7, 2025 · 7 comments · May be fixed by #7690
Assignees
Labels
area/vertical-pod-autoscaler kind/feature Categorizes issue or PR as related to a new feature.

Comments

@omerap12
Copy link
Member

omerap12 commented Jan 7, 2025

Which component are you using?:
/area vertical-pod-autoscaler

Is your feature request designed to solve a problem? If so describe the problem this feature should solve.:
Currently, VPA validation is implemented programmatically in the admission webhook (pkg/admission-controller/resource/vpa/handler.go). This makes the validation rules less declarative and harder to maintain. CEL validation would provide validation at the API server level before object persistence, improving validation efficiency and maintainability.
Ref: https://kubernetes.io/docs/reference/using-api/cel/

Describe the solution you'd like.:
Moving applicable validations to CEL would align with Kubernetes best practices and provide earlier validation in the request flow.

Describe any alternative solutions you've considered.:

Additional context.:

/assign

@omerap12 omerap12 added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 7, 2025
@adrianmoisey
Copy link
Member

I'm curious to see what this would look like, as I'm a little sceptical if it's going to be better than what we currently have

@omerap12
Copy link
Member Author

omerap12 commented Jan 7, 2025

Why do you feel that way? I think using CEL is an improvement over our current approach.. It aligns with Kubernetes best practices and enables earlier validation in the request flow.

@omerap12
Copy link
Member Author

omerap12 commented Jan 7, 2025

We can also remove large chunks of code which is an idea I always appreciate

@adrianmoisey
Copy link
Member

My understanding for the need for Validating Admission Policy is to allow users to do validation without having to run infrastructure to do that validation, since it can happen inside the api-server. Ie: https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/3488-cel-admission-control/README.md#motivation

Since we have to be running a a pod to receive webhooks, it means the value of a Validating Admission Policy is decreased slightly.

Obviously a project like the VPA can benefit from a few of the other advantages, such as less network calls.

We can also remove large chunks of code which is an idea I always appreciate

My assumption is that we would be replacing Go code with CEL code. I'm not sure if that will be better or worse.

@omerap12 omerap12 linked a pull request Jan 14, 2025 that will close this issue
@adrianmoisey
Copy link
Member

I had an idea for MutatingAdmissionPolicies... I'm unsure if this idea if good, but hear me out.

Instead of the VPA webhook pod receiving webhooks, making a decision, and returning the response... what if a VPA pod were to write MutatingAdmissionPolicies to the API server, and keep those MutatingAdmissionPolicies updated as the recommendations change?
This was the webhook pod is not in the critical path of pods being created. I don't know if all the VPA features are possible (such as the limit/request ratio that is maintained), but may be this is worth exploring at some stage?

@voelzmo
Copy link
Contributor

voelzmo commented Jan 21, 2025

I had an idea for MutatingAdmissionPolicies... I'm unsure if this idea if good, but hear me out.

Instead of the VPA webhook pod receiving webhooks, making a decision, and returning the response... what if a VPA pod were to write MutatingAdmissionPolicies to the API server, and keep those MutatingAdmissionPolicies updated as the recommendations change? This was the webhook pod is not in the critical path of pods being created. I don't know if all the VPA features are possible (such as the limit/request ratio that is maintained), but may be this is worth exploring at some stage?

This might be an interesting way to simplify things for the future, especially given that MutatingAdmissionPolicies can get their configuration from custom resources – this way we could always just replace the currently set requests with the information from the VPA status target.

Currently, however, this feature is alpha in k8s 1.32, there's still some way to go until we can make this a first-class thing in VPA.

@omerap12
Copy link
Member Author

I agree. This has potential, but we shouldn't support it as long as MutatingAdmissionPolicies remain in Alpha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertical-pod-autoscaler kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants