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(lambda): add support for log retention #2067

Merged
merged 9 commits into from
Mar 28, 2019

Conversation

jogold
Copy link
Contributor

@jogold jogold commented Mar 21, 2019

Adds a new property logRetentionDays on Function to control the log
retention policy of the function logs in CloudWatch Logs.

The implementation uses a Custom Resource to create the log group if it doesn't
exist yet and to set the retention policy as discussed in #667.

A retention policy of 1 day is set on the logs of the Lambda provider.

The different retention days supported by CloudWatch Logs have been centralized
in @aws-cdk/aws-logs. Some have been renamed to better match the console
experience.

Closes #667

BREAKING CHANGE: cloudWatchLogsRetentionTimeDays in @aws-cdk/aws-cloudtrail
now uses a logs.RetentionDays instead of a LogRetention.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

Adds a new property `logRetentionDays` on `Function` to control the log
retention policy of the function logs in CloudWatch Logs.

The implementation uses a Custom Resource to create the log group if it doesn't
exist yet and to set the retention policy as discussed in aws#667.

A retention policy of 1 day is set on the logs of the Lambda provider.

The different retention days supported by CloudWatch Logs have been centralized
in `@aws-cdk/aws-logs`. Some have been renamed to better match the console
experience.

Closes aws#667

BREAKING CHANGE: `cloudWatchLogsRetentionTimeDays` in `@aws-cdk/aws-cloudtrail`
now uses a `logs.RetentionDays` instead of a `LogRetention`.
@jogold jogold requested a review from a team as a code owner March 21, 2019 09:34
packages/@aws-cdk/aws-lambda/lib/function.ts Show resolved Hide resolved
// Need to use a CfnResource here to prevent lerna dependency cycles
// @aws-cdk/aws-cloudformation -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation
new cdk.CfnResource(this, 'LogRetentionCustomResource', {
type: 'AWS::CloudFormation::CustomResource',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a custom type name that indicates what this type is doing (I believe "AWS::Custom::XXX" or something along those lines)

// Log retention
if (props.logRetentionDays) {
// Custom resource provider
const provider = new SingletonFunction(this, 'LogRetentionProvider', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap this whole thing up into a construct that encapsulates all the custom resource details.

Copy link
Contributor Author

@jogold jogold Mar 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, what about moving this construct to aws-cloudformation or aws-logs (would cause a dep cycle) then? It could be used for CodeBuild and other services. For RDS, we have a similar situation when CloudWatch Logs are enabled (the log group is created during instance creation)

new LogRetention(this, 'LogRetention', {
  logGroupName: ...,
  retentionInDays: ...
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or to a new package (@aws-cdk/custom-resource) as discussed in #1850.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this will create dependency cycles (SingletonFunction)...

} catch (e) {
console.log(e);

await respond('FAILED', e.message, context.logStreamName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use logGroupName as the physical resource name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, should be logGroupName here also.

@@ -0,0 +1,30 @@
import logs = require('@aws-cdk/aws-logs');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a unit test for the handler

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

@eladb eladb merged commit 63132ec into aws:master Mar 28, 2019
@jogold jogold deleted the lambda-log-retention branch March 28, 2019 14:12
@mgjam
Copy link

mgjam commented Nov 1, 2019

Hello, when using this property on my existing lambda resource, new lambda resource attempts to be created, is this intentional behavior? Why this new lambda function is attempted to be created?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make log groups for Lambda/CodeBuild accessible and/or manipulable
3 participants