From 43d1fed17a921b66126e2eae06a48684cf976165 Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Tue, 18 Aug 2020 16:31:35 +0100 Subject: [PATCH 01/12] refactor(lambda): move log retention custom resource to logs (#9671) move LogRetention construct definition from lambda to logs while refactoring it so it does not depend on lambda constructs this required reimplementing the functionality provided by lambda.SingletonFunction using CfnResource keep declared classes/interfaces in lambda for compatability while marking them as deprecated they should be removed in an upcoming breaking change for their current customers in lambda and rds Fixes #9671 --- .../@aws-cdk/aws-lambda/lib/log-retention.ts | 109 ++----------- packages/@aws-cdk/aws-lambda/package.json | 6 +- .../test/integ.log-retention.expected.json | 22 +-- packages/@aws-cdk/aws-logs/lib/index.ts | 1 + .../lib/log-retention-provider/index.ts | 0 .../@aws-cdk/aws-logs/lib/log-retention.ts | 151 ++++++++++++++++++ packages/@aws-cdk/aws-logs/package.json | 8 +- .../test/test.log-retention-provider.ts | 0 .../test/test.log-retention.ts | 16 +- 9 files changed, 191 insertions(+), 122 deletions(-) rename packages/@aws-cdk/{aws-lambda => aws-logs}/lib/log-retention-provider/index.ts (100%) create mode 100644 packages/@aws-cdk/aws-logs/lib/log-retention.ts rename packages/@aws-cdk/{aws-lambda => aws-logs}/test/test.log-retention-provider.ts (100%) rename packages/@aws-cdk/{aws-lambda => aws-logs}/test/test.log-retention.ts (89%) diff --git a/packages/@aws-cdk/aws-lambda/lib/log-retention.ts b/packages/@aws-cdk/aws-lambda/lib/log-retention.ts index 8e64262143a10..fed62dc2563ba 100644 --- a/packages/@aws-cdk/aws-lambda/lib/log-retention.ts +++ b/packages/@aws-cdk/aws-lambda/lib/log-retention.ts @@ -1,116 +1,31 @@ -import * as path from 'path'; -import * as iam from '@aws-cdk/aws-iam'; import * as logs from '@aws-cdk/aws-logs'; import * as cdk from '@aws-cdk/core'; -import { Code } from './code'; -import { Runtime } from './runtime'; -import { SingletonFunction } from './singleton-lambda'; /** - * Construction properties for a LogRetention. + * Retry options for all AWS API calls. + * + * @deprecated use `LogRetentionRetryOptions` from '@aws-cdk/aws-logs' instead */ -export interface LogRetentionProps { - /** - * The log group name. - */ - readonly logGroupName: string; - - /** - * The number of days log events are kept in CloudWatch Logs. - */ - readonly retention: logs.RetentionDays; - - /** - * The IAM role for the Lambda function associated with the custom resource. - * - * @default - A new role is created - */ - readonly role?: iam.IRole; - - /** - * Retry options for all AWS API calls. - * - * @default - AWS SDK default retry options - */ - readonly logRetentionRetryOptions?: LogRetentionRetryOptions; +export interface LogRetentionRetryOptions extends logs.LogRetentionRetryOptions { } /** - * Retry options for all AWS API calls. + * Construction properties for a LogRetention. + * + * @deprecated use `LogRetentionProps` from '@aws-cdk/aws-logs' instead */ -export interface LogRetentionRetryOptions { - /** - * The maximum amount of retries. - * - * @default 3 (AWS SDK default) - */ - readonly maxRetries?: number; - /** - * The base duration to use in the exponential backoff for operation retries. - * - * @default Duration.millis(100) (AWS SDK default) - */ - readonly base?: cdk.Duration; +export interface LogRetentionProps extends logs.LogRetentionProps { } /** * Creates a custom resource to control the retention policy of a CloudWatch Logs * log group. The log group is created if it doesn't already exist. The policy * is removed when `retentionDays` is `undefined` or equal to `Infinity`. + * + * @deprecated use `LogRetention` from '@aws-cdk/aws-logs' instead */ -export class LogRetention extends cdk.Construct { - - /** - * The ARN of the LogGroup. - */ - public readonly logGroupArn: string; - +export class LogRetention extends logs.LogRetention { constructor(scope: cdk.Construct, id: string, props: LogRetentionProps) { - super(scope, id); - - // Custom resource provider - const provider = new SingletonFunction(this, 'Provider', { - code: Code.fromAsset(path.join(__dirname, 'log-retention-provider')), - runtime: Runtime.NODEJS_10_X, - handler: 'index.handler', - uuid: 'aae0aa3c-5b4d-4f87-b02d-85b201efdd8a', - lambdaPurpose: 'LogRetention', - role: props.role, - }); - - // Duplicate statements will be deduplicated by `PolicyDocument` - provider.addToRolePolicy(new iam.PolicyStatement({ - actions: ['logs:PutRetentionPolicy', 'logs:DeleteRetentionPolicy'], - // We need '*' here because we will also put a retention policy on - // the log group of the provider function. Referencing it's name - // creates a CF circular dependency. - resources: ['*'], - })); - - // Need to use a CfnResource here to prevent lerna dependency cycles - // @aws-cdk/aws-cloudformation -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation - const retryOptions = props.logRetentionRetryOptions; - const resource = new cdk.CfnResource(this, 'Resource', { - type: 'Custom::LogRetention', - properties: { - ServiceToken: provider.functionArn, - LogGroupName: props.logGroupName, - SdkRetry: retryOptions ? { - maxRetries: retryOptions.maxRetries, - base: retryOptions.base?.toMilliseconds(), - } : undefined, - RetentionInDays: props.retention === logs.RetentionDays.INFINITE ? undefined : props.retention, - }, - }); - - const logGroupName = resource.getAtt('LogGroupName').toString(); - // Append ':*' at the end of the ARN to match with how CloudFormation does this for LogGroup ARNs - // See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-loggroup.html#aws-resource-logs-loggroup-return-values - this.logGroupArn = cdk.Stack.of(this).formatArn({ - service: 'logs', - resource: 'log-group', - resourceName: `${logGroupName}:*`, - sep: ':', - }); + super(scope, id, { ...props }); } } diff --git a/packages/@aws-cdk/aws-lambda/package.json b/packages/@aws-cdk/aws-lambda/package.json index b7fe98f9a2f8e..7d84beff67dd6 100644 --- a/packages/@aws-cdk/aws-lambda/package.json +++ b/packages/@aws-cdk/aws-lambda/package.json @@ -71,16 +71,12 @@ "@types/lodash": "^4.14.157", "@types/nodeunit": "^0.0.31", "@types/sinon": "^9.0.4", - "aws-sdk": "^2.736.0", - "aws-sdk-mock": "^5.1.0", "cdk-build-tools": "0.0.0", "cdk-integ-tools": "0.0.0", "cfn2ts": "0.0.0", "lodash": "^4.17.20", - "nock": "^13.0.2", "nodeunit": "^0.11.3", - "pkglint": "0.0.0", - "sinon": "^9.0.2" + "pkglint": "0.0.0" }, "dependencies": { "@aws-cdk/aws-applicationautoscaling": "0.0.0", diff --git a/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json index f123d24edf60a..1aff749c96c42 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json @@ -55,7 +55,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", "Arn" ] }, @@ -73,7 +73,7 @@ "RetentionInDays": 7 } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0": { "Type": "AWS::IAM::Role", "Properties": { "AssumeRolePolicyDocument": { @@ -104,7 +104,7 @@ ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64": { "Type": "AWS::IAM::Policy", "Properties": { "PolicyDocument": { @@ -120,15 +120,15 @@ ], "Version": "2012-10-17" }, - "PolicyName": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB", + "PolicyName": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64", "Roles": [ { - "Ref": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB" + "Ref": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0" } ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a": { "Type": "AWS::Lambda::Function", "Properties": { "Code": { @@ -172,15 +172,15 @@ "Handler": "index.handler", "Role": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0", "Arn" ] }, "Runtime": "nodejs10.x" }, "DependsOn": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB", - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB" + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0" ] }, "OneMonthServiceRoleFBD1064F": { @@ -238,7 +238,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", "Arn" ] }, @@ -311,7 +311,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", "Arn" ] }, diff --git a/packages/@aws-cdk/aws-logs/lib/index.ts b/packages/@aws-cdk/aws-logs/lib/index.ts index bf626238843c3..5054715ffe52b 100644 --- a/packages/@aws-cdk/aws-logs/lib/index.ts +++ b/packages/@aws-cdk/aws-logs/lib/index.ts @@ -4,6 +4,7 @@ export * from './log-stream'; export * from './metric-filter'; export * from './pattern'; export * from './subscription-filter'; +export * from './log-retention'; // AWS::Logs CloudFormation Resources: export * from './logs.generated'; diff --git a/packages/@aws-cdk/aws-lambda/lib/log-retention-provider/index.ts b/packages/@aws-cdk/aws-logs/lib/log-retention-provider/index.ts similarity index 100% rename from packages/@aws-cdk/aws-lambda/lib/log-retention-provider/index.ts rename to packages/@aws-cdk/aws-logs/lib/log-retention-provider/index.ts diff --git a/packages/@aws-cdk/aws-logs/lib/log-retention.ts b/packages/@aws-cdk/aws-logs/lib/log-retention.ts new file mode 100644 index 0000000000000..439d28b01e4eb --- /dev/null +++ b/packages/@aws-cdk/aws-logs/lib/log-retention.ts @@ -0,0 +1,151 @@ +import * as path from 'path'; +import * as iam from '@aws-cdk/aws-iam'; +import * as s3_assets from '@aws-cdk/aws-s3-assets'; +import * as cdk from '@aws-cdk/core'; +import { RetentionDays } from './log-group'; + +/** + * Construction properties for a LogRetention. + */ +export interface LogRetentionProps { + /** + * The log group name. + */ + readonly logGroupName: string; + + /** + * The number of days log events are kept in CloudWatch Logs. + */ + readonly retention: RetentionDays; + + /** + * The IAM role for the Lambda function associated with the custom resource. + * + * @default - A new role is created + */ + readonly role?: iam.IRole; + + /** + * Retry options for all AWS API calls. + * + * @default - AWS SDK default retry options + */ + readonly logRetentionRetryOptions?: LogRetentionRetryOptions; +} + +/** + * Retry options for all AWS API calls. + */ +export interface LogRetentionRetryOptions { + /** + * The maximum amount of retries. + * + * @default 3 (AWS SDK default) + */ + readonly maxRetries?: number; + /** + * The base duration to use in the exponential backoff for operation retries. + * + * @default Duration.millis(100) (AWS SDK default) + */ + readonly base?: cdk.Duration; +} + +/** + * Creates a custom resource to control the retention policy of a CloudWatch Logs + * log group. The log group is created if it doesn't already exist. The policy + * is removed when `retentionDays` is `undefined` or equal to `Infinity`. + */ +export class LogRetention extends cdk.Construct { + + /** + * The ARN of the LogGroup. + */ + public readonly logGroupArn: string; + + constructor(scope: cdk.Construct, id: string, props: LogRetentionProps) { + super(scope, id); + + // Custom resource provider + const provider = this.providerSingletonFunction(props); + + // Need to use a CfnResource here to prevent lerna dependency cycles + // @aws-cdk/aws-cloudformation -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation + const retryOptions = props.logRetentionRetryOptions; + const resource = new cdk.CfnResource(this, 'Resource', { + type: 'Custom::LogRetention', + properties: { + ServiceToken: provider.getAtt('Arn'), + LogGroupName: props.logGroupName, + SdkRetry: retryOptions ? { + maxRetries: retryOptions.maxRetries, + base: retryOptions.base?.toMilliseconds(), + } : undefined, + RetentionInDays: props.retention === RetentionDays.INFINITE ? undefined : props.retention, + }, + }); + + const logGroupName = resource.getAtt('LogGroupName').toString(); + // Append ':*' at the end of the ARN to match with how CloudFormation does this for LogGroup ARNs + // See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-loggroup.html#aws-resource-logs-loggroup-return-values + this.logGroupArn = cdk.Stack.of(this).formatArn({ + service: 'logs', + resource: 'log-group', + resourceName: `${logGroupName}:*`, + sep: ':', + }); + } + + private providerSingletonFunction(props: LogRetentionProps) { + const stack = cdk.Stack.of(this); + const functionConstructName = 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a'; + const existing = stack.node.tryFindChild(functionConstructName); + if (existing) { + // Just assume this is true + return existing as cdk.CfnResource; + } + + // Provider Role + const role = props.role || new iam.Role(stack, `${functionConstructName}ServiceRole`, { + assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), + managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole')], + }); + // Duplicate statements will be deduplicated by `PolicyDocument` + role.addToPrincipalPolicy(new iam.PolicyStatement({ + actions: ['logs:PutRetentionPolicy', 'logs:DeleteRetentionPolicy'], + // We need '*' here because we will also put a retention policy on + // the log group of the provider function. Referencing it's name + // creates a CF circular dependency. + resources: ['*'], + })); + + // Provider Singleton Function + const asset = new s3_assets.Asset(this, `${functionConstructName}Code`, { + path: path.join(__dirname, 'log-retention-provider'), + }); + const provider = new cdk.CfnResource(stack, functionConstructName, { + type: 'AWS::Lambda::Function', + properties: { + Code: { + S3Bucket: asset.s3BucketName, + S3Key: asset.s3ObjectKey, + }, + Handler: 'index.handler', + Role: role.roleArn, + Runtime: 'nodejs10.x', + }, + }); + + // Function dependencies + role.node.children.forEach((child) => { + if (cdk.CfnResource.isCfnResource(child)) { + provider.addDependsOn(child); + } + if (cdk.Construct.isConstruct(child) && child.node.defaultChild && cdk.CfnResource.isCfnResource(child.node.defaultChild)) { + provider.addDependsOn(child.node.defaultChild); + } + }); + + return provider; + } +} diff --git a/packages/@aws-cdk/aws-logs/package.json b/packages/@aws-cdk/aws-logs/package.json index c7dc3e58d9753..61202d66bf535 100644 --- a/packages/@aws-cdk/aws-logs/package.json +++ b/packages/@aws-cdk/aws-logs/package.json @@ -64,15 +64,20 @@ "devDependencies": { "@aws-cdk/assert": "0.0.0", "@types/nodeunit": "^0.0.31", + "aws-sdk": "^2.715.0", + "aws-sdk-mock": "^5.1.0", "cdk-build-tools": "0.0.0", "cdk-integ-tools": "0.0.0", "cfn2ts": "0.0.0", + "nock": "^13.0.2", "nodeunit": "^0.11.3", - "pkglint": "0.0.0" + "pkglint": "0.0.0", + "sinon": "^9.0.2" }, "dependencies": { "@aws-cdk/aws-cloudwatch": "0.0.0", "@aws-cdk/aws-iam": "0.0.0", + "@aws-cdk/aws-s3-assets": "0.0.0", "@aws-cdk/core": "0.0.0", "constructs": "^3.0.2" }, @@ -80,6 +85,7 @@ "peerDependencies": { "@aws-cdk/aws-cloudwatch": "0.0.0", "@aws-cdk/aws-iam": "0.0.0", + "@aws-cdk/aws-s3-assets": "0.0.0", "@aws-cdk/core": "0.0.0", "constructs": "^3.0.2" }, diff --git a/packages/@aws-cdk/aws-lambda/test/test.log-retention-provider.ts b/packages/@aws-cdk/aws-logs/test/test.log-retention-provider.ts similarity index 100% rename from packages/@aws-cdk/aws-lambda/test/test.log-retention-provider.ts rename to packages/@aws-cdk/aws-logs/test/test.log-retention-provider.ts diff --git a/packages/@aws-cdk/aws-lambda/test/test.log-retention.ts b/packages/@aws-cdk/aws-logs/test/test.log-retention.ts similarity index 89% rename from packages/@aws-cdk/aws-lambda/test/test.log-retention.ts rename to packages/@aws-cdk/aws-logs/test/test.log-retention.ts index b06c6089982ba..7c58995806597 100644 --- a/packages/@aws-cdk/aws-lambda/test/test.log-retention.ts +++ b/packages/@aws-cdk/aws-logs/test/test.log-retention.ts @@ -1,8 +1,8 @@ import { ABSENT, countResources, expect, haveResource } from '@aws-cdk/assert'; import * as iam from '@aws-cdk/aws-iam'; -import * as logs from '@aws-cdk/aws-logs'; import * as cdk from '@aws-cdk/core'; import { Test } from 'nodeunit'; +import { RetentionDays } from '../lib'; import { LogRetention } from '../lib/log-retention'; /* eslint-disable quote-props */ @@ -15,7 +15,7 @@ export = { // WHEN new LogRetention(stack, 'MyLambda', { logGroupName: 'group', - retention: logs.RetentionDays.ONE_MONTH, + retention: RetentionDays.ONE_MONTH, }); // THEN @@ -33,10 +33,10 @@ export = { ], 'Version': '2012-10-17', }, - 'PolicyName': 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB', + 'PolicyName': 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64', 'Roles': [ { - 'Ref': 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB', + 'Ref': 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0', }, ], })); @@ -44,7 +44,7 @@ export = { expect(stack).to(haveResource('Custom::LogRetention', { 'ServiceToken': { 'Fn::GetAtt': [ - 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A', + 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a', 'Arn', ], }, @@ -64,7 +64,7 @@ export = { // WHEN new LogRetention(stack, 'MyLambda', { logGroupName: 'group', - retention: logs.RetentionDays.ONE_MONTH, + retention: RetentionDays.ONE_MONTH, role, }); @@ -100,7 +100,7 @@ export = { new LogRetention(stack, 'MyLambda', { logGroupName: 'group', - retention: logs.RetentionDays.INFINITE, + retention: RetentionDays.INFINITE, }); expect(stack).to(haveResource('Custom::LogRetention', { @@ -114,7 +114,7 @@ export = { const stack = new cdk.Stack(); const group = new LogRetention(stack, 'MyLambda', { logGroupName: 'group', - retention: logs.RetentionDays.ONE_MONTH, + retention: RetentionDays.ONE_MONTH, }); const logGroupArn = group.logGroupArn; From 56459f467307665efb85cdd9aef7c31f88264f2b Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Tue, 18 Aug 2020 20:04:32 +0100 Subject: [PATCH 02/12] update log retention logical ids in integ.instance.lit.expected.json --- .../test/integ.instance.lit.expected.json | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json b/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json index 9f591e2399a62..05381bbad77c9 100644 --- a/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json +++ b/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json @@ -701,7 +701,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", "Arn" ] }, @@ -725,7 +725,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", "Arn" ] }, @@ -749,7 +749,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", "Arn" ] }, @@ -773,7 +773,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", "Arn" ] }, @@ -907,7 +907,7 @@ ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0": { "Type": "AWS::IAM::Role", "Properties": { "AssumeRolePolicyDocument": { @@ -938,7 +938,7 @@ ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64": { "Type": "AWS::IAM::Policy", "Properties": { "PolicyDocument": { @@ -954,15 +954,15 @@ ], "Version": "2012-10-17" }, - "PolicyName": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB", + "PolicyName": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64", "Roles": [ { - "Ref": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB" + "Ref": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0" } ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a": { "Type": "AWS::Lambda::Function", "Properties": { "Code": { @@ -1006,15 +1006,15 @@ "Handler": "index.handler", "Role": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0", "Arn" ] }, "Runtime": "nodejs10.x" }, "DependsOn": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB", - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB" + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0" ] }, "HighCPU94686517": { From a7e6ea39942c242eae5b543cd28ec8179db5d5fb Mon Sep 17 00:00:00 2001 From: Ahmed Kamel Date: Mon, 24 Aug 2020 11:15:29 +0100 Subject: [PATCH 03/12] ensure logical ids remain unchanged --- .../test/integ.log-retention.expected.json | 14 ++-- .../@aws-cdk/aws-logs/lib/log-retention.ts | 69 +++++++++++-------- .../aws-logs/test/test.log-retention.ts | 4 +- .../test/integ.instance.lit.expected.json | 14 ++-- 4 files changed, 55 insertions(+), 46 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json index 1aff749c96c42..836d0b77d081f 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json @@ -73,7 +73,7 @@ "RetentionInDays": 7 } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB": { "Type": "AWS::IAM::Role", "Properties": { "AssumeRolePolicyDocument": { @@ -104,7 +104,7 @@ ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB": { "Type": "AWS::IAM::Policy", "Properties": { "PolicyDocument": { @@ -120,10 +120,10 @@ ], "Version": "2012-10-17" }, - "PolicyName": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64", + "PolicyName": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB", "Roles": [ { - "Ref": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0" + "Ref": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB" } ] } @@ -172,15 +172,15 @@ "Handler": "index.handler", "Role": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB", "Arn" ] }, "Runtime": "nodejs10.x" }, "DependsOn": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64", - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0" + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB" ] }, "OneMonthServiceRoleFBD1064F": { diff --git a/packages/@aws-cdk/aws-logs/lib/log-retention.ts b/packages/@aws-cdk/aws-logs/lib/log-retention.ts index 439d28b01e4eb..d273ec1e586ca 100644 --- a/packages/@aws-cdk/aws-logs/lib/log-retention.ts +++ b/packages/@aws-cdk/aws-logs/lib/log-retention.ts @@ -67,7 +67,7 @@ export class LogRetention extends cdk.Construct { super(scope, id); // Custom resource provider - const provider = this.providerSingletonFunction(props); + const provider = this.ensureSingletonLogRetentionFunction(props); // Need to use a CfnResource here to prevent lerna dependency cycles // @aws-cdk/aws-cloudformation -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation @@ -96,17 +96,44 @@ export class LogRetention extends cdk.Construct { }); } - private providerSingletonFunction(props: LogRetentionProps) { - const stack = cdk.Stack.of(this); - const functionConstructName = 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a'; - const existing = stack.node.tryFindChild(functionConstructName); + /** + * This a helper method to ensure that only one instance of LogRetentionFunction resources are in the stack + */ + private ensureSingletonLogRetentionFunction(props: LogRetentionProps) { + const functionLogicalId = 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a'; + const existing = cdk.Stack.of(this).node.tryFindChild(functionLogicalId); if (existing) { // Just assume this is true - return existing as cdk.CfnResource; + return existing as LogRetentionFunction; } + return new LogRetentionFunction(cdk.Stack.of(this), functionLogicalId, props); + } +} + +/** + * This is a private Lambda function to support the log retention custom resource. + */ +class LogRetentionFunction extends cdk.CfnResource { + constructor(scope: cdk.Construct, id: string, props: LogRetentionProps) { + super(scope, id, { + type: 'AWS::Lambda::Function', + properties: { + Handler: 'index.handler', + Runtime: 'nodejs10.x', + }, + }); - // Provider Role - const role = props.role || new iam.Role(stack, `${functionConstructName}ServiceRole`, { + // Code + const asset = new s3_assets.Asset(this, 'Code', { + path: path.join(__dirname, 'log-retention-provider'), + }); + this.addPropertyOverride('Code', { + S3Bucket: asset.s3BucketName, + S3Key: asset.s3ObjectKey, + }); + + // Role + const role = props.role || new iam.Role(this, 'ServiceRole', { assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole')], }); @@ -114,38 +141,20 @@ export class LogRetention extends cdk.Construct { role.addToPrincipalPolicy(new iam.PolicyStatement({ actions: ['logs:PutRetentionPolicy', 'logs:DeleteRetentionPolicy'], // We need '*' here because we will also put a retention policy on - // the log group of the provider function. Referencing it's name + // the log group of the provider function. Referencing its name // creates a CF circular dependency. resources: ['*'], })); - - // Provider Singleton Function - const asset = new s3_assets.Asset(this, `${functionConstructName}Code`, { - path: path.join(__dirname, 'log-retention-provider'), - }); - const provider = new cdk.CfnResource(stack, functionConstructName, { - type: 'AWS::Lambda::Function', - properties: { - Code: { - S3Bucket: asset.s3BucketName, - S3Key: asset.s3ObjectKey, - }, - Handler: 'index.handler', - Role: role.roleArn, - Runtime: 'nodejs10.x', - }, - }); + this.addPropertyOverride('Role', role.roleArn); // Function dependencies role.node.children.forEach((child) => { if (cdk.CfnResource.isCfnResource(child)) { - provider.addDependsOn(child); + this.addDependsOn(child); } if (cdk.Construct.isConstruct(child) && child.node.defaultChild && cdk.CfnResource.isCfnResource(child.node.defaultChild)) { - provider.addDependsOn(child.node.defaultChild); + this.addDependsOn(child.node.defaultChild); } }); - - return provider; } } diff --git a/packages/@aws-cdk/aws-logs/test/test.log-retention.ts b/packages/@aws-cdk/aws-logs/test/test.log-retention.ts index 7c58995806597..1865b3b32e154 100644 --- a/packages/@aws-cdk/aws-logs/test/test.log-retention.ts +++ b/packages/@aws-cdk/aws-logs/test/test.log-retention.ts @@ -33,10 +33,10 @@ export = { ], 'Version': '2012-10-17', }, - 'PolicyName': 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64', + 'PolicyName': 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB', 'Roles': [ { - 'Ref': 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0', + 'Ref': 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB', }, ], })); diff --git a/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json b/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json index 05381bbad77c9..5086e10683c3c 100644 --- a/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json +++ b/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json @@ -907,7 +907,7 @@ ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB": { "Type": "AWS::IAM::Role", "Properties": { "AssumeRolePolicyDocument": { @@ -938,7 +938,7 @@ ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB": { "Type": "AWS::IAM::Policy", "Properties": { "PolicyDocument": { @@ -954,10 +954,10 @@ ], "Version": "2012-10-17" }, - "PolicyName": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64", + "PolicyName": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB", "Roles": [ { - "Ref": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0" + "Ref": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB" } ] } @@ -1006,15 +1006,15 @@ "Handler": "index.handler", "Role": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB", "Arn" ] }, "Runtime": "nodejs10.x" }, "DependsOn": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicy13931C64", - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole0B18ADA0" + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB" ] }, "HighCPU94686517": { From 555d0df25453fb76cc566bec88ed840715b0e8cc Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Mon, 24 Aug 2020 11:55:52 +0100 Subject: [PATCH 04/12] fix rds's test.cluster --- packages/@aws-cdk/aws-rds/test/test.cluster.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-rds/test/test.cluster.ts b/packages/@aws-cdk/aws-rds/test/test.cluster.ts index 4414ed784b393..d9314a779a9e1 100644 --- a/packages/@aws-cdk/aws-rds/test/test.cluster.ts +++ b/packages/@aws-cdk/aws-rds/test/test.cluster.ts @@ -1134,7 +1134,7 @@ export = { expect(stack).to(haveResource('Custom::LogRetention', { ServiceToken: { 'Fn::GetAtt': [ - 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A', + 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a', 'Arn', ], }, @@ -1144,7 +1144,7 @@ export = { expect(stack).to(haveResource('Custom::LogRetention', { ServiceToken: { 'Fn::GetAtt': [ - 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A', + 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a', 'Arn', ], }, From ebbddc96efb2a16357dd5bee4a3d617421fdc5db Mon Sep 17 00:00:00 2001 From: Ahmed Kamel Date: Mon, 24 Aug 2020 14:05:52 +0100 Subject: [PATCH 05/12] use new types in lambda/rds while retaining backwards compatability --- packages/@aws-cdk/aws-lambda/lib/function.ts | 8 ++++---- packages/@aws-cdk/aws-lambda/lib/log-retention.ts | 2 +- packages/@aws-cdk/aws-rds/lib/cluster.ts | 2 +- packages/@aws-cdk/aws-rds/lib/instance.ts | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/function.ts b/packages/@aws-cdk/aws-lambda/lib/function.ts index 04150a2d86144..b790b7ce4fa3a 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function.ts @@ -14,7 +14,7 @@ import { calculateFunctionHash, trimFromStart } from './function-hash'; import { Version, VersionOptions } from './lambda-version'; import { CfnFunction } from './lambda.generated'; import { ILayerVersion } from './layers'; -import { LogRetention, LogRetentionRetryOptions } from './log-retention'; +import { LogRetentionRetryOptions } from './log-retention'; import { Runtime } from './runtime'; /** @@ -256,7 +256,7 @@ export interface FunctionOptions extends EventInvokeConfigOptions { * * @default - Default AWS SDK retry options. */ - readonly logRetentionRetryOptions?: LogRetentionRetryOptions; + readonly logRetentionRetryOptions?: LogRetentionRetryOptions | logs.LogRetentionRetryOptions; /** * Options for the `lambda.Version` resource automatically created by the @@ -633,7 +633,7 @@ export class Function extends FunctionBase { // Log retention if (props.logRetention) { - const logretention = new LogRetention(this, 'LogRetention', { + const logretention = new logs.LogRetention(this, 'LogRetention', { logGroupName: `/aws/lambda/${this.functionName}`, retention: props.logRetention, role: props.logRetentionRole, @@ -759,7 +759,7 @@ export class Function extends FunctionBase { */ public get logGroup(): logs.ILogGroup { if (!this._logGroup) { - const logretention = new LogRetention(this, 'LogRetention', { + const logretention = new logs.LogRetention(this, 'LogRetention', { logGroupName: `/aws/lambda/${this.functionName}`, retention: logs.RetentionDays.INFINITE, }); diff --git a/packages/@aws-cdk/aws-lambda/lib/log-retention.ts b/packages/@aws-cdk/aws-lambda/lib/log-retention.ts index fed62dc2563ba..12ba9e914bbf0 100644 --- a/packages/@aws-cdk/aws-lambda/lib/log-retention.ts +++ b/packages/@aws-cdk/aws-lambda/lib/log-retention.ts @@ -25,7 +25,7 @@ export interface LogRetentionProps extends logs.LogRetentionProps { * @deprecated use `LogRetention` from '@aws-cdk/aws-logs' instead */ export class LogRetention extends logs.LogRetention { - constructor(scope: cdk.Construct, id: string, props: LogRetentionProps) { + constructor(scope: cdk.Construct, id: string, props: LogRetentionProps | logs.LogRetentionProps) { super(scope, id, { ...props }); } } diff --git a/packages/@aws-cdk/aws-rds/lib/cluster.ts b/packages/@aws-cdk/aws-rds/lib/cluster.ts index 56075630e6e79..f82d6d8757b22 100644 --- a/packages/@aws-cdk/aws-rds/lib/cluster.ts +++ b/packages/@aws-cdk/aws-rds/lib/cluster.ts @@ -629,7 +629,7 @@ export class DatabaseCluster extends DatabaseClusterBase { if (props.cloudwatchLogsRetention) { for (const log of props.cloudwatchLogsExports) { - new lambda.LogRetention(this, `LogRetention${log}`, { + new logs.LogRetention(this, `LogRetention${log}`, { logGroupName: `/aws/rds/cluster/${this.clusterIdentifier}/${log}`, retention: props.cloudwatchLogsRetention, role: props.cloudwatchLogsRetentionRole, diff --git a/packages/@aws-cdk/aws-rds/lib/instance.ts b/packages/@aws-cdk/aws-rds/lib/instance.ts index dd8c100ca925b..4726796fcc5f4 100644 --- a/packages/@aws-cdk/aws-rds/lib/instance.ts +++ b/packages/@aws-cdk/aws-rds/lib/instance.ts @@ -594,7 +594,7 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData protected setLogRetention() { if (this.cloudwatchLogsExports && this.cloudwatchLogsRetention) { for (const log of this.cloudwatchLogsExports) { - new lambda.LogRetention(this, `LogRetention${log}`, { + new logs.LogRetention(this, `LogRetention${log}`, { logGroupName: `/aws/rds/instance/${this.instanceIdentifier}/${log}`, retention: this.cloudwatchLogsRetention, role: this.cloudwatchLogsRetentionRole, From 6bfec3d35d8942294bb2aa3266306b447dfbbcc0 Mon Sep 17 00:00:00 2001 From: Ahmed Kamel Date: Mon, 24 Aug 2020 14:29:54 +0100 Subject: [PATCH 06/12] enure lambda's logRetentionRetryOptions is of correct type --- packages/@aws-cdk/aws-lambda/lib/function.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/function.ts b/packages/@aws-cdk/aws-lambda/lib/function.ts index b790b7ce4fa3a..874621b7610c6 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function.ts @@ -637,7 +637,7 @@ export class Function extends FunctionBase { logGroupName: `/aws/lambda/${this.functionName}`, retention: props.logRetention, role: props.logRetentionRole, - logRetentionRetryOptions: props.logRetentionRetryOptions, + logRetentionRetryOptions: {...props.logRetentionRetryOptions}, }); this._logGroup = logs.LogGroup.fromLogGroupArn(this, 'LogGroup', logretention.logGroupArn); } From 521e0ad73cd0c5b18adc12d9345b0fe2b6e53ba7 Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Mon, 24 Aug 2020 14:58:07 +0100 Subject: [PATCH 07/12] use correct props in lambda's LogRetention --- packages/@aws-cdk/aws-lambda/lib/function.ts | 2 +- .../@aws-cdk/aws-lambda/lib/log-retention.ts | 2 +- .../@aws-cdk/aws-logs/lib/log-retention.ts | 34 ++++++++++--------- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/function.ts b/packages/@aws-cdk/aws-lambda/lib/function.ts index 874621b7610c6..586420006b33d 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function.ts @@ -637,7 +637,7 @@ export class Function extends FunctionBase { logGroupName: `/aws/lambda/${this.functionName}`, retention: props.logRetention, role: props.logRetentionRole, - logRetentionRetryOptions: {...props.logRetentionRetryOptions}, + logRetentionRetryOptions: { ...props.logRetentionRetryOptions }, }); this._logGroup = logs.LogGroup.fromLogGroupArn(this, 'LogGroup', logretention.logGroupArn); } diff --git a/packages/@aws-cdk/aws-lambda/lib/log-retention.ts b/packages/@aws-cdk/aws-lambda/lib/log-retention.ts index 12ba9e914bbf0..fed62dc2563ba 100644 --- a/packages/@aws-cdk/aws-lambda/lib/log-retention.ts +++ b/packages/@aws-cdk/aws-lambda/lib/log-retention.ts @@ -25,7 +25,7 @@ export interface LogRetentionProps extends logs.LogRetentionProps { * @deprecated use `LogRetention` from '@aws-cdk/aws-logs' instead */ export class LogRetention extends logs.LogRetention { - constructor(scope: cdk.Construct, id: string, props: LogRetentionProps | logs.LogRetentionProps) { + constructor(scope: cdk.Construct, id: string, props: LogRetentionProps) { super(scope, id, { ...props }); } } diff --git a/packages/@aws-cdk/aws-logs/lib/log-retention.ts b/packages/@aws-cdk/aws-logs/lib/log-retention.ts index d273ec1e586ca..f232c148b9540 100644 --- a/packages/@aws-cdk/aws-logs/lib/log-retention.ts +++ b/packages/@aws-cdk/aws-logs/lib/log-retention.ts @@ -111,26 +111,16 @@ export class LogRetention extends cdk.Construct { } /** - * This is a private Lambda function to support the log retention custom resource. + * This is a private provider Lambda function to support the log retention custom resource. */ -class LogRetentionFunction extends cdk.CfnResource { +class LogRetentionFunction extends cdk.Construct { constructor(scope: cdk.Construct, id: string, props: LogRetentionProps) { - super(scope, id, { - type: 'AWS::Lambda::Function', - properties: { - Handler: 'index.handler', - Runtime: 'nodejs10.x', - }, - }); + super(scope, id); // Code const asset = new s3_assets.Asset(this, 'Code', { path: path.join(__dirname, 'log-retention-provider'), }); - this.addPropertyOverride('Code', { - S3Bucket: asset.s3BucketName, - S3Key: asset.s3ObjectKey, - }); // Role const role = props.role || new iam.Role(this, 'ServiceRole', { @@ -145,15 +135,27 @@ class LogRetentionFunction extends cdk.CfnResource { // creates a CF circular dependency. resources: ['*'], })); - this.addPropertyOverride('Role', role.roleArn); + + const resource = new cdk.CfnResource(this, 'Resource', { + type: 'AWS::Lambda::Function', + properties: { + Handler: 'index.handler', + Runtime: 'nodejs10.x', + Code: { + S3Bucket: asset.s3BucketName, + S3Key: asset.s3ObjectKey, + }, + Role: role.roleArn, + }, + }); // Function dependencies role.node.children.forEach((child) => { if (cdk.CfnResource.isCfnResource(child)) { - this.addDependsOn(child); + resource.addDependsOn(child); } if (cdk.Construct.isConstruct(child) && child.node.defaultChild && cdk.CfnResource.isCfnResource(child.node.defaultChild)) { - this.addDependsOn(child.node.defaultChild); + resource.addDependsOn(child.node.defaultChild); } }); } From d3d3d4e64ba5b37f1695bc05d064a94571822fbe Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Mon, 24 Aug 2020 15:42:50 +0100 Subject: [PATCH 08/12] fix lambda's logical Id and retrieving its arn --- .../aws-lambda/test/integ.log-retention.expected.json | 8 ++++---- packages/@aws-cdk/aws-logs/lib/log-retention.ts | 11 ++++++++--- packages/@aws-cdk/aws-logs/test/test.log-retention.ts | 2 +- packages/@aws-cdk/aws-rds/test/test.cluster.ts | 4 ++-- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json index 836d0b77d081f..f123d24edf60a 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.log-retention.expected.json @@ -55,7 +55,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", "Arn" ] }, @@ -128,7 +128,7 @@ ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A": { "Type": "AWS::Lambda::Function", "Properties": { "Code": { @@ -238,7 +238,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", "Arn" ] }, @@ -311,7 +311,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", "Arn" ] }, diff --git a/packages/@aws-cdk/aws-logs/lib/log-retention.ts b/packages/@aws-cdk/aws-logs/lib/log-retention.ts index f232c148b9540..8d48786b6fb5f 100644 --- a/packages/@aws-cdk/aws-logs/lib/log-retention.ts +++ b/packages/@aws-cdk/aws-logs/lib/log-retention.ts @@ -75,7 +75,7 @@ export class LogRetention extends cdk.Construct { const resource = new cdk.CfnResource(this, 'Resource', { type: 'Custom::LogRetention', properties: { - ServiceToken: provider.getAtt('Arn'), + ServiceToken: provider.functionArn, LogGroupName: props.logGroupName, SdkRetry: retryOptions ? { maxRetries: retryOptions.maxRetries, @@ -97,7 +97,8 @@ export class LogRetention extends cdk.Construct { } /** - * This a helper method to ensure that only one instance of LogRetentionFunction resources are in the stack + * Helper method to ensure that only one instance of LogRetentionFunction resources are in the stack mimicking the + * behaviour of @aws-cdk/aws-lambda's SingletonFunction to prevent circular dependencies */ private ensureSingletonLogRetentionFunction(props: LogRetentionProps) { const functionLogicalId = 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a'; @@ -111,9 +112,11 @@ export class LogRetention extends cdk.Construct { } /** - * This is a private provider Lambda function to support the log retention custom resource. + * Private provider Lambda function to support the log retention custom resource. */ class LogRetentionFunction extends cdk.Construct { + public readonly functionArn: cdk.Reference; + constructor(scope: cdk.Construct, id: string, props: LogRetentionProps) { super(scope, id); @@ -136,6 +139,7 @@ class LogRetentionFunction extends cdk.Construct { resources: ['*'], })); + // Lambda function const resource = new cdk.CfnResource(this, 'Resource', { type: 'AWS::Lambda::Function', properties: { @@ -148,6 +152,7 @@ class LogRetentionFunction extends cdk.Construct { Role: role.roleArn, }, }); + this.functionArn = resource.getAtt('Arn'); // Function dependencies role.node.children.forEach((child) => { diff --git a/packages/@aws-cdk/aws-logs/test/test.log-retention.ts b/packages/@aws-cdk/aws-logs/test/test.log-retention.ts index 1865b3b32e154..5f7047fc2e820 100644 --- a/packages/@aws-cdk/aws-logs/test/test.log-retention.ts +++ b/packages/@aws-cdk/aws-logs/test/test.log-retention.ts @@ -44,7 +44,7 @@ export = { expect(stack).to(haveResource('Custom::LogRetention', { 'ServiceToken': { 'Fn::GetAtt': [ - 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a', + 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A', 'Arn', ], }, diff --git a/packages/@aws-cdk/aws-rds/test/test.cluster.ts b/packages/@aws-cdk/aws-rds/test/test.cluster.ts index d9314a779a9e1..4414ed784b393 100644 --- a/packages/@aws-cdk/aws-rds/test/test.cluster.ts +++ b/packages/@aws-cdk/aws-rds/test/test.cluster.ts @@ -1134,7 +1134,7 @@ export = { expect(stack).to(haveResource('Custom::LogRetention', { ServiceToken: { 'Fn::GetAtt': [ - 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a', + 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A', 'Arn', ], }, @@ -1144,7 +1144,7 @@ export = { expect(stack).to(haveResource('Custom::LogRetention', { ServiceToken: { 'Fn::GetAtt': [ - 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a', + 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A', 'Arn', ], }, From 5bd90ceea9c6a962318a73f441ca7aaa2a2ac8bf Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Mon, 24 Aug 2020 16:34:44 +0100 Subject: [PATCH 09/12] fix lambda's logRetentionRetryOptions type --- packages/@aws-cdk/aws-lambda/lib/function.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/function.ts b/packages/@aws-cdk/aws-lambda/lib/function.ts index 586420006b33d..b29a563317f6e 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function.ts @@ -633,13 +633,13 @@ export class Function extends FunctionBase { // Log retention if (props.logRetention) { - const logretention = new logs.LogRetention(this, 'LogRetention', { + const logRetention = new logs.LogRetention(this, 'LogRetention', { logGroupName: `/aws/lambda/${this.functionName}`, retention: props.logRetention, role: props.logRetentionRole, - logRetentionRetryOptions: { ...props.logRetentionRetryOptions }, + logRetentionRetryOptions: props.logRetentionRetryOptions as logs.LogRetentionRetryOptions, }); - this._logGroup = logs.LogGroup.fromLogGroupArn(this, 'LogGroup', logretention.logGroupArn); + this._logGroup = logs.LogGroup.fromLogGroupArn(this, 'LogGroup', logRetention.logGroupArn); } props.code.bindToResource(resource); @@ -759,11 +759,11 @@ export class Function extends FunctionBase { */ public get logGroup(): logs.ILogGroup { if (!this._logGroup) { - const logretention = new logs.LogRetention(this, 'LogRetention', { + const logRetention = new logs.LogRetention(this, 'LogRetention', { logGroupName: `/aws/lambda/${this.functionName}`, retention: logs.RetentionDays.INFINITE, }); - this._logGroup = logs.LogGroup.fromLogGroupArn(this, `${this.node.id}-LogGroup`, logretention.logGroupArn); + this._logGroup = logs.LogGroup.fromLogGroupArn(this, `${this.node.id}-LogGroup`, logRetention.logGroupArn); } return this._logGroup; } From c99ab9a52d85bcce2177478fe8f9c068fbe209ed Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Mon, 24 Aug 2020 16:56:47 +0100 Subject: [PATCH 10/12] remove unused lambda import/dependency from rds --- packages/@aws-cdk/aws-rds/lib/cluster.ts | 1 - packages/@aws-cdk/aws-rds/lib/instance.ts | 1 - packages/@aws-cdk/aws-rds/package.json | 3 +-- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/@aws-cdk/aws-rds/lib/cluster.ts b/packages/@aws-cdk/aws-rds/lib/cluster.ts index f82d6d8757b22..1f8a0c090dbfe 100644 --- a/packages/@aws-cdk/aws-rds/lib/cluster.ts +++ b/packages/@aws-cdk/aws-rds/lib/cluster.ts @@ -1,7 +1,6 @@ import * as ec2 from '@aws-cdk/aws-ec2'; import { IRole, ManagedPolicy, Role, ServicePrincipal } from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import * as lambda from '@aws-cdk/aws-lambda'; import * as logs from '@aws-cdk/aws-logs'; import * as s3 from '@aws-cdk/aws-s3'; import * as secretsmanager from '@aws-cdk/aws-secretsmanager'; diff --git a/packages/@aws-cdk/aws-rds/lib/instance.ts b/packages/@aws-cdk/aws-rds/lib/instance.ts index 4726796fcc5f4..7135a84592fd5 100644 --- a/packages/@aws-cdk/aws-rds/lib/instance.ts +++ b/packages/@aws-cdk/aws-rds/lib/instance.ts @@ -2,7 +2,6 @@ import * as ec2 from '@aws-cdk/aws-ec2'; import * as events from '@aws-cdk/aws-events'; import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import * as lambda from '@aws-cdk/aws-lambda'; import * as logs from '@aws-cdk/aws-logs'; import * as secretsmanager from '@aws-cdk/aws-secretsmanager'; import { CfnDeletionPolicy, Construct, Duration, IResource, Lazy, RemovalPolicy, Resource, SecretValue, Stack, Token } from '@aws-cdk/core'; diff --git a/packages/@aws-cdk/aws-rds/package.json b/packages/@aws-cdk/aws-rds/package.json index df41aac87053b..45169f5574c90 100644 --- a/packages/@aws-cdk/aws-rds/package.json +++ b/packages/@aws-cdk/aws-rds/package.json @@ -64,6 +64,7 @@ "devDependencies": { "@aws-cdk/assert": "0.0.0", "@aws-cdk/aws-events-targets": "0.0.0", + "@aws-cdk/aws-lambda": "0.0.0", "@types/nodeunit": "^0.0.31", "cdk-build-tools": "0.0.0", "cdk-integ-tools": "0.0.0", @@ -77,7 +78,6 @@ "@aws-cdk/aws-events": "0.0.0", "@aws-cdk/aws-iam": "0.0.0", "@aws-cdk/aws-kms": "0.0.0", - "@aws-cdk/aws-lambda": "0.0.0", "@aws-cdk/aws-logs": "0.0.0", "@aws-cdk/aws-s3": "0.0.0", "@aws-cdk/aws-secretsmanager": "0.0.0", @@ -91,7 +91,6 @@ "@aws-cdk/aws-events": "0.0.0", "@aws-cdk/aws-iam": "0.0.0", "@aws-cdk/aws-kms": "0.0.0", - "@aws-cdk/aws-lambda": "0.0.0", "@aws-cdk/aws-logs": "0.0.0", "@aws-cdk/aws-secretsmanager": "0.0.0", "@aws-cdk/core": "0.0.0", From 2bf1c6356d2370d14c17ca67cd47d0886447d9ae Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Mon, 24 Aug 2020 17:00:06 +0100 Subject: [PATCH 11/12] fix integ.instance.lit in rds --- .../aws-rds/test/integ.instance.lit.expected.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json b/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json index 5086e10683c3c..9f591e2399a62 100644 --- a/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json +++ b/packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json @@ -701,7 +701,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", "Arn" ] }, @@ -725,7 +725,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", "Arn" ] }, @@ -749,7 +749,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", "Arn" ] }, @@ -773,7 +773,7 @@ "Properties": { "ServiceToken": { "Fn::GetAtt": [ - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a", + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A", "Arn" ] }, @@ -962,7 +962,7 @@ ] } }, - "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a": { + "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A": { "Type": "AWS::Lambda::Function", "Properties": { "Code": { From ca17c63e372306ed6e5524c22775a85484da773b Mon Sep 17 00:00:00 2001 From: Ahmed Sobhi Date: Tue, 25 Aug 2020 12:06:08 +0100 Subject: [PATCH 12/12] address comments --- packages/@aws-cdk/aws-lambda/lib/function.ts | 2 +- packages/@aws-cdk/aws-logs/lib/log-retention.ts | 1 - packages/@aws-cdk/aws-logs/test/test.log-retention.ts | 3 +-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/function.ts b/packages/@aws-cdk/aws-lambda/lib/function.ts index b29a563317f6e..97b70b700db3f 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function.ts @@ -256,7 +256,7 @@ export interface FunctionOptions extends EventInvokeConfigOptions { * * @default - Default AWS SDK retry options. */ - readonly logRetentionRetryOptions?: LogRetentionRetryOptions | logs.LogRetentionRetryOptions; + readonly logRetentionRetryOptions?: LogRetentionRetryOptions; /** * Options for the `lambda.Version` resource automatically created by the diff --git a/packages/@aws-cdk/aws-logs/lib/log-retention.ts b/packages/@aws-cdk/aws-logs/lib/log-retention.ts index 8d48786b6fb5f..4a83d46ac01ff 100644 --- a/packages/@aws-cdk/aws-logs/lib/log-retention.ts +++ b/packages/@aws-cdk/aws-logs/lib/log-retention.ts @@ -104,7 +104,6 @@ export class LogRetention extends cdk.Construct { const functionLogicalId = 'LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a'; const existing = cdk.Stack.of(this).node.tryFindChild(functionLogicalId); if (existing) { - // Just assume this is true return existing as LogRetentionFunction; } return new LogRetentionFunction(cdk.Stack.of(this), functionLogicalId, props); diff --git a/packages/@aws-cdk/aws-logs/test/test.log-retention.ts b/packages/@aws-cdk/aws-logs/test/test.log-retention.ts index 5f7047fc2e820..c6139b11bd635 100644 --- a/packages/@aws-cdk/aws-logs/test/test.log-retention.ts +++ b/packages/@aws-cdk/aws-logs/test/test.log-retention.ts @@ -2,8 +2,7 @@ import { ABSENT, countResources, expect, haveResource } from '@aws-cdk/assert'; import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; import { Test } from 'nodeunit'; -import { RetentionDays } from '../lib'; -import { LogRetention } from '../lib/log-retention'; +import { LogRetention, RetentionDays } from '../lib'; /* eslint-disable quote-props */