fix(redshift-alpha): extract tableName from custom resource functions

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

@@ -11,17 +11,18 @@ export async function handler(props: UserTablePrivilegesHandlerProps & ClusterPr
   const clusterProps = props;
 
   if (event.RequestType === 'Create') {
-    await grantPrivileges(username, tablePrivileges, clusterProps);
+    await grantPrivileges(username, tablePrivileges, clusterProps, event.StackId);
     return { PhysicalResourceId: makePhysicalId(username, clusterProps, event.RequestId) };
   } else if (event.RequestType === 'Delete') {
-    await revokePrivileges(username, tablePrivileges, clusterProps);
+    await revokePrivileges(username, tablePrivileges, clusterProps, event.StackId);
     return;
   } else if (event.RequestType === 'Update') {
     const { replace } = await updatePrivileges(
       username,
       tablePrivileges,
       clusterProps,
       event.OldResourceProperties as unknown as UserTablePrivilegesHandlerProps & ClusterProps,
+      event.StackId,
     );
     const physicalId = replace ? makePhysicalId(username, clusterProps, event.RequestId) : event.PhysicalResourceId;
     return { PhysicalResourceId: physicalId };
@@ -31,19 +32,35 @@ export async function handler(props: UserTablePrivilegesHandlerProps & ClusterPr
   }
 }
 
-async function revokePrivileges(username: string, tablePrivileges: TablePrivilege[], clusterProps: ClusterProps) {
+async function revokePrivileges(
+  username: string,
+  tablePrivileges: TablePrivilege[],
+  clusterProps: ClusterProps,
+  stackId: string,
+) {
   // Limited by human input
   // eslint-disable-next-line @cdklabs/promiseall-no-unbounded-parallelism
   await Promise.all(tablePrivileges.map(({ tableName, actions }) => {
-    return executeStatement(`REVOKE ${actions.join(', ')} ON ${tableName} FROM ${username}`, clusterProps);
+    return executeStatement(
+      `REVOKE ${actions.join(', ')} ON ${normalizedTableName(tableName, stackId)} FROM ${username}`,
+      clusterProps,
+    );
   }));
 }
 
-async function grantPrivileges(username: string, tablePrivileges: TablePrivilege[], clusterProps: ClusterProps) {
+async function grantPrivileges(
+  username: string,
+  tablePrivileges: TablePrivilege[],
+  clusterProps: ClusterProps,
+  stackId: string,
+) {
   // Limited by human input
   // eslint-disable-next-line @cdklabs/promiseall-no-unbounded-parallelism
   await Promise.all(tablePrivileges.map(({ tableName, actions }) => {
-    return executeStatement(`GRANT ${actions.join(', ')} ON ${tableName} TO ${username}`, clusterProps);
+    return executeStatement(
+      `GRANT ${actions.join(', ')} ON ${normalizedTableName(tableName, stackId)} TO ${username}`,
+      clusterProps,
+    );
   }));
 }
 
@@ -52,16 +69,17 @@ async function updatePrivileges(
   tablePrivileges: TablePrivilege[],
   clusterProps: ClusterProps,
   oldResourceProperties: UserTablePrivilegesHandlerProps & ClusterProps,
+  stackId: string,
 ): Promise<{ replace: boolean }> {
   const oldClusterProps = oldResourceProperties;
   if (clusterProps.clusterName !== oldClusterProps.clusterName || clusterProps.databaseName !== oldClusterProps.databaseName) {
-    await grantPrivileges(username, tablePrivileges, clusterProps);
+    await grantPrivileges(username, tablePrivileges, clusterProps, stackId);
     return { replace: true };
   }
 
   const oldUsername = oldResourceProperties.username;
   if (oldUsername !== username) {
-    await grantPrivileges(username, tablePrivileges, clusterProps);
+    await grantPrivileges(username, tablePrivileges, clusterProps, stackId);
     return { replace: true };
   }
 
@@ -72,7 +90,7 @@ async function updatePrivileges(
     ))
   ));
   if (tablesToRevoke.length > 0) {
-    await revokePrivileges(username, tablesToRevoke, clusterProps);
+    await revokePrivileges(username, tablesToRevoke, clusterProps, stackId);
   }
 
   const tablesToGrant = tablePrivileges.filter(({ tableId, tableName, actions }) => {
@@ -85,8 +103,21 @@ async function updatePrivileges(
     return tableAdded || actionsAdded;
   });
   if (tablesToGrant.length > 0) {
-    await grantPrivileges(username, tablesToGrant, clusterProps);
+    await grantPrivileges(username, tablesToGrant, clusterProps, stackId);
   }
 
   return { replace: false };
 }
+
+/**
+ * We need this normalization logic because some of the `TableName` values
+ * are physical IDs generated in the `./util.ts` module.
+ * */
+const normalizedTableName = (tableName: string, stackId: string): string => {
+  const segments = tableName.split(':');
+  const suffix = segments.slice(-1);
+  if (suffix != null && stackId.endsWith(suffix[0])) {
+    return segments.slice(-2)[0] ?? tableName;
+  }
+  return tableName;
+};

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

@@ -15,7 +15,7 @@ export async function handler(props: TableAndClusterProps, event: AWSLambda.Clou
 
   if (event.RequestType === 'Create') {
     tableName = await createTable(tableNamePrefix, getTableNameSuffix(props.tableName.generateSuffix), tableColumns, tableAndClusterProps);
-    return { PhysicalResourceId: makePhysicalId(tableNamePrefix, tableAndClusterProps, event.StackId.substring(event.StackId.length - 12)) };
+    return { PhysicalResourceId: makePhysicalId(tableName, tableAndClusterProps, event.StackId.substring(event.StackId.length - 12)) };
   } else if (event.RequestType === 'Delete') {
     await dropTable(
       event.PhysicalResourceId.includes(event.StackId.substring(event.StackId.length - 12)) ? tableName : event.PhysicalResourceId,

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

@@ -5,7 +5,7 @@ import { ITable, TableAction } from '../table';
 import { IUser } from '../user';
 import { DatabaseQuery } from './database-query';
 import { HandlerName } from './database-query-provider/handler-name';
-import { TablePrivilege as SerializedTablePrivilege, UserTablePrivilegesHandlerProps } from './handler-props';
+import { UserTablePrivilegesHandlerProps } from './handler-props';
 
 /**
  * The Redshift table and action that make up a privilege that can be granted to a Redshift user.
@@ -62,33 +62,25 @@ export class UserTablePrivileges extends Construct {
         username: props.user.username,
         tablePrivileges: cdk.Lazy.any({
           produce: () => {
-            const reducedPrivileges = this.privileges.reduce((privileges, { table, actions }) => {
-              const tableId = table.node.id;
-              if (!(tableId in privileges)) {
-                privileges[tableId] = {
+            const groupedPrivileges = this.privileges.reduce(
+              (privileges, { table, actions }) => ({
+                ...privileges,
+                [table.node.id]: {
+                  actions: [
+                    ...(privileges[table.node.id]?.actions ?? []),
+                    ...actions,
+                  ],
                   tableName: table.tableName,
-                  actions: [],
-                };
-              }
-              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[tableId] = {
-                tableName: table.tableName,
-                actions: Array.from(new Set(actions)),
-              };
-              return privileges;
-            }, {} as { [key: string]: { tableName: string; actions: TableAction[] } });
-            const serializedPrivileges: SerializedTablePrivilege[] = Object.entries(reducedPrivileges).map(([tableId, config]) => ({
+                },
+              }),
+              {} as Record<string, { tableName: string; actions: TableAction[] }>,
+            );
+
+            return Object.entries(groupedPrivileges).map(([tableId, config]) => ({
               tableId,
               tableName: config.tableName,
-              actions: config.actions.map(action => TableAction[action]),
+              actions: unifyTableActions(config.actions).map(action => TableAction[action]),
             }));
-            return serializedPrivileges;
           },
         }) as any,
       },
@@ -102,3 +94,17 @@ export class UserTablePrivileges extends Construct {
     this.privileges.push({ table, actions });
   }
 }
+
+const unifyTableActions = (tableActions: TableAction[]): TableAction[] => {
+  const set = new Set<TableAction>(tableActions);
+
+  if (set.has(TableAction.ALL)) {
+    return [TableAction.ALL];
+  }
+
+  if (set.has(TableAction.UPDATE) || set.has(TableAction.DELETE)) {
+    set.add(TableAction.SELECT);
+  }
+
+  return [...set];
+};

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

@@ -272,7 +272,7 @@ export class Table extends TableBase {
       properties: {
         tableName: {
           prefix: props.tableName ?? cdk.Names.uniqueId(this),
-          generateSuffix: !props.tableName ? 'true' : 'false',
+          generateSuffix: (props.tableName == null).toString(),
         },
         tableColumns: this.tableColumns,
         distStyle: props.distStyle,
@@ -282,7 +282,7 @@ export class Table extends TableBase {
       },
     });
 
-    this.tableName = this.resource.ref;
+    this.tableName = props.tableName ?? this.resource.ref;
   }
 
   /**

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

@@ -40,6 +40,7 @@ jest.mock('@aws-sdk/client-redshift-data', () => {
 });
 
 import { handler as managePrivileges } from '../../lib/private/database-query-provider/privileges';
+import { makePhysicalId } from '../../lib/private/database-query-provider/util';
 
 beforeEach(() => {
   jest.clearAllMocks();
@@ -61,6 +62,31 @@ describe('create', () => {
       Sql: `GRANT INSERT, SELECT ON ${tableName} TO ${username}`,
     }));
   });
+
+  test('serializes properties in statement when tableName in physical resource ID', async () => {
+    const properties = {
+      ...resourceProperties,
+      tablePrivileges: [{
+        tableId,
+        tableName: `${makePhysicalId(tableName, resourceProperties, requestId)}`,
+        actions,
+      }],
+    };
+
+    const event = {
+      ...baseEvent,
+      ResourceProperties: properties,
+      StackId: 'xxxxx:' + requestId,
+    };
+
+    await expect(managePrivileges(properties, event)).resolves.toEqual({
+      PhysicalResourceId: 'clusterName:databaseName:username:requestId',
+    });
+
+    expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
+      Sql: `GRANT INSERT, SELECT ON ${tableName} TO ${username}`,
+    }));
+  });
 });
 
 describe('delete', () => {
@@ -79,6 +105,29 @@ describe('delete', () => {
       Sql: `REVOKE INSERT, SELECT ON ${tableName} FROM ${username}`,
     }));
   });
+
+  test('serializes properties in statement when tableName in physical resource ID', async () => {
+    const properties = {
+      ...resourceProperties,
+      tablePrivileges: [{
+        tableId,
+        tableName: `${makePhysicalId(tableName, resourceProperties, requestId)}`,
+        actions,
+      }],
+    };
+
+    const event = {
+      ...baseEvent,
+      ResourceProperties: properties,
+      StackId: 'xxxxx:' + requestId,
+    };
+
+    await managePrivileges(properties, event);
+
+    expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
+      Sql: `REVOKE INSERT, SELECT ON ${tableName} FROM ${username}`,
+    }));
+  });
 });
 
 describe('update', () => {
@@ -244,4 +293,50 @@ describe('update', () => {
       Sql: expect.stringMatching(new RegExp(`.+ ON ${tableName} FROM ${username}`)),
     }));
   });
+
+  test('serializes properties in grant statement when tableName in physical resource ID', async () => {
+    const properties = {
+      ...resourceProperties,
+      tablePrivileges: [{
+        tableId,
+        tableName: `${makePhysicalId(tableName, resourceProperties, requestId)}`,
+        actions,
+      }],
+    };
+
+    const newEvent = {
+      ...event,
+      ResourceProperties: properties,
+      StackId: 'xxxxx:' + requestId,
+    };
+
+    await managePrivileges(properties, newEvent);
+
+    expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
+      Sql: `GRANT INSERT, SELECT ON ${tableName} TO ${username}`,
+    }));
+  });
+
+  test('serializes properties in drop statement when tableName in physical resource ID', async () => {
+    const properties = {
+      ...resourceProperties,
+      tablePrivileges: [{
+        tableId,
+        tableName: `${makePhysicalId(tableName, resourceProperties, requestId)}`,
+        actions: ['DROP'],
+      }],
+    };
+
+    const newEvent = {
+      ...event,
+      ResourceProperties: properties,
+      StackId: 'xxxxx:' + requestId,
+    };
+
+    await managePrivileges(properties, newEvent);
+
+    expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
+      Sql: `REVOKE INSERT, SELECT ON ${tableName} FROM ${username}`,
+    }));
+  });
 });

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

@@ -61,7 +61,7 @@ describe('create', () => {
     const event = baseEvent;
 
     await expect(manageTable(resourceProperties, event)).resolves.toEqual({
-      PhysicalResourceId: 'clusterName:databaseName:tableNamePrefix:111111111111',
+      PhysicalResourceId: 'clusterName:databaseName:tableNamePrefix111111111111:111111111111',
     });
     expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
       Sql: `CREATE TABLE ${tableNamePrefix}${stackIdTruncated} (col1 varchar(1))`,