Migrate Admission Controller Validation to CEL #7690

omerap12 · 2025-01-14T13:19:06Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR migrates the admission controller validation to use CEL (Common Expression Language) for improved flexibility and consistency in validation logic at the API server level.

Which issue(s) this PR fixes:

Fixes #7665

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The validation process is now handled using CEL at the API server level. Expect different validation messages as a result.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

k8s-ci-robot · 2025-01-14T13:19:10Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

omerap12 · 2025-01-14T13:20:49Z

I've tested it locally and it works, but I still need to add integration tests. Can you check if it looks good to you?
@adrianmoisey
@voelzmo

adrianmoisey · 2025-01-14T13:31:46Z

Whoa! That is neat!
I didn't know that the validation could happen in the CRD itself.

Two questions:

The CRD is generated, I assume these CEL rules need to be configured in the types somewhere, so they are outputted to the generated CRD
Moving these to CELs means the tests move from unit tests to e2e tests, what does that look like?

omerap12 · 2025-01-14T13:41:00Z

Whoa! That is neat!

I didn't know that the validation could happen in the CRD itself.

Two questions:

The CRD is generated, I assume these CEL rules need to be configured in the types somewhere, so they are outputted to the generated CRD

Moving these to CELs means the tests move from unit tests to e2e tests, what does that look like?

Yup!

Regarding point 1, I need to explore how to configure it, but kubebuilder annotations do support this.
As for point 2, I haven’t implemented the tests yet, but that is the intended approach (this is one of the downsides of this method).

adrianmoisey · 2025-01-14T13:42:33Z

Regarding point 1, I need to explore how to configure it, but kubebuilder annotations do support this.

Yup, I see https://book.kubebuilder.io/reference/markers/crd-validation has these listed

Signed-off-by: Omer Aplatony <[email protected]>

omerap12 · 2025-01-14T14:55:59Z

Regarding point 1, I need to explore how to configure it, but kubebuilder annotations do support this.

Yup, I see https://book.kubebuilder.io/reference/markers/crd-validation has these listed

Now it was generated by kubebuilder.

omerap12 · 2025-01-14T15:02:35Z

Tests are failing because of the change I made in the admission controller, so we should migrate those tests into e2e/integration tests.

Signed-off-by: Omer Aplatony <[email protected]>

voelzmo · 2025-01-15T16:21:30Z

vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml

@@ -324,7 +325,11 @@ spec:
                            Name of the container or DefaultContainerResourcePolicy, in which
                            case the policy is used by the containers that don't have their own
                            policy specified.
+                          pattern: ^[a-zA-Z0-9-_]+$


I don't think you can introduce new CRD validations without increasing the apiVersion.

Additionally, containerName: '*' is explicitly supported as a catch-all solution, see

autoscaler/vertical-pod-autoscaler/pkg/utils/vpa/api.go

Lines 216 to 218 in adda3d4

if containerPolicy.ContainerName == vpa_types.DefaultContainerResourcePolicy {

defaultPolicy = &policy.ContainerPolicies[i]

}

Yes you are right it's just WIP at the moment.

Thanks, I will adjust :)

Added in: 9ea4821

voelzmo · 2025-01-15T16:33:37Z

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go

@@ -112,25 +112,8 @@ func parseVPA(raw []byte) (*vpa_types.VerticalPodAutoscaler, error) {

 // ValidateVPA checks the correctness of VPA Spec and returns an error if there is a problem.
 func ValidateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error {
-	if vpa.Spec.UpdatePolicy != nil {
-		mode := vpa.Spec.UpdatePolicy.UpdateMode
-		if mode == nil {


I don't see any checks added regarding updatePolicy.updateMode – is this intentional and those checks are implicitly done somewhere else now? Or do we need to add them as CEL validations as well?

In the CRD itself I saw this:
https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml#L456

So added in here: ea90c23

voelzmo

Sorry, didn't mean to approve 🙈

k8s-ci-robot · 2025-01-15T16:35:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: omerap12
Once this PR has been reviewed and has the lgtm label, please assign jbartosik for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

vertical-pod-autoscaler/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Omer Aplatony <[email protected]>

omerap12 · 2025-01-23T07:29:39Z

I'm opening this draft PR for review to trigger the e2e tests (just to ensure everything is working), and then I plan to update the apiVersion.
Pinging @voelzmo, @raywainman, and @adrianmoisey for reviews.

/hold

maxcao13 · 2025-02-18T17:38:08Z

vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go

 	ContainerPolicies []ContainerResourcePolicy `json:"containerPolicies,omitempty" patchStrategy:"merge" patchMergeKey:"containerName" protobuf:"bytes,1,rep,name=containerPolicies"`
 }

 // ContainerResourcePolicy controls how autoscaler computes the recommended
 // resources for a specific container.
+// +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"


I have a question about this. If I wanted to turn my VPA "On" and Off in my workload, either for testing purposes or whatever the case may be, would this change force me to remove containerResourcePolicies before doing so in order to apply because of cel validation?

This rule is saying that it's invalid to have both mode set to "Off" and controlledValues set to "RequestsAndLimits" at the same time. You don't necessarily need to remove the entire containerResourcePolicies before turning the VPA off. You only need to ensure that when you set mode to "Off", you're not also specifying controlledValues as "RequestsAndLimits" for the same container.

So If you're just toggling the mode between "On" and "Off" and you're not using controlledValues: RequestsAndLimits, you can do so freely without modifying other parts of your configuration.
If you have controlledValues: RequestsAndLimits set and you want to turn the VPA off, you would need to either remove the controlledValues field or set it to a different value before setting mode: Off.

I need to add tests for this behavior of course.

Got it thanks for the explanation 💯

maxcao13 · 2025-02-18T17:38:54Z

vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go

@@ -183,18 +186,23 @@ type PodResourcePolicy struct {
 	// +optional
 	// +patchMergeKey=containerName
 	// +patchStrategy=merge
+	// +kubebuilder:validation:MaxItems=100


Is max 100 items documented somewhere or just an arbitrary number?

It's still a work in progress - I just wanted to show what we can do with CEL validation, so I didn't document anything yet. I added this because of the runtime cost of CEL validation (https://kubernetes.io/docs/reference/using-api/cel/#runtime-cost-budget). I don’t think anyone will use more than 100 resource policies (right?)

Let's not introduce new validations in this PR, but rather convert the existing ones.
If we want to introduce new validations, we can discuss this in a new issue/PR.

Understood. Ill fix it

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/vertical-pod-autoscaler labels Jan 14, 2025

k8s-ci-robot requested a review from kgolab January 14, 2025 13:19

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2025

k8s-ci-robot requested a review from raywainman January 14, 2025 13:19

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 14, 2025

Migrate Admission Controller Validation to CEL

45785a4

Signed-off-by: Omer Aplatony <[email protected]>

omerap12 force-pushed the vpa-cel branch from b60d176 to 45785a4 Compare January 14, 2025 14:55

Add e2e tests and remove unit tests

7c07a31

Signed-off-by: Omer Aplatony <[email protected]>

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2025

voelzmo approved these changes Jan 15, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2025

voelzmo suggested changes Jan 15, 2025

View reviewed changes

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2025

omerap12 added 2 commits January 15, 2025 18:49

Add default for UpdateMode

ea90c23

Signed-off-by: Omer Aplatony <[email protected]>

Fixed * in containerPolicy.ContainerName

9ea4821

Signed-off-by: Omer Aplatony <[email protected]>

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 23, 2025

omerap12 marked this pull request as ready for review January 23, 2025 07:29

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2025

maxcao13 reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Admission Controller Validation to CEL #7690

Migrate Admission Controller Validation to CEL #7690

omerap12 commented Jan 14, 2025

k8s-ci-robot commented Jan 14, 2025

omerap12 commented Jan 14, 2025

adrianmoisey commented Jan 14, 2025

omerap12 commented Jan 14, 2025

adrianmoisey commented Jan 14, 2025

omerap12 commented Jan 14, 2025

omerap12 commented Jan 14, 2025

voelzmo Jan 15, 2025

omerap12 Jan 15, 2025

omerap12 Jan 16, 2025

voelzmo Jan 15, 2025

omerap12 Jan 15, 2025

voelzmo left a comment

k8s-ci-robot commented Jan 15, 2025

omerap12 commented Jan 23, 2025

maxcao13 Feb 18, 2025 •

edited

Loading

omerap12 Feb 19, 2025

maxcao13 Feb 19, 2025

maxcao13 Feb 18, 2025

omerap12 Feb 19, 2025

voelzmo Feb 19, 2025

omerap12 Feb 19, 2025

	if containerPolicy.ContainerName == vpa_types.DefaultContainerResourcePolicy {
	defaultPolicy = &policy.ContainerPolicies[i]
	}

Migrate Admission Controller Validation to CEL #7690

Are you sure you want to change the base?

Migrate Admission Controller Validation to CEL #7690

Conversation

omerap12 commented Jan 14, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jan 14, 2025

omerap12 commented Jan 14, 2025

adrianmoisey commented Jan 14, 2025

omerap12 commented Jan 14, 2025

adrianmoisey commented Jan 14, 2025

omerap12 commented Jan 14, 2025

omerap12 commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voelzmo left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 15, 2025

omerap12 commented Jan 23, 2025

maxcao13 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxcao13 Feb 18, 2025 •

edited

Loading