feat(synthetics): enable auto delete lambdas via custom resource#26580
feat(synthetics): enable auto delete lambdas via custom resource#26580mergify[bot] merged 29 commits intoaws:mainfrom kaizencc:conroy/synth-delete
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.
kaizencc
left a comment
There was a problem hiding this comment.
Struggling whether to call this a breaking change. The behavior should stay the same... the implementation will be wildly different. I'm tempted to call it a breaking change just to make sure people's attention are called to this PR.
Also: this PR is missing proper testing
…-cdk into conroy/synth-delete
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
I think the deprecation of one parameter in favor of another is the right implementation, and makes this not a breaking change. People will get deprecation warnings on using the old API, which will enough reason for them to move off of it. |
There was a problem hiding this comment.
Mostly a set of questions, biggest one being the rollbackability of these cleanup handlers. Consider the following scenario:
- UPDATE target resource (replacement, creates a new resource)
- UPDATE custom resource (old -> new)
- (...stuff happens...)
- ERROR, triggers a rollback
- UPDATE custom resource (new -> old)
- DELETE target resource (deletes the new resource, remembers the existing one)
Won't we have deleted the old Lambda at that point? Since you probably copy/pasted this, it might be that all cleanup handlers suffer from this problem, but let's make sure we fix it here and then also fix it for the other ones after.
| Setting `autoDeleteLambda: true` will take care of cleaning up Lambda functions on deletion, | ||
| but you still have to manually delete other resources like S3 buckets and CloudWatch logs. |
There was a problem hiding this comment.
Ufff. Is that a good experience? I don't care so much about the log group, but the S3 bucket seems like a pain?
Alternative solution:
cleanup: Cleanup.CLEANUP_LAMBDAWhich we can extend in the future with Cleanup.CLEANUP_ALL ?
...ws-cdk/custom-resource-handlers/lib/aws-synthetics-alpha/auto-delete-lambda-handler/index.ts
Outdated
Show resolved
Hide resolved
...ws-cdk/custom-resource-handlers/lib/aws-synthetics-alpha/auto-delete-lambda-handler/index.ts
Outdated
Show resolved
Hide resolved
rix0rrr
left a comment
There was a problem hiding this comment.
Conditionally shipped; I think the onUpdate handler can be simplified a bit more.
|
|
||
| async function onUpdate(event: AWSLambda.CloudFormationCustomResourceEvent) { | ||
| const updateEvent = event as AWSLambda.CloudFormationCustomResourceUpdateEvent; | ||
| const oldCanaryName = updateEvent.OldResourceProperties?.CanaryName; |
There was a problem hiding this comment.
This might be correct, but it feels more natural to me to get this from PhysicalResourceId.
And then the rest of this method is also quite trivial, because you can always just return newCanaryName (if it's the same, that's fine!)
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). |
…32738) ### Issue # (if applicable) None ### Reason for this change For the Lambda and Layer used in Canary, the deletion of related resources is [handled by a custom resource](#26580), but this functionality is now supported natively by CloudFormation. https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-resource-synthetics-canary.html#cfn-synthetics-canary-provisionedresourcecleanup ### Description of changes - Add `provisionedResourceCleanup` prop to `CanaryProps` - deprecate `cleanup` prop which uses custom resource ### Describe any new or updated permissions being added None ### Description of how you validated changes Add both unit and integ test ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Synthetics used to have a property
deleteLambdaResourceOnCanaryDeletionthat has since been deprecated and erased from cloudformation docs. Although this property still works today synthetics makes no promises that this is supported in the future.Here in CDK land, this PR serves as a replacement to the
deleteLambdaResourceOnCanaryDeletionproperty (calledenableAutoDeleteLambdason the L2 Canary) by implementing a custom resource similar to what we have in S3 and ECR.This PR deprecates
enableAutoDeleteLambdasin favor ofcleanup: cleanup.LAMBDA, an enum that achieves the same thing but via custom resourceCloses #18448
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license