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

Issue [#2459#issuecomment-2879691276](aws-controllers-k8s/community#2459 (comment))

Description of changes:
Currently the `adopted` annotation was getting removed when the resource
goes through update path during adopt-or-create adoption. These changes
ensure the annotation is persisted in k8s.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: michaelhtm

pkg/runtime/reconciler.go

@@ -740,13 +740,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 {
@@ -762,21 +762,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

pkg/runtime/reconciler_test.go

@@ -459,7 +459,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()
@@ -484,31 +483,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()
@@ -517,36 +544,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)
 	// Assert that the resource is not created or updated
 	rm.AssertNumberOfCalls(t, "Create", 0)
 }