[CDAP-8257] Introduce permissions for Tables through dataset properties #7773

anew · 2017-02-01T20:51:28Z

implemented as a series of grants in HBase

HBase 0.96 and 0.98 do not have support in their APIs for that. Therefore it is not implemented in these compat modules. Instead they will log a warning as follows

2017-02-01 12:34:59,317 - WARN  [main:c.c.c.d.u.h.DefaultHBase98DDLExecutor@65] - Granting permissions is not implemented for HBase 0.98. Please grant these permissions manually in the hbase shell: 
grant 'joe', 'RWX', 'perm:priv'
grant '@readers', 'RX', 'perm:priv'

If an invalid permission string is given, the error message is:

java.io.IOException: Error granting permissions 'RWt' for table perm:priv to user joe: Unknown Action 't'

anew · 2017-02-01T20:52:22Z

Note that the first commit is already reviewed as part of #7771 but is not merged to develop yet (pending build success).

anew · 2017-02-01T21:00:36Z

...compat-1.0-cdh/src/main/java/co/cask/cdap/data2/util/hbase/DefaultHBase10CDHDDLExecutor.java

@@ -33,4 +48,28 @@ public HTableDescriptor getHTableDescriptor(TableDescriptor descriptor) {
  public TableDescriptor getTableDescriptor(HTableDescriptor descriptor) {
    return HBase10CDHTableDescriptorUtil.getTableDescriptor(descriptor);
  }
+
+  @Override


Note this uses ConnectionFactory, which is not available in HBase before 1.0. Because the hbase-compat-base depends on HBase 0.98, this cannot be implemented in the base class and must unfortunately be copied to all compat modules. I tried to change the base to depend on HBase 1.0, but that would require updating our Hadoop dependency to 2.5.1+, and it appears to big of a change so short before code freeze.

anew · 2017-02-01T21:07:37Z

Build running here: http://builds.cask.co/browse/CDAP-DUT5372-7

sagarkapare

Minor nits else LGTM 👍

sagarkapare · 2017-02-01T21:05:02Z

....2-cdh5.7.0/src/main/java/co/cask/cdap/data2/util/hbase/DefaultHBase12CDH570DDLExecutor.java

+    TableName table = TableName.valueOf(namespace, name);
+    try (Connection connection = ConnectionFactory.createConnection(admin.getConfiguration())) {
+      if (!AccessControlClient.isAccessControllerRunning(connection)) {
+        LOG.debug("Access control if off. Not granting privileges. ");


nit: Access control is off

sagarkapare · 2017-02-01T21:22:13Z

...compat-1.0-cdh/src/main/java/co/cask/cdap/data2/util/hbase/DefaultHBase10CDHDDLExecutor.java

+    TableName table = TableName.valueOf(namespace, name);
+    try (Connection connection = ConnectionFactory.createConnection(admin.getConfiguration())) {
+      if (!AccessControlClient.isAccessControllerRunning(connection)) {
+        LOG.debug("Access control if off. Not granting privileges. ");


nit: is off

sagarkapare · 2017-02-01T21:23:22Z

cdap-hbase-compat-base/src/main/java/co/cask/cdap/data2/util/hbase/DefaultHBaseDDLExecutor.java

+    return actions;
+  }
+
+  @Nullable


no need to have @Nullable

good catch. I first had it return null so that the caller can throw a detailed exception. Then changed it throw here, and forgot the @nullable.

sagarkapare · 2017-02-01T21:24:07Z

cdap-hbase-compat-base/src/main/java/co/cask/cdap/data2/util/hbase/HBaseTableUtil.java

@@ -498,6 +498,12 @@ public void truncateTable(HBaseDDLExecutor ddlExecutor, TableId tableId) throws
    ddlExecutor.truncateTable(tableName.getNamespaceAsString(), tableName.getQualifierAsString());
  }

+  public void grantPrivileges(HBaseDDLExecutor ddlExecutor, TableId tableId,
+                              Map<String, String> privileges) throws IOException {


we don't need to have this method. Caller can simply call ddlExecutor.grantPersmissions

Then why do we have it truncate()? I guess it still encapsulates the correct logic to map the table name to an HBase table name.

sagarkapare · 2017-02-01T21:26:56Z

cdap-hbase-spi/src/main/java/co/cask/cdap/spi/hbase/HBaseDDLExecutor.java

+   *
+   * @param namespace the namespace of the table
+   * @param name the name of the table
+   * @param permissions A map from user name to the permissions for that user, given as a string containing


nit: A map from user or group name

anew · 2017-02-01T22:05:50Z

@sagarkapare I addressed your comments and also added another commit (see above). Please take another look. Thanks

sagarkapare

minor questions, else LGTM

sagarkapare · 2017-02-01T22:40:57Z

...a-fabric-tests/src/test/java/co/cask/cdap/data2/dataset2/lib/table/hbase/HBaseTableTest.java

+    admin.drop();
+
+    admin = getTableAdmin(CONTEXT1, "invalidPerms", TableProperties.builder()
+      .setTablePermissions(ImmutableMap.of("joe", "iwx")).build()); // invalid perma


nit: // invalid permissions

sagarkapare · 2017-02-01T22:45:17Z

cdap-api/src/main/java/co/cask/cdap/api/dataset/table/TableProperties.java

+  public static Map<String, String> getTablePermissions(Map<String, String> properties) {
+    String propertyValue = properties.get(PROPERTY_TABLE_PERMISSIONS);
+    if (propertyValue == null) {
+      return null;


better return empty map? then the caller would have to only check for isEmpty

The caller may have a default. If it is null, he would use the default. If it is empty, it would override his default.

sagarkapare · 2017-02-01T22:57:36Z

cdap-data-fabric/src/main/java/co/cask/cdap/data2/dataset2/lib/table/hbase/HBaseTableAdmin.java

+        if (permissions != null && !permissions.isEmpty()) {
+          tableUtil.grantPrivileges(ddlExecutor, tableId, permissions);
+        }
+      } catch (IOException | RuntimeException e) {


any reason not to catch Throwable here?

Because I can't rethrow that.

anew · 2017-02-02T01:16:56Z

squashing and rebasing to develop

anew · 2017-02-02T03:02:49Z

Build for #7775 passed here: http://builds.cask.co/browse/CDAP-DUT5388-1. It is based on this one and therefore validates this one, too.

anew commented Feb 1, 2017

View reviewed changes

prinam added the 4.1 label Feb 1, 2017

sagarkapare approved these changes Feb 1, 2017

View reviewed changes

anew mentioned this pull request Feb 1, 2017

[CDAP-8308] Upon Namespace creation, assign C(reate) permissions for the group to the HBase namespace #7775

Merged

anew force-pushed the feature/cdap-8257-table-permissions branch from f29c829 to fe29df7 Compare February 2, 2017 01:16

[CDAP-8257] Introduce permissions for Tables through dataset properties

65c1014

anew force-pushed the feature/cdap-8257-table-permissions branch from fe29df7 to 65c1014 Compare February 2, 2017 01:18

anew merged commit 65c1014 into develop Feb 2, 2017

chtyim deleted the feature/cdap-8257-table-permissions branch April 20, 2017 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDAP-8257] Introduce permissions for Tables through dataset properties #7773

[CDAP-8257] Introduce permissions for Tables through dataset properties #7773

anew commented Feb 1, 2017

anew commented Feb 1, 2017

anew Feb 1, 2017

anew commented Feb 1, 2017

sagarkapare left a comment

sagarkapare Feb 1, 2017

anew Feb 1, 2017

sagarkapare Feb 1, 2017

sagarkapare Feb 1, 2017

anew Feb 1, 2017

sagarkapare Feb 1, 2017

anew Feb 1, 2017

sagarkapare Feb 1, 2017

anew commented Feb 1, 2017

sagarkapare left a comment

sagarkapare Feb 1, 2017

sagarkapare Feb 1, 2017

anew Feb 1, 2017

sagarkapare Feb 1, 2017

anew Feb 1, 2017

anew commented Feb 2, 2017

anew commented Feb 2, 2017

[CDAP-8257] Introduce permissions for Tables through dataset properties #7773

[CDAP-8257] Introduce permissions for Tables through dataset properties #7773

Conversation

anew commented Feb 1, 2017

anew commented Feb 1, 2017

Choose a reason for hiding this comment

anew commented Feb 1, 2017

sagarkapare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anew commented Feb 1, 2017

sagarkapare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anew commented Feb 2, 2017

anew commented Feb 2, 2017