Skip to content

Commit

Permalink
fix(dynamodb): old global table replicas cannot be deleted
Browse files Browse the repository at this point in the history
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
  • Loading branch information
RomainMuller committed May 27, 2020
1 parent f26063f commit 6aa87ae
Showing 1 changed file with 60 additions and 5 deletions.
65 changes: 60 additions & 5 deletions packages/@aws-cdk/aws-dynamodb/lib/table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
}));
Expand Down Expand Up @@ -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,
};
}
}

0 comments on commit 6aa87ae

Please sign in to comment.