Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add new AWSResourceManager.EnsureControllerTags() method #90

Merged
merged 5 commits into from
Jun 1, 2022
Merged

add new AWSResourceManager.EnsureControllerTags() method #90

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b0c273b
introduce new AWSResourceManager.EnsureControllerTags() method
vijtrip2 May 25, 2022
aabcc3e
add new ResourceTags wrapper for merging various tags
vijtrip2 May 25, 2022
c87709c
EnsureControllerTags after adding the finalizer
vijtrip2 May 26, 2022
774b6e9
refactor ResourceTags -> Tags
vijtrip2 May 27, 2022
7b308d8
Refactor rm.EnsureControllerTags -> rm.EnsureTags
vijtrip2 Jun 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions mocks/pkg/types/aws_resource_manager.go
View file
Open in desktop

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

16 changes: 16 additions & 0 deletions pkg/runtime/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,13 @@ func (r *resourceReconciler) Sync(
}
desired = resolvedRefDesired

rlog.Enter("rm.EnsureControllerTags")
err = rm.EnsureControllerTags(ctx, desired)
rlog.Exit("rm.EnsureControllerTags", err)
if err != nil {
return desired, err
}

rlog.Enter("rm.ReadOne")
latest, err = rm.ReadOne(ctx, desired)
rlog.Exit("rm.ReadOne", err)
Expand Down Expand Up @@ -373,6 +380,15 @@ func (r *resourceReconciler) createResource(
return resolvedRefDesired, err
}
desired = resolvedRefDesired

// Add controller tags again after adding the finalizer and patching
// the resource
rlog.Enter("rm.EnsureControllerTags")
err = rm.EnsureControllerTags(ctx, desired)
rlog.Exit("rm.EnsureControllerTags", err)
if err != nil {
return desired, err
}
vijtrip2 marked this conversation as resolved.
Show resolved Hide resolved
}

rlog.Enter("rm.Create")
Expand Down
112 changes: 112 additions & 0 deletions pkg/runtime/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)
kc.On("Patch", ctx, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", 2)
rm.AssertCalled(t, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerCreate_ManagedResource_CheckReferencesResolveOnce(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", 1)
rm.AssertCalled(t, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand Down Expand Up @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate_ResourceNotSynced(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate_NoDelta_ResourceNotSynced(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate_NoDelta_ResourceSynced(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate_IsSyncedError(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerHandleReconcilerError_PatchStatus_Latest(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
Expand Down Expand Up @@ -1101,6 +1124,7 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
latest, nil,
)
rm.On("IsSynced", ctx, latest).Return(true, nil)
rm.On("EnsureControllerTags", ctx, desired).Return(nil)

rmf, rd := managerFactoryMocks(desired, latest, false)

Expand All @@ -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, "EnsureControllerTags", ctx, desired)
}

func TestReconcilerUpdate_ResolveReferencesError(t *testing.T) {
Expand Down Expand Up @@ -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("EnsureControllerTags", ctx, desired).Return(nil)

r, kc := reconcilerMocks(rmf)

Expand All @@ -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, "EnsureControllerTags", 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("EnsureControllerTags", 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, "EnsureControllerTags", ctx, desired)
}
52 changes: 52 additions & 0 deletions pkg/tags/resource_tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// 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

// ResourceTags 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...
// ResourceTags type will be used as a hub/mediator to merge tags represented
// using different types.
type ResourceTags struct {
vijtrip2 marked this conversation as resolved.
Show resolved Hide resolved
Tags map[string]string
}
vijtrip2 marked this conversation as resolved.
Show resolved Hide resolved

// NewResourceTags returns ResourceTags with empty tags
func NewResourceTags() ResourceTags {
return ResourceTags{make(map[string]string)}
}

// NewResourceTagsFrom creates ResourceTags using the tags passed
// inside the parameter
func NewResourceTagsFrom(tags map[string]string) ResourceTags {
return ResourceTags{tags}
}

// Merge merges the tags between two ResourceTags.
// If a tag-key is already present inside the original ResourceTag('rt'), it
// does not get overwritten.
func (rt *ResourceTags) Merge(other ResourceTags) {
if other.Tags != nil && len(other.Tags) > 0 {
// Initialize if the Tags field is nil
if rt.Tags == nil {
rt.Tags = make(map[string]string)
}
// Add all the tags which are not already present
for tk, tv := range other.Tags {
if _, found := rt.Tags[tk]; !found {
rt.Tags[tk] = tv
}
}
}
}
Loading