[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r303277745
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
##
@@ -515,6 +560,18 @@ private static AclEntry aclEntry(AclEntryScope scope,
String name) {
.setPermission(READ_EXECUTE).build();
}
+ void createNonExistDir(Path path) throws IOException {
Review comment:
createDirIfNotExist.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r303277299
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java
##
@@ -138,161 +142,185 @@ public void
preMasterInitialization(ObserverContext c)
throws IOException {
-if (checkInitialized()) {
- try (Admin admin = c.getEnvironment().getConnection().getAdmin()) {
-if (admin.tableExists(PermissionStorage.ACL_TABLE_NAME)) {
- // Check if hbase acl table has 'm' CF, if not, add 'm' CF
- TableDescriptor tableDescriptor =
admin.getDescriptor(PermissionStorage.ACL_TABLE_NAME);
- boolean containHdfsAclFamily =
-
Arrays.stream(tableDescriptor.getColumnFamilies()).anyMatch(family -> Bytes
- .equals(family.getName(),
SnapshotScannerHDFSAclStorage.HDFS_ACL_FAMILY));
- if (!containHdfsAclFamily) {
-TableDescriptorBuilder builder =
TableDescriptorBuilder.newBuilder(tableDescriptor)
-.setColumnFamily(ColumnFamilyDescriptorBuilder
-
.newBuilder(SnapshotScannerHDFSAclStorage.HDFS_ACL_FAMILY).build());
-admin.modifyTable(builder.build());
+if (!initialized) {
+ return;
+}
+try (Admin admin = c.getEnvironment().getConnection().getAdmin()) {
+ if (admin.tableExists(PermissionStorage.ACL_TABLE_NAME)) {
+// Check if acl table has 'm' CF, if not, add 'm' CF
+TableDescriptor tableDescriptor =
admin.getDescriptor(PermissionStorage.ACL_TABLE_NAME);
+boolean containHdfsAclFamily =
Arrays.stream(tableDescriptor.getColumnFamilies()).anyMatch(
+ family -> Bytes.equals(family.getName(),
SnapshotScannerHDFSAclStorage.HDFS_ACL_FAMILY));
+if (!containHdfsAclFamily) {
+ TableDescriptorBuilder builder =
TableDescriptorBuilder.newBuilder(tableDescriptor)
+ .setColumnFamily(ColumnFamilyDescriptorBuilder
+
.newBuilder(SnapshotScannerHDFSAclStorage.HDFS_ACL_FAMILY).build());
+ admin.modifyTable(builder.build());
+}
+// wait until acl table is online
+List tableRegions = MetaTableAccessor.getTableRegions(
Review comment:
Why to wait & assign the hbase:acl region ? In theory, the modifyTable will
ensure that all of the regions are online.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r298539875
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java
##
@@ -263,14 +262,29 @@ public void
postDeleteTable(ObserverContext ctx,
}
}
+ @Override
+ public void postModifyTable(ObserverContext
ctx,
+ TableName tableName, TableDescriptor oldDescriptor, TableDescriptor
currentDescriptor)
+ throws IOException {
+if (needHandleTableHdfsAcl(currentDescriptor, "modifyTable " + tableName)
+&& !hdfsAclHelper.isTableUserScanSnapshotEnabled(oldDescriptor)) {
+ hdfsAclHelper.createTableDirectories(tableName);
+ Set tableUsers =
hdfsAclHelper.getUsersWithTableReadAction(tableName, false, false);
+ Set users =
+
hdfsAclHelper.getUsersWithNamespaceReadAction(tableName.getNamespaceAsString(),
true);
+ users.addAll(tableUsers);
+ hdfsAclHelper.addTableAcl(tableName, users);
+
SnapshotScannerHDFSAclStorage.addUserTableHdfsAcl(ctx.getEnvironment().getConnection(),
+tableUsers, tableName);
+}
Review comment:
How it will happen if we have an TableUserScanSnapshot Enabled table, and
then modify to disabled ? I think we shold also remove the HDFS ACL ?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table URL: https://github.com/apache/hbase/pull/336#discussion_r298150480 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java ## @@ -151,6 +157,8 @@ public void postStartMaster(ObserverContext c) thr .setColumnFamily(ColumnFamilyDescriptorBuilder .newBuilder(SnapshotScannerHDFSAclStorage.HDFS_ACL_FAMILY).build()); admin.modifyTable(builder.build()); Review comment: After modifyTable succesfuly, the aclTableInitialized should also set to true ? IMO, once the acl table contains the HDFS_ACL_FAMILY, then we should init the flag to true ? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r298151393
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java
##
@@ -513,12 +518,33 @@ private boolean isHdfsAclSet(Table aclTable, String
userName, String namespace,
return isSet;
}
- private boolean checkInitialized() {
+ @VisibleForTesting
+ boolean checkInitialized(Supplier operation) {
Review comment:
Just use the string as the argument ? I don't think here we need the
Supplier wrapper ...
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r298151667
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java
##
@@ -513,12 +518,33 @@ private boolean isHdfsAclSet(Table aclTable, String
userName, String namespace,
return isSet;
}
- private boolean checkInitialized() {
+ @VisibleForTesting
+ boolean checkInitialized(Supplier operation) {
if (initialized) {
- return true;
-} else {
- return false;
+ if (aclTableInitialized) {
+return true;
+ } else {
+LOG.warn("Skip set HDFS acls because acl table is not initialized when
" + operation.get());
+ }
}
+return false;
+ }
+
+ private boolean needSetTableHdfsAcl(TablePermission tablePermission) throws
IOException {
+return needSetTableHdfsAcl(tablePermission.getTableName(), () -> "")
+&& hdfsAclHelper.checkTablePermissionHasNoCfOrCq(tablePermission);
+ }
+
+ private boolean needSetTableHdfsAcl(TableName tableName, Supplier
operation)
Review comment:
Also can remoe the Supplier wrapper ?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r298155797
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
##
@@ -447,28 +458,80 @@ private void setTableAcl(TableName tableName,
Set users)
.collect(Collectors.toList());
}
+ /**
+ * Return users with global read permission
+ * @return users with global read permission
+ * @throws IOException if an error occurred
+ */
+ private Set getUsersWithGlobalReadAction() throws IOException {
+return
getUsersWithReadAction(PermissionStorage.getGlobalPermissions(conf));
+ }
+
/**
* Return users with namespace read permission
* @param namespace the namespace
+ * @param includeGlobal true if include users with global read action
* @return users with namespace read permission
* @throws IOException if an error occurred
*/
- private Set getUsersWithNamespaceReadAction(String namespace) throws
IOException {
-return PermissionStorage.getNamespacePermissions(conf,
namespace).entries().stream()
-.filter(entry -> entry.getValue().getPermission().implies(READ))
-.map(entry -> entry.getKey()).collect(Collectors.toSet());
+ Set getUsersWithNamespaceReadAction(String namespace, boolean
includeGlobal)
+ throws IOException {
+Set users =
+getUsersWithReadAction(PermissionStorage.getNamespacePermissions(conf,
namespace));
+if (includeGlobal) {
+ users.addAll(getUsersWithGlobalReadAction());
+}
+return users;
}
/**
* Return users with table read permission
* @param tableName the table
+ * @param includeNamespace true if include users with namespace read action
+ * @param includeGlobal true if include users with global read action
* @return users with table read permission
* @throws IOException if an error occurred
*/
- private Set getUsersWithTableReadAction(TableName tableName) throws
IOException {
-return PermissionStorage.getTablePermissions(conf,
tableName).entries().stream()
-.filter(entry -> entry.getValue().getPermission().implies(READ))
-.map(entry -> entry.getKey()).collect(Collectors.toSet());
+ Set getUsersWithTableReadAction(TableName tableName, boolean
includeNamespace,
+ boolean includeGlobal) throws IOException {
+Set users =
+getUsersWithReadAction(PermissionStorage.getTablePermissions(conf,
tableName));
+if (includeNamespace) {
+ users
+
.addAll(getUsersWithNamespaceReadAction(tableName.getNamespaceAsString(),
includeGlobal));
+}
+return users;
+ }
+
+ private Set
+ getUsersWithReadAction(ListMultimap
permissionMultimap) {
+return permissionMultimap.entries().stream()
+.filter(entry -> checkUserPermission(entry.getValue())).map(entry ->
entry.getKey())
+.collect(Collectors.toSet());
+ }
+
+ private boolean checkUserPermission(UserPermission userPermission) {
+boolean result = containReadAction(userPermission);
+if (result && userPermission.getPermission() instanceof TablePermission) {
+ result = checkTablePermissionHasNoCfOrCq((TablePermission)
userPermission.getPermission());
+}
+return result;
+ }
+
+ boolean containReadAction(UserPermission userPermission) {
+return userPermission.getPermission().implies(Permission.Action.READ);
+ }
+
+ boolean checkTablePermissionHasNoCfOrCq(TablePermission tablePermission) {
+return !tablePermission.hasFamily() && !tablePermission.hasQualifier();
+ }
+
+ boolean isTableUserScanSnapshotEnabled(TableDescriptor tableDescriptor) {
+String value = tableDescriptor.getValue(USER_SCAN_SNAPSHOT_ENABLE);
+if (value != null && value.equals("true")) {
Review comment:
Just :
```
return Boolean.valueOf(value) ?
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r298162008
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
##
@@ -447,28 +458,80 @@ private void setTableAcl(TableName tableName,
Set users)
.collect(Collectors.toList());
}
+ /**
+ * Return users with global read permission
+ * @return users with global read permission
+ * @throws IOException if an error occurred
+ */
+ private Set getUsersWithGlobalReadAction() throws IOException {
+return
getUsersWithReadAction(PermissionStorage.getGlobalPermissions(conf));
+ }
+
/**
* Return users with namespace read permission
* @param namespace the namespace
+ * @param includeGlobal true if include users with global read action
* @return users with namespace read permission
* @throws IOException if an error occurred
*/
- private Set getUsersWithNamespaceReadAction(String namespace) throws
IOException {
-return PermissionStorage.getNamespacePermissions(conf,
namespace).entries().stream()
-.filter(entry -> entry.getValue().getPermission().implies(READ))
-.map(entry -> entry.getKey()).collect(Collectors.toSet());
+ Set getUsersWithNamespaceReadAction(String namespace, boolean
includeGlobal)
+ throws IOException {
+Set users =
+getUsersWithReadAction(PermissionStorage.getNamespacePermissions(conf,
namespace));
+if (includeGlobal) {
+ users.addAll(getUsersWithGlobalReadAction());
+}
+return users;
}
/**
* Return users with table read permission
* @param tableName the table
+ * @param includeNamespace true if include users with namespace read action
+ * @param includeGlobal true if include users with global read action
* @return users with table read permission
* @throws IOException if an error occurred
*/
- private Set getUsersWithTableReadAction(TableName tableName) throws
IOException {
-return PermissionStorage.getTablePermissions(conf,
tableName).entries().stream()
-.filter(entry -> entry.getValue().getPermission().implies(READ))
-.map(entry -> entry.getKey()).collect(Collectors.toSet());
+ Set getUsersWithTableReadAction(TableName tableName, boolean
includeNamespace,
+ boolean includeGlobal) throws IOException {
+Set users =
+getUsersWithReadAction(PermissionStorage.getTablePermissions(conf,
tableName));
+if (includeNamespace) {
+ users
+
.addAll(getUsersWithNamespaceReadAction(tableName.getNamespaceAsString(),
includeGlobal));
+}
+return users;
+ }
+
+ private Set
+ getUsersWithReadAction(ListMultimap
permissionMultimap) {
+return permissionMultimap.entries().stream()
+.filter(entry -> checkUserPermission(entry.getValue())).map(entry ->
entry.getKey())
+.collect(Collectors.toSet());
+ }
+
+ private boolean checkUserPermission(UserPermission userPermission) {
+boolean result = containReadAction(userPermission);
+if (result && userPermission.getPermission() instanceof TablePermission) {
+ result = checkTablePermissionHasNoCfOrCq((TablePermission)
userPermission.getPermission());
+}
+return result;
+ }
+
+ boolean containReadAction(UserPermission userPermission) {
+return userPermission.getPermission().implies(Permission.Action.READ);
+ }
+
+ boolean checkTablePermissionHasNoCfOrCq(TablePermission tablePermission) {
+return !tablePermission.hasFamily() && !tablePermission.hasQualifier();
+ }
+
+ boolean isTableUserScanSnapshotEnabled(TableDescriptor tableDescriptor) {
+String value = tableDescriptor.getValue(USER_SCAN_SNAPSHOT_ENABLE);
Review comment:
The flag should mean whether will we sync the table access to HDFS files
ACL ? I think we should a more clear name ?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r298160417
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java
##
@@ -263,14 +264,32 @@ public void
postDeleteTable(ObserverContext ctx,
}
}
+ @Override
+ public void postModifyTable(ObserverContext
ctx,
+ TableName tableName, TableDescriptor oldDescriptor, TableDescriptor
currentDescriptor)
+ throws IOException {
+if (needSetTableHdfsAcl(currentDescriptor, () -> "modifyTable " +
tableName)
+&& !hdfsAclHelper.isTableUserScanSnapshotEnabled(oldDescriptor)) {
+ hdfsAclHelper.createTableDirectories(tableName);
+ Set tableUsers =
hdfsAclHelper.getUsersWithTableReadAction(tableName, false, false);
+ Set users =
+
hdfsAclHelper.getUsersWithNamespaceReadAction(tableName.getNamespaceAsString(),
true);
+ users.addAll(tableUsers);
+ hdfsAclHelper.addTableAcl(tableName, users);
+
SnapshotScannerHDFSAclStorage.addUserTableHdfsAcl(ctx.getEnvironment().getConnection(),
+tableUsers, tableName);
+} else if (!aclTableInitialized
+&& Bytes.equals(PermissionStorage.ACL_GLOBAL_NAME,
tableName.getName())) {
+ aclTableInitialized = true;
Review comment:
Oh, you update the aclTableInitialized here when add HDFS_ACL_FAMILY in
hbase:acl ? Why just update the flag once the modifyTable called successfully
? I think that will be easy to understand.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r298154042
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
##
@@ -447,28 +458,80 @@ private void setTableAcl(TableName tableName,
Set users)
.collect(Collectors.toList());
}
+ /**
+ * Return users with global read permission
+ * @return users with global read permission
+ * @throws IOException if an error occurred
+ */
+ private Set getUsersWithGlobalReadAction() throws IOException {
+return
getUsersWithReadAction(PermissionStorage.getGlobalPermissions(conf));
+ }
+
/**
* Return users with namespace read permission
* @param namespace the namespace
+ * @param includeGlobal true if include users with global read action
* @return users with namespace read permission
* @throws IOException if an error occurred
*/
- private Set getUsersWithNamespaceReadAction(String namespace) throws
IOException {
-return PermissionStorage.getNamespacePermissions(conf,
namespace).entries().stream()
-.filter(entry -> entry.getValue().getPermission().implies(READ))
-.map(entry -> entry.getKey()).collect(Collectors.toSet());
+ Set getUsersWithNamespaceReadAction(String namespace, boolean
includeGlobal)
+ throws IOException {
+Set users =
+getUsersWithReadAction(PermissionStorage.getNamespacePermissions(conf,
namespace));
+if (includeGlobal) {
+ users.addAll(getUsersWithGlobalReadAction());
+}
+return users;
}
/**
* Return users with table read permission
* @param tableName the table
+ * @param includeNamespace true if include users with namespace read action
+ * @param includeGlobal true if include users with global read action
* @return users with table read permission
* @throws IOException if an error occurred
*/
- private Set getUsersWithTableReadAction(TableName tableName) throws
IOException {
-return PermissionStorage.getTablePermissions(conf,
tableName).entries().stream()
-.filter(entry -> entry.getValue().getPermission().implies(READ))
-.map(entry -> entry.getKey()).collect(Collectors.toSet());
+ Set getUsersWithTableReadAction(TableName tableName, boolean
includeNamespace,
+ boolean includeGlobal) throws IOException {
+Set users =
+getUsersWithReadAction(PermissionStorage.getTablePermissions(conf,
tableName));
+if (includeNamespace) {
+ users
+
.addAll(getUsersWithNamespaceReadAction(tableName.getNamespaceAsString(),
includeGlobal));
+}
+return users;
+ }
+
+ private Set
+ getUsersWithReadAction(ListMultimap
permissionMultimap) {
+return permissionMultimap.entries().stream()
+.filter(entry -> checkUserPermission(entry.getValue())).map(entry ->
entry.getKey())
+.collect(Collectors.toSet());
+ }
+
+ private boolean checkUserPermission(UserPermission userPermission) {
+boolean result = containReadAction(userPermission);
+if (result && userPermission.getPermission() instanceof TablePermission) {
+ result = checkTablePermissionHasNoCfOrCq((TablePermission)
userPermission.getPermission());
+}
+return result;
+ }
+
+ boolean containReadAction(UserPermission userPermission) {
+return userPermission.getPermission().implies(Permission.Action.READ);
+ }
+
+ boolean checkTablePermissionHasNoCfOrCq(TablePermission tablePermission) {
Review comment:
checkTablePermissionHasNoCfOrCq - > isFamilyOrColumnPermission ?
return it as : `tablePermission.hasFamily() ||
tablePermission.hasQualifier()` .
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r297130175
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java
##
@@ -263,10 +265,36 @@ public void
postDeleteTable(ObserverContext ctx,
}
}
+ @Override
+ public void postModifyTable(ObserverContext
ctx,
+ TableName tableName, TableDescriptor oldDescriptor, TableDescriptor
currentDescriptor)
+ throws IOException {
+if (checkTable(currentDescriptor, () -> "modifyTable " + tableName)
+&& !hdfsAclHelper.isTableUserScanSnapshotEnabled(oldDescriptor)) {
+ hdfsAclHelper.createTableDirectories(tableName);
+ Set users = new HashSet<>();
+ users.addAll(hdfsAclHelper.getUsersWithGlobalReadAction());
+
users.addAll(hdfsAclHelper.getUsersWithNamespaceReadAction(tableName.getNamespaceAsString()));
+ Set userWithTableReadPermission =
+ hdfsAclHelper.getUsersWithTableReadAction(tableName);
+ users.addAll(userWithTableReadPermission);
+ hdfsAclHelper.addTableAcl(tableName, users);
+ try (Table aclTable =
Review comment:
Also can abstract a method for the flag updating
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r297129520
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclController.java
##
@@ -263,10 +265,36 @@ public void
postDeleteTable(ObserverContext ctx,
}
}
+ @Override
+ public void postModifyTable(ObserverContext
ctx,
+ TableName tableName, TableDescriptor oldDescriptor, TableDescriptor
currentDescriptor)
+ throws IOException {
+if (checkTable(currentDescriptor, () -> "modifyTable " + tableName)
+&& !hdfsAclHelper.isTableUserScanSnapshotEnabled(oldDescriptor)) {
+ hdfsAclHelper.createTableDirectories(tableName);
+ Set users = new HashSet<>();
Review comment:
Abstract an small method here ? I saw many places has the logic: to pick
up all the users who has the read permission on table / ns / global etc...
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r297126966
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
##
@@ -465,10 +484,36 @@ private void setTableAcl(TableName tableName,
Set users)
* @return users with table read permission
* @throws IOException if an error occurred
*/
- private Set getUsersWithTableReadAction(TableName tableName) throws
IOException {
-return PermissionStorage.getTablePermissions(conf,
tableName).entries().stream()
-.filter(entry -> entry.getValue().getPermission().implies(READ))
-.map(entry -> entry.getKey()).collect(Collectors.toSet());
+ Set getUsersWithTableReadAction(TableName tableName) throws
IOException {
+return getUsersWithReadAction(PermissionStorage.getTablePermissions(conf,
tableName));
+ }
+
+ private Set
+ getUsersWithReadAction(ListMultimap
permissionMultimap) {
+return permissionMultimap.entries().stream()
+.filter(entry -> checkUserPermission(entry.getValue())).map(entry ->
entry.getKey())
+.collect(Collectors.toSet());
+ }
+
+ boolean checkUserPermission(UserPermission userPermission) {
+boolean result = containReadAction(userPermission);
+if (result && userPermission.getPermission() instanceof TablePermission) {
+ result = checkTablePermission((TablePermission)
userPermission.getPermission());
+}
+return result;
+ }
+
+ boolean containReadAction(UserPermission userPermission) {
+return userPermission.getPermission().implies(Permission.Action.READ);
+ }
+
+ boolean checkTablePermission(TablePermission tablePermission) {
+return !tablePermission.hasFamily() && !tablePermission.hasQualifier();
+ }
+
+ boolean isTableUserScanSnapshotEnabled(TableDescriptor tableDescriptor) {
+return tableDescriptor.getCoprocessorDescriptors().stream()
Review comment:
Just use the TableDescriptor#hasCoprocessor ?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] openinx commented on a change in pull request #336: HBASE-22580 Add a table attribute to make user scan snapshot feature configurable for table
openinx commented on a change in pull request #336: HBASE-22580 Add a table
attribute to make user scan snapshot feature configurable for table
URL: https://github.com/apache/hbase/pull/336#discussion_r297071634
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptor.java
##
@@ -345,4 +345,11 @@ default boolean matchReplicationScope(boolean enabled) {
}
return !enabled;
}
+
+ /**
+ * Check if user scan snapshot enable flag of the table is true. If flag is
false then
+ * SnapshotScannerHDFSAclController won't handle HDFS acls for users with
table read permission
+ * @return true if user scan snapshot is enabled for this table
+ */
+ boolean isUserScanSnapshotEnabled();
Review comment:
The TableDescriptor is @InterfaceAudience.Public interfaces, right ? and
the snapshot acl is a optional coproccesor ...
I think we can not add an extra specific method in the PUBLIC interface,
once someone use this method, then devs can not remove or change this easily,
we MUST deprecated it in a old major release, then remove it in another new
major release. It is troublesome.
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:
us...@infra.apache.org
With regards,
Apache Git Services
