-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(custom-resources): After failed update of AwsCustomResource, log stream ID is passed to onDelete, causing it to get stuck #30012
Comments
Thank you. We'll check this with the team. |
I will investigate this issue but I would appreciate if you have any working sample for me that I can simply run and verify in my account. I actually doubt that In the case where the When a CloudFormation stack update fails, CloudFormation will attempt to roll back the stack to the previous, working state. This means that any changes made by the failed Since the custom resource was not successfully updated, there is no need to delete the resource. The Can you tell me what made you believe that onDelete was invoked? Did you see any logs? And this was already in the code base in 2.137.0 so I don't think this is the root cause: Line 172 in bb90b4c
Please share some minimal required code snippets with me so I can reproduce it in my account. Thanks. |
By the way I would highly recommend not assigning physicalResourceId: cr.PhysicalResourceId.fromResponse('...'), Let's consider this scenario:
With that being said, I would suggest to make the physicalResourceId decoupled from the API response like physicalResourceId: cr.PhysicalResourceId.of('some-static-id'); I guess this would not trigger the unexpected behavior as I presume above. Let me know if it works for you. |
This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled. |
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Describe the bug
I am creating an AWS custom resource that requires replacement every time, meaning the onCreate & onUpdate both return a physicalResourceId and don't require one as input, however the onDelete does require one as input using
new cr.PhysicalResourceIdReference()
:When an update fails, the physical ID is changed to a log stream ID, eg.
2024/04/30/[$LATEST]xxxxxxxx
. CFN then tries to call onDelete with this log stream ID as the resource ID, which obviously fails, meaning onDelete gets stuck.Expected Behavior
When an update fails, the physical resource ID is not changed, and onDelete is called with the existing physical resource ID.
Current Behavior
When an update fails, the physical resource ID is changed to be the ID of a log stream, and onDelete is called with that log stream ID as the physical resource ID, causing it to fail.
Reproduction Steps
new cr.PhysicalResourceIdReference()
passed to onDelete.Possible Solution
The bug comes from this line in the SDK v2 handler:
aws-cdk/packages/@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/aws-sdk-v2-handler.ts
Line 175 in fd3a5e3
And this line in the SDK v3 handler:
aws-cdk/packages/@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/aws-sdk-v3-handler.ts
Line 171 in fd3a5e3
When sending a FAILED event,
context.logStreamName
is passed as the physical resource ID torespond()
, rather than passingphysicalResourceId
. I've been trying to work out the reasoning behind why it's like this and nothing has come to mind, I can't see any scenario where you want a failure log stream as the physical ID, so it could just be an accidental error.The fix would be to pass
physicalResourceId
as the 4th parameter torespond()
rather than passingcontext.logStreamName
.Additional Information/Context
No response
CDK CLI Version
2.115.0
Framework Version
No response
Node.js Version
20.11.1
OS
Windows 10 Enterprise
Language
TypeScript
Language Version
No response
Other information
No response
The text was updated successfully, but these errors were encountered: