You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/10/17 22:51:00 UTC
[GitHub] [ozone] aswinshakil opened a new pull request, #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
aswinshakil opened a new pull request, #3854:
URL: https://github.com/apache/ozone/pull/3854
## What changes were proposed in this pull request?
- Add error message when the same user is assigned as tenant admin.
- Add error message when the non-admin user is revoked as tenant admin.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7347
## How was this patch tested?
The patch was tested using unit tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3854:
URL: https://github.com/apache/ozone/pull/3854#issuecomment-1281603847
Why can't these APIs be idempotent, if the user is already admin, it should pass, and if the user is not admin, it revoke should pass as well. Why raise an exception?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3854:
URL: https://github.com/apache/ozone/pull/3854#discussion_r997598889
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java:
##########
@@ -399,6 +399,14 @@ public void testAssignAdmin() throws IOException {
+ " \"isDelegatedAdmin\": true\n" + "}\n", true, true);
checkOutput(err, "", true);
+ //Re-assign same admin
Review Comment:
nit
```suggestion
// Assign admin again, expect failure message
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3854:
URL: https://github.com/apache/ozone/pull/3854#discussion_r997598165
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignAdminRequest.java:
##########
@@ -116,6 +116,13 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
OMException.ResultCodes.INVALID_TENANT_ID);
}
+ // Prevent assigning the same user as admin.
+ if (accessIdInfo.getIsAdmin()) {
+ throw new OMException("accessId '" + accessId +
+ "' is already the tenant '" + tenantId + "' admin.",
Review Comment:
```suggestion
"' is already a tenant admin of '" + tenantId + "'.",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl closed pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
Posted by GitBox <gi...@apache.org>.
smengcl closed pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
URL: https://github.com/apache/ozone/pull/3854
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
Posted by GitBox <gi...@apache.org>.
smengcl commented on PR #3854:
URL: https://github.com/apache/ozone/pull/3854#issuecomment-1286536102
Thanks @aswinshakil for the effort. Closing this PR without merging it after our discussion with @kerneltime .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on PR #3854:
URL: https://github.com/apache/ozone/pull/3854#issuecomment-1281619837
The goal is to propagate back clear messages for these scenarios to improve user experience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3854:
URL: https://github.com/apache/ozone/pull/3854#discussion_r997598096
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeAdminRequest.java:
##########
@@ -116,6 +116,13 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
OMException.ResultCodes.INVALID_TENANT_ID);
}
+ // Check if the accessId is already the tenant admin
+ if (!accessIdInfo.getIsAdmin()) {
+ throw new OMException("accessId '" + accessId +
+ "' is already not the tenant '" + tenantId + "' admin.",
Review Comment:
```suggestion
"' is not a tenant admin of '" + tenantId + "'.",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3854: HDDS-7347. [Multi-Tenant] Add proper error message to TenantAssignAdmin and TenantRevokeAdmin
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3854:
URL: https://github.com/apache/ozone/pull/3854#issuecomment-1282780962
> The goal is to propagate back clear messages for these scenarios to improve user experience.
I would still push back against this change. Usually, set ownership or admin access APIs/CLIs are idempotent. The only variation I can imagine is printing that the user was already admin or had access revoked, but the exit/return code/status should be successful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org