diff --git a/api/v1beta1/tags.go b/api/v1beta1/tags.go index f9b3e85e8d3..4f9ce94be26 100644 --- a/api/v1beta1/tags.go +++ b/api/v1beta1/tags.go @@ -136,6 +136,12 @@ const ( // See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/ // for annotation formatting rules. VMTagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-tags-vm" + + // RGTagsLastAppliedAnnotation is the key for the Azure Cluster object annotation + // which tracks the AdditionalTags for Resource Group which is part in the Azure Cluster. + // See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/ + // for annotation formatting rules. + RGTagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-tags-rg" ) // SpecVersionHashTagKey is the key for the spec version hash used to enable quick spec difference comparison. diff --git a/azure/defaults.go b/azure/defaults.go index fb3d1eabf81..4a818f590d7 100644 --- a/azure/defaults.go +++ b/azure/defaults.go @@ -187,6 +187,11 @@ func WithIndex(name string, n int) string { return fmt.Sprintf("%s-%d", name, n) } +// ResourceGroupID returns the azure resource ID for a given resource group. +func ResourceGroupID(subscriptionID, resourceGroup string) string { + return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", subscriptionID, resourceGroup) +} + // VMID returns the azure resource ID for a given VM. func VMID(subscriptionID, resourceGroup, vmName string) string { return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/virtualMachines/%s", subscriptionID, resourceGroup, vmName) diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index d13c12ce167..7ea68da55f8 100644 --- a/azure/scope/cluster.go +++ b/azure/scope/cluster.go @@ -18,6 +18,7 @@ package scope import ( "context" + "encoding/json" "fmt" "hash/fnv" "strconv" @@ -796,3 +797,49 @@ func (s *ClusterScope) UpdatePatchStatus(condition clusterv1.ConditionType, serv conditions.MarkFalse(s.AzureCluster, condition, infrav1.FailedReason, clusterv1.ConditionSeverityError, "%s failed to update. err: %s", service, err.Error()) } } + +// AnnotationJSON returns a map[string]interface from a JSON annotation. +func (s *ClusterScope) AnnotationJSON(annotation string) (map[string]interface{}, error) { + out := map[string]interface{}{} + jsonAnnotation := s.AzureCluster.GetAnnotations()[annotation] + if len(jsonAnnotation) == 0 { + return out, nil + } + err := json.Unmarshal([]byte(jsonAnnotation), &out) + if err != nil { + return out, err + } + return out, nil +} + +// 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 +} + +// TagsSpecs returns the tag specs for the AzureCluster. +func (s *ClusterScope) TagsSpecs() []azure.TagsSpec { + return []azure.TagsSpec{ + { + Scope: azure.ResourceGroupID(s.SubscriptionID(), s.ResourceGroup()), + Tags: s.AdditionalTags(), + Annotation: infrav1.RGTagsLastAppliedAnnotation, + }, + } +} diff --git a/azure/scope/managedcontrolplane.go b/azure/scope/managedcontrolplane.go index 44ad99169d9..6ac1515615c 100644 --- a/azure/scope/managedcontrolplane.go +++ b/azure/scope/managedcontrolplane.go @@ -19,6 +19,7 @@ package scope import ( "context" "encoding/base64" + "encoding/json" "fmt" "net" "strings" @@ -622,3 +623,49 @@ func (s *ManagedControlPlaneScope) UpdatePutStatus(condition clusterv1.Condition func (s *ManagedControlPlaneScope) UpdatePatchStatus(condition clusterv1.ConditionType, service string, err error) { // TODO: add condition to AzureManagedControlPlane status } + +// AnnotationJSON returns a map[string]interface from a JSON annotation. +func (s *ManagedControlPlaneScope) AnnotationJSON(annotation string) (map[string]interface{}, error) { + out := map[string]interface{}{} + jsonAnnotation := s.ControlPlane.GetAnnotations()[annotation] + if len(jsonAnnotation) == 0 { + return out, nil + } + err := json.Unmarshal([]byte(jsonAnnotation), &out) + if err != nil { + return out, err + } + return out, nil +} + +// 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 *ManagedControlPlaneScope) 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 ControlPlane. +func (s *ManagedControlPlaneScope) SetAnnotation(key, value string) { + if s.ControlPlane.Annotations == nil { + s.ControlPlane.Annotations = map[string]string{} + } + s.ControlPlane.Annotations[key] = value +} + +// TagsSpecs returns the tag specs for the ManagedControlPlane. +func (s *ManagedControlPlaneScope) TagsSpecs() []azure.TagsSpec { + return []azure.TagsSpec{ + { + Scope: azure.ResourceGroupID(s.SubscriptionID(), s.ResourceGroup()), + Tags: s.AdditionalTags(), + Annotation: infrav1.RGTagsLastAppliedAnnotation, + }, + } +} diff --git a/azure/services/groups/spec.go b/azure/services/groups/spec.go index 3c1311ad7c0..cd4a407600b 100644 --- a/azure/services/groups/spec.go +++ b/azure/services/groups/spec.go @@ -51,16 +51,18 @@ func (s *GroupSpec) OwnerResourceName() string { func (s *GroupSpec) Parameters(existing interface{}) (interface{}, error) { if existing != nil { // rg already exists, nothing to update. + // Note that rg tags are updated separately using tags service. return nil, nil } return resources.Group{ Location: to.StringPtr(s.Location), + // We create only CAPZ default tags. User defined additional tags + // are created and updated using tags service. Tags: converters.TagsToMap(infrav1.Build(infrav1.BuildParams{ ClusterName: s.ClusterName, Lifecycle: infrav1.ResourceLifecycleOwned, Name: to.StringPtr(s.Name), Role: to.StringPtr(infrav1.CommonRole), - Additional: s.AdditionalTags, })), }, nil } diff --git a/azure/services/tags/client.go b/azure/services/tags/client.go index 16b09e0a639..ed29d5fda67 100644 --- a/azure/services/tags/client.go +++ b/azure/services/tags/client.go @@ -29,7 +29,7 @@ import ( // client wraps go-sdk. type client interface { GetAtScope(context.Context, string) (resources.TagsResource, error) - CreateOrUpdateAtScope(context.Context, string, resources.TagsResource) (resources.TagsResource, error) + UpdateAtScope(context.Context, string, resources.TagsPatchResource) (resources.TagsResource, error) } // azureClient contains the Azure go-sdk Client. @@ -60,10 +60,11 @@ func (ac *azureClient) GetAtScope(ctx context.Context, scope string) (resources. return ac.tags.GetAtScope(ctx, scope) } -// CreateOrUpdateAtScope allows adding or replacing the entire set of tags on the specified resource or subscription. -func (ac *azureClient) CreateOrUpdateAtScope(ctx context.Context, scope string, parameters resources.TagsResource) (resources.TagsResource, error) { - ctx, _, done := tele.StartSpanWithLogger(ctx, "tags.AzureClient.CreateOrUpdateAtScope") +// UpdateAtScope this operation allows replacing, merging or selectively deleting tags on the specified resource or +// subscription. +func (ac *azureClient) UpdateAtScope(ctx context.Context, scope string, parameters resources.TagsPatchResource) (resources.TagsResource, error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "tags.AzureClient.UpdateAtScope") defer done() - return ac.tags.CreateOrUpdateAtScope(ctx, scope, parameters) + return ac.tags.UpdateAtScope(ctx, scope, parameters) } diff --git a/azure/services/tags/mock_tags/client_mock.go b/azure/services/tags/mock_tags/client_mock.go index 05b0885dbbe..3797994441d 100644 --- a/azure/services/tags/mock_tags/client_mock.go +++ b/azure/services/tags/mock_tags/client_mock.go @@ -51,32 +51,32 @@ func (m *Mockclient) EXPECT() *MockclientMockRecorder { return m.recorder } -// CreateOrUpdateAtScope mocks base method. -func (m *Mockclient) CreateOrUpdateAtScope(arg0 context.Context, arg1 string, arg2 resources.TagsResource) (resources.TagsResource, error) { +// GetAtScope mocks base method. +func (m *Mockclient) GetAtScope(arg0 context.Context, arg1 string) (resources.TagsResource, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateAtScope", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "GetAtScope", arg0, arg1) ret0, _ := ret[0].(resources.TagsResource) ret1, _ := ret[1].(error) return ret0, ret1 } -// CreateOrUpdateAtScope indicates an expected call of CreateOrUpdateAtScope. -func (mr *MockclientMockRecorder) CreateOrUpdateAtScope(arg0, arg1, arg2 interface{}) *gomock.Call { +// GetAtScope indicates an expected call of GetAtScope. +func (mr *MockclientMockRecorder) GetAtScope(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateAtScope", reflect.TypeOf((*Mockclient)(nil).CreateOrUpdateAtScope), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAtScope", reflect.TypeOf((*Mockclient)(nil).GetAtScope), arg0, arg1) } -// GetAtScope mocks base method. -func (m *Mockclient) GetAtScope(arg0 context.Context, arg1 string) (resources.TagsResource, error) { +// UpdateAtScope mocks base method. +func (m *Mockclient) UpdateAtScope(arg0 context.Context, arg1 string, arg2 resources.TagsPatchResource) (resources.TagsResource, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetAtScope", arg0, arg1) + ret := m.ctrl.Call(m, "UpdateAtScope", arg0, arg1, arg2) ret0, _ := ret[0].(resources.TagsResource) ret1, _ := ret[1].(error) return ret0, ret1 } -// GetAtScope indicates an expected call of GetAtScope. -func (mr *MockclientMockRecorder) GetAtScope(arg0, arg1 interface{}) *gomock.Call { +// UpdateAtScope indicates an expected call of UpdateAtScope. +func (mr *MockclientMockRecorder) UpdateAtScope(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAtScope", reflect.TypeOf((*Mockclient)(nil).GetAtScope), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateAtScope", reflect.TypeOf((*Mockclient)(nil).UpdateAtScope), arg0, arg1, arg2) } diff --git a/azure/services/tags/mock_tags/tags_mock.go b/azure/services/tags/mock_tags/tags_mock.go index 601f54cc653..15f7c9154f3 100644 --- a/azure/services/tags/mock_tags/tags_mock.go +++ b/azure/services/tags/mock_tags/tags_mock.go @@ -26,7 +26,6 @@ import ( autorest "github.com/Azure/go-autorest/autorest" logr "github.com/go-logr/logr" gomock "github.com/golang/mock/gomock" - v1beta1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" azure "sigs.k8s.io/cluster-api-provider-azure/azure" ) @@ -53,20 +52,6 @@ func (m *MockTagScope) EXPECT() *MockTagScopeMockRecorder { return m.recorder } -// AdditionalTags mocks base method. -func (m *MockTagScope) AdditionalTags() v1beta1.Tags { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AdditionalTags") - ret0, _ := ret[0].(v1beta1.Tags) - return ret0 -} - -// AdditionalTags indicates an expected call of AdditionalTags. -func (mr *MockTagScopeMockRecorder) AdditionalTags() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AdditionalTags", reflect.TypeOf((*MockTagScope)(nil).AdditionalTags)) -} - // AnnotationJSON mocks base method. func (m *MockTagScope) AnnotationJSON(arg0 string) (map[string]interface{}, error) { m.ctrl.T.Helper() @@ -96,20 +81,6 @@ func (mr *MockTagScopeMockRecorder) Authorizer() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Authorizer", reflect.TypeOf((*MockTagScope)(nil).Authorizer)) } -// AvailabilitySetEnabled mocks base method. -func (m *MockTagScope) AvailabilitySetEnabled() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AvailabilitySetEnabled") - ret0, _ := ret[0].(bool) - return ret0 -} - -// AvailabilitySetEnabled indicates an expected call of AvailabilitySetEnabled. -func (mr *MockTagScopeMockRecorder) AvailabilitySetEnabled() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AvailabilitySetEnabled", reflect.TypeOf((*MockTagScope)(nil).AvailabilitySetEnabled)) -} - // BaseURI mocks base method. func (m *MockTagScope) BaseURI() string { m.ctrl.T.Helper() @@ -166,20 +137,6 @@ func (mr *MockTagScopeMockRecorder) CloudEnvironment() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudEnvironment", reflect.TypeOf((*MockTagScope)(nil).CloudEnvironment)) } -// CloudProviderConfigOverrides mocks base method. -func (m *MockTagScope) CloudProviderConfigOverrides() *v1beta1.CloudProviderConfigOverrides { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CloudProviderConfigOverrides") - ret0, _ := ret[0].(*v1beta1.CloudProviderConfigOverrides) - return ret0 -} - -// CloudProviderConfigOverrides indicates an expected call of CloudProviderConfigOverrides. -func (mr *MockTagScopeMockRecorder) CloudProviderConfigOverrides() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudProviderConfigOverrides", reflect.TypeOf((*MockTagScope)(nil).CloudProviderConfigOverrides)) -} - // ClusterName mocks base method. func (m *MockTagScope) ClusterName() string { m.ctrl.T.Helper() @@ -225,20 +182,6 @@ func (mr *MockTagScopeMockRecorder) Error(err, msg interface{}, keysAndValues .. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Error", reflect.TypeOf((*MockTagScope)(nil).Error), varargs...) } -// FailureDomains mocks base method. -func (m *MockTagScope) FailureDomains() []string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "FailureDomains") - ret0, _ := ret[0].([]string) - return ret0 -} - -// FailureDomains indicates an expected call of FailureDomains. -func (mr *MockTagScopeMockRecorder) FailureDomains() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FailureDomains", reflect.TypeOf((*MockTagScope)(nil).FailureDomains)) -} - // HashKey mocks base method. func (m *MockTagScope) HashKey() string { m.ctrl.T.Helper() @@ -270,34 +213,6 @@ func (mr *MockTagScopeMockRecorder) Info(msg interface{}, keysAndValues ...inter return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Info", reflect.TypeOf((*MockTagScope)(nil).Info), varargs...) } -// Location mocks base method. -func (m *MockTagScope) Location() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Location") - ret0, _ := ret[0].(string) - return ret0 -} - -// Location indicates an expected call of Location. -func (mr *MockTagScopeMockRecorder) Location() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Location", reflect.TypeOf((*MockTagScope)(nil).Location)) -} - -// ResourceGroup mocks base method. -func (m *MockTagScope) ResourceGroup() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ResourceGroup") - ret0, _ := ret[0].(string) - return ret0 -} - -// ResourceGroup indicates an expected call of ResourceGroup. -func (mr *MockTagScopeMockRecorder) ResourceGroup() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResourceGroup", reflect.TypeOf((*MockTagScope)(nil).ResourceGroup)) -} - // SubscriptionID mocks base method. func (m *MockTagScope) SubscriptionID() string { m.ctrl.T.Helper() diff --git a/azure/services/tags/tags.go b/azure/services/tags/tags.go index 74a2d8fc2fd..56e0193039c 100644 --- a/azure/services/tags/tags.go +++ b/azure/services/tags/tags.go @@ -25,13 +25,15 @@ import ( "github.com/pkg/errors" "sigs.k8s.io/cluster-api-provider-azure/azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/converters" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) // TagScope defines the scope interface for a tags service. type TagScope interface { logr.Logger - azure.ClusterDescriber + azure.Authorizer + ClusterName() string TagsSpecs() []azure.TagsSpec AnnotationJSON(string) (map[string]interface{}, error) UpdateAnnotationJSON(string, map[string]interface{}) error @@ -57,31 +59,47 @@ func (s *Service) Reconcile(ctx context.Context) error { defer done() for _, tagsSpec := range s.Scope.TagsSpecs() { - annotation, err := s.Scope.AnnotationJSON(tagsSpec.Annotation) + existingTags, err := s.client.GetAtScope(ctx, tagsSpec.Scope) + if err != nil { + return errors.Wrap(err, "failed to get existing tags") + } + tags := make(map[string]*string) + if existingTags.Properties != nil && existingTags.Properties.Tags != nil { + tags = existingTags.Properties.Tags + } + + if !s.isResourceManaged(tags) { + s.Scope.V(4).Info("Skipping tags reconcile for not managed resource") + continue + } + + lastAppliedTags, err := s.Scope.AnnotationJSON(tagsSpec.Annotation) if err != nil { return err } - changed, created, deleted, newAnnotation := tagsChanged(annotation, tagsSpec.Tags) + changed, createdOrUpdated, deleted, newAnnotation := tagsChanged(lastAppliedTags, tagsSpec.Tags, tags) if changed { s.Scope.V(2).Info("Updating tags") - result, err := s.client.GetAtScope(ctx, tagsSpec.Scope) - if err != nil { - return errors.Wrap(err, "failed to get existing tags") - } - tags := make(map[string]*string) - if result.Properties != nil && result.Properties.Tags != nil { - tags = result.Properties.Tags - } - for k, v := range created { - tags[k] = to.StringPtr(v) + if len(createdOrUpdated) > 0 { + createdOrUpdatedTags := make(map[string]*string) + for k, v := range createdOrUpdated { + createdOrUpdatedTags[k] = to.StringPtr(v) + } + + if _, err := s.client.UpdateAtScope(ctx, tagsSpec.Scope, resources.TagsPatchResource{Operation: "Merge", Properties: &resources.Tags{Tags: createdOrUpdatedTags}}); err != nil { + return errors.Wrap(err, "cannot update tags") + } } - for k := range deleted { - delete(tags, k) - } + if len(deleted) > 0 { + deletedTags := make(map[string]*string) + for k, v := range deleted { + deletedTags[k] = to.StringPtr(v) + } - if _, err := s.client.CreateOrUpdateAtScope(ctx, tagsSpec.Scope, resources.TagsResource{Properties: &resources.Tags{Tags: tags}}); err != nil { - return errors.Wrap(err, "cannot update tags") + if _, err := s.client.UpdateAtScope(ctx, tagsSpec.Scope, resources.TagsPatchResource{Operation: "Delete", Properties: &resources.Tags{Tags: deletedTags}}); err != nil { + return errors.Wrap(err, "cannot update tags") + } } // We also need to update the annotation if anything changed. @@ -94,6 +112,10 @@ func (s *Service) Reconcile(ctx context.Context) error { return nil } +func (s *Service) isResourceManaged(tags map[string]*string) bool { + return converters.MapToTags(tags).HasOwned(s.Scope.ClusterName()) +} + // Delete is a no-op as the tags get deleted as part of VM deletion. func (s *Service) Delete(ctx context.Context) error { _, _, done := tele.StartSpanWithLogger(ctx, "tags.Service.Delete") @@ -103,12 +125,12 @@ func (s *Service) Delete(ctx context.Context) error { } // tagsChanged determines which tags to delete and which to add. -func tagsChanged(annotation map[string]interface{}, src map[string]string) (bool, map[string]string, map[string]string, map[string]interface{}) { +func tagsChanged(lastAppliedTags map[string]interface{}, desiredTags map[string]string, currentTags map[string]*string) (bool, map[string]string, map[string]string, map[string]interface{}) { // Bool tracking if we found any changed state. changed := false // Tracking for created/updated - created := map[string]string{} + createdOrUpdated := map[string]string{} // Tracking for tags that were deleted. deleted := map[string]string{} @@ -116,13 +138,13 @@ func tagsChanged(annotation map[string]interface{}, src map[string]string) (bool // The new annotation that we need to set if anything is created/updated. newAnnotation := map[string]interface{}{} - // Loop over annotation, checking if entries are in src. - // If an entry is present in annotation but not src, it has been deleted + // Loop over lastAppliedTags, checking if entries are in desiredTags. + // If an entry is present in lastAppliedTags but not in desiredTags, it has been deleted // since last time. We flag this in the deleted map. - for t, v := range annotation { - _, ok := src[t] + for t, v := range lastAppliedTags { + _, ok := desiredTags[t] - // Entry isn't in src, it has been deleted. + // Entry isn't in desiredTags, it has been deleted. if !ok { // Cast v to a string here. This should be fine, tags are always // strings. @@ -131,40 +153,40 @@ func tagsChanged(annotation map[string]interface{}, src map[string]string) (bool } } - // Loop over src, checking for entries in annotation. + // Loop over desiredTags, checking for entries in currentTags. // - // If an entry is in src, but not annotation, it has been created since - // last time. + // If an entry is in desiredTags, but not currentTags, it has been created since + // last time, or some external entity deleted it. // - // If an entry is in both src and annotation, we compare their values, if - // the value in src differs from that in annotation, the tag has been - // updated since last time. - for t, v := range src { - av, ok := annotation[t] + // If an entry is in both desiredTags and currentTags, we compare their values, if + // the value in desiredTags differs from that in currentTags, the tag has been + // updated since last time or some external entity modified it. + for t, v := range desiredTags { + av, ok := currentTags[t] - // Entries in the src always need to be noted in the newAnnotation. We + // Entries in the desiredTags always need to be noted in the newAnnotation. We // know they're going to be created or updated. newAnnotation[t] = v - // Entry isn't in annotation, it's new. + // Entry isn't in desiredTags, it's new. if !ok { - created[t] = v + createdOrUpdated[t] = v newAnnotation[t] = v changed = true continue } - // Entry is in annotation, has the value changed? - if v != av { - created[t] = v + // Entry is in desiredTags, has the value changed? + if v != *av { + createdOrUpdated[t] = v changed = true } - // Entry existed in both src and annotation, and their values were + // Entry existed in both desiredTags and desiredTags, and their values were // equal. Nothing to do. } - // We made it through the loop, and everything that was in src, was also + // We made it through the loop, and everything that was in desiredTags, was also // in dst. Nothing changed. - return changed, created, deleted, newAnnotation + return changed, createdOrUpdated, deleted, newAnnotation } diff --git a/azure/services/tags/tags_test.go b/azure/services/tags/tags_test.go index 1e296d3d4c6..32dd7dcd207 100644 --- a/azure/services/tags/tags_test.go +++ b/azure/services/tags/tags_test.go @@ -40,9 +40,70 @@ func TestReconcileTags(t *testing.T) { expectedError string }{ { - name: "create tags", + name: "create tags for managed resources", expectedError: "", expect: func(s *mock_tags.MockTagScopeMockRecorder, m *mock_tags.MockclientMockRecorder) { + s.ClusterName().AnyTimes().Return("test-cluster") + s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + gomock.InOrder( + s.TagsSpecs().Return([]azure.TagsSpec{ + { + Scope: "/sub/123/fake/scope", + Tags: map[string]string{ + "foo": "bar", + "thing": "stuff", + }, + Annotation: "my-annotation", + }, + { + Scope: "/sub/123/other/scope", + Tags: map[string]string{ + "tag1": "value1", + }, + Annotation: "my-annotation-2", + }, + }), + m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{Properties: &resources.Tags{ + Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": to.StringPtr("owned"), + "externalSystemTag": to.StringPtr("randomValue"), + }, + }}, nil), + s.AnnotationJSON("my-annotation"), + m.UpdateAtScope(gomockinternal.AContext(), "/sub/123/fake/scope", resources.TagsPatchResource{ + Operation: "Merge", + Properties: &resources.Tags{ + Tags: map[string]*string{ + "foo": to.StringPtr("bar"), + "thing": to.StringPtr("stuff"), + }, + }, + }), + s.UpdateAnnotationJSON("my-annotation", map[string]interface{}{"foo": "bar", "thing": "stuff"}), + m.GetAtScope(gomockinternal.AContext(), "/sub/123/other/scope").Return(resources.TagsResource{Properties: &resources.Tags{ + Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": to.StringPtr("owned"), + "externalSystem2Tag": to.StringPtr("randomValue2"), + }, + }}, nil), + s.AnnotationJSON("my-annotation-2"), + m.UpdateAtScope(gomockinternal.AContext(), "/sub/123/other/scope", resources.TagsPatchResource{ + Operation: "Merge", + Properties: &resources.Tags{ + Tags: map[string]*string{ + "tag1": to.StringPtr("value1"), + }, + }, + }), + s.UpdateAnnotationJSON("my-annotation-2", map[string]interface{}{"tag1": "value1"}), + ) + }, + }, + { + name: "do not create tags for unmanaged resources", + expectedError: "", + expect: func(s *mock_tags.MockTagScopeMockRecorder, m *mock_tags.MockclientMockRecorder) { + s.ClusterName().AnyTimes().Return("test-cluster") s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.TagsSpecs().Return([]azure.TagsSpec{ { @@ -53,41 +114,51 @@ func TestReconcileTags(t *testing.T) { }, Annotation: "my-annotation", }, - { - Scope: "/sub/123/other/scope", - Tags: map[string]string{ - "tag1": "value1", - }, - Annotation: "my-annotation-2", - }, }) m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{}, nil) - s.AnnotationJSON("my-annotation") - m.CreateOrUpdateAtScope(gomockinternal.AContext(), "/sub/123/fake/scope", resources.TagsResource{ - Properties: &resources.Tags{ + }, + }, + { + name: "delete removed tags", + expectedError: "", + expect: func(s *mock_tags.MockTagScopeMockRecorder, m *mock_tags.MockclientMockRecorder) { + s.ClusterName().AnyTimes().Return("test-cluster") + s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + gomock.InOrder( + s.TagsSpecs().Return([]azure.TagsSpec{ + { + Scope: "/sub/123/fake/scope", + Tags: map[string]string{ + "foo": "bar", + }, + Annotation: "my-annotation", + }, + }), + m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{Properties: &resources.Tags{ Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": to.StringPtr("owned"), "foo": to.StringPtr("bar"), "thing": to.StringPtr("stuff"), }, - }, - }) - s.UpdateAnnotationJSON("my-annotation", map[string]interface{}{"foo": "bar", "thing": "stuff"}) - m.GetAtScope(gomockinternal.AContext(), "/sub/123/other/scope").Return(resources.TagsResource{}, nil) - s.AnnotationJSON("my-annotation-2") - m.CreateOrUpdateAtScope(gomockinternal.AContext(), "/sub/123/other/scope", resources.TagsResource{ - Properties: &resources.Tags{ - Tags: map[string]*string{ - "tag1": to.StringPtr("value1"), + }}, nil), + s.AnnotationJSON("my-annotation").Return(map[string]interface{}{"foo": "bar", "thing": "stuff"}, nil), + m.UpdateAtScope(gomockinternal.AContext(), "/sub/123/fake/scope", resources.TagsPatchResource{ + Operation: "Delete", + Properties: &resources.Tags{ + Tags: map[string]*string{ + "thing": to.StringPtr("stuff"), + }, }, - }, - }) - s.UpdateAnnotationJSON("my-annotation-2", map[string]interface{}{"tag1": "value1"}) + }), + s.UpdateAnnotationJSON("my-annotation", map[string]interface{}{"foo": "bar"}), + ) }, }, { name: "error getting existing tags", expectedError: "failed to get existing tags: #: Internal Server Error: StatusCode=500", expect: func(s *mock_tags.MockTagScopeMockRecorder, m *mock_tags.MockclientMockRecorder) { + s.ClusterName().AnyTimes().Return("test-cluster") s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.TagsSpecs().Return([]azure.TagsSpec{ { @@ -99,7 +170,6 @@ func TestReconcileTags(t *testing.T) { Annotation: "my-annotation", }, }) - s.AnnotationJSON("my-annotation") m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) }, }, @@ -107,6 +177,7 @@ func TestReconcileTags(t *testing.T) { name: "error updating tags", expectedError: "cannot update tags: #: Internal Server Error: StatusCode=500", expect: func(s *mock_tags.MockTagScopeMockRecorder, m *mock_tags.MockclientMockRecorder) { + s.ClusterName().AnyTimes().Return("test-cluster") s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.TagsSpecs().Return([]azure.TagsSpec{ { @@ -117,9 +188,14 @@ func TestReconcileTags(t *testing.T) { Annotation: "my-annotation", }, }) - m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{}, nil) + m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{Properties: &resources.Tags{ + Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": to.StringPtr("owned"), + }, + }}, nil) s.AnnotationJSON("my-annotation") - m.CreateOrUpdateAtScope(gomockinternal.AContext(), "/sub/123/fake/scope", resources.TagsResource{ + m.UpdateAtScope(gomockinternal.AContext(), "/sub/123/fake/scope", resources.TagsPatchResource{ + Operation: "Merge", Properties: &resources.Tags{ Tags: map[string]*string{ "key": to.StringPtr("value"), @@ -132,6 +208,7 @@ func TestReconcileTags(t *testing.T) { name: "tags unchanged", expectedError: "", expect: func(s *mock_tags.MockTagScopeMockRecorder, m *mock_tags.MockclientMockRecorder) { + s.ClusterName().AnyTimes().Return("test-cluster") s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.TagsSpecs().Return([]azure.TagsSpec{ { @@ -142,6 +219,12 @@ func TestReconcileTags(t *testing.T) { Annotation: "my-annotation", }, }) + m.GetAtScope(gomockinternal.AContext(), "/sub/123/fake/scope").Return(resources.TagsResource{Properties: &resources.Tags{ + Tags: map[string]*string{ + "sigs.k8s.io_cluster-api-provider-azure_cluster_test-cluster": to.StringPtr("owned"), + "key": to.StringPtr("value"), + }, + }}, nil) s.AnnotationJSON("my-annotation").Return(map[string]interface{}{"key": "value"}, nil) }, }, @@ -179,35 +262,42 @@ func TestTagsChanged(t *testing.T) { g := NewWithT(t) var tests = map[string]struct { - annotation map[string]interface{} - src map[string]string - expectedResult bool - expectedCreated map[string]string - expectedDeleted map[string]string - expectedNewAnnotations map[string]interface{} + lastAppliedTags map[string]interface{} + desiredTags map[string]string + currentTags map[string]*string + expectedResult bool + expectedCreatedOrUpdated map[string]string + expectedDeleted map[string]string + expectedNewAnnotations map[string]interface{} }{ "tags are the same": { - annotation: map[string]interface{}{ + lastAppliedTags: map[string]interface{}{ "foo": "hello", }, - src: map[string]string{ + desiredTags: map[string]string{ "foo": "hello", }, - expectedResult: false, - expectedCreated: map[string]string{}, - expectedDeleted: map[string]string{}, + currentTags: map[string]*string{ + "foo": to.StringPtr("hello"), + }, + expectedResult: false, + expectedCreatedOrUpdated: map[string]string{}, + expectedDeleted: map[string]string{}, expectedNewAnnotations: map[string]interface{}{ "foo": "hello", }, }, "tag value changed": { - annotation: map[string]interface{}{ + lastAppliedTags: map[string]interface{}{ "foo": "hello", }, - src: map[string]string{ + desiredTags: map[string]string{ "foo": "goodbye", }, + currentTags: map[string]*string{ + "foo": to.StringPtr("hello"), + }, expectedResult: true, - expectedCreated: map[string]string{ + expectedCreatedOrUpdated: map[string]string{ "foo": "goodbye", }, expectedDeleted: map[string]string{}, @@ -215,26 +305,32 @@ func TestTagsChanged(t *testing.T) { "foo": "goodbye", }, }, "tag deleted": { - annotation: map[string]interface{}{ + lastAppliedTags: map[string]interface{}{ "foo": "hello", }, - src: map[string]string{}, - expectedResult: true, - expectedCreated: map[string]string{}, + desiredTags: map[string]string{}, + currentTags: map[string]*string{ + "foo": to.StringPtr("hello"), + }, + expectedResult: true, + expectedCreatedOrUpdated: map[string]string{}, expectedDeleted: map[string]string{ "foo": "hello", }, expectedNewAnnotations: map[string]interface{}{}, }, "tag created": { - annotation: map[string]interface{}{ + lastAppliedTags: map[string]interface{}{ "foo": "hello", }, - src: map[string]string{ + desiredTags: map[string]string{ "foo": "hello", "bar": "welcome", }, + currentTags: map[string]*string{ + "foo": to.StringPtr("hello"), + }, expectedResult: true, - expectedCreated: map[string]string{ + expectedCreatedOrUpdated: map[string]string{ "bar": "welcome", }, expectedDeleted: map[string]string{}, @@ -243,14 +339,17 @@ func TestTagsChanged(t *testing.T) { "bar": "welcome", }, }, "tag deleted and another created": { - annotation: map[string]interface{}{ + lastAppliedTags: map[string]interface{}{ "foo": "hello", }, - src: map[string]string{ + desiredTags: map[string]string{ "bar": "welcome", }, + currentTags: map[string]*string{ + "foo": to.StringPtr("hello"), + }, expectedResult: true, - expectedCreated: map[string]string{ + expectedCreatedOrUpdated: map[string]string{ "bar": "welcome", }, expectedDeleted: map[string]string{ @@ -259,13 +358,60 @@ func TestTagsChanged(t *testing.T) { expectedNewAnnotations: map[string]interface{}{ "bar": "welcome", }, + }, + "current tags removed by external entity": { + lastAppliedTags: map[string]interface{}{ + "foo": "hello", + "bar": "welcome", + }, + desiredTags: map[string]string{ + "foo": "hello", + "bar": "welcome", + }, + currentTags: map[string]*string{ + "foo": to.StringPtr("hello"), + }, + expectedResult: true, + expectedCreatedOrUpdated: map[string]string{ + "bar": "welcome", + }, + expectedDeleted: map[string]string{}, + expectedNewAnnotations: map[string]interface{}{ + "foo": "hello", + "bar": "welcome", + }, + }, + "current tags modified by external entity": { + lastAppliedTags: map[string]interface{}{ + "foo": "hello", + "bar": "welcome", + }, + desiredTags: map[string]string{ + "foo": "hello", + "bar": "welcome", + }, + currentTags: map[string]*string{ + "foo": to.StringPtr("hello"), + "bar": to.StringPtr("random"), + }, + expectedResult: true, + expectedCreatedOrUpdated: map[string]string{ + "bar": "welcome", + }, + expectedDeleted: map[string]string{}, + expectedNewAnnotations: map[string]interface{}{ + "foo": "hello", + "bar": "welcome", + }, }} for name, test := range tests { + test := test t.Run(name, func(t *testing.T) { - changed, created, deleted, newAnnotation := tagsChanged(test.annotation, test.src) + t.Parallel() + changed, createdOrUpdated, deleted, newAnnotation := tagsChanged(test.lastAppliedTags, test.desiredTags, test.currentTags) g.Expect(changed).To(Equal(test.expectedResult)) - g.Expect(created).To(Equal(test.expectedCreated)) + g.Expect(createdOrUpdated).To(Equal(test.expectedCreatedOrUpdated)) g.Expect(deleted).To(Equal(test.expectedDeleted)) g.Expect(newAnnotation).To(Equal(test.expectedNewAnnotations)) }) diff --git a/azure/types.go b/azure/types.go index c4c997fb590..4b9aca372d5 100644 --- a/azure/types.go +++ b/azure/types.go @@ -198,8 +198,11 @@ type ScaleSetSpec struct { // TagsSpec defines the specification for a set of tags. type TagsSpec struct { - Scope string - Tags infrav1.Tags + Scope string + Tags infrav1.Tags + // Annotation is the key which stores the last applied tags as value in JSON format. + // The last applied tags are used to find out which tags are being managed by CAPZ + // and if any has to be deleted by comparing it with the new desired tags Annotation string } diff --git a/controllers/azurecluster_reconciler.go b/controllers/azurecluster_reconciler.go index 1fd00a315da..c4445db3a44 100644 --- a/controllers/azurecluster_reconciler.go +++ b/controllers/azurecluster_reconciler.go @@ -34,6 +34,7 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/services/routetables" "sigs.k8s.io/cluster-api-provider-azure/azure/services/securitygroups" "sigs.k8s.io/cluster-api-provider-azure/azure/services/subnets" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/tags" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualnetworks" "sigs.k8s.io/cluster-api-provider-azure/azure/services/vnetpeerings" "sigs.k8s.io/cluster-api-provider-azure/util/tele" @@ -54,6 +55,7 @@ type azureClusterService struct { skuCache *resourceskus.Cache natGatewaySvc azure.Reconciler peeringsSvc azure.Reconciler + tagsSvc azure.Reconciler } // newAzureClusterService populates all the services based on input scope. @@ -77,6 +79,7 @@ func newAzureClusterService(scope *scope.ClusterScope) (*azureClusterService, er bastionSvc: bastionhosts.New(scope), skuCache: skuCache, peeringsSvc: vnetpeerings.New(scope), + tagsSvc: tags.New(scope), }, nil } @@ -138,6 +141,10 @@ func (s *azureClusterService) Reconcile(ctx context.Context) error { return errors.Wrap(err, "failed to reconcile bastion") } + if err := s.tagsSvc.Reconcile(ctx); err != nil { + return errors.Wrap(err, "unable to update tags") + } + return nil } diff --git a/exp/controllers/azuremanagedcontrolplane_reconciler.go b/exp/controllers/azuremanagedcontrolplane_reconciler.go index 579df8cdccb..373b7f16571 100644 --- a/exp/controllers/azuremanagedcontrolplane_reconciler.go +++ b/exp/controllers/azuremanagedcontrolplane_reconciler.go @@ -29,6 +29,7 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/services/groups" "sigs.k8s.io/cluster-api-provider-azure/azure/services/managedclusters" "sigs.k8s.io/cluster-api-provider-azure/azure/services/subnets" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/tags" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualnetworks" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) @@ -41,6 +42,7 @@ type azureManagedControlPlaneService struct { groupsSvc azure.Reconciler vnetSvc azure.Reconciler subnetsSvc azure.Reconciler + tagsSvc azure.Reconciler } // newAzureManagedControlPlaneReconciler populates all the services based on input scope. @@ -52,6 +54,7 @@ func newAzureManagedControlPlaneReconciler(scope *scope.ManagedControlPlaneScope groupsSvc: groups.New(scope), vnetSvc: virtualnetworks.New(scope), subnetsSvc: subnets.New(scope), + tagsSvc: tags.New(scope), } } @@ -81,6 +84,10 @@ func (r *azureManagedControlPlaneService) Reconcile(ctx context.Context) error { return errors.Wrap(err, "failed to reconcile kubeconfig secret") } + if err := r.tagsSvc.Reconcile(ctx); err != nil { + return errors.Wrap(err, "unable to update tags") + } + return nil }