Skip to content

Discussion: Add maxItems=100 CEL Validation to PodResourcePolicy #8010

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 Apr 4, 2025 · 2 comments
Open

Discussion: Add maxItems=100 CEL Validation to PodResourcePolicy #8010

omerap12 opened this issue Apr 4, 2025 · 2 comments

Comments

@omerap12
Copy link
Member

omerap12 commented Apr 4, 2025

There is an ongoing PR that migrates some admission-controller validation to CEL validation, reducing the overall code footprint: kubernetes/autoscaler#7690.

I would like to extend this effort by introducing additional CEL validation to further simplify the code. Specifically, I propose adding the following rule to PodResourcePolicy:

// +kubebuilder:validation:XValidation:rule="!has(self.mode) || !has(self.controlledValues) || self.mode != 'Off' || self.controlledValues != 'RequestsAndLimits'",message="ControlledValues shouldn't be specified if container scaling mode is off"

This rule ensures that when container scaling is turned off (mode = 'Off'), users cannot set controlledValues = 'RequestsAndLimits'. In other words: If container scaling is disabled, requests and limits control should not be specified.

For complex rules like this, CEL validation requires boundary constraints to stay within the evaluation budget. Without them, we may encounter errors such as:

The CustomResourceDefinition "verticalpodautoscalers.autoscaling.k8s.io" is invalid: estimated rule cost exceeds budget (try adding maxItems, maxProperties, maxLength)

To address this, I propose adding the following constraint:

// +kubebuilder:validation:MaxItems=100

/area vertical-pod-autoscaler

@raywainman
Copy link
Contributor

@omerap12 I'm not super familiar with all of this but does this effectively mean that we are adding a limit to the number of containerPolicies items in the PodResourcePolicy?

@omerap12
Copy link
Member Author

@omerap12 I'm not super familiar with all of this but does this effectively mean that we are adding a limit to the number of containerPolicies items in the PodResourcePolicy?

Exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants