Skip to content

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

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

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

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

omerap12 opened this issue Jan 7, 2025 · 11 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.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 21, 2025
@omerap12
Copy link
Member Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 21, 2025
@raywainman
Copy link
Contributor

I don't follow this part:

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?

Doesn't the webhook need to run in the critical path of pods being created so that it can adjust resources? Or is this proposing that the admission-controller gets out of the way of pods that are NOT controlled by VPA?

Also on this overall proposal - I'm all for standardizing this stuff to align with best practices. Using this seems like a nice way to simplify things in the admission-controller.

@adrianmoisey
Copy link
Member

I don't follow this part:

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?

Doesn't the webhook need to run in the critical path of pods being created so that it can adjust resources? Or is this proposing that the admission-controller gets out of the way of pods that are NOT controlled by VPA?

Sorry, the English in that sentence wasn't very good.

I had a not-very-well-though-out-idea, that in theory the webhook pod could be removed from the critical path and be replaced with a CEL rule. May be (if possible) the recommender could create a rule (and update those rules as the recommendations change) that has enough logic in it to replace what the admission-controller pod does.

I highly doubt that this would work, but thought I'd write down the idea.

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.

6 participants