diff --git a/azure/services/aso/aso.go b/azure/services/aso/aso.go index f860dfaca78..673940c5179 100644 --- a/azure/services/aso/aso.go +++ b/azure/services/aso/aso.go @@ -150,6 +150,12 @@ func (s *Service) CreateOrUpdateResource(ctx context.Context, spec azure.ASOReso parameters.SetName(resourceName) parameters.SetNamespace(resourceNamespace) + if t, ok := spec.(TagsGetterSetter); ok { + if err := reconcileTags(t, existing, parameters); err != nil { + return nil, errors.Wrap(err, "failed to reconcile tags") + } + } + labels := make(map[string]string) annotations := make(map[string]string) diff --git a/azure/services/aso/aso_test.go b/azure/services/aso/aso_test.go index cfd2658f6d0..8ffe228c334 100644 --- a/azure/services/aso/aso_test.go +++ b/azure/services/aso/aso_test.go @@ -33,6 +33,7 @@ import ( 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/mock_azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/aso/mock_aso" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -675,6 +676,127 @@ func TestCreateOrUpdateResource(t *testing.T) { g.Expect(err).NotTo(BeNil()) g.Expect(err.Error()).To(ContainSubstring("failed to update resource")) }) + + t.Run("with tags success", func(t *testing.T) { + g := NewGomegaWithT(t) + + sch := runtime.NewScheme() + g.Expect(asoresourcesv1.AddToScheme(sch)).To(Succeed()) + c := fakeclient.NewClientBuilder(). + WithScheme(sch). + Build() + s := New(c, clusterName) + + mockCtrl := gomock.NewController(t) + specMock := struct { + *mock_azure.MockASOResourceSpecGetter + *mock_aso.MockTagsGetterSetter + }{ + MockASOResourceSpecGetter: mock_azure.NewMockASOResourceSpecGetter(mockCtrl), + MockTagsGetterSetter: mock_aso.NewMockTagsGetterSetter(mockCtrl), + } + specMock.MockASOResourceSpecGetter.EXPECT().ResourceRef().Return(&asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + }) + specMock.MockASOResourceSpecGetter.EXPECT().Parameters(gomockinternal.AContext(), gomock.Any()).DoAndReturn(func(_ context.Context, object genruntime.MetaObject) (genruntime.MetaObject, error) { + return nil, nil + }) + specMock.MockASOResourceSpecGetter.EXPECT().WasManaged(gomock.Any()).Return(false) + + specMock.MockTagsGetterSetter.EXPECT().GetActualTags(gomock.Any()).Return(nil) + specMock.MockTagsGetterSetter.EXPECT().GetAdditionalTags().Return(nil) + specMock.MockTagsGetterSetter.EXPECT().GetDesiredTags(gomock.Any()).Return(nil) + specMock.MockTagsGetterSetter.EXPECT().SetTags(gomock.Any(), gomock.Any()) + + ctx := context.Background() + g.Expect(c.Create(ctx, &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + Labels: map[string]string{ + infrav1.OwnedByClusterLabelKey: clusterName, + }, + Annotations: map[string]string{ + ReconcilePolicyAnnotation: ReconcilePolicyManage, + }, + }, + Status: asoresourcesv1.ResourceGroup_STATUS{ + Conditions: []conditions.Condition{ + { + Type: conditions.ConditionTypeReady, + Status: metav1.ConditionTrue, + }, + }, + }, + })).To(Succeed()) + + result, err := s.CreateOrUpdateResource(ctx, specMock, "service") + g.Expect(result).To(BeNil()) + g.Expect(azure.IsOperationNotDoneError(err)).To(BeTrue()) + + updated := &asoresourcesv1.ResourceGroup{} + g.Expect(c.Get(ctx, types.NamespacedName{Name: "name", Namespace: "namespace"}, updated)).To(Succeed()) + g.Expect(updated.Annotations).To(HaveKey(tagsLastAppliedAnnotation)) + }) + + t.Run("with tags failure", func(t *testing.T) { + g := NewGomegaWithT(t) + + sch := runtime.NewScheme() + g.Expect(asoresourcesv1.AddToScheme(sch)).To(Succeed()) + c := fakeclient.NewClientBuilder(). + WithScheme(sch). + Build() + s := New(c, clusterName) + + mockCtrl := gomock.NewController(t) + specMock := struct { + *mock_azure.MockASOResourceSpecGetter + *mock_aso.MockTagsGetterSetter + }{ + MockASOResourceSpecGetter: mock_azure.NewMockASOResourceSpecGetter(mockCtrl), + MockTagsGetterSetter: mock_aso.NewMockTagsGetterSetter(mockCtrl), + } + specMock.MockASOResourceSpecGetter.EXPECT().ResourceRef().Return(&asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + }) + specMock.MockASOResourceSpecGetter.EXPECT().Parameters(gomockinternal.AContext(), gomock.Any()).DoAndReturn(func(_ context.Context, object genruntime.MetaObject) (genruntime.MetaObject, error) { + return nil, nil + }) + + ctx := context.Background() + g.Expect(c.Create(ctx, &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + Labels: map[string]string{ + infrav1.OwnedByClusterLabelKey: clusterName, + }, + Annotations: map[string]string{ + ReconcilePolicyAnnotation: ReconcilePolicyManage, + tagsLastAppliedAnnotation: "{", + }, + }, + Status: asoresourcesv1.ResourceGroup_STATUS{ + Conditions: []conditions.Condition{ + { + Type: conditions.ConditionTypeReady, + Status: metav1.ConditionTrue, + }, + }, + }, + })).To(Succeed()) + + result, err := s.CreateOrUpdateResource(ctx, specMock, "service") + g.Expect(result).To(BeNil()) + g.Expect(err.Error()).To(ContainSubstring("failed to reconcile tags")) + }) } // TestDeleteResource tests the DeleteResource function. diff --git a/azure/services/aso/interfaces.go b/azure/services/aso/interfaces.go index ddb2b27a62b..253f45bfbfa 100644 --- a/azure/services/aso/interfaces.go +++ b/azure/services/aso/interfaces.go @@ -20,6 +20,7 @@ import ( "context" "github.com/Azure/azure-service-operator/v2/pkg/genruntime" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" ) @@ -28,3 +29,11 @@ type Reconciler interface { CreateOrUpdateResource(ctx context.Context, spec azure.ASOResourceSpecGetter, serviceName string) (result genruntime.MetaObject, err error) DeleteResource(ctx context.Context, spec azure.ASOResourceSpecGetter, serviceName string) (err error) } + +// TagsGetterSetter represents an object that supports tags. +type TagsGetterSetter interface { + GetAdditionalTags() infrav1.Tags + GetDesiredTags(resource genruntime.MetaObject) infrav1.Tags + GetActualTags(resource genruntime.MetaObject) infrav1.Tags + SetTags(resource genruntime.MetaObject, tags infrav1.Tags) +} diff --git a/azure/services/aso/mock_aso/aso_mock.go b/azure/services/aso/mock_aso/aso_mock.go index 090975aee6a..29c4c24dff4 100644 --- a/azure/services/aso/mock_aso/aso_mock.go +++ b/azure/services/aso/mock_aso/aso_mock.go @@ -26,6 +26,7 @@ import ( genruntime "github.com/Azure/azure-service-operator/v2/pkg/genruntime" 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" ) @@ -80,3 +81,80 @@ func (mr *MockReconcilerMockRecorder) DeleteResource(ctx, spec, serviceName inte mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteResource", reflect.TypeOf((*MockReconciler)(nil).DeleteResource), ctx, spec, serviceName) } + +// MockTagsGetterSetter is a mock of TagsGetterSetter interface. +type MockTagsGetterSetter struct { + ctrl *gomock.Controller + recorder *MockTagsGetterSetterMockRecorder +} + +// MockTagsGetterSetterMockRecorder is the mock recorder for MockTagsGetterSetter. +type MockTagsGetterSetterMockRecorder struct { + mock *MockTagsGetterSetter +} + +// NewMockTagsGetterSetter creates a new mock instance. +func NewMockTagsGetterSetter(ctrl *gomock.Controller) *MockTagsGetterSetter { + mock := &MockTagsGetterSetter{ctrl: ctrl} + mock.recorder = &MockTagsGetterSetterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockTagsGetterSetter) EXPECT() *MockTagsGetterSetterMockRecorder { + return m.recorder +} + +// GetActualTags mocks base method. +func (m *MockTagsGetterSetter) GetActualTags(resource genruntime.MetaObject) v1beta1.Tags { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetActualTags", resource) + ret0, _ := ret[0].(v1beta1.Tags) + return ret0 +} + +// GetActualTags indicates an expected call of GetActualTags. +func (mr *MockTagsGetterSetterMockRecorder) GetActualTags(resource interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetActualTags", reflect.TypeOf((*MockTagsGetterSetter)(nil).GetActualTags), resource) +} + +// GetAdditionalTags mocks base method. +func (m *MockTagsGetterSetter) GetAdditionalTags() v1beta1.Tags { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetAdditionalTags") + ret0, _ := ret[0].(v1beta1.Tags) + return ret0 +} + +// GetAdditionalTags indicates an expected call of GetAdditionalTags. +func (mr *MockTagsGetterSetterMockRecorder) GetAdditionalTags() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAdditionalTags", reflect.TypeOf((*MockTagsGetterSetter)(nil).GetAdditionalTags)) +} + +// GetDesiredTags mocks base method. +func (m *MockTagsGetterSetter) GetDesiredTags(resource genruntime.MetaObject) v1beta1.Tags { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetDesiredTags", resource) + ret0, _ := ret[0].(v1beta1.Tags) + return ret0 +} + +// GetDesiredTags indicates an expected call of GetDesiredTags. +func (mr *MockTagsGetterSetterMockRecorder) GetDesiredTags(resource interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDesiredTags", reflect.TypeOf((*MockTagsGetterSetter)(nil).GetDesiredTags), resource) +} + +// SetTags mocks base method. +func (m *MockTagsGetterSetter) SetTags(resource genruntime.MetaObject, tags v1beta1.Tags) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetTags", resource, tags) +} + +// SetTags indicates an expected call of SetTags. +func (mr *MockTagsGetterSetterMockRecorder) SetTags(resource, tags interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetTags", reflect.TypeOf((*MockTagsGetterSetter)(nil).SetTags), resource, tags) +} diff --git a/azure/services/aso/tags.go b/azure/services/aso/tags.go new file mode 100644 index 00000000000..3c2429e08c1 --- /dev/null +++ b/azure/services/aso/tags.go @@ -0,0 +1,71 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package aso + +import ( + "encoding/json" + + "github.com/Azure/azure-service-operator/v2/pkg/genruntime" + "github.com/pkg/errors" + "sigs.k8s.io/cluster-api-provider-azure/azure/converters" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/tags" + "sigs.k8s.io/cluster-api-provider-azure/util/maps" +) + +// tagsLastAppliedAnnotation is the key for the annotation which tracks the AdditionalTags. +// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/ +// for annotation formatting rules. +const tagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-tags" + +// reconcileTags modifies parameters in place to update its tags and its last-applied annotation. +func reconcileTags(t TagsGetterSetter, existing genruntime.MetaObject, parameters genruntime.MetaObject) error { + lastAppliedTags := map[string]interface{}{} + if existing != nil { + lastAppliedTagsJSON := existing.GetAnnotations()[tagsLastAppliedAnnotation] + if lastAppliedTagsJSON != "" { + err := json.Unmarshal([]byte(lastAppliedTagsJSON), &lastAppliedTags) + if err != nil { + return errors.Wrapf(err, "failed to unmarshal JSON from %s annotation", tagsLastAppliedAnnotation) + } + } + } + + existingTags := t.GetActualTags(existing) + existingTagsMap := converters.TagsToMap(existingTags) + + _, createdOrUpdated, deleted, newAnnotation := tags.TagsChanged(lastAppliedTags, t.GetAdditionalTags(), existingTagsMap) + newTags := maps.Merge(maps.Merge(existingTags, t.GetDesiredTags(parameters)), createdOrUpdated) + for k := range deleted { + delete(newTags, k) + } + if len(newTags) == 0 { + newTags = nil + } + t.SetTags(parameters, newTags) + + // We also need to update the annotation even if nothing changed to + // ensure it's set immediately following resource creation. + newAnnotationJSON, err := json.Marshal(newAnnotation) + if err != nil { + return errors.Wrapf(err, "failed to marshal JSON to %s annotation", tagsLastAppliedAnnotation) + } + parameters.SetAnnotations(maps.Merge(parameters.GetAnnotations(), map[string]string{ + tagsLastAppliedAnnotation: string(newAnnotationJSON), + })) + + return nil +} diff --git a/azure/services/aso/tags_test.go b/azure/services/aso/tags_test.go new file mode 100644 index 00000000000..a6c0b4bc402 --- /dev/null +++ b/azure/services/aso/tags_test.go @@ -0,0 +1,126 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package aso + +import ( + "encoding/json" + "testing" + + asoresourcesv1 "github.com/Azure/azure-service-operator/v2/api/resources/v1api20200601" + "github.com/golang/mock/gomock" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/aso/mock_aso" +) + +func TestReconcileTags(t *testing.T) { + tests := []struct { + name string + lastAppliedTags infrav1.Tags + existingTags infrav1.Tags + additionalTagsSpec infrav1.Tags + tagsFromParams infrav1.Tags + expectedTags infrav1.Tags + }{ + { + name: "tag update", + lastAppliedTags: infrav1.Tags{ + "oldAdditionalTag": "oldAdditionalVal", + }, + existingTags: infrav1.Tags{ + "nonAdditionalTag": "nonAdditionalVal", + }, + additionalTagsSpec: infrav1.Tags{ + "additionalTag": "additionalVal", + }, + tagsFromParams: infrav1.Tags{ + "paramTag": "paramVal", + }, + expectedTags: infrav1.Tags{ + "additionalTag": "additionalVal", + "nonAdditionalTag": "nonAdditionalVal", + "paramTag": "paramVal", + }, + }, + { + name: "no tag update needed", + lastAppliedTags: infrav1.Tags{ + "additionalTag": "additionalVal", + }, + additionalTagsSpec: infrav1.Tags{ + "additionalTag": "additionalVal", + }, + expectedTags: infrav1.Tags{ + "additionalTag": "additionalVal", + }, + }, + { + name: "no additional tags", + lastAppliedTags: nil, + additionalTagsSpec: nil, + expectedTags: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + mockCtrl := gomock.NewController(t) + tag := mock_aso.NewMockTagsGetterSetter(mockCtrl) + + lastAppliedTagsJSON, err := json.Marshal(test.lastAppliedTags) + g.Expect(err).NotTo(HaveOccurred()) + + existing := &asoresourcesv1.ResourceGroup{} + if test.lastAppliedTags != nil { + existing.SetAnnotations(map[string]string{ + tagsLastAppliedAnnotation: string(lastAppliedTagsJSON), + }) + } + tag.EXPECT().GetActualTags(existing).Return(test.existingTags) + tag.EXPECT().GetAdditionalTags().Return(test.additionalTagsSpec) + + parameters := &asoresourcesv1.ResourceGroup{} + tag.EXPECT().GetDesiredTags(parameters).Return(test.tagsFromParams) + tag.EXPECT().SetTags(parameters, test.expectedTags) + + err = reconcileTags(tag, existing, parameters) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(parameters.GetAnnotations()).To(HaveKey(tagsLastAppliedAnnotation)) + }) + } + + t.Run("error unmarshaling last applied tags", func(t *testing.T) { + g := NewWithT(t) + + mockCtrl := gomock.NewController(t) + tag := mock_aso.NewMockTagsGetterSetter(mockCtrl) + + existing := &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + tagsLastAppliedAnnotation: "{", + }, + }, + } + + err := reconcileTags(tag, existing, nil) + g.Expect(err).To(HaveOccurred()) + }) +} diff --git a/azure/services/asogroups/spec.go b/azure/services/asogroups/spec.go index 4658407b8b9..a97b577854e 100644 --- a/azure/services/asogroups/spec.go +++ b/azure/services/asogroups/spec.go @@ -24,6 +24,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/aso" ) // GroupSpec defines the specification for a Resource Group. @@ -77,3 +78,34 @@ func (s *GroupSpec) WasManaged(object genruntime.MetaObject) bool { } return infrav1.Tags(group.Status.Tags).HasOwned(s.ClusterName) } + +var _ aso.TagsGetterSetter = (*GroupSpec)(nil) + +// GetAdditionalTags implements aso.TagsGetterSetter. +func (s *GroupSpec) GetAdditionalTags() infrav1.Tags { + return s.AdditionalTags +} + +// GetDesiredTags implements aso.TagsGetterSetter. +func (s *GroupSpec) GetDesiredTags(resource genruntime.MetaObject) infrav1.Tags { + if resource == nil { + return nil + } + return resource.(*asoresourcesv1.ResourceGroup).Spec.Tags +} + +// GetActualTags implements aso.TagsGetterSetter. +func (s *GroupSpec) GetActualTags(resource genruntime.MetaObject) infrav1.Tags { + if resource == nil { + return nil + } + return resource.(*asoresourcesv1.ResourceGroup).Status.Tags +} + +// SetTags implements aso.TagsGetterSetter. +func (s *GroupSpec) SetTags(resource genruntime.MetaObject, tags infrav1.Tags) { + if resource == nil { + return + } + resource.(*asoresourcesv1.ResourceGroup).Spec.Tags = tags +} diff --git a/azure/services/tags/tags.go b/azure/services/tags/tags.go index f9360ef26bc..17619881b91 100644 --- a/azure/services/tags/tags.go +++ b/azure/services/tags/tags.go @@ -89,7 +89,7 @@ func (s *Service) Reconcile(ctx context.Context) error { if err != nil { return err } - changed, createdOrUpdated, deleted, newAnnotation := tagsChanged(lastAppliedTags, tagsSpec.Tags, tags) + changed, createdOrUpdated, deleted, newAnnotation := TagsChanged(lastAppliedTags, tagsSpec.Tags, tags) if changed { log.V(2).Info("Updating tags") if len(createdOrUpdated) > 0 { @@ -137,8 +137,8 @@ func (s *Service) Delete(ctx context.Context) error { return nil } -// tagsChanged determines which tags to delete and which to add. -func tagsChanged(lastAppliedTags map[string]interface{}, desiredTags map[string]string, currentTags map[string]*string) (change bool, createOrUpdates map[string]string, deletes map[string]string, annotation map[string]interface{}) { +// TagsChanged determines which tags to delete and which to add. +func TagsChanged(lastAppliedTags map[string]interface{}, desiredTags map[string]string, currentTags map[string]*string) (change bool, createOrUpdates map[string]string, deletes map[string]string, annotation map[string]interface{}) { // Bool tracking if we found any changed state. changed := false diff --git a/azure/services/tags/tags_test.go b/azure/services/tags/tags_test.go index a5394b6d0e7..b1e1939bd7e 100644 --- a/azure/services/tags/tags_test.go +++ b/azure/services/tags/tags_test.go @@ -434,7 +434,7 @@ func TestTagsChanged(t *testing.T) { test := test t.Run(name, func(t *testing.T) { t.Parallel() - changed, createdOrUpdated, deleted, newAnnotation := tagsChanged(test.lastAppliedTags, test.desiredTags, test.currentTags) + changed, createdOrUpdated, deleted, newAnnotation := TagsChanged(test.lastAppliedTags, test.desiredTags, test.currentTags) g.Expect(changed).To(Equal(test.expectedResult)) g.Expect(createdOrUpdated).To(Equal(test.expectedCreatedOrUpdated)) g.Expect(deleted).To(Equal(test.expectedDeleted))