From e11a0db633b9e77b84ccfa68ae712d6ecfaba321 Mon Sep 17 00:00:00 2001 From: tejassinghbhati Date: Tue, 25 Aug 2026 09:41:07 +0530 Subject: [PATCH 1/2] feat: enforce non-empty nodeSelector with CEL instead of the webhook An empty nodeSelector matches every Node in the cluster, so a rule carrying one applies its taint fleet wide. The only thing stopping that today is validateSpec in the validating webhook, and the webhook is optional and off by default, so the guard is absent on a default install. That gap is what #403 documented from the chart side. Move the constraint onto the CRD as CEL, where it applies on every cluster regardless of whether the webhook is deployed. This continues the migration described in #449, and does not overlap #451, which covers the two bootstrap-only constraints. CEL cannot express whether a selector parses, so the LabelSelectorAsSelector error check stays in the webhook. The empty check is removed rather than left alongside it: CRD validation runs before validating webhooks, so that branch is now unreachable. An absent nodeSelector is still caught by the required marker rather than by CEL, because the field is omitempty/omitzero and serialises away. Both reject the object, they just report it differently, and the tests cover each path. Signed-off-by: tejassinghbhati --- api/v1alpha1/nodereadinessrule_types.go | 4 + ...ness.node.x-k8s.io_nodereadinessrules.yaml | 10 +- internal/controller/nodeselector_cel_test.go | 109 ++++++++++++++++++ .../webhook/nodereadinessgaterule_webhook.go | 11 +- .../nodereadinessgaterule_webhook_test.go | 52 +++------ 5 files changed, 142 insertions(+), 44 deletions(-) create mode 100644 internal/controller/nodeselector_cel_test.go diff --git a/api/v1alpha1/nodereadinessrule_types.go b/api/v1alpha1/nodereadinessrule_types.go index ed0cee13..4523aae4 100644 --- a/api/v1alpha1/nodereadinessrule_types.go +++ b/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. diff --git a/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml b/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml index 85b617cf..9a3e3d8c 100644 --- a/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml +++ b/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 diff --git a/internal/controller/nodeselector_cel_test.go b/internal/controller/nodeselector_cel_test.go new file mode 100644 index 00000000..babbb745 --- /dev/null +++ b/internal/controller/nodeselector_cel_test.go @@ -0,0 +1,109 @@ +/* +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" + + . "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 len(r.Name) >= 12 && r.Name[:12] == "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()) + }) +}) diff --git a/internal/webhook/nodereadinessgaterule_webhook.go b/internal/webhook/nodereadinessgaterule_webhook.go index 387fd3b5..3046c0c8 100644 --- a/internal/webhook/nodereadinessgaterule_webhook.go +++ b/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 } diff --git a/internal/webhook/nodereadinessgaterule_webhook_test.go b/internal/webhook/nodereadinessgaterule_webhook_test.go index ac92e32f..d45dd373 100644 --- a/internal/webhook/nodereadinessgaterule_webhook_test.go +++ b/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")) }) From 40b5e0c215bc71f2e3077368d64c63c7b4b7528b Mon Sep 17 00:00:00 2001 From: tejassinghbhati Date: Tue, 25 Aug 2026 21:16:07 +0530 Subject: [PATCH 2/2] fix: sync the chart CRD and tidy the test cleanup Review catch from @yindia. The CEL rule was regenerated into config/crd/bases but never copied into the chart's crds directory, so verify-chart-drift.sh would have failed and, more to the point, a Helm install would not have shipped the rule at all, which is the thing this PR exists to do. Also takes the suggested strings.HasPrefix over the hand-rolled slice comparison in the test cleanup. Signed-off-by: tejassinghbhati --- .../nodereadinessrules.readiness.node.x-k8s.io.yaml | 10 ++++++++-- internal/controller/nodeselector_cel_test.go | 3 ++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml b/charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml index 85b617cf..9a3e3d8c 100644 --- a/charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml +++ b/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 diff --git a/internal/controller/nodeselector_cel_test.go b/internal/controller/nodeselector_cel_test.go index babbb745..885454bc 100644 --- a/internal/controller/nodeselector_cel_test.go +++ b/internal/controller/nodeselector_cel_test.go @@ -18,6 +18,7 @@ package controller import ( "context" + "strings" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -55,7 +56,7 @@ var _ = Describe("NodeReadinessRule nodeSelector CEL validation", func() { if err := k8sClient.List(celCtx, list); err == nil { for i := range list.Items { r := &list.Items[i] - if len(r.Name) >= 12 && r.Name[:12] == "cel-selector" { + if strings.HasPrefix(r.Name, "cel-selector-") { r.Finalizers = nil _ = k8sClient.Update(celCtx, r) _ = k8sClient.Delete(celCtx, r)