fix tag delete following resource create

[nojnhuh]

What type of PR is this?
/kind bug

What this PR does / why we need it: Fixes an issue where deleting any values from an AzureCluster's or AzureManagedCluster's spec.additionalTags before making any other changes to that field never takes effect on the Azure resource group because the last-applied-tags annotation is not applied initially. This change ensures the annotation is set on each reconciliation loop, whether or not the tags need to be updated.

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 #3185

Special notes for your reviewer:

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:

NONE

[willie-yao]

This looks great! Just one comment: Is there a way to add a test case in TestReconcileTags for ensuring the annotations are updated even when the tags aren't changed?

[nawazkh]

Will this change lead to a new reconciliation for every tag from the for loop up above (as the resource version gets updated on annotation update) ?
If yes, could such a change create throttling situation?

[nojnhuh]

This UpdateAnnotationJSON method doesn't make any API calls, it only sets the annotation on the corresponding CAPZ resource like this:

cluster-api-provider-azure/azure/scope/cluster.go

Lines 980 to 999 in 7c92038

	// UpdateAnnotationJSON updates the `annotation` with
	// `content`. `content` in this case should be a `map[string]interface{}`
	// suitable for turning into JSON. This `content` map will be marshalled into a
	// JSON string before being set as the given `annotation`.
	func (s *ClusterScope) UpdateAnnotationJSON(annotation string, content map[string]interface{}) error {
	b, err := json.Marshal(content)
	if err != nil {
	return err
	}
	s.SetAnnotation(annotation, string(b))
	return nil
	}
	// SetAnnotation sets a key value annotation on the AzureCluster.
	func (s *ClusterScope) SetAnnotation(key, value string) {
	if s.AzureCluster.Annotations == nil {
	s.AzureCluster.Annotations = map[string]string{}
	}
	s.AzureCluster.Annotations[key] = value
	}

Then at the very end of the AzureCluster-level Reconcile, that resource is committed to the API with all of the changes made in that reconciliation, so different iterations of this for loop would be coalesced into a single Kubernetes API update. I think this change in particular will only introduce at most one additional reconciliation during the lifetime of the resource which adds the annotation the first time.

[nawazkh]

Thank you for pinning cluster-api-provider-azure/azure/scope/cluster.go lines above, helps to put things in perspective.
Following the code flow from the perspective of generating additional reconciliations, I do not see any Patch/Update/return reconcile:true calls on updating annotations or in function s.Scope.UpdateAnnotationJSON().

So looks good to me!

[nojnhuh]

This looks great! Just one comment: Is there a way to add a test case in TestReconcileTags for ensuring the annotations are updated even when the tags aren't changed?

@willie-yao Yes, this does include an update to the "tags unchanged" test case.

[willie-yao]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: a2e6819f015e1d014a6f43f0ddfd45900cbb7598

[CecileRobertMichon]

instead of updating it every time, would it make sense (and be possible) to only set it the first time we go through here and the annotation doesn't exist? otherwise we're updating the annotation every time even if nothing changed

[nojnhuh]

When nothing changes, doesn't this basically turn into a no-op? A patch is still unconditionally applied at the end of the reconciliation loop whether or not anything in the AzureCluster, etc. changes, so I don't think this would generate any additional ongoing API overhead. That being said, it would be simple to wrap this in something like a if tagsSpec.Annotation != newAnnotation block.

[CecileRobertMichon]

fair

[nawazkh]

/lgtm

[CecileRobertMichon]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 [CecileRobertMichon]

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