From 6aa87aea4c7bfb7c2d387dffe1d97cf8fdc1db54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Wed, 27 May 2020 10:41:49 +0200 Subject: [PATCH 1/3] fix(dynamodb): old global table replicas cannot be deleted The permissions required to clean up old DynamoDB Global Tables replicas were set up in such a way that removing a replication region, or dropping replication entirely (or when cuasing a table replacement), they were removed before CloudFormation gets to the `CLEAN_UP` phase, causing a clean up failure (and old tables would remain there). This changes the way permissions are granted to the replication handler resource so that they are added using a separate `iam.Policy` resource, so that deleted permissions are also removed during the `CLEAN_UP` phase after the resources depending on them have been deleted. The tradeoff is that two additional resources are added to the stack that defines the DynamoDB Global Tables, where previously those permissions were mastered in the nested stack that holds the replication handler. Unofrtunately, the nested stack gets it's `CLEAN_UP` phase executed as part of the nested stack resource update, not during it's parent stack's `CLEAN_UP` phase. Fixes #7189 --- packages/@aws-cdk/aws-dynamodb/lib/table.ts | 65 +++++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-dynamodb/lib/table.ts b/packages/@aws-cdk/aws-dynamodb/lib/table.ts index f63ff13a25cf4..de92a30120236 100644 --- a/packages/@aws-cdk/aws-dynamodb/lib/table.ts +++ b/packages/@aws-cdk/aws-dynamodb/lib/table.ts @@ -2,7 +2,10 @@ import * as appscaling from '@aws-cdk/aws-applicationautoscaling'; import * as cloudwatch from '@aws-cdk/aws-cloudwatch'; import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import { Aws, CfnCondition, CfnCustomResource, Construct, CustomResource, Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; +import { + Aws, CfnCondition, CfnCustomResource, Construct, CustomResource, Fn, + IResource, Lazy, RemovalPolicy, Resource, Stack, Token, +} from '@aws-cdk/core'; import { CfnTable, CfnTableProps } from './dynamodb.generated'; import * as perms from './perms'; import { ReplicaProvider } from './replica-provider'; @@ -911,7 +914,7 @@ export class Table extends TableBase { this.tableSortKey = props.sortKey; } - if (props.replicationRegions) { + if (props.replicationRegions && props.replicationRegions.length > 0) { this.createReplicaTables(props.replicationRegions); } } @@ -1225,9 +1228,12 @@ export class Table extends TableBase { // Documentation at https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/V2gt_IAM.html // is currently incorrect. AWS Support recommends `dynamodb:*` in both source and destination regions + const onEventHandlerPolicy = new SourceTableAttachedPolicy(this, provider.onEventHandler.role!); + const isCompleteHandlerPolicy = new SourceTableAttachedPolicy(this, provider.isCompleteHandler.role!); + // Permissions in the source region - this.grant(provider.onEventHandler, 'dynamodb:*'); - this.grant(provider.isCompleteHandler, 'dynamodb:DescribeTable'); + this.grant(onEventHandlerPolicy, 'dynamodb:*'); + this.grant(isCompleteHandlerPolicy, 'dynamodb:DescribeTable'); let previousRegion; for (const region of new Set(regions)) { // Remove duplicates @@ -1241,6 +1247,10 @@ export class Table extends TableBase { Region: region, }, }); + currentRegion.node.addDependency( + onEventHandlerPolicy.policy, + isCompleteHandlerPolicy.policy, + ); // Deploy time check to prevent from creating a replica in the region // where this stack is deployed. Only needed for environment agnostic @@ -1272,7 +1282,7 @@ export class Table extends TableBase { // Permissions in the destination regions (outside of the loop to // minimize statements in the policy) - provider.onEventHandler.addToRolePolicy(new iam.PolicyStatement({ + onEventHandlerPolicy.grantPrincipal.addToPolicy(new iam.PolicyStatement({ actions: ['dynamodb:*'], resources: this.regionalArns, })); @@ -1408,3 +1418,48 @@ interface ScalableAttributePair { scalableReadAttribute?: ScalableTableAttribute; scalableWriteAttribute?: ScalableTableAttribute; } + +/** + * An inline policy that is logically bound to the source table of a DynamoDB Global Tables + * "cluster“. This is here to ensure permissions are removed as part of (and not before) the + * CleanUp phase of a stack update, when a replica is removed (or the entire "cluster" gets + * replaced). + * + * If statements are added directly to the handler roles (as opposed to in a separate inline + * policy resource), new permissions are in effect before clean up happens, and so replicas that + * need to be dropped can no longer be due to lack of permissions. + */ +class SourceTableAttachedPolicy extends Construct implements iam.IGrantable { + public readonly grantPrincipal: iam.IPrincipal; + public readonly policy: iam.IPolicy; + + public constructor(sourceTable: Table, role: iam.IRole) { + super(sourceTable, `SourceTableAttachedPolicy-${role.node.uniqueId}`); + + const policy = new iam.Policy(this, 'Resource', { roles: [role] }); + this.policy = policy; + this.grantPrincipal = new SourceTableAttachedPrincipal(role, policy); + } +} + +/** + * An `IPrincipal` entity that can be used as the target of `grant` calls, used by the + * `SourceTableAttachedPolicy` class so it can act as an `IGrantable`. + */ +class SourceTableAttachedPrincipal extends iam.PrincipalBase { + public constructor(private readonly role: iam.IRole, private readonly policy: iam.Policy) { + super(); + } + + public get policyFragment(): iam.PrincipalPolicyFragment { + return this.role.policyFragment; + } + + public addToPrincipalPolicy(statement: iam.PolicyStatement): iam.AddToPrincipalPolicyResult { + this.policy.addStatements(statement); + return { + policyDependable: this.policy, + statementAdded: true, + }; + } +} From 7447585159b6c6fb6e990f4be126b68a2abcbd08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Thu, 28 May 2020 11:09:19 +0200 Subject: [PATCH 2/3] update test expectations --- .../test/integ.global.expected.json | 169 ++++++++++++++++-- 1 file changed, 150 insertions(+), 19 deletions(-) diff --git a/packages/@aws-cdk/aws-dynamodb/test/integ.global.expected.json b/packages/@aws-cdk/aws-dynamodb/test/integ.global.expected.json index dc4b5ce676ce6..9057e8c7ae31b 100644 --- a/packages/@aws-cdk/aws-dynamodb/test/integ.global.expected.json +++ b/packages/@aws-cdk/aws-dynamodb/test/integ.global.expected.json @@ -41,6 +41,140 @@ "UpdateReplacePolicy": "Delete", "DeletionPolicy": "Delete" }, + "TableSourceTableAttachedPolicycdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderOnEventHandlerServiceRole6F43DF4AA4E210EA": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": "dynamodb:*", + "Effect": "Allow", + "Resource": [ + { + "Fn::GetAtt": [ + "TableCD117FA1", + "Arn" + ] + }, + { + "Fn::Join": [ + "", + [ + { + "Fn::GetAtt": [ + "TableCD117FA1", + "Arn" + ] + }, + "/index/*" + ] + ] + } + ] + }, + { + "Action": "dynamodb:*", + "Effect": "Allow", + "Resource": [ + { + "Fn::Join": [ + "", + [ + "arn:", + { + "Ref": "AWS::Partition" + }, + ":dynamodb:eu-west-2:", + { + "Ref": "AWS::AccountId" + }, + ":table/", + { + "Ref": "TableCD117FA1" + } + ] + ] + }, + { + "Fn::Join": [ + "", + [ + "arn:", + { + "Ref": "AWS::Partition" + }, + ":dynamodb:eu-central-1:", + { + "Ref": "AWS::AccountId" + }, + ":table/", + { + "Ref": "TableCD117FA1" + } + ] + ] + } + ] + } + ], + "Version": "2012-10-17" + }, + "PolicyName": "TableSourceTableAttachedPolicycdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderOnEventHandlerServiceRole6F43DF4AA4E210EA", + "Roles": [ + { + "Fn::GetAtt": [ + "awscdkawsdynamodbReplicaProviderNestedStackawscdkawsdynamodbReplicaProviderNestedStackResource18E3F12D", + "Outputs.cdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderOnEventHandlerServiceRole3E8625F3Ref" + ] + } + ] + } + }, + "TableSourceTableAttachedPolicycdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderIsCompleteHandlerServiceRole397161288F61AAFA": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action": "dynamodb:DescribeTable", + "Effect": "Allow", + "Resource": [ + { + "Fn::GetAtt": [ + "TableCD117FA1", + "Arn" + ] + }, + { + "Fn::Join": [ + "", + [ + { + "Fn::GetAtt": [ + "TableCD117FA1", + "Arn" + ] + }, + "/index/*" + ] + ] + } + ] + } + ], + "Version": "2012-10-17" + }, + "PolicyName": "leSourceTableAttachedPolicycdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderIsCompleteHandlerServiceRole397161288F61AAFA", + "Roles": [ + { + "Fn::GetAtt": [ + "awscdkawsdynamodbReplicaProviderNestedStackawscdkawsdynamodbReplicaProviderNestedStackResource18E3F12D", + "Outputs.cdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderIsCompleteHandlerServiceRole2F936EC4Ref" + ] + } + ] + } + }, "TableReplicaeuwest290D3CD3A": { "Type": "Custom::DynamoDBReplica", "Properties": { @@ -55,6 +189,10 @@ }, "Region": "eu-west-2" }, + "DependsOn": [ + "TableSourceTableAttachedPolicycdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderIsCompleteHandlerServiceRole397161288F61AAFA", + "TableSourceTableAttachedPolicycdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderOnEventHandlerServiceRole6F43DF4AA4E210EA" + ], "UpdateReplacePolicy": "Delete", "DeletionPolicy": "Delete" }, @@ -73,7 +211,9 @@ "Region": "eu-central-1" }, "DependsOn": [ - "TableReplicaeuwest290D3CD3A" + "TableReplicaeuwest290D3CD3A", + "TableSourceTableAttachedPolicycdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderIsCompleteHandlerServiceRole397161288F61AAFA", + "TableSourceTableAttachedPolicycdkdynamodbglobal20191121awscdkawsdynamodbReplicaProviderOnEventHandlerServiceRole6F43DF4AA4E210EA" ], "UpdateReplacePolicy": "Delete", "DeletionPolicy": "Delete" @@ -91,7 +231,7 @@ }, "/", { - "Ref": "AssetParameters1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510S3BucketCE06C497" + "Ref": "AssetParametersffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947S3Bucket5148F39F" }, "/", { @@ -101,7 +241,7 @@ "Fn::Split": [ "||", { - "Ref": "AssetParameters1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510S3VersionKey6B6B0A66" + "Ref": "AssetParametersffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947S3VersionKey0618C4C3" } ] } @@ -114,7 +254,7 @@ "Fn::Split": [ "||", { - "Ref": "AssetParameters1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510S3VersionKey6B6B0A66" + "Ref": "AssetParametersffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947S3VersionKey0618C4C3" } ] } @@ -124,15 +264,6 @@ ] }, "Parameters": { - "referencetocdkdynamodbglobal20191121TableB640876BArn": { - "Fn::GetAtt": [ - "TableCD117FA1", - "Arn" - ] - }, - "referencetocdkdynamodbglobal20191121TableB640876BRef": { - "Ref": "TableCD117FA1" - }, "referencetocdkdynamodbglobal20191121AssetParameters012c6b101abc4ea1f510921af61a3e08e05f30f84d7b35c40ca4adb1ace60746S3BucketE0999323Ref": { "Ref": "AssetParameters012c6b101abc4ea1f510921af61a3e08e05f30f84d7b35c40ca4adb1ace60746S3BucketBDDEC9DD" }, @@ -174,17 +305,17 @@ "Type": "String", "Description": "Artifact hash for asset \"5e49cf64d8027f48872790f80cdb76c5b836ecf9a70b71be1eb937a5c25a47c1\"" }, - "AssetParameters1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510S3BucketCE06C497": { + "AssetParametersffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947S3Bucket5148F39F": { "Type": "String", - "Description": "S3 bucket for asset \"1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510\"" + "Description": "S3 bucket for asset \"ffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947\"" }, - "AssetParameters1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510S3VersionKey6B6B0A66": { + "AssetParametersffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947S3VersionKey0618C4C3": { "Type": "String", - "Description": "S3 key for asset version \"1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510\"" + "Description": "S3 key for asset version \"ffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947\"" }, - "AssetParameters1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510ArtifactHashAB28BC52": { + "AssetParametersffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947ArtifactHashBF6B619B": { "Type": "String", - "Description": "Artifact hash for asset \"1e7110d85a2e13b58c2a0fb09f018c144489abfafc62bf10f8ab3561a9cb8510\"" + "Description": "Artifact hash for asset \"ffa367e57788c5b58cfac966968712006cbe11cfd301e6c94eb067350f8de947\"" } } } \ No newline at end of file From 53e475aebfaa85257f99272bcb723ff5b68f1674 Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Mon, 8 Jun 2020 09:19:32 +0200 Subject: [PATCH 3/3] Update packages/@aws-cdk/aws-dynamodb/lib/table.ts Co-authored-by: Niranjan Jayakar --- packages/@aws-cdk/aws-dynamodb/lib/table.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-dynamodb/lib/table.ts b/packages/@aws-cdk/aws-dynamodb/lib/table.ts index de92a30120236..0bd932b78a1b0 100644 --- a/packages/@aws-cdk/aws-dynamodb/lib/table.ts +++ b/packages/@aws-cdk/aws-dynamodb/lib/table.ts @@ -1421,7 +1421,7 @@ interface ScalableAttributePair { /** * An inline policy that is logically bound to the source table of a DynamoDB Global Tables - * "cluster“. This is here to ensure permissions are removed as part of (and not before) the + * "cluster". This is here to ensure permissions are removed as part of (and not before) the * CleanUp phase of a stack update, when a replica is removed (or the entire "cluster" gets * replaced). *