feat(dynamodb): add resource polices for table#30251
feat(dynamodb): add resource polices for table#30251mergify[bot] merged 20 commits intoaws:mainfrom LeeroyHannigan:rbp-lhnng
Conversation
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
Really looking forward to this, resource based policies would simplify our architecture a lot |
scanlonp
left a comment
There was a problem hiding this comment.
@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!
| // /** | ||
| // * Resource policy to assign to DynamoDB Table. | ||
| // * | ||
| // * @default - No resource policy statements are added to the created table. | ||
| // */ | ||
| // readonly resourcePolicy?: iam.PolicyDocument; | ||
|
|
| /** | ||
| * @attribute | ||
| */ | ||
| public resourcePolicy?: PolicyDocument; | ||
|
|
There was a problem hiding this comment.
Let's move this below encryptionKey.
There was a problem hiding this comment.
Noting that this should not be readonly as we can add policies to it. Checkout s3/bucket.ts for a similar pattern.
| /** | ||
| * 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 | ||
| */ |
There was a problem hiding this comment.
Can cut down this doc string.
| name: 'id', | ||
| type: dynamodb.AttributeType.STRING, | ||
| }, | ||
| removalPolicy: RemovalPolicy.DESTROY, |
There was a problem hiding this comment.
Appreciate these removal polices!
| new IntegTest(app, 'resource-policy-integ-test', { | ||
| testCases: [stack], | ||
| regions: ['us-east-1'], | ||
| cdkCommandOptions: { | ||
| deploy: { | ||
| args: { | ||
| rollback: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
| 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.
| new IntegTest(app, 'table-v2-resource-policy-integ-test', { | ||
| testCases: [stack], | ||
| regions: ['us-east-1'], | ||
| cdkCommandOptions: { | ||
| deploy: { | ||
| args: { | ||
| rollback: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); No newline at end of file |
There was a problem hiding this comment.
| 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
Show resolved
Hide resolved
| }); | ||
| }); | ||
|
|
||
| test('throws if trying to add a resource policy to a region other than local region', () => { |
There was a problem hiding this comment.
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.
|
@Mergifyio update |
❌ Mergify doesn't have permission to updateDetailsFor security reasons, Mergify can't update this pull request. Try updating locally. |
|
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. |
scanlonp
left a comment
There was a problem hiding this comment.
Looks good. Thanks for your hard work on this @LeeroyHannigan
|
@Mergifyio update |
❌ Mergify doesn't have permission to updateDetailsFor security reasons, Mergify can't update this pull request. Try updating locally. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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*
|
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. |
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
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license