From 05cbad7e994ca95075d20473607d14366ebb5189 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 | 47 +++++++++++------ azure/services/agentpools/spec_test.go | 71 +++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 19 deletions(-) diff --git a/azure/services/agentpools/spec.go b/azure/services/agentpools/spec.go index 9d75a36d38f..b4a3b3c5f25 100644 --- 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 index 48b64d238c7..68d53d016c5 100644 --- 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",