Send empty type to AKS API when deleting NodeLabels, NodeTaints

- CAPZ will send out the below return types to AKS API so that the AKS API clears-out `NodeLabels` and `NodeTaints` respectively. - an empty Map when the `NodeLabels` are deleted. - an empty pointer array when the `NodeTaints` are deleted.
kubernetes-sigs · Oct 12, 2023 · 05cbad7 · 05cbad7
1 parent 85c23aa
commit 05cbad7
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 19 deletions.
diff --git a/azure/services/agentpools/spec.go b/azure/services/agentpools/spec.go
@@ -388,40 +388,53 @@ 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 {
-	ret := capz
-	if ret == nil {
+func mergeSystemNodeTaints(capzNodeTaints, aksNodeTaints []*string) []*string {
+	if capzNodeTaints == nil && aksNodeTaints == nil {
+		return nil
+	}
+
+	var ret []*string
+	if capzNodeTaints != nil {
+		ret = capzNodeTaints
+	} else {
 		ret = make([]*string, 0)
 	}
+
 	// Look for taints returned from the AKS node pool API that begin with kubernetes.azure.com
-	for _, aksNodeTaint := range aks {
+	for _, aksNodeTaint := range aksNodeTaints {
 		if azureutil.IsAzureSystemNodeLabelKey(*aksNodeTaint) {
 			ret = append(ret, aksNodeTaint)
 		}
 	}
-	// Preserve nil-ness of capz
-	if capz == nil && len(ret) == 0 {
-		ret = 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
@@ -156,6 +156,12 @@ func sdkWithNodeTaints(nodeTaints []*string) func(*armcontainerservice.AgentPool
 	}
 }
 
+func sdkWithNodeLabels(nodeLabels map[string]*string) func(*armcontainerservice.AgentPool) {
+	return func(pool *armcontainerservice.AgentPool) {
+		pool.Properties.NodeLabels = nodeLabels
+	}
+}
+
 func TestParameters(t *testing.T) {
 	testcases := []struct {
 		name          string
@@ -364,6 +370,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 *armcontainerservice.AgentPool) {
+					pool.Properties.NodeLabels = map[string]*string{
+						"fake-label":                            ptr.To("fake-value"),
+						"kubernetes.azure.com/scalesetpriority": ptr.To("spot"),
+					}
+				},
+				sdkWithProvisioningState("Succeeded"),
+			),
+			expected: sdkFakeAgentPool(sdkWithNodeLabels(map[string]*string{
+				"kubernetes.azure.com/scalesetpriority": ptr.To("spot"),
+			})),
+			expectedError: nil,
+		},
 		{
 			name: "difference in non-system node taints with empty taints should trigger update",
 			spec: fakeAgentPool(
@@ -377,7 +404,7 @@ func TestParameters(t *testing.T) {
 				},
 				sdkWithProvisioningState("Succeeded"),
 			),
-			expected:      sdkFakeAgentPool(sdkWithNodeTaints(nil)),
+			expected:      sdkFakeAgentPool(sdkWithNodeTaints(make([]*string, 0))),
 			expectedError: nil,
 		},
 		{
@@ -396,6 +423,25 @@ 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 *armcontainerservice.AgentPool) {
+					pool.Properties.NodeTaints = []*string{
+						ptr.To(azureutil.AzureSystemNodeLabelPrefix + "-fake-taint"),
+						ptr.To("fake-old-taint"),
+					}
+				},
+				sdkWithProvisioningState("Succeeded"),
+			),
+			expected:      sdkFakeAgentPool(sdkWithNodeTaints([]*string{ptr.To(azureutil.AzureSystemNodeLabelPrefix + "-fake-taint")})),
+			expectedError: nil,
+		},
 		{
 			name: "parameters with an existing agent pool and update needed on node taints",
 			spec: fakeAgentPool(),
@@ -437,6 +483,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 *armcontainerservice.AgentPool) { pool.Properties.NodeLabels = nil },
+				sdkWithProvisioningState("Succeeded"),
+			),
+			expected:      nil,
+			expectedError: nil,
+		},
 	}
 	for _, tc := range testcases {
 		tc := tc
@@ -493,7 +551,16 @@ func TestMergeSystemNodeLabels(t *testing.T) {
 				"foo":   ptr.To("bar"),
 				"hello": ptr.To("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":   ptr.To("bar"),
+				"hello": ptr.To("world"),
+			},
+			expected: map[string]*string{},
 		},
 		{
 			name: "delete one label",