-
Notifications
You must be signed in to change notification settings - Fork 40
Add DynamoDB permission test #2847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @KodaiD, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the testing suite for ScalarDB's DynamoDB adapter by introducing dedicated integration tests to validate the precise AWS IAM permissions required for all DistributedStorage and DistributedStorageAdmin operations. It sets up the necessary build infrastructure and utility classes to dynamically manage IAM policies during test execution, ensuring robust verification of permission requirements for the DynamoDB backend.
Highlights
- New Permission Integration Tests: Introduced comprehensive permission integration tests for DynamoDB, covering both
DistributedStorageandDistributedStorageAdminoperations to verify required AWS IAM permissions for ScalarDB operations. - Dedicated Test Infrastructure: Established a new Gradle source set (
integrationTestDynamoPermission) and a corresponding task to isolate and run these specific permission tests, ensuring proper build configuration and dependency management for the new test modules. - Dynamic IAM Policy Management: Added a new utility class,
DynamoPermissionTestUtils, which leverages the AWS IAM SDK to programmatically create, update, and attach IAM policies with the necessary DynamoDB and Application Auto Scaling permissions required for testing various operations. - AWS SDK IAM Dependencies: Incorporated new AWS SDK dependencies (
iamandiam-policy-builder) into the project's build to support programmatic IAM policy generation and management, which is crucial for the new permission testing framework.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces permission tests for DynamoDB, which is a great addition for ensuring the security and correctness of database operations. My feedback focuses on improving the exception handling in the DynamoPermissionTestUtils class for better maintainability and robustness.
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoPermissionTestUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds integration tests and supporting configuration to verify required IAM permissions for DynamoDB operations in ScalarDB, including workflow support in GitHub Actions.
- Introduces
DynamoPermissionTestUtilsfor creating and managing IAM policies in tests. - Adds
DynamoPermissionIntegrationTestandDynamoAdminPermissionIntegrationTestclasses to validate storage and admin APIs under permission constraints. - Updates Gradle build and CI workflow to run DynamoDB permission integration tests.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoPermissionTestUtils.java | Utility for creating, attaching, and managing IAM policies for DynamoDB permission tests |
| core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoPermissionIntegrationTest.java | Integration test for storage-level permission checks |
| core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoAdminPermissionIntegrationTest.java | Integration test for admin-level permission checks |
| core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java | Environment property handling for emulator support and test configuration |
| core/build.gradle | Configures new integrationTestDynamoPermission source set, tasks, and dependencies |
| .github/workflows/permission-check.yaml | Adds GitHub Actions job to execute DynamoDB permission integration tests |
Comments suppressed due to low confidence (2)
.github/workflows/permission-check.yaml:107
- The Gradle command doesn't pass the
scalardb.dynamo.emulatororscalardb.dynamo.regionsystem properties, so the tests default to emulator mode and local endpoint, causing failures in CI. Consider adding-Dscalardb.dynamo.emulator=falseand-Dscalardb.dynamo.region=$REGIONto the Gradle command.
run: ./gradlew integrationTestDynamoPermission
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java:20
- The default value for DEFAULT_DYNAMO_EMULATOR is set to "true", causing tests to always run in emulator mode (overriding actual AWS setup) which will fail in CI. Consider defaulting this to "false" or making it configurable via system properties in CI.
private static final String DEFAULT_DYNAMO_EMULATOR = "true";
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java
Show resolved
Hide resolved
...integration-test/java/com/scalar/db/storage/dynamo/DynamoAdminPermissionIntegrationTest.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java
Outdated
Show resolved
Hide resolved
.../src/integration-test/java/com/scalar/db/storage/dynamo/DynamoPermissionIntegrationTest.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoPermissionTestUtils.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoPermissionTestUtils.java
Outdated
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoPermissionTestUtils.java
Outdated
Show resolved
Hide resolved
4727904 to
bb483f9
Compare
16f8929 to
04e6dcf
Compare
komamitsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
|
I've confirmed that the permission test works as expected in: |
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left minor comments. Other than that, LGTM!
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java
Outdated
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java
Outdated
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java
Outdated
Show resolved
Hide resolved
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Torch3333
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Description
This PR adds a permission test for DynamoDB under ScalarDB. To check what permissions are required for all ScalarDB operations, all DistributedStorage and DistributedStorageAdmin methods are tested. This is a second step of the permission tests project. Permission tests for JDBC backends will be added in the future.
Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A