Merge pull request #4154 from nojnhuh/aso-tags

[release-1.11] wait for ASO tags to converge before updating
kubernetes-sigs · Oct 18, 2023 · fe7b9ff · fe7b9ff
2 parents 1db4717 + 4a9486d
commit fe7b9ff
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 6 deletions.
diff --git a/azure/services/aso/aso_test.go b/azure/services/aso/aso_test.go
@@ -736,7 +736,7 @@ func TestCreateOrUpdateResource(t *testing.T) {
 
 		specMock.MockTagsGetterSetter.EXPECT().GetActualTags(gomock.Any()).Return(nil, nil)
 		specMock.MockTagsGetterSetter.EXPECT().GetAdditionalTags().Return(nil)
-		specMock.MockTagsGetterSetter.EXPECT().GetDesiredTags(gomock.Any()).Return(nil, nil)
+		specMock.MockTagsGetterSetter.EXPECT().GetDesiredTags(gomock.Any()).Return(nil, nil).Times(2)
 		specMock.MockTagsGetterSetter.EXPECT().SetTags(gomock.Any(), gomock.Any())
 
 		ctx := context.Background()

diff --git a/azure/services/aso/tags.go b/azure/services/aso/tags.go
@@ -18,9 +18,13 @@ 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"
@@ -33,6 +37,7 @@ const tagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-a
 
 // 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 {
+	var existingTags infrav1.Tags
 	lastAppliedTags := map[string]interface{}{}
 	if existing != nil {
 		lastAppliedTagsJSON := existing.GetAnnotations()[tagsLastAppliedAnnotation]
@@ -42,14 +47,29 @@ func reconcileTags(t TagsGetterSetter, existing genruntime.MetaObject, parameter
 				return errors.Wrapf(err, "failed to unmarshal JSON from %s annotation", tagsLastAppliedAnnotation)
 			}
 		}
-	}
 
-	existingTags, err := t.GetActualTags(existing)
-	if err != nil {
-		return errors.Wrapf(err, "failed to get actual tags for %s %s/%s", existing.GetObjectKind().GroupVersionKind(), existing.GetNamespace(), existing.GetName())
+		var err error
+		existingTags, err = t.GetActualTags(existing)
+		if err != nil {
+			return errors.Wrapf(err, "failed to get actual tags for %s %s/%s", existing.GetObjectKind().GroupVersionKind(), existing.GetNamespace(), existing.GetName())
+		}
+		desiredTags, err := t.GetDesiredTags(existing)
+		if err != nil {
+			return errors.Wrapf(err, "failed to get desired tags for %s %s/%s", existing.GetObjectKind().GroupVersionKind(), existing.GetNamespace(), existing.GetName())
+		}
+		// 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(desiredTags, existingTags) &&
+			existing.GetAnnotations()[asoannotations.ReconcilePolicy] == string(asoannotations.ReconcilePolicyManage) {
+			return azure.WithTransientError(azure.NewOperationNotDoneError(&infrav1.Future{
+				Type:          createOrUpdateFutureType,
+				ResourceGroup: existing.GetNamespace(),
+				Name:          existing.GetName(),
+			}), requeueInterval)
+		}
 	}
-	existingTagsMap := converters.TagsToMap(existingTags)
 
+	existingTagsMap := converters.TagsToMap(existingTags)
 	_, createdOrUpdated, deleted, newAnnotation := tags.TagsChanged(lastAppliedTags, t.GetAdditionalTags(), existingTagsMap)
 	desiredTags, err := t.GetDesiredTags(parameters)
 	if err != nil {

diff --git a/azure/services/aso/tags_test.go b/azure/services/aso/tags_test.go
@@ -21,11 +21,13 @@ import (
 	"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"
 	"github.com/pkg/errors"
 	"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"
 )
 
@@ -44,6 +46,7 @@ func TestReconcileTags(t *testing.T) {
 				"oldAdditionalTag": "oldAdditionalVal",
 			},
 			existingTags: infrav1.Tags{
+				"oldAdditionalTag": "oldAdditionalVal",
 				"nonAdditionalTag": "nonAdditionalVal",
 			},
 			additionalTagsSpec: infrav1.Tags{
@@ -95,6 +98,7 @@ func TestReconcileTags(t *testing.T) {
 				})
 			}
 			tag.EXPECT().GetActualTags(existing).Return(test.existingTags, nil)
+			tag.EXPECT().GetDesiredTags(existing).Return(test.existingTags, nil)
 			tag.EXPECT().GetAdditionalTags().Return(test.additionalTagsSpec)
 
 			parameters := &asoresourcesv1.ResourceGroup{}
@@ -146,6 +150,7 @@ func TestReconcileTags(t *testing.T) {
 
 		existing := &asoresourcesv1.ResourceGroup{}
 		tag.EXPECT().GetActualTags(existing).Return(nil, nil)
+		tag.EXPECT().GetDesiredTags(existing).Return(nil, nil)
 		tag.EXPECT().GetAdditionalTags().Return(nil)
 
 		parameters := &asoresourcesv1.ResourceGroup{}
@@ -163,6 +168,7 @@ func TestReconcileTags(t *testing.T) {
 
 		existing := &asoresourcesv1.ResourceGroup{}
 		tag.EXPECT().GetActualTags(existing).Return(nil, nil)
+		tag.EXPECT().GetDesiredTags(existing).Return(nil, nil)
 		tag.EXPECT().GetAdditionalTags().Return(nil)
 
 		parameters := &asoresourcesv1.ResourceGroup{}
@@ -172,4 +178,27 @@ func TestReconcileTags(t *testing.T) {
 		err := reconcileTags(tag, existing, parameters)
 		g.Expect(err).To(MatchError(ContainSubstring("some error")))
 	})
+
+	t.Run("existing tags not up to date", 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{
+					asoannotations.ReconcilePolicy: string(asoannotations.ReconcilePolicyManage),
+				},
+			},
+		}
+		tag.EXPECT().GetActualTags(existing).Return(infrav1.Tags{"new": "value"}, nil)
+		tag.EXPECT().GetDesiredTags(existing).Return(infrav1.Tags{"old": "tag"}, nil)
+
+		err := reconcileTags(tag, existing, 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())
+	})
 }