fix: Remove Finalizer from resource if create returns error (#185)

Issue [#2406#issuecomment-2860631351](aws-controllers-k8s/community#2406 (comment))

Description of changes:
The reason marking a resource as managed (putting the finalizer)
before attempting a create is a general practice in kubernetes. The main reason we 
do it is to protect against deletion protection.
If we don't put the finalizer, there is no deletion protection against the resource.

The current adoption logic expects the resource to not be managed 
(no finalizer) to trigger an adoption.
If the initial creation attempt of a resource fails due to any AWS 
error, any subsequent reconciliations attempting to adopt an existing 
resource will not succeed.

These changes set the resource as unmanaged if for any reason there is
an error during the create call, which will allow the adoption logic to 
run in subsequent reconciliations.

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

@@ -636,6 +636,16 @@ func (r *resourceReconciler) createResource(
 	latest, err = rm.Create(ctx, desired)
 	rlog.Exit("rm.Create", err)
 	if err != nil {
+		// Here we're deciding to set a resource as unmanaged
+		// if the error is an AWS API Error. This will ensure
+		// that we're only managing (put finalizer) the resources
+		// that actually exist in AWS.
+		if _, ok := ackerr.AWSError(err); ok {
+			mErr := r.setResourceUnmanaged(ctx, rm, desired)
+			if mErr != nil {
+				return latest, err
+			}
+		}
 		return latest, err
 	}
 

pkg/runtime/reconciler_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/aws/smithy-go"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -224,6 +225,68 @@ func TestReconcilerCreate_BackoffRetries(t *testing.T) {
 	rm.AssertNumberOfCalls(t, "ReadOne", 6)
 }
 
+type awsError struct {
+	smithy.APIError
+}
+
+func (err awsError) Error() string {
+	return "mock error"
+}
+
+func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) {
+	require := require.New(t)
+
+	ctx := context.TODO()
+	arn := ackv1alpha1.AWSResourceName("mybook-arn")
+
+	desired, desiredRTObj, _ := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
+
+	ids := &ackmocks.AWSResourceIdentifiers{}
+	ids.On("ARN").Return(&arn)
+
+	latest, latestRTObj, _ := resourceMocks()
+	latest.On("Identifiers").Return(ids)
+
+	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
+
+	rm := &ackmocks.AWSResourceManager{}
+	rm.On("ResolveReferences", ctx, nil, desired).Return(
+		desired, false, nil,
+	).Times(2)
+	rm.On("ClearResolvedReferences", desired).Return(desired)
+	rm.On("ClearResolvedReferences", latest).Return(latest)
+	rm.On("ReadOne", ctx, desired).Return(
+		latest, ackerr.NotFound,
+	).Once()
+	rm.On("Create", ctx, desired).Return(
+		latest, awsError{},
+	)
+	rm.On("IsSynced", ctx, latest).Return(false, nil)
+	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
+	rd.On("IsManaged", desired).Return(false).Twice()
+	rd.On("IsManaged", desired).Return(true)
+	rd.On("MarkUnmanaged", desired)
+	rd.On("MarkManaged", desired)
+	rd.On("MarkUnmanaged", desired)
+	rd.On("ResourceFromRuntimeObject", desiredRTObj).Return(desired)
+	rd.On("Delta", desired, desired).Return(ackcompare.NewDelta())
+
+	r, kc, scmd := reconcilerMocks(rmf)
+	rm.On("EnsureTags", ctx, desired, scmd).Return(nil)
+	// Use specific matcher for WithoutCancel context instead of mock.Anything
+	kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
+	_, err := r.Sync(ctx, rm, desired)
+	require.NotNil(err)
+	rm.AssertNumberOfCalls(t, "ReadOne", 1)
+	rd.AssertCalled(t, "MarkUnmanaged", desired)
+	rd.AssertCalled(t, "MarkManaged", desired)
+}
+
 func TestReconcilerReadOnlyResource(t *testing.T) {
 	require := require.New(t)
 
@@ -1249,7 +1312,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...