Review fixes

Author: chrischdi

internal/controllers/machine/machine_controller_noderef.go

@@ -398,7 +398,7 @@ func propagateMachineTaintsToNode(node *corev1.Node, machineTaints []clusterv1.M
 		}
 
 		// Ensure the taint is set on the node and has the correct value.
-		if changedTaints := taints.EnsureNodeTaint(node, convertMachineTaintToCoreV1Taint(taint)); changedTaints {
+		if changedTaints := ensureNodeTaintWithValue(node, convertMachineTaintToCoreV1Taint(taint)); changedTaints {
 			changed = true
 		}
 	}
@@ -408,6 +408,10 @@ func propagateMachineTaintsToNode(node *corev1.Node, machineTaints []clusterv1.M
 
 	// Remove all identified taints from the node.
 	for taintToDelete := range taintsToDelete {
+		if taintToDelete == "" {
+			continue
+		}
+
 		splitted := strings.Split(taintToDelete, ":")
 		if len(splitted) != 2 {
 			return changed, fmt.Errorf("invalid taint format: %q", taintToDelete)
@@ -430,6 +434,29 @@ func propagateMachineTaintsToNode(node *corev1.Node, machineTaints []clusterv1.M
 	return changed, nil
 }
 
+// ensureNodeTaintWithValue makes sure the node has the Taint with the expected value.
+// It returns true if the taints are modified, false otherwise.
+func ensureNodeTaintWithValue(node *corev1.Node, taint corev1.Taint) bool {
+	for i, currentTaint := range node.Spec.Taints {
+		if !taint.MatchTaint(&currentTaint) {
+			continue
+		}
+
+		// Modify the taint if the value is different.
+		if currentTaint.Value != taint.Value {
+			node.Spec.Taints[i] = taint
+			return true
+		}
+
+		// The taint is already set and has the correct value.
+		return false
+	}
+
+	// Add the taint if not present.
+	node.Spec.Taints = append(node.Spec.Taints, taint)
+	return true
+}
+
 // marshalMachineTaintsAnnotation marshals the tracking annotation value.
 func marshalMachineTaintsAnnotation(ownedTaints sets.Set[string]) string {
 	taints := ownedTaints.UnsortedList()

internal/controllers/machine/machine_controller_noderef_test.go

@@ -1589,9 +1589,18 @@ func Test_propagateMachineTaintsToNode(t *testing.T) {
 		Effect:      corev1.TaintEffectNoSchedule,
 		Propagation: clusterv1.MachineTaintPropagationAlways,
 	}
+
 	transitionOnInitialization := transitionAlways
 	transitionOnInitialization.Propagation = clusterv1.MachineTaintPropagationOnInitialization
 
+	transitionExistingAlwaysNewValue := existingAlwaysTaint
+	transitionExistingAlwaysNewValue.Value = "transition-value-new"
+
+	transitionExistingAlwaysNewEffect := existingAlwaysTaint
+	transitionExistingAlwaysNewEffect.Effect = corev1.TaintEffectNoSchedule
+
+	externalNodeTaint := corev1.Taint{Key: "external-taint", Value: "external-value", Effect: corev1.TaintEffectNoExecute}
+
 	tests := []struct {
 		name               string
 		node               *corev1.Node
@@ -1642,6 +1651,15 @@ func Test_propagateMachineTaintsToNode(t *testing.T) {
 			expectedAnnotation: "added-always:NoSchedule",
 			expectChanged:      true,
 		},
+		{
+			name: "Add missing but tracked Always taint, tracking annotation, no other taints",
+			node: builder.Node("").
+				WithAnnotations(map[string]string{clusterv1.TaintsFromMachineAnnotation: "existing-always:NoExecute"}).Build(),
+			machineTaints:      []clusterv1.MachineTaint{existingAlwaysTaint},
+			expectedTaints:     []corev1.Taint{convertMachineTaintToCoreV1Taint(existingAlwaysTaint)},
+			expectedAnnotation: "existing-always:NoExecute",
+			expectChanged:      true,
+		},
 		{
 			name: "Delete Always taint, tracking annotation, no other taints",
 			node: builder.Node("").
@@ -1701,6 +1719,54 @@ func Test_propagateMachineTaintsToNode(t *testing.T) {
 			expectedAnnotation: "transition-taint:NoSchedule",
 			expectChanged:      true,
 		},
+		{
+			name: "Transition Always taint to have a new value should change the value also on the node",
+			node: builder.Node("").
+				WithAnnotations(map[string]string{clusterv1.TaintsFromMachineAnnotation: "existing-always:NoExecute"}).
+				WithTaints(convertMachineTaintToCoreV1Taint(existingAlwaysTaint)).Build(),
+			machineTaints:      []clusterv1.MachineTaint{transitionExistingAlwaysNewValue},
+			expectedTaints:     []corev1.Taint{convertMachineTaintToCoreV1Taint(transitionExistingAlwaysNewValue)},
+			expectedAnnotation: "existing-always:NoExecute",
+			expectChanged:      true,
+		},
+		{
+			name: "Transition Always taint to have a new effect should change the effect also on the node",
+			node: builder.Node("").
+				WithAnnotations(map[string]string{clusterv1.TaintsFromMachineAnnotation: "existing-always:NoExecute"}).
+				WithTaints(convertMachineTaintToCoreV1Taint(existingAlwaysTaint)).Build(),
+			machineTaints:      []clusterv1.MachineTaint{transitionExistingAlwaysNewEffect},
+			expectedTaints:     []corev1.Taint{convertMachineTaintToCoreV1Taint(transitionExistingAlwaysNewEffect)},
+			expectedAnnotation: "existing-always:NoSchedule",
+			expectChanged:      true,
+		},
+		{
+			name: "Add missing taints, no tracking annotation, preserve other taints",
+			node: builder.Node("").
+				WithTaints(externalNodeTaint).Build(),
+			machineTaints:      []clusterv1.MachineTaint{alwaysTaint, onInitializationTaint},
+			expectedTaints:     []corev1.Taint{externalNodeTaint, convertMachineTaintToCoreV1Taint(alwaysTaint), convertMachineTaintToCoreV1Taint(onInitializationTaint)},
+			expectedAnnotation: "added-always:NoSchedule",
+			expectChanged:      true,
+		},
+		{
+			name: "Adopt existing taint, no tracking annotation",
+			node: builder.Node("").
+				WithTaints(convertMachineTaintToCoreV1Taint(existingAlwaysTaint)).Build(),
+			machineTaints:      []clusterv1.MachineTaint{existingAlwaysTaint},
+			expectedTaints:     []corev1.Taint{convertMachineTaintToCoreV1Taint(existingAlwaysTaint)},
+			expectedAnnotation: "existing-always:NoExecute",
+			expectChanged:      true,
+		},
+		{
+			name: "Recover from broken tracking annotation",
+			node: builder.Node("").
+				WithAnnotations(map[string]string{clusterv1.TaintsFromMachineAnnotation: "existing-always:NoExecute,"}).
+				WithTaints(convertMachineTaintToCoreV1Taint(existingAlwaysTaint)).Build(),
+			machineTaints:      []clusterv1.MachineTaint{existingAlwaysTaint},
+			expectedTaints:     []corev1.Taint{convertMachineTaintToCoreV1Taint(existingAlwaysTaint)},
+			expectedAnnotation: "existing-always:NoExecute",
+			expectChanged:      true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {