Skip to content

Commit

Permalink
add new AWSResourceManager.EnsureControllerTags() method (#90)
Browse files Browse the repository at this point in the history
Issue #, if available: aws-controllers-k8s/community#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.
  • Loading branch information
vijtrip2 authored Jun 1, 2022
1 parent ae6e790 commit fb55374
Show file tree
Hide file tree
Showing 6 changed files with 256 additions and 0 deletions.
14 changes: 14 additions & 0 deletions mocks/pkg/types/aws_resource_manager.go

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

18 changes: 18 additions & 0 deletions pkg/runtime/reconciler.go
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.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)
Expand Down Expand Up @@ -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")
Expand Down
112 changes: 112 additions & 0 deletions pkg/runtime/reconciler_test.go
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("EnsureTags", 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("EnsureTags", 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, "EnsureTags", 2)
rm.AssertCalled(t, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 1)
rm.AssertCalled(t, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "EnsureTags", 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("EnsureTags", 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, "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)
}
49 changes: 49 additions & 0 deletions pkg/tags/tags.go
Original file line number Diff line number Diff line change
@@ -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
}
Loading

0 comments on commit fb55374

Please sign in to comment.