From 444d1a8f58129df0bb8d35351b304f5eac304a17 Mon Sep 17 00:00:00 2001 From: Gustavo Diaz Date: Tue, 8 Sep 2026 17:29:02 +0000 Subject: [PATCH] fix: Keep finalizer on post-create failures A resource manager's Create may issue several AWS API calls: creating the resource, then applying tags, attributes or associated sub-resources. When a later call fails, the resource exists in AWS but Create still returns an error, and createResource cannot tell that apart from a create that never happened. It keys off the error type alone, so any AWS API error removes the finalizer. The next reconciliation then finds an existing resource with no finalizer and terminally conditions it as not managed by ACK. Reconciliation stops there and the resource is orphaned in AWS. Add PostCreateError, which a resource manager uses to signal that the resource was created before the error occurred, and honour it in createResource ahead of the existing AWS error check. WrapPostCreateError only wraps AWS API errors, since those are the only ones that unmanage a resource; everything else passes through so sentinels such as NotFound and the ackrequeue signals stay comparable by identity. Removing the finalizer on a plain AWS error is unchanged, which is what lets the adoption logic run on subsequent reconciliations (#185). Issue: https://github.com/aws-controllers-k8s/community/issues/2849 --- pkg/errors/error.go | 46 +++++++++++ pkg/errors/error_test.go | 145 +++++++++++++++++++++++++++++++++ pkg/runtime/reconciler.go | 10 +++ pkg/runtime/reconciler_test.go | 126 ++++++++++++++++++++++++++++ 4 files changed, 327 insertions(+) create mode 100644 pkg/errors/error_test.go diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 0a17820c..4eb59a7e 100644 --- a/pkg/errors/error.go +++ b/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) +} diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go new file mode 100644 index 00000000..f2a006cf --- /dev/null +++ b/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()) +} diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index bcdc0bdd..c7c996dc 100644 --- a/pkg/runtime/reconciler.go +++ b/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 diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index d0c6c75f..0e6f9a09 100644 --- a/pkg/runtime/reconciler_test.go +++ b/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)