Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. 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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
2acaa3f to
16c524b
Compare
…cloudformation rollback
16c524b to
edc1af6
Compare
| from the event's PhysicalResourceId will trigger a `Delete` event for the custom | ||
| resource. The `Delete` event will trigger `onDelete` function which will | ||
| empty the content of the repository and then proceed to delete the repository. */ | ||
| return { PhysicalResourceId: newRepositoryName }; |
There was a problem hiding this comment.
What was the original return value from onDelete() and how was it used after this function call? Or does the return value not matter and it was just to perform the deletion as part of thie onUpdate()?
Also where is the logic that checks if the repository name has changed or not and triggers the deletion of the bucket at the end of the deployment?
There was a problem hiding this comment.
onDelete doesn't return any value so it doesn't matter. We used to directly call the delete methods defined in onDelete not not using the default behaviour CFN provided (which is to do the deletion for us if the id changes). We didn't need to return anything before since we're explicitly calling the delete. Now we want to leverage the default behaviour of CFN custom resource so we returns the new physical id.
That logic is default with in CFN custom resource. The value returned for a PhysicalResourceId can change custom resource update operations. If the value returned is the same, it is considered a normal update. If the value returned is different, AWS CloudFormation recognizes the update as a replacement and sends a delete request to the old resource. . https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/crpg-ref-requesttypes-delete.html
|
@Mergifyio update |
☑️ Nothing to doDetails
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Issue # (if applicable)
Closes #27199
Reason for this change
Based on the way the custom resource is implemented, it is likely that unexpected behavior happens on Cloudformation rollback, i.e. the custom resource will prematurely delete the objects.
Consider the following scenario:
We will have deleted objects in the bucket that has been rolled back to in this scenario, but the content is now gone.
Description of changes
Instead of deleting it right during update, we send back
PhysicalResourceIdin the event handler which if the id changes, it will let CFN to empty and delete the bucket at the end of the deployment.Description of how you validated changes
New & updated tests. Also manually tested with deploying a template
Once the deployment is successful, add some random content to the bucket, then update the code so that the first bucket's bucketName is updated to another valid name. Update the second bucket's bucketName to be an existing bucket name, which will trigger a deployment failure hence roll back.
After the change, the content will stay there if a deployment failure happens. The content & bucket will be deleted if deployment is successful.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license