diff --git a/api/v1alpha1/nodereadinessrule_types.go b/api/v1alpha1/nodereadinessrule_types.go index 6694f7fd..ed0cee13 100644 --- a/api/v1alpha1/nodereadinessrule_types.go +++ b/api/v1alpha1/nodereadinessrule_types.go @@ -67,6 +67,8 @@ const ( // // +kubebuilder:validation:XValidation:rule="(!has(oldSelf.conditionPolicy) ? 'allOf' : oldSelf.conditionPolicy) == (!has(self.conditionPolicy) ? 'allOf' : self.conditionPolicy)",message="conditionPolicy is immutable" // +kubebuilder:validation:XValidation:rule="!has(self.conditionPolicy) || self.conditionPolicy != 'anyOf' || self.conditions.all(c, !has(c.defaultStatus))",message="defaultStatus is not supported when conditionPolicy is anyOf" +// +kubebuilder:validation:XValidation:rule="!has(self.enforcementMode) || self.enforcementMode != 'bootstrap-only' || !has(self.conditionPolicy) || self.conditionPolicy != 'anyOf'",message="anyOf conditionPolicy is not supported with bootstrap-only enforcementMode" +// +kubebuilder:validation:XValidation:rule="!has(self.enforcementMode) || self.enforcementMode != 'bootstrap-only' || !has(self.conditions) || self.conditions.all(c, !has(c.defaultStatus))",message="defaultStatus should not be used with bootstrap-only enforcementMode" type NodeReadinessRuleSpec struct { // conditions contains a list of the Node conditions that defines the specific // criteria that must be met for taints to be managed on the target Node. 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 0c7c9fdb..85b617cf 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 @@ -269,6 +269,13 @@ spec: - message: defaultStatus is not supported when conditionPolicy is anyOf rule: '!has(self.conditionPolicy) || self.conditionPolicy != ''anyOf'' || self.conditions.all(c, !has(c.defaultStatus))' + - message: anyOf conditionPolicy is not supported with bootstrap-only + enforcementMode + rule: '!has(self.enforcementMode) || self.enforcementMode != ''bootstrap-only'' + || !has(self.conditionPolicy) || self.conditionPolicy != ''anyOf''' + - message: defaultStatus should not be used with bootstrap-only enforcementMode + rule: '!has(self.enforcementMode) || self.enforcementMode != ''bootstrap-only'' + || !has(self.conditions) || self.conditions.all(c, !has(c.defaultStatus))' status: description: status defines the observed state of NodeReadinessRule minProperties: 1 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 0c7c9fdb..85b617cf 100644 --- a/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml +++ b/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml @@ -269,6 +269,13 @@ spec: - message: defaultStatus is not supported when conditionPolicy is anyOf rule: '!has(self.conditionPolicy) || self.conditionPolicy != ''anyOf'' || self.conditions.all(c, !has(c.defaultStatus))' + - message: anyOf conditionPolicy is not supported with bootstrap-only + enforcementMode + rule: '!has(self.enforcementMode) || self.enforcementMode != ''bootstrap-only'' + || !has(self.conditionPolicy) || self.conditionPolicy != ''anyOf''' + - message: defaultStatus should not be used with bootstrap-only enforcementMode + rule: '!has(self.enforcementMode) || self.enforcementMode != ''bootstrap-only'' + || !has(self.conditions) || self.conditions.all(c, !has(c.defaultStatus))' status: description: status defines the observed state of NodeReadinessRule minProperties: 1 diff --git a/docs/book/src/user-guide/concepts.md b/docs/book/src/user-guide/concepts.md index 75f164b3..4d02965e 100644 --- a/docs/book/src/user-guide/concepts.md +++ b/docs/book/src/user-guide/concepts.md @@ -86,7 +86,7 @@ The choice of `defaultStatus` has critical interaction with the rule's `enforcem > > `defaultStatus` is useful when a condition may never appear on a node in its healthy state, effectively treating its absence as a known-good signal. However, `bootstrap-only` mode exists precisely to *wait* for conditions to be explicitly reported before completing the bootstrap gate. > - > Because these two features serve opposing purposes, using them together can lead to unintended behavior, such as completing the bootstrap phase before a condition is actually verified. To prevent this, the admission webhook explicitly rejects this combination. + > Because these two features serve opposing purposes, using them together can lead to unintended behavior, such as completing the bootstrap phase before a condition is actually verified. To prevent this, the controller explicitly rejects this combination. ## Readiness Condition Reporting diff --git a/internal/controller/nodereadinessrule_controller_test.go b/internal/controller/nodereadinessrule_controller_test.go index 3c5cc11e..273244e6 100644 --- a/internal/controller/nodereadinessrule_controller_test.go +++ b/internal/controller/nodereadinessrule_controller_test.go @@ -2114,6 +2114,43 @@ var _ = Describe("NodeReadinessRule Controller", func() { }) }) + Context("CEL Validation for bootstrap-only constraints", func() { + It("should reject rule creation when enforcementMode is bootstrap-only and defaultStatus is set", func() { + rule := &nodereadinessiov1alpha1.NodeReadinessRule{ + ObjectMeta: metav1.ObjectMeta{Name: "cel-test-reject-bootstrap-defaultstatus"}, + Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{ + EnforcementMode: nodereadinessiov1alpha1.EnforcementModeBootstrapOnly, + Taint: corev1.Taint{Key: "readiness.k8s.io/cel-test", Effect: corev1.TaintEffectNoSchedule}, + NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + Conditions: []nodereadinessiov1alpha1.ConditionRequirement{ + {Type: "TestReady", RequiredStatus: corev1.ConditionTrue, DefaultStatus: corev1.ConditionFalse}, + }, + }, + } + err := k8sClient.Create(ctx, rule) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("defaultStatus should not be used with bootstrap-only enforcementMode")) + }) + + It("should reject rule creation when enforcementMode is bootstrap-only and conditionPolicy is anyOf", func() { + rule := &nodereadinessiov1alpha1.NodeReadinessRule{ + ObjectMeta: metav1.ObjectMeta{Name: "cel-test-reject-bootstrap-anyof"}, + Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{ + EnforcementMode: nodereadinessiov1alpha1.EnforcementModeBootstrapOnly, + Taint: corev1.Taint{Key: "readiness.k8s.io/cel-test", Effect: corev1.TaintEffectNoSchedule}, + ConditionPolicy: nodereadinessiov1alpha1.ConditionPolicyAnyOf, + NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + Conditions: []nodereadinessiov1alpha1.ConditionRequirement{ + {Type: "TestReady", RequiredStatus: corev1.ConditionTrue}, + }, + }, + } + err := k8sClient.Create(ctx, rule) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("anyOf conditionPolicy is not supported with bootstrap-only enforcementMode")) + }) + }) + Context("when existing rule is updated", func() { var rule *nodereadinessiov1alpha1.NodeReadinessRule var node *corev1.Node diff --git a/internal/webhook/nodereadinessgaterule_webhook.go b/internal/webhook/nodereadinessgaterule_webhook.go index 7e99e3f4..387fd3b5 100644 --- a/internal/webhook/nodereadinessgaterule_webhook.go +++ b/internal/webhook/nodereadinessgaterule_webhook.go @@ -47,7 +47,7 @@ func (w *NodeReadinessRuleWebhook) validateNodeReadinessRule(ctx context.Context allErrs := make(field.ErrorList, 0, 4) // Validate basic fields - allErrs = append(allErrs, w.validateSpec(rule.Spec, isUpdate)...) + allErrs = append(allErrs, w.validateSpec(rule.Spec)...) // Check for conflicting rules (same taint key) allErrs = append(allErrs, w.validateTaintConflicts(ctx, rule, isUpdate)...) @@ -58,7 +58,6 @@ func (w *NodeReadinessRuleWebhook) validateNodeReadinessRule(ctx context.Context // validateSpec validates the spec fields that CRD CEL based XValidation cannot handle. func (w *NodeReadinessRuleWebhook) validateSpec( spec readinessv1alpha1.NodeReadinessRuleSpec, - isUpdate bool, ) field.ErrorList { var allErrs field.ErrorList @@ -71,33 +70,6 @@ func (w *NodeReadinessRuleWebhook) validateSpec( allErrs = append(allErrs, field.Required(field.NewPath("spec", "nodeSelector"), "nodeSelector must not be empty")) } - // skip below checks for update because `enforcementMode`, `conditions`, - // and `conditionPolicy` are immutable as constrained by CEL XValidation rules. - if isUpdate { - return allErrs - } - - // validate defaultStatus is not used in bootstrap-only mode - if spec.EnforcementMode == readinessv1alpha1.EnforcementModeBootstrapOnly { - for i, cond := range spec.Conditions { - if cond.DefaultStatus != "" { - allErrs = append(allErrs, field.Forbidden( - field.NewPath("spec", "conditions").Index(i).Child("defaultStatus"), - "defaultStatus should not be used with bootstrap-only enforcementMode", - )) - } - } - } - - // validate anyOf conditionPolicy is not combined with bootstrap-only mode. - if spec.ConditionPolicy == readinessv1alpha1.ConditionPolicyAnyOf && - spec.EnforcementMode == readinessv1alpha1.EnforcementModeBootstrapOnly { - allErrs = append(allErrs, field.Forbidden( - field.NewPath("spec", "conditionPolicy"), - "anyOf conditionPolicy is not supported with bootstrap-only enforcementMode", - )) - } - return allErrs } diff --git a/internal/webhook/nodereadinessgaterule_webhook_test.go b/internal/webhook/nodereadinessgaterule_webhook_test.go index 01f8a7c2..ac92e32f 100644 --- a/internal/webhook/nodereadinessgaterule_webhook_test.go +++ b/internal/webhook/nodereadinessgaterule_webhook_test.go @@ -63,8 +63,7 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() { }, }, } - // using isUpdate true to keep the test isolated to nodeSelector validation - allErrs := webhook.validateSpec(rule.Spec, true) + allErrs := webhook.validateSpec(rule.Spec) Expect(allErrs).To(HaveLen(1)) Expect(allErrs[0].Field).To(Equal("spec.nodeSelector")) }) @@ -79,9 +78,7 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() { }, }, } - - // using isUpdate true to keep the test isolated to nodeSelector validation - allErrs := webhook.validateSpec(rule.Spec, true) + allErrs := webhook.validateSpec(rule.Spec) Expect(allErrs).To(BeEmpty()) }) @@ -99,9 +96,7 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() { EnforcementMode: readinessv1alpha1.EnforcementModeContinuous, }, } - - // using isUpdate true to keep the test isolated to nodeSelector validation - allErrs := webhook.validateSpec(rule.Spec, true) + 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)) @@ -124,111 +119,13 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() { EnforcementMode: readinessv1alpha1.EnforcementModeContinuous, }, } - - // using isUpdate true to keep the test isolated to nodeSelector validation - allErrs := webhook.validateSpec(rule.Spec, true) + 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.ErrorTypeInvalid)) }) }) - Context("defaultStatus enforcement", func() { - var spec readinessv1alpha1.NodeReadinessRuleSpec - - BeforeEach(func() { - spec = readinessv1alpha1.NodeReadinessRuleSpec{ - NodeSelector: metav1.LabelSelector{ - MatchLabels: map[string]string{ - "node-role.kubernetes.io/worker": "", - }, - }, - Conditions: []readinessv1alpha1.ConditionRequirement{ - { - Type: "Ready", - RequiredStatus: corev1.ConditionTrue, - DefaultStatus: corev1.ConditionTrue, // offending field - }, - }, - EnforcementMode: readinessv1alpha1.EnforcementModeBootstrapOnly, - } - }) - - It("should skip defaultStatus check on update (early return)", func() { - allErrs := webhook.validateSpec(spec, true) - Expect(allErrs).To(BeEmpty()) - }) - - It("should skip defaultStatus check for continuous enforcement", func() { - spec.EnforcementMode = readinessv1alpha1.EnforcementModeContinuous - allErrs := webhook.validateSpec(spec, false) - Expect(allErrs).To(BeEmpty()) - }) - - It("should forbid defaultStatus with bootstrap-only enforcement", func() { - allErrs := webhook.validateSpec(spec, false) - Expect(allErrs).To(HaveLen(1)) - Expect(allErrs[0].Field).To(Equal("spec.conditions[0].defaultStatus")) - Expect(allErrs[0].Type).To(Equal(field.ErrorTypeForbidden)) - }) - }) - - Context("conditionPolicy with bootstrap-only", func() { - var spec readinessv1alpha1.NodeReadinessRuleSpec - - BeforeEach(func() { - spec = readinessv1alpha1.NodeReadinessRuleSpec{ - NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - ConditionPolicy: readinessv1alpha1.ConditionPolicyAnyOf, - EnforcementMode: readinessv1alpha1.EnforcementModeBootstrapOnly, - } - }) - - It("should skip conditionPolicy check on update (early return)", func() { - allErrs := webhook.validateSpec(spec, true) - Expect(allErrs).To(BeEmpty()) - }) - - It("should allow anyOf conditionPolicy for continuous enforcement", func() { - spec.EnforcementMode = readinessv1alpha1.EnforcementModeContinuous - allErrs := webhook.validateSpec(spec, false) - Expect(allErrs).To(BeEmpty()) - }) - - It("should forbid anyOf conditionPolicy with bootstrap-only enforcement", func() { - allErrs := webhook.validateSpec(spec, false) - Expect(allErrs).To(HaveLen(1)) - Expect(allErrs[0].Field).To(Equal("spec.conditionPolicy")) - Expect(allErrs[0].Type).To(Equal(field.ErrorTypeForbidden)) - }) - }) - - It("should accumulate errors across nodeSelector, defaultStatus, and conditionPolicy violations", func() { - spec := readinessv1alpha1.NodeReadinessRuleSpec{ - NodeSelector: metav1.LabelSelector{}, // empty → ErrorTypeRequired - ConditionPolicy: readinessv1alpha1.ConditionPolicyAnyOf, // anyOf + bootstrap-only → ErrorTypeForbidden - Conditions: []readinessv1alpha1.ConditionRequirement{ - { - Type: "Ready", - RequiredStatus: corev1.ConditionTrue, - DefaultStatus: corev1.ConditionFalse, - }, - {Type: "NetworkReady", - RequiredStatus: corev1.ConditionTrue, - DefaultStatus: corev1.ConditionTrue, - }, - }, - EnforcementMode: readinessv1alpha1.EnforcementModeBootstrapOnly, - } - - allErrs := webhook.validateSpec(spec, false) - Expect(allErrs).To(HaveLen(4)) - Expect(allErrs[0].Field).To(Equal("spec.nodeSelector")) - Expect(allErrs[1].Field).To(Equal("spec.conditions[0].defaultStatus")) - Expect(allErrs[2].Field).To(Equal("spec.conditions[1].defaultStatus")) - Expect(allErrs[3].Field).To(Equal("spec.conditionPolicy")) - }) - It("should pass validation for valid spec", func() { rule := &readinessv1alpha1.NodeReadinessRule{ Spec: readinessv1alpha1.NodeReadinessRuleSpec{ @@ -250,7 +147,7 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() { }, } - allErrs := webhook.validateSpec(rule.Spec, false) + allErrs := webhook.validateSpec(rule.Spec) Expect(allErrs).To(BeEmpty()) }) }) @@ -893,60 +790,6 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() { Expect(allErrs).To(HaveLen(1)) // Empty nodeSelector validation Expect(allErrs[0].Field).To(Equal("spec.nodeSelector")) - // Test defaultStatus used with bootstrap-only enforcement - bootstrapDefaultStatusRule := &readinessv1alpha1.NodeReadinessRule{ - ObjectMeta: metav1.ObjectMeta{Name: "bootstrap-defaultstatus-comprehensive"}, - Spec: readinessv1alpha1.NodeReadinessRuleSpec{ - Conditions: []readinessv1alpha1.ConditionRequirement{ - { - Type: "Ready", - RequiredStatus: corev1.ConditionTrue, - DefaultStatus: corev1.ConditionFalse, - }, - }, - NodeSelector: metav1.LabelSelector{ - MatchLabels: map[string]string{ - "node-role.kubernetes.io/worker": "", - }, - }, - Taint: corev1.Taint{ - Key: "readiness.k8s.io/test-key", - Effect: corev1.TaintEffectNoSchedule, - }, - EnforcementMode: readinessv1alpha1.EnforcementModeBootstrapOnly, - }, - } - - allErrs = webhook.validateNodeReadinessRule(ctx, bootstrapDefaultStatusRule, false) - Expect(allErrs).To(HaveLen(1)) - Expect(allErrs[0].Field).To(Equal("spec.conditions[0].defaultStatus")) - Expect(allErrs[0].Type).To(Equal(field.ErrorTypeForbidden)) - - // Test anyOf conditionPolicy used with bootstrap-only enforcement - bootstrapAnyOfPolicyRule := &readinessv1alpha1.NodeReadinessRule{ - ObjectMeta: metav1.ObjectMeta{Name: "bootstrap-anyof-comprehensive"}, - Spec: readinessv1alpha1.NodeReadinessRuleSpec{ - Conditions: []readinessv1alpha1.ConditionRequirement{ - {Type: "Ready", RequiredStatus: corev1.ConditionTrue}, - }, - ConditionPolicy: readinessv1alpha1.ConditionPolicyAnyOf, - NodeSelector: metav1.LabelSelector{ - MatchLabels: map[string]string{ - "node-role.kubernetes.io/worker": "", - }, - }, - Taint: corev1.Taint{ - Key: "readiness.k8s.io/test-key-2", - Effect: corev1.TaintEffectNoSchedule, - }, - EnforcementMode: readinessv1alpha1.EnforcementModeBootstrapOnly, - }, - } - - allErrs = webhook.validateNodeReadinessRule(ctx, bootstrapAnyOfPolicyRule, false) - Expect(allErrs).To(HaveLen(1)) - Expect(allErrs[0].Field).To(Equal("spec.conditionPolicy")) - Expect(allErrs[0].Type).To(Equal(field.ErrorTypeForbidden)) }) }) })