fix deleting all tags on AKS resources

[nojnhuh]

What type of PR is this?
/kind bug

What this PR does / why we need it: When spec.additionalTags are defined on AzureManagedControlPlane or AzureManagedMachinePool resources, setting them to null or empty does not delete all tags because the resulting PUT request specifies null tags which seems to be interpreted by AKS as "do not change the tags." This change ensures that if the tags are specified in the CAPZ resources as null, then a non-nil, empty set of tags are sent to the AKS API to express "please delete all of the existing tags."

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer: I came across this issue while implementing an e2e test covering this scenario (currently drafted in #2917). Those tests verify this new behavior for me locally. Additional context in #2745 (comment) and #2802.

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

Fixed a bug preventing `spec.additionalTags` from being deleted entirely on AzureManagedControlPlane

Note: spec.additionalTags is not yet implemented for AzureManagedMachinePools in any tagged release

[jackfrancis]

would this be better?

existingMC.Tags, managedCluster.Tags = nil, nil

setting the "existing" to the "local" seems to suggest we're preferring the "local" value, when in fact what we're really trying to do is to ensure that they both have the same value so that a diff isn't computed between them

is that right?

[nojnhuh]

I suppose what I was trying to enforce here is that the resulting managedCluster.Tags always resolves to a non-nil, empty map when the spec.additionalTags are nil or empty. But I see now that it looks fine for managedCluster.Tags to be nil when existingMC.Tags is also empty since our desired state already matches the actual state in that case. Overall I think your suggestion is a bit more clear so I'll test that to make sure it works and make that change unless I run into any issues.

[nojnhuh]

/retest

[nawazkh]

Great find!
/lgtm

[jackfrancis]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jackfrancis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jackfrancis]

/cherry-pick release-1.5

[k8s-infra-cherrypick-robot]

@jackfrancis: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jackfrancis]

/cherry-pick release-1.6

[k8s-infra-cherrypick-robot]

@jackfrancis: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-infra-cherrypick-robot]

@jackfrancis: #2916 failed to apply on top of branch "release-1.5":

Applying: fix deleting all tags on AKS resources
Using index info to reconstruct a base tree...
M	azure/services/agentpools/spec.go
M	azure/services/agentpools/spec_test.go
M	azure/services/managedclusters/spec_test.go
Falling back to patching base and 3-way merge...
Auto-merging azure/services/managedclusters/spec_test.go
Auto-merging azure/services/agentpools/spec_test.go
CONFLICT (content): Merge conflict in azure/services/agentpools/spec_test.go
Auto-merging azure/services/agentpools/spec.go
CONFLICT (content): Merge conflict in azure/services/agentpools/spec.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix deleting all tags on AKS resources
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-infra-cherrypick-robot]

@jackfrancis: #2916 failed to apply on top of branch "release-1.6":

Applying: fix deleting all tags on AKS resources
Using index info to reconstruct a base tree...
M	azure/services/agentpools/spec.go
M	azure/services/managedclusters/spec_test.go
Falling back to patching base and 3-way merge...
Auto-merging azure/services/managedclusters/spec_test.go
Auto-merging azure/services/agentpools/spec.go
CONFLICT (content): Merge conflict in azure/services/agentpools/spec.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix deleting all tags on AKS resources
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jackfrancis]

nevermind the cherry-picks, the AdditionalTags feature was only introduced in the current release cycle, it doesn't exist as a feature in 1.6 or below.

[nojnhuh]

nevermind the cherry-picks, the AdditionalTags feature was only introduced in the current release cycle, it doesn't exist as a feature in 1.6 or below.

The field is new for AzureManagedMachinePools, but this issue does affect AzureManagedControlPlane's spec.additionalTags on 1.6 and 1.5, so I'll work on manual cherry-picks for those.