From fb553741e0bfbbd1fee8f52b2e36b8c58dd7bf8e Mon Sep 17 00:00:00 2001 From: Vijay Tripathi Date: Wed, 1 Jun 2022 12:01:11 -0700 Subject: [PATCH] add new AWSResourceManager.EnsureControllerTags() method (#90) Issue #, if available: https://github.com/aws-controllers-k8s/community/issues/1261 Description of changes: * Adds new `AWSResourceManager.EnsureTags()` method * `EnsureTags` method will be called before invoking `reconcile` method to update the desired resource with the ACK controller tags * This change also introduces a new `Tags` shape, which will be used as mediator/hub to merge AWS tags represented using different go types. * code-generator will generate the logic of conversion to/from `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 | 14 +++ pkg/runtime/reconciler.go | 18 ++++ pkg/runtime/reconciler_test.go | 112 ++++++++++++++++++++++++ pkg/tags/tags.go | 49 +++++++++++ pkg/tags/tags_test.go | 55 ++++++++++++ pkg/types/aws_resource_manager.go | 8 ++ 6 files changed, 256 insertions(+) create mode 100644 pkg/tags/tags.go create mode 100644 pkg/tags/tags_test.go diff --git a/mocks/pkg/types/aws_resource_manager.go b/mocks/pkg/types/aws_resource_manager.go index c0167e6..84b87d5 100644 --- a/mocks/pkg/types/aws_resource_manager.go +++ b/mocks/pkg/types/aws_resource_manager.go @@ -78,6 +78,20 @@ 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) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, types.AWSResource) error); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // IsSynced provides a mock function with given fields: _a0, _a1 func (_m *AWSResourceManager) IsSynced(_a0 context.Context, _a1 types.AWSResource) (bool, error) { ret := _m.Called(_a0, _a1) diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index b572ea4..a60f517 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -233,6 +233,13 @@ func (r *resourceReconciler) Sync( } desired = resolvedRefDesired + rlog.Enter("rm.EnsureTags") + err = rm.EnsureTags(ctx, desired) + rlog.Exit("rm.EnsureTags", err) + if err != nil { + return desired, err + } + rlog.Enter("rm.ReadOne") latest, err = rm.ReadOne(ctx, desired) rlog.Exit("rm.ReadOne", err) @@ -373,6 +380,17 @@ func (r *resourceReconciler) createResource( return resolvedRefDesired, err } desired = resolvedRefDesired + + // Ensure tags again after adding the finalizer and patching the + // resource. Patching desired resource omits the controller tags + // 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) + rlog.Exit("rm.EnsureTags", err) + if err != nil { + return desired, err + } } rlog.Enter("rm.Create") diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index d56ed48..261bf49 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -183,6 +183,7 @@ 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) kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil) @@ -240,6 +241,7 @@ 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) @@ -266,6 +268,8 @@ func TestReconcilerCreate_UnManagedResource_CheckReferencesResolveTwice(t *testi kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) + rm.AssertNumberOfCalls(t, "EnsureTags", 2) + rm.AssertCalled(t, "EnsureTags", ctx, desired) } func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing.T) { @@ -315,6 +319,7 @@ 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) @@ -341,6 +346,8 @@ func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing. kc.AssertNotCalled(t, "Status") rm.AssertCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "IsSynced", ctx, latest) + rm.AssertNumberOfCalls(t, "EnsureTags", 1) + rm.AssertCalled(t, "EnsureTags", ctx, desired) } func TestReconcilerUpdate(t *testing.T) { @@ -392,6 +399,7 @@ 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) @@ -419,6 +427,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) } func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) { @@ -474,6 +483,7 @@ 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) @@ -499,6 +509,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) } func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) { @@ -547,6 +558,7 @@ 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) @@ -573,6 +585,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) } func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) { @@ -621,6 +634,7 @@ 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) @@ -647,6 +661,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) } func TestReconcilerUpdate_IsSyncedError(t *testing.T) { @@ -706,6 +721,7 @@ 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) @@ -731,6 +747,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) } func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) { @@ -778,6 +795,7 @@ 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) @@ -795,6 +813,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) } func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) { @@ -853,6 +872,7 @@ 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) @@ -868,6 +888,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) } func TestReconcilerHandleReconcilerError_PatchStatus_Latest(t *testing.T) { @@ -998,6 +1019,7 @@ 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) @@ -1014,6 +1036,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) } func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) { @@ -1101,6 +1124,7 @@ 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) @@ -1114,6 +1138,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) } func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) { @@ -1176,6 +1201,7 @@ 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) @@ -1197,4 +1223,90 @@ 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) +} + +func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { + require := require.New(t) + + ctx := context.TODO() + arn := ackv1alpha1.AWSResourceName("mybook-arn") + + delta := ackcompare.NewDelta() + delta.Add("Spec.A", "val1", "val2") + + desired, _, _ := resourceMocks() + desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return() + + ids := &ackmocks.AWSResourceIdentifiers{} + ids.On("ARN").Return(&arn) + + latest, latestRTObj, _ := resourceMocks() + latest.On("Identifiers").Return(ids) + + ensureControllerTagsError := errors.New("failed to ensure controller tags") + + // resourceReconciler.ensureConditions will ensure that if the resource + // manager has not set any Conditions on the resource, that at least an + // ACK.ResourceSynced condition with status Unknown will be set on the + // resource. + latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) + latest.On( + "ReplaceConditions", + mock.AnythingOfType("[]*v1alpha1.Condition"), + ).Return().Run(func(args mock.Arguments) { + conditions := args.Get(0).([]*ackv1alpha1.Condition) + assert.Equal(t, 1, len(conditions)) + cond := conditions[0] + assert.Equal(t, ackv1alpha1.ConditionTypeResourceSynced, cond.Type) + // The non-terminal reconciler error causes the ResourceSynced + // condition to be False + assert.Equal(t, corev1.ConditionFalse, cond.Status) + assert.Equal(t, ackcondition.NotSyncedMessage, *cond.Message) + assert.Equal(t, ensureControllerTagsError.Error(), *cond.Reason) + }) + + rm := &ackmocks.AWSResourceManager{} + rm.On("ResolveReferences", ctx, nil, desired).Return(desired, nil) + rm.On("ReadOne", ctx, desired).Return( + latest, nil, + ) + rm.On("Update", ctx, desired, latest, delta).Return( + latest, nil, + ) + + rmf, rd := managedResourceManagerFactoryMocks(desired, latest) + rd.On("Delta", desired, latest).Return( + delta, + ).Once() + rd.On("Delta", desired, latest).Return(ackcompare.NewDelta()) + + 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( + 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 + // non-error return from `AWSResourceManager.ReadOne()` and the + // `AWSResourceDescriptor.Delta()` returned a non-empty Delta, that we end + // up calling the AWSResourceManager.Update() call in the Reconciler.Sync() + // method, + _, err := r.Sync(ctx, rm, desired) + require.NotNil(err) + rm.AssertCalled(t, "ResolveReferences", ctx, nil, desired) + rm.AssertNotCalled(t, "ReadOne", ctx, desired) + rd.AssertNotCalled(t, "Delta", desired, latest) + rm.AssertNotCalled(t, "Update", ctx, desired, latest, delta) + // No changes to metadata or spec so Patch on the object shouldn't be done + kc.AssertNotCalled(t, "Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")) + // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus + kc.AssertNotCalled(t, "Status") + rm.AssertNotCalled(t, "LateInitialize", ctx, latest) + rm.AssertCalled(t, "EnsureTags", ctx, desired) } diff --git a/pkg/tags/tags.go b/pkg/tags/tags.go new file mode 100644 index 0000000..6bcc0ff --- /dev/null +++ b/pkg/tags/tags.go @@ -0,0 +1,49 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file 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 tags + +// Tags represents the AWS tags which will be added to the AWS resource. +// Inside aws-sdk-go, Tags are represented using multiple types, Ex: map of +// string, list of structs etc... +// Tags type will be used as a hub/mediator to merge tags represented +// using different types. +type Tags map[string]string + +// NewTags returns Tags with empty tags +func NewTags() Tags { + return map[string]string{} +} + +// Merge merges two set of Tags and returns the merge result. +// In case of collision precedence is given to tags present in the first +// parameter 'a'. +func Merge(a Tags, b Tags) Tags { + var result Tags + // Initialize result with the first set of tags 'a'. + // If first set is nil, initialize result with empty set of tags. + if a == nil { + result = NewTags() + } else { + result = a + } + if b != nil && len(b) > 0 { + // Add all the tags which are not already present in result + for tk, tv := range b { + if _, found := result[tk]; !found { + result[tk] = tv + } + } + } + return result +} diff --git a/pkg/tags/tags_test.go b/pkg/tags/tags_test.go new file mode 100644 index 0000000..9c5e812 --- /dev/null +++ b/pkg/tags/tags_test.go @@ -0,0 +1,55 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file 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 tags_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + acktags "github.com/aws-controllers-k8s/runtime/pkg/tags" +) + +func TestNewTags(t *testing.T) { + assert := assert.New(t) + tags := acktags.NewTags() + assert.NotNil(tags) + assert.Empty(tags) +} + +func TestResourceTags_Merge_NilTags(t *testing.T) { + assert := assert.New(t) + + var t1, t2 acktags.Tags + res := acktags.Merge(t1, t2) + assert.NotNil(res) + assert.Empty(res) + + t2 = acktags.Tags{"tk": "tv", "tk2": "tv2"} + res = acktags.Merge(t1, t2) + assert.Equal("tv", res["tk"]) + assert.Equal("tv2", res["tk2"]) + assert.Equal(2, len(res)) +} + +func TestResourceTags_Merge(t *testing.T) { + assert := assert.New(t) + + t1 := acktags.Tags{"tk": "tv"} + t2 := acktags.Tags{"tk": "tv1", "tk2": "tv2"} + res := acktags.Merge(t1, t2) + assert.Equal("tv", res["tk"]) + assert.Equal("tv2", res["tk2"]) + assert.Equal(2, len(res)) +} diff --git a/pkg/types/aws_resource_manager.go b/pkg/types/aws_resource_manager.go index 8a8d022..a2788e7 100644 --- a/pkg/types/aws_resource_manager.go +++ b/pkg/types/aws_resource_manager.go @@ -85,6 +85,14 @@ type AWSResourceManager interface { ResolveReferences(context.Context, client.Reader, AWSResource) (AWSResource, error) // IsSynced returns true if a resource is synced. IsSynced(context.Context, AWSResource) (bool, error) + // EnsureTags ensures that tags are present inside the AWSResource. + // If the AWSResource does not have any existing resource tags, the 'tags' + // field is initialized and the controller tags are added. + // If the AWSResource has existing resource tags, then controller tags are + // 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 } // AWSResourceManagerFactory returns an AWSResourceManager that can be used to