From 49fdb409b969917efb11d24dbca658dfbd618dde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Wed, 3 Apr 2024 20:56:07 +0200 Subject: [PATCH] Replace webhooks with CEL validation rules for LocalQueue (#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 --- apis/kueue/v1beta1/localqueue_types.go | 3 + .../crd/kueue.x-k8s.io_localqueues.yaml | 5 ++ .../crd/kueue.x-k8s.io_workloads.yaml | 2 + charts/kueue/templates/webhook/webhook.yaml | 20 ----- .../crd/bases/kueue.x-k8s.io_localqueues.yaml | 5 ++ .../crd/bases/kueue.x-k8s.io_workloads.yaml | 2 + config/components/webhook/manifests.yaml | 20 ----- pkg/util/testing/error_matchers.go | 78 +++++++++++++++++ pkg/util/testing/not_found_error_cmp.go | 77 ----------------- pkg/webhooks/localqueue_webhook.go | 77 ----------------- pkg/webhooks/localqueue_webhook_test.go | 83 ------------------- pkg/webhooks/webhooks.go | 4 - test/integration/webhook/localqueue_test.go | 7 +- 13 files changed, 101 insertions(+), 282 deletions(-) create mode 100644 pkg/util/testing/error_matchers.go delete mode 100644 pkg/util/testing/not_found_error_cmp.go delete mode 100644 pkg/webhooks/localqueue_webhook.go delete mode 100644 pkg/webhooks/localqueue_webhook_test.go diff --git a/apis/kueue/v1beta1/localqueue_types.go b/apis/kueue/v1beta1/localqueue_types.go index 035408714f..63ed9d6464 100644 --- a/apis/kueue/v1beta1/localqueue_types.go +++ b/apis/kueue/v1beta1/localqueue_types.go @@ -25,10 +25,13 @@ import ( // LocalQueueSpec defines the desired state of LocalQueue type LocalQueueSpec struct { // clusterQueue is a reference to a clusterQueue that backs this localQueue. + // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="field is immutable" 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])?)*$" type ClusterQueueReference string // LocalQueueStatus defines the observed state of LocalQueue diff --git a/charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml b/charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml index 905eb9f51d..76559bc9e8 100644 --- a/charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml +++ b/charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml @@ -73,7 +73,12 @@ spec: clusterQueue: description: clusterQueue is a reference to a clusterQueue that backs this localQueue. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string + x-kubernetes-validations: + - message: field is immutable + rule: self == oldSelf type: object status: description: LocalQueueStatus defines the observed state of LocalQueue diff --git a/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml b/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml index 13f5db94d9..a6d85f6841 100644 --- a/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml +++ b/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml @@ -7822,6 +7822,8 @@ spec: clusterQueue: description: clusterQueue is the name of the ClusterQueue that admitted this workload. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string podSetAssignments: description: PodSetAssignments hold the admission results for diff --git a/charts/kueue/templates/webhook/webhook.yaml b/charts/kueue/templates/webhook/webhook.yaml index cba9441220..8df48da682 100644 --- a/charts/kueue/templates/webhook/webhook.yaml +++ b/charts/kueue/templates/webhook/webhook.yaml @@ -580,26 +580,6 @@ webhooks: resources: - clusterqueues sideEffects: None - - admissionReviewVersions: - - v1 - clientConfig: - service: - name: '{{ include "kueue.fullname" . }}-webhook-service' - namespace: '{{ .Release.Namespace }}' - path: /validate-kueue-x-k8s-io-v1beta1-localqueue - failurePolicy: Fail - name: vlocalqueue.kb.io - rules: - - apiGroups: - - kueue.x-k8s.io - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - localqueues - sideEffects: None - admissionReviewVersions: - v1 clientConfig: diff --git a/config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml b/config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml index 8e062d9c22..af64fbf32e 100644 --- a/config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml +++ b/config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml @@ -58,7 +58,12 @@ spec: clusterQueue: description: clusterQueue is a reference to a clusterQueue that backs this localQueue. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string + x-kubernetes-validations: + - message: field is immutable + rule: self == oldSelf type: object status: description: LocalQueueStatus defines the observed state of LocalQueue diff --git a/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml b/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml index 742bc938b7..56a156e8bf 100644 --- a/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml +++ b/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml @@ -7807,6 +7807,8 @@ spec: clusterQueue: description: clusterQueue is the name of the ClusterQueue that admitted this workload. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string podSetAssignments: description: PodSetAssignments hold the admission results for diff --git a/config/components/webhook/manifests.yaml b/config/components/webhook/manifests.yaml index 4705b36dbf..04419124f4 100644 --- a/config/components/webhook/manifests.yaml +++ b/config/components/webhook/manifests.yaml @@ -536,26 +536,6 @@ webhooks: resources: - clusterqueues sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-kueue-x-k8s-io-v1beta1-localqueue - failurePolicy: Fail - name: vlocalqueue.kb.io - rules: - - apiGroups: - - kueue.x-k8s.io - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - localqueues - sideEffects: None - admissionReviewVersions: - v1 clientConfig: diff --git a/pkg/util/testing/error_matchers.go b/pkg/util/testing/error_matchers.go new file mode 100644 index 0000000000..92910bf338 --- /dev/null +++ b/pkg/util/testing/error_matchers.go @@ -0,0 +1,78 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package testing + +import ( + "fmt" + + "github.com/onsi/gomega/format" + "github.com/onsi/gomega/types" + apierrors "k8s.io/apimachinery/pkg/api/errors" +) + +func BeNotFoundError() types.GomegaMatcher { + return BeAPIError(NotFoundError) +} + +func BeForbiddenError() types.GomegaMatcher { + return BeAPIError(ForbiddenError) +} + +type errorMatcher int + +const ( + NotFoundError errorMatcher = iota + ForbiddenError + InvalidError +) + +func (em errorMatcher) String() string { + return []string{"NotFoundError", "ForbiddenError", "InvalidError"}[em] +} + +type apiError func(error) bool + +func (em errorMatcher) isAPIError(err error) bool { + return []apiError{apierrors.IsNotFound, apierrors.IsForbidden, apierrors.IsInvalid}[em](err) +} + +type isErrorMatch struct { + name errorMatcher +} + +func BeAPIError(name errorMatcher) types.GomegaMatcher { + return &isErrorMatch{ + name: name, + } +} + +func (matcher *isErrorMatch) Match(actual interface{}) (success bool, err error) { + err, ok := actual.(error) + if !ok { + return false, fmt.Errorf("%s expects an error", matcher.name.String()) + } + + return err != nil && matcher.name.isAPIError(err), nil +} + +func (matcher *isErrorMatch) FailureMessage(actual interface{}) (message string) { + return format.Message(actual, "to be a %s", matcher.name.String()) +} + +func (matcher *isErrorMatch) NegatedFailureMessage(actual interface{}) (message string) { + return format.Message(actual, "not to be %s", matcher.name.String()) +} diff --git a/pkg/util/testing/not_found_error_cmp.go b/pkg/util/testing/not_found_error_cmp.go deleted file mode 100644 index 96092040af..0000000000 --- a/pkg/util/testing/not_found_error_cmp.go +++ /dev/null @@ -1,77 +0,0 @@ -/* -Copyright 2022 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package testing - -import ( - "fmt" - - "github.com/onsi/gomega/format" - "github.com/onsi/gomega/types" - apierrors "k8s.io/apimachinery/pkg/api/errors" -) - -func BeNotFoundError() types.GomegaMatcher { - return ¬FoundErrorMatch{} -} - -type notFoundErrorMatch struct { -} - -func (matcher *notFoundErrorMatch) Match(actual interface{}) (success bool, err error) { - if actual == nil { - return false, nil - } - - err, ok := actual.(error) - if !ok { - return false, fmt.Errorf("NotFoundError expects an error") - } - - return err != nil && apierrors.IsNotFound(err), nil -} - -func (matcher *notFoundErrorMatch) FailureMessage(actual interface{}) (message string) { - return format.Message(actual, "to be a NotFound error") -} - -func (matcher *notFoundErrorMatch) NegatedFailureMessage(actual interface{}) (message string) { - return format.Message(actual, "not to be NotFound error") -} - -func BeForbiddenError() types.GomegaMatcher { - return &isForbiddenErrorMatch{} -} - -type isForbiddenErrorMatch struct { -} - -func (matcher *isForbiddenErrorMatch) Match(actual interface{}) (success bool, err error) { - err, ok := actual.(error) - if !ok { - return false, fmt.Errorf("ForbiddenError expects an error") - } - - return err != nil && apierrors.IsForbidden(err), nil -} - -func (matcher *isForbiddenErrorMatch) FailureMessage(actual interface{}) (message string) { - return format.Message(actual, "to be a Forbidden error") -} - -func (matcher *isForbiddenErrorMatch) NegatedFailureMessage(actual interface{}) (message string) { - return format.Message(actual, "not to be Forbidden error") -} diff --git a/pkg/webhooks/localqueue_webhook.go b/pkg/webhooks/localqueue_webhook.go deleted file mode 100644 index 002df8193c..0000000000 --- a/pkg/webhooks/localqueue_webhook.go +++ /dev/null @@ -1,77 +0,0 @@ -/* -Copyright 2022 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package webhooks - -import ( - "context" - - apivalidation "k8s.io/apimachinery/pkg/api/validation" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/klog/v2" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1" -) - -type LocalQueueWebhook struct{} - -func setupWebhookForLocalQueue(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(&kueue.LocalQueue{}). - WithValidator(&LocalQueueWebhook{}). - Complete() -} - -// +kubebuilder:webhook:path=/validate-kueue-x-k8s-io-v1beta1-localqueue,mutating=false,failurePolicy=fail,sideEffects=None,groups=kueue.x-k8s.io,resources=localqueues,verbs=create;update,versions=v1beta1,name=vlocalqueue.kb.io,admissionReviewVersions=v1 - -var _ webhook.CustomValidator = &LocalQueueWebhook{} - -// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type -func (w *LocalQueueWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { - q := obj.(*kueue.LocalQueue) - log := ctrl.LoggerFrom(ctx).WithName("localqueue-webhook") - log.V(5).Info("Validating create", "localQueue", klog.KObj(q)) - return nil, ValidateLocalQueue(q).ToAggregate() -} - -// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type -func (w *LocalQueueWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { - newQ := newObj.(*kueue.LocalQueue) - oldQ := oldObj.(*kueue.LocalQueue) - log := ctrl.LoggerFrom(ctx).WithName("localqueue-webhook") - log.V(5).Info("Validating update", "localQueue", klog.KObj(newQ)) - return nil, ValidateLocalQueueUpdate(newQ, oldQ).ToAggregate() -} - -// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type -func (w *LocalQueueWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { - return nil, nil -} - -func ValidateLocalQueue(q *kueue.LocalQueue) field.ErrorList { - var allErrs field.ErrorList - clusterQueuePath := field.NewPath("spec", "clusterQueue") - allErrs = append(allErrs, validateNameReference(string(q.Spec.ClusterQueue), clusterQueuePath)...) - return allErrs -} - -func ValidateLocalQueueUpdate(newObj, oldObj *kueue.LocalQueue) field.ErrorList { - return apivalidation.ValidateImmutableField(newObj.Spec.ClusterQueue, oldObj.Spec.ClusterQueue, field.NewPath("spec", "clusterQueue")) -} diff --git a/pkg/webhooks/localqueue_webhook_test.go b/pkg/webhooks/localqueue_webhook_test.go deleted file mode 100644 index b2cdfd5ff8..0000000000 --- a/pkg/webhooks/localqueue_webhook_test.go +++ /dev/null @@ -1,83 +0,0 @@ -/* -Copyright 2022 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package webhooks - -import ( - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "k8s.io/apimachinery/pkg/util/validation/field" - - kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1" - testingutil "sigs.k8s.io/kueue/pkg/util/testing" -) - -const ( - testLocalQueueName = "test-queue" - testLocalQueueNamespace = "test-queue-ns" -) - -func TestValidateLocalQueueCreate(t *testing.T) { - testCases := map[string]struct { - queue *kueue.LocalQueue - wantErr field.ErrorList - }{ - "should reject queue creation with an invalid clusterQueue": { - queue: testingutil.MakeLocalQueue(testLocalQueueName, testLocalQueueNamespace).ClusterQueue("invalid_cluster_queue").Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("clusterQueue"), "invalid_name", ""), - }, - }, - } - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - errList := ValidateLocalQueue(tc.queue) - if diff := cmp.Diff(tc.wantErr, errList, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); diff != "" { - t.Errorf("ValidateLocalQueueCreate() mismatch (-want +got):\n%s", diff) - } - }) - } -} - -func TestValidateLocalQueueUpdate(t *testing.T) { - testCases := map[string]struct { - before, after *kueue.LocalQueue - wantErr field.ErrorList - }{ - "clusterQueue cannot be updated": { - before: testingutil.MakeLocalQueue(testLocalQueueName, testLocalQueueNamespace).ClusterQueue("foo").Obj(), - after: testingutil.MakeLocalQueue(testLocalQueueName, testLocalQueueNamespace).ClusterQueue("bar").Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("clusterQueue"), nil, ""), - }, - }, - "status could be updated": { - before: testingutil.MakeLocalQueue(testLocalQueueName, testLocalQueueNamespace).Obj(), - after: testingutil.MakeLocalQueue(testLocalQueueName, testLocalQueueNamespace).PendingWorkloads(10).Obj(), - wantErr: field.ErrorList{}, - }, - } - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - errList := ValidateLocalQueueUpdate(tc.before, tc.after) - if diff := cmp.Diff(tc.wantErr, errList, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); diff != "" { - t.Errorf("ValidateLocalQueueUpdate() mismatch (-want +got):\n%s", diff) - } - }) - } -} diff --git a/pkg/webhooks/webhooks.go b/pkg/webhooks/webhooks.go index cc4abbe78f..a74a6b99a8 100644 --- a/pkg/webhooks/webhooks.go +++ b/pkg/webhooks/webhooks.go @@ -33,10 +33,6 @@ func Setup(mgr ctrl.Manager) (string, error) { return "ClusterQueue", err } - if err := setupWebhookForLocalQueue(mgr); err != nil { - return "Queue", err - } - if err := setupWebhookForAdmissionCheck(mgr); err != nil { return "AdmissionCheck", err } diff --git a/test/integration/webhook/localqueue_test.go b/test/integration/webhook/localqueue_test.go index a337f0d833..c78794f6b1 100644 --- a/test/integration/webhook/localqueue_test.go +++ b/test/integration/webhook/localqueue_test.go @@ -27,6 +27,11 @@ const queueName = "queue-test" var _ = ginkgo.Describe("Queue validating webhook", func() { ginkgo.When("Updating a Queue", func() { + ginkgo.It("Should reject bad value for spec.clusterQueue", func() { + ginkgo.By("Creating a new Queue") + obj := testing.MakeLocalQueue(queueName, ns.Name).ClusterQueue("invalid_name").Obj() + gomega.Expect(k8sClient.Create(ctx, obj)).Should(testing.BeAPIError(testing.InvalidError)) + }) ginkgo.It("Should reject the change of spec.clusterQueue", func() { ginkgo.By("Creating a new Queue") obj := testing.MakeLocalQueue(queueName, ns.Name).ClusterQueue("foo").Obj() @@ -38,7 +43,7 @@ var _ = ginkgo.Describe("Queue validating webhook", func() { gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), &updatedQ)).Should(gomega.Succeed()) updatedQ.Spec.ClusterQueue = "bar" return k8sClient.Update(ctx, &updatedQ) - }, util.Timeout, util.Interval).Should(testing.BeForbiddenError()) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) }) }) })