feat(lambda): avail function log group in the CDK #5878

nija-at · 2020-01-20T17:47:46Z

Commit Message

The lambda function's log group is now available for use in the CDK app
so it can be used further to create subscription filters, metric
filters, etc. or in any other way that a regular log group may be used.

The implementation uses an underlying CustomResource to guarantee the
existence of this log group as part of the CloudFormation stack.
However, the custom resource is only created either when the
logRetention property is set or when logGroup getter is called. This
prevents existing stacks that use Lambda function to get the custom
resource and can be opted into.

closes #3838

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

The lambda function's log group is now available for use in the CDK app so it can be used further to create subscription filters, metric filters, etc. or in any other way that a regular log group may be used. The implementation uses an underlying CustomResource to guarantee the existence of this log group as part of the CloudFormation stack. A new property called `exposeLogGroup` to enable this, so that existing stacks that have a Lambda function don't change significantly by automatically getting this CustomResource, as well as, users who are not interested in this log group can opt out. closes #3838

aws-cdk-automation · 2020-01-20T17:57:53Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-lambda/lib/function.ts

aws-cdk-automation · 2020-01-21T09:57:49Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

eladb

Two notes:

Feels like there is something we can do to lazily create the LogRetention in case logGroup is accessed instead of "polluting" the API with exposeLogGroup, which feels a bit hacky.
What was the rational for implementing ILogGroup by LogRetention instead of just logs.LogGroup.fromLogGroupArn(). Feels like it would have saved a lot code duplication from the logs module.

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

aws-cdk-automation · 2020-01-21T13:28:05Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

nija-at · 2020-01-21T18:53:35Z

instead of just logs.LogGroup.fromLogGroupArn(). Feels like it would have saved a lot code duplication from the logs module.

You are correct. That is simpler. I was struggling a bit to get that right. Turns out the solution is quite nifty.

Feels like there is something we can do to lazily create the LogRetention in case logGroup is accessed instead of "polluting" the API with exposeLogGroup, which feels a bit hacky.

I was hoping to give more power to the user to control this, but you're right. Switched this around.

aws-cdk-automation · 2020-01-21T19:20:33Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-21T19:29:56Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-21T19:41:54Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

addressed feedback

rix0rrr

Yes, this works. Looks good, please add a ## Commit Message to the PR description and update the text in there to match what you ended up doing.

nija-at · 2020-01-24T11:09:30Z

Yes, this works. Looks good, please add a ## Commit Message to the PR description and update the text in there to match what you ended up doing.

@rix0rrr - done!

packages/@aws-cdk/aws-lambda/lib/function.ts

RomainMuller · 2020-01-24T11:07:15Z

packages/@aws-cdk/aws-lambda/lib/function.ts

+      const logretention = new LogRetention(this, 'LogRetention', {
+        logGroupName: `/aws/lambda/${this.functionName}`,
+        retention: logs.RetentionDays.INFINITE,
+      });


Do we even need this? Can't we just fromAttributes the LogGroup?

I feel like setting INFINITE retention here may not be the right thing to do... People may have manually changed the retention to something else before they went through this code path, and they probably don't expect you'll be resetting it to Expire: NEVER when they just access the logGroup property...

Now obviously, you could have the problem that the LogGroup will not exist in the service before the function has been invoked at least once (if you don't create it yourself)... In such case, it'd be better to have a custom handler that just creates the log group if missing, but leaves the configured retention if it already exists...

Do we even need this? Can't we just fromAttributes the LogGroup?

We need it because the log group might not exist. Take a look at LogRetention if you haven't already and you'll get the idea.

Now obviously, you could have the problem that the LogGroup will not exist in the service before the function has been invoked at least once (if you don't create it yourself)... In such case, it'd be better to have a custom handler that just creates the log group if missing, but leaves the configured retention if it already exists...

LogRetention does exactly that.

they probably don't expect you'll be resetting it to Expire: NEVER when they just access the logGroup property...

Good point. Let me look into that.

they probably don't expect you'll be resetting it to Expire: NEVER when they just access the logGroup property...

Good point. Let me look into that.

Wait a second. They shouldn't have set the retention policy of the function out of band; they should use the logRetention property here. If they did set it out of band, it's expected that a stack deployment may reset it, right?

That's not what Vanilla CloudFormation would do for you... I don't have a problem if you'll be resetting aggressively and consider you have to change it through here, but this should be mentioned in the documentation at the very least!

Maybe the correct solution is to make the retention property optional and have the custom resource do nothing if the property is unset and the group already exists?

On the other hand, I don't think this is going to come up a lot. Who really precreates Lambda Function Log Groups AND subsequently uses CDK to manage their Lambdas? With generated names? Plus, the reset will only happen on the first deployment, because afterwards none of the resource properties will have changed so CFN is never going to touch this custom resource ever again.

My concern is more towards the fact that a getter should be side-effects free, as it's only observing state. There are possible side-effects here, which isn't something you'd expect (and hence check for).

If your lambda logs stuff out that you are not allowed to keep for longer than X for some reason... You may suddenly be in violation of law without knowing it...

An alternative way to go here would be to always create a (regular) LogGroup, and use an auto-generated physical name (instead of ever leaving it out to CloudFormation to decide).

An alternative way to go here would be to always create a (regular) LogGroup, and use an auto-generated physical name (instead of ever leaving it out to CloudFormation to decide).

I'm not sure I follow your alternative here.
However, reminding you that the lambda service decides that the log group must be /aws/lambda/<function-name> and we have no control over it.

If your lambda logs stuff out that you are not allowed to keep for longer than X for some reason... You may suddenly be in violation of law without knowing it...

You're right, but if you need to comply with the law, you should be setting the logRetention property which will do the correct thing for you. By not managing an AWS resource that is regulated lawfully in you IaaC is already bad.

However, your philosophical point is well taken.

I've updated the README with more documentation on this. Does that satisfy, or do you feel strongly that I change the logic?

aws-cdk-automation · 2020-01-24T13:37:20Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-24T14:22:07Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-24T15:33:54Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2020-01-24T15:34:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-24T16:36:28Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-24T16:36:50Z

Thank you for contributing! Your pull request is now being automatically merged.

nija-at added the contribution/core This is a PR that came from AWS. label Jan 20, 2020

nija-at requested review from RomainMuller, rix0rrr and eladb January 20, 2020 17:47

nija-at self-assigned this Jan 20, 2020

jogold reviewed Jan 21, 2020

View reviewed changes

packages/@aws-cdk/aws-lambda/lib/function.ts Outdated Show resolved Hide resolved

fix broken integ test

d1bdda8

nija-at added the pr/do-not-merge This PR should not be merged at this time. label Jan 21, 2020

fix broken rds integ test

d4bb962

nija-at requested review from shivlaks and skinny85 as code owners January 21, 2020 12:50

eladb previously requested changes Jan 21, 2020

View reviewed changes

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

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

Niranjan Jayakar added 3 commits January 21, 2020 18:33

PR feedback

2a3949f

switched to creating LogRetention on demand instead of an upfront flag

4143431

additional unit test

9f38d96

nija-at requested a review from eladb January 21, 2020 18:53

Merge branch 'master' into nija-at/lambda-loggroup

aec3391

nija-at removed the pr/do-not-merge This PR should not be merged at this time. label Jan 21, 2020

rix0rrr requested changes Jan 24, 2020

View reviewed changes

nija-at requested a review from rix0rrr January 24, 2020 11:09

RomainMuller reviewed Jan 24, 2020

View reviewed changes

dont use node.id prefix

1ca5ebb

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Jan 24, 2020

rix0rrr approved these changes Jan 24, 2020

View reviewed changes

add a section to the readme

30155e2

Added same documentation to the getter

83bc28c

nija-at removed the pr/do-not-merge This PR should not be merged at this time. label Jan 24, 2020

Merge branch 'master' into nija-at/lambda-loggroup

0ef214a

mergify bot merged commit fd54a17 into master Jan 24, 2020

mergify bot deleted the nija-at/lambda-loggroup branch January 24, 2020 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda): avail function log group in the CDK #5878

feat(lambda): avail function log group in the CDK #5878

nija-at commented Jan 20, 2020 •

edited

Loading

aws-cdk-automation commented Jan 20, 2020

aws-cdk-automation commented Jan 21, 2020

eladb left a comment

aws-cdk-automation commented Jan 21, 2020

nija-at commented Jan 21, 2020

aws-cdk-automation commented Jan 21, 2020

aws-cdk-automation commented Jan 21, 2020

aws-cdk-automation commented Jan 21, 2020

rix0rrr left a comment

nija-at commented Jan 24, 2020

RomainMuller Jan 24, 2020

nija-at Jan 24, 2020 •

edited

Loading

nija-at Jan 24, 2020

RomainMuller Jan 24, 2020

rix0rrr Jan 24, 2020

RomainMuller Jan 24, 2020

nija-at Jan 24, 2020

aws-cdk-automation commented Jan 24, 2020

aws-cdk-automation commented Jan 24, 2020

mergify bot commented Jan 24, 2020

aws-cdk-automation commented Jan 24, 2020

aws-cdk-automation commented Jan 24, 2020

mergify bot commented Jan 24, 2020

feat(lambda): avail function log group in the CDK #5878

feat(lambda): avail function log group in the CDK #5878

Conversation

nija-at commented Jan 20, 2020 • edited Loading

Commit Message

aws-cdk-automation commented Jan 20, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 21, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 21, 2020

AWS CodeBuild CI Report

nija-at commented Jan 21, 2020

aws-cdk-automation commented Jan 21, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 21, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 21, 2020

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

nija-at commented Jan 24, 2020

RomainMuller Jan 24, 2020

Choose a reason for hiding this comment

nija-at Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

nija-at Jan 24, 2020

Choose a reason for hiding this comment

RomainMuller Jan 24, 2020

Choose a reason for hiding this comment

rix0rrr Jan 24, 2020

Choose a reason for hiding this comment

RomainMuller Jan 24, 2020

Choose a reason for hiding this comment

nija-at Jan 24, 2020

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 24, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 24, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 24, 2020

aws-cdk-automation commented Jan 24, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 24, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 24, 2020

nija-at commented Jan 20, 2020 •

edited

Loading

nija-at Jan 24, 2020 •

edited

Loading