fix(redshift): UserTablePriviliges to track changes using table IDs (#26955)

To support solving #24246, there needs to first be a refactor of the UserTablePriviliges to instead record the table id. The reason being that new privileges would be granted/revoked on old tables that now have new names, since a change in table name caused an UPDATE action to be triggered. However the issue remains where, since the table has a new name, the grant/revoke action will be called on an invalid table name (i.e. the old table name).
We now use the table id to track tables, therefore preventing UPDATE events to be triggered.

This blocks #24308

This was originally PR #24874, however that had closed. @kaizencc requested changes, that I had added in here.

This was originally PR #26558, however that had closed. @kaizencc requested changes, that I have already implemented previously. However, he did not review them.

BREAKING CHANGE: the behavior of redshift tables has changed. UPDATE action will not be triggered on new table names and instead be triggered on table id changes.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Author: Rizxcviii

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

@@ -1,9 +1,9 @@
 /* eslint-disable-next-line import/no-unresolved */
 import * as AWSLambda from 'aws-lambda';
+import { TablePrivilege, UserTablePrivilegesHandlerProps } from '../handler-props';
 import { executeStatement } from './redshift-data';
 import { ClusterProps } from './types';
 import { makePhysicalId } from './util';
-import { TablePrivilege, UserTablePrivilegesHandlerProps } from '../handler-props';
 
 export async function handler(props: UserTablePrivilegesHandlerProps & ClusterProps, event: AWSLambda.CloudFormationCustomResourceEvent) {
   const username = props.username;
@@ -62,10 +62,26 @@ 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 }) => (
+    tablePrivileges.find(({ tableId: otherTableId, actions: otherActions }) => (
+      tableId === otherTableId && actions.some(action => !otherActions.includes(action))
+    ))
+  ));
+  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

@@ -1,11 +1,11 @@
 import * as cdk from 'aws-cdk-lib/core';
 import { Construct } from 'constructs';
-import { DatabaseQuery } from './database-query';
-import { HandlerName } from './database-query-provider/handler-name';
-import { TablePrivilege as SerializedTablePrivilege, UserTablePrivilegesHandlerProps } from './handler-props';
 import { DatabaseOptions } from '../database-options';
 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';
 
 /**
  * The Redshift table and action that make up a privilege that can be granted to a Redshift user.
@@ -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.node.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/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';
@@ -147,22 +149,99 @@ 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,
     };
 
+    // Checking if the table resource has not been recreated
     await expect(managePrivileges(newResourceProperties, event)).resolves.toMatchObject({
       PhysicalResourceId: physicalResourceId,
     });
+    // Upon a table name change, Redshift maintains the same table priviliges as before.
+    // The name of the table has changed, a new table has not been created.
+    // Therefore 'REVOKE' statements should not be used.
+    expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
+      Sql: `REVOKE INSERT, SELECT ON ${newTableName} FROM ${username}`,
+    }));
+    // Likewise, here the table name has changed, so the current priviliges will still be intact.
+    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,
+    };
+
+    // Checking if the table resource has not been recreated
+    await expect(managePrivileges(newResourceProperties, event)).resolves.toMatchObject({
+      PhysicalResourceId: physicalResourceId,
+    });
+    // Old actions are REVOKED, as they are not included in the list
     expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
       Sql: `REVOKE INSERT, SELECT ON ${tableName} FROM ${username}`,
     }));
+    // New actions are GRANTED, as they are included in the list
     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,
+    };
+
+    // Checking if the table resource has not been recreated, we are not changing on table id either.
+    // Due to the construct only needing to be changed on a new user, not a new table
+    await expect(managePrivileges(newResourceProperties, event)).resolves.toMatchObject({
+      PhysicalResourceId: physicalResourceId,
+    });
+    // Upon removal of the old table, the priviliges will also be revoked automatically,
+    // as the table no longer exists.
+    // Calling REVOKE statments on a non-existing table will throw errors by Redshift
+    expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
+      Sql: expect.stringMatching(new RegExp(`REVOKE .+ ON ${tableName} FROM ${username}`)),
+    }));
+    // Adds the permissions onto the newly created table
+    expect(mockExecuteStatement).toHaveBeenCalledWith(expect.objectContaining({
+      Sql: expect.stringMatching(new RegExp(`GRANT .+ ON ${tableName} TO ${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 }],
+      },
+    };
+
+    // Checking if the table resource has not been recreated
+    await expect(managePrivileges(newResourceProperties, newEvent)).resolves.toMatchObject({
+      PhysicalResourceId: physicalResourceId,
+    });
+    // Upon initial deployment from non table id usage to table id usage,
+    // permissions would not need to be granted/revoked, as the table should already exist
+    expect(mockExecuteStatement).not.toHaveBeenCalledWith(expect.objectContaining({
+      Sql: expect.stringMatching(new RegExp(`.+ ON ${tableName} FROM ${username}`)),
+    }));
+  });
+});