Make NodeReadinessRule spec fields immutable to prevent security issues (#164)

Author: Priyankasaggu11929

api/v1alpha1/nodereadinessrule_types.go

@@ -56,6 +56,7 @@ type NodeReadinessRuleSpec struct {
 	// +listMapKey=type
 	// +kubebuilder:validation:MinItems=1
 	// +kubebuilder:validation:MaxItems=32
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="conditions is immutable"
 	Conditions []ConditionRequirement `json:"conditions"` //nolint:kubeapilinter
 
 	// enforcementMode specifies how the controller maintains the desired state.
@@ -64,6 +65,7 @@ type NodeReadinessRuleSpec struct {
 	// "continuous" ensures the state is monitored and corrected throughout the resource lifecycle.
 	//
 	// +required
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="enforcementMode is immutable"
 	EnforcementMode EnforcementMode `json:"enforcementMode,omitempty"`
 
 	// taint defines the specific Taint (Key, Value, and Effect) to be managed
@@ -86,11 +88,15 @@ type NodeReadinessRuleSpec struct {
 	// +kubebuilder:validation:XValidation:rule="self.key.split('/')[1].matches('^[A-Za-z0-9]([-A-Za-z0-9_.]*[A-Za-z0-9])?$')",message="taint key name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character"
 	// +kubebuilder:validation:XValidation:rule="!has(self.value) || self.value.size() <= 63",message="taint value length must be at most 63 characters"
 	// +kubebuilder:validation:XValidation:rule="self.effect in ['NoSchedule', 'PreferNoSchedule', 'NoExecute']",message="taint effect must be one of 'NoSchedule', 'PreferNoSchedule', 'NoExecute'"
+	// +kubebuilder:validation:XValidation:rule="!has(oldSelf.key) || self.key == oldSelf.key",message="taint key is immutable"
+	// +kubebuilder:validation:XValidation:rule="!has(oldSelf.effect) || self.effect == oldSelf.effect",message="taint effect is immutable"
+	// +kubebuilder:validation:XValidation:rule="!has(oldSelf.value) || self.value == oldSelf.value",message="taint value is immutable"
 	Taint corev1.Taint `json:"taint,omitempty,omitzero"`
 
 	// nodeSelector limits the scope of this rule to a specific subset of Nodes.
 	//
 	// +required
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="nodeSelector is immutable"
 	NodeSelector metav1.LabelSelector `json:"nodeSelector,omitempty,omitzero"`
 
 	// dryRun when set to true, The controller will evaluate Node conditions and log intended taint modifications

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

@@ -91,6 +91,9 @@ spec:
                 x-kubernetes-list-map-keys:
                 - type
                 x-kubernetes-list-type: map
+                x-kubernetes-validations:
+                - message: conditions is immutable
+                  rule: self == oldSelf
               dryRun:
                 description: |-
                   dryRun when set to true, The controller will evaluate Node conditions and log intended taint modifications
@@ -106,6 +109,9 @@ spec:
                 - bootstrap-only
                 - continuous
                 type: string
+                x-kubernetes-validations:
+                - message: enforcementMode is immutable
+                  rule: self == oldSelf
               nodeSelector:
                 description: nodeSelector limits the scope of this rule to a specific
                   subset of Nodes.
@@ -153,6 +159,9 @@ spec:
                     type: object
                 type: object
                 x-kubernetes-map-type: atomic
+                x-kubernetes-validations:
+                - message: nodeSelector is immutable
+                  rule: self == oldSelf
               taint:
                 description: |-
                   taint defines the specific Taint (Key, Value, and Effect) to be managed
@@ -207,6 +216,12 @@ spec:
                 - message: taint effect must be one of 'NoSchedule', 'PreferNoSchedule',
                     'NoExecute'
                   rule: self.effect in ['NoSchedule', 'PreferNoSchedule', 'NoExecute']
+                - message: taint key is immutable
+                  rule: '!has(oldSelf.key) || self.key == oldSelf.key'
+                - message: taint effect is immutable
+                  rule: '!has(oldSelf.effect) || self.effect == oldSelf.effect'
+                - message: taint value is immutable
+                  rule: '!has(oldSelf.value) || self.value == oldSelf.value'
             required:
             - conditions
             - enforcementMode

internal/controller/nodereadinessrule_controller_test.go

@@ -445,7 +445,6 @@ var _ = Describe("NodeReadinessRule Controller", func() {
 				Key:    "readiness.k8s.io/missing-key",
 				Effect: corev1.TaintEffectNoSchedule,
 			}
-
 			hasTaint = readinessController.hasTaintBySpec(node, nonExistentTaint)
 			Expect(hasTaint).To(BeFalse())
 		})
@@ -958,7 +957,7 @@ var _ = Describe("NodeReadinessRule Controller", func() {
 		})
 	})
 
-	Context("when a rule's nodeSelector changes", func() {
+	Context("when a rule's nodeSelector is modified", func() {
 		var rule *nodereadinessiov1alpha1.NodeReadinessRule
 		var prodNode, devNode *corev1.Node
 
@@ -1016,78 +1015,93 @@ var _ = Describe("NodeReadinessRule Controller", func() {
 			_ = k8sClient.Delete(ctx, rule)
 		})
 
-		It("should remove taints from nodes that no longer match the selector", func() {
-			// Initial reconcile - prod node should be managed, dev node should not
-			_, err := ruleReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "selector-change-rule"}})
-			Expect(err).NotTo(HaveOccurred())
-
-			// Verify prod node still has taint (condition not met)
-			Eventually(func() bool {
-				updatedNode := &corev1.Node{}
-				if err := k8sClient.Get(ctx, types.NamespacedName{Name: "prod-node"}, updatedNode); err != nil {
-					return false
-				}
-				for _, taint := range updatedNode.Spec.Taints {
-					if taint.Key == selectorChangeTaintKey {
-						return true
-					}
-				}
-				return false
-			}, time.Second*5).Should(BeTrue(), "Prod node should have taint")
-
-			// Verify dev node does not have taint (not selected)
-			Consistently(func() bool {
-				updatedNode := &corev1.Node{}
-				if err := k8sClient.Get(ctx, types.NamespacedName{Name: "dev-node"}, updatedNode); err != nil {
-					return false
-				}
-				for _, taint := range updatedNode.Spec.Taints {
-					if taint.Key == selectorChangeTaintKey {
-						return false // Taint found (unexpected)
-					}
-				}
-				return true // No taint (expected)
-			}, time.Second*2).Should(BeTrue(), "Dev node should not have taint")
-
-			// Update rule to target dev nodes instead of prod nodes
+		It("should reject attempts to change the nodeSelector (immutable)", func() {
 			updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{}
 			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "selector-change-rule"}, updatedRule)).To(Succeed())
 			updatedRule.Spec.NodeSelector = metav1.LabelSelector{
 				MatchLabels: map[string]string{"env": "dev"},
 			}
-			Expect(k8sClient.Update(ctx, updatedRule)).To(Succeed())
+			err := k8sClient.Update(ctx, updatedRule)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("nodeSelector is immutable"))
+		})
+	})
 
-			// Trigger reconciliation
-			_, err = ruleReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "selector-change-rule"}})
-			Expect(err).NotTo(HaveOccurred())
+	Context("when attempting to modify immutable fields", func() {
+		var rule *nodereadinessiov1alpha1.NodeReadinessRule
 
-			// Verify taint is removed from prod node (no longer selected)
-			Eventually(func() bool {
-				updatedNode := &corev1.Node{}
-				if err := k8sClient.Get(ctx, types.NamespacedName{Name: "prod-node"}, updatedNode); err != nil {
-					return false
-				}
-				for _, taint := range updatedNode.Spec.Taints {
-					if taint.Key == selectorChangeTaintKey {
-						return false // Taint still exists
-					}
-				}
-				return true // Taint removed
-			}, time.Second*10).Should(BeTrue(), "Prod node taint should be removed after selector change")
+		BeforeEach(func() {
+			rule = &nodereadinessiov1alpha1.NodeReadinessRule{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "immutability-test-rule",
+				},
+				Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{
+					Conditions: []nodereadinessiov1alpha1.ConditionRequirement{
+						{Type: "Ready", RequiredStatus: corev1.ConditionTrue},
+					},
+					Taint: corev1.Taint{
+						Key:    "readiness.k8s.io/immutable",
+						Effect: corev1.TaintEffectNoSchedule,
+						Value:  "test-value",
+					},
+					EnforcementMode: nodereadinessiov1alpha1.EnforcementModeBootstrapOnly,
+					NodeSelector: metav1.LabelSelector{
+						MatchLabels: map[string]string{"test": "immutable"},
+					},
+				},
+			}
+			Expect(k8sClient.Create(ctx, rule)).To(Succeed())
+		})
 
-			// Verify dev node now gets taint (newly selected, condition not met)
-			Eventually(func() bool {
-				updatedNode := &corev1.Node{}
-				if err := k8sClient.Get(ctx, types.NamespacedName{Name: "dev-node"}, updatedNode); err != nil {
-					return false
-				}
-				for _, taint := range updatedNode.Spec.Taints {
-					if taint.Key == selectorChangeTaintKey {
-						return true
-					}
-				}
-				return false
-			}, time.Second*10).Should(BeTrue(), "Dev node should now have taint after selector change")
+		AfterEach(func() {
+			_ = k8sClient.Delete(ctx, rule)
+		})
+
+		It("should reject attempts to change taint.key", func() {
+			updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "immutability-test-rule"}, updatedRule)).To(Succeed())
+			updatedRule.Spec.Taint.Key = "readiness.k8s.io/different-key"
+			err := k8sClient.Update(ctx, updatedRule)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("taint key is immutable"))
+		})
+
+		It("should reject attempts to change taint.effect", func() {
+			updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "immutability-test-rule"}, updatedRule)).To(Succeed())
+			updatedRule.Spec.Taint.Effect = corev1.TaintEffectNoExecute
+			err := k8sClient.Update(ctx, updatedRule)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("taint effect is immutable"))
+		})
+
+		It("should reject attempts to change taint.value", func() {
+			updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "immutability-test-rule"}, updatedRule)).To(Succeed())
+			updatedRule.Spec.Taint.Value = "different-value"
+			err := k8sClient.Update(ctx, updatedRule)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("taint value is immutable"))
+		})
+
+		It("should reject attempts to change conditions", func() {
+			updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "immutability-test-rule"}, updatedRule)).To(Succeed())
+			updatedRule.Spec.Conditions = []nodereadinessiov1alpha1.ConditionRequirement{
+				{Type: "DiskPressure", RequiredStatus: corev1.ConditionFalse},
+			}
+			err := k8sClient.Update(ctx, updatedRule)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("conditions is immutable"))
+		})
+
+		It("should reject attempts to change enforcementMode", func() {
+			updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "immutability-test-rule"}, updatedRule)).To(Succeed())
+			updatedRule.Spec.EnforcementMode = nodereadinessiov1alpha1.EnforcementModeContinuous
+			err := k8sClient.Update(ctx, updatedRule)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("enforcementMode is immutable"))
 		})
 	})
 

internal/webhook/nodereadinessgaterule_webhook.go

@@ -44,8 +44,6 @@ func NewNodeReadinessRuleWebhook(c client.Client) *NodeReadinessRuleWebhook {
 	}
 }
 
-// +kubebuilder:webhook:path=/validate-readiness-node-x-k8s-io-v1alpha1-nodereadinessrule,mutating=false,failurePolicy=fail,sideEffects=None,groups=readiness.node.x-k8s.io,resources=nodereadinessrules,verbs=create;update,versions=v1alpha1,name=vnodereadinessrule.kb.io,admissionReviewVersions=v1
-
 // validateNodeReadinessRule performs validation logic.
 func (w *NodeReadinessRuleWebhook) validateNodeReadinessRule(ctx context.Context, rule *readinessv1alpha1.NodeReadinessRule, isUpdate bool) field.ErrorList {
 	var allErrs field.ErrorList
@@ -59,54 +57,17 @@ func (w *NodeReadinessRuleWebhook) validateNodeReadinessRule(ctx context.Context
 	return allErrs
 }
 
-// validateSpec validates the spec fields.
+// validateSpec validates the spec fields that CRD CEL based XValidation cannot handle.
 func (w *NodeReadinessRuleWebhook) validateSpec(spec readinessv1alpha1.NodeReadinessRuleSpec) field.ErrorList {
 	var allErrs field.ErrorList
-	specField := field.NewPath("spec")
-
-	// Validate conditions
-	if len(spec.Conditions) == 0 {
-		allErrs = append(allErrs, field.Required(specField.Child("conditions"), "at least one condition is required"))
-	}
-
-	for i, condition := range spec.Conditions {
-		condField := specField.Child("conditions").Index(i)
-		if condition.Type == "" {
-			allErrs = append(allErrs, field.Required(condField.Child("type"), "condition type cannot be empty"))
-		}
-		if condition.RequiredStatus == "" {
-			allErrs = append(allErrs, field.Required(condField.Child("requiredStatus"), "required status cannot be empty"))
-		}
-	}
 
-	// Validate nodeSelector
+	// validate that the nodeSelector isn't empty
 	selector, err := metav1.LabelSelectorAsSelector(&spec.NodeSelector)
 	if err != nil {
-		allErrs = append(allErrs, field.Invalid(specField.Child("nodeSelector"), spec.NodeSelector, err.Error()))
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "nodeSelector"), spec.NodeSelector, err.Error()))
 	}
-
-	// Validate that the nodeSelector isn't empty.
 	if selector != nil && selector.Empty() {
-		allErrs = append(allErrs, field.Required(specField.Child("nodeSelector"), "nodeSelector must not be empty"))
-	}
-
-	// Validate taint
-	taintField := specField.Child("taint")
-	if spec.Taint.Key == "" {
-		allErrs = append(allErrs, field.Required(taintField.Child("key"), "taint key cannot be empty"))
-	}
-	if spec.Taint.Effect == "" {
-		allErrs = append(allErrs, field.Required(taintField.Child("effect"), "taint effect cannot be empty"))
-	}
-
-	// Validate enforcement mode
-	if spec.EnforcementMode != readinessv1alpha1.EnforcementModeBootstrapOnly &&
-		spec.EnforcementMode != readinessv1alpha1.EnforcementModeContinuous {
-		allErrs = append(allErrs, field.Invalid(
-			specField.Child("enforcementMode"),
-			spec.EnforcementMode,
-			"must be 'bootstrap-only' or 'continuous'",
-		))
+		allErrs = append(allErrs, field.Required(field.NewPath("spec", "nodeSelector"), "nodeSelector must not be empty"))
 	}
 
 	return allErrs
@@ -169,6 +130,7 @@ func (w *NodeReadinessRuleWebhook) nodSelectorsOverlap(selector1, selector2 meta
 // generateNoExecuteWarnings generates admission warnings for NoExecute taint usage.
 // NoExecute taints cause immediate pod eviction, which can be disruptive when
 // used with continuous enforcement mode.
+// Note: This is only called on CREATE since taint.effect is immutable after creation.
 func (w *NodeReadinessRuleWebhook) generateNoExecuteWarnings(spec readinessv1alpha1.NodeReadinessRuleSpec) admission.Warnings {
 	var warnings admission.Warnings
 
@@ -189,6 +151,7 @@ func (w *NodeReadinessRuleWebhook) generateNoExecuteWarnings(spec readinessv1alp
 	return warnings
 }
 
+// +kubebuilder:webhook:path=/validate-readiness-node-x-k8s-io-v1alpha1-nodereadinessrule,mutating=false,failurePolicy=fail,sideEffects=None,groups=readiness.node.x-k8s.io,resources=nodereadinessrules,verbs=create;update,versions=v1alpha1,name=vnodereadinessrule.kb.io,admissionReviewVersions=v1
 // SetupWithManager sets up the webhook with the manager.
 func (w *NodeReadinessRuleWebhook) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
@@ -216,18 +179,18 @@ func (w *NodeReadinessRuleWebhook) ValidateCreate(ctx context.Context, obj runti
 }
 
 func (w *NodeReadinessRuleWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
-	rule, ok := newObj.(*readinessv1alpha1.NodeReadinessRule)
+	// Update validations are handled at the API level, so we can skip them here to avoid redundant checks.
+
+	newRule, ok := newObj.(*readinessv1alpha1.NodeReadinessRule)
 	if !ok {
 		return nil, fmt.Errorf("expected NodeReadinessRule, got %T", newObj)
 	}
 
-	if allErrs := w.validateNodeReadinessRule(ctx, rule, true); len(allErrs) > 0 {
+	if allErrs := w.validateNodeReadinessRule(ctx, newRule, true); len(allErrs) > 0 {
 		return nil, fmt.Errorf("validation failed: %v", allErrs)
 	}
 
-	// Generate warnings for NoExecute taint usage
-	warnings := w.generateNoExecuteWarnings(rule.Spec)
-	return warnings, nil
+	return nil, nil
 }
 
 func (w *NodeReadinessRuleWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {