Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[#5570] improve(CLI): Add the ability to revoke all roles or groups in the Gravitino CLI. #6240

Merged
merged 13 commits into from
Jan 16, 2025
Merged

[#5570] improve(CLI): Add the ability to revoke all roles or groups in the Gravitino CLI. #6240

merged 13 commits into from
Jan 16, 2025

Conversation

Abyss-lord
Copy link
Contributor

What changes were proposed in this pull request?

Add ability to revoke all roles from a user or group in the Gravitino CLI.

  1. Add logic to handle the --all flag in UserCommandHandler#handleRevokeCommand;
  2. Add logic to handle the --all flag in GroupCommandHandler#handleRevokeCommand;
  3. Add new RemoveAllRoles command to handle user revoke --all and group revoke --all;
  4. Add unit tests;

Why are the changes needed?

Fix: #5570

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT + local test.

User test.

# step1: grant roles to testRole
gcli user grant -m demo_metalake --user testRole --role roleA roleB roleC -I
# roleA added to testRole
# roleB added to testRole
# roleC added to testRole
# Add roles roleA, roleB, roleC to user testRole

# step2: get details of testRole 
gcli user details -m demo_metalake --user testRole -I
# roleA,roleC,roleB

# step3: revoke all roles from testRole
gcli user revoke -m demo_metalake --user testRole -i --all
# Remove all roles from user testRole

# step4: get details of testRole
gcli user details -m demo_metalake --user testRole -I
# The user has no roles.

# step5: list exists roles
gcli role list -m demo_metalake -I
# admin,roleA,roleB,roleC

Group test.

# step1: grant roles to test_group
gcli group grant -m demo_metalake --group test_group --role roleA roleB roleC -I
# roleA added to test_group
# roleB added to test_group
# roleC added to test_group
# Grant roles roleA, roleB, roleC to group test_group

# step2: get details of test_group
gcli group details -m demo_metalake --group test_group -I
# admin,roleA,roleC,roleB

# step3: revoke all roles from test_group
gcli group revoke -m demo_metalake --group test_group -i --all
# Remove all roles from group test_group

# step4: get details of test_group
gcli group details -m demo_metalake --group test_group -I
# The group has no roles.

# step5: list exists roles
gcli role list -m demo_metalake -I
# admin,roleA,roleB,roleC

@Abyss-lord
[apache#5570] improve(CLI): Add the ability to revoke all roles or gr…
f6e9fc2 
…oups in the Gravitino CLI.

Add ability to revoke all roles from a user or group in the Gravitino CLI.
@Abyss-lord
Copy link
Contributor Author

@justinmclean @tengqm , could you please review this PR when you have time? I’d really appreciate your feedback.

@Abyss-lord
Copy link
Contributor Author

@justinmclean I’ve finished updating the code. Please take a look at the PR again when you have time.

@tengqm
Copy link
Contributor

tengqm commented Jan 15, 2025

lgtm

@justinmclean
Copy link
Member

The code looks fine to me. Can you also update the documentation and help content?

liuchunhao and others added 8 commits January 16, 2025 09:28
@liuchunhao @Abyss-lord
[apache#5976] Improvement(bin):Add validation checks to the startup s…
308e042 
…cripts to prevent incorrect usage (apache#5977)

### What changes were proposed in this pull request?

apache#5976

- Add file suffix ‘template’ to the following scripts:
    - bin/gravitino.sh
    - bin/common.sh
    - bin/gravitino-iceberg-rest-server.sh
- Add a validation check on `GRAVITINO_VERSION` in the script
bin/common.sh ( renamed to bin/common.sh.template ) with the followings
:
    
    ```bash
    GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER
    if [[ "$GRAVITINO_VERSION" == *_VERSION_PLACEHOLDER ]]; then
echo "GRAVITINO_VERSION is not set. Please make sure you are running the
script from the distribution/package/bin and before running the script,
run './gradle clean build -x test compileDistribution'"
      exit 1
    fi
    
    ```
    
- Update the following tasks in the root build.gradle.kts as described
below :
    - compileDistribution
    - compileIcebergRESTServer
    ```bash
     eachFile {
          if (name == "gravitino-env.sh" || name == "common.sh") {
            filter { line ->
              line.replace("GRAVITINO_VERSION_PLACEHOLDER", "$version")
            }
          }
        }
    ```

### Why are the changes needed?

To prevent incorrect usage with startup scripts

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?
- The scripts below will exit with status 1 and print an error message
with the correct instructions
```bash

cd bin
gravitino.sh.template start    
gravitino-iceberg-rest-server.sh.template start
```

- correct way to run gravitino : 
```bash
./gradle clean build -x test compileDistribution

cd distribution/package/bin

./gravitino.sh start

./gravitino-iceberg-rest-server.sh start

```
@hdygxsj @Abyss-lord
[apache#5194] feat(flink): Support basic table DDL Operation for paim…
0c78355 
…on-catalog (apache#6224)

### What changes were proposed in this pull request?

Support basic table DDL Operation for paimon-catalog

### Why are the changes needed?

Fix: apache#5194

### Does this PR introduce _any_ user-facing change?

None.

### How was this patch tested?


org.apache.gravitino.flink.connector.integration.test.paimon.FlinkPaimonCatalogIT
@sunxiaojian @Abyss-lord
[apache#6237]fix: add missing @OverRide annotations (apache#6244)
5e69663 
### What changes were proposed in this pull request?

Add missing `@override` annotations

### Why are the changes needed?

Fix:  apache#6237

### Does this PR introduce _any_ user-facing change?
N/A

### How was this patch tested?
N/A
@jerqi @Abyss-lord
[apache#6245] fix(authz): Authorization should use classloader to cre…
72080a8 
…ate the plugin (apache#6246)

### What changes were proposed in this pull request?

Authorization should use classloader to create the plugin

### Why are the changes needed?

Fix: apache#6245

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

I tested it manually.
@frankvicky @Abyss-lord
[apache#6243] fix(docs): Fix wrong seperator of yaml example in flink…
c08f5d9 
…-connector.md (apache#6259)

### What changes were proposed in this pull request?

Replace `=` with `:` to correct the yaml example.

### Why are the changes needed?
The yaml example uses wrong seperator.

Fix: apache#6243

### Does this PR introduce _any_ user-facing change?
None

### How was this patch tested?
None.
@yuqi1129 @Abyss-lord
[MINOR] fix(docs): Fix several document errors (apache#6251)
a748bec 
### What changes were proposed in this pull request?

Fix several errors in the document about hadoop-catalog and hive-catalog

### Why are the changes needed?

Improving the user experience. 

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

N/A.
@sunxiaojian @Abyss-lord
[apache#6239] fix: No need for toString() on string (CLI) (apache#6247)
b6a6993 
### What changes were proposed in this pull request?

Remove unnecessary toSrting() from CLI

### Why are the changes needed?

Fix: [#(6239)](apache#6239)

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

N/A
@Abyss-lord
[apache#5570] improve(CLI): Add the ability to revoke all roles or gr…
a584938 
…oups in the Gravitino CLI.

Add docs.
@Abyss-lord
Copy link
Contributor Author

The code looks fine to me. Can you also update the documentation and help content?

@justinmclean I’ve finished updating the code and docs. Please take a look at the PR again when you have time.

@justinmclean
Copy link
Member

Looks good, but you also need to update the description of -all in GravitinoOptions. Perhaps something like "on all entities".

@Abyss-lord
[apache#5570] improve(CLI): Add the ability to revoke all roles or gr…
b9f6635 
…oups in the Gravitino CLI.

fix description of all option.
@Abyss-lord
Copy link
Contributor Author

Looks good, but you also need to update the description of -all in GravitinoOptions. Perhaps something like "on all entities".

@justinmclean thank you Justin, I just fix the code.

justinmclean
justinmclean approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@justinmclean justinmclean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your contribution.

@justinmclean justinmclean merged commit 375116a into apache:main Jan 16, 2025
27 checks passed
@Abyss-lord Abyss-lord deleted the revoke-all branch January 16, 2025 03:06
Abyss-lord added a commit to Abyss-lord/gravitino that referenced this pull request Jan 16, 2025
@Abyss-lord @liuchunhao
@hdygxsj @sunxiaojian @jerqi @frankvicky @yuqi1129
[apache#5570] improve(CLI): Add the ability to revoke all roles or gr…
3df6134 
…oups in the Gravitino CLI. (apache#6240)

### What changes were proposed in this pull request?

Add ability to revoke all roles from a user or group in the Gravitino
CLI.
1. Add logic to handle the `--all` flag in
`UserCommandHandler#handleRevokeCommand`;
2. Add logic to handle the `--all `flag in
`GroupCommandHandler#handleRevokeCommand`;
3. Add new `RemoveAllRoles` command to handle user revoke --all and
group revoke `--all`;
4. Add unit tests;

### Why are the changes needed?

Fix: apache#5570 

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

UT + local test.

User test.

```bash
# step1: grant roles to testRole
gcli user grant -m demo_metalake --user testRole --role roleA roleB roleC -I
# roleA added to testRole
# roleB added to testRole
# roleC added to testRole
# Add roles roleA, roleB, roleC to user testRole

# step2: get details of testRole 
gcli user details -m demo_metalake --user testRole -I
# roleA,roleC,roleB

# step3: revoke all roles from testRole
gcli user revoke -m demo_metalake --user testRole -i --all
# Remove all roles from user testRole

# step4: get details of testRole
gcli user details -m demo_metalake --user testRole -I
# The user has no roles.

# step5: list exists roles
gcli role list -m demo_metalake -I
# admin,roleA,roleB,roleC
```

Group test.

```bash
# step1: grant roles to test_group
gcli group grant -m demo_metalake --group test_group --role roleA roleB roleC -I
# roleA added to test_group
# roleB added to test_group
# roleC added to test_group
# Grant roles roleA, roleB, roleC to group test_group

# step2: get details of test_group
gcli group details -m demo_metalake --group test_group -I
# admin,roleA,roleC,roleB

# step3: revoke all roles from test_group
gcli group revoke -m demo_metalake --group test_group -i --all
# Remove all roles from group test_group

# step4: get details of test_group
gcli group details -m demo_metalake --group test_group -I
# The group has no roles.

# step5: list exists roles
gcli role list -m demo_metalake -I
# admin,roleA,roleB,roleC
```

---------

Co-authored-by: Chun-Hao Liu <[email protected]>
Co-authored-by: yangyang zhong <[email protected]>
Co-authored-by: Xiaojian Sun <[email protected]>
Co-authored-by: roryqi <[email protected]>
Co-authored-by: TengYao Chi <[email protected]>
Co-authored-by: Qi Yu <[email protected]>
jerqi added a commit that referenced this pull request Jan 23, 2025
@Abyss-lord @liuchunhao
@hdygxsj @sunxiaojian @jerqi @frankvicky @yuqi1129
[#5570] improve(CLI): Add the ability to revoke all roles or groups i…
d06bfc3 
…n the Gravitino CLI. (#6240)

### What changes were proposed in this pull request?

Add ability to revoke all roles from a user or group in the Gravitino
CLI.
1. Add logic to handle the `--all` flag in
`UserCommandHandler#handleRevokeCommand`;
2. Add logic to handle the `--all `flag in
`GroupCommandHandler#handleRevokeCommand`;
3. Add new `RemoveAllRoles` command to handle user revoke --all and
group revoke `--all`;
4. Add unit tests;

### Why are the changes needed?

Fix: #5570 

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

UT + local test.

User test.

```bash
# step1: grant roles to testRole
gcli user grant -m demo_metalake --user testRole --role roleA roleB roleC -I
# roleA added to testRole
# roleB added to testRole
# roleC added to testRole
# Add roles roleA, roleB, roleC to user testRole

# step2: get details of testRole 
gcli user details -m demo_metalake --user testRole -I
# roleA,roleC,roleB

# step3: revoke all roles from testRole
gcli user revoke -m demo_metalake --user testRole -i --all
# Remove all roles from user testRole

# step4: get details of testRole
gcli user details -m demo_metalake --user testRole -I
# The user has no roles.

# step5: list exists roles
gcli role list -m demo_metalake -I
# admin,roleA,roleB,roleC
```

Group test.

```bash
# step1: grant roles to test_group
gcli group grant -m demo_metalake --group test_group --role roleA roleB roleC -I
# roleA added to test_group
# roleB added to test_group
# roleC added to test_group
# Grant roles roleA, roleB, roleC to group test_group

# step2: get details of test_group
gcli group details -m demo_metalake --group test_group -I
# admin,roleA,roleC,roleB

# step3: revoke all roles from test_group
gcli group revoke -m demo_metalake --group test_group -i --all
# Remove all roles from group test_group

# step4: get details of test_group
gcli group details -m demo_metalake --group test_group -I
# The group has no roles.

# step5: list exists roles
gcli role list -m demo_metalake -I
# admin,roleA,roleB,roleC
```

---------

Co-authored-by: Chun-Hao Liu <[email protected]>
Co-authored-by: yangyang zhong <[email protected]>
Co-authored-by: Xiaojian Sun <[email protected]>
Co-authored-by: roryqi <[email protected]>
Co-authored-by: TengYao Chi <[email protected]>
Co-authored-by: Qi Yu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmclean justinmclean justinmclean approved these changes

Assignees

@Abyss-lord Abyss-lord

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement] Add the ability to revoke all roles or groups in the Gravitino CLI
9 participants
@Abyss-lord @tengqm @justinmclean @liuchunhao @hdygxsj @sunxiaojian @jerqi @frankvicky @yuqi1129