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/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/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..885454bc --- /dev/null +++ b/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()) + }) +}) 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")) })