Keep the ACK finalizer when a create partially succeeds

pkg/errors/error.go

@@ -124,3 +124,49 @@ func (e TerminalError) Unwrap() error {
 }
 
 var _ error = &TerminalError{}
+
+// PostCreateError wraps an error that occurred after the backend AWS resource
+// was successfully created, when a later call in the same Create operation
+// failed. It tells the reconciler the resource exists in AWS, so the ACK
+// finalizer must be retained.
+type PostCreateError struct {
+	err error
+}
+
+func (e *PostCreateError) Error() string {
+	if e.err == nil {
+		return ""
+	}
+	return "post-create failure: " + e.err.Error()
+}
+
+func (e *PostCreateError) Unwrap() error {
+	return e.err
+}
+
+var _ error = &PostCreateError{}
+
+// WrapPostCreateError marks err as a post-create failure, returning it
+// unchanged if it is not an AWS API error.
+//
+// Only AWS API errors cause the reconciler to unmanage a resource, so wrapping
+// anything else would change no behaviour while hiding sentinels such as
+// NotFound and the ackrequeue signals from the identity comparisons callers
+// perform on them. The check unwraps, so an AWS error carried inside another
+// error is still wrapped -- it already triggers the unmanage path today.
+func WrapPostCreateError(err error) error {
+	if err == nil {
+		return nil
+	}
+	if _, ok := AWSError(err); !ok {
+		return err
+	}
+	return &PostCreateError{err: err}
+}
+
+// IsPostCreateError returns true if the backend AWS resource was created before
+// the supplied error occurred.
+func IsPostCreateError(err error) bool {
+	var postCreateErr *PostCreateError
+	return errors.As(err, &postCreateErr)
+}

pkg/errors/error_test.go

@@ -0,0 +1,145 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package errors_test
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/aws/smithy-go"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
+	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
+)
+
+// mockAWSError is a minimal smithy.APIError, as the AWS SDK surfaces service
+// errors.
+type mockAWSError struct {
+	code string
+}
+
+func (e *mockAWSError) Error() string {
+	return "api error " + e.code
+}
+func (e *mockAWSError) ErrorCode() string             { return e.code }
+func (e *mockAWSError) ErrorMessage() string          { return e.Error() }
+func (e *mockAWSError) ErrorFault() smithy.ErrorFault { return smithy.FaultClient }
+
+var _ smithy.APIError = &mockAWSError{}
+
+func TestWrapPostCreateError_WrapsAWSError(t *testing.T) {
+	require := require.New(t)
+	assert := assert.New(t)
+
+	awsErr := &mockAWSError{code: "UnauthorizedOperation"}
+
+	wrapped := ackerr.WrapPostCreateError(awsErr)
+
+	require.NotNil(wrapped)
+	assert.True(ackerr.IsPostCreateError(wrapped),
+		"an AWS API error must be marked as a post-create failure")
+
+	// The original must stay reachable so terminal-code classification and HTTP
+	// status introspection keep working through the wrapper.
+	var postCreateErr *ackerr.PostCreateError
+	require.True(errors.As(wrapped, &postCreateErr))
+	assert.Equal(awsErr, postCreateErr.Unwrap())
+
+	gotAWSErr, ok := ackerr.AWSError(wrapped)
+	require.True(ok, "the wrapped AWS error must still be detected by AWSError")
+	assert.Equal("UnauthorizedOperation", gotAWSErr.ErrorCode())
+
+	assert.Contains(wrapped.Error(), "UnauthorizedOperation")
+}
+
+func TestWrapPostCreateError_Nil(t *testing.T) {
+	assert.Nil(t, ackerr.WrapPostCreateError(nil))
+	assert.False(t, ackerr.IsPostCreateError(nil))
+}
+
+// TestWrapPostCreateError_PassesThroughNonAWSErrors is the guarantee that lets
+// the wrap be applied unconditionally to every error returned after a successful
+// create: errors that would never have unmanaged a resource are returned
+// untouched, so identity comparisons on them keep working.
+func TestWrapPostCreateError_PassesThroughNonAWSErrors(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+	}{
+		{"ackerr.NotFound", ackerr.NotFound},
+		{"ackerr.Terminal", ackerr.Terminal},
+		{"ackerr.SecretNotFound", ackerr.SecretNotFound},
+		{"ackerr.SecretTypeNotSupported", ackerr.SecretTypeNotSupported},
+		{"requeue.Needed", ackrequeue.Needed(fmt.Errorf("resource created, requeuing"))},
+		{"requeue.NeededAfter", ackrequeue.NeededAfter(
+			fmt.Errorf("requeuing for post-create updates"), time.Second)},
+		{"bare RequeueNeeded", &ackrequeue.RequeueNeeded{}},
+		{"plain error", fmt.Errorf("something went wrong")},
+		{"TerminalError", ackerr.NewTerminalError(fmt.Errorf("bad input"))},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ackerr.WrapPostCreateError(tt.err)
+			assert.Same(t, tt.err, got,
+				"a non-AWS error must be returned unchanged")
+			assert.False(t, ackerr.IsPostCreateError(got))
+		})
+	}
+}
+
+// TestWrapPostCreateError_SentinelIdentityPreserved is the concrete reason the
+// wrap is gated: the reconciler and generated controller code compare these
+// sentinels by identity, not with errors.Is.
+func TestWrapPostCreateError_SentinelIdentityPreserved(t *testing.T) {
+	assert := assert.New(t)
+
+	assert.True(ackerr.WrapPostCreateError(ackerr.NotFound) == ackerr.NotFound)
+	assert.True(ackerr.WrapPostCreateError(ackerr.Terminal) == ackerr.Terminal)
+}
+
+// TestWrapPostCreateError_RequeueSurvivesWrappedAWSError covers a requeue that
+// carries an AWS error. The wrap does fire here, so requeue detection has to
+// keep working through it.
+func TestWrapPostCreateError_RequeueSurvivesWrappedAWSError(t *testing.T) {
+	require := require.New(t)
+	assert := assert.New(t)
+
+	awsErr := &mockAWSError{code: "ThrottlingException"}
+	requeueErr := ackrequeue.NeededAfter(awsErr, 5*time.Second)
+
+	wrapped := ackerr.WrapPostCreateError(requeueErr)
+
+	require.True(ackerr.IsPostCreateError(wrapped))
+
+	// This is how the reconciler detects requeues in HandleReconcileError.
+	var requeueNeededAfter *ackrequeue.RequeueNeededAfter
+	require.True(errors.As(wrapped, &requeueNeededAfter),
+		"requeue detection must see through the post-create wrapper")
+	assert.Equal(5*time.Second, requeueNeededAfter.Duration())
+}
+
+func TestWrapPostCreateError_DoubleWrapIsDetectedOnce(t *testing.T) {
+	awsErr := &mockAWSError{code: "UnauthorizedOperation"}
+
+	wrapped := ackerr.WrapPostCreateError(ackerr.WrapPostCreateError(awsErr))
+
+	assert.True(t, ackerr.IsPostCreateError(wrapped))
+	gotAWSErr, ok := ackerr.AWSError(wrapped)
+	require.True(t, ok)
+	assert.Equal(t, "UnauthorizedOperation", gotAWSErr.ErrorCode())
+}

pkg/runtime/reconciler.go

@@ -860,6 +860,16 @@ func (r *resourceReconciler) createResource(
 	latest, err = rm.Create(ctx, desired)
 	rlog.Exit("rm.Create", err)
 	if err != nil {
+		// The resource was created before this error occurred, so keep the
+		// finalizer. Unmanaging here would orphan it: the next reconciliation
+		// finds an existing resource with no finalizer and terminally
+		// conditions it as not managed by ACK.
+		//
+		// Must precede the AWS error check below, which a PostCreateError also
+		// satisfies.
+		if ackerr.IsPostCreateError(err) {
+			return latest, err
+		}
 		// 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

pkg/runtime/reconciler_test.go

@@ -296,6 +296,132 @@ func TestReconcilerCreate_UnmanageResourceOnAWSErrors(t *testing.T) {
 	rd.AssertCalled(t, "MarkManaged", desired)
 }
 
+// TestReconcilerCreate_KeepManagedOnPostCreateError is the counterpart to
+// TestReconcilerCreate_UnmanageResourceOnAWSErrors: when the resource was
+// created before the error occurred, the finalizer must be retained even though
+// the error is an AWS API error.
+// See https://github.com/aws-controllers-k8s/community/issues/2849.
+func TestReconcilerCreate_KeepManagedOnPostCreateError(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()
+
+	// What a resource manager returns when a call after the create itself fails.
+	postCreateErr := ackerr.WrapPostCreateError(awsError{})
+	require.True(ackerr.IsPostCreateError(postCreateErr))
+
+	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, postCreateErr,
+	)
+	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("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)
+	rm.On("FilterSystemTags", mock.Anything, []string{})
+	kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
+
+	_, err := r.Sync(ctx, rm, desired)
+
+	// The error is still surfaced, so the reconciliation is requeued.
+	require.NotNil(err)
+	require.True(ackerr.IsPostCreateError(err))
+	rm.AssertNumberOfCalls(t, "ReadOne", 1)
+	rd.AssertCalled(t, "MarkManaged", desired)
+	rd.AssertNotCalled(t, "MarkUnmanaged", desired)
+}
+
+// TestReconcilerCreate_UnmanageOnAWSErrorWithoutPostCreateMarker asserts a plain
+// AWS API error from Create still unmanages the resource, which is what lets the
+// adoption logic run on subsequent reconciliations.
+// See https://github.com/aws-controllers-k8s/runtime/pull/185.
+func TestReconcilerCreate_UnmanageOnAWSErrorWithoutPostCreateMarker(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()
+
+	// A requeue carrying no AWS error is not wrapped, so it must not be mistaken
+	// for a post-create failure.
+	createErr := requeue.NeededAfter(
+		fmt.Errorf("resource already exists"), time.Second,
+	)
+	require.False(ackerr.IsPostCreateError(createErr))
+
+	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("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)
+	rm.On("FilterSystemTags", mock.Anything, []string{})
+	kc.On("Patch", withoutCancelContextMatcher, latestRTObj, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
+
+	_, err := r.Sync(ctx, rm, desired)
+	require.NotNil(err)
+	rd.AssertCalled(t, "MarkUnmanaged", desired)
+}
+
 func TestReconcilerReadOnlyResource(t *testing.T) {
 	require := require.New(t)