impala-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mjac...@apache.org
Subject [1/2] incubator-impala git commit: IMPALA-5638: Fix Kudu table set tblproperties inconsistencies
Date Thu, 20 Jul 2017 13:34:04 GMT
Repository: incubator-impala
Updated Branches:
  refs/heads/master 5c8ed4fd4 -> 34d9a79c5


IMPALA-5638: Fix Kudu table set tblproperties inconsistencies

Kudu tables did not treat some table properties correctly.

Change-Id: I69fa661419897f2aab4632015a147b784a6e7009
Reviewed-on: http://gerrit.cloudera.org:8080/7454
Reviewed-by: Matthew Jacobs <mj@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/d0bf4132
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/d0bf4132
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/d0bf4132

Branch: refs/heads/master
Commit: d0bf4132084d466be820dcd867fc36110af93d82
Parents: 5c8ed4f
Author: Matthew Jacobs <mj@cloudera.com>
Authored: Tue Jun 27 18:06:38 2017 -0700
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
Committed: Thu Jul 20 09:39:34 2017 +0000

----------------------------------------------------------------------
 .../analysis/AlterTableSetTblProperties.java    |  13 +++
 .../apache/impala/analysis/CreateTableStmt.java |  27 +++--
 .../org/apache/impala/util/MetaStoreUtil.java   |  19 +++-
 .../impala/analysis/AuthorizationTest.java      |   7 +-
 .../queries/QueryTest/grant_revoke_kudu.test    | 109 +++++++++++++++++++
 tests/authorization/test_grant_revoke.py        |   8 ++
 6 files changed, 170 insertions(+), 13 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d0bf4132/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
index 2288663..fb83f24 100644
--- a/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
+++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
@@ -24,6 +24,7 @@ import java.util.Map;
 import org.apache.avro.SchemaParseException;
 import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
 import org.apache.hadoop.hive.serde2.avro.AvroSerdeUtils;
+import org.apache.impala.authorization.PrivilegeRequestBuilder;
 import org.apache.impala.catalog.Column;
 import org.apache.impala.catalog.HBaseTable;
 import org.apache.impala.catalog.HdfsTable;
@@ -90,6 +91,18 @@ public class AlterTableSetTblProperties extends AlterTableSetStmt {
           hive_metastoreConstants.META_TABLE_STORAGE));
     }
 
+    if (getTargetTable() instanceof KuduTable && analyzer.getAuthzConfig().isEnabled())
{
+      // Checking for 'EXTERNAL' is case-insensitive, see IMPALA-5637.
+      boolean setsExternal =
+          MetaStoreUtil.findTblPropKeyCaseInsensitive(tblProperties_, "EXTERNAL") != null;
+      if (setsExternal || tblProperties_.containsKey(KuduTable.KEY_MASTER_HOSTS)) {
+        String authzServer = analyzer.getAuthzConfig().getServerName();
+        Preconditions.checkNotNull(authzServer);
+        analyzer.registerPrivReq(new PrivilegeRequestBuilder().onServer(
+            authzServer).all().toRequest());
+      }
+    }
+
     // Check avro schema when it is set in avro.schema.url or avro.schema.literal to
     // avoid potential metadata corruption (see IMPALA-2042).
     // If both properties are set then only check avro.schema.literal and ignore

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d0bf4132/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java b/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
index 6010eb6..17ac46d 100644
--- a/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
@@ -34,6 +34,7 @@ import org.apache.impala.util.AvroSchemaConverter;
 import org.apache.impala.util.AvroSchemaParser;
 import org.apache.impala.util.AvroSchemaUtils;
 import org.apache.impala.util.KuduUtil;
+import org.apache.impala.util.MetaStoreUtil;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
@@ -210,6 +211,22 @@ public class CreateTableStmt extends StatementBase {
    * Kudu tables.
    */
   private void analyzeKuduTableProperties(Analyzer analyzer) throws AnalysisException {
+    if (analyzer.getAuthzConfig().isEnabled()) {
+      // Today there is no comprehensive way of enforcing a Sentry authorization policy
+      // against tables stored in Kudu. This is why only users with ALL privileges on
+      // SERVER may create external Kudu tables or set the master addresses.
+      // See IMPALA-4000 for details.
+      boolean isExternal = tableDef_.isExternal() ||
+          MetaStoreUtil.findTblPropKeyCaseInsensitive(
+              getTblProperties(), "EXTERNAL") != null;
+      if (getTblProperties().containsKey(KuduTable.KEY_MASTER_HOSTS) || isExternal) {
+        String authzServer = analyzer.getAuthzConfig().getServerName();
+        Preconditions.checkNotNull(authzServer);
+        analyzer.registerPrivReq(new PrivilegeRequestBuilder().onServer(
+            authzServer).all().toRequest());
+      }
+    }
+
     // Only the Kudu storage handler may be specified for Kudu tables.
     String handler = getTblProperties().get(KuduTable.KEY_STORAGE_HANDLER);
     if (handler != null && !handler.equals(KuduTable.KUDU_STORAGE_HANDLER)) {
@@ -244,15 +261,6 @@ public class CreateTableStmt extends StatementBase {
    */
   private void analyzeExternalKuduTableParams(Analyzer analyzer)
       throws AnalysisException {
-    if (analyzer.getAuthzConfig().isEnabled()) {
-      // Today there is no comprehensive way of enforcing a Sentry authorization policy
-      // against tables stored in Kudu. This is why only users with ALL privileges on
-      // SERVER may create external Kudu tables. See IMPALA-4000 for details.
-      String authzServer = analyzer.getAuthzConfig().getServerName();
-      Preconditions.checkNotNull(authzServer);
-      analyzer.registerPrivReq(new PrivilegeRequestBuilder().onServer(
-          authzServer).all().toRequest());
-    }
     AnalysisUtils.throwIfNull(getTblProperties().get(KuduTable.KEY_TABLE_NAME),
         String.format("Table property %s must be specified when creating " +
             "an external Kudu table.", KuduTable.KEY_TABLE_NAME));
@@ -276,6 +284,7 @@ public class CreateTableStmt extends StatementBase {
   private void analyzeManagedKuduTableParams(Analyzer analyzer) throws AnalysisException
{
     // If no Kudu table name is specified in tblproperties, generate one using the
     // current database as a prefix to avoid conflicts in Kudu.
+    // TODO: Disallow setting this manually for managed tables (IMPALA-5654).
     if (!getTblProperties().containsKey(KuduTable.KEY_TABLE_NAME)) {
       getTblProperties().put(KuduTable.KEY_TABLE_NAME,
           KuduUtil.getDefaultCreateKuduTableName(getDb(), getTbl()));

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d0bf4132/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java b/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
index 7671a45..4cc9057 100644
--- a/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
+++ b/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
@@ -163,8 +163,8 @@ public class MetaStoreUtil {
   }
 
   /**
-   * Checks that each key and value in a proprty map is short enough for HMS to handle. If
-   * not, An 'AnalysisException' is thrown with 'mapName' as its prefix.
+   * Checks that each key and value in a property map is short enough for HMS to handle.
+   * If not, An 'AnalysisException' is thrown with 'mapName' as its prefix.
    */
   public static void checkShortPropertyMap(
       String mapName, Map<String, String> propertyMap) throws AnalysisException {
@@ -178,6 +178,21 @@ public class MetaStoreUtil {
   }
 
   /**
+   * Does a case-insensitive search for 'propertyKey' in 'propertyMap'. If a match is
+   * found, the matched key is returned, otherwise null is returned. 'propertyMap' and
+   * 'propertyKey' must not be null.
+   */
+  public static String findTblPropKeyCaseInsensitive(Map<String, String> propertyMap,
+      String propertyKey) {
+    Preconditions.checkNotNull(propertyMap);
+    Preconditions.checkNotNull(propertyKey);
+    for (String key : propertyMap.keySet()) {
+      if (key != null && key.equalsIgnoreCase(propertyKey)) return key;
+    }
+    return null;
+  }
+
+  /**
    * Returns a copy of the comma-separated list of values 'inputCsv', with all occurences
    * of 'toReplace' replaced with value 'replaceWith'.
    */

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d0bf4132/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java b/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
index 40d9753..30a0238 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
@@ -904,11 +904,14 @@ public class AuthorizationTest {
     AuthzError("create external table tpch.kudu_tbl stored as kudu " +
         "TBLPROPERTIES ('kudu.master_addresses'='127.0.0.1', 'kudu.table_name'='tbl')",
         "User '%s' does not have privileges to access: server1");
+    AuthzError("create table tpch.kudu_tbl (i int, j int, primary key (i))" +
+        " PARTITION BY HASH (i) PARTITIONS 9 stored as kudu TBLPROPERTIES " +
+        "('kudu.master_addresses'='127.0.0.1')",
+        "User '%s' does not have privileges to access: server1");
 
     // IMPALA-4000: ALL privileges on SERVER are not required to create managed tables.
     AuthzOk("create table tpch.kudu_tbl (i int, j int, primary key (i))" +
-        " PARTITION BY HASH (i) PARTITIONS 9 stored as kudu TBLPROPERTIES " +
-        "('kudu.master_addresses'='127.0.0.1')");
+        " PARTITION BY HASH (i) PARTITIONS 9 stored as kudu");
 
     // User does not have permission to create table at the specified location..
     AuthzError("create table tpch.new_table (i int) location " +

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d0bf4132/testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
b/testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
new file mode 100644
index 0000000..84e8d2a
--- /dev/null
+++ b/testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
@@ -0,0 +1,109 @@
+====
+---- QUERY
+create role grant_revoke_test_ALL_SERVER
+---- RESULTS
+====
+---- QUERY
+create role grant_revoke_test_ALL_TEST_DB
+---- RESULTS
+====
+---- QUERY
+show roles
+---- RESULTS: VERIFY_IS_SUBSET
+'grant_revoke_test_ALL_SERVER'
+'grant_revoke_test_ALL_TEST_DB'
+---- TYPES
+STRING
+====
+---- QUERY
+grant role grant_revoke_test_ALL_SERVER to group $GROUP_NAME
+====
+---- QUERY
+grant all on server to grant_revoke_test_ALL_SERVER
+====
+---- QUERY
+create database grant_rev_db
+====
+---- QUERY
+grant role grant_revoke_test_ALL_TEST_DB to group $GROUP_NAME
+====
+---- QUERY
+# Should now have all privileges on the test db
+grant all on database grant_rev_db to grant_revoke_test_ALL_TEST_DB
+====
+---- QUERY
+revoke role grant_revoke_test_ALL_SERVER from group $GROUP_NAME
+====
+---- QUERY
+show current roles
+---- RESULTS
+'grant_revoke_test_ALL_TEST_DB'
+---- TYPES
+STRING
+====
+---- QUERY
+# Even though the user has all privileges on the database, they do not have privileges
+# to set a Kudu table to be EXTERNAL as that requires ALL on the server. Create a
+# managed table with the EXTERNAL property explicitly set.
+create table grant_rev_db.kudu_tbl_with_ext (i int primary key, a string)
+partition by hash(i) partitions 3 stored as kudu
+tblproperties('EXTERNAL'='TRUE')
+---- CATCH
+does not have privileges to access:
+====
+---- QUERY
+# Check 'external' case-insensitive (see IMPALA-5637).
+create table grant_rev_db.kudu_tbl_with_ext (i int primary key, a string)
+partition by hash(i) partitions 3 stored as kudu
+tblproperties('external'='true')
+---- CATCH
+does not have privileges to access:
+====
+---- QUERY
+# Similarly, a managed table with explicit master addresses requires ALL on server.
+create table grant_rev_db.kudu_tbl_with_addr (i int primary key, a string)
+partition by hash(i) partitions 3 stored as kudu
+tblproperties('kudu.master_addresses'='foo')
+---- CATCH
+does not have privileges to access:
+====
+---- QUERY
+create table grant_rev_db.kudu_tbl (i int primary key, a string)
+partition by hash(i) partitions 3 stored as kudu;
+====
+---- QUERY
+# Similarly, the table properties cannot be set via alter table set tblproperties.
+alter table grant_rev_db.kudu_tbl set tblproperties('kudu.master_addresses'='foo');
+---- CATCH
+does not have privileges to access:
+====
+---- QUERY
+alter table grant_rev_db.kudu_tbl set tblproperties('EXTERNAL'='TRUE');
+---- CATCH
+does not have privileges to access:
+====
+---- QUERY
+alter table grant_rev_db.kudu_tbl set tblproperties('external'='true');
+---- CATCH
+does not have privileges to access:
+====
+---- QUERY
+grant role grant_revoke_test_ALL_SERVER to group $GROUP_NAME
+====
+---- QUERY
+# Now the alter table succeeds
+alter table grant_rev_db.kudu_tbl set tblproperties('EXTERNAL'='TRUE');
+====
+---- QUERY
+# Set it back to FALSE
+alter table grant_rev_db.kudu_tbl set tblproperties('EXTERNAL'='FALSE');
+====
+---- QUERY
+drop table grant_rev_db.kudu_tbl
+====
+---- QUERY
+# Cleanup test roles
+drop role grant_revoke_test_ALL_SERVER;
+drop role grant_revoke_test_ALL_TEST_DB;
+---- RESULTS
+====

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d0bf4132/tests/authorization/test_grant_revoke.py
----------------------------------------------------------------------
diff --git a/tests/authorization/test_grant_revoke.py b/tests/authorization/test_grant_revoke.py
index f6b8420..987aecb 100644
--- a/tests/authorization/test_grant_revoke.py
+++ b/tests/authorization/test_grant_revoke.py
@@ -86,6 +86,14 @@ class TestGrantRevoke(CustomClusterTestSuite, ImpalaTestSuite):
   def test_grant_revoke(self, vector):
     self.run_test_case('QueryTest/grant_revoke', vector, use_db="default")
 
+  @pytest.mark.execute_serially
+  @CustomClusterTestSuite.with_args(
+      impalad_args="--server_name=server1",
+      catalogd_args="--sentry_config=" + SENTRY_CONFIG_FILE)
+  def test_grant_revoke_kudu(self, vector):
+    if getenv("KUDU_IS_SUPPORTED") == "false":
+      pytest.skip("Kudu is not supported")
+    self.run_test_case('QueryTest/grant_revoke_kudu', vector, use_db="default")
 
   @pytest.mark.execute_serially
   @CustomClusterTestSuite.with_args(


Mime
View raw message