fix(dynamodb): changing waitForReplicationToFinish fails deployment (#17842)

skinny85 · web-flow · commit 36b8fdb026c7 · 2021-12-06T10:45:24.000Z
Our CFN Custom Resource for global DynamoDB Tables did not correctly handle changing the `waitForReplicationToFinish` parameter introduced in #16983 - adding it to an existing replica would error out by attempting to re-create the existing replica. Fix this by adding a check in the Custom Resource whether a replica already exists. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
diff --git a/packages/@aws-cdk/aws-dynamodb/lib/replica-handler/index.ts b/packages/@aws-cdk/aws-dynamodb/lib/replica-handler/index.ts
@@ -7,31 +7,45 @@ export async function onEventHandler(event: OnEventRequest): Promise<OnEventResp
 
   const dynamodb = new DynamoDB();
 
-  let updateTableAction: 'Create' | 'Update' | 'Delete';
+  const tableName = event.ResourceProperties.TableName;
+  const region = event.ResourceProperties.Region;
+
+  let updateTableAction: 'Create' | 'Update' | 'Delete' | undefined;
   if (event.RequestType === 'Create' || event.RequestType === 'Delete') {
     updateTableAction = event.RequestType;
   } else { // Update
-    // This can only be a table replacement so we create a replica
-    // in the new table. The replica for the "old" table will be
-    // deleted when CF issues a Delete event on the old physical
-    // resource id.
-    updateTableAction = 'Create';
+    // There are two cases where an Update can happen:
+    // 1. A table replacement. In that case, we need to create the replica in the new Table
+    // (the replica for the "old" Table will be deleted when CFN issues a Delete event on the old physical resource id).
+    // 2. A customer has changed one of the properties of the Custom Resource,
+    // like 'waitForReplicationToFinish'. In that case, we don't have to do anything.
+    // To differentiate the two cases, we make an API call to DynamoDB to check whether a replica already exists.
+    const describeTableResult = await dynamodb.describeTable({
+      TableName: tableName,
+    }).promise();
+    console.log('Describe table: %j', describeTableResult);
+    const replicaExists = describeTableResult.Table?.Replicas?.some(replica => replica.RegionName === region);
+    updateTableAction = replicaExists ? undefined : 'Create';
   }
 
-  const data = await dynamodb.updateTable({
-    TableName: event.ResourceProperties.TableName,
-    ReplicaUpdates: [
-      {
-        [updateTableAction]: {
-          RegionName: event.ResourceProperties.Region,
+  if (updateTableAction) {
+    const data = await dynamodb.updateTable({
+      TableName: tableName,
+      ReplicaUpdates: [
+        {
+          [updateTableAction]: {
+            RegionName: region,
+          },
         },
-      },
-    ],
-  }).promise();
-  console.log('Update table: %j', data);
+      ],
+    }).promise();
+    console.log('Update table: %j', data);
+  } else {
+    console.log("Skipping updating Table, as a replica in '%s' already exists", region);
+  }
 
   return event.RequestType === 'Create' || event.RequestType === 'Update'
-    ? { PhysicalResourceId: `${event.ResourceProperties.TableName}-${event.ResourceProperties.Region}` }
+    ? { PhysicalResourceId: `${tableName}-${region}` }
     : {};
 }
 
@@ -45,11 +59,11 @@ export async function isCompleteHandler(event: IsCompleteRequest): Promise<IsCom
   }).promise();
   console.log('Describe table: %j', data);
 
-  const tableActive = !!(data.Table?.TableStatus === 'ACTIVE');
+  const tableActive = data.Table?.TableStatus === 'ACTIVE';
   const replicas = data.Table?.Replicas ?? [];
   const regionReplica = replicas.find(r => r.RegionName === event.ResourceProperties.Region);
-  const replicaActive = !!(regionReplica?.ReplicaStatus === 'ACTIVE');
-  const skipReplicationCompletedWait = event.ResourceProperties.SkipReplicationCompletedWait ?? false;
+  const replicaActive = regionReplica?.ReplicaStatus === 'ACTIVE';
+  const skipReplicationCompletedWait = event.ResourceProperties.SkipReplicationCompletedWait === 'true';
 
   switch (event.RequestType) {
     case 'Create':
diff --git a/packages/@aws-cdk/aws-dynamodb/lib/table.ts b/packages/@aws-cdk/aws-dynamodb/lib/table.ts
@@ -1557,7 +1557,11 @@ export class Table extends TableBase {
         properties: {
           TableName: this.tableName,
           Region: region,
-          SkipReplicationCompletedWait: waitForReplicationToFinish === undefined ? undefined : !waitForReplicationToFinish,
+          SkipReplicationCompletedWait: waitForReplicationToFinish == null
+            ? undefined
+            // CFN changes Custom Resource properties to strings anyways,
+            // so let's do that ourselves to make it clear in the handler this is a string, not a boolean
+            : (!waitForReplicationToFinish).toString(),
         },
       });
       currentRegion.node.addDependency(
diff --git a/packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts b/packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts
@@ -2468,7 +2468,7 @@ describe('global', () => {
           Ref: 'TableCD117FA1',
         },
         Region: 'eu-west-2',
-        SkipReplicationCompletedWait: true,
+        SkipReplicationCompletedWait: 'true',
       },
       Condition: 'TableStackRegionNotEqualseuwest2A03859E7',
     }, ResourcePart.CompleteDefinition);
@@ -2479,7 +2479,7 @@ describe('global', () => {
           Ref: 'TableCD117FA1',
         },
         Region: 'eu-central-1',
-        SkipReplicationCompletedWait: true,
+        SkipReplicationCompletedWait: 'true',
       },
       Condition: 'TableStackRegionNotEqualseucentral199D46FC0',
     }, ResourcePart.CompleteDefinition);
diff --git a/packages/@aws-cdk/aws-dynamodb/test/replica-provider.test.ts b/packages/@aws-cdk/aws-dynamodb/test/replica-provider.test.ts
@@ -16,11 +16,13 @@ afterAll(() => {
 
 AWS.setSDK(require.resolve('aws-sdk'));
 
+const REGION = 'eu-west-2';
+
 const createEvent: OnEventRequest = {
   RequestType: 'Create',
   ResourceProperties: {
     TableName: 'my-table',
-    Region: 'eu-west-2',
+    Region: REGION,
     ServiceToken: 'token',
   },
   ServiceToken: 'token',
@@ -47,7 +49,7 @@ test('on event', async () => {
     ReplicaUpdates: [
       {
         Create: {
-          RegionName: 'eu-west-2',
+          RegionName: REGION,
         },
       },
     ],
@@ -58,9 +60,16 @@ test('on event', async () => {
   });
 });
 
-test('on event calls updateTable with Create for Update requests with table replacement', async () => {
-  const updateTableMock = sinon.fake.resolves({});
+test("on Update event from CFN calls updateTable with Create if a replica in the region doesn't exist", async () => {
+  AWS.mock('DynamoDB', 'describeTable', sinon.fake.resolves({
+    Table: {
+      Replicas: [
+        // no replicas exist yet
+      ],
+    },
+  }));
 
+  const updateTableMock = sinon.fake.resolves({});
   AWS.mock('DynamoDB', 'updateTable', updateTableMock);
 
   const data = await onEventHandler({
@@ -76,7 +85,7 @@ test('on event calls updateTable with Create for Update requests with table repl
     ReplicaUpdates: [
       {
         Create: {
-          RegionName: 'eu-west-2',
+          RegionName: REGION,
         },
       },
     ],
@@ -87,6 +96,29 @@ test('on event calls updateTable with Create for Update requests with table repl
   });
 });
 
+test("on Update event from CFN calls doesn't call updateTable if a replica in the region does exist", async () => {
+  AWS.mock('DynamoDB', 'describeTable', sinon.fake.resolves({
+    Table: {
+      Replicas: [
+        { RegionName: REGION },
+      ],
+    },
+  }));
+
+  const updateTableMock = sinon.fake.resolves({});
+  AWS.mock('DynamoDB', 'updateTable', updateTableMock);
+
+  await onEventHandler({
+    ...createEvent,
+    OldResourceProperties: {
+      TableName: 'my-old-table',
+    },
+    RequestType: 'Update',
+  });
+
+  sinon.assert.notCalled(updateTableMock);
+});
+
 test('on event calls updateTable with Delete', async () => {
   const updateTableMock = sinon.fake.resolves({});
 
@@ -102,7 +134,7 @@ test('on event calls updateTable with Delete', async () => {
     ReplicaUpdates: [
       {
         Delete: {
-          RegionName: 'eu-west-2',
+          RegionName: REGION,
         },
       },
     ],
@@ -129,7 +161,7 @@ test('is complete for create returns false when replica is not active', async ()
     Table: {
       Replicas: [
         {
-          RegionName: 'eu-west-2',
+          RegionName: REGION,
           ReplicaStatus: 'CREATING',
         },
       ],
@@ -148,7 +180,7 @@ test('is complete for create returns false when table is not active', async () =
     Table: {
       Replicas: [
         {
-          RegionName: 'eu-west-2',
+          RegionName: REGION,
           ReplicaStatus: 'ACTIVE',
         },
       ],
@@ -168,7 +200,7 @@ test('is complete for create returns true when replica is active', async () => {
     Table: {
       Replicas: [
         {
-          RegionName: 'eu-west-2',
+          RegionName: REGION,
           ReplicaStatus: 'ACTIVE',
         },
       ],
