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

LeeroyHannigan · 2024-05-17T08:38:23Z

Issue # (if applicable)

#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](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

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

scanlonp · 2024-05-20T16:21:06Z

@Mergifyio update

mergify · 2024-05-20T16:21:18Z

update

✅ Branch has been successfully updated

JavierLuna · 2024-05-22T12:14:29Z

Really looking forward to this, resource based policies would simplify our architecture a lot

scanlonp

@LeeroyHannigan, this looks amazing! Just a bit of clean up and some implementation questions. I said in the comments, but I think we can take all of the doc strings and code style from table-v2.ts and copy it to the corresponding places in table.ts.

I see that we are slightly changing the grant implementation; I assume it should not affect its current behavior. I wish we had some grant coverage in our integ tests, but it looks like we do not currently test it beyond a couple grantReadData/grantWriteData calls. However, I see it is thoroughly unit tested, and none were effected, so I feel good.

Lastly, you call out that adding a replica and adding a resource-based policy to that replica in the same deployment (update) is not possible. Is this an error that is thrown on the CloudFormation side? Can you detail how this affects a user creating a TableV2? It sounds to me like a user needs to define all their replicas, deploy, then add all the resource policies, the deploy again. Is that correct?
I do not believe we can validate this at synth time since we are stateless. Calling it out may be the only option.

Again, thanks for submitting this PR and excited for customers being able to use this soon!

scanlonp · 2024-05-23T10:29:22Z

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

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


Let's clean this up.

scanlonp · 2024-05-23T10:30:15Z

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

+  /**
+   * @attribute
+   */
+  public resourcePolicy?: PolicyDocument;
+


Let's move this below encryptionKey.

Noting that this should not be readonly as we can add policies to it. Checkout s3/bucket.ts for a similar pattern.

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

scanlonp · 2024-05-23T10:32:41Z

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

+  /**
+   * Resource policy to assign to table.
+   * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-dynamodb-table.html#cfn-dynamodb-table-resourcepolicy
+   * @default - No resource policy statement
+   */


Can cut down this doc string.

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

scanlonp · 2024-05-23T10:43:26Z

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

+        name: 'id',
+        type: dynamodb.AttributeType.STRING,
+      },
+      removalPolicy: RemovalPolicy.DESTROY,


Appreciate these removal polices!

scanlonp · 2024-05-23T10:46:51Z

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

+new IntegTest(app, 'resource-policy-integ-test', {
+  testCases: [stack],
+  regions: ['us-east-1'],
+  cdkCommandOptions: {
+    deploy: {
+      args: {
+        rollback: true,
+      },
+    },
+  },
+});


Suggested change

new IntegTest(app, 'resource-policy-integ-test', {

testCases: [stack],

regions: ['us-east-1'],

cdkCommandOptions: {

deploy: {

args: {

rollback: true,

},

},

},

});

new IntegTest(app, 'resource-policy-integ-test', {

testCases: [stack],

});

Let's simplify the integ test configuration. Unless there is a specific reason, this may be better for our automation.

scanlonp · 2024-05-23T10:47:29Z

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

+new IntegTest(app, 'table-v2-resource-policy-integ-test', {
+  testCases: [stack],
+  regions: ['us-east-1'],
+  cdkCommandOptions: {
+    deploy: {
+      args: {
+        rollback: true,
+      },
+    },
+  },
+});


Suggested change

new IntegTest(app, 'table-v2-resource-policy-integ-test', {

testCases: [stack],

regions: ['us-east-1'],

cdkCommandOptions: {

deploy: {

args: {

rollback: true,

},

},

},

});

new IntegTest(app, 'table-v2-resource-policy-integ-test', {

testCases: [stack],

});

Same thing here. Paring down the config.

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

scanlonp · 2024-05-23T11:07:20Z

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

+  });
+});
+
+test('throws if trying to add a resource policy to a region other than local region', () => {


This is an odd test. It is not actually testing what is says its testing. There is no error added related to policy statements, so we cannot throw in a unit test for that scenario.

This error is already covered by a test in this section, so I suggest we delete this test unless it is testing something related to policy documents.

scanlonp · 2024-05-23T11:14:21Z

@Mergifyio update

mergify · 2024-05-23T11:14:28Z

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission

Pull request has been modified.

LeeroyHannigan · 2024-05-24T20:57:36Z

Lastly, you call out that adding a replica and adding a resource-based policy to that replica in the same deployment (update) is not possible. Is this an error that is thrown on the CloudFormation side? Can you detail how this affects a user creating a TableV2? It sounds to me like a user needs to define all their replicas, deploy, then add all the resource policies, the deploy again. Is that correct? I do not believe we can validate this at synth time since we are stateless. Calling it out may be the only option.

Yes, precisely. You first would need to create the replica and then update with the resource based policy. This limitation comes from the CFN implementation.

aws-cdk-automation · 2024-05-28T00:10:53Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

packages/aws-cdk-lib/aws-dynamodb/README.md

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

scanlonp

Looks good. Thanks for your hard work on this @LeeroyHannigan

scanlonp · 2024-05-29T21:49:04Z

@Mergifyio update

mergify · 2024-05-29T21:49:38Z

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission

Pull request has been modified.

aws-cdk-automation · 2024-05-29T22:19:39Z

AWS CodeBuild CI Report

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

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

mergify · 2024-05-29T22:19:57Z

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).

Issue # (if applicable) Closes aws#29600. aws#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](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Lee Hannigan and others added 12 commits April 12, 2024 19:38

Add reource polices to DynamoDB Table aws#29600

e113524

Add resource polices to DynamoDB Table aws#29600

7915c7e

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

Merge branch 'aws:main' into lhnng

949958e

Resource policy for TableV2 implemented

ebf50ed

Resource policy for TableV2 README updated

ffbe667

Add docstring to resourcePolicy attribute

4fbda83

Adding docstring to resourcePolicy

26f76eb

Including snapshots for integ tests

1ee53fe

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

aws-cdk-automation requested a review from a team May 17, 2024 08:38

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

Lee Hannigan added 2 commits May 17, 2024 12:23

Increase test coverage

2548289

Revert changes to sdk package

5f2a8ba

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

Weekly PR metrics report #30274

Closed

scanlonp previously requested changes May 23, 2024

View reviewed changes

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

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

scanlonp self-assigned this May 23, 2024

LeeroyHannigan force-pushed the rbp-lhnng branch from e5a7f8c to 91ed0b2 Compare May 28, 2024 14:00

Incorporating feedback - Tidy up

c8b40ee

LeeroyHannigan force-pushed the rbp-lhnng branch from 91ed0b2 to c8b40ee Compare May 28, 2024 16:36

scanlonp added 2 commits May 29, 2024 14:39

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

bf89bb5

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

417db1e

scanlonp reviewed May 29, 2024

View reviewed changes

packages/aws-cdk-lib/aws-dynamodb/README.md Outdated Show resolved Hide resolved

Update packages/aws-cdk-lib/aws-dynamodb/README.md

1befa1e

scanlonp reviewed May 29, 2024

View reviewed changes

packages/aws-cdk-lib/aws-dynamodb/lib/table.ts Outdated Show resolved Hide resolved

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

cae42d6

scanlonp previously approved these changes May 29, 2024

View reviewed changes

Merge branch 'main' into rbp-lhnng

5e3b3d1

scanlonp approved these changes May 29, 2024

View reviewed changes

mergify bot merged commit 7dc6d27 into aws:main May 29, 2024
11 checks passed

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

Monthly PR metrics report #30414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

LeeroyHannigan commented May 17, 2024

scanlonp commented May 20, 2024

mergify bot commented May 20, 2024

JavierLuna commented May 22, 2024

scanlonp left a comment

scanlonp May 23, 2024

scanlonp May 23, 2024

scanlonp May 23, 2024

scanlonp May 23, 2024

scanlonp May 23, 2024

scanlonp May 23, 2024

scanlonp May 23, 2024

scanlonp May 23, 2024

scanlonp commented May 23, 2024

mergify bot commented May 23, 2024

LeeroyHannigan commented May 24, 2024

aws-cdk-automation commented May 28, 2024

scanlonp left a comment

scanlonp commented May 29, 2024

mergify bot commented May 29, 2024

aws-cdk-automation commented May 29, 2024

mergify bot commented May 29, 2024

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

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

Conversation

LeeroyHannigan commented May 17, 2024

scanlonp commented May 20, 2024

mergify bot commented May 20, 2024

✅ Branch has been successfully updated

JavierLuna commented May 22, 2024

scanlonp 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

scanlonp commented May 23, 2024

mergify bot commented May 23, 2024

❌ Mergify doesn't have permission to update

LeeroyHannigan commented May 24, 2024

aws-cdk-automation commented May 28, 2024

scanlonp left a comment

Choose a reason for hiding this comment

scanlonp commented May 29, 2024

mergify bot commented May 29, 2024

❌ Mergify doesn't have permission to update

aws-cdk-automation commented May 29, 2024

AWS CodeBuild CI Report

mergify bot commented May 29, 2024