fix(redshift-alpha): use same role for database-query singleton function #32363

5d · 2024-12-02T20:59:36Z

Issue # (if applicable)

Closes #32089.

Reason for this change

The Redshift tables use a singleton function as the invoker for various custom resource onEvent Lambda functions. Currently, each custom resource lambda function has a dedicated IAM role to assume. However, since it’s the same singleton function, a shared role could achieve the same outcome.

Description of changes

Use the same IAM role for the singleton invoker function to assume.

Description of how you validated changes

deployed to my local stack

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

codecov · 2024-12-02T21:11:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.96%. Comparing base (de04742) to head (751a303).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32363   +/-   ##
=======================================
  Coverage   66.96%   66.96%           
=======================================
  Files         329      329           
  Lines       18663    18663           
  Branches     3258     3258           
=======================================
  Hits        12497    12497           
  Misses       5839     5839           
  Partials      327      327

Flag	Coverage Δ
suite.unit	`66.96% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.71% <ø> (ø)`
packages/aws-cdk-lib/core	`82.08% <ø> (ø)`

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

...shot/asset.c973f9a285ae54986627703f13cdd21d38dcdb3cf29620c5411d7d3baa53a3b9/redshift-data.js

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

godwingrs22

Thanks @5d for fixing this. Left some nit comments.

godwingrs22 · 2025-01-03T00:30:47Z

packages/aws-cdk-lib/aws-lambda/lib/singleton-lambda.ts

+  /**
+   * The name of the singleton function. It acts as a unique ID within its CDK stack.
+   * */
+  public readonly constructName: string;


nit: I think we can use the existing property itself using functionName to get the name of the function. Don't think we need a new property here.

From my understanding, the functionName of a singleton Lambda differs from the constructName used to identify the function within the stack. Additionally, the functionName is often a token and is typically only resolved at deployment time.

godwingrs22 · 2025-01-03T00:31:58Z

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

+   * We only need one function since it's just acting as a trigger.
+   * */
+  private getProviderRole(handler: lambda.SingletonFunction): iam.IRole {
+    const id = handler.constructName + 'ProviderRole';


nit: we could get the function name like below

const id = handler.functionName + 'ProviderRole';

godwingrs22 · 2025-01-03T00:36:30Z

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

+   * Get or create the IAM role for the singleton lambda function.
+   * We only need one function since it's just acting as a trigger.
+   * */
+  private getProviderRole(handler: lambda.SingletonFunction): iam.IRole {


nit: Since the method returns either an existing role or creates a new role, i think we can rename the method name to be getOrCreateLambdaProviderRole for better readability.

Can we have a unit test for this method to get an existing role and create a new role?

thanks, will update the function name. I will add the unit test case for it.

godwingrs22 · 2025-01-03T00:37:56Z

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

@@ -112,6 +112,8 @@ abstract class UserBase extends Construct implements IUser {
        ...this.databaseProps,
        user: this,
      });
+
+      this.privileges.node.addDependency(table);


just for my understanding, why do we need this to be add as dependency?

I've encountered errors during stack deletion when the table is removed before its associated privileges are deleted.

I suggest adding the reason as a code comment.

sure, will add a comment to it.

samson-keung · 2025-01-03T04:55:51Z

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

@@ -64,6 +64,7 @@ export class DatabaseQuery<HandlerProps> extends Construct implements iam.IGrant

    const provider = new customresources.Provider(this, 'Provider', {
      onEventHandler: handler,
+      role: this.getProviderRole(handler),


Is it possible to use handler.role (so everything about the singleton lambda is nicely scoped inside the SingletonFunction instance and you don't need to add the constructName)?

The handler in this case is the singleton lambda function. However, the role is not associated to the handler. Instead, the role is assumed by the custom resource provider, which is responsible for triggering the handler.

I see! Thank you.

samson-keung · 2025-01-03T04:56:33Z

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

@@ -112,6 +112,8 @@ abstract class UserBase extends Construct implements IUser {
        ...this.databaseProps,
        user: this,
      });
+
+      this.privileges.node.addDependency(table);


I suggest adding the reason as a code comment.

godwingrs22 · 2025-01-03T21:06:18Z

packages/@aws-cdk/integ-runner/lib/recommended-feature-flags.json

QQ: Is there any reason of adding the feature flag context file into integ-runner for this PR?

oh, it's auto generated. I will remove it

aws-cdk-automation · 2025-01-03T21:57:14Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 751a303
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2025-01-03T22:46:44Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2025-01-03T22:47:04Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Dec 2, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Dec 2, 2024

aws-cdk-automation requested a review from a team December 2, 2024 22:22

5d force-pushed the 5d/redshift-iam-role branch from c1402b3 to e5a6c7b Compare December 9, 2024 23:50

aws-cdk-automation previously requested changes Dec 9, 2024

View reviewed changes

github-advanced-security bot found potential problems Dec 10, 2024

View reviewed changes

...shot/asset.c973f9a285ae54986627703f13cdd21d38dcdb3cf29620c5411d7d3baa53a3b9/redshift-data.js Fixed Show fixed Hide fixed

5d force-pushed the 5d/redshift-iam-role branch 2 times, most recently from e9a9b6b to ea8b8f0 Compare December 12, 2024 00:44

5d changed the base branch from main to 5d/fix-31817 December 12, 2024 00:44

5d force-pushed the 5d/redshift-iam-role branch from ea8b8f0 to cc05f22 Compare December 12, 2024 01:49

5d marked this pull request as ready for review December 12, 2024 01:54

Base automatically changed from 5d/fix-31817 to main December 13, 2024 19:14

5d added 6 commits December 13, 2024 19:44

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

daece0a

fix unit test cases

2500815

revert some integ file changes

a998017

fix(redshift-alpha): use same role for database-query singleton function

8f89f02

add additional integ test case

06e8851

update integ tests

4331a07

5d force-pushed the 5d/redshift-iam-role branch from a63c2a2 to 4331a07 Compare December 13, 2024 19:45

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 13, 2024

5d added 2 commits December 13, 2024 12:22

Merge branch 'main' into 5d/redshift-iam-role

8723099

Merge branch 'main' into 5d/redshift-iam-role

e89b32b

github-actions bot mentioned this pull request Jan 1, 2025

Monthly PR metrics report #32703

Open

Merge branch 'main' into 5d/redshift-iam-role

6c0f908

godwingrs22 self-requested a review January 3, 2025 00:16

godwingrs22 reviewed Jan 3, 2025

View reviewed changes

samson-keung requested changes Jan 3, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 3, 2025

5d added 3 commits January 3, 2025 19:10

add more unit test cases

13b536f

Merge remote-tracking branch 'origin/main' into 5d/redshift-iam-role

af94e56

update integ tests

03a3946

5d requested review from godwingrs22 and samson-keung January 3, 2025 20:42

godwingrs22 reviewed Jan 3, 2025

View reviewed changes

remove auto generated feature flag file

751a303

godwingrs22 approved these changes Jan 3, 2025

View reviewed changes

samson-keung approved these changes Jan 3, 2025

View reviewed changes

mergify bot merged commit db950b3 into main Jan 3, 2025
22 checks passed

mergify bot deleted the 5d/redshift-iam-role branch January 3, 2025 22:46

github-actions bot locked as resolved and limited conversation to collaborators Jan 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(redshift-alpha): use same role for database-query singleton function #32363

fix(redshift-alpha): use same role for database-query singleton function #32363

5d commented Dec 2, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

aws-cdk-automation left a comment

godwingrs22 left a comment

godwingrs22 Jan 3, 2025

5d Jan 3, 2025

godwingrs22 Jan 3, 2025

5d Jan 3, 2025

godwingrs22 Jan 3, 2025

5d Jan 3, 2025

godwingrs22 Jan 3, 2025

5d Jan 3, 2025

samson-keung Jan 3, 2025

5d Jan 3, 2025

samson-keung Jan 3, 2025

5d Jan 3, 2025

samson-keung Jan 3, 2025

samson-keung Jan 3, 2025

godwingrs22 Jan 3, 2025

5d Jan 3, 2025

aws-cdk-automation commented Jan 3, 2025

mergify bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

fix(redshift-alpha): use same role for database-query singleton function #32363

fix(redshift-alpha): use same role for database-query singleton function #32363

Conversation

5d commented Dec 2, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

aws-cdk-automation left a comment

Choose a reason for hiding this comment

godwingrs22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 3, 2025

AWS CodeBuild CI Report

mergify bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

codecov bot commented Dec 2, 2024 •

edited

Loading