feat(dynamodb): add resource polices for table #29818

LeeroyHannigan · 2024-04-12T18:43:00Z

Issue # (if applicable)

Closes #29600.

#29600

Reason for this change

Adding a new feature

Description of changes

Add resourcePolicy for DynamoDB Table component in aws-dynamodb

Description of how you validated changes

integration test integ.dynamodb.policy.ts

Checklist

[X ] 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

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.

msambol · 2024-04-14T13:27:00Z

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

@@ -370,6 +370,13 @@ export interface TableOptions extends SchemaOptions {
   * @default - no data import from the S3 bucket
   */
  readonly importSource?: ImportSourceSpecification;
+
+  /**
+   * Resource policy to assign to DynamoDB Table.


nit: Resource policy to assign to the table.

msambol · 2024-04-14T13:27:22Z

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

+
+new TestStack(app, 'ResourcePolicyTest');
+
+app.synth();


Can you run this with yarn integ-runner... ? It should produce a snapshot.

I can. I was just running cdk synth. So yarn integ-runner filename.js is the way to run it?

yeah, take a look here: https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md

yes when you update any integ.*.ts under framework-integ, you should run yarn integ to update the snapshots.
https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#integration-tests

msambol · 2024-04-14T13:28:13Z

Title: feat(dynamodb): add resource policy for table

toutou826 · 2024-04-22T23:39:27Z

Hi @LeeroyHannigan ,

I am interested to use this functionality, thank you taking on this feature!

It would be great to change the grant functions in Table construct:

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-dynamodb/lib/table-v2-base.ts#L442

from using Grant.addToPrincipal to Grant.addToPrincipalOrResource so people could use TableV2.grant and related functions for resource based permission as well

comcalvi · 2024-04-25T17:55:39Z

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

+   *
+   * @default - No resource policy statements are added to the created table.
+   */
+  readonly resourcePolicy?: iam.PolicyDocument;


Don't expose the resourcePolicy like this, integrate it with the L2's grant methods (try using addToPrincipalOrResource). You'll need to put this under a feature flag, because otherwise this will be modifying people's IAM permissions on a version bump, which is a breaking change.

This is the same suggestion that @toutou826 made here: #29818 (comment).

@comcalvi can you explain how it will work with the grant methods and why it would be backward incompatible?

My understanding of grant, specifically addToPrincipalOrResource will first add IAM policy to the principal of the grantee resource and fall back to adding resource-based policy addToResourcePolicy on fail. For all existing grant usage for dynamodb table, it calls addToPrincipal which tries to add IAM policy and throw an exception when fail.

Are you suggesting to make resourcePolicy field private and table construct will need to implement addToResourcePolicy function, then when failing to add IAM policy fall back to call addToResourcePolicy and update resourcePolicy field? I don't see why this would be backward incompatible though.

@comcalvi

Don't expose the resourcePolicy like this, integrate it with the L2's grant methods (try using addToPrincipalOrResource). You'll need to put this under a feature flag, because otherwise this will be modifying people's IAM permissions on a version bump, which is a breaking change.

According to the design guidelines we should expose this prop:

When a construct represents an AWS resource that supports a resource policy, it should expose an optional prop that will allow initializing resource with a specified policy [awslint:resource-policy-prop]:

resourcePolicy?: iam.PolicyStatement[]

Are you suggesting otherwise?

I didn't realize this was in our design guidelines. We should follow the design guidelines here. As we discussed offline, this grant needs to support using addToPrincpalOrResource. TableBase needs to implement IResourceWithPolicy.

comcalvi · 2024-04-25T17:56:10Z

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

+    super(scope, id, props);
+
+    // table with resource policy
+    new dynamodb.Table(this, 'TableTest1', {


We need unit test changes as well as integ test changes.

aws-cdk-automation · 2024-05-04T00:09:05Z

This PR has been in the BUILD FAILING 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.

comcalvi

Please add some unit tests for this change.

comcalvi · 2024-05-10T17:32:35Z

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

+  /**
+   * Resource policy to assign to DynamoDB Table.
+   *
+   * @default - No resource policy statements are added to the created table.


Suggested change

* @default - No resource policy statements are added to the created table.

* @default - No resource policy.

comcalvi · 2024-05-10T17:32:42Z

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

+  /**
+   * Resource policy to assign to DynamoDB Table.
+   *
+   * @default - No resource policy statements are added to the created table.


Suggested change

* @default - No resource policy statements are added to the created table.

* @default - No resource policy.

comcalvi · 2024-05-10T17:34:23Z

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

@@ -21,6 +21,7 @@ import {
  Aws, CfnCondition, CfnCustomResource, CfnResource, Duration,
  Fn, Lazy, Names, RemovalPolicy, Stack, Token, CustomResource,
 } from '../../core';
+import { Grant, IResourceWithPolicy } from '../../aws-iam';


can we use the package import iam object instead of these directly?

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

Pull request has been modified.

aws-cdk-automation · 2024-05-16T18:58:35Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2024-05-17T00:09:09Z

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.

Add reource polices to DynamoDB Table aws#29600

e113524

aws-cdk-automation requested a review from a team April 12, 2024 18:43

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Apr 12, 2024

aws-cdk-automation previously requested changes Apr 12, 2024

View reviewed changes

Add resource polices to DynamoDB Table aws#29600

7915c7e

msambol reviewed Apr 14, 2024

View reviewed changes

github-actions bot mentioned this pull request Apr 15, 2024

Weekly PR metrics report #29829

Closed

paulhcsun changed the title ~~feat(aws-dynamodb) Add resource polices to DynamoDB Table~~ feat(dynamodb) add resource polices for DynamoDB Table Apr 19, 2024

comcalvi requested changes Apr 25, 2024

View reviewed changes

github-actions bot mentioned this pull request May 1, 2024

Monthly PR metrics report #30026

Closed

Lee Hannigan added 4 commits May 7, 2024 11:45

add grants, not yet working

7ceb46f

add grants, not yet working

c0ff7ca

Fix to table-v2.policy integ test

aaf694c

grant() not working as expected

7399e9f

comcalvi previously requested changes May 10, 2024

View reviewed changes

LeeroyHannigan and others added 3 commits May 11, 2024 18:07

Merge branch 'aws:main' into lhnng

949958e

Resource policy for TableV2 implemented

ebf50ed

Resource policy for TableV2 README updated

ffbe667

LeeroyHannigan changed the title ~~feat(dynamodb) add resource polices for DynamoDB Table~~ feat(dynamodb): add resource polices for dynamoDB table May 13, 2024

LeeroyHannigan changed the title ~~feat(dynamodb): add resource polices for dynamoDB table~~ feat(dynamodb): add resource polices for table May 13, 2024

Add docstring to resourcePolicy attribute

4fbda83

Lee Hannigan and others added 3 commits May 16, 2024 17:47

Adding docstring to resourcePolicy

26f76eb

Including snapshots for integ tests

1ee53fe

Merge branch 'main' into lhnng

dc14bba

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

aws-cdk-automation closed this May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dynamodb): add resource polices for table #29818

feat(dynamodb): add resource polices for table #29818

LeeroyHannigan commented Apr 12, 2024

aws-cdk-automation left a comment

msambol Apr 14, 2024

msambol Apr 14, 2024

LeeroyHannigan Apr 14, 2024

msambol Apr 14, 2024

pahud Apr 15, 2024

msambol commented Apr 14, 2024

toutou826 commented Apr 22, 2024 •

edited

comcalvi Apr 25, 2024

comcalvi Apr 25, 2024

GavinZZ May 2, 2024 •

edited

LeeroyHannigan May 3, 2024 •

edited

comcalvi May 6, 2024

comcalvi Apr 25, 2024

aws-cdk-automation commented May 4, 2024

comcalvi left a comment

comcalvi May 10, 2024

comcalvi May 10, 2024

comcalvi May 10, 2024

aws-cdk-automation commented May 16, 2024

aws-cdk-automation commented May 17, 2024

	* @default - No resource policy statements are added to the created table.
	* @default - No resource policy.

feat(dynamodb): add resource polices for table #29818

feat(dynamodb): add resource polices for table #29818

Conversation

LeeroyHannigan commented Apr 12, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation 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

msambol commented Apr 14, 2024

toutou826 commented Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinZZ May 2, 2024 • edited

Choose a reason for hiding this comment

LeeroyHannigan May 3, 2024 • edited

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 May 4, 2024

comcalvi 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

aws-cdk-automation commented May 16, 2024

AWS CodeBuild CI Report

aws-cdk-automation commented May 17, 2024

toutou826 commented Apr 22, 2024 •

edited

GavinZZ May 2, 2024 •

edited

LeeroyHannigan May 3, 2024 •

edited