Skip to content

Commit

Permalink
Send empty type to AKS API when deleting NodeLabels, NodeTaints
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
nawazkh committed Oct 18, 2023
1 parent 0033404 commit 18d4089
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 23 deletions.
51 changes: 30 additions & 21 deletions azure/services/agentpools/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
78 changes: 76 additions & 2 deletions azure/services/agentpools/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand All @@ -319,7 +346,10 @@ func TestParameters(t *testing.T) {
},
sdkWithProvisioningState("Succeeded"),
),
expected: sdkFakeAgentPool(sdkWithNodeTaints(nil)),
expected: sdkFakeAgentPool(sdkWithNodeTaints(
func() *[]string {
return &[]string{}
}())),
expectedError: nil,
},
{
Expand All @@ -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(),
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 18d4089

Please sign in to comment.