-
Notifications
You must be signed in to change notification settings - Fork 40
Add permission list management #2819
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.
Pull Request Overview
This PR adds permission-based integration tests for Cassandra, DynamoDB, and JDBC storage backends in ScalarDB. It introduces a PermissionTestUtils interface, two abstract test bases for storage and admin operations, concrete implementations for each backend, and updates build scripts and GitHub workflows to include these new tests.
- New
PermissionTestUtilsinterface and implementations for JDBC, DynamoDB, and Cassandra - Abstract test bases (
DistributedStoragePermissionIntegrationTestBaseandDistributedStorageAdminPermissionIntegrationTestBase) with concrete integration tests - Updated
build.gradleand GitHub Actions workflow to run permission integration tests
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-test/src/main/java/com/scalar/db/util/PermissionTestUtils.java | Defines common interface for creating/granting users |
| integration-test/src/main/java/com/scalar/db/api/DistributedStoragePermissionIntegrationTestBase.java | Adds base tests for storage operations with normal user |
| integration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminPermissionIntegrationTestBase.java | Adds base tests for admin operations with normal user |
| core/src/integration-test/java/com/scalar/db/storage/jdbc/* | JDBC-specific PermissionTestUtils, tests, and env |
| core/src/integration-test/java/com/scalar/db/storage/dynamo/* | Dynamo-specific PermissionTestUtils, tests, and env |
| core/src/integration-test/java/com/scalar/db/storage/cassandra/* | Cassandra-specific PermissionTestUtils, tests, and env |
| core/build.gradle | Adds new source sets and tasks for permission tests |
| .github/workflows/test-permission.yaml | Workflow to run permission tests on GitHub Actions |
Comments suppressed due to low confidence (2)
core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcPermissionTestUtils.java:72
- [nitpick] There's an unintended trailing backslash in this comment. Please remove '\' for clarity.
}
integration-test/src/main/java/com/scalar/db/util/PermissionTestUtils.java:3
- [nitpick] To support try-with-resources and clearer lifecycle management, consider having this interface extend
AutoCloseable.
public interface PermissionTestUtils {
...on-test/src/main/java/com/scalar/db/api/DistributedStoragePermissionIntegrationTestBase.java
Outdated
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,199 @@ | |||
| name: Test Permissions | |||
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.
For JDBC databases, only MySQL is tested for now.
| on: | ||
| workflow_dispatch: |
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.
Currently, the job is assumed to be run manually, for example, when a new release is made.
| exclude '**/com/scalar/db/storage/cassandra/CassandraPermissionTestUtils.java' | ||
| exclude '**/com/scalar/db/storage/dynamo/DynamoPermissionTestUtils.java' | ||
| exclude '**/com/scalar/db/storage/jdbc/JdbcPermissionTestUtils.java' | ||
| exclude '**/com/scalar/db/storage/cassandra/CassandraPermissionIntegrationTest.java' | ||
| exclude '**/com/scalar/db/storage/dynamo/DynamoPermissionIntegrationTest.java' | ||
| exclude '**/com/scalar/db/storage/jdbc/JdbcPermissionIntegrationTest.java' | ||
| exclude '**/com/scalar/db/storage/cassandra/CassandraAdminPermissionIntegrationTest.java' | ||
| exclude '**/com/scalar/db/storage/dynamo/DynamoAdminPermissionIntegrationTest.java' | ||
| exclude '**/com/scalar/db/storage/jdbc/JdbcAdminPermissionIntegrationTest.java' |
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.
To avoid the permission tests are run in the CI, the test files are excluded.
| @Override | ||
| protected void waitForNamespaceCreation() { |
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.
Because CassandraAdmin does not have the wait for table creation mechanism, we need to prepare it in the test.
| return properties; | ||
| } | ||
|
|
||
| public static Properties getPropertiesForNormalUser(String testName) { |
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.
To test the permission list, a normal user is created and used for the test.
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.
To handle non-emulator DynamoDB, the option to distinguish between emulator and non-emulator is added.
|
|
||
| @Override | ||
| protected Map<String, String> getCreationOptions() { | ||
| return ImmutableMap.of(DynamoAdmin.NO_SCALING, "false", DynamoAdmin.NO_BACKUP, "false"); |
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.
To test under the environment assuming the production environment, the scaling and the backup are enabled.
| @Override | ||
| public void createNormalUser(String userName, String password) { | ||
| // Do nothing for DynamoDB. | ||
| } | ||
|
|
||
| @Override | ||
| public void dropNormalUser(String userName) { | ||
| // Do nothing for DynamoDB. | ||
| } |
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.
The current implementation assumes that the same IAM user is used for tests. Only the IAM policy is updated for each test.
|
|
||
| on: | ||
| workflow_dispatch: | ||
| pull_request: |
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.
To ensure the workflow runs correctly, a pull request trigger is added. It will be removed before merging.
|
Since it is going to be a big PR, I will split it into smaller PRs for each backend. |
Description
This PR adds integration tests for the list of permissions required by ScalarDB per underlying storage. The list is helpful for users to know what permissions they need where they have restricted permissions rather than full admin access.
Related issues and/or PRs
N/A
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A