Skip to content

Commit

Permalink
Update rm.EnsureTags to accept controller metadata (#91)
Browse files Browse the repository at this point in the history 
Issue #, if available: aws-controllers-k8s/community#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.
  • Loading branch information
@vijtrip2
vijtrip2 authored Jun 8, 2022
1 parent de27382 commit 1163c8e
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 61 deletions.
10 changes: 5 additions & 5 deletions mocks/pkg/types/aws_resource_manager.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions pkg/runtime/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
93 changes: 48 additions & 45 deletions pkg/runtime/reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ func reconcilerMocks(
) (
acktypes.AWSResourceReconciler,
*ctrlrtclientmock.Client,
acktypes.ServiceControllerMetadata,
) {
zapOptions := ctrlrtzap.Options{
Development: true,
Expand All @@ -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(
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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)

Expand All @@ -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) {
Expand Down Expand Up @@ -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)

Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)

Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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)

Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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)
}
10 changes: 5 additions & 5 deletions pkg/runtime/tags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down
Loading

0 comments on commit 1163c8e

Please sign in to comment.