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.

Sign up for GitHub

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

Jump to bottom

Migrate Admission Controller Validation to CEL #7690

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Migrate Admission Controller Validation to CEL #7690

wants to merge 3 commits into from
+132 −144

Conversation

omerap12
Copy link
Member

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
Copy link
Contributor

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

@k8s-ci-robot 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 k8s-ci-robot requested a review from kgolab January 14, 2025 13:19
@k8s-ci-robot 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 k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 14, 2025
@omerap12
Copy link
Member Author

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
Copy link
Member

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

Two questions:

  1. 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
  2. Moving these to CELs means the tests move from unit tests to e2e tests, what does that look like?

@omerap12
Copy link
Member Author

Whoa! That is neat!

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

Two questions:

  1. 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

  2. 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
Copy link
Member

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

@omerap12
Copy link
Member Author

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
Copy link
Member Author

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

@k8s-ci-robot 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
voelzmo approved these changes Jan 15, 2025
View reviewed changes
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-_]+$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes you are right it's just WIP at the moment.
  2. Thanks, I will adjust :)

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@k8s-ci-robot 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
voelzmo suggested changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@voelzmo voelzmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't mean to approve 🙈

@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot 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
Add default for UpdateMode
ea90c23 
Signed-off-by: Omer Aplatony <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voelzmo voelzmo voelzmo requested changes

@kgolab kgolab Awaiting requested review from kgolab

@raywainman raywainman Awaiting requested review from raywainman

Assignees
No one assigned
Labels
area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

VPA: Migrate admission webhook validations to CEL where possible
4 participants
@omerap12 @k8s-ci-robot @adrianmoisey @voelzmo