Merge pull request #4153 from nawazkh/agentpools_labels_fix_release-1.10

[release-1.10] fix NodeTaints and NodeLabels return type for AzureManagedMachinePools
kubernetes-sigs · Oct 18, 2023 · 928ddf4 · 928ddf4
2 parents 0033404 + 18d4089
commit 928ddf4
Show file tree

Hide file tree

Showing 2 changed files with 106 additions and 23 deletions.
diff --git a/azure/services/agentpools/spec.go b/azure/services/agentpools/spec.go
@@ -370,46 +370,55 @@ func (s *AgentPoolSpec) Parameters(ctx context.Context, existing interface{}) (p
 
 // mergeSystemNodeLabels appends any kubernetes.azure.com-prefixed labels from the AKS label set
 // into the local capz label set.
-func mergeSystemNodeLabels(capz, aks map[string]*string) map[string]*string {
-	ret := capz
-	if ret == nil {
+func mergeSystemNodeLabels(capzLabels, aksLabels map[string]*string) map[string]*string {
+	// if both are nil, return nil for no change
+	if capzLabels == nil && aksLabels == nil {
+		return nil
+	}
+
+	// Either one of the capzLabels or aksLabels is nil.
+	// Prioritize capzLabels if it is not nil.
+	var ret map[string]*string
+	if capzLabels != nil {
+		ret = capzLabels
+	} else {
 		ret = make(map[string]*string)
 	}
+
 	// Look for labels returned from the AKS node pool API that begin with kubernetes.azure.com
-	for aksNodeLabelKey := range aks {
+	for aksNodeLabelKey := range aksLabels {
 		if azureutil.IsAzureSystemNodeLabelKey(aksNodeLabelKey) {
-			ret[aksNodeLabelKey] = aks[aksNodeLabelKey]
+			ret[aksNodeLabelKey] = aksLabels[aksNodeLabelKey]
 		}
 	}
-	// Preserve nil-ness of capz
-	if capz == nil && len(ret) == 0 {
-		ret = nil
-	}
+
+	// if no labels are found, ret will be an empty map to clear out the NodeLabels at AKS API
 	return ret
 }
 
 // mergeSystemNodeTaints appends any kubernetes.azure.com-prefixed taints from the AKS taint set
 // into the local capz taint set.
-func mergeSystemNodeTaints(capz, aks *[]string) *[]string {
-	var ret []string
-	if capz != nil {
-		ret = *capz
+func mergeSystemNodeTaints(capzNodeTaints, aksNodeTaints *[]string) *[]string {
+	if capzNodeTaints == nil && aksNodeTaints == nil {
+		return nil
 	}
-	if ret == nil {
+
+	var ret []string
+	if capzNodeTaints != nil {
+		ret = *capzNodeTaints
+	} else {
 		ret = make([]string, 0)
 	}
-	// Look for labels returned from the AKS node pool API that begin with kubernetes.azure.com
-	if aks != nil {
-		for _, aksNodeTaint := range *aks {
+
+	// Look for taints returned from the AKS node pool API that begin with kubernetes.azure.com
+	if aksNodeTaints != nil {
+		for _, aksNodeTaint := range *aksNodeTaints {
 			if azureutil.IsAzureSystemNodeLabelKey(aksNodeTaint) {
 				ret = append(ret, aksNodeTaint)
 			}
 		}
 	}
 
-	// Preserve nil-ness of capz
-	if capz == nil && len(ret) == 0 {
-		return nil
-	}
+	// if no taints are found, ret will be an empty array to clear out the nodeTaints at AKS API
 	return &ret
 }
diff --git a/azure/services/agentpools/spec_test.go b/azure/services/agentpools/spec_test.go
@@ -133,6 +133,12 @@ func sdkWithNodeTaints(nodeTaints *[]string) func(*containerservice.AgentPool) {
 	}
 }
 
+func sdkWithNodeLabels(nodeLabels map[string]*string) func(*containerservice.AgentPool) {
+	return func(pool *containerservice.AgentPool) {
+		pool.NodeLabels = nodeLabels
+	}
+}
+
 func TestParameters(t *testing.T) {
 	testcases := []struct {
 		name          string
@@ -306,6 +312,27 @@ func TestParameters(t *testing.T) {
 			expected:      nil,
 			expectedError: nil,
 		},
+		{
+			name: "difference in system node labels with empty label at CAPZ and non-nil label at AKS should preserve AKS K8s label",
+			spec: fakeAgentPool(
+				func(pool *AgentPoolSpec) {
+					pool.NodeLabels = nil
+				},
+			),
+			existing: sdkFakeAgentPool(
+				func(pool *containerservice.AgentPool) {
+					pool.NodeLabels = map[string]*string{
+						"fake-label":                            pointer.String("fake-value"),
+						"kubernetes.azure.com/scalesetpriority": pointer.String("spot"),
+					}
+				},
+				sdkWithProvisioningState("Succeeded"),
+			),
+			expected: sdkFakeAgentPool(sdkWithNodeLabels(map[string]*string{
+				"kubernetes.azure.com/scalesetpriority": pointer.String("spot"),
+			})),
+			expectedError: nil,
+		},
 		{
 			name: "difference in non-system node taints with empty taints should trigger update",
 			spec: fakeAgentPool(
@@ -319,7 +346,10 @@ func TestParameters(t *testing.T) {
 				},
 				sdkWithProvisioningState("Succeeded"),
 			),
-			expected:      sdkFakeAgentPool(sdkWithNodeTaints(nil)),
+			expected: sdkFakeAgentPool(sdkWithNodeTaints(
+				func() *[]string {
+					return &[]string{}
+				}())),
 			expectedError: nil,
 		},
 		{
@@ -338,6 +368,29 @@ func TestParameters(t *testing.T) {
 			expected:      nil,
 			expectedError: nil,
 		},
+		{
+			name: "difference in system node taints with empty taints at CAPZ and non-nil taints at AKS should preserve K8s taints",
+			spec: fakeAgentPool(
+				func(pool *AgentPoolSpec) {
+					pool.NodeTaints = nil
+				},
+			),
+			existing: sdkFakeAgentPool(
+				func(pool *containerservice.AgentPool) {
+					dummyTaints := []string{
+						azureutil.AzureSystemNodeLabelPrefix + "-fake-taint",
+						"fake-old-taint",
+					}
+					pool.NodeTaints = &dummyTaints
+				},
+				sdkWithProvisioningState("Succeeded"),
+			),
+			expected: sdkFakeAgentPool(sdkWithNodeTaints(func() *[]string {
+				dummy := []string{azureutil.AzureSystemNodeLabelPrefix + "-fake-taint"}
+				return &dummy
+			}())),
+			expectedError: nil,
+		},
 		{
 			name: "parameters with an existing agent pool and update needed on node taints",
 			spec: fakeAgentPool(),
@@ -377,6 +430,18 @@ func TestParameters(t *testing.T) {
 			expected:      nil,
 			expectedError: nil,
 		},
+		{
+			name: "empty node labels should not trigger an update",
+			spec: fakeAgentPool(
+				func(pool *AgentPoolSpec) { pool.NodeLabels = nil },
+			),
+			existing: sdkFakeAgentPool(
+				func(pool *containerservice.AgentPool) { pool.NodeLabels = nil },
+				sdkWithProvisioningState("Succeeded"),
+			),
+			expected:      nil,
+			expectedError: nil,
+		},
 	}
 	for _, tc := range testcases {
 		tc := tc
@@ -433,7 +498,16 @@ func TestMergeSystemNodeLabels(t *testing.T) {
 				"foo":   pointer.String("bar"),
 				"hello": pointer.String("world"),
 			},
-			expected: nil,
+			expected: map[string]*string{},
+		},
+		{
+			name:       "labels are nil when both the capz and aks labels are nil",
+			capzLabels: nil,
+			aksLabels: map[string]*string{
+				"foo":   pointer.String("bar"),
+				"hello": pointer.String("world"),
+			},
+			expected: map[string]*string{},
 		},
 		{
 			name: "delete one label",