HDDS-1600. Add userName and IPAddress as part of OMRequest. #857
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bharatviswa504
force-pushed
the
HDDS-1600
branch
2 times, most recently
from
b4a0740 to
1d18242
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
xiaoyuyao
reviewed
May 31, 2019
xiaoyuyao
reviewed
May 31, 2019
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Show resolved
Hide resolved
xiaoyuyao
reviewed
May 31, 2019
| throw new OzoneAclException("User " + user.getUserName() + " doesn't " + | ||
| "have " + acl + " permission to access " + resType, | ||
| ErrorCode.PERMISSION_DENIED); | ||
| user.getUserName(), aclType, resType); |
There was a problem hiding this comment.
The unit test failure seems related:
TestOmAcls.testOMAclsPermissionDenied
org.apache.hadoop.ozone.om.exceptions.OMException: User jenkins1000 doesn't have CREATE permission to access volume
There was a problem hiding this comment.
Yes @xiaoyuyao. Will address in the next patch along with review comments.
xiaoyuyao
reviewed
May 31, 2019
| volumeName, bucketName, null); | ||
| } | ||
|
|
||
| metadataManager.getLock().acquireVolumeLock(volumeName); |
There was a problem hiding this comment.
I think the right lock pattern is to acquire lock out of try {} block and release them in the final block.
xiaoyuyao
reviewed
May 31, 2019
| } | ||
|
|
||
| // acquire lock | ||
| omMetadataManager.getLock().acquireBucketLock(volumeName, bucketName); |
xiaoyuyao
reviewed
May 31, 2019
...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java
Show resolved
Hide resolved
xiaoyuyao
reviewed
May 31, 2019
There was a problem hiding this comment.
Thanks @bharatviswa504 for working on this. Patch LGTM overall. A few minor comments inline.
|
@bharatviswa504 thanks for the patch. On a second thought i wonder why don't we complete authorization on the OM which receives the first request from client, this will save us the trouble of propagating credentials in rest of the call and simplify HA design. |
We cannot do checkAcls on any OM(which some times might not be leader), because think of a case like setAcl's is not applied on that OM(as it is a follower) but we are performing check Acl's. Discussed offline with @xiaoyuyao and @ajayydv, we cannot take this approach as OM followers can lag leader OM, so it might not have latest changes, if we do check on Non-leader OM, we might see some inconsistent behavior. |
|
Hi @xiaoyuyao |
|
LGTM, thanks for the update. +1, pending CI. |
|
💔 -1 overall
This message was automatically generated. |
|
/retest |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Checkstyle issues and test failures are not related to this patch. |
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
Author: Hai Lu <halu@linkedin.com> Reviewers: Xinyu Liu <xiliu@linkedin.com> Closes apache#857 from lhaiesp/master
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In OM HA, the actual execution of request happens under GRPC context, so UGI object which we retrieve from ProtobufRpcEngine.Server.getRemoteUser(); will not be available.
In similar manner ProtobufRpcEngine.Server.getRemoteIp().
So, during preExecute(which happens under RPC context) extract userName and IPAddress and add it to the OMRequest, and then send the request to ratis server.