Skip to content

Mark errors raised after a successful create - #740

Draft
gustavodiaz7722 wants to merge 1 commit into
aws-controllers-k8s:mainfrom
gustavodiaz7722:fix/wrap-post-create-errors
Draft

Mark errors raised after a successful create#740
gustavodiaz7722 wants to merge 1 commit into
aws-controllers-k8s:mainfrom
gustavodiaz7722:fix/wrap-post-create-errors

Conversation

@gustavodiaz7722

Copy link
Copy Markdown
Member

Issue #2849

Depends on aws-controllers-k8s/runtime#268. Do not merge before that is
released
— generated code calls ackerr.WrapPostCreateError, so regenerating
a controller against a runtime without it fails to compile:

pkg/resource/security_group/sdk.go:257:17: undefined: ackerr.WrapPostCreateError

Description of changes:

sdkCreate issues the create API call and then, for many resources, further
calls that apply tags, attributes or associated sub-resources. When one of those
fails the resource exists in AWS but sdkCreate still returns an error, and the
reconciler removes the ACK finalizer because it cannot tell that apart from a
create that never happened. The next reconciliation finds an existing resource
with no finalizer and terminally conditions it as not managed by ACK, orphaning
it. runtime#268 has the full analysis.

This defers a wrap once the create call has returned, so every error raised
below it is marked as a post-create failure:

rm.setStatusDefaults(ko)
defer func() {
	if err != nil {
		err = ackerr.WrapPostCreateError(err)
		if created == nil {
			created = &resource{ko}
		}
	}
}()

Three things about this shape are deliberate.

A defer rather than edits at each return. Scanning every generated
sdkCreate in the fleet, 41 of 277 resources can fail after a successful
create, and 28 of those return nil, err from inlined hook code. Because the
defer mutates the named return, it covers those returns without any hook
template being edited.

Placed after setStatusDefaults so ko is in scope. That matters for the
created backfill below. The region above it is output field mapping; every one
of the 41 real post-create failures sits at or after this point, including the
set_output_custom_method_name block and the sdk_create_post_set_output hook.

Backfilling created. When a hook returned nil, generated
manager.Create substitutes desired, so the identifiers from the create
response are never persisted. For a server-generated identifier the next
reconciliation reads NotFound and creates a second resource, leaking the
first. Returning ko prevents that.

WrapPostCreateError only wraps AWS API errors, so the ackerr.NotFound
guards and ackrequeue signals that hooks return here pass through unchanged.
Of the resources that return a requeue after a successful create, none carries
an AWS error inside it.

Not covered: custom_implementation on the create operation short-circuits at
the top of sdkCreate, before this defer is registered. Two resources use it —
elasticache/Snapshot (CustomCreateSnapshot) and apigatewayv2/API
(customCreateApi) — and need the wrap applied by hand in their controller
repos.

Testing

go build ./... and the full go test ./... suite pass.

Regenerating ec2-controller produces the defer in all 20 resources that have a
create operation, with no other change beyond the expected version.go and
ack-generate-metadata.yaml timestamps. In security_group/sdk.go it lands
ahead of both the requiredFieldsMissingForSGRule guard and
deleteDefaultSecurityGroupRule, which is the call that triggers #2849. The
regenerated controller compiles against runtime#268.

The regenerated controller was then deployed to an EKS cluster and the
post-create failure injected two ways (an IAM deny of
ec2:RevokeSecurityGroupEgress, and an ingress rule referencing a non-existent
security group). The finalizer was retained, the condition was
ACK.Recoverable rather than terminal, exactly one security group was created
per CR, the resource self-healed once the permission was restored, and deleting
the CRs removed the security groups from AWS. Full before/after in runtime#268.

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

sdkCreate issues the create API call and then, for many resources, further
calls that apply tags, attributes or associated sub-resources. When one of
those fails the resource exists in AWS but sdkCreate still returns an
error, and the reconciler removes the ACK finalizer because it cannot tell
that apart from a create that never happened. The next reconciliation finds
an existing resource with no finalizer and terminally conditions it as not
managed by ACK, orphaning it.

Defer a wrap once the create call has returned, so every error raised below
it is marked as a post-create failure and the reconciler keeps the
finalizer. Because the defer mutates the named return, it also covers the
returns inside inlined hook code, which no hook has to be edited for.

Place it after setStatusDefaults so ko is in scope, and backfill created
from ko when a hook returned nil. Without that the identifiers from the
create response are never persisted, and for a server-generated identifier
the next reconciliation reads NotFound and creates a second resource.

WrapPostCreateError only wraps AWS API errors, so ackerr.NotFound and the
ackrequeue signals that hooks return here pass through unchanged.

Requires a runtime release containing WrapPostCreateError.

Issue: aws-controllers-k8s/community#2849
@ack-prow
ack-prow Bot requested review from jlbutler and knottnt 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

@ack-prow

ack-prow Bot commented Sep 8, 2026

Copy link
Copy Markdown

@gustavodiaz7722: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
dynamodb-controller-test a15f9bb link true /test dynamodb-controller-test
eventbridge-controller-test a15f9bb link true /test eventbridge-controller-test
iam-controller-test a15f9bb link true /test iam-controller-test
eks-controller-test a15f9bb link true /test eks-controller-test
apigatewayv2-controller-test a15f9bb link true /test apigatewayv2-controller-test
efs-controller-test a15f9bb link true /test efs-controller-test
documentdb-controller-test a15f9bb link true /test documentdb-controller-test
ecr-controller-test a15f9bb link true /test ecr-controller-test
prometheusservice-controller-test a15f9bb link true /test prometheusservice-controller-test
pipes-controller-test a15f9bb link true /test pipes-controller-test
cloudfront-controller-test a15f9bb link true /test cloudfront-controller-test
s3-controller-test a15f9bb link true /test s3-controller-test
acm-controller-test a15f9bb link true /test acm-controller-test
ec2-controller-test a15f9bb link true /test ec2-controller-test
lambda-controller-test a15f9bb link true /test lambda-controller-test

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@gustavodiaz7722

Copy link
Copy Markdown
Member Author

The controller-test failures here are the runtime dependency, not a defect in the change. Converting to draft until aws-controllers-k8s/runtime#268 is released.

Every job that regenerates a controller and builds it fails, while the jobs that do not — unit-test, crd-compat-check, s3-olm-test, verify-attribution — pass. Generated sdkCreate now calls ackerr.WrapPostCreateError, which does not exist in the released runtime, so those builds fail with:

pkg/resource/security_group/sdk.go:257:17: undefined: ackerr.WrapPostCreateError

I could not pull the job log text to quote it directly, but I reproduced exactly this locally: a regenerated ec2-controller compiles with a go.mod replace pointing at runtime#268 and fails with the above without it.

Sequence to land this:

  1. Merge and release runtime#268 (it is inert on its own — no controller produces a PostCreateError until this change ships).
  2. Bump the runtime dependency in the controllers, or wait for the usual dependency bump.
  3. Un-draft this, at which point the controller-test jobs should pass.

Note runtime#268 is currently blocked by a separate, pre-existing CI break — mockery v2.53.3 cannot run under the Go 1.27.1 image — fixed in aws-controllers-k8s/runtime#269.

@gustavodiaz7722
gustavodiaz7722 marked this pull request as draft September 8, 2026 19:10
@ack-prow ack-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant