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.

Sign up for GitHub

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

Jump to bottom

feat(lambda): configurable retries for log retention custom resource #8258

Merged
merged 15 commits into from
Jun 12, 2020
Merged

feat(lambda): configurable retries for log retention custom resource #8258

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
53fcc4e
fix(lambda): increase retry count for creating cloudwatch log groups
jaapvanblaaderen May 28, 2020
4856cb3
Merge branch 'master' into log-retention-rate-exceeded-error
jaapvanblaaderen Jun 8, 2020
044a81c
fix(lambda): Make retry options configurable for CloudWatchLogs group…
jaapvanblaaderen Jun 8, 2020
4f3ef1d
Merge branch 'master' into log-retention-rate-exceeded-error
jaapvanblaaderen Jun 8, 2020
1f86f2f
fix(lambda): Fix integration test
jaapvanblaaderen Jun 8, 2020
5e9a4f0
Merge remote-tracking branch 'jaap/log-retention-rate-exceeded-error'…
jaapvanblaaderen Jun 8, 2020
7ca18c7
fix(lambda): Code cleanup - fix review comments
jaapvanblaaderen Jun 11, 2020
f33ae20
Merge branch 'master' into log-retention-rate-exceeded-error
jaapvanblaaderen Jun 11, 2020
c4823d5
fix(lambda):Update readme
jaapvanblaaderen Jun 11, 2020
1c6f5d7
Update packages/@aws-cdk/aws-lambda/lib/log-retention.ts
jaapvanblaaderen Jun 12, 2020
6708723
Tweak comment
jaapvanblaaderen Jun 12, 2020
6f4057c
fix(lambda): Fix review comments
jaapvanblaaderen Jun 12, 2020
458b074
Merge remote-tracking branch 'jaap/log-retention-rate-exceeded-error'…
jaapvanblaaderen Jun 12, 2020
1c15e29
Merge branch 'master' into log-retention-rate-exceeded-error
mergify[bot] Jun 12, 2020
7de918b
Merge branch 'master' into log-retention-rate-exceeded-error
mergify[bot] Jun 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/@aws-cdk/aws-lambda/lib/function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { calculateFunctionHash, trimFromStart } from './function-hash';
import { Version, VersionOptions } from './lambda-version';
import { CfnFunction } from './lambda.generated';
import { ILayerVersion } from './layers';
import { LogRetention } from './log-retention';
import { LogRetention, LogRetentionRetryOptions } from './log-retention';
import { Runtime } from './runtime';

/**
Expand Down Expand Up @@ -232,6 +232,16 @@ export interface FunctionOptions extends EventInvokeConfigOptions {
*/
readonly logRetentionRole?: iam.IRole;

/**
* Retry options for creating CloudWatch log groups. Deploying many Lambdas
* with log retention resources may result in rate limit issues when creating
* CloudWatch Log groups. The retry options allow you to customize the retry
* options in order to successfully create these.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sufficient to just document what these options are. There can be many reasons to use them.

Something like this -

Suggested change
* Retry options for creating CloudWatch log groups. Deploying many Lambdas
* with log retention resources may result in rate limit issues when creating
* CloudWatch Log groups. The retry options allow you to customize the retry
* options in order to successfully create these.
* When log retention is specified, a custom resource attempts to create the CloudWatch log group.
* These options control the retry policy when interacting with CloudWatch APIs.

Copy link
Contributor Author

@jaapvanblaaderen jaapvanblaaderen Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Agree, will change that.

*
* @default - Default retry options of the AWS SDK.
*/
readonly logRetentionRetryOptions?: LogRetentionRetryOptions;

/**
* Options for the `lambda.Version` resource automatically created by the
* `fn.currentVersion` method.
Expand Down Expand Up @@ -544,6 +554,7 @@ export class Function extends FunctionBase {
logGroupName: `/aws/lambda/${this.functionName}`,
retention: props.logRetention,
role: props.logRetentionRole,
logRetentionRetryOptions: props.logRetentionRetryOptions,
});
this._logGroup = logs.LogGroup.fromLogGroupArn(this, 'LogGroup', logretention.logGroupArn);
}
Expand Down
35 changes: 27 additions & 8 deletions packages/@aws-cdk/aws-lambda/lib/log-retention-provider/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@

// eslint-disable-next-line import/no-extraneous-dependencies
import * as AWS from 'aws-sdk';
import { LogRetentionRetryOptions } from '../log-retention';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will work, the lambda function execution should fail. Did you try deploying a stack that used this?

This file gets deployed as a lambda function during stack deployment. The CDK code is not available there and it should not be.

Copy link
Contributor Author

@jaapvanblaaderen jaapvanblaaderen Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I linked it using a postinstall hook in my local test project. Then it works (I guess since it's just an interface that doesn't add any logic to the deployed lambda function). But this is indeed incorrect, I'll fix it.


/**
* Creates a log group and doesn't throw if it exists.
*
* @param logGroupName the name of the log group to create
*/
async function createLogGroupSafe(logGroupName: string) {
async function createLogGroupSafe(logGroupName: string, retryOptions?: LogRetentionRetryOptions) {
try { // Try to create the log group
const cloudwatchlogs = new AWS.CloudWatchLogs({ apiVersion: '2014-03-28' });
const cloudwatchlogs = new AWS.CloudWatchLogs({ apiVersion: '2014-03-28', ...retryOptions });
await cloudwatchlogs.createLogGroup({ logGroupName }).promise();
} catch (e) {
if (e.code !== 'ResourceAlreadyExistsException') {
Expand All @@ -25,8 +26,8 @@ async function createLogGroupSafe(logGroupName: string) {
* @param logGroupName the name of the log group to create
* @param retentionInDays the number of days to retain the log events in the specified log group.
*/
async function setRetentionPolicy(logGroupName: string, retentionInDays?: number) {
const cloudwatchlogs = new AWS.CloudWatchLogs({ apiVersion: '2014-03-28' });
async function setRetentionPolicy(logGroupName: string, retryOptions?: LogRetentionRetryOptions, retentionInDays?: number) {
const cloudwatchlogs = new AWS.CloudWatchLogs({ apiVersion: '2014-03-28', ...retryOptions });
if (!retentionInDays) {
await cloudwatchlogs.deleteRetentionPolicy({ logGroupName }).promise();
} else {
Expand All @@ -41,10 +42,13 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent
// The target log group
const logGroupName = event.ResourceProperties.LogGroupName;

// Parse retry options for creating the target log group
const retryOptions = parseRetryOptions(event.ResourceProperties.LogRetentionRetryOptions);

if (event.RequestType === 'Create' || event.RequestType === 'Update') {
// Act on the target log group
await createLogGroupSafe(logGroupName);
await setRetentionPolicy(logGroupName, parseInt(event.ResourceProperties.RetentionInDays, 10));
await createLogGroupSafe(logGroupName, retryOptions);
await setRetentionPolicy(logGroupName, retryOptions, parseInt(event.ResourceProperties.RetentionInDays, 10));

if (event.RequestType === 'Create') {
// Set a retention policy of 1 day on the logs of this function. The log
Expand All @@ -56,8 +60,8 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent
// same time. This can sometime result in an OperationAbortedException. To
// avoid this and because this operation is not critical we catch all errors.
try {
await createLogGroupSafe(`/aws/lambda/${context.functionName}`);
await setRetentionPolicy(`/aws/lambda/${context.functionName}`, 1);
await createLogGroupSafe(`/aws/lambda/${context.functionName}`, retryOptions);
await setRetentionPolicy(`/aws/lambda/${context.functionName}`, retryOptions, 1);
} catch (e) {
console.log(e);
}
Expand Down Expand Up @@ -108,4 +112,19 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent
}
});
}

function parseRetryOptions(rawOptions: any) {
const retryOptions: { maxRetries?: number, retryOptions?: { base?: number } } = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to take advantage of type checking (instead of using any) and define a type like

interface SdkRetryOptions {
  readonly maxRetries?: number;
  retryOptions?: AWS.RetryDelayOptions;
}

parseRetryOptions will return this type and the options parameter in APIs like createLogGroupSafe and setRetentionPolicy() can be set to this type.

if (rawOptions) {
if (rawOptions.maxRetries) {
retryOptions.maxRetries = parseInt(rawOptions.maxRetries, 10);
}
if (rawOptions.base) {
retryOptions.retryOptions = {
base: parseInt(rawOptions.base, 10),
};
}
}
return retryOptions;
}
}
26 changes: 26 additions & 0 deletions packages/@aws-cdk/aws-lambda/lib/log-retention.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,31 @@ export interface LogRetentionProps {
* @default - A new role is created
*/
readonly role?: iam.IRole;

/**
* Retry options for managing CloudWatch log groups.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this?

Suggested change
* Retry options for managing CloudWatch log groups.
* Retry options for all AWS API calls.

*
* @default - AWS SDK default retry options
*/
readonly logRetentionRetryOptions?: LogRetentionRetryOptions;
}

/**
* Retry options for managing CloudWatch log groups
*/
export interface LogRetentionRetryOptions {
/**
* The maximum amount of retries.
*
* @default - AWS SDK default
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually document the default, even if it comes from a dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, yeah I can change that, but left it like this on purpose. How will you make sure it stays in sync?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's unlikely the SDK's defaults change. Changing so may break existing customers using the CLI whose applications work because of a given set of defaults, so this is likely to be considered a breaking change.

It may change on the next major version but we have better control over that from the import statements.

*/
readonly maxRetries?: number;
nija-at marked this conversation as resolved.
Show resolved Hide resolved
/**
* The base number of milliseconds to use in the exponential backoff for operation retries.
*
* @default - AWS SDK default
*/
readonly base?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Duration type from the @aws-cdk/core module.

}

/**
Expand Down Expand Up @@ -69,6 +94,7 @@ export class LogRetention extends cdk.Construct {
properties: {
ServiceToken: provider.functionArn,
LogGroupName: props.logGroupName,
LogRetentionRetryOptions: props.logRetentionRetryOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just Retry or SdkRetry. We're already in the context of LogRetention.

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, will change

RetentionInDays: props.retention === logs.RetentionDays.INFINITE ? undefined : props.retention,
},
});
Expand Down
18 changes: 9 additions & 9 deletions packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
"Properties": {
"Code": {
"S3Bucket": {
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3Bucket7046E6CE"
"Ref": "AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3BucketA7A09DD7"
},
"S3Key": {
"Fn::Join": [
Expand All @@ -146,7 +146,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583"
"Ref": "AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3VersionKey579A73D3"
}
]
}
Expand All @@ -159,7 +159,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583"
"Ref": "AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3VersionKey579A73D3"
}
]
}
Expand Down Expand Up @@ -331,17 +331,17 @@
}
},
"Parameters": {
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3Bucket7046E6CE": {
"AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3BucketA7A09DD7": {
"Type": "String",
"Description": "S3 bucket for asset \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "S3 bucket for asset \"e375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028f\""
},
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583": {
"AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3VersionKey579A73D3": {
"Type": "String",
"Description": "S3 key for asset version \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "S3 key for asset version \"e375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028f\""
},
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eArtifactHashB967D42A": {
"AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fArtifactHashE75963CF": {
"Type": "String",
"Description": "Artifact hash for asset \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "Artifact hash for asset \"e375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028f\""
}
}
}
37 changes: 37 additions & 0 deletions packages/@aws-cdk/aws-lambda/test/test.log-retention-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,4 +296,41 @@ export = {

test.done();
},

async 'custom log retention retry options'(test: Test) {
AWS.mock('CloudWatchLogs', 'createLogGroup', sinon.fake.resolves({}));
AWS.mock('CloudWatchLogs', 'putRetentionPolicy', sinon.fake.resolves({}));
AWS.mock('CloudWatchLogs', 'deleteRetentionPolicy', sinon.fake.resolves({}));

const event = {
...eventCommon,
RequestType: 'Create',
ResourceProperties: {
ServiceToken: 'token',
RetentionInDays: '30',
LogGroupName: 'group',
LogRetentionRetryOptions: {
maxRetries: '5',
base: '300',
},
},
};

const request = createRequest('SUCCESS');

await provider.handler(event as AWSLambda.CloudFormationCustomResourceCreateEvent, context);

sinon.assert.calledWith(AWSSDK.CloudWatchLogs as any, {
apiVersion: '2014-03-28',
maxRetries: 5,
retryOptions: {
base: 300,
},
});

test.equal(request.isDone(), true);

test.done();
},

};
20 changes: 10 additions & 10 deletions packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,7 @@
"Properties": {
"Code": {
"S3Bucket": {
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3Bucket7046E6CE"
"Ref": "AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3BucketA7A09DD7"
},
"S3Key": {
"Fn::Join": [
Expand All @@ -980,7 +980,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583"
"Ref": "AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3VersionKey579A73D3"
}
]
}
Expand All @@ -993,7 +993,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583"
"Ref": "AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3VersionKey579A73D3"
}
]
}
Expand Down Expand Up @@ -1108,17 +1108,17 @@
}
},
"Parameters": {
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3Bucket7046E6CE": {
"AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3BucketA7A09DD7": {
"Type": "String",
"Description": "S3 bucket for asset \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "S3 bucket for asset \"e375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028f\""
},
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eS3VersionKey3194A583": {
"AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fS3VersionKey579A73D3": {
"Type": "String",
"Description": "S3 key for asset version \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "S3 key for asset version \"e375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028f\""
},
"AssetParameters82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4eArtifactHashB967D42A": {
"AssetParameterse375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028fArtifactHashE75963CF": {
"Type": "String",
"Description": "Artifact hash for asset \"82c54bfa7c42ba410d6d18dad983ba51c93a5ea940818c5c20230f8b59c19d4e\""
"Description": "Artifact hash for asset \"e375da66e6ab168bb13b858a043e9e8c8c20334b443d746983fa4ad0dcc7028f\""
}
}
}
}