HBASE-21995 Add a coprocessor to set HDFS ACL for hbase granted user #163
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
openinx
reviewed
Apr 19, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java
Outdated
Show resolved
Hide resolved
| @@ -314,12 +316,24 @@ private FSDataInputStream tryOpen() throws IOException { | |||
| return(in); | |||
| } catch (FileNotFoundException e) { | |||
| // Try another file location | |||
| } catch (AccessControlException e) { | |||
There was a problem hiding this comment.
Here, I prefer to simplify the logic as a small method:
- remember the thrown exception as e;
- if notfound or accessControl exception, continue to try another file;
- if still not find an right file. then throw the e.
Please consider this.
There was a problem hiding this comment.
Please abstract all the exception handling logic as method named handleException ?
| for (int i = 0; i < locations.length; ++i) { | ||
| try { | ||
| return fs.getFileStatus(locations[i]); | ||
| } catch (FileNotFoundException e) { | ||
| // Try another file location | ||
| } catch (AccessControlException e) { |
| */ | ||
| @CoreCoprocessor | ||
| @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.CONFIG) | ||
| public class HDFSAclController implements MasterCoprocessor, MasterObserver { |
There was a problem hiding this comment.
Not a good class name, the class want to sync file acl between HBase and HDFS ? and mostly for those directories when scanning snapshot ? we don't consider those directories which is unrelated to snapshot, such as WAL, oldWals etc... Please consider another name.
There was a problem hiding this comment.
How about "SnapshotScannerHDFSAclController"?
| } | ||
|
|
||
| @Override | ||
| public void preMasterInitialization(final ObserverContext<MasterCoprocessorEnvironment> c) |
There was a problem hiding this comment.
the final can remove now in jdk8 ?
| masterServices = ((HasMasterServices) mEnv).getMasterServices(); | ||
| } | ||
| if (masterServices == null) { | ||
| throw new RuntimeException("master services can not be null"); |
| if (!fs.exists(path)) { | ||
| fs.mkdirs(path); | ||
| } | ||
| fs.setPermission(path, ACL_ENABLE_PUBLIC_HFILE_PERMISSION); |
There was a problem hiding this comment.
This acl need also to be configurable ?
| ColumnFamilyDescriptorBuilder.newBuilder(HDFSAclStorage.HDFS_ACL_FAMILY).build()); | ||
| admin.modifyTable(builder.build()); | ||
| } | ||
| } |
There was a problem hiding this comment.
What if the hbase:acl does not exist ? should throw an exception ?
There was a problem hiding this comment.
This coprocessor should be configured after the AccessController, if hbase:acl table does not exist, the AccessController will not work incorrectly firstly?
Let me add some logs and throw an TableNotFoundException here.
| try (Admin admin = ctx.getEnvironment().getConnection().getAdmin()) { | ||
| if (admin.tableExists(PermissionStorage.ACL_TABLE_NAME)) { | ||
| // check if hbase:acl table has 'm' CF | ||
| TableDescriptor tableDescriptor = admin.getDescriptor(PermissionStorage.ACL_TABLE_NAME); |
There was a problem hiding this comment.
Would the newly introduced CF impact the original AccessController ?
There was a problem hiding this comment.
The new CF is only used in this CP, it records if the hbase read permission is synchronized to related hfile.
This flag has two usages:
- check if we need to remove hdfs acls for a grant without READ permission;
- skip some hdfs acl sync because it may be already added.
|
💔 -1 overall
This message was automatically generated. |
This was referenced Apr 22, 2019
|
💔 -1 overall
This message was automatically generated. |
This was referenced May 4, 2019
|
💔 -1 overall
This message was automatically generated. |
|
@mymeiyi Please check the failed UT. |
|
Any updates here? |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
This was referenced May 16, 2019
This was referenced May 28, 2019
|
💔 -1 overall
This message was automatically generated. |
This was referenced Jan 21, 2022
Merged
Merged
This was referenced Jan 29, 2022
Merged
This was referenced Feb 5, 2022
Merged
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.
To make hbase granted user have the access to scan table snapshots, use HDFS ACLs to set user 'access r-x' or 'default r-x' ACLs over hfiles.
The basic implementation is:
The feature is configurable because it's implemented in a master coprocessor.