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/06/10 12:58:02 UTC
[GitHub] [ozone] kerneltime opened a new pull request, #3503: HDDS-6870 Clean up isTenantAdmin to use UGI
kerneltime opened a new pull request, #3503:
URL: https://github.com/apache/ozone/pull/3503
## What changes were proposed in this pull request?
Avoid duplication of dealing with short and full names that are part of UGI.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6870
## How was this patch tested?
Existing tests run by CI
--
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 #3503: HDDS-6870. Clean up isTenantAdmin to use UGI
Posted by GitBox <gi...@apache.org>.
smengcl commented on PR #3503:
URL: https://github.com/apache/ozone/pull/3503#issuecomment-1154243628
CI failure looks related:
```
Error: Tests run: 4, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 4.8 s <<< FAILURE! - in org.apache.hadoop.ozone.om.ratis.TestOzoneManagerStateMachine
Error: org.apache.hadoop.ozone.om.ratis.TestOzoneManagerStateMachine.testPreAppendTransaction Time elapsed: 4.217 s <<< ERROR!
java.lang.IllegalArgumentException: Null user
at org.apache.hadoop.security.UserGroupInformation.createRemoteUser(UserGroupInformation.java:1415)
at org.apache.hadoop.security.UserGroupInformation.createRemoteUser(UserGroupInformation.java:1402)
at org.apache.hadoop.ozone.om.ratis.OzoneManagerStateMachine.preAppendTransaction(OzoneManagerStateMachine.java:246)
...
```
https://github.com/apache/ozone/runs/6866680894
--
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] errose28 merged pull request #3503: HDDS-6870. Clean up isTenantAdmin to use UGI
Posted by GitBox <gi...@apache.org>.
errose28 merged PR #3503:
URL: https://github.com/apache/ozone/pull/3503
--
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 #3503: HDDS-6870 Clean up isTenantAdmin to use UGI
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3503:
URL: https://github.com/apache/ozone/pull/3503#discussion_r894814085
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMFinalizeUpgradeRequest.java:
##########
@@ -66,10 +66,11 @@ public OMClientResponse validateAndUpdateCache(
OMClientResponse response = null;
try {
- String username = getOmRequest().getUserInfo().getUserName();
- if (ozoneManager.getAclsEnabled() && !ozoneManager.isAdmin(username)) {
- throw new OMException("Access denied for user " + username + ". " +
- "Superuser privilege is required to finalize upgrade.",
+ if (ozoneManager.getAclsEnabled()
+ && !ozoneManager.isAdmin(createUGI())) {
+ throw new OMException("Access denied for user "
+ + createUGI() + ". "
+ + "Superuser privilege is required to finalize upgrade.",
OMException.ResultCodes.ACCESS_DENIED);
}
Review Comment:
Same for `OMCancelPrepareRequest`
--
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 #3503: HDDS-6870 Clean up isTenantAdmin to use UGI
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3503:
URL: https://github.com/apache/ozone/pull/3503#issuecomment-1152620496
This builds on top of https://github.com/apache/ozone/pull/3502
--
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 #3503: HDDS-6870 Clean up isTenantAdmin to use UGI
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3503:
URL: https://github.com/apache/ozone/pull/3503#discussion_r894812764
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMFinalizeUpgradeRequest.java:
##########
@@ -66,10 +66,11 @@ public OMClientResponse validateAndUpdateCache(
OMClientResponse response = null;
try {
- String username = getOmRequest().getUserInfo().getUserName();
- if (ozoneManager.getAclsEnabled() && !ozoneManager.isAdmin(username)) {
- throw new OMException("Access denied for user " + username + ". " +
- "Superuser privilege is required to finalize upgrade.",
+ if (ozoneManager.getAclsEnabled()
+ && !ozoneManager.isAdmin(createUGI())) {
+ throw new OMException("Access denied for user "
+ + createUGI() + ". "
+ + "Superuser privilege is required to finalize upgrade.",
OMException.ResultCodes.ACCESS_DENIED);
}
Review Comment:
```suggestion
if (ozoneManager.getAclsEnabled()) {
final UserGroupInformation ugi = createUGI();
if (!ozoneManager.isAdmin(ugi)) {
throw new OMException("Access denied for user " + ugi + ". "
+ "Superuser privilege is required to finalize upgrade.",
OMException.ResultCodes.ACCESS_DENIED);
}
}
```
--
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 #3503: HDDS-6870 Clean up isTenantAdmin to use UGI
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3503:
URL: https://github.com/apache/ozone/pull/3503#discussion_r894815243
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMFinalizeUpgradeRequest.java:
##########
@@ -66,10 +66,11 @@ public OMClientResponse validateAndUpdateCache(
OMClientResponse response = null;
try {
- String username = getOmRequest().getUserInfo().getUserName();
- if (ozoneManager.getAclsEnabled() && !ozoneManager.isAdmin(username)) {
- throw new OMException("Access denied for user " + username + ". " +
- "Superuser privilege is required to finalize upgrade.",
+ if (ozoneManager.getAclsEnabled()
+ && !ozoneManager.isAdmin(createUGI())) {
+ throw new OMException("Access denied for user "
+ + createUGI() + ". "
+ + "Superuser privilege is required to finalize upgrade.",
OMException.ResultCodes.ACCESS_DENIED);
}
Review Comment:
I see that `createUGI()` saves the result from `UGI#createRemoteUser`.
So this could be optional. But I personally think this is cleaner.
--
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 a diff in pull request #3503: HDDS-6870 Clean up isTenantAdmin to use UGI
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3503:
URL: https://github.com/apache/ozone/pull/3503#discussion_r894900492
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMFinalizeUpgradeRequest.java:
##########
@@ -66,10 +66,11 @@ public OMClientResponse validateAndUpdateCache(
OMClientResponse response = null;
try {
- String username = getOmRequest().getUserInfo().getUserName();
- if (ozoneManager.getAclsEnabled() && !ozoneManager.isAdmin(username)) {
- throw new OMException("Access denied for user " + username + ". " +
- "Superuser privilege is required to finalize upgrade.",
+ if (ozoneManager.getAclsEnabled()
+ && !ozoneManager.isAdmin(createUGI())) {
+ throw new OMException("Access denied for user "
+ + createUGI() + ". "
+ + "Superuser privilege is required to finalize upgrade.",
OMException.ResultCodes.ACCESS_DENIED);
}
Review Comment:
the second call is in an exception path and the code is one less line ;-)
--
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