fix AKS out-of-band tag reconciliation

Author: nojnhuh

azure/const.go

@@ -29,6 +29,12 @@ const (
 	// for annotation formatting rules.
 	RGTagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-tags-rg"
 
+	// ManagedClusterTagsLastAppliedAnnotation is the key for the AzureManagedControlPlane
+	// object annotation which tracks the AdditionalTags for managed clusters.
+	// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
+	// for annotation formatting rules.
+	ManagedClusterTagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-tags-managedcluster"
+
 	// CustomDataHashAnnotation is the key for the machine object annotation
 	// which tracks the hash of the custom data.
 	// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/

azure/scope/managedcontrolplane.go

@@ -422,11 +422,12 @@ func (s *ManagedControlPlaneScope) ManagedClusterAnnotations() map[string]string
 }
 
 // ManagedClusterSpec returns the managed cluster spec.
-func (s *ManagedControlPlaneScope) ManagedClusterSpec(ctx context.Context) azure.ResourceSpecGetter {
+func (s *ManagedControlPlaneScope) ManagedClusterSpec() azure.ResourceSpecGetter {
 	managedClusterSpec := managedclusters.ManagedClusterSpec{
 		Name:              s.ControlPlane.Name,
 		ResourceGroup:     s.ControlPlane.Spec.ResourceGroupName,
 		NodeResourceGroup: s.ControlPlane.Spec.NodeResourceGroupName,
+		ClusterName:       s.ClusterName(),
 		Location:          s.ControlPlane.Spec.Location,
 		Tags:              s.ControlPlane.Spec.AdditionalTags,
 		Headers:           maps.FilterByKeyPrefix(s.ManagedClusterAnnotations(), infrav1.CustomHeaderPrefix),
@@ -688,6 +689,11 @@ func (s *ManagedControlPlaneScope) TagsSpecs() []azure.TagsSpec {
 			Tags:       s.AdditionalTags(),
 			Annotation: azure.RGTagsLastAppliedAnnotation,
 		},
+		{
+			Scope:      azure.ManagedClusterID(s.SubscriptionID(), s.ResourceGroup(), s.ManagedClusterSpec().ResourceName()),
+			Tags:       s.AdditionalTags(),
+			Annotation: azure.ManagedClusterTagsLastAppliedAnnotation,
+		},
 	}
 }
 

azure/scope/managedcontrolplane_test.go

@@ -88,7 +88,7 @@ func TestManagedControlPlaneScope_OutboundType(t *testing.T) {
 			c.Input.Client = fakeClient
 			s, err := NewManagedControlPlaneScope(context.TODO(), c.Input)
 			g.Expect(err).To(Succeed())
-			managedCluster := s.ManagedClusterSpec(context.TODO())
+			managedCluster := s.ManagedClusterSpec()
 			result := managedCluster.(*managedclusters.ManagedClusterSpec).OutboundType == nil
 			g.Expect(result).To(Equal(c.Expected))
 		})
@@ -326,7 +326,7 @@ func TestManagedControlPlaneScope_AddonProfiles(t *testing.T) {
 			c.Input.Client = fakeClient
 			s, err := NewManagedControlPlaneScope(context.TODO(), c.Input)
 			g.Expect(err).To(Succeed())
-			managedCluster := s.ManagedClusterSpec(context.TODO())
+			managedCluster := s.ManagedClusterSpec()
 			g.Expect(managedCluster.(*managedclusters.ManagedClusterSpec).AddonProfiles).To(Equal(c.Expected))
 		})
 	}

azure/services/managedclusters/managedclusters.go

@@ -37,7 +37,7 @@ const serviceName = "managedcluster"
 type ManagedClusterScope interface {
 	azure.Authorizer
 	azure.AsyncStatusUpdater
-	ManagedClusterSpec(context.Context) azure.ResourceSpecGetter
+	ManagedClusterSpec() azure.ResourceSpecGetter
 	SetControlPlaneEndpoint(clusterv1.APIEndpoint)
 	MakeEmptyKubeConfigSecret() corev1.Secret
 	GetKubeConfigData() []byte
@@ -74,7 +74,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
 	ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout)
 	defer cancel()
 
-	managedClusterSpec := s.Scope.ManagedClusterSpec(ctx)
+	managedClusterSpec := s.Scope.ManagedClusterSpec()
 	if managedClusterSpec == nil {
 		return nil
 	}
@@ -112,7 +112,7 @@ func (s *Service) Delete(ctx context.Context) error {
 	ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout)
 	defer cancel()
 
-	managedClusterSpec := s.Scope.ManagedClusterSpec(ctx)
+	managedClusterSpec := s.Scope.ManagedClusterSpec()
 	if managedClusterSpec == nil {
 		return nil
 	}

azure/services/managedclusters/managedclusters_test.go

@@ -44,14 +44,14 @@ func TestReconcile(t *testing.T) {
 			name:          "noop if managedcluster spec is nil",
 			expectedError: "",
 			expect: func(m *mock_managedclusters.MockCredentialGetterMockRecorder, s *mock_managedclusters.MockManagedClusterScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.ManagedClusterSpec(gomockinternal.AContext()).Return(nil)
+				s.ManagedClusterSpec().Return(nil)
 			},
 		},
 		{
 			name:          "create managed cluster returns an error",
 			expectedError: "some unexpected error occurred",
 			expect: func(m *mock_managedclusters.MockCredentialGetterMockRecorder, s *mock_managedclusters.MockManagedClusterScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.ManagedClusterSpec(gomockinternal.AContext()).Return(fakeManagedClusterSpec)
+				s.ManagedClusterSpec().Return(fakeManagedClusterSpec)
 				r.CreateOrUpdateResource(gomockinternal.AContext(), fakeManagedClusterSpec, serviceName).Return(nil, errors.New("some unexpected error occurred"))
 				s.UpdatePutStatus(infrav1.ManagedClusterRunningCondition, serviceName, errors.New("some unexpected error occurred"))
 			},
@@ -60,7 +60,7 @@ func TestReconcile(t *testing.T) {
 			name:          "create managed cluster succeeds",
 			expectedError: "",
 			expect: func(m *mock_managedclusters.MockCredentialGetterMockRecorder, s *mock_managedclusters.MockManagedClusterScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.ManagedClusterSpec(gomockinternal.AContext()).Return(fakeManagedClusterSpec)
+				s.ManagedClusterSpec().Return(fakeManagedClusterSpec)
 				r.CreateOrUpdateResource(gomockinternal.AContext(), fakeManagedClusterSpec, serviceName).Return(containerservice.ManagedCluster{
 					ManagedClusterProperties: &containerservice.ManagedClusterProperties{
 						Fqdn:              pointer.String("my-managedcluster-fqdn"),
@@ -80,7 +80,7 @@ func TestReconcile(t *testing.T) {
 			name:          "fail to get managed cluster credentials",
 			expectedError: "failed to get credentials for managed cluster: internal server error",
 			expect: func(m *mock_managedclusters.MockCredentialGetterMockRecorder, s *mock_managedclusters.MockManagedClusterScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.ManagedClusterSpec(gomockinternal.AContext()).Return(fakeManagedClusterSpec)
+				s.ManagedClusterSpec().Return(fakeManagedClusterSpec)
 				r.CreateOrUpdateResource(gomockinternal.AContext(), fakeManagedClusterSpec, serviceName).Return(containerservice.ManagedCluster{
 					ManagedClusterProperties: &containerservice.ManagedClusterProperties{
 						Fqdn:              pointer.String("my-managedcluster-fqdn"),
@@ -136,14 +136,14 @@ func TestDelete(t *testing.T) {
 			name:          "noop if no managed cluster spec is found",
 			expectedError: "",
 			expect: func(s *mock_managedclusters.MockManagedClusterScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.ManagedClusterSpec(gomockinternal.AContext()).Return(nil)
+				s.ManagedClusterSpec().Return(nil)
 			},
 		},
 		{
 			name:          "successfully delete an existing managed cluster",
 			expectedError: "",
 			expect: func(s *mock_managedclusters.MockManagedClusterScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.ManagedClusterSpec(gomockinternal.AContext()).Return(fakeManagedClusterSpec)
+				s.ManagedClusterSpec().Return(fakeManagedClusterSpec)
 				r.DeleteResource(gomockinternal.AContext(), fakeManagedClusterSpec, serviceName).Return(nil)
 				s.UpdateDeleteStatus(infrav1.ManagedClusterRunningCondition, serviceName, nil)
 			},
@@ -152,7 +152,7 @@ func TestDelete(t *testing.T) {
 			name:          "managed cluster deletion fails",
 			expectedError: "internal error",
 			expect: func(s *mock_managedclusters.MockManagedClusterScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
-				s.ManagedClusterSpec(gomockinternal.AContext()).Return(fakeManagedClusterSpec)
+				s.ManagedClusterSpec().Return(fakeManagedClusterSpec)
 				r.DeleteResource(gomockinternal.AContext(), fakeManagedClusterSpec, serviceName).Return(errors.New("internal error"))
 				s.UpdateDeleteStatus(infrav1.ManagedClusterRunningCondition, serviceName, errors.New("internal error"))
 			},

azure/services/managedclusters/mock_managedclusters/managedclusters_mock.go

@@ -44,6 +44,9 @@ type ManagedClusterSpec struct {
 	// NodeResourceGroup is the name of the Azure resource group containing IaaS VMs.
 	NodeResourceGroup string
 
+	// ClusterName is the name of the owning Cluster API Cluster resource.
+	ClusterName string
+
 	// VnetSubnetID is the Azure Resource ID for the subnet which should contain nodes.
 	VnetSubnetID string
 
@@ -270,7 +273,13 @@ func (s *ManagedClusterSpec) Parameters(ctx context.Context, existing interface{
 			Type: containerservice.ResourceIdentityTypeSystemAssigned,
 		},
 		Location: &s.Location,
-		Tags:     *azure.StringMapPtr(s.Tags),
+		Tags: converters.TagsToMap(infrav1.Build(infrav1.BuildParams{
+			Lifecycle:   infrav1.ResourceLifecycleOwned,
+			ClusterName: s.ClusterName,
+			Name:        pointer.String(s.Name),
+			Role:        pointer.String(infrav1.CommonRole),
+			Additional:  s.Tags,
+		})),
 		ManagedClusterProperties: &containerservice.ManagedClusterProperties{
 			NodeResourceGroup: &s.NodeResourceGroup,
 			EnableRBAC:        pointer.Bool(true),
@@ -410,12 +419,6 @@ func (s *ManagedClusterSpec) Parameters(ctx context.Context, existing interface{
 		// AgentPool changes are managed through AMMP.
 		managedCluster.AgentPoolProfiles = existingMC.AgentPoolProfiles
 
-		// Do not trigger an update because of nil/empty discrepancies between the two sets of tags.
-		if len(existingMC.Tags) == 0 && len(managedCluster.Tags) == 0 {
-			existingMC.Tags = nil
-			managedCluster.Tags = nil
-		}
-
 		diff := computeDiffOfNormalizedClusters(managedCluster, existingMC)
 		if diff == "" {
 			log.V(4).Info("no changes found between user-updated spec and existing spec")
@@ -563,11 +566,9 @@ func computeDiffOfNormalizedClusters(managedCluster containerservice.ManagedClus
 
 	clusterNormalized := &containerservice.ManagedCluster{
 		ManagedClusterProperties: propertiesNormalized,
-		Tags:                     managedCluster.Tags,
 	}
 	existingMCClusterNormalized := &containerservice.ManagedCluster{
 		ManagedClusterProperties: existingMCPropertiesNormalized,
-		Tags:                     existingMC.Tags,
 	}
 
 	if managedCluster.Sku != nil {

azure/services/managedclusters/spec.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/utils/pointer"
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
+	"sigs.k8s.io/cluster-api-provider-azure/azure/converters"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/agentpools"
 	gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock"
 )
@@ -60,6 +61,7 @@ func TestParameters(t *testing.T) {
 				Name:              "test-managedcluster",
 				ResourceGroup:     "test-rg",
 				NodeResourceGroup: "test-node-rg",
+				ClusterName:       "test-cluster",
 				Location:          "test-location",
 				Tags: map[string]string{
 					"test-tag": "test-value",
@@ -139,13 +141,17 @@ func TestParameters(t *testing.T) {
 			name:     "delete all tags",
 			existing: getExistingCluster(),
 			spec: &ManagedClusterSpec{
-				Tags: nil,
+				Name:            "test-managedcluster",
+				ResourceGroup:   "test-rg",
+				Location:        "test-location",
+				Tags:            nil,
+				Version:         "v1.22.0",
+				LoadBalancerSKU: "Standard",
 			},
 			expect: func(g *WithT, result interface{}) {
-				g.Expect(result).To(BeAssignableToTypeOf(containerservice.ManagedCluster{}))
-				tags := result.(containerservice.ManagedCluster).Tags
-				g.Expect(tags).NotTo(BeNil())
-				g.Expect(tags).To(BeEmpty())
+				// Additional tags are handled by azure/services/tags, so a diff
+				// here shouldn't trigger an update on the managed cluster resource.
+				g.Expect(result).To(BeNil())
 			},
 		},
 	}
@@ -227,8 +233,14 @@ func getSampleManagedCluster() containerservice.ManagedCluster {
 			Type: containerservice.ResourceIdentityTypeSystemAssigned,
 		},
 		Location: pointer.String("test-location"),
-		Tags: map[string]*string{
-			"test-tag": pointer.String("test-value"),
-		},
+		Tags: converters.TagsToMap(infrav1.Build(infrav1.BuildParams{
+			Lifecycle:   infrav1.ResourceLifecycleOwned,
+			ClusterName: "test-cluster",
+			Name:        pointer.String("test-managedcluster"),
+			Role:        pointer.String(infrav1.CommonRole),
+			Additional: infrav1.Tags{
+				"test-tag": "test-value",
+			},
+		})),
 	}
 }

azure/services/managedclusters/spec_test.go

@@ -57,6 +57,14 @@ func (s *Service) Name() string {
 	return serviceName
 }
 
+// Some resource types are always assumed to be managed by CAPZ whether or not
+// they have the canonical "owned" tag applied to most resources. The annotation
+// key for those types should be listed here so their tags are always
+// interpreted as managed.
+var alwaysManagedAnnotations = map[string]struct{}{
+	azure.ManagedClusterTagsLastAppliedAnnotation: {},
+}
+
 // Reconcile ensures tags are correct.
 func (s *Service) Reconcile(ctx context.Context) error {
 	ctx, log, done := tele.StartSpanWithLogger(ctx, "tags.Service.Reconcile")
@@ -72,7 +80,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
 			tags = existingTags.Properties.Tags
 		}
 
-		if !s.isResourceManaged(tags) {
+		if _, alwaysManaged := alwaysManagedAnnotations[tagsSpec.Annotation]; !alwaysManaged && !s.isResourceManaged(tags) {
 			log.V(4).Info("Skipping tags reconcile for not managed resource")
 			continue
 		}

azure/services/tags/tags.go

@@ -114,6 +114,38 @@ func TestReconcileTags(t *testing.T) {
 				m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{}, nil)
 			},
 		},
+		{
+			name:          "create tags for managed resource without \"owned\" tag",
+			expectedError: "",
+			expect: func(s *mock_tags.MockTagScopeMockRecorder, m *mock_tags.MockclientMockRecorder) {
+				annotation := azure.ManagedClusterTagsLastAppliedAnnotation
+				gomock.InOrder(
+					s.ClusterName().AnyTimes().Return("test-cluster"),
+					s.TagsSpecs().Return([]azure.TagsSpec{
+						{
+							Scope: "/sub/123/fake/scope",
+							Tags: map[string]string{
+								"foo":   "bar",
+								"thing": "stuff",
+							},
+							Annotation: annotation,
+						},
+					}),
+					m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{}, nil),
+					s.AnnotationJSON(annotation),
+					m.UpdateAtScope(gomockinternal.AContext(), "/sub/123/fake/scope", resources.TagsPatchResource{
+						Operation: "Merge",
+						Properties: &resources.Tags{
+							Tags: map[string]*string{
+								"foo":   pointer.String("bar"),
+								"thing": pointer.String("stuff"),
+							},
+						},
+					}),
+					s.UpdateAnnotationJSON(annotation, map[string]interface{}{"foo": "bar", "thing": "stuff"}),
+				)
+			},
+		},
 		{
 			name:          "delete removed tags",
 			expectedError: "",