From 589013e285159e59391744ee178436054b0d2097 Mon Sep 17 00:00:00 2001 From: Nawaz Hussain Khazielakha Date: Thu, 12 Oct 2023 11:12:11 -0500 Subject: [PATCH] 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. --- azure/services/agentpools/spec.go | 59 +++++++++++---------- azure/services/agentpools/spec_test.go | 71 +++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 28 deletions(-) diff --git a/azure/services/agentpools/spec.go b/azure/services/agentpools/spec.go index 1aa85b7ce21..17874221c9e 100644 --- a/azure/services/agentpools/spec.go +++ b/azure/services/agentpools/spec.go @@ -370,46 +370,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 { - var ret []string - if capz != nil { - ret = *capz +func mergeSystemNodeTaints(capzNodeTaints, aksNodeTaints []*string) []*string { + if capzNodeTaints == nil && aksNodeTaints == nil { + return nil } - if ret == nil { - ret = make([]string, 0) + + 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 { - if azureutil.IsAzureSystemNodeLabelKey(aksNodeTaint) { - ret = append(ret, aksNodeTaint) - } + + // Look for taints returned from the AKS node pool API that begin with kubernetes.azure.com + 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 - } - return &ret + // 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 index a4271ecc892..e9dc4a8142d 100644 --- 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(*armcontainerservice.AgentPool) { + return func(pool *armcontainerservice.AgentPool) { + pool.Properties.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 *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( @@ -319,7 +346,7 @@ func TestParameters(t *testing.T) { }, sdkWithProvisioningState("Succeeded"), ), - expected: sdkFakeAgentPool(sdkWithNodeTaints(nil)), + expected: sdkFakeAgentPool(sdkWithNodeTaints(make([]*string, 0))), expectedError: nil, }, { @@ -338,6 +365,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(), @@ -377,6 +423,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 @@ -433,7 +491,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": ptr.To("bar"), + "hello": ptr.To("world"), + }, + expected: map[string]*string{}, }, { name: "delete one label",