Replace webhooks with CEL validation rules for LocalQueue #1938

IrvingMg · 2024-04-02T13:33:35Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

As mentioned in #463, we can enforce immutability and other validations using CEL rules instead of webhooks. Thus, this PR removes the validating webhooks for the localqueue type and adds the corresponding validation rules.

Which issue(s) this PR fixes:

Relates to #463

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Replaced LocalQueue admission webhook with CRD validation rules.

ACTION REQUIRED: Requires Kubernetes 1.25 or newer

linux-foundation-easycla · 2024-04-02T13:33:40Z

The committers listed above are authorized under a signed CLA.

✅ login: IrvingMg / name: Irving Mondragón (2a2aa19, 2e6756e, df12e67, ff6c139, 56ec5ad)

k8s-ci-robot · 2024-04-02T13:33:44Z

Welcome @IrvingMg!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kueue has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-04-02T13:33:45Z

Hi @IrvingMg. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2024-04-02T13:33:53Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`ff6c139`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/660da020e71780000861ae38

trasc · 2024-04-03T07:14:29Z

/ok-to-test

alculquicondor · 2024-04-03T13:17:03Z

apis/kueue/v1beta1/localqueue_types.go

 	ClusterQueue ClusterQueueReference `json:"clusterQueue,omitempty"`
 }

 // ClusterQueueReference is the name of the ClusterQueue.
+// +kubebuilder:validation:MaxLength=253
+// +kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$"


@jpbetz there is no CEL library for common k8s regexes, is there?

Not yet, but @alexzielenski is working on adding them as part of declarative validation, both OpenAPI formats and CEL utility functions to check common regexes (like dns1123label) are planned.

EDIT: So using a RE2 regex directly that exactly matches the official k8s regex would be safe to migrate in the future.

pkg/util/testing/not_found_error_cmp.go

alculquicondor · 2024-04-03T13:56:11Z

By the way, this requires a release note.

Also, please include the text ACTION REQUIRED, followed by a request to use kubernetes 1.25 or newer https://kubernetes.io/blog/2022/09/23/crd-validation-rules-beta/

pkg/util/testing/error_matchers.go

alculquicondor · 2024-04-03T18:54:43Z

/release-note-edit

Replaced LocalQueue admission webhook with CRD validation rules.

ACTION REQUIRED: Requires Kubernetes 1.25 or newer

alculquicondor · 2024-04-03T18:55:02Z

/lgtm
/approve

k8s-ci-robot · 2024-04-03T18:55:07Z

LGTM label has been added.

Git tree hash: 9f9596cdbc9d18fb265b3b1084f763a3c961ffbf

k8s-ci-robot · 2024-04-03T18:55:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, IrvingMg

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alculquicondor]

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

…-sigs#1938) * Replace webhooks with CEL for LocalQueue * Rename error matchers file * Add generic error matcher * Rename api error functions * Reimplement existing error matchers with the generic one

k8s-ci-robot requested review from mimowo and trasc April 2, 2024 13:33

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 2, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 2, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 2, 2024

IrvingMg changed the title ~~Replace webhooks with CEL validation rules~~ Replace webhooks with CEL validation rules for LocalQueue Apr 3, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 3, 2024

IrvingMg marked this pull request as ready for review April 3, 2024 07:33

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 Apr 3, 2024

k8s-ci-robot requested a review from alculquicondor April 3, 2024 07:33

Replace webhooks with CEL for LocalQueue

2a2aa19

IrvingMg force-pushed the replace-webhooks-with-cel branch from 8519deb to 2a2aa19 Compare April 3, 2024 08:18

alculquicondor reviewed Apr 3, 2024

View reviewed changes

IrvingMg added 2 commits April 3, 2024 16:16

Rename error matchers file

56ec5ad

Add generic error matcher

df12e67

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 3, 2024

alculquicondor reviewed Apr 3, 2024

View reviewed changes

pkg/util/testing/error_matchers.go Outdated Show resolved Hide resolved

pkg/util/testing/error_matchers.go Outdated Show resolved Hide resolved

pkg/util/testing/error_matchers.go Show resolved Hide resolved

IrvingMg added 2 commits April 3, 2024 20:20

Rename api error functions

2e6756e

Reimplement existing error matchers with the generic one

ff6c139

k8s-ci-robot assigned alculquicondor Apr 3, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2024

k8s-ci-robot merged commit 49fdb40 into kubernetes-sigs:main Apr 3, 2024
14 checks passed

k8s-ci-robot added this to the v0.7 milestone Apr 3, 2024

trasc deleted the replace-webhooks-with-cel branch April 4, 2024 06:11

IrvingMg mentioned this pull request Apr 25, 2024

REQUEST: New membership for IrvingMg kubernetes/org#4907

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace webhooks with CEL validation rules for LocalQueue #1938

Replace webhooks with CEL validation rules for LocalQueue #1938

IrvingMg commented Apr 2, 2024 •

edited by k8s-ci-robot

linux-foundation-easycla bot commented Apr 2, 2024 •

edited

k8s-ci-robot commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

netlify bot commented Apr 2, 2024 •

edited

trasc commented Apr 3, 2024

alculquicondor Apr 3, 2024

jpbetz Apr 3, 2024 •

edited

alculquicondor commented Apr 3, 2024

alculquicondor commented Apr 3, 2024

alculquicondor commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

Replace webhooks with CEL validation rules for LocalQueue #1938

Replace webhooks with CEL validation rules for LocalQueue #1938

Conversation

IrvingMg commented Apr 2, 2024 • edited by k8s-ci-robot

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?

linux-foundation-easycla bot commented Apr 2, 2024 • edited

k8s-ci-robot commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

netlify bot commented Apr 2, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

trasc commented Apr 3, 2024

alculquicondor Apr 3, 2024

Choose a reason for hiding this comment

jpbetz Apr 3, 2024 • edited

Choose a reason for hiding this comment

alculquicondor commented Apr 3, 2024

alculquicondor commented Apr 3, 2024

alculquicondor commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

IrvingMg commented Apr 2, 2024 •

edited by k8s-ci-robot

linux-foundation-easycla bot commented Apr 2, 2024 •

edited

netlify bot commented Apr 2, 2024 •

edited

jpbetz Apr 3, 2024 •

edited