-
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
feat(synthetics): enable auto delete lambdas via custom resource #26580
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_LAMBDA
Which 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
Synthetics used to have a property
deleteLambdaResourceOnCanaryDeletion
that 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
deleteLambdaResourceOnCanaryDeletion
property (calledenableAutoDeleteLambdas
on the L2 Canary) by implementing a custom resource similar to what we have in S3 and ECR.This PR deprecates
enableAutoDeleteLambdas
in 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