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

(aws-s3): retention on S3 autoDeleteObjects lambda CloudWatch log group is Never expire #24815

Closed
ozeebee opened this issue Mar 28, 2023 · 13 comments · Fixed by #30394
Closed
Assignees
Labels
@aws-cdk/aws-s3 Related to Amazon S3 effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@ozeebee
Copy link

ozeebee commented Mar 28, 2023

Describe the bug

Currently, when setting autoDeleteObjects on a S3 bucket to true, the lambda creates a log group whose retention is set to 'Never expire'.
When deploying/destroying a lot of stacks (typically during development), this generates a lot of log groups (with pattern /aws/lambda/$StackName-CustomS3AutoDeleteObjects-$someid) that will stay there forever unless manually cleaned up.

Expected Behavior

We should be able to override the default log group retention or have a reasonable log retetion period for those logs (for instance 3 months).

Current Behavior

The log group is kept forever, retention is 'Never expire'.

Reproduction Steps

Create a bucket with autoDeleteObjects such as:

const bucket = new s3.Bucket(this, 'MyTempFileBucket', {
  removalPolicy: cdk.RemovalPolicy.DESTROY,
  autoDeleteObjects: true,
});

Then deploy/destroy the stack mulitple times, this will generate a different log group for each cycle, and all of them will be on CW logs forever unitl manually deleted.

Possible Solution

Either expose a log retention period for the lambda, for instance:

const bucket = new s3.Bucket(this, 'MyTempFileBucket', {
  removalPolicy: cdk.RemovalPolicy.DESTROY,
  autoDeleteObjects: true,
  autoDeleteObjectsLambdaLogRetention: logs.RetentionDays.THREE_MONTHS
});

or set a default log retention period other than 'Never expire' (3 months seems reasonnable).

Additional Information/Context

No response

CDK CLI Version

2.69.0 (build 60a5b2a)

Framework Version

No response

Node.js Version

v18.12.0

OS

macOS 12.6

Language

Typescript

Language Version

4.9.5

Other information

No response

@ozeebee ozeebee added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 28, 2023
@github-actions github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Mar 28, 2023
@peterwoodworth peterwoodworth added p2 feature-request A feature should be added or improved. effort/medium Medium work item – several days of effort and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 28, 2023
@peterwoodworth
Copy link
Contributor

This makes sense, thanks for the request.

We take contributions 🙂 We probably won't be able to get to this for a long time if this issue doesn't get enough 👍🏻 support

@FelixFeliciant
Copy link

Hello, we are facing the same issue, any workaround for this?
ideally I would prefer that such log group would not be created at all if possible, but setting it for 1 day is good as well.

Thank you.

@kishiel
Copy link
Contributor

kishiel commented Jan 24, 2024

I spent some time trying to solve this myself, and I think I've chased it to the bottom. In order for this to work as currently implemented we'll need:

  1. A new interface property autoDeleteObjectsLambdaLogRetention in bucket.ts that accepts aws-logs/RetentionDays.
  2. Add a new property logRetention?: RetentionDays to CustomResourceProviderOptions in shared.ts
  3. Add a new dimension LoggingConfig to the properties section of the CfnResource definition in custom-resource-provider-base
  4. extend the functionality of the existing AWS::Lambda::Function LoggingConfig to incorporate the Log Duration as a dimension.

Related: aws-cloudformation/cloudformation-coverage-roadmap#147

There's a lot more complexity in this than I expected, to be honest. This is hard because we're not leveraging native CDK constructs to build the resource, but rather we're leveraging Cloudformation Templates.

I suspect we could get around this by implementing the autoDeleteObjects handler as a custom-resource. This seems like a better long-term solution, but I know it will present some headaches in the near-term.

Edit: #28737 was merged into the aws-cdk-lib release one hour after this post. This deprecates logRetention in favor of logGroup, which doesn't really change this post, but it will make the interface a bit weirder.

@github-actions github-actions bot added p1 and removed p2 labels Jan 28, 2024
Copy link

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

@JimHBeam
Copy link

JimHBeam commented Feb 8, 2024

also having problems with this, just realised our log group count got over 10000 in our dev env due to this, would like a way to set the log retention

@mpiltz
Copy link

mpiltz commented Mar 20, 2024

Also it would be really nice to have control over the log group itself. Since we are now able to set the LogGroup for a lambda function, custom_resource.Provider and AwsCustomResource explicitly, could we also have the same ability in this case.

@kishiel
Copy link
Contributor

kishiel commented Mar 26, 2024

I've got a working prototype of a solution for this problem following the suggestion by @mpiltz. This solution creates the log group and then feeds it into the lambda by name.

  private enableAutoDeleteObjects(retention?: RetentionDays, removalPolicy?: RemovalPolicy) {

    const logGroup = new LogGroup(this, 'auto-deletion-log-group', {
      retention: retention ?? RetentionDays.THREE_MONTHS,
      removalPolicy: removalPolicy ?? RemovalPolicy.DESTROY,
    });

    const provider = AutoDeleteObjectsProvider.getOrCreateProvider(this, AUTO_DELETE_OBJECTS_RESOURCE_TYPE, {
      useCfnResponseWrapper: false,
      description: `Lambda function for auto-deleting objects in ${this.bucketName} S3 bucket.`,
      logGroupName: logGroup.logGroupName,
    });

This requires a new optional parameter:

export interface CustomResourceProviderOptions {
  [...]

  /**
   * The Cloudwatch Log Group name to use for the provider.
   * @default - A log group name is automatically generated
   */
  readonly logGroupName?: string;
}

An unexpected behavior was that the provider lambda was re-creating the log group after I got it to successfully delete itself on stack deletion, but the new log group came back with infinite duration (argh!). This is because the default role for the lambda has permission to create a log group (which was occurring when it issued a final log stream on delete). Fortunately policyStatements was already wired up so I just explicitly denied CreateLogGroup for the Lambda role.

  public constructor(scope: Construct, id: string, props?: CustomResourceProviderOptions) {
    super(scope, id, {
      ...props,
      "codeDirectory": path.join(__dirname, 'auto-delete-objects-handler'),
      "runtimeName": "nodejs18.x",
      policyStatements: [
        {
          Effect: 'Deny',
          Action: 'logs:CreateLogGroup',
          Resource: '*',
        }
      ],
    });
  }

The name of the log group is plumbed into the custom resource lambda via loggingConfig:

    let loggingConfig = {};
    if (props.logGroupName) {
      loggingConfig = { LogGroup: props.logGroupName };
    }

    const handler = new CfnResource(this, 'Handler', {
      type: 'AWS::Lambda::Function',
      properties: {
        Code: code,
        Timeout: timeout.toSeconds(),
        MemorySize: memory.toMebibytes(),
        Handler: codeHandler,
        LoggingConfig: loggingConfig ?? undefined,
        Role: this.roleArn,
        Runtime: props.runtimeName,
        Environment: this.renderEnvironmentVariables(props.environment),
        Description: props.description ?? undefined,
      },
    });

I need to write some tests for this and then go through at least three affected snapshot tests and then I'll open a PR.

@yerzhan7
Copy link
Contributor

yerzhan7 commented May 4, 2024

I think there is a similar problem with Bucket Notification Custom Resource Lambda:

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource-handler.ts

It currently creates log group without expiration. Example from integration test: /aws/lambda/aws-cdk-s3-notifications-BucketNotificationsHandle-suEzv62inRVo

@samson-keung
Copy link
Contributor

Providing an update on my progress:

I had an PR up to add a autoDeleteObjectsLogGroup prop but it did not work out (see PR comment for reason).

I am now taking another approach where a new static method will be added to the Bucket class:

export class Bucket extends BucketBase {
  // ...
  public static setAutoDeleteObjectsLogGroup(scope: Construct, logGroup: logs.ILogGroup): void {
    // find the AutoDeleteObjectsProvider lambda and set the log group
  }
  // ...
}

When completed, users should be able to do the following:

class TestStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

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

@mergify mergify bot closed this as completed in 95938b2 Jun 17, 2024
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@GavinZZ GavinZZ reopened this Jun 19, 2024
@GavinZZ
Copy link
Contributor

GavinZZ commented Jun 19, 2024

A follow-up on this issue. The original PR that fixes this issue was reverted due to a number of issues. Note that this is a more general issue happening across all custom resources. Will going to keep this issue as closed and track this issue in a duplicate GitHub issue #23909.

@GavinZZ GavinZZ closed this as completed Jun 19, 2024
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

sarangarav pushed a commit to sarangarav/aws-cdk that referenced this issue Jun 21, 2024
…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*
mazyu36 pushed a commit to mazyu36/aws-cdk that referenced this issue Jun 22, 2024
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment