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): deprecate logRetention properties in favor of logGroup #28737

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Jan 17, 2024

#28039 introduced support for custom logging configurations for AWS Lambda Functions.

This change deprecates the logRetention, logRetentionRole and logRetentionRetryOptions properties in favor of using a custom logging configuration.

By default, Lambda functions send logs to an automatically created default log group named /aws/lambda/<function name>. However you cannot change the properties of this auto-created log group using the AWS CDK, e.g. you cannot set a different log retention. To overcome the limitation, a custom resource was introduced and configuration exposed via the logRetention properties. This is what we are deprecating in this change.

With the introduction of custom logging configuration and the new logGroup property, users can now create a fully customizable LogGroup ahead of time, and instruct the Lambda function to send logs to it.

Migrating from logRetention to logGroup will cause the name of the log group to change. Don't attempt to use the name of the auto-created log group, this will cause subtle issue. We recommend using auto-naming for lambda log groups, they can easily be accessed via the Lambda Console. If you want use a well-known name, we recommend using a pattern like /<your service>/lambda/<function name>. Be aware that a names log group can prevent a stack from being recreated without manual intervention after it has been deployed (error Resource already exists). This is because LogGroups are retained by default.

Either way, users will have to adjust and documentation will need to be updated. Any code referencing the old log group name verbatim will have to be changed as well. Keep in mind that in AWS CDK code, you can access the log group name directly from the LogGroup construct:

declare const myLogGroup: logs.LogGroup;
myLogGroup.logGroupName;

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

@aws-cdk-automation aws-cdk-automation requested a review from a team January 17, 2024 10:50
@github-actions github-actions bot added the p2 label Jan 17, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 17, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@mrgrain mrgrain added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exempt-readme The PR linter will not require README changes labels Jan 17, 2024
@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 17, 2024

Exempting tests, as this is a deprecation without changing functionality. Feat in name only to show up in change log.

@mrgrain mrgrain changed the title fix(lambda): deprecate logRetention properties in favor of logGroup feat(lambda): deprecate logRetention properties in favor of logGroup Jan 17, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 17, 2024 11:17

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@mrgrain mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Jan 17, 2024
@mrgrain mrgrain removed the pr/do-not-merge This PR should not be merged at this time. label Jan 17, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

mergify bot commented Jan 17, 2024

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

@mergify mergify bot merged commit 4a09720 into main Jan 17, 2024
12 checks passed
@mergify mergify bot deleted the mrgrain/fix/no-more-log-retention-for-you branch January 17, 2024 12:46
@isker
Copy link
Contributor

isker commented Jan 18, 2024

Question from the peanut gallery: I pulled this change in from today's release, and there are warnings coming from code that is seemingly native to the CDK, in EKS. Should that be happening? I can't do anything about it, as far as I can tell, so this is a pretty tiresome warning. Here's the stack logged from my tests:

    > 95 |     const cluster = new Cluster(this, "Cluster", {
         |                     ^
      96 |       clusterName,
      97 |       version: k8sVersion,
      98 |       kubectlLayer,

      at print (node_modules/aws-cdk-lib/.warnings.jsii.js:5:146)
      at Object.aws_cdk_lib_aws_lambda_FunctionProps (node_modules/aws-cdk-lib/.warnings.jsii.js:1:1044823)
      at new Function (node_modules/aws-cdk-lib/aws-lambda/lib/function.js:1:7775)
      at Provider.createFunction (node_modules/aws-cdk-lib/custom-resources/lib/provider-framework/provider.js:1:2730)
      at new Provider (node_modules/aws-cdk-lib/custom-resources/lib/provider-framework/provider.js:1:1809)
      at new KubectlProvider (node_modules/aws-cdk-lib/aws-eks/lib/kubectl-provider.js:1:4575)
      at Cluster.defineKubectlProvider (node_modules/aws-cdk-lib/aws-eks/lib/cluster.js:1:21288)
      at new Cluster (node_modules/aws-cdk-lib/aws-eks/lib/cluster.js:1:15434)

I can't find where this is happening in the source of this repo. There is some magic going on here that I do not understand:

import { KubectlFunction } from '../../custom-resource-handlers/dist/aws-eks/kubectl-provider.generated';

@relm923
Copy link
Contributor

relm923 commented Jan 19, 2024

Seeing similar warnings when using CustomResourceProviders with no way to resolve

@thecardcheat
Copy link

thecardcheat commented Jan 19, 2024

Noisy for CDK project(s) with s3.BucketDeployment with no clear path to resolve since you cannot provide the logGroup or function params to this construct.

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 19, 2024

Thanks for calling this out folks! The internal usage should have been flagged by our tools, but wasn't. Looking into options right now.

mrgrain added a commit that referenced this pull request Jan 19, 2024
…avor of `logGroup`

See #28737 for full details.

Some custom resources have made the `logRetention` property part of their own API. In these cases, we are now also deprecating `logRetention`.

Migrating log groups for custom resource would follow the same steps as outline in #28737. Given that custom resource logging is for debugging purposes and there are no guarantees about the output format, it should be possible to simply replace `logRetention` with a simple `logGroup` in most cases:

```ts
const awsCustom1 = new cr.AwsCustomResource(this, 'API1', {
  // Replace this
  logRetention: logs.RetentionDays.ONE_WEEK,
  // with
  logGroup: new logs.LogGroup(this, 'AwsCustomResourceLogs', {
    retention: logs.RetentionDays.ONE_WEEK,
  }),
});
```
@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 19, 2024

Fix is coming via #28783. Cluster is now perfectly quiet again. 🤫

image

@isker
Copy link
Contributor

isker commented Jan 19, 2024

Thanks!

mergify bot pushed a commit that referenced this pull request Jan 22, 2024
…gGroup` (#28783)

In #28737 we have deprecated `logRetention` in favor of `logGroup`. Some custom resources have made the `logRetention` property part of their own API and are now emitting deprecation warnings with no way forward to resolve them. So we are now also deprecating `logRetention` for any custom resources.

Migrating log groups for custom resource would follow the same steps as outline in #28737. Given that custom resource logging is for debugging purposes and there are no guarantees about the output format, it should be possible to simply replace `logRetention` with a simple `logGroup` in most cases:

```ts
const awsCustom1 = new cr.AwsCustomResource(this, 'API1', {
  // Replace this
  logRetention: logs.RetentionDays.ONE_WEEK,
  // with
  logGroup: new logs.LogGroup(this, 'AwsCustomResourceLogs', {
    retention: logs.RetentionDays.ONE_WEEK,
  }),
});
```
Fixes #28806
Fixes #28809
Related to #28737

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@fiserv-plat-eng
Copy link

What happens to orphaned log groups? What is the cost implications of having both log groups from older stacks.

@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 22, 2024

What happens to orphaned log groups? What is the cost implications of having both log groups from older stacks.

You can find pricing information here:
https://aws.amazon.com/cloudwatch/pricing/

Logs are charged by data, not log groups.
Hope that helps.

mergify bot pushed a commit that referenced this pull request Jan 31, 2024
### Issue

Closes #28919

### Reason for this change

In #28737 and #28783 we have deprecated various `logRetention` properties in favor of the new Logging Configuration feature for Lambda Functions. This new feature provides full control over the Lambda Function log group via the `logGroup` property.

However Logging Configuration is not available yet for all regions. Particularly GovCloud and CN regions (at the time this issue was created). For customers in of these regions there is currently no clear migration path.

We therefore revert the deprecation of previously deprecated properties.

### Description of changes

Revert the deprecation of previously deprecated `logRetention` properties.
Update documentation to be more explicit about the various options customers have.

### Description of how you validated changes

Documentation & annotation change only. This is a partial revert of the PRs linked about, with minor documentation updates.

### 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*
SankyRed pushed a commit that referenced this pull request Feb 8, 2024
### Issue

Closes #28919

### Reason for this change

In #28737 and #28783 we have deprecated various `logRetention` properties in favor of the new Logging Configuration feature for Lambda Functions. This new feature provides full control over the Lambda Function log group via the `logGroup` property.

However Logging Configuration is not available yet for all regions. Particularly GovCloud and CN regions (at the time this issue was created). For customers in of these regions there is currently no clear migration path.

We therefore revert the deprecation of previously deprecated properties.

### Description of changes

Revert the deprecation of previously deprecated `logRetention` properties.
Update documentation to be more explicit about the various options customers have.

### Description of how you validated changes

Documentation & annotation change only. This is a partial revert of the PRs linked about, with minor documentation updates.

### 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*
@dude0001
Copy link

What happens to orphaned log groups? What is the cost implications of having both log groups from older stacks.

You can find pricing information here: https://aws.amazon.com/cloudwatch/pricing/

Logs are charged by data, not log groups. Hope that helps.

@mrgrain I converted all Lambdas in a Stack to use logGroup and redeployed. The Custom::LogRetention custom resource that was created when using logRetention is still showing in the Stack Template and is assumed to be orphaned.

Expanding on the first question "What happens to orphaned log groups?" and disregarding the cost implications question:

  • Are there manual or other recommended steps to clean up CloudFormation Stacks and AWS environments after refactoring your CDK to use logGroup instead of logRetention on Lambdas?

  • What is the recommended way to remove the Cutom::LogRetention Resource from your Stack and clean up its resources? Should this be automated in CDK if this is truly orphaned?

  • En lieu of an automated/native solution in CDK, is it okay to manually delete the resources Custom::LogRetention creates (LogRetention Lambda, IAM Roles and the CloudWatch Log Groups the LogRetention Lambda creates) and manually delete Custom::LogRetention from the CloudFormation Stack Template? Or is this used in some other way by CDK?

@fiserv-plat-eng
Copy link

fiserv-plat-eng commented Feb 23, 2024 via email

@mrgrain
Copy link
Contributor Author

mrgrain commented Feb 26, 2024

  • En lieu of an automated/native solution in CDK, is it okay to manually delete the resources Custom::LogRetention creates (LogRetention Lambda, IAM Roles and the CloudWatch Log Groups the LogRetention Lambda creates) and manually delete Custom::LogRetention from the CloudFormation Stack Template? Or is this used in some other way by CDK?
  • What is the recommended way to remove the Cutom::LogRetention Resource from your Stack and clean up its resources? Should this be automated in CDK if this is truly orphaned?

If you still have a Custom::LogRetention resource inside your stack template, you have not removed all usages of LogRetention. Note that there are other valid usages of this Custom Resource. The resources created by logRetention are by default removed automatically, with the exception of the log group created for the custom resource lambda (see next question).

  • Are there manual or other recommended steps to clean up CloudFormation Stacks and AWS environments after refactoring your CDK to use logGroup instead of logRetention on Lambdas?

No. With a default configuration, only the log group created for the execution logs of the custom resource lambda will remain in your account. However this LogGroup has a retention policy of 1 day. If you want to you can delete this log group manually.
You can also decide if you would like to remove any log groups that were originally created by the Lambda Service (the very issue the introduction of logGroup addresses). Consider this once these log groups have passed their Retention policy timeframe and do not contain any log streams anymore. This is similar to the situation of a Lambda Function being deleted from a stack for any other reason.

There is no easy automated way to detect these. However you can limit the search by filtering by name prefix /aws/lambda and looking for log groups that do not have a stored size. If you wanted to automated this, you could take the log group name, strip the prefix and check your account for a lambda function of that name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants