feat(s3): allow user to set Log Group on S3 Bucket autoDeleteObjects provider lambda #30394

samson-keung · 2024-05-31T00:31:26Z

Issue # (if applicable)

Reason for this change

To allow log group customization on the custom resource lambda created for the autoDeleteObjects feature.

Description of changes

At the highest level overview, a static method setAutoDeleteObjectsLogGroup is added to the Bucket class. When it is called, it will set the log group on the AutoDeleteObjectsProvider lambda (i.e. setting the LoggingConfig.LogGroup.

In order to support the above change, 2 underlying changes had to be made:

setAutoDeleteObjectsLogGroup(..) needs to have a way to find the singleton AutoDeleteObjectsProvider lambda. This means a method needs to be added in the AutoDeleteObjectsProvider class that returns the singleton. Note that the AutoDeleteObjectsProvider class itself is code generated. So I have modified the code gen logic to generate the getProvider(..) method, which returns the singleton.
With a handle of the singleton of type AutoDeleteObjectsProvider, which wraps the actual AWS::Lambda::Function, we need a way to set the log group on the lambda. With AutoDeleteObjectsProvider extending the CustomResourceProviderBase type, a method is added to CustomResourceProviderBase class to set the log group.

Description of how you validated changes

Updated the integ test and ran it against my own AWS account

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

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.

GavinZZ

Left some comments

GavinZZ · 2024-06-03T17:24:52Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

+    if (provider === undefined) {
+      throw new Error('Requires at least one bucket with \'autoDeleteObjects: true\'. None is found.');
+    }
+    provider.configureLambdaLogGroup(logGroup.logGroupName);


Would this code be problematic eventually? Consider the example where I have the following setups:

const bucket = new s3.Bucket(this, 'MyTempFileBucket', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, }); s3.Bucket.setAutoDeleteObjectsLogGroup(this, new logs.LogGroup(this, 'MyLogGroup', { logGroupName: 'MyLogGroup', retention: logs.RetentionDays.FIVE_YEARS })) const bucket2 = new s3.Bucket(this, 'MyTempFileBucket2', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, });

The first s3 bucket creation and setAutoDeleteObjectsLogGroup will work as expected. However, during the second bucket creation, it will use the existing singleton custom resource function (since it already exists) and a lambda log group is configured already.

I have updated the code to use a different implementation. But to answer your question:

The bucket2 will re-use to the same singleton custom resource function which has been configured to use MyLogGroup. This is expected.

GavinZZ · 2024-06-03T17:25:40Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

+  public static setAutoDeleteObjectsLogGroup(scope: Construct, logGroup: logs.ILogGroup): void {
+    const provider = AutoDeleteObjectsProvider.getProvider(scope, AUTO_DELETE_OBJECTS_RESOURCE_TYPE);
+
+    if (provider === undefined) {


This feels weird to me where I would expect users to do something

s3.Bucket.setAutoDeleteObjectsLogGroup(this, new logs.LogGroup(this, 'MyLogGroup', { logGroupName: 'MyLogGroup', retention: logs.RetentionDays.FIVE_YEARS })) const bucket = new s3.Bucket(this, 'MyTempFileBucket', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, });

Implementation is updated such that the order of the method calls no longer matters.

TheRealAmazonKendra

As discussed last week, I don't believe that this is the right place for this functionality to exist.

TheRealAmazonKendra · 2024-06-03T19:16:14Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

+   * @param scope the stack with bucket(s) with `autoDeleteObjects: true`.
+   * @param logGroup the log group to use on the lambda.
+   */
+  public static setAutoDeleteObjectsLogGroup(scope: Construct, logGroup: logs.ILogGroup): void {


I don't think this is the right place for this function. Given that the lambda is a singleton for the entire stack, such a function should be at the stack level. Having it exist on the Bucket class and taking in a scope that can be set at the construct level is misleading as to the actual scope of this function.

You could argue that you've explained that in the docstring, but I think this will still mislead customers.

Also, as mentioned last week, I think that a solution here needs to be implemented so that it is consistent across custom resources.

… to set LoggingConfig for the underlying lambda

…ruct

…eObjectsLogGroup method

…truct scope

GavinZZ · 2024-06-13T21:16:58Z

packages/@aws-cdk/cx-api/FEATURE_FLAGS.md

-    "@aws-cdk/aws-ec2:ebsDefaultGp3Volume": true
+    "@aws-cdk/aws-ec2:ebsDefaultGp3Volume": true,
+    "@aws-cdk/aws-ecs:removeDefaultDeploymentAlarm": true


Can you update from main to get rid of these changes (which I believe is not part of ur changes).

GavinZZ · 2024-06-13T21:23:13Z

packages/@aws-cdk/custom-resource-handlers/lib/custom-resources-framework/classes.ts

+        const useLogGroupMethod = this.addMethod({
+          name: 'useLogGroup',
+          static: true,
+          docs: {
+            summary: 'Set the log group to be used by the singleton provider',
+          },
+        });


Technically you're adding useLogGroup functionality to all custom resource handlers not only the s3 ones right? But the functionality is only exposed to the s3 auto delete handler. It would be nice to expose useLogGroup to other handlers as I believe this issue doesn't only happen on s3 auto delete but also on all custom resource handlers.

GavinZZ · 2024-06-17T17:22:51Z

Discussed with Samson on this issue. Since there are over 27 upvotes, will merge the fix for s3 custom resource handler first. There’s already a tracking issue for the underlying issue for all custom resources. Will handle that separately.

GavinZZ · 2024-06-17T17:23:24Z

@mergify update

mergify · 2024-06-17T17:23:38Z

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission

samson-keung · 2024-06-17T17:55:42Z

Discussed with Samson on this issue. Since there are over 27 upvotes, will merge the fix for s3 custom resource handler first. There’s already a tracking issue for the underlying issue for all custom resources. Will handle that separately.

Linking the issue tracking the ability to set log retention across all custom resource: #23909

aws-cdk-automation · 2024-06-17T17:56:58Z

AWS CodeBuild CI Report

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

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

GavinZZ · 2024-06-17T19:29:06Z

@mergify update

mergify · 2024-06-17T19:29:19Z

update

☑️ Nothing to do

#commits-behind>0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]
queue-position=-1 [📌 update requirement]

Dismissing to merge the fix in for s3. Have a separate issue to track all other custom resource handlers.

mergify · 2024-06-17T19:30:57Z

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

…eObjects provider lambda" (#30590) Reverts #30394

…provider lambda (aws#30394) ### Issue # (if applicable) Closes aws#24815. ### Reason for this change To allow log group customization on the custom resource lambda created for the `autoDeleteObjects` feature. ### Description of changes At the highest level overview, a static method `setAutoDeleteObjectsLogGroup` is added to the `Bucket` class. When it is called, it will set the log group on the `AutoDeleteObjectsProvider` lambda (i.e. setting the [`LoggingConfig.LogGroup`](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-loggingconfig.html#cfn-lambda-function-loggingconfig-loggroup). In order to support the above change, 2 underlying changes had to be made: 1. `setAutoDeleteObjectsLogGroup(..)` needs to have a way to find the singleton `AutoDeleteObjectsProvider` lambda. This means a method needs to be added in the `AutoDeleteObjectsProvider` class that returns the singleton. Note that the `AutoDeleteObjectsProvider` class itself is code generated. So I have modified the code gen logic to generate the `getProvider(..)` method, which returns the singleton. 2. With a handle of the singleton of type `AutoDeleteObjectsProvider`, which wraps the actual `AWS::Lambda::Function`, we need a way to set the log group on the lambda. With `AutoDeleteObjectsProvider` extending the `CustomResourceProviderBase` type, a method is added to `CustomResourceProviderBase` class to set the log group. ### Description of how you validated changes Updated the integ test and ran it against my own AWS account ### 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*

…eObjects provider lambda" (aws#30590) Reverts aws#30394

…provider lambda (aws#30394) ### Issue # (if applicable) Closes aws#24815. ### Reason for this change To allow log group customization on the custom resource lambda created for the `autoDeleteObjects` feature. ### Description of changes At the highest level overview, a static method `setAutoDeleteObjectsLogGroup` is added to the `Bucket` class. When it is called, it will set the log group on the `AutoDeleteObjectsProvider` lambda (i.e. setting the [`LoggingConfig.LogGroup`](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-loggingconfig.html#cfn-lambda-function-loggingconfig-loggroup). In order to support the above change, 2 underlying changes had to be made: 1. `setAutoDeleteObjectsLogGroup(..)` needs to have a way to find the singleton `AutoDeleteObjectsProvider` lambda. This means a method needs to be added in the `AutoDeleteObjectsProvider` class that returns the singleton. Note that the `AutoDeleteObjectsProvider` class itself is code generated. So I have modified the code gen logic to generate the `getProvider(..)` method, which returns the singleton. 2. With a handle of the singleton of type `AutoDeleteObjectsProvider`, which wraps the actual `AWS::Lambda::Function`, we need a way to set the log group on the lambda. With `AutoDeleteObjectsProvider` extending the `CustomResourceProviderBase` type, a method is added to `CustomResourceProviderBase` class to set the log group. ### Description of how you validated changes Updated the integ test and ran it against my own AWS account ### 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*

…eObjects provider lambda" (aws#30590) Reverts aws#30394

aws-cdk-automation · 2024-07-25T20:50:11Z

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels May 31, 2024

aws-cdk-automation requested a review from a team May 31, 2024 00:31

mergify bot added the contribution/core This is a PR that came from AWS. label May 31, 2024

aws-cdk-automation previously requested changes May 31, 2024

View reviewed changes

samson-keung changed the title ~~feat(s3): Allow user to set Log Group on S3 Bucket autoDeleteObjects provider lambda~~ feat(s3): allow user to set Log Group on S3 Bucket autoDeleteObjects provider lambda May 31, 2024

samson-keung marked this pull request as ready for review May 31, 2024 18:15

This was referenced Jun 1, 2024

Monthly PR metrics report #30414

Closed

Weekly PR metrics report #30434

Closed

GavinZZ requested changes Jun 3, 2024

View reviewed changes

TheRealAmazonKendra previously requested changes Jun 3, 2024

View reviewed changes

samson-keung force-pushed the bucket-auto-delete-objects-logging branch from e14888d to 43718d5 Compare June 11, 2024 00:25

samson-keung added 9 commits June 12, 2024 09:24

Create 'configureLambdaLogGroup' method in CustomResourceProviderBase…

c0b10d7

… to set LoggingConfig for the underlying lambda

Update provider code gen logic to create pure getProvider() method

3859bb1

Create new static method setAutoDeleteObjectsLogGroup in Bucket const…

f5dcbe7

…ruct

Update integ.bucket-auto-delete-objects to cover the new setAutoDelet…

3b2ef56

…eObjectsLogGroup method

Update S3 README

91c4710

Update provider code gen logic to create useLogGroup() method

21c771d

allow setAutoDeleteObjectsLogGroup to be called before bucket creation

fba53e8

update setAutoDeleteObjectsLogGroup to take a Stack instead of a Cons…

cebc4f3

…truct scope

resolve conflicts

90520ad

samson-keung force-pushed the bucket-auto-delete-objects-logging branch from 43718d5 to 90520ad Compare June 12, 2024 17:08

update unit test with latest changes from main

3b7e4de

GavinZZ reviewed Jun 13, 2024

View reviewed changes

samson-keung added 2 commits June 13, 2024 15:26

revert unintended change in cx-api

ca1acbe

update doc

5d12bf3

GavinZZ approved these changes Jun 17, 2024

View reviewed changes

Merge branch 'main' into bucket-auto-delete-objects-logging

fbde1ad

mergify bot merged commit 95938b2 into aws:main Jun 17, 2024
10 checks passed

TheRealAmazonKendra mentioned this pull request Jun 19, 2024

revert: "feat(s3): allow user to set Log Group on S3 Bucket autoDeleteObjects provider lambda" #30590

Merged

mergify bot pushed a commit that referenced this pull request Jun 19, 2024

revert: "feat(s3): allow user to set Log Group on S3 Bucket autoDelet…

e02578f

…eObjects provider lambda" (#30590) Reverts #30394

sarangarav pushed a commit to sarangarav/aws-cdk that referenced this pull request Jun 21, 2024

revert: "feat(s3): allow user to set Log Group on S3 Bucket autoDelet…

cc27aba

…eObjects provider lambda" (aws#30590) Reverts aws#30394

mazyu36 pushed a commit to mazyu36/aws-cdk that referenced this pull request Jun 22, 2024

revert: "feat(s3): allow user to set Log Group on S3 Bucket autoDelet…

0d6f897

…eObjects provider lambda" (aws#30590) Reverts aws#30394

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s3): allow user to set Log Group on S3 Bucket autoDeleteObjects provider lambda #30394

feat(s3): allow user to set Log Group on S3 Bucket autoDeleteObjects provider lambda #30394

samson-keung commented May 31, 2024

aws-cdk-automation left a comment

GavinZZ left a comment

GavinZZ Jun 3, 2024

samson-keung Jun 10, 2024

GavinZZ Jun 3, 2024

samson-keung Jun 10, 2024

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jun 3, 2024

TheRealAmazonKendra Jun 3, 2024

TheRealAmazonKendra Jun 3, 2024

GavinZZ Jun 13, 2024

GavinZZ Jun 13, 2024

GavinZZ commented Jun 17, 2024

GavinZZ commented Jun 17, 2024

mergify bot commented Jun 17, 2024

samson-keung commented Jun 17, 2024

aws-cdk-automation commented Jun 17, 2024

GavinZZ commented Jun 17, 2024

mergify bot commented Jun 17, 2024

mergify bot commented Jun 17, 2024

aws-cdk-automation commented Jul 25, 2024

feat(s3): allow user to set Log Group on S3 Bucket autoDeleteObjects provider lambda #30394

feat(s3): allow user to set Log Group on S3 Bucket autoDeleteObjects provider lambda #30394

Conversation

samson-keung commented May 31, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinZZ commented Jun 17, 2024

GavinZZ commented Jun 17, 2024

mergify bot commented Jun 17, 2024

❌ Mergify doesn't have permission to update

samson-keung commented Jun 17, 2024

aws-cdk-automation commented Jun 17, 2024

AWS CodeBuild CI Report

GavinZZ commented Jun 17, 2024

mergify bot commented Jun 17, 2024

☑️ Nothing to do

mergify bot commented Jun 17, 2024

aws-cdk-automation commented Jul 25, 2024