fix(dynamodb): addtoresourcepolicy fix for table (v1) construct #31516

LeeroyHannigan · 2024-09-21T09:06:00Z

Issue # (if applicable)

Closes #30793.

Reason for this change

addToResourcePolicy() was not correctly adding a policy statement to the table

Description of changes

Fixed the method to correctly add the policy to the table constructor.

Description of how you validated changes

Tests added

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

everilae · 2024-09-23T06:21:30Z

packages/aws-cdk-lib/aws-dynamodb/lib/table.ts

@@ -564,7 +564,7 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc
   * @param actions The set of actions to allow (i.e. "dynamodb:PutItem", "dynamodb:GetItem", ...)
   */
  public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant {
-    return iam.Grant.addToPrincipalOrResource({
+    return iam.Grant.addToPrincipal({


Why does this need to become addToPrincipal? We are for example already using the resource policy to grant cross-account access to a table directly. We build the policy in CDK from external account ArnPrincipals.

Because addToPrincipalOrResource never adds to the resource, as IAM is missing methods. So I'm putting it back to how it was before I changed it until I have time to implement the missing methods in IAM

I don't fully understand what methods are missing. Looking at Grant, it seems to be handling this already, if given the resource in the call: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-iam/lib/grant.ts#L145-L157. It's just that Table.addToResourcePolicy is not functional, but instead adds statements to a policy instance not bound with the internal CfnTable.

@LeeroyHannigan I want to check if I follow your line of thoughts here.

I see the iam.Grant.addToPrincipalOrResource is implemented here. It basically does 2 things, call addToPrincipal and a bit further down on line 151, it calls options.resource.addToResourcePolicy, which means the Table.addToResourcePolicy. Since the method is reportedly a no-op (from #30793), you are saying your change here from addToPrincipalOrResource to addToPrincipal will not change any actual behaviour.

I agree with that. But if I understand @everilae correctly, even though Table.addToResourcePolicy was a no-op, there may be CDK users who are currently relying on what iam.Grant.addToPrincipalOrResource returns (it returns the resource policy statement whereas iam.Grant.addToPrincipal returns principal policy statement.). This allows CDK users to use this method to build the resource policy instead of creating their own.

Hi,

We rely on the "...OrResource" part, or would like to, once all this is fixed. If the call is replaced with just "addToPrincipal", then attempts at granting to external accounts would not work.

everilae · 2024-09-23T06:23:45Z

packages/aws-cdk-lib/aws-dynamodb/lib/table.ts

@@ -958,11 +941,11 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc
          produce: () => this.hasIndex ? `${arn}/index/*` : Aws.NO_VALUE,
        })),
      ];
-      const ret = iam.Grant.addToPrincipalOrResource({
+      const ret = iam.Grant.addToPrincipal({


Same as above, this would break Table v1 for us.

aws-cdk-automation · 2024-10-12T14:31:30Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2024-12-09T00:12:27Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

samson-keung · 2024-12-11T06:58:41Z

Hi @LeeroyHannigan, I see there are some merge conflicts as well as comments about this can be a breaking change. Are you planning to update this PR?

LeeroyHannigan · 2024-12-11T09:06:19Z

Hi @LeeroyHannigan, I see there are some merge conflicts as well as comments about this can be a breaking change. Are you planning to update this PR?

Hi, yes I plan to fix this. Although I don't believe there are any breaking changes, I changed the methods previously, but they never worked as intended and this change just reverts them back to original state.

samson-keung · 2024-12-11T22:19:26Z

packages/@aws-cdk-testing/framework-integ/test/aws-dynamodb/test/integ.dynamodb.policy.ts

+      principals: [new iam.AccountRootPrincipal()],
+      resources: ['*'],
+    }));
+


I think the change in this PR is already covered in the unit test. Integ test does not seem necessary to me because we are not using new L1 props. Is there anything the integ test would catch while the unit test cannot?

samson-keung · 2024-12-11T22:45:13Z

packages/aws-cdk-lib/aws-dynamodb/lib/table.ts

@@ -564,7 +564,7 @@ export abstract class TableBase extends Resource implements ITable, iam.IResourc
   * @param actions The set of actions to allow (i.e. "dynamodb:PutItem", "dynamodb:GetItem", ...)
   */
  public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant {
-    return iam.Grant.addToPrincipalOrResource({
+    return iam.Grant.addToPrincipal({


@LeeroyHannigan I want to check if I follow your line of thoughts here.

I see the iam.Grant.addToPrincipalOrResource is implemented here. It basically does 2 things, call addToPrincipal and a bit further down on line 151, it calls options.resource.addToResourcePolicy, which means the Table.addToResourcePolicy. Since the method is reportedly a no-op (from #30793), you are saying your change here from addToPrincipalOrResource to addToPrincipal will not change any actual behaviour.

I agree with that. But if I understand @everilae correctly, even though Table.addToResourcePolicy was a no-op, there may be CDK users who are currently relying on what iam.Grant.addToPrincipalOrResource returns (it returns the resource policy statement whereas iam.Grant.addToPrincipal returns principal policy statement.). This allows CDK users to use this method to build the resource policy instead of creating their own.

aws-cdk-automation · 2024-12-16T00:12:53Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2024-12-16T00:13:20Z

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.

fix(dynamodb): addtoresourcepolicy fix for table (v1)

49bfdb1

aws-cdk-automation requested a review from a team September 21, 2024 09:06

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Sep 21, 2024

Merge branch 'main' into addtorbp-fix-v1

57dab53

LeeroyHannigan changed the title ~~fix(dynamodb): addtoresourcepolicy fix for table (v1)~~ fix(dynamodb): addtoresourcepolicy fix for table (v1) construct Sep 21, 2024

everilae reviewed Sep 23, 2024

View reviewed changes

Lee Hannigan and others added 4 commits September 24, 2024 09:25

Fixed addToResourcePolicy, grants need work

057ab1e

Merge branch 'main' into addtorbp-fix-v1

4668ce7

Fix failing unit tests with statement length check

3fb88ac

Fixing addToResourcePolicy

99284b8

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 12, 2024

samson-keung self-assigned this Dec 5, 2024

samson-keung requested changes Dec 11, 2024

View reviewed changes

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Dec 16, 2024

aws-cdk-automation closed this Dec 16, 2024

github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2024

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dynamodb): addtoresourcepolicy fix for table (v1) construct #31516

fix(dynamodb): addtoresourcepolicy fix for table (v1) construct #31516

LeeroyHannigan commented Sep 21, 2024

everilae Sep 23, 2024

LeeroyHannigan Sep 23, 2024

everilae Sep 23, 2024

samson-keung Dec 11, 2024

everilae Dec 11, 2024

everilae Sep 23, 2024

aws-cdk-automation commented Oct 12, 2024

aws-cdk-automation commented Dec 9, 2024

samson-keung commented Dec 11, 2024

LeeroyHannigan commented Dec 11, 2024

samson-keung Dec 11, 2024

samson-keung Dec 11, 2024

aws-cdk-automation commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

fix(dynamodb): addtoresourcepolicy fix for table (v1) construct #31516

fix(dynamodb): addtoresourcepolicy fix for table (v1) construct #31516

Conversation

LeeroyHannigan commented Sep 21, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

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 Oct 12, 2024

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 9, 2024

samson-keung commented Dec 11, 2024

LeeroyHannigan commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 16, 2024

github-actions bot commented Dec 16, 2024