Skip to content

Keep the ACK finalizer when a create partially succeeds - #268

Open
gustavodiaz7722 wants to merge 1 commit into
aws-controllers-k8s:mainfrom
gustavodiaz7722:fix/post-create-error-retains-finalizer
Open

Keep the ACK finalizer when a create partially succeeds#268
gustavodiaz7722 wants to merge 1 commit into
aws-controllers-k8s:mainfrom
gustavodiaz7722:fix/post-create-error-retains-finalizer

Conversation

@gustavodiaz7722

@gustavodiaz7722 gustavodiaz7722 commented Sep 8, 2026

Copy link
Copy Markdown
Member

Issue #2849

Description of changes:

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:

latest, err = rm.Create(ctx, desired)
if err != nil {
	if _, ok := ackerr.AWSError(err); ok {
		mErr := r.setResourceUnmanaged(ctx, rm, desired)

The next reconciliation then finds an existing resource with no finalizer and
failOnResourceUnmanaged terminally conditions it as not managed by ACK.
HandleReconcileError swallows ackerr.Terminal, so there is no requeue:
reconciliation stops and the resource is orphaned in AWS.

This is a recurring class rather than a single-service bug. #2670
(publicAccessBlock denied by an SCP) and #2730
(bucket policy fails) are the same root cause; both were closed by moving the
extra work out of the create path in s3-controller, leaving the runtime
heuristic in place. Scanning every generated sdkCreate across the controller
fleet, 41 of 277 resources can fail after a successful create.

These changes 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, returning anything else
unchanged. Those are the only errors that cause the reconciler to unmanage a
resource, so wrapping anything else would change no behaviour while hiding
sentinels such as ackerr.NotFound and the ackrequeue signals from the
identity comparisons callers still perform on them. Because the check unwraps,
an AWS error carried inside another error is still wrapped, which is intended:
such an error already triggers the unmanage path today.

Removing the finalizer on a plain AWS API error is unchanged, so the adoption
behaviour from #185 is preserved.

The companion code-generator change is
aws-controllers-k8s/code-generator#740, which makes generated
sdkCreate produce these errors. It depends on a runtime release containing
WrapPostCreateError, so this should merge and release first.

Testing

Unit tests cover the wrapper (AWS error wrapped, ackrequeue and sentinel
passthrough, requeue detection surviving a wrapped AWS error, Unwrap chain)
and the reconciler in both directions: the finalizer is retained on a
post-create error, and still removed on a plain AWS API error. I confirmed the
new reconciler test fails without the createResource change, reporting
MarkUnmanaged called.

Verified end to end on an EKS cluster with ec2-controller, whose SecurityGroup
sdkCreate calls RevokeSecurityGroupEgress after the create. The failure was
injected two ways: an IAM deny of ec2:RevokeSecurityGroupEgress (the reported
scenario) and an ingress rule referencing a non-existent security group, which
fails in syncSGRules with no IAM involved.

Before, both scenarios lost the finalizer and reported the terminal
NotManagedReason, and both security groups remained in AWS after the CRs were
deleted:

NAME                ID                     FINALIZERS   SYNCED   TERMINAL
ack2849-iamdeny     sg-0cb4905a1656b9076   <none>       False    True
ack2849-badrule     sg-0d61a483c57b312d2   <none>       False    True

After, the finalizer is retained and the condition is ACK.Recoverable
carrying the UnauthorizedOperation, which is the outcome the issue asks for:

NAME                    ID                     FINALIZERS                       SYNCED    TERMINAL   RECOVERABLE
ack2849-fixed-iamdeny   sg-07cd10539910eb4f4   [finalizers.ec2...SecurityGroup] Unknown   <none>     True
ack2849-fixed-badrule   sg-0be1974c9449d2d9b   [finalizers.ec2...SecurityGroup] Unknown   <none>     True

Reconciliation continued through the update path rather than dead-ending, no
duplicate resources were created, and once the deny was removed the resource
reached Synced=True on its own and the originally failed
RevokeSecurityGroupEgress completed. Deleting the CRs removed both security
groups from AWS, so nothing was orphaned.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

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 (aws-controllers-k8s#185).

Issue: aws-controllers-k8s/community#2849
@ack-prow
ack-prow Bot requested review from knottnt and michaelhtm September 8, 2026 18:36
@ack-prow

ack-prow Bot commented Sep 8, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gustavodiaz7722
Once this PR has been reviewed and has the lgtm label, please assign knottnt for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gustavodiaz7722

Copy link
Copy Markdown
Member Author

/retest

make test depends on the mocks target, so this failed in mock generation before any test ran:

building mocks for pkg/types ... 
internal error: package "k8s.io/apimachinery/pkg/apis/meta/v1" without types was imported from "github.com/aws-controllers-k8s/runtime/pkg/types"
make: *** [Makefile:24: mocks] Error 1

This looks unrelated to the change. pkg/types imports neither pkg/errors nor pkg/runtime, so nothing in this diff is in mockery's load graph for that target. Locally, with the same mockery v2.53.3 and golang.org/x/tools v0.30.0 the job installs, a clean run (bin/mockery removed so it rebuilds from source, make clean-mocks first) generates all six mock sets and the full go test ./... passes. The regenerated mocks match what is committed. ecr-controller-test, sqs-controller-test and verify-attribution also passed on this PR, and those compile the runtime as a dependency.

internal error: package ... without types is an intermittent go/packages loader failure, so retesting.

@gustavodiaz7722

Copy link
Copy Markdown
Member Author

Root-caused: unit-test is broken on main for every PR, not by this change.

scripts/install-mockery.sh builds mockery from source with the CI image's Go, and test-infra#1084 moved go_version from 1.26.5 to 1.27.1 on 2026-09-02. mockery v2.53.3 pins golang.org/x/tools v0.30.0, whose go/packages predates Go 1.27 and cannot type-check its standard library, which is exactly the without types loader error. The timeline fits: the last unit-test run before the image bump passed (#267, 2026-09-02T19:04Z) and runs after it fail.

Verified by building mockery from source the way the job does:

Go mockery make mocks
1.26.0 v2.53.3 passes
1.27.1 v2.53.3 fails with this error
1.27.1 v2.53.7 passes

I also reproduced the failure on unmodified main (0c9194d, this PR's base) under Go 1.27.1, so nothing in this diff is involved — pkg/types imports neither pkg/errors nor pkg/runtime, so this change is not even in mockery's load graph for the failing target.

Fix is in #269, where unit-test passes. This PR should go green on its next run once that merges, since presubmits test the PR merged with base — no rebase needed here. Happy to fold the bump into this PR instead if you would rather not carry two.

@gustavodiaz7722

Copy link
Copy Markdown
Member Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant