diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 0a17820..4eb59a7 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 0000000..f2a006c --- /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 bcdc0bd..c7c996d 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 d0c6c75..0e6f9a0 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)