refactor(redshift): UserTablePriviliges to track changes using table IDs'

packages/@aws-cdk/aws-redshift-alpha/lib/private/database-query-provider/privileges.ts

@@ -62,10 +62,28 @@ async function updatePrivileges(
   }
 
   const oldTablePrivileges = oldResourceProperties.tablePrivileges;
-  if (oldTablePrivileges !== tablePrivileges) {
-    await revokePrivileges(username, oldTablePrivileges, clusterProps);
-    await grantPrivileges(username, tablePrivileges, clusterProps);
-    return { replace: false };
+  const tablesToRevoke = oldTablePrivileges.filter(({ tableId, actions }) => {
+    const tableRemoved = !tablePrivileges.find(({ tableId: otherTableId }) => tableId === otherTableId);
+    const actionsRemoved = tablePrivileges.find(({ tableId: otherTableId, actions: otherActions }) => (
+      tableId === otherTableId && actions.some(action => !otherActions.includes(action))
+    ));
+    return tableRemoved || actionsRemoved;
+  });
+  if (tablesToRevoke.length > 0) {
+    await revokePrivileges(username, tablesToRevoke, clusterProps);
+  }
+
+  const tablesToGrant = tablePrivileges.filter(({ tableId, tableName, actions }) => {
+    const tableAdded = !oldTablePrivileges.find(({ tableId: otherTableId, tableName: otherTableName }) => (
+      tableId === otherTableId && tableName === otherTableName
+    ));
+    const actionsAdded = oldTablePrivileges.find(({ tableId: otherTableId, actions: otherActions }) => (
+      tableId === otherTableId && otherActions.some(action => !actions.includes(action))
+    ));
+    return tableAdded || actionsAdded;
+  });
+  if (tablesToGrant.length > 0) {
+    await grantPrivileges(username, tablesToGrant, clusterProps);
   }
 
   return { replace: false };

packages/@aws-cdk/aws-redshift-alpha/lib/private/handler-props.ts

@@ -25,6 +25,7 @@ export interface TableHandlerProps {
 }
 
 export interface TablePrivilege {
+  readonly tableId: string;
   readonly tableName: string;
   readonly actions: string[];
 }

packages/@aws-cdk/aws-redshift-alpha/lib/private/privileges.ts

@@ -63,23 +63,30 @@ export class UserTablePrivileges extends Construct {
         tablePrivileges: cdk.Lazy.any({
           produce: () => {
             const reducedPrivileges = this.privileges.reduce((privileges, { table, actions }) => {
-              const tableName = table.tableName;
-              if (!(tableName in privileges)) {
-                privileges[tableName] = [];
+              const tableId = table.id;
+              if (!(tableId in privileges)) {
+                privileges[tableId] = {
+                  tableName: table.tableName,
+                  actions: [],
+                };
               }
-              actions = actions.concat(privileges[tableName]);
+              actions = actions.concat(privileges[tableId].actions);
               if (actions.includes(TableAction.ALL)) {
                 actions = [TableAction.ALL];
               }
               if (actions.includes(TableAction.UPDATE) || actions.includes(TableAction.DELETE)) {
                 actions.push(TableAction.SELECT);
               }
-              privileges[tableName] = Array.from(new Set(actions));
+              privileges[tableId] = {
+                tableName: table.tableName,
+                actions: Array.from(new Set(actions)),
+              };
               return privileges;
-            }, {} as { [key: string]: TableAction[] });
-            const serializedPrivileges: SerializedTablePrivilege[] = Object.entries(reducedPrivileges).map(([tableName, actions]) => ({
-              tableName: tableName,
-              actions: actions.map(action => TableAction[action]),
+            }, {} as { [key: string]: { tableName: string, actions: TableAction[] } });
+            const serializedPrivileges: SerializedTablePrivilege[] = Object.entries(reducedPrivileges).map(([tableId, config]) => ({
+              tableId,
+              tableName: config.tableName,
+              actions: config.actions.map(action => TableAction[action]),
             }));
             return serializedPrivileges;
           },

packages/@aws-cdk/aws-redshift-alpha/lib/table.ts

@@ -155,6 +155,11 @@ export interface TableProps extends DatabaseOptions {
  * Represents a table in a Redshift database.
  */
 export interface ITable extends IConstruct {
+  /**
+   * Idenfifier of the table construct.
+   */
+  readonly id: string;
+
   /**
    * Name of the table.
    */
@@ -211,6 +216,7 @@ abstract class TableBase extends Construct implements ITable {
   abstract readonly tableColumns: Column[];
   abstract readonly cluster: ICluster;
   abstract readonly databaseName: string;
+  abstract readonly id: string;
   grant(user: IUser, ...actions: TableAction[]) {
     user.addTablePrivileges(this, ...actions);
   }
@@ -229,13 +235,15 @@ export class Table extends TableBase {
       readonly tableColumns = attrs.tableColumns;
       readonly cluster = attrs.cluster;
       readonly databaseName = attrs.databaseName;
+      readonly id = id;
     }(scope, id);
   }
 
   readonly tableName: string;
   readonly tableColumns: Column[];
   readonly cluster: ICluster;
   readonly databaseName: string;
+  readonly id: string;
 
   private resource: DatabaseQuery<TableHandlerProps>;
 
@@ -253,6 +261,7 @@ export class Table extends TableBase {
     this.tableColumns = this.configureTableColumns(props.tableColumns);
     this.cluster = props.cluster;
     this.databaseName = props.databaseName;
+    this.id = id;
 
     const useColumnIds = !!cdk.FeatureFlags.of(this).isEnabled(REDSHIFT_COLUMN_ID);
 

packages/@aws-cdk/aws-redshift-alpha/test/database-query-provider/privileges.test.ts

@@ -3,7 +3,9 @@ import type * as AWSLambda from 'aws-lambda';
 
 const username = 'username';
 const tableName = 'tableName';
-const tablePrivileges = [{ tableName, actions: ['INSERT', 'SELECT'] }];
+const tableId = 'tableId';
+const actions = ['INSERT', 'SELECT'];
+const tablePrivileges = [{ tableId, tableName, actions }];
 const clusterName = 'clusterName';
 const adminUserArn = 'adminUserArn';
 const databaseName = 'databaseName';
@@ -142,9 +144,27 @@ describe('update', () => {
     }));
   });
 
-  test('does not replace when privileges change', async () => {
+  test('does not replace when table name is changed', async () => {
     const newTableName = 'newTableName';
-    const newTablePrivileges = [{ tableName: newTableName, actions: ['DROP'] }];
+    const newTablePrivileges = [{ tableId, tableName: newTableName, actions }];
+    const newResourceProperties = {
+      ...resourceProperties,
+      tablePrivileges: newTablePrivileges,
+    };
+
+    await expect(managePrivileges(newResourceProperties, event)).resolves.toMatchObject({
+      PhysicalResourceId: physicalResourceId,
+    });
+    expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
+      Sql: `REVOKE INSERT, SELECT ON ${newTableName} FROM ${username}`,
+    }));
+    expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
+      Sql: expect.stringMatching(new RegExp(`.+ ON ${tableName} TO ${username}`)),
+    }));
+  });
+
+  test('does not replace when table actions are changed', async () => {
+    const newTablePrivileges = [{ tableId, tableName, actions: ['DROP'] }];
     const newResourceProperties = {
       ...resourceProperties,
       tablePrivileges: newTablePrivileges,
@@ -157,7 +177,46 @@ describe('update', () => {
       Sql: `REVOKE INSERT, SELECT ON ${tableName} FROM ${username}`,
     }));
     expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
-      Sql: `GRANT DROP ON ${newTableName} TO ${username}`,
+      Sql: `GRANT DROP ON ${tableName} TO ${username}`,
     }));
   });
-});
+
+  test('does not replace when table id is changed', async () => {
+    const newTableId = 'newTableId';
+    const newTablePrivileges = [{ tableId: newTableId, tableName, actions }];
+    const newResourceProperties = {
+      ...resourceProperties,
+      tablePrivileges: newTablePrivileges,
+    };
+
+    await expect(managePrivileges(newResourceProperties, event)).resolves.toMatchObject({
+      PhysicalResourceId: physicalResourceId,
+    });
+    expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
+      Sql: `.+ ON ${tableName} FROM ${username}`,
+    }));
+  });
+
+  test('does not replace when table id is appended', async () => {
+    const newTablePrivileges = [{ tableId: 'newTableId', tableName, actions }];
+    const newResourceProperties = {
+      ...resourceProperties,
+      tablePrivileges: newTablePrivileges,
+    };
+
+    const newEvent = {
+      ...event,
+      OldResourceProperties: {
+        ...event.OldResourceProperties,
+        tablePrivileges: [{ tableName, actions }],
+      },
+    };
+
+    await expect(managePrivileges(newResourceProperties, newEvent)).resolves.toMatchObject({
+      PhysicalResourceId: physicalResourceId,
+    });
+    expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
+      Sql: `.+ ON ${tableName} FROM ${username}`,
+    }));
+  });
+});