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 2020/06/17 19:00:03 UTC
[GitHub] [hadoop-ozone] smengcl opened a new pull request #1087: HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled
smengcl opened a new pull request #1087:
URL: https://github.com/apache/hadoop-ozone/pull/1087
## What changes were proposed in this pull request?
Don't ignore `userName` input when ACL is enabled for `OzoneManager#listVolumeByUser`.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3819
## How was this patch tested?
No new unit test added. Yet?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1087: HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1087:
URL: https://github.com/apache/hadoop-ozone/pull/1087#discussion_r441770689
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1583,11 +1583,11 @@ private void checkAcls(ResourceType resType, StoreType store,
*
* @return true if permission granted, false if permission denied.
*/
- private boolean hasAcls(ResourceType resType, StoreType store,
- ACLType acl, String vol, String bucket, String key) {
+ private boolean hasAcls(String userName, ResourceType resType,
+ StoreType store, ACLType acl, String vol, String bucket, String key) {
try {
return checkAcls(resType, store, acl, vol, bucket, key,
- ProtobufRpcEngine.Server.getRemoteUser(),
+ UserGroupInformation.createRemoteUser(userName),
Review comment:
Question for @xiaoyuyao here: `UserGroupInformation.createRemoteUser()` create a new UGI with `AuthMethod.SIMPLE`. Does this matter?
IMO in this case only username matters.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1087: HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1087:
URL: https://github.com/apache/hadoop-ozone/pull/1087#discussion_r444507606
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1583,11 +1583,11 @@ private void checkAcls(ResourceType resType, StoreType store,
*
* @return true if permission granted, false if permission denied.
*/
- private boolean hasAcls(ResourceType resType, StoreType store,
- ACLType acl, String vol, String bucket, String key) {
+ private boolean hasAcls(String userName, ResourceType resType,
+ StoreType store, ACLType acl, String vol, String bucket, String key) {
try {
return checkAcls(resType, store, acl, vol, bucket, key,
- ProtobufRpcEngine.Server.getRemoteUser(),
+ UserGroupInformation.createRemoteUser(userName),
Review comment:
That should be fine in this case given the user has passed the RPC layer Kerberos auth.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1087: HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1087:
URL: https://github.com/apache/hadoop-ozone/pull/1087#discussion_r441818232
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerListVolumes.java
##########
@@ -146,7 +146,6 @@ private void setVolumeAcl(ObjectStore objectStore, String volumeName,
private void checkUser(MiniOzoneCluster cluster, UserGroupInformation user,
List<String> expectVol, boolean expectListAllSuccess) throws IOException {
- UserGroupInformation.setLoginUser(user);
Review comment:
Just a note:
Removing this line enables the existing tests to detect the bug solved by this jira.
You see previously I set the login user to the user I'm calling listVolume with, which unintentionally hides the bug.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl merged pull request #1087: HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled
Posted by GitBox <gi...@apache.org>.
smengcl merged pull request #1087:
URL: https://github.com/apache/hadoop-ozone/pull/1087
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1087: HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1087:
URL: https://github.com/apache/hadoop-ozone/pull/1087#discussion_r444405600
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerListVolumes.java
##########
@@ -146,7 +146,6 @@ private void setVolumeAcl(ObjectStore objectStore, String volumeName,
private void checkUser(MiniOzoneCluster cluster, UserGroupInformation user,
List<String> expectVol, boolean expectListAllSuccess) throws IOException {
- UserGroupInformation.setLoginUser(user);
Review comment:
I've also added new test cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1087: HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1087:
URL: https://github.com/apache/hadoop-ozone/pull/1087#discussion_r445090267
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1583,11 +1583,11 @@ private void checkAcls(ResourceType resType, StoreType store,
*
* @return true if permission granted, false if permission denied.
*/
- private boolean hasAcls(ResourceType resType, StoreType store,
- ACLType acl, String vol, String bucket, String key) {
+ private boolean hasAcls(String userName, ResourceType resType,
+ StoreType store, ACLType acl, String vol, String bucket, String key) {
try {
return checkAcls(resType, store, acl, vol, bucket, key,
- ProtobufRpcEngine.Server.getRemoteUser(),
+ UserGroupInformation.createRemoteUser(userName),
Review comment:
Sounds good. Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1087: HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1087:
URL: https://github.com/apache/hadoop-ozone/pull/1087#issuecomment-648991708
Thanks @xiaoyuyao for the review and +1. Merging in a minute.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org