From 1163c8e4bfc481fcad6cf536c4882e367eeda16a Mon Sep 17 00:00:00 2001 From: Vijay Tripathi Date: Wed, 8 Jun 2022 11:06:17 -0700 Subject: [PATCH] Update `rm.EnsureTags` to accept controller metadata (#91) Issue #, if available: https://github.com/aws-controllers-k8s/community/issues/1261 Description of changes: * Pass `acktypes.ServiceControllerMetadata` parameter to `rm.EnsureTags` so that tag formats like `%CONTROLLER_VERSION%` can be resolved. * ACK controller metadata required to expand tag formats is present inside `ServiceControllerMetadata` object. * Change the return type of `ackrt.GetDefaultTags` method from `map[string]string` to `acktags.Tags` . By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --- mocks/pkg/types/aws_resource_manager.go | 10 +-- pkg/runtime/reconciler.go | 4 +- pkg/runtime/reconciler_test.go | 93 +++++++++++++------------ pkg/runtime/tags.go | 10 +-- pkg/runtime/tags_test.go | 6 +- pkg/types/aws_resource_manager.go | 2 +- 6 files changed, 64 insertions(+), 61 deletions(-) diff --git a/mocks/pkg/types/aws_resource_manager.go b/mocks/pkg/types/aws_resource_manager.go index 84b87d5..da247c5 100644 --- a/mocks/pkg/types/aws_resource_manager.go +++ b/mocks/pkg/types/aws_resource_manager.go @@ -78,13 +78,13 @@ func (_m *AWSResourceManager) Delete(_a0 context.Context, _a1 types.AWSResource) return r0, r1 } -// EnsureTags provides a mock function with given fields: _a0, _a1 -func (_m *AWSResourceManager) EnsureTags(_a0 context.Context, _a1 types.AWSResource) error { - ret := _m.Called(_a0, _a1) +// EnsureTags provides a mock function with given fields: _a0, _a1, _a2 +func (_m *AWSResourceManager) EnsureTags(_a0 context.Context, _a1 types.AWSResource, _a2 types.ServiceControllerMetadata) error { + ret := _m.Called(_a0, _a1, _a2) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, types.AWSResource) error); ok { - r0 = rf(_a0, _a1) + if rf, ok := ret.Get(0).(func(context.Context, types.AWSResource, types.ServiceControllerMetadata) error); ok { + r0 = rf(_a0, _a1, _a2) } else { r0 = ret.Error(0) } diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 7d2363b..449a9ea 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -234,7 +234,7 @@ func (r *resourceReconciler) Sync( desired = resolvedRefDesired rlog.Enter("rm.EnsureTags") - err = rm.EnsureTags(ctx, desired) + err = rm.EnsureTags(ctx, desired, r.sc.GetMetadata()) rlog.Exit("rm.EnsureTags", err) if err != nil { return desired, err @@ -386,7 +386,7 @@ func (r *resourceReconciler) createResource( // because they are not persisted in etcd. So we again ensure // that tags are present before performing the create operation. rlog.Enter("rm.EnsureTags") - err = rm.EnsureTags(ctx, desired) + err = rm.EnsureTags(ctx, desired, r.sc.GetMetadata()) rlog.Exit("rm.EnsureTags", err) if err != nil { return desired, err diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index 261bf49..99f445d 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -84,6 +84,7 @@ func reconcilerMocks( ) ( acktypes.AWSResourceReconciler, *ctrlrtclientmock.Client, + acktypes.ServiceControllerMetadata, ) { zapOptions := ctrlrtzap.Options{ Development: true, @@ -94,11 +95,13 @@ func reconcilerMocks( metrics := ackmetrics.NewMetrics("bookstore") sc := &ackmocks.ServiceController{} + scmd := acktypes.ServiceControllerMetadata{} + sc.On("GetMetadata").Return(scmd) kc := &ctrlrtclientmock.Client{} return ackrt.NewReconcilerWithClient( sc, kc, rmf, fakeLogger, cfg, metrics, ackrtcache.Caches{}, - ), kc + ), kc, scmd } func managedResourceManagerFactoryMocks( @@ -183,9 +186,9 @@ func TestReconcilerCreate_BackoffRetries(t *testing.T) { rd.On("IsManaged", desired).Return(true) rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) _, err := r.Sync(ctx, rm, desired) require.Nil(err) @@ -241,9 +244,9 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveTwice(t *testi rd.On("IsManaged", desired).Return(true) rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during // assertion even when parameters inside two objects are same. @@ -269,7 +272,7 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveTwice(t *testi rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) rm.AssertNumberOfCalls(t, "EnsureTags", 2) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing.T) { @@ -319,9 +322,9 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing. rd.On("IsManaged", desired).Return(true) rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during // assertion even when parameters inside two objects are same. @@ -347,7 +350,7 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing. rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) rm.AssertNumberOfCalls(t, "EnsureTags", 1) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate(t *testing.T) { @@ -399,9 +402,9 @@ func TestReconcilerUpdate(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during // assertion even when parameters inside two objects are same. @@ -427,7 +430,7 @@ func TestReconcilerUpdate(t *testing.T) { kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { @@ -483,9 +486,9 @@ func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during // assertion even when parameters inside two objects are same. @@ -509,7 +512,7 @@ func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { @@ -558,9 +561,9 @@ func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rd.On("Delta", latest, latest).Return(delta) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during // assertion even when parameters inside two objects are same. @@ -585,7 +588,7 @@ func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { @@ -634,9 +637,9 @@ func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rd.On("Delta", latest, latest).Return(delta) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during // assertion even when parameters inside two objects are same. @@ -661,7 +664,7 @@ func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_IsSyncedError(t *testing.T) { @@ -721,9 +724,9 @@ func TestReconcilerUpdate_IsSyncedError(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) // pointers returned from "client.MergeFrom" fails the equality check during // assertion even when parameters inside two objects are same. @@ -747,7 +750,7 @@ func TestReconcilerUpdate_IsSyncedError(t *testing.T) { kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { @@ -795,9 +798,9 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rm.On("IsSynced", ctx, latest).Return(true, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) @@ -813,7 +816,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { rm.AssertCalled(t, "LateInitialize", ctx, latest) latest.AssertCalled(t, "DeepCopy") latest.AssertCalled(t, "SetStatus", latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { @@ -872,9 +875,9 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rm.On("IsSynced", ctx, latest).Return(true, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) @@ -888,7 +891,7 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerHandleReconcilerError_PatchStatus_Latest(t *testing.T) { @@ -917,7 +920,7 @@ func TestReconcilerHandleReconcilerError_PatchStatus_Latest(t *testing.T) { latestMetaObj.SetAnnotations(map[string]string{"a": "b"}) rmf, _ := managedResourceManagerFactoryMocks(desired, latest) - r, kc := reconcilerMocks(rmf) + r, kc, _ := reconcilerMocks(rmf) statusWriter := &ctrlrtclientmock.StatusWriter{} kc.On("Status").Return(statusWriter) @@ -942,7 +945,7 @@ func TestReconcilerHandleReconcilerError_NoPatchStatus_NoLatest(t *testing.T) { desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return() rmf, _ := managedResourceManagerFactoryMocks(desired, nil) - r, kc := reconcilerMocks(rmf) + r, kc, _ := reconcilerMocks(rmf) statusWriter := &ctrlrtclientmock.StatusWriter{} kc.On("Status").Return(statusWriter) @@ -1019,9 +1022,9 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, requeueError) rm.On("IsSynced", ctx, latest).Return(true, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) @@ -1036,7 +1039,7 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) { // No difference in desired, latest metadata and spec kc.AssertNotCalled(t, "Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")) rm.AssertCalled(t, "LateInitialize", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { @@ -1124,11 +1127,11 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { latest, nil, ) rm.On("IsSynced", ctx, latest).Return(true, nil) - rm.On("EnsureTags", ctx, desired).Return(nil) rmf, rd := managerFactoryMocks(desired, latest, false) - r, _ := reconcilerMocks(rmf) + r, _, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) _, err := r.Sync(ctx, rm, desired) require.NotNil(err) @@ -1138,7 +1141,7 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { rd.AssertNotCalled(t, "Delta", desired, latest) rm.AssertNotCalled(t, "Update", ctx, desired, latest, delta) rm.AssertNotCalled(t, "LateInitialize", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { @@ -1201,9 +1204,9 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rm.On("IsSynced", ctx, latest).Return(true, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return(nil) - r, kc := reconcilerMocks(rmf) + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return(nil) kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) @@ -1223,7 +1226,7 @@ func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus kc.AssertNotCalled(t, "Status") rm.AssertNotCalled(t, "LateInitialize", ctx, latest) - rm.AssertNotCalled(t, "EnsureTags", ctx, desired) + rm.AssertNotCalled(t, "EnsureTags", ctx, desired, scmd) } func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { @@ -1284,12 +1287,12 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { rm.On("LateInitialize", ctx, latest).Return(latest, nil) rm.On("IsSynced", ctx, latest).Return(true, nil) rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()) - rm.On("EnsureTags", ctx, desired).Return( + + r, kc, scmd := reconcilerMocks(rmf) + rm.On("EnsureTags", ctx, desired, scmd).Return( ensureControllerTagsError, ) - r, kc := reconcilerMocks(rmf) - kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) // With the above mocks and below assertions, we check that if we got a @@ -1308,5 +1311,5 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus kc.AssertNotCalled(t, "Status") rm.AssertNotCalled(t, "LateInitialize", ctx, latest) - rm.AssertCalled(t, "EnsureTags", ctx, desired) + rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } diff --git a/pkg/runtime/tags.go b/pkg/runtime/tags.go index 17a2738..2be889f 100644 --- a/pkg/runtime/tags.go +++ b/pkg/runtime/tags.go @@ -82,11 +82,11 @@ func GetDefaultTags( config *ackconfig.Config, obj rtclient.Object, md acktypes.ServiceControllerMetadata, -) map[string]string { +) acktags.Tags { + defaultTags := acktags.NewTags() if obj == nil || config == nil || len(config.ResourceTags) == 0 { - return nil + return defaultTags } - var populatedTags = make(map[string]string) for _, tagKeyVal := range config.ResourceTags { keyVal := strings.Split(tagKeyVal, "=") if keyVal == nil || len(keyVal) != 2 { @@ -97,9 +97,9 @@ func GetDefaultTags( if key == "" || val == "" { continue } - populatedTags[key] = expandTagValue(val, obj, md) + defaultTags[key] = expandTagValue(val, obj, md) } - return populatedTags + return defaultTags } // expandTagValue returns the tag value after expanding all the ACKResourceTag diff --git a/pkg/runtime/tags_test.go b/pkg/runtime/tags_test.go index 1bbc9cb..bb99206 100644 --- a/pkg/runtime/tags_test.go +++ b/pkg/runtime/tags_test.go @@ -42,13 +42,13 @@ func TestGetDefaultTags(t *testing.T) { } // nil config - assert.Nil(runtime.GetDefaultTags(nil, &obj, md)) + assert.Empty(runtime.GetDefaultTags(nil, &obj, md)) // nil object - assert.Nil(runtime.GetDefaultTags(&cfg, nil, md)) + assert.Empty(runtime.GetDefaultTags(&cfg, nil, md)) // no resource tags - assert.Nil(runtime.GetDefaultTags(&cfg, &obj, md)) + assert.Empty(runtime.GetDefaultTags(&cfg, &obj, md)) // ill formed tags cfg.ResourceTags = []string{"foobar"} diff --git a/pkg/types/aws_resource_manager.go b/pkg/types/aws_resource_manager.go index a2788e7..2e449e1 100644 --- a/pkg/types/aws_resource_manager.go +++ b/pkg/types/aws_resource_manager.go @@ -92,7 +92,7 @@ type AWSResourceManager interface { // added to the existing resource tags without overriding them. // If the AWSResource does not support tags, only then the controller tags // will not be added to the AWSResource. - EnsureTags(context.Context, AWSResource) error + EnsureTags(context.Context, AWSResource, ServiceControllerMetadata) error } // AWSResourceManagerFactory returns an AWSResourceManager that can be used to