From 62595ef28e35494ffacc199b4c6c0ec537ae3e57 Mon Sep 17 00:00:00 2001
From: michaelhtm <98621731+michaelhtm@users.noreply.github.com>
Date: Fri, 28 Nov 2025 16:25:02 -0800
Subject: [PATCH] ensure we set resource conditions to terminal on adoption
 failure

---
 pkg/condition/condition.go | 15 +++++++++++++++
 pkg/runtime/reconciler.go  |  4 ++--
 pkg/runtime/util.go        | 24 +++++++++++-------------
 pkg/runtime/util_test.go   | 31 +++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go
index 8e9c449..7127b7f 100644
--- a/pkg/condition/condition.go
+++ b/pkg/condition/condition.go
@@ -324,6 +324,21 @@ func WithReferencesResolvedCondition(
 	return ko
 }
 
+// WithTerminalCondition returns a new AWSResource with the
+// ConditionTypeTerminal set based on the err parameter
+func WithTerminalCondition(
+	resource acktypes.AWSResource,
+	err error,
+) acktypes.AWSResource {
+	ko := resource.DeepCopy()
+
+	if err != nil {
+		errString := err.Error()
+		SetTerminal(ko, corev1.ConditionTrue, nil, &errString)
+	}
+	return ko
+}
+
 // LateInitializationInProgress return true if ConditionTypeLateInitialized has "False" status
 // False status means that resource has LateInitializationConfig but has not been completely
 // late initialized yet.
diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go
index 2abf836..dd26487 100644
--- a/pkg/runtime/reconciler.go
+++ b/pkg/runtime/reconciler.go
@@ -441,7 +441,7 @@ func (r *resourceReconciler) handlePopulation(
 		if adoptionPolicy == AdoptionPolicy_AdoptOrCreate {
 			return desired, nil
 		}
-		return desired, ackerr.NewTerminalError(err)
+		return ackcondition.WithTerminalCondition(desired, err), ackerr.Terminal
 	}
 
 	populated := desired.DeepCopy()
@@ -554,7 +554,7 @@ func (r *resourceReconciler) Sync(
 	if needAdoption {
 		populated, err := r.handlePopulation(ctx, desired)
 		if err != nil {
-			return nil, err
+			return populated, err
 		}
 		if adoptionPolicy == AdoptionPolicy_AdoptOrCreate {
 			// here we assume the spec fields are provided in the spec.
diff --git a/pkg/runtime/util.go b/pkg/runtime/util.go
index 81faca5..c8c0b1e 100644
--- a/pkg/runtime/util.go
+++ b/pkg/runtime/util.go
@@ -208,30 +208,28 @@ func NeedAdoption(res acktypes.AWSResource) bool {
 }
 
 func ExtractAdoptionFields(res acktypes.AWSResource) (map[string]string, error) {
-	fields := getAdoptionFields(res)
-
-	extractedFields := &map[string]string{}
-	err := json.Unmarshal([]byte(fields), extractedFields)
+	fields, ok := getAdoptionFields(res)
+	if !ok {
+		return nil, fmt.Errorf("%s annotation is not defined. Cannot extract resource identifiers", ackv1alpha1.AnnotationAdoptionFields)
+	}
+	extractedFields := map[string]string{}
+	err := json.Unmarshal([]byte(fields), &extractedFields)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("error parsing content of %s annotation: %w", ackv1alpha1.AnnotationAdoptionFields, err)
 	}
 
-	return *extractedFields, nil
+	return extractedFields, nil
 }
 
-func getAdoptionFields(res acktypes.AWSResource) string {
+func getAdoptionFields(res acktypes.AWSResource) (string, bool) {
 	mo := res.MetaObject()
 	if mo == nil {
 		// Should never happen... if it does, it's buggy code.
 		panic("ExtractRequiredFields received resource with nil RuntimeObject")
 	}
 
-	for k, v := range mo.GetAnnotations() {
-		if k == ackv1alpha1.AnnotationAdoptionFields {
-			return v
-		}
-	}
-	return ""
+	fields, ok := mo.GetAnnotations()[ackv1alpha1.AnnotationAdoptionFields]
+	return fields, ok
 }
 
 // patchObject performs a patch operation using context.WithoutCancel to prevent
diff --git a/pkg/runtime/util_test.go b/pkg/runtime/util_test.go
index 52e7875..bea44aa 100644
--- a/pkg/runtime/util_test.go
+++ b/pkg/runtime/util_test.go
@@ -14,6 +14,8 @@
 package runtime
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -118,3 +120,32 @@ func TestExtractAdoptionFields(t *testing.T) {
 	require.NoError(err)
 	require.Equal(expected, actual)
 }
+
+func TestExtractAdoptionFields_NoAdoptionFields(t *testing.T) {
+	require := require.New(t)
+
+	res := &mocks.AWSResource{}
+	res.On("MetaObject").Return(&metav1.ObjectMeta{
+		Annotations: map[string]string{},
+	})
+
+	_, err := ExtractAdoptionFields(res)
+	require.Error(err)
+	require.Equal(err, fmt.Errorf("%s annotation is not defined. Cannot extract resource identifiers", ackv1alpha1.AnnotationAdoptionFields))
+}
+
+func TestExtractAdoptionFields_InvalidAdoptionFields(t *testing.T) {
+	require := require.New(t)
+
+	res := &mocks.AWSResource{}
+	res.On("MetaObject").Return(&metav1.ObjectMeta{
+		Annotations: map[string]string{
+			ackv1alpha1.AnnotationAdoptionFields: "misconfigured",
+		},
+	})
+
+	_, err := ExtractAdoptionFields(res)
+	require.Error(err)
+	expectedErr := fmt.Sprintf("error parsing content of %s annotation", ackv1alpha1.AnnotationAdoptionFields)
+	require.True(strings.Contains(err.Error(), expectedErr))
+}