Skip to content

fix: ensure we mark resource as adopted adopt-or-create #186

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

Merged
merged 2 commits into from
Jun 20, 2025
Merged

fix: ensure we mark resource as adopted adopt-or-create #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29515da
fix: ensure we mark resource as adopted adopt-or-create
michaelhtm Jun 16, 2025
05721fb
test: ensure mark updated resource as adopted
michaelhtm Jun 17, 2025
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
17 changes: 10 additions & 7 deletions pkg/runtime/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,13 +730,13 @@ func (r *resourceReconciler) updateResource(
rm acktypes.AWSResourceManager,
desired acktypes.AWSResource,
latest acktypes.AWSResource,
) (acktypes.AWSResource, error) {
var err error
) (updated acktypes.AWSResource, err error) {
rlog := ackrtlog.FromContext(ctx)
exit := rlog.Trace("r.updateResource")
defer func() {
exit(err)
}()
updated = latest

// Ensure the resource is managed
if err = r.failOnResourceUnmanaged(ctx, latest); err != nil {
Expand All @@ -752,21 +752,24 @@ func (r *resourceReconciler) updateResource(
"diff", delta.Differences,
)
rlog.Enter("rm.Update")
latest, err = rm.Update(ctx, desired, latest, delta)
updated, err = rm.Update(ctx, desired, latest, delta)
rlog.Exit("rm.Update", err, "latest", latest)
if err != nil {
return latest, err
return updated, err
}
// Ensure that we are patching any changes to the annotations/metadata and
// the Spec that may have been set by the resource manager's successful
// Update call above.
latest, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, latest)
if IsAdopted(latest) {
r.rd.MarkAdopted(updated)
}
updated, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, updated)
if err != nil {
return latest, err
return updated, err
}
rlog.Info("updated resource")
}
return latest, nil
return updated, nil
}

// lateInitializeResource calls AWSResourceManager.LateInitialize() method and
Expand Down
73 changes: 57 additions & 16 deletions pkg/runtime/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,6 @@ func TestReconcilerAdoptOrCreateResource_Create(t *testing.T) {
).Once()
rm.On("IsSynced", ctx, latest).Return(true, nil)
rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
rd.On("MarkAdopted", latest).Return()

rm.On("LateInitialize", ctx, latest).Return(latest, nil)
rd.On("IsManaged", desired).Return(false).Once()
Expand All @@ -421,31 +420,59 @@ func TestReconcilerAdoptOrCreateResource_Create(t *testing.T) {

func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) {
require := require.New(t)
assert := assert.New(t)

ctx := context.TODO()
adoptionFieldsString := "{\"arn\": \"my-adopt-book-arn\"}"
adoptionFields := map[string]string{
"arn": "my-adopt-book-arn",
}

desired, _, metaObj := resourceMocks()
desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
metaObj.SetAnnotations(map[string]string{
ackv1alpha1.AnnotationAdoptionPolicy: "adopt-or-create",
ackv1alpha1.AnnotationAdoptionFields: "{\"arn\": \"my-adopt-book-arn\"}",
ackv1alpha1.AnnotationAdoptionFields: adoptionFieldsString,
})

ids := &ackmocks.AWSResourceIdentifiers{}
delta := ackcompare.NewDelta()
delta.Add("Spec.A", "val1", "val2")
delta.Add("Spec", "val1", "val2")

latest, latestRTObj, _ := resourceMocks()
latest, latestRTObj, latestMetaObj := resourceMocks()
latest.On("Identifiers").Return(ids)
latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
latest.On("MetaObject").Return(metav1.ObjectMeta{
latest.On(
"ReplaceConditions",
mock.AnythingOfType("[]*v1alpha1.Condition"),
).Return().Run(func(args mock.Arguments) {
conditions := args.Get(0).([]*ackv1alpha1.Condition)
hasSynced := false
for _, condition := range conditions {
if condition.Type != ackv1alpha1.ConditionTypeResourceSynced {
continue
}

hasSynced = true
assert.Equal(corev1.ConditionTrue, condition.Status)
assert.Equal(ackcondition.SyncedMessage, *condition.Message)
}
assert.True(hasSynced)
})
latestMetaObj.SetAnnotations(map[string]string{
ackv1alpha1.AnnotationAdoptionPolicy: "adopt-or-create",
ackv1alpha1.AnnotationAdoptionFields: adoptionFieldsString,
})
updated, updatedRTObj, _ := resourceMocks()
updated.On("Identifiers").Return(ids)
updated.On("Conditions").Return([]*ackv1alpha1.Condition{})
updated.On("MetaObject").Return(metav1.ObjectMeta{
Annotations: map[string]string{
ackv1alpha1.AnnotationAdoptionPolicy: "adopt-or-create",
ackv1alpha1.AnnotationAdoptionFields: "{\"arn\": \"my-adopt-book-arn\"}",
},
})
latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
latest.On(
updated.On(
"ReplaceConditions",
mock.AnythingOfType("[]*v1alpha1.Condition"),
).Return()
Expand All @@ -454,36 +481,50 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) {
rm.On("ResolveReferences", ctx, nil, desired).Return(
desired, false, nil,
).Times(2)
desired.On("PopulateResourceFromAnnotation", adoptionFields).Return(nil)
rm.On("ClearResolvedReferences", desired).Return(desired)
rm.On("ClearResolvedReferences", updated).Return(updated)
rm.On("ClearResolvedReferences", latest).Return(latest)
rm.On("ReadOne", ctx, desired).Return(
latest, nil,
).Once()
rm.On("Update", ctx, desired, latest, delta).Return(
latest, nil,
updated, nil,
).Once()
rm.On("IsSynced", ctx, latest).Return(true, nil)
rmf, rd := managedResourceManagerFactoryMocks(desired, latest)

rm.On("LateInitialize", ctx, latest).Return(latest, nil)
rm.On("LateInitialize", ctx, updated).Return(latest, nil)
rd.On("IsManaged", desired).Return(false).Once()
rd.On("IsManaged", desired).Return(true)
rd.On("Delta", desired, latest).Return(
delta,
).Once()
rd.On("Delta", desired, latest).Return(ackcompare.NewDelta())
rd.On("Delta", latest, latest).Return(ackcompare.NewDelta())
rd.On("MarkAdopted", latest).Return().Once()
latestMetaObj.SetAnnotations(map[string]string{
ackv1alpha1.AnnotationAdoptionPolicy: "adopt-or-create",
ackv1alpha1.AnnotationAdoptionFields: adoptionFieldsString,
ackv1alpha1.AnnotationAdopted: "true",
})
// setManaged
rd.On("Delta", latest, latest).Return(ackcompare.NewDelta()).Once()
// update
rd.On("Delta", desired, latest).Return(delta).Once()
//
rd.On("Delta", desired, updated).Return(ackcompare.NewDelta())
rd.On("Delta", updated, updated).Return(ackcompare.NewDelta())
rd.On("MarkAdopted", updated).Return().Once()

r, kc, scmd := reconcilerMocks(rmf)
rm.On("EnsureTags", ctx, desired, scmd).Return(nil)
statusWriter := &ctrlrtclientmock.SubResourceWriter{}
kc.On("Status").Return(statusWriter)
kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
statusWriter.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
kc.On("Patch", withoutCancelContextMatcher, updatedRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
statusWriter.On("Patch", withoutCancelContextMatcher, updatedRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
_, err := r.Sync(ctx, rm, desired)
require.Nil(err)
rm.AssertNumberOfCalls(t, "ReadOne", 1)
rm.AssertCalled(t, "Update", ctx, desired, latest, delta)
rd.AssertCalled(t, "Delta", desired, latest)
rd.AssertNumberOfCalls(t, "MarkAdopted", 2)
Copy link

@knottnt knottnt Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assert that the AwsResource has the expected adoption fields as well when it is patched? Checking that the function call happened twice is a good indication, but we could be more definitive.

Copy link
Member Author

@michaelhtm michaelhtm Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When MarkAdopted is called (an interface method) we're not really doing anything in these unit tests. We can only trust the actual implementation is handling the MarkAdopted is correct. The best way to actually test this is using e2e tests

Copy link

@knottnt knottnt Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see. That's due to the implementation being generated per service controller?

Copy link
Member Author

@michaelhtm michaelhtm Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

// Assert that the resource is not created or updated
rm.AssertNumberOfCalls(t, "Create", 0)
}
Expand Down Expand Up @@ -1249,7 +1290,7 @@ func TestReconcilerHandleReconcilerError_PatchStatus_Latest(t *testing.T) {

_, err := r.HandleReconcileError(ctx, desired, latest, nil)
require.Nil(err)
statusWriter.AssertCalled(t, "Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch"))
statusWriter.AssertCalled(t, "Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch"))
// The HandleReconcilerError function never updates spec or metadata, so
// even though there is a change to the annotations we expect no call to
// patch the spec/metadata...
Expand Down