Skip to content
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

Merged
merged 29 commits into from
Aug 23, 2023
Merged

feat(synthetics): enable auto delete lambdas via custom resource #26580

merged 29 commits into from
Aug 23, 2023

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Jul 31, 2023

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 (called enableAutoDeleteLambdas on the L2 Canary) by implementing a custom resource similar to what we have in S3 and ECR.

This PR deprecates enableAutoDeleteLambdas in favor of cleanup: cleanup.LAMBDA, an enum that achieves the same thing but via custom resource

Closes #18448


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the p2 label Jul 31, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team July 31, 2023 20:39
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 31, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

Copy link
Contributor Author

@kaizencc kaizencc left a 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

@kaizencc kaizencc marked this pull request as draft July 31, 2023 20:42
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 and removed p2 labels Jul 31, 2023
@kaizencc kaizencc marked this pull request as ready for review August 2, 2023 15:00
@aws-cdk-automation aws-cdk-automation dismissed their stale review August 2, 2023 15:02

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 4, 2023

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.

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.

@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 4, 2023
Copy link
Contributor

@rix0rrr rix0rrr left a 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.

packages/@aws-cdk/aws-synthetics-alpha/README.md Outdated Show resolved Hide resolved
Comment on lines 132 to 133
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.
Copy link
Contributor

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 ?

packages/@aws-cdk/aws-synthetics-alpha/lib/canary.ts Outdated Show resolved Hide resolved
@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Aug 4, 2023
@kaizencc kaizencc requested a review from rix0rrr August 20, 2023 17:30
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 21, 2023
Copy link
Contributor

@rix0rrr rix0rrr left a 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;
Copy link
Contributor

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!)

@kaizencc kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Aug 22, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 3f582cf
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Aug 23, 2023

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).

@mergify mergify bot merged commit 6d1dc5b into aws:main Aug 23, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-synthetics): add cleanup lambda
3 participants