From 18d408928c5c238ae5efc5c6f7e0077104dc2c54 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. - update mergeSystemNodeTaints return type from []*string to *[]string - Customize tests for release-1.10 --- azure/services/agentpools/spec.go | 51 ++++++++++------- azure/services/agentpools/spec_test.go | 78 +++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 23 deletions(-) diff --git a/azure/services/agentpools/spec.go b/azure/services/agentpools/spec.go index 1aa85b7ce21..5fc3f007883 100644 --- 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 index a4271ecc892..c189a68c9f6 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(*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",