add validations to clusterQueue resource name

apis/kueue/v1alpha1/clusterqueue_webhook.go

@@ -17,14 +17,29 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/validation"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
+	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
 
-// log is for logging in this package.
-var clusterQueueLog = ctrl.Log.WithName("cluster-queue-webhook")
+var (
+	// log is for logging in this package.
+	clusterQueueLog = ctrl.Log.WithName("cluster-queue-webhook")
+
+	queueingStrategies = sets.NewString(string(StrictFIFO), string(BestEffortFIFO))
+)
+
+const isNegativeErrorMsg string = `must be greater than or equal to 0`
 
 func (cq *ClusterQueue) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
@@ -43,3 +58,105 @@ func (cq *ClusterQueue) Default() {
 		controllerutil.AddFinalizer(cq, ResourceInUseFinalizerName)
 	}
 }
+
+// +kubebuilder:webhook:path=/validate-kueue-x-k8s-io-v1alpha1-clusterqueue,mutating=false,failurePolicy=fail,sideEffects=None,groups=kueue.x-k8s.io,resources=clusterqueues,verbs=create;update,versions=v1alpha1,name=vclusterqueue.kb.io,admissionReviewVersions=v1
+
+var _ webhook.Validator = &ClusterQueue{}
+
+// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
+func (cq *ClusterQueue) ValidateCreate() error {
+	allErrs := ValidateClusterQueue(cq)
+	return allErrs.ToAggregate()
+}
+
+// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
+func (cq *ClusterQueue) ValidateUpdate(old runtime.Object) error {
+	allErrs := ValidateClusterQueue(cq)
+	return allErrs.ToAggregate()
+}
+
+// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
+func (cq *ClusterQueue) ValidateDelete() error {
+	return nil
+}
+
+func ValidateClusterQueue(cq *ClusterQueue) field.ErrorList {
+	path := field.NewPath("spec")
+
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, ValidateResources(cq, path.Child("resources"))...)
+	allErrs = append(allErrs, validateQueueingStrategy(string(cq.Spec.QueueingStrategy), path.Child("queueingStrategy"))...)
+	allErrs = append(allErrs, validateNamespaceSelector(cq.Spec.NamespaceSelector, path.Child("namespaceSelector"))...)
+
+	return allErrs
+}
+
+func ValidateResources(cq *ClusterQueue, path *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	for i, resource := range cq.Spec.Resources {
+		allErrs = append(allErrs, validateResourceName(string(resource.Name), path.Index(i).Child("name"))...)
+
+		flavorPath := path.Index(i).Child("flavors")
+		for j, flavor := range resource.Flavors {
+			allErrs = append(allErrs, validateFlavorName(string(flavor.Name), flavorPath.Index(j).Child("name"))...)
+			allErrs = append(allErrs, validateFlavorQuota(flavor, flavorPath.Index(j).Child("quota"))...)
+		}
+	}
+	return allErrs
+}
+
+func validateResourceName(name string, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	for _, msg := range utilvalidation.IsQualifiedName(name) {
+		allErrs = append(allErrs, field.Invalid(fldPath, name, msg))
+	}
+	return allErrs
+}
+
+func validateFlavorName(name string, path *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if msgs := utilvalidation.IsDNS1123Subdomain(name); len(msgs) > 0 {
+		for _, msg := range msgs {
+			allErrs = append(allErrs, field.Invalid(path, name, msg))
+		}
+	}
+	return allErrs
+}
+
+func validateFlavorQuota(flavor Flavor, path *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, validateResourceQuantity(flavor.Quota.Min, path.Child("min"))...)
+
+	if flavor.Quota.Max != nil {
+		allErrs = append(allErrs, validateResourceQuantity(*flavor.Quota.Max, path.Child("max"))...)
+		if flavor.Quota.Min.Cmp(*flavor.Quota.Max) > 0 {
+			allErrs = append(allErrs, field.Invalid(path.Child("min"), flavor.Quota.Min.String(), fmt.Sprintf("must be less than or equal to %s max", flavor.Name)))
+		}
+	}
+	return allErrs
+}
+
+// validateResourceQuantity enforces that specified quantity is valid for specified resource
+func validateResourceQuantity(value resource.Quantity, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if value.Cmp(resource.Quantity{}) < 0 {
+		allErrs = append(allErrs, field.Invalid(fldPath, value.String(), isNegativeErrorMsg))
+	}
+	return allErrs
+}
+
+func validateQueueingStrategy(strategy string, path *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if len(strategy) > 0 && !queueingStrategies.Has(strategy) {
+		allErrs = append(allErrs, field.Invalid(path, strategy, fmt.Sprintf("queueing strategy %s is not supported, available strategies are %v", strategy, queueingStrategies.List())))
+	}
+
+	return allErrs
+}
+
+func validateNamespaceSelector(selector *metav1.LabelSelector, path *field.Path) field.ErrorList {
+	allErrs := validation.ValidateLabelSelector(selector, path)
+	return allErrs
+}

apis/kueue/v1alpha1/clusterqueue_webhook_test.go

@@ -0,0 +1,168 @@
+/*
+Copyright 2021 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 v1alpha1_test
+
+// Rename the package to avoid circular dependencies which is caused by "sigs.k8s.io/kueue/pkg/util/testing".
+// See also: https://github.com/golang/go/wiki/CodeReviewComments#import-dot
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	. "sigs.k8s.io/kueue/apis/kueue/v1alpha1"
+	testingutil "sigs.k8s.io/kueue/pkg/util/testing"
+)
+
+func TestValidateClusterQueue(t *testing.T) {
+	specField := field.NewPath("spec")
+	resourceField := specField.Child("resources")
+
+	testcases := []struct {
+		name         string
+		clusterQueue *ClusterQueue
+		wantErr      field.ErrorList
+	}{
+		{
+			name: "native resources with qualified names",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("cpu").Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{},
+		},
+		{
+			name: "native resources with unqualified names",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("@cpu").Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(resourceField.Index(0).Child("name"), "@cpu", ""),
+			},
+		},
+		{
+			name: "extended resources with qualified names",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("example.com/gpu").Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{},
+		},
+		{
+			name: "extended resources with unqualified names",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("example.com/@gpu").Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(resourceField.Index(0).Child("name"), "example.com/@gpu", ""),
+			},
+		},
+		{
+			name: "flavor with qualified names",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("cpu").Flavor(testingutil.MakeFlavor("x86", "10").Obj()).Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{},
+		},
+		{
+			name: "flavor with unqualified names",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("cpu").Flavor(testingutil.MakeFlavor("invalid_name", "10").Obj()).Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(resourceField.Index(0).Child("flavors").Index(0).Child("name"), "invalid_name", ""),
+			},
+		},
+		{
+			name: "flavor quota with negative value",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("cpu").Flavor(testingutil.MakeFlavor("x86", "-1").Obj()).Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(resourceField.Index(0).Child("flavors").Index(0).Child("quota", "min"), "-1", ""),
+			},
+		},
+		{
+			name: "flavor quota with zero value",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("cpu").Flavor(testingutil.MakeFlavor("x86", "0").Obj()).Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{},
+		},
+		{
+			name: "flavor quota with min is equal to max",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("cpu").Flavor(testingutil.MakeFlavor("x86", "1").Max("1").Obj()).Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{},
+		},
+		{
+			name: "flavor quota with min is greater than max",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Resource(
+				testingutil.MakeResource("cpu").Flavor(testingutil.MakeFlavor("x86", "2").Max("1").Obj()).Obj(),
+			).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(resourceField.Index(0).Child("flavors").Index(0).Child("quota", "min"), "2", ""),
+			},
+		},
+		{
+			name:         "empty queueing strategy is supported",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Obj(),
+			wantErr:      field.ErrorList{},
+		},
+		{
+			name:         "unknown queueing strategy is not supported",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").QueueingStrategy("unknown").Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(specField.Child("queueingStrategy"), "unknown", ""),
+			},
+		},
+		{
+			name: "namespaceSelector with invalid labels",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").NamespaceSelector(&metav1.LabelSelector{
+				MatchLabels: map[string]string{"nospecialchars^=@": "bar"},
+			}).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(specField.Child("namespaceSelector", "matchLabels"), "nospecialchars^=@", ""),
+			},
+		},
+		{
+			name: "namespaceSelector with invalid expressions",
+			clusterQueue: testingutil.MakeClusterQueue("cluster-queue").NamespaceSelector(&metav1.LabelSelector{
+				MatchExpressions: []metav1.LabelSelectorRequirement{
+					{
+						Key:      "key",
+						Operator: "In",
+					},
+				},
+			}).Obj(),
+			wantErr: field.ErrorList{
+				field.Required(specField.Child("namespaceSelector", "matchExpressions").Index(0).Child("values"), ""),
+			},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			gotErr := ValidateClusterQueue(tc.clusterQueue)
+			if diff := cmp.Diff(tc.wantErr, gotErr, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); diff != "" {
+				t.Errorf("ValidateResources() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}

config/webhook/manifests.yaml

@@ -70,6 +70,26 @@ metadata:
   creationTimestamp: null
   name: validating-webhook-configuration
 webhooks:
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-kueue-x-k8s-io-v1alpha1-clusterqueue
+  failurePolicy: Fail
+  name: vclusterqueue.kb.io
+  rules:
+  - apiGroups:
+    - kueue.x-k8s.io
+    apiVersions:
+    - v1alpha1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - clusterqueues
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:

test/integration/controller/core/resourceflavor_controller_test.go

@@ -88,7 +88,7 @@ var _ = ginkgo.Describe("ResourceFlavor controller", func() {
 
 			ginkgo.By("Update clusterQueue's flavor")
 			gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterQueue), &cq)).To(gomega.Succeed())
-			cq.Spec.Resources[0].Flavors[0].Name = "foo-resourceFlavor"
+			cq.Spec.Resources[0].Flavors[0].Name = "foo-resourceflavor"
 			gomega.Expect(k8sClient.Update(ctx, &cq)).To(gomega.Succeed())
 
 			gomega.Eventually(func() error {