Merge pull request #4541 from nojnhuh/aso-tags-1.13

[release-1.13] only consider spec in existing when reconciling ASO tags

Author: k8s-ci-robot

azure/services/aso/aso.go

@@ -152,9 +152,6 @@ func (r *reconciler[T]) CreateOrUpdateResource(ctx context.Context, spec azure.A
 
 	if t, ok := spec.(TagsGetterSetter[T]); ok {
 		if err := reconcileTags(t, existing, resourceExists, parameters); err != nil {
-			if azure.IsOperationNotDoneError(err) && readyErr != nil {
-				return zero, readyErr
-			}
 			return zero, errors.Wrap(err, "failed to reconcile tags")
 		}
 	}

azure/services/aso/aso_test.go

@@ -731,7 +731,6 @@ func TestCreateOrUpdateResource(t *testing.T) {
 		})
 		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).Times(2)
 		specMock.MockTagsGetterSetter.EXPECT().SetTags(gomock.Any(), gomock.Any())
@@ -823,71 +822,6 @@ func TestCreateOrUpdateResource(t *testing.T) {
 		g.Expect(err.Error()).To(ContainSubstring("failed to reconcile tags"))
 	})
 
-	t.Run("with tags not done error and readyErr", 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[*asoresourcesv1.ResourceGroup](c, clusterName)
-
-		mockCtrl := gomock.NewController(t)
-		specMock := struct {
-			*mock_azure.MockASOResourceSpecGetter[*asoresourcesv1.ResourceGroup]
-			*mock_aso.MockTagsGetterSetter[*asoresourcesv1.ResourceGroup]
-		}{
-			MockASOResourceSpecGetter: mock_azure.NewMockASOResourceSpecGetter[*asoresourcesv1.ResourceGroup](mockCtrl),
-			MockTagsGetterSetter:      mock_aso.NewMockTagsGetterSetter[*asoresourcesv1.ResourceGroup](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, group *asoresourcesv1.ResourceGroup) (*asoresourcesv1.ResourceGroup, error) {
-			return group, nil
-		})
-
-		existing := &asoresourcesv1.ResourceGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "name",
-				Namespace: "namespace",
-				Labels: map[string]string{
-					infrav1.OwnedByClusterLabelKey: clusterName,
-				},
-				Annotations: map[string]string{
-					asoannotations.ReconcilePolicy: string(asoannotations.ReconcilePolicyManage),
-				},
-			},
-			Spec: asoresourcesv1.ResourceGroup_Spec{
-				Tags: map[string]string{"desired": "tags"},
-			},
-			Status: asoresourcesv1.ResourceGroup_STATUS{
-				Tags: map[string]string{"actual": "tags"},
-				Conditions: []conditions.Condition{
-					{
-						Type:    conditions.ConditionTypeReady,
-						Status:  metav1.ConditionFalse,
-						Message: "not ready :(",
-					},
-				},
-			},
-		}
-
-		specMock.MockTagsGetterSetter.EXPECT().GetActualTags(gomock.Any()).Return(existing.Status.Tags)
-		specMock.MockTagsGetterSetter.EXPECT().GetDesiredTags(gomock.Any()).Return(existing.Spec.Tags)
-
-		ctx := context.Background()
-		g.Expect(c.Create(ctx, existing)).To(Succeed())
-
-		result, err := s.CreateOrUpdateResource(ctx, specMock, "service")
-		g.Expect(result).To(BeNil())
-		g.Expect(err.Error()).To(ContainSubstring("not ready :("))
-	})
-
 	t.Run("reconcile policy annotation resets after un-pause", func(t *testing.T) {
 		g := NewGomegaWithT(t)
 

azure/services/aso/interfaces.go

@@ -36,7 +36,6 @@ type Reconciler[T genruntime.MetaObject] interface {
 type TagsGetterSetter[T genruntime.MetaObject] interface {
 	GetAdditionalTags() infrav1.Tags
 	GetDesiredTags(resource T) infrav1.Tags
-	GetActualTags(resource T) infrav1.Tags
 	SetTags(resource T, tags infrav1.Tags)
 }
 

azure/services/aso/mock_aso/aso_mock.go

@@ -18,13 +18,10 @@ package aso
 
 import (
 	"encoding/json"
-	"reflect"
 
-	asoannotations "github.com/Azure/azure-service-operator/v2/pkg/common/annotations"
 	"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
 	"github.com/pkg/errors"
 	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/tags"
 	"sigs.k8s.io/cluster-api-provider-azure/util/maps"
@@ -49,17 +46,7 @@ func reconcileTags[T genruntime.MetaObject](t TagsGetterSetter[T], existing T, r
 			}
 		}
 
-		existingTags = t.GetActualTags(existing)
-		// Wait for tags to converge so we know for sure which ones are deleted from additionalTags (and
-		// should be deleted) and which were added manually (and should be kept).
-		if !reflect.DeepEqual(t.GetDesiredTags(existing), existingTags) &&
-			existing.GetAnnotations()[asoannotations.ReconcilePolicy] == string(asoannotations.ReconcilePolicyManage) {
-			return azure.WithTransientError(azure.NewOperationNotDoneError(&infrav1.Future{
-				Type:          createOrUpdateFutureType,
-				ResourceGroup: existing.GetNamespace(),
-				Name:          existing.GetName(),
-			}), requeueInterval)
-		}
+		existingTags = t.GetDesiredTags(existing)
 	}
 
 	existingTagsMap := converters.TagsToMap(existingTags)

azure/services/aso/tags.go

@@ -18,16 +18,13 @@ package aso
 
 import (
 	"encoding/json"
-	"errors"
 	"testing"
 
 	asoresourcesv1 "github.com/Azure/azure-service-operator/v2/api/resources/v1api20200601"
-	asoannotations "github.com/Azure/azure-service-operator/v2/pkg/common/annotations"
 	. "github.com/onsi/gomega"
 	"go.uber.org/mock/gomock"
 	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"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/aso/mock_aso"
 )
 
@@ -97,7 +94,6 @@ func TestReconcileTags(t *testing.T) {
 					tagsLastAppliedAnnotation: string(lastAppliedTagsJSON),
 				})
 			}
-			tag.EXPECT().GetActualTags(existing).Return(test.existingTags)
 			tag.EXPECT().GetDesiredTags(existing).Return(test.existingTags)
 			tag.EXPECT().GetAdditionalTags().Return(test.additionalTagsSpec)
 
@@ -128,27 +124,4 @@ func TestReconcileTags(t *testing.T) {
 		err := reconcileTags[*asoresourcesv1.ResourceGroup](tag, existing, existing != nil, nil)
 		g.Expect(err).To(HaveOccurred())
 	})
-
-	t.Run("existing tags not up to date", func(t *testing.T) {
-		g := NewWithT(t)
-
-		mockCtrl := gomock.NewController(t)
-		tag := mock_aso.NewMockTagsGetterSetter[*asoresourcesv1.ResourceGroup](mockCtrl)
-
-		existing := &asoresourcesv1.ResourceGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Annotations: map[string]string{
-					asoannotations.ReconcilePolicy: string(asoannotations.ReconcilePolicyManage),
-				},
-			},
-		}
-		tag.EXPECT().GetActualTags(existing).Return(infrav1.Tags{"new": "value"})
-		tag.EXPECT().GetDesiredTags(existing).Return(infrav1.Tags{"old": "tag"})
-
-		err := reconcileTags[*asoresourcesv1.ResourceGroup](tag, existing, existing != nil, nil)
-		g.Expect(azure.IsOperationNotDoneError(err)).To(BeTrue())
-		var recerr azure.ReconcileError
-		g.Expect(errors.As(err, &recerr)).To(BeTrue())
-		g.Expect(recerr.IsTransient()).To(BeTrue())
-	})
 }

azure/services/aso/tags_test.go

@@ -86,11 +86,6 @@ func (*GroupSpec) GetDesiredTags(resource *asoresourcesv1.ResourceGroup) infrav1
 	return resource.Spec.Tags
 }
 
-// GetActualTags implements aso.TagsGetterSetter.
-func (*GroupSpec) GetActualTags(resource *asoresourcesv1.ResourceGroup) infrav1.Tags {
-	return resource.Status.Tags
-}
-
 // SetTags implements aso.TagsGetterSetter.
 func (*GroupSpec) SetTags(resource *asoresourcesv1.ResourceGroup, tags infrav1.Tags) {
 	resource.Spec.Tags = tags

azure/services/groups/spec.go

@@ -601,11 +601,6 @@ func (*ManagedClusterSpec) GetDesiredTags(resource *asocontainerservicev1.Manage
 	return resource.Spec.Tags
 }
 
-// GetActualTags implements aso.TagsGetterSetter.
-func (*ManagedClusterSpec) GetActualTags(resource *asocontainerservicev1.ManagedCluster) infrav1.Tags {
-	return resource.Status.Tags
-}
-
 // SetTags implements aso.TagsGetterSetter.
 func (*ManagedClusterSpec) SetTags(resource *asocontainerservicev1.ManagedCluster, tags infrav1.Tags) {
 	resource.Spec.Tags = tags