feat: enforce non-empty nodeSelector with CEL instead of the webhook

api/v1alpha1/nodereadinessrule_types.go

@@ -118,8 +118,12 @@ type NodeReadinessRuleSpec struct {
 
 	// nodeSelector limits the scope of this rule to a specific subset of Nodes.
 	//
+	// An empty selector matches every Node in the cluster, so it is rejected.
+	// At least one of matchLabels or matchExpressions must be set.
+	//
 	// +required
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="nodeSelector is immutable"
+	// +kubebuilder:validation:XValidation:rule="(has(self.matchLabels) && size(self.matchLabels) > 0) || (has(self.matchExpressions) && size(self.matchExpressions) > 0)",message="nodeSelector must not be empty"
 	NodeSelector metav1.LabelSelector `json:"nodeSelector,omitempty,omitzero"`
 
 	// conditionPolicy controls how the conditions list is evaluated.

charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml

@@ -147,8 +147,11 @@ spec:
                 - message: enforcementMode is immutable
                   rule: self == oldSelf
               nodeSelector:
-                description: nodeSelector limits the scope of this rule to a specific
-                  subset of Nodes.
+                description: |-
+                  nodeSelector limits the scope of this rule to a specific subset of Nodes.
+
+                  An empty selector matches every Node in the cluster, so it is rejected.
+                  At least one of matchLabels or matchExpressions must be set.
                 properties:
                   matchExpressions:
                     description: matchExpressions is a list of label selector requirements.
@@ -196,6 +199,9 @@ spec:
                 x-kubernetes-validations:
                 - message: nodeSelector is immutable
                   rule: self == oldSelf
+                - message: nodeSelector must not be empty
+                  rule: (has(self.matchLabels) && size(self.matchLabels) > 0) || (has(self.matchExpressions)
+                    && size(self.matchExpressions) > 0)
               taint:
                 description: |-
                   taint defines the specific Taint (Key, Value, and Effect) to be managed

config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml

@@ -147,8 +147,11 @@ spec:
                 - message: enforcementMode is immutable
                   rule: self == oldSelf
               nodeSelector:
-                description: nodeSelector limits the scope of this rule to a specific
-                  subset of Nodes.
+                description: |-
+                  nodeSelector limits the scope of this rule to a specific subset of Nodes.
+
+                  An empty selector matches every Node in the cluster, so it is rejected.
+                  At least one of matchLabels or matchExpressions must be set.
                 properties:
                   matchExpressions:
                     description: matchExpressions is a list of label selector requirements.
@@ -196,6 +199,9 @@ spec:
                 x-kubernetes-validations:
                 - message: nodeSelector is immutable
                   rule: self == oldSelf
+                - message: nodeSelector must not be empty
+                  rule: (has(self.matchLabels) && size(self.matchLabels) > 0) || (has(self.matchExpressions)
+                    && size(self.matchExpressions) > 0)
               taint:
                 description: |-
                   taint defines the specific Taint (Key, Value, and Effect) to be managed

internal/controller/nodeselector_cel_test.go

@@ -0,0 +1,110 @@
+/*
+Copyright 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 controller
+
+import (
+	"context"
+	"strings"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+
+	nodereadinessiov1alpha1 "sigs.k8s.io/node-readiness-controller/api/v1alpha1"
+)
+
+// The empty nodeSelector constraint is enforced by CEL on the CRD rather than by
+// the validating webhook, so it applies on every cluster instead of only where the
+// optional webhook is deployed. These specs assert it through the API server.
+var _ = Describe("NodeReadinessRule nodeSelector CEL validation", func() {
+	var celCtx context.Context
+
+	newRule := func(name string, selector metav1.LabelSelector) *nodereadinessiov1alpha1.NodeReadinessRule {
+		return &nodereadinessiov1alpha1.NodeReadinessRule{
+			ObjectMeta: metav1.ObjectMeta{Name: name},
+			Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{
+				Conditions: []nodereadinessiov1alpha1.ConditionRequirement{
+					{Type: "CELReady", RequiredStatus: corev1.ConditionTrue},
+				},
+				NodeSelector:    selector,
+				Taint:           corev1.Taint{Key: "readiness.k8s.io/cel-selector", Effect: corev1.TaintEffectNoSchedule},
+				EnforcementMode: nodereadinessiov1alpha1.EnforcementModeContinuous,
+			},
+		}
+	}
+
+	BeforeEach(func() { celCtx = context.Background() })
+
+	AfterEach(func() {
+		list := &nodereadinessiov1alpha1.NodeReadinessRuleList{}
+		if err := k8sClient.List(celCtx, list); err == nil {
+			for i := range list.Items {
+				r := &list.Items[i]
+				if strings.HasPrefix(r.Name, "cel-selector-") {
+					r.Finalizers = nil
+					_ = k8sClient.Update(celCtx, r)
+					_ = k8sClient.Delete(celCtx, r)
+				}
+			}
+		}
+	})
+
+	// A wholly absent selector is caught by the required marker rather than by the
+	// CEL rule, because the field is omitempty/omitzero and serialises away
+	// entirely. Both reject the object, they just report it differently.
+	It("rejects a rule with no selector at all", func() {
+		err := k8sClient.Create(celCtx, newRule("cel-selector-absent", metav1.LabelSelector{}))
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("spec.nodeSelector: Required value"))
+	})
+
+	It("rejects a selector whose matchLabels map is present but empty", func() {
+		err := k8sClient.Create(celCtx, newRule("cel-selector-emptylabels",
+			metav1.LabelSelector{MatchLabels: map[string]string{}}))
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("nodeSelector must not be empty"))
+	})
+
+	It("rejects a selector whose matchExpressions list is present but empty", func() {
+		err := k8sClient.Create(celCtx, newRule("cel-selector-emptyexprs",
+			metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{}}))
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("nodeSelector must not be empty"))
+	})
+
+	It("accepts a selector with matchLabels", func() {
+		rule := newRule("cel-selector-labels", metav1.LabelSelector{
+			MatchLabels: map[string]string{"node-role.kubernetes.io/worker": ""},
+		})
+		Expect(k8sClient.Create(celCtx, rule)).To(Succeed())
+
+		persisted := &nodereadinessiov1alpha1.NodeReadinessRule{}
+		Expect(k8sClient.Get(celCtx, types.NamespacedName{Name: rule.Name}, persisted)).To(Succeed())
+		Expect(persisted.Spec.NodeSelector.MatchLabels).To(HaveKey("node-role.kubernetes.io/worker"))
+	})
+
+	It("accepts a selector with only matchExpressions", func() {
+		rule := newRule("cel-selector-exprs", metav1.LabelSelector{
+			MatchExpressions: []metav1.LabelSelectorRequirement{
+				{Key: "node-role.kubernetes.io/control-plane", Operator: metav1.LabelSelectorOpDoesNotExist},
+			},
+		})
+		Expect(k8sClient.Create(celCtx, rule)).To(Succeed())
+	})
+})

internal/webhook/nodereadinessgaterule_webhook.go

@@ -61,14 +61,13 @@ func (w *NodeReadinessRuleWebhook) validateSpec(
 ) field.ErrorList {
 	var allErrs field.ErrorList
 
-	// validate that the nodeSelector isn't empty
-	selector, err := metav1.LabelSelectorAsSelector(&spec.NodeSelector)
-	if err != nil {
+	// An empty nodeSelector is rejected by CEL on the CRD, which runs before
+	// validating webhooks, so that case cannot reach here. What CEL cannot express
+	// is whether the selector actually parses, for example an unknown
+	// matchExpressions operator, so that check stays.
+	if _, err := metav1.LabelSelectorAsSelector(&spec.NodeSelector); err != nil {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "nodeSelector"), spec.NodeSelector, err.Error()))
 	}
-	if selector != nil && selector.Empty() {
-		allErrs = append(allErrs, field.Required(field.NewPath("spec", "nodeSelector"), "nodeSelector must not be empty"))
-	}
 
 	return allErrs
 }

internal/webhook/nodereadinessgaterule_webhook_test.go

@@ -55,19 +55,6 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() {
 	})
 
 	Context("Spec Validation", func() {
-		It("should validate nodeSelector is not empty", func() {
-			rule := &readinessv1alpha1.NodeReadinessRule{
-				Spec: readinessv1alpha1.NodeReadinessRuleSpec{
-					NodeSelector: metav1.LabelSelector{
-						// Empty selector
-					},
-				},
-			}
-			allErrs := webhook.validateSpec(rule.Spec)
-			Expect(allErrs).To(HaveLen(1))
-			Expect(allErrs[0].Field).To(Equal("spec.nodeSelector"))
-		})
-
 		It("should accept valid nodeSelector", func() {
 			rule := &readinessv1alpha1.NodeReadinessRule{
 				Spec: readinessv1alpha1.NodeReadinessRuleSpec{
@@ -83,24 +70,6 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() {
 		})
 
 		Context("Validate nodeSelector", func() {
-			It("nodeSelector should be set", func() {
-				rule := &readinessv1alpha1.NodeReadinessRule{
-					Spec: readinessv1alpha1.NodeReadinessRuleSpec{
-						Conditions: []readinessv1alpha1.ConditionRequirement{
-							{Type: "Ready", RequiredStatus: corev1.ConditionTrue},
-						},
-						Taint: corev1.Taint{
-							Key:    "readiness.k8s.io/test-key",
-							Effect: corev1.TaintEffectNoSchedule,
-						},
-						EnforcementMode: readinessv1alpha1.EnforcementModeContinuous,
-					},
-				}
-				allErrs := webhook.validateSpec(rule.Spec)
-				Expect(allErrs).To(HaveLen(1))
-				Expect(allErrs[0].Field).To(Equal("spec.nodeSelector"))
-				Expect(allErrs[0].Type).To(Equal(field.ErrorTypeRequired))
-			})
 			It("with invalid nodeSelector", func() {
 				rule := &readinessv1alpha1.NodeReadinessRule{
 					Spec: readinessv1alpha1.NodeReadinessRuleSpec{
@@ -480,10 +449,16 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() {
 		})
 
 		It("should reject invalid create operations", func() {
+			// An empty selector is rejected by CEL on the CRD now, so exercise what the
+			// webhook still owns: a selector that does not parse.
 			rule := &readinessv1alpha1.NodeReadinessRule{
 				ObjectMeta: metav1.ObjectMeta{Name: "invalid-create"},
-				Spec:       readinessv1alpha1.NodeReadinessRuleSpec{
-					// Missing required fields
+				Spec: readinessv1alpha1.NodeReadinessRuleSpec{
+					NodeSelector: metav1.LabelSelector{
+						MatchExpressions: []metav1.LabelSelectorRequirement{
+							{Key: "k", Operator: "NotARealOperator"},
+						},
+					},
 				},
 			}
 
@@ -770,14 +745,19 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() {
 			Expect(allErrs).To(HaveLen(1))
 			Expect(allErrs[0].Field).To(Equal("spec.taint.key"))
 
-			// Test empty nodeSelector
+			// An unparseable nodeSelector. The empty case is enforced by CEL on the CRD
+			// now, so it never reaches the webhook.
 			invalidRule := &readinessv1alpha1.NodeReadinessRule{
 				ObjectMeta: metav1.ObjectMeta{Name: "invalid-comprehensive"},
 				Spec: readinessv1alpha1.NodeReadinessRuleSpec{
 					Conditions: []readinessv1alpha1.ConditionRequirement{
 						{Type: "Ready", RequiredStatus: corev1.ConditionTrue},
 					},
-					NodeSelector: metav1.LabelSelector{}, // Empty selector
+					NodeSelector: metav1.LabelSelector{
+						MatchExpressions: []metav1.LabelSelectorRequirement{
+							{Key: "k", Operator: "NotARealOperator"},
+						},
+					},
 					Taint: corev1.Taint{
 						Key:    "readiness.k8s.io/test-key",
 						Effect: corev1.TaintEffectNoSchedule,
@@ -787,7 +767,7 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() {
 			}
 
 			allErrs = webhook.validateNodeReadinessRule(ctx, invalidRule, false)
-			Expect(allErrs).To(HaveLen(1)) // Empty nodeSelector validation
+			Expect(allErrs).To(HaveLen(1))
 			Expect(allErrs[0].Field).To(Equal("spec.nodeSelector"))
 
 		})