feat(logs): delete associated log group when stack is deleted #21113

YichenQian09 · 2022-07-12T22:13:56Z

Motivation: The log group created by LogRetention lingers after the stack is deleted. This commit allows users to choose whether to retain or destroy associated logs of the deleted stack.

Use case: To avoid having hundreds of dead log groups after the stack is deleted.

most salient design aspects:
Added an enum LogDeletionPolicy and set LogDeletionPolicy in the LogRetetion custom resource. Added an API deleteLogGroup and added an event 'Delete' to the handler of LogRetetion to delete the log group.

closes #11549

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-07-12T22:14:03Z

…he bug during building

packages/@aws-cdk/aws-logs/lib/log-retention-provider/index.ts

packages/@aws-cdk/aws-logs/lib/log-retention.ts

packages/@aws-cdk/aws-logs/lib/log-retention-provider/index.ts

packages/@aws-cdk/aws-logs/lib/log-retention.ts

Pull request has been modified.

packages/@aws-cdk/aws-logs/lib/log-retention.ts

packages/@aws-cdk/aws-logs/test/integ.log-retention.ts

TheRealAmazonKendra

Thanks for your responses. Please see my additional comments below.

TheRealAmazonKendra · 2022-07-22T03:44:01Z

I realized that I jumped right into this review without also saying thank you for your contribution! This feature will be super helpful so we appreciate you taking it on!

…into delete-logs-clean

Pull request has been modified.

…into delete-logs-clean

Pull request has been modified.

TheRealAmazonKendra

Just a few minor things, mostly formatting, but also the arn for long groups wasn't quite right. Neediness that, I think this looks ready to go.

packages/@aws-cdk/aws-logs/README.md

TheRealAmazonKendra · 2022-08-06T02:22:04Z

packages/@aws-cdk/aws-logs/lib/log-retention.ts

@@ -93,6 +99,8 @@ export class LogRetention extends Construct {
          base: retryOptions.base?.toMilliseconds(),
        } : undefined,
        RetentionInDays: props.retention === RetentionDays.INFINITE ? undefined : props.retention,
+        //only add RemovalPolicy if it is set to DESTROY
+        RemovalPolicy: props.removalPolicy && props.removalPolicy === cdk.RemovalPolicy.DESTROY ? props.removalPolicy : undefined,


There's no reason here to not just pass props.removalPolicy here.

Suggested change

RemovalPolicy: props.removalPolicy && props.removalPolicy === cdk.RemovalPolicy.DESTROY ? props.removalPolicy : undefined,

RemovalPolicy: props.removalPolicy,

When I wrote this line I thought only add RemovalPolicy if it is set to DESTROY(not add it if it is undefined or set RETAIN), so it is more reasonable to set it as it is?

You're later checking against RemovalPolicy.DESTROY, which kind of does make it so that you don't need RemovalPolicy.RETAIN but there's no reason to add this extra logic here. It's better to just pass the value as input by the user. You're not getting any benefit from the extra logic and it opens up the possibility of more failure points when more logic is introduced.

TheRealAmazonKendra · 2022-08-06T02:23:50Z

packages/@aws-cdk/aws-logs/lib/log-retention.ts

+        resources: [cdk.Stack.of(this).formatArn({
+          service: 'logs',
+          resource: 'log-group',
+          resourceName: props.logGroupName+':*',


Suggested change

resourceName: props.logGroupName+':*',

resourceName: `${props.logGroupName}:*`,

After I removed the ':*', I manually deploy and destroy the stack in integ.log-retention.ts. However, the destroy failed, the error message is: the loggroup is not allowed to delete. After I added the ':*' again, the destroy succeeded. I found the arn of log group is 'arn:aws:logs:us-east-1:account-id:log-group:logRetentionLogGroup:*' from the cloud watch

.

OK. I was incorrect here, but let's at least us better string formatting. See my edited suggestion.

packages/@aws-cdk/aws-logs/lib/log-retention.ts

Pull request has been modified.

TheRealAmazonKendra

@YichenQian09 Just a few more issues/updates from last review. This is very much almost ready to go.

packages/@aws-cdk/aws-logs/test/log-retention.test.ts

TheRealAmazonKendra · 2022-08-08T16:57:30Z

packages/@aws-cdk/aws-logs/lib/log-retention.ts

@@ -93,6 +99,8 @@ export class LogRetention extends Construct {
          base: retryOptions.base?.toMilliseconds(),
        } : undefined,
        RetentionInDays: props.retention === RetentionDays.INFINITE ? undefined : props.retention,
+        //only add RemovalPolicy if it is set to DESTROY
+        RemovalPolicy: props.removalPolicy && props.removalPolicy === cdk.RemovalPolicy.DESTROY ? props.removalPolicy : undefined,


You're later checking against RemovalPolicy.DESTROY, which kind of does make it so that you don't need RemovalPolicy.RETAIN but there's no reason to add this extra logic here. It's better to just pass the value as input by the user. You're not getting any benefit from the extra logic and it opens up the possibility of more failure points when more logic is introduced.

TheRealAmazonKendra · 2022-08-08T16:58:42Z

packages/@aws-cdk/aws-logs/lib/log-retention.ts

+        resources: [cdk.Stack.of(this).formatArn({
+          service: 'logs',
+          resource: 'log-group',
+          resourceName: props.logGroupName+':*',


OK. I was incorrect here, but let's at least us better string formatting. See my edited suggestion.

Pull request has been modified.

Co-authored-by: Kendra Neil <[email protected]>

TheRealAmazonKendra

Looks good! Thanks for all your work on this!

mergify · 2022-08-08T18:38:05Z

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

aws-cdk-automation · 2022-08-08T19:14:26Z

AWS CodeBuild CI Report

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

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

mergify · 2022-08-08T19:14:44Z

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

robertd · 2022-08-08T20:01:20Z

@YichenQian09 This is great PR... Now I just need to track down all the orphaned Log Groups pre-this-PR lol. Looking forward to the next cdk release. 👍

YichenQian09 · 2022-08-08T20:14:29Z

@TheRealAmazonKendra
Thanks for all your reviews, comments and patient help!

) ---- Motivation: The log group created by LogRetention lingers after the stack is deleted. This commit allows users to choose whether to retain or destroy associated logs of the deleted stack. Use case: To avoid having hundreds of dead log groups after the stack is deleted. most salient design aspects: Added an enum LogDeletionPolicy and set LogDeletionPolicy in the LogRetetion custom resource. Added an API deleteLogGroup and added an event 'Delete' to the handler of LogRetetion to delete the log group. closes [aws#11549](aws#11549) ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

tmokmss · 2022-08-09T04:53:09Z

Cool! Now I guess we need to add LogRemovalPolicy prop to Lambda function construct just like LogRetention.

) ---- Motivation: The log group created by LogRetention lingers after the stack is deleted. This commit allows users to choose whether to retain or destroy associated logs of the deleted stack. Use case: To avoid having hundreds of dead log groups after the stack is deleted. most salient design aspects: Added an enum LogDeletionPolicy and set LogDeletionPolicy in the LogRetetion custom resource. Added an API deleteLogGroup and added an event 'Delete' to the handler of LogRetetion to delete the log group. closes [aws#11549](aws#11549) ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

stec00 · 2022-09-11T10:38:55Z

Am I right in my understanding that this PR doesn't actually change anything that can be done in CDK as there isn't a property that can be set on a lambda to change the associated log group's removal policy? So it further requires #21804 to be done in order to achieve the statement of "This commit allows users to choose whether to retain or destroy associated logs of the deleted stack."?

YichenQian09 · 2022-09-25T01:17:17Z

Am I right in my understanding that this PR doesn't actually change anything that can be done in CDK as there isn't a property that can be set on a lambda to change the associated log group's removal policy? So it further requires #21804 to be done in order to achieve the statement of "This commit allows users to choose whether to retain or destroy associated logs of the deleted stack."?

This PR allows automatically deleting log groups created by logRetention by setting RemovalPolicy. The logGroup created by the lambda function will not be deleted. For example, if we create a logGroup named logRetentionLogGroup using logRetention and set removalPolicy to “destroy”, the second logGroup in the picture will be deleted, but the first will not. Please reply to this if I misunderstand your question or if you have further questions. Thanks!

stec00 · 2022-10-13T13:50:27Z

Thanks for your reply @YichenQian09. So as I now understand it, this PR will allow Log Groups that are explictly defined in the CDK script to be deleted as part of a stack delete (assuming removalPolicy="destroy"). But a Log Group that is implicitly created (e.g. due the presence of a Lambda function in the script) will not be deleted - hence why #21804 is still open. Hope that's right - thanks for clarifying!

add delete policy to manage logs created by logretention

720b9a6

aws-cdk-automation requested a review from a team July 12, 2022 22:14

github-actions bot added the p2 label Jul 12, 2022

YichenQian09 and others added 3 commits July 12, 2022 19:07

move the enum variable LogDeletionPolicy to log-retention.ts, solve t…

5a7dbe5

…he bug during building

Merge branch 'main' into delete-logs

fcf6988

Merge branch 'main' into delete-logs

9d93294

TheRealAmazonKendra previously requested changes Jul 20, 2022

View reviewed changes

add a integration test and resolve some conversations

21bad7e

Merge branch 'main' into delete-logs

b84c87e

TheRealAmazonKendra reviewed Jul 22, 2022

View reviewed changes

packages/@aws-cdk/aws-logs/lib/log-retention.ts Outdated Show resolved Hide resolved

TheRealAmazonKendra reviewed Jul 22, 2022

View reviewed changes

packages/@aws-cdk/aws-logs/test/integ.log-retention.ts Outdated Show resolved Hide resolved

TheRealAmazonKendra previously requested changes Jul 22, 2022

View reviewed changes

YichenQian09 added 3 commits July 22, 2022 18:27

run integration test

ec31cc8

add IntegTest

df3d211

Merge branch 'delete-logs' of https://github.com/YichenQian09/aws-cdk …

1a81cf7

…into delete-logs-clean

YichenQian09 added 5 commits July 23, 2022 14:47

integ.log-retention passed

8e23c53

Merge branch 'main' into delete-logs

ee29525

update readme.md

e38d647

Merge branch 'delete-logs' of https://github.com/YichenQian09/aws-cdk …

24c478a

…into delete-logs-clean

updated the unit test of log-retention

3421be2

YichenQian09 marked this pull request as ready for review July 23, 2022 20:47

YichenQian09 added 5 commits July 27, 2022 13:36

add new unit test for delete log group in log-retention-provider.test

a9dab41

Merge branch 'main' into delete-logs

8b5bcdc

fix the error of template

23a09e6

Merge branch 'delete-logs' of https://github.com/YichenQian09/aws-cdk …

9833f4d

…into delete-logs-clean

scope the role down to only allow delete specific log group

fe481ec

YichenQian09 added 2 commits August 5, 2022 13:44

changed the iam

e2e35bf

add additional unit tests

df84129

YichenQian09 added 3 commits August 5, 2022 15:49

update snapshot

52d5b8d

only add removalPolicy if it is set to destroy; update unit tests

6682f3c

update readme.md

989433a

TheRealAmazonKendra previously requested changes Aug 6, 2022

View reviewed changes

udpate logstream arn and readme

89a9286

TheRealAmazonKendra previously requested changes Aug 8, 2022

View reviewed changes

pass the value directly and format

209fb69

remove empty line

9343ae4

Co-authored-by: Kendra Neil <[email protected]>

TheRealAmazonKendra approved these changes Aug 8, 2022

View reviewed changes

Merge branch 'main' into delete-logs

aa30341

mergify bot merged commit 2bdd504 into aws:main Aug 8, 2022

tmokmss mentioned this pull request Aug 29, 2022

(lambda): Add property for log removal policy of Lambda function log groups #21804

Open

2 tasks

Jimlinz mentioned this pull request Nov 21, 2022

feat: Destroy aws lambda log group (non-prod) on teardown linz/geostore#2334

Closed

mrgrain mentioned this pull request Aug 1, 2023

feat(lambda): allow to delete log group of a lambda function when it is removed #21820

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logs): delete associated log group when stack is deleted #21113

feat(logs): delete associated log group when stack is deleted #21113

YichenQian09 commented Jul 12, 2022 •

edited

Loading

gitpod-io bot commented Jul 12, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra commented Jul 22, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Aug 6, 2022

YichenQian09 Aug 8, 2022

TheRealAmazonKendra Aug 8, 2022

TheRealAmazonKendra Aug 6, 2022 •

edited

Loading

YichenQian09 Aug 8, 2022 •

edited

Loading

TheRealAmazonKendra Aug 8, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Aug 8, 2022

TheRealAmazonKendra Aug 8, 2022

TheRealAmazonKendra left a comment

mergify bot commented Aug 8, 2022

aws-cdk-automation commented Aug 8, 2022

mergify bot commented Aug 8, 2022

robertd commented Aug 8, 2022

YichenQian09 commented Aug 8, 2022 •

edited

Loading

tmokmss commented Aug 9, 2022

stec00 commented Sep 11, 2022

YichenQian09 commented Sep 25, 2022

stec00 commented Oct 13, 2022

	RemovalPolicy: props.removalPolicy && props.removalPolicy === cdk.RemovalPolicy.DESTROY ? props.removalPolicy : undefined,
	RemovalPolicy: props.removalPolicy,

	resourceName: props.logGroupName+':*',
	resourceName: `${props.logGroupName}:*`,

feat(logs): delete associated log group when stack is deleted #21113

feat(logs): delete associated log group when stack is deleted #21113

Conversation

YichenQian09 commented Jul 12, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 12, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Jul 22, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 6, 2022

Choose a reason for hiding this comment

YichenQian09 Aug 8, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 8, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 6, 2022 • edited Loading

Choose a reason for hiding this comment

YichenQian09 Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 8, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 8, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 8, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 8, 2022

aws-cdk-automation commented Aug 8, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 8, 2022

robertd commented Aug 8, 2022

YichenQian09 commented Aug 8, 2022 • edited Loading

tmokmss commented Aug 9, 2022

stec00 commented Sep 11, 2022

YichenQian09 commented Sep 25, 2022

stec00 commented Oct 13, 2022

YichenQian09 commented Jul 12, 2022 •

edited

Loading

TheRealAmazonKendra Aug 6, 2022 •

edited

Loading

YichenQian09 Aug 8, 2022 •

edited

Loading

YichenQian09 commented Aug 8, 2022 •

edited

Loading