Repository navigation
fix(s3): notifications cleanup fails when bucket is already deleted - #39552
abhiramshaji-cyber wants to merge 3 commits into
Conversation
During stack deletion CloudFormation can remove the bucket before the Custom::S3BucketNotifications resource runs its Delete. The handler then calls S3 on a bucket that no longer exists, gets NoSuchBucket, reports FAILED, and every retry fails the same way, so the stack can never be deleted. On Delete, a ClientError whose code is NoSuchBucket now reports SUCCESS, because there is nothing left to clean up. This covers managed and imported buckets, whether the error comes from the read or the write. Every other error, and NoSuchBucket on Create or Update, still fails. Closes aws#35352
The notifications handler is inlined into each template, so changing it changes the inline code and the template hash in every snapshot that uses it. Only those values change here, nothing else in the templates.
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Exemption Request: a bucket that is already gone during Delete cannot be set up in an integ test, and I cannot deploy to regenerate snapshots. The fix is covered by |
CONTRIBUTING.md requires integ snapshots to come from a real deployment, not from cdk-integ --dry-run or a manual update. These snapshots were built locally, so they are removed and left for a maintainer to regenerate with a deployed run.
Issue # (if applicable)
Closes #35352.
Deleting a stack whose bucket has event notifications could get stuck forever once the bucket was gone; now the cleanup step sees the bucket is gone and lets the deletion finish.
What this code is for
bucket.addEventNotification()adds aCustom::S3BucketNotificationsresource backed by a small Python Lambda,index.pyinnotifications-resource-handler. CloudFormation calls itshandler()on Create, Update and Delete. On Delete it writes an empty (or, for an imported bucket, an external only) configuration withput_bucket_notification_configuration, and for an imported bucket it first reads the current one withget_bucket_notification_configuration.Before this PR
A developer runs
cdk destroyon a stack with a bucket andbucket.addEventNotification(...).Deleterequest tohandler().handler()callsput_bucket_notification_configuration(orget_bucket_notification_configurationfirst, for an imported bucket).ClientErrorwhose code isNoSuchBucket.except Exceptionblock reportsFAILEDfor every error, including this one.DELETE_FAILED, and every retry repeats steps 2 to 5, so the stack can never be deleted.After this PR
Same developer, same
cdk destroy. Same as above through step 4.exceptblock now checks: is this aDeleterequest, and is the error aClientErrorwith codeNoSuchBucket?Bucket no longer exists, nothing to clean upand reportsSUCCESS.AccessDenied, or ifNoSuchBucketcomes back on aCreateorUpdate, the handler reportsFAILEDexactly as before.The diagram
flowchart LR classDef box fill:#eef1ff,stroke:#4c5bd4,color:#111827 classDef bad fill:#ffd6d6,stroke:#cc0000,color:#111827,stroke-width:3px classDef good fill:#c8f0c8,stroke:#008800,color:#111827,stroke-width:3px subgraph BEFORE["BEFORE: every delete and every retry fails"] direction TB B1["cdk destroy, bucket already gone"] --> B2["handler() gets RequestType Delete"] B2 --> B3["S3 returns ClientError NoSuchBucket"] B3 --> B4{"except Exception: any error?"} B4 -->|"NoSuchBucket counts as an error"| B5["FAILED, stack stuck in DELETE_FAILED"] B4 -->|"no error"| B6["SUCCESS"] end subgraph AFTER["AFTER: the delete finishes in one try"] direction TB A1["cdk destroy, bucket already gone"] --> A2["handler() gets RequestType Delete"] A2 --> A3["S3 returns ClientError NoSuchBucket"] A3 --> A4{"THE CHANGE: Delete and code is NoSuchBucket?"} A4 -->|"yes, nothing left to clean"| A5["SUCCESS, stack deletes"] A4 -->|"no, e.g. AccessDenied"| A6["FAILED, same as before"] end B1 ~~~ A1 class B1,B2,B3,B6 box class A1,A2,A3,A6 box class B4,B5 bad class A4,A5 good linkStyle 3 stroke:#cc0000,stroke-width:5px linkStyle 8 stroke:#008800,stroke-width:5px style BEFORE fill:#fff5f5,stroke:#cc0000,color:#111827 style AFTER fill:#f3fff3,stroke:#008800,color:#111827The change
The fix is in the
exceptblock ofhandler():is_missing_bucketchecks the structured error,isinstance(error, ClientError)anderror.response["Error"]["Code"] == "NoSuchBucket", not the message text. Because the check sits inhandler(), it covers both managed buckets and imported buckets (Managed: false), whether the error comes from the read or the write. All destination types (SNS, SQS and Lambda) go through this one handler, so all of them are covered.What changes for a user
cdk destroyon a stack with bucket notifications now completes when the bucket is already gone, for SNS, SQS and Lambda destinations alike.DELETE_FAILEDcannot receive the new handler through an update, so it still needsaws cloudformation delete-stack --retain-resourcesfor the notifications resource, or theFORCE_DELETE_STACKdeletion mode.AccessDenied, throttling, anything that is notNoSuchBucket.NoSuchBucketonCreateorUpdatestill fails, because there it means a real problem.Reason for this change
Thank you @aramfe for the clear report, and @pahud for confirming it and for describing the fix: let the Delete in
NotificationsResourceHandlersucceed onNoSuchBucket. @rittneje's point shaped this PR too: fixing the order between the two resources would not be enough, since CloudFormation still deletes the bucket when the custom resource fails, and the retry then fails forever. So the handler has to accept that the bucket is gone.I also read the two earlier attempts, #35388 and #35481, which went in the same direction. In this PR:
ClientErrorcode only, and only on Delete, so every other failure is still reported.On #35481, @TheRealAmazonKendra suggested fixing the dependency between the bucket and the notifications resource instead, and asked which destination types are affected. That resource already depends on the bucket, so CloudFormation normally deletes it first; the trouble starts when the bucket is gone anyway, and @rittneje's comment explains why ordering alone cannot help then. Every destination type (SNS, SQS, Lambda) shares this one handler, so they are all affected and all covered here. I would really value your view on whether this handler change is enough on its own, or whether you would like a dependency change as well.
Description of changes
See "The change" above. One new helper,
is_missing_bucket, and oneifin theexceptblock ofhandler(). No change to the TypeScript constructs.Describe any new or updated permissions being added
None.
Description of how you validated changes
MissingBucketTestintest_index.py. It covers managed and imported buckets,NoSuchBucketraised from the read or the write,AccessDeniedon Delete, andNoSuchBucketon Create and Update. The three Delete cases fail onmainand pass with this change. All 25 handler tests pass on Python 3.9 and 3.13.pr/needs-integration-tests-deploymentwhen you have a moment?pr-linter/exempt-integ-test? I am glad to add a test instead if you see a way to set it up.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license