Replace webhooks with CEL validation rules for LocalQueue (kubernetes…

…-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
village-way · Apr 3, 2024 · 49fdb40 · 49fdb40
1 parent e63709b
commit 49fdb40
Show file tree

Hide file tree

Showing 13 changed files with 101 additions and 282 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/pkg/webhooks/localqueue_webhook.go b/pkg/webhooks/localqueue_webhook.go