feat(dynamodb): imported tables always grant permissions for indexes#20682
feat(dynamodb): imported tables always grant permissions for indexes#20682mergify[bot] merged 5 commits intoaws:mainfrom
Conversation
|
|
| public readonly encryptionKey?: kms.IKey; | ||
| protected readonly hasIndex = (attrs.globalIndexes ?? []).length > 0 || | ||
| (attrs.localIndexes ?? []).length > 0; | ||
| protected readonly hasIndex = true; |
There was a problem hiding this comment.
Instead of keeping this property and changing it to true can we just remove all usage of it (since
it's always true now)?
There was a problem hiding this comment.
Thank you for the review! I was wondering if there was a use case to grant granular permissions with the Table class. To grant permissions for indexed with imported table, users need to additionally provide index information. However, with the original Table class, users are supposed to provide index information to create indexes. Then the minimum permissions are granted. The behavior seems better since ideally only permissions users need should be given.
If there is no concern to remove this behavior, I'll remove hasIndex property from the base class.
There was a problem hiding this comment.
ahh I see what you are saying. After thinking about this more I wonder if we should just keep the existing behavior and add an additional property when importing a table. Something like grantIndexPermissions?: boolean.
Currently it looks like on both imported tables and managed tables as long as there is any index we just grant access to all indexes. I think it would be better if we granted access to the specific indexes if they are provided so I want to leave that option open for a future PR (it would need a feature flag).
What do you think?
There was a problem hiding this comment.
I like the idea to add a new parameter. I would go with forceGrantIndexPermissions? to make it less confusing when we provide both parameters.
Currently it looks like on both imported tables and managed tables as long as there is any index we just grant access to all indexes. I think it would be better if we granted access to the specific indexes if they are provided so I want to leave that option open for a future PR (it would need a feature flag).
Agreed. It would be also nice that grant method accepts a parameter to specify how the permissions are granted (e.g. all indexes, specific indexes, no index) in the future.
I'll update the PR. Thanks!
There was a problem hiding this comment.
I've updated it. Please take a look. On second thought, if we want to leave the possibility for the future extensions you mentioned, grantIndexPermissions seems better. So I went with it.
If grantIndexPermissions is provided, grant methods always grant permissions for all indexes.
79f482a to
84bb985
Compare
| `@aws-cdk/aws-lambda-event-source.DynamoEventSource` on the imported table), you *must* use the | ||
| `Table.fromTableAttributes` method and the `tableStreamArn` property *must* be populated. | ||
|
|
||
| To grant permissions for indexes with imported tables, `grantIndexPermissions` property or indexes of the importing tables |
There was a problem hiding this comment.
Maybe something like:
"In order to grant permissions to indexes on imported tables you can either set grantIndexPermissions to true, or you can provide the indexes via the globalIndexes or localIndexes properties. This will enable grant* methods to also grant permissions to all table indexes."
|
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). |
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). |
…ws#20682) When we use imported tables, grant methods don't grant permissions for indexes unless local indexes or global secondary indexes are specified. The information for indexes is used only for grant permissions now. Users either keep track of index information of the imported tables or specify random index (e.g. `*`) as a workaround to obtain the permissions. This PR let imported tables grant permissions for indexes without providing indexes. close aws#13703 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
When we use imported tables, grant methods don't grant permissions for indexes unless local indexes or global secondary indexes are specified. The information for indexes is used only for grant permissions now. Users either keep track of index information of the imported tables or specify random index (e.g.
*) as a workaround to obtain the permissions. This PR let imported tables grant permissions for indexes without providing indexes.close #13703
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license