fix: ensure resource status is patched before checking for updates on

Author: michaelhtm

pkg/runtime/reconciler.go

@@ -589,10 +589,9 @@ func (r *resourceReconciler) Sync(
 		}
 	} else if adoptionPolicy == AdoptionPolicy_Adopt {
 		rm.FilterSystemTags(latest)
-		if err = r.setResourceManaged(ctx, rm, latest); err != nil {
+		if err = r.setResourceManagedAndAdopted(ctx, rm, latest); err != nil {
 			return latest, err
 		}
-		r.rd.MarkAdopted(latest)
 		latest, err = r.patchResourceMetadataAndSpec(ctx, rm, desired, latest)
 		if err != nil {
 			return latest, err
@@ -609,12 +608,12 @@ func (r *resourceReconciler) Sync(
 		return latest, nil
 	} else {
 		if adoptionPolicy == AdoptionPolicy_AdoptOrCreate {
-			// set adopt-or-create resource as managed before attempting
-			// update
-			if err = r.setResourceManaged(ctx, rm, latest); err != nil {
+			// set adopt-or-create resource as managed
+			// and requeue to ensure status is patched
+			if err = r.setResourceManagedAndAdopted(ctx, rm, latest); err != nil {
 				return latest, err
 			}
-			r.rd.MarkAdopted(latest)
+			return latest, requeue.Needed(fmt.Errorf("requeuing to check for updates"))
 		}
 		if latest, err = r.updateResource(ctx, rm, resolved, latest); err != nil {
 			return latest, err
@@ -1173,6 +1172,34 @@ func (r *resourceReconciler) setResourceManaged(
 	rlog.Debug("marked resource as managed")
 	return nil
 }
+// setResourceManagedAndAdopted marks the underlying CR in the supplied AWSResource with
+// a finalizer and adopted annotation that indicates the object is under ACK management and will not
+// be deleted until that finalizer is removed (in setResourceUnmanaged())
+func (r *resourceReconciler) setResourceManagedAndAdopted(
+	ctx context.Context,
+	rm acktypes.AWSResourceManager,
+	res acktypes.AWSResource,
+) error {
+	if r.rd.IsManaged(res) {
+		return nil
+	}
+	var err error
+	rlog := ackrtlog.FromContext(ctx)
+	exit := rlog.Trace("r.setResourceManaged")
+	defer func() {
+		exit(err)
+	}()
+
+	orig := res.DeepCopy().RuntimeObject()
+	r.rd.MarkManaged(res)
+	r.rd.MarkAdopted(res)
+	res, err = r.patchResourceMetadataAndSpec(ctx, rm, r.rd.ResourceFromRuntimeObject(orig), res)
+	if err != nil {
+		return err
+	}
+	rlog.Debug("marked resource as managed")
+	return nil
+}
 
 // setResourceUnmanaged removes a finalizer from the underlying CR in the
 // supplied AWSResource that indicates the object is under ACK management. This

pkg/runtime/reconciler_test.go

@@ -532,38 +532,21 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) {
 		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\"}",
-		},
-	})
-	updated.On(
-		"ReplaceConditions",
-		mock.AnythingOfType("[]*v1alpha1.Condition"),
-	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	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("ClearResolvedReferences", latest).Return(latest)
 	rm.On("ReadOne", ctx, desired).Return(
 		latest, nil,
 	).Once()
-	rm.On("Update", ctx, desired, latest, delta).Return(
-		updated, nil,
-	).Once()
 	rm.On("IsSynced", ctx, latest).Return(true, nil)
 	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
 
-	rm.On("LateInitialize", ctx, updated).Return(latest, nil)
 	rd.On("IsManaged", desired).Return(false).Once()
 	rd.On("IsManaged", desired).Return(true)
 	rd.On("MarkAdopted", latest).Return().Once()
@@ -572,28 +555,16 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) {
 		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)
-	kc.On("Patch", withoutCancelContextMatcher, updatedRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
-	statusWriter.On("Patch", withoutCancelContextMatcher, updatedRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
+	statusWriter.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
 	_, err := r.Sync(ctx, rm, desired)
-	require.Nil(err)
+	require.NotNil(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)
 }