feat: implement validation webhook for Queue #264

knight42 · 2022-05-15T12:30:29Z

Signed-off-by: Jian Zeng anonymousknight96@gmail.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Ref #171

Special notes for your reviewer:

knight42 · 2022-05-15T12:31:29Z

/cc @alculquicondor

apis/kueue/v1alpha1/queue_webhook_test.go

kerthcet · 2022-05-23T10:37:00Z

test/integration/webhook/v1alpha1/queue_test.go

+const queueName = "queue-test"
+
+var _ = ginkgo.Describe("Queue validating webhook", func() {
+	ginkgo.Context("When updating a Queue", func() {


Add a normal path as you did in workload validation?

didn't we settle on using when instead of context?

alculquicondor · 2022-07-06T20:19:46Z

Sorry for the delay, I'm finally catching up with kueue's PRs. I'll get back to this tomorrow.

/cc

apis/kueue/v1alpha1/queue_webhook.go

apis/kueue/v1alpha1/queue_webhook_test.go

ahg-g · 2022-07-18T13:05:10Z

@knight42 better to comment on the reviewer's comments so that they get notified that you addressed their review. I don't think @alculquicondor got notified that you uploaded a new patch addressing his comments.

ahg-g · 2022-07-19T10:35:13Z

apis/kueue/v1alpha1/queue_webhook.go

+}
+
+func ValidateQueueCreate(q *Queue) field.ErrorList {
+	return apivalidation.ValidateObjectMetaAccessor(q.GetObjectMeta(), true, apivalidation.NameIsDNSLabel, field.NewPath("metadata"))


we want to validate spec.ClusterQueue as a valid name, not metadata.Name (this one is validated by the api-server)

And the method to use to validate is NameIsDNSSubdomain https://github.com/kubernetes/kubernetes/blob/8c8a4cf3e4a18e97359ce750530a4fa27bbd3b88/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go#L60

@ahg-g @alculquicondor Thanks for the suggestion, it should be fixed now. Please take another look

kerthcet · 2022-07-21T03:41:46Z

main.go

@@ -161,7 +161,11 @@ func setupControllers(mgr ctrl.Manager, cCache *cache.Cache, queues *queue.Manag
 		setupLog.Error(err, "Unable to create webhook", "webhook", failedWebhook)
 		os.Exit(1)
 	}
-	//+kubebuilder:scaffold:builder
+	if err := (&kueuev1alpha1.Queue{}).SetupWebhookWithManager(mgr); err != nil {


We have settled this in SetupWebhooks. Plz remove it.

kerthcet · 2022-07-21T03:53:21Z

apis/kueue/v1alpha1/queue_webhook_test.go

+		wantErr field.ErrorList
+	}{
+		"should reject invalid clusterQueue": {
+			queue: testingutil.MakeQueue(testQueueName, testQueueNamespace).ClusterQueue("invalid_queue").Obj(),


"invalid_clusterQueue" ?

kerthcet · 2022-07-21T03:55:45Z

apis/kueue/v1alpha1/queue_webhook_test.go

+		queue   *Queue
+		wantErr field.ErrorList
+	}{
+		"should reject invalid clusterQueue": {


should reject queue creation with an invalid clusterQueue name?

kerthcet · 2022-07-21T03:58:24Z

apis/kueue/v1alpha1/queue_webhook_test.go

+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			errList := ValidateQueueCreate(tc.queue)
+			if len(errList) == 0 {


Why do we need this? Diff the results seems enough.

kerthcet · 2022-07-21T03:59:31Z

apis/kueue/v1alpha1/queue_webhook_test.go

+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			errList := ValidateQueueUpdate(tc.before, tc.after)
+			if len(tc.wantErr) == 0 {


ditto, if no errors expected, you can set the wantErr: field.ErrorList{}.

@knight42 can you address this please as well?

I have overlooked.. should be fixed this time

ahg-g

few nits, can you please address Kante's comments so we merge this?

ahg-g · 2022-07-26T18:17:33Z

apis/kueue/v1alpha1/queue_webhook.go

+func ValidateQueueCreate(q *Queue) field.ErrorList {
+	var allErrs field.ErrorList
+	for _, msg := range apivalidation.NameIsDNSSubdomain(string(q.Spec.ClusterQueue), false) {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "clusterQueue"), q.Spec.ClusterQueue, msg))


create the path once before the for loop

ahg-g · 2022-07-26T18:20:05Z

test/integration/webhook/v1alpha1/queue_test.go

+const queueName = "queue-test"
+
+var _ = ginkgo.Describe("Queue validating webhook", func() {
+	ginkgo.Context("When updating a Queue", func() {


didn't we settle on using when instead of context?

ahg-g · 2022-07-27T16:21:31Z

Hi @knight42 are you still on this one?

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

knight42 · 2022-07-28T14:08:26Z

All comments should be addressed now, PTAL

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

ahg-g · 2022-07-28T23:57:40Z

/label tide/merge-method-squash

ahg-g · 2022-07-28T23:58:02Z

/lgtm
/approve

k8s-ci-robot · 2022-07-28T23:58:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, knight42

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~~ [ahg-g]

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

* feat: implement validation webhook for Queue Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * address comments Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * fix: fix build error Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * feat: validate clusterQueue in Queue Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * fix: setup webhook for Queue Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * test: update test Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * address comments Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 15, 2022

k8s-ci-robot requested review from ArangoGutierrez and denkensk May 15, 2022 12:30

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 15, 2022

k8s-ci-robot requested a review from alculquicondor May 15, 2022 12:31

knight42 force-pushed the feat/queue-validate-webhook branch from 756d510 to fd983f1 Compare May 15, 2022 13:10

kerthcet reviewed May 23, 2022

View reviewed changes

alculquicondor reviewed Jul 12, 2022

View reviewed changes

knight42 force-pushed the feat/queue-validate-webhook branch from fd983f1 to 4c2ba7d Compare July 13, 2022 16:14

knight42 requested a review from alculquicondor July 13, 2022 16:15

ahg-g mentioned this pull request Jul 13, 2022

Add webhook for APIs defaulting and validation #171

Closed

ahg-g reviewed Jul 19, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2022

knight42 force-pushed the feat/queue-validate-webhook branch from 939e44a to 4b47e8b Compare July 20, 2022 14:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2022

kerthcet reviewed Jul 21, 2022

View reviewed changes

ahg-g reviewed Jul 26, 2022

View reviewed changes

knight42 added 6 commits July 28, 2022 22:02

feat: implement validation webhook for Queue

4d4a75e

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

address comments

45816d5

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

fix: fix build error

dfac09d

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

feat: validate clusterQueue in Queue

1b57fc6

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

fix: setup webhook for Queue

08c7612

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

test: update test

fef91f4

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

knight42 force-pushed the feat/queue-validate-webhook branch 2 times, most recently from 44c5063 to 9f7d9b3 Compare July 28, 2022 14:07

address comments

242fc47

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

knight42 force-pushed the feat/queue-validate-webhook branch from 9f7d9b3 to 242fc47 Compare July 28, 2022 17:13

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 28, 2022

k8s-ci-robot assigned ahg-g Jul 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2022

k8s-ci-robot merged commit 4fca39d into kubernetes-sigs:main Jul 28, 2022

knight42 deleted the feat/queue-validate-webhook branch July 29, 2022 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement validation webhook for Queue #264

feat: implement validation webhook for Queue #264

knight42 commented May 15, 2022

knight42 commented May 15, 2022

kerthcet May 23, 2022 •

edited

Loading

ahg-g Jul 26, 2022 •

edited

Loading

alculquicondor commented Jul 6, 2022

ahg-g commented Jul 18, 2022

ahg-g Jul 19, 2022

alculquicondor Jul 19, 2022

knight42 Jul 20, 2022

kerthcet Jul 21, 2022

kerthcet Jul 21, 2022

kerthcet Jul 21, 2022

kerthcet Jul 21, 2022

kerthcet Jul 21, 2022

ahg-g Jul 28, 2022

knight42 Jul 28, 2022

ahg-g left a comment

ahg-g Jul 26, 2022

ahg-g Jul 26, 2022 •

edited

Loading

ahg-g commented Jul 27, 2022

knight42 commented Jul 28, 2022

ahg-g commented Jul 28, 2022

ahg-g commented Jul 28, 2022

k8s-ci-robot commented Jul 28, 2022

feat: implement validation webhook for Queue #264

feat: implement validation webhook for Queue #264

Conversation

knight42 commented May 15, 2022

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:

knight42 commented May 15, 2022

kerthcet May 23, 2022 • edited Loading

Choose a reason for hiding this comment

ahg-g Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

alculquicondor commented Jul 6, 2022

ahg-g commented Jul 18, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

ahg-g commented Jul 27, 2022

knight42 commented Jul 28, 2022

ahg-g commented Jul 28, 2022

ahg-g commented Jul 28, 2022

k8s-ci-robot commented Jul 28, 2022

kerthcet May 23, 2022 •

edited

Loading

ahg-g Jul 26, 2022 •

edited

Loading

ahg-g Jul 26, 2022 •

edited

Loading