IMPALA-6451: AuthorizationException in CTAS for Kudu tables

CREATE TABLE on EXTERNAL Kudu tables or with TBLPROPERTIES
('kudu.master_addresses') require ALL privileges on SERVER.
See IMPALA-4000.

The patch fixes a bug where the CTAS statement is rewritten
and during the initial analysis phase, 'kudu.master_addresses'
property is added into TBLPROPERTIES which causes the next
analysis phase to assume the statement was executed with
TBLPROPERTIES ('kudu.master_addresses'). Hence, causing the
ALL privilege request on SERVER to be registered.

This patch also refactors the generated Kudu table name into
a single place that stores all generated Kudu properties that
gets cleared when the statement is reset.

Testing:
- Added a new test
- Ran all front-end tests
- Ran Kudu end-to-end query tests

Change-Id: Iac3bbe4dceb80dfefd9756bc322f8c10ceab9f49
Reviewed-on: http://gerrit.cloudera.org:8080/10030
Reviewed-by: Alex Behm <alex.b...@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>


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

Branch: refs/heads/master
Commit: 6f8f757446597ffde4d4d77ce51543f82fed0c0b
Parents: 2dfa261
Author: Fredy Wijaya <fwij...@cloudera.com>
Authored: Wed Apr 11 21:00:50 2018 -0500
Committer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Committed: Fri Apr 13 20:55:01 2018 +0000

----------------------------------------------------------------------
 .../apache/impala/analysis/CreateTableStmt.java | 21 ++++++++++----------
 .../org/apache/impala/analysis/TableDef.java    | 15 +++++++-------
 .../org/apache/impala/analysis/ToSqlUtils.java  | 19 ++++++++++++++----
 .../impala/analysis/AuthorizationTest.java      | 11 ++++++++++
 4 files changed, 44 insertions(+), 22 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/6f8f7574/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 ee020df..ec689b6 100644
--- a/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
@@ -112,11 +112,11 @@ public class CreateTableStmt extends StatementBase {
   Map<String, String> getSerdeProperties() { return 
tableDef_.getSerdeProperties(); }
   public THdfsFileFormat getFileFormat() { return tableDef_.getFileFormat(); }
   RowFormat getRowFormat() { return tableDef_.getRowFormat(); }
-  private String getGeneratedKuduTableName() {
-    return tableDef_.getGeneratedKuduTableName();
+  private void putGeneratedKuduProperty(String key, String value) {
+    tableDef_.putGeneratedKuduProperty(key, value);
   }
-  private void setGeneratedKuduTableName(String tableName) {
-    tableDef_.setGeneratedKuduTableName(tableName);
+  public Map<String, String> getGeneratedKuduProperties() {
+    return tableDef_.getGeneratedKuduProperties();
   }
 
   // Only exposed for ToSqlUtils. Returns the list of primary keys declared by 
the user
@@ -166,10 +166,7 @@ public class CreateTableStmt extends StatementBase {
     params.setIf_not_exists(getIfNotExists());
     params.setSort_columns(getSortColumns());
     params.setTable_properties(Maps.newHashMap(getTblProperties()));
-    if (!getGeneratedKuduTableName().isEmpty()) {
-      params.getTable_properties().put(KuduTable.KEY_TABLE_NAME,
-          getGeneratedKuduTableName());
-    }
+    
params.getTable_properties().putAll(Maps.newHashMap(getGeneratedKuduProperties()));
     params.setSerde_properties(getSerdeProperties());
     for (KuduPartitionParam d: getKuduPartitionParams()) {
       params.addToPartition_by(d.toThrift());
@@ -261,7 +258,8 @@ public class CreateTableStmt extends StatementBase {
       throw new AnalysisException("Invalid storage handler specified for Kudu 
table: " +
           handler);
     }
-    getTblProperties().put(KuduTable.KEY_STORAGE_HANDLER, 
KuduTable.KUDU_STORAGE_HANDLER);
+    putGeneratedKuduProperty(KuduTable.KEY_STORAGE_HANDLER,
+        KuduTable.KUDU_STORAGE_HANDLER);
 
     String masterHosts = getTblProperties().get(KuduTable.KEY_MASTER_HOSTS);
     if (Strings.isNullOrEmpty(masterHosts)) {
@@ -271,7 +269,7 @@ public class CreateTableStmt extends StatementBase {
             "Table property '%s' is required when the impalad startup flag " +
             "-kudu_master_hosts is not used.", KuduTable.KEY_MASTER_HOSTS));
       }
-      getTblProperties().put(KuduTable.KEY_MASTER_HOSTS, masterHosts);
+      putGeneratedKuduProperty(KuduTable.KEY_MASTER_HOSTS, masterHosts);
     }
 
     // TODO: Find out what is creating a directory in HDFS and stop doing 
that. Kudu
@@ -357,7 +355,8 @@ public class CreateTableStmt extends StatementBase {
     
AnalysisUtils.throwIfNotNull(getTblProperties().get(KuduTable.KEY_TABLE_NAME),
         String.format("Not allowed to set '%s' manually for managed Kudu 
tables .",
             KuduTable.KEY_TABLE_NAME));
-    setGeneratedKuduTableName(KuduUtil.getDefaultCreateKuduTableName(getDb(), 
getTbl()));
+    putGeneratedKuduProperty(KuduTable.KEY_TABLE_NAME,
+        KuduUtil.getDefaultCreateKuduTableName(getDb(), getTbl()));
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/impala/blob/6f8f7574/fe/src/main/java/org/apache/impala/analysis/TableDef.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/TableDef.java 
b/fe/src/main/java/org/apache/impala/analysis/TableDef.java
index 915bbba..948515b 100644
--- a/fe/src/main/java/org/apache/impala/analysis/TableDef.java
+++ b/fe/src/main/java/org/apache/impala/analysis/TableDef.java
@@ -17,6 +17,7 @@
 
 package org.apache.impala.analysis;
 
+import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -88,8 +89,8 @@ class TableDef {
   // True if analyze() has been called.
   private boolean isAnalyzed_ = false;
 
-  //Kudu table name generated during analysis for managed Kudu tables
-  private String generatedKuduTableName_ = "";
+  // Generated Kudu properties set during analysis.
+  private Map<String, String> generatedKuduProperties_ = new HashMap<>();
 
   // END: Members that need to be reset()
   /////////////////////////////////////////
@@ -163,7 +164,7 @@ class TableDef {
     dataLayout_.reset();
     columnDefs_.clear();
     isAnalyzed_ = false;
-    generatedKuduTableName_ = "";
+    generatedKuduProperties_.clear();
   }
 
   public TableName getTblName() {
@@ -187,10 +188,10 @@ class TableDef {
   List<ColumnDef> getPrimaryKeyColumnDefs() { return primaryKeyColDefs_; }
   boolean isExternal() { return isExternal_; }
   boolean getIfNotExists() { return ifNotExists_; }
-  String getGeneratedKuduTableName() { return generatedKuduTableName_; }
-  void setGeneratedKuduTableName(String tableName) {
-    Preconditions.checkNotNull(tableName);
-    generatedKuduTableName_ = tableName;
+  Map<String, String> getGeneratedKuduProperties() { return 
generatedKuduProperties_; }
+  void putGeneratedKuduProperty(String key, String value) {
+    Preconditions.checkNotNull(key);
+    generatedKuduProperties_.put(key, value);
   }
   List<KuduPartitionParam> getKuduPartitionParams() {
     return dataLayout_.getKuduPartitionParams();

http://git-wip-us.apache.org/repos/asf/impala/blob/6f8f7574/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java 
b/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
index 9a164f0..a3c084d 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
@@ -67,8 +67,10 @@ public class ToSqlUtils {
   /**
    * Removes all hidden properties from the given 'tblProperties' map.
    */
-  private static void removeHiddenTableProperties(Map<String, String> 
tblProperties) {
+  private static void removeHiddenTableProperties(Map<String, String> 
tblProperties,
+      Map<String, String> generatedTblProperties) {
     for (String key: HIDDEN_TABLE_PROPERTIES) tblProperties.remove(key);
+    generatedTblProperties.remove(KuduTable.KEY_TABLE_NAME);
   }
 
   /**
@@ -165,11 +167,17 @@ public class ToSqlUtils {
     for (ColumnDef col: stmt.getPartitionColumnDefs()) {
       partitionColsSql.add(col.toString());
     }
+    LinkedHashMap<String, String> properties = Maps.newLinkedHashMap(
+        stmt.getTblProperties());
+    LinkedHashMap<String, String> generatedProperties = Maps.newLinkedHashMap(
+        stmt.getGeneratedKuduProperties());
+    removeHiddenTableProperties(properties, generatedProperties);
+    properties.putAll(generatedProperties);
     String kuduParamsSql = getKuduPartitionByParams(stmt);
     // TODO: Pass the correct compression, if applicable.
     return getCreateTableSql(stmt.getDb(), stmt.getTbl(), stmt.getComment(), 
colsSql,
         partitionColsSql, stmt.getTblPrimaryKeyColumnNames(), kuduParamsSql,
-        stmt.getSortColumns(), stmt.getTblProperties(), 
stmt.getSerdeProperties(),
+        stmt.getSortColumns(), properties, stmt.getSerdeProperties(),
         stmt.isExternal(), stmt.getIfNotExists(), stmt.getRowFormat(),
         HdfsFileFormat.fromThrift(stmt.getFileFormat()), HdfsCompression.NONE, 
null,
         stmt.getLocation());
@@ -190,7 +198,10 @@ public class ToSqlUtils {
     // Use a LinkedHashMap to preserve the ordering of the table properties.
     LinkedHashMap<String, String> properties =
         Maps.newLinkedHashMap(innerStmt.getTblProperties());
-    removeHiddenTableProperties(properties);
+    LinkedHashMap<String, String> generatedProperties = Maps.newLinkedHashMap(
+        stmt.getCreateStmt().getGeneratedKuduProperties());
+    removeHiddenTableProperties(properties, generatedProperties);
+    properties.putAll(generatedProperties);
     String kuduParamsSql = getKuduPartitionByParams(innerStmt);
     // TODO: Pass the correct compression, if applicable.
     String createTableSql = getCreateTableSql(innerStmt.getDb(), 
innerStmt.getTbl(),
@@ -220,7 +231,7 @@ public class ToSqlUtils {
         msTable.getTableType().equals(TableType.EXTERNAL_TABLE.toString());
     List<String> sortColsSql = getSortColumns(properties);
     String comment = properties.get("comment");
-    removeHiddenTableProperties(properties);
+    removeHiddenTableProperties(properties, Maps.<String, String>newHashMap());
     ArrayList<String> colsSql = Lists.newArrayList();
     ArrayList<String> partitionColsSql = Lists.newArrayList();
     boolean isHbaseTable = table instanceof HBaseTable;

http://git-wip-us.apache.org/repos/asf/impala/blob/6f8f7574/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 9a62e93..f0b7332 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
@@ -1126,6 +1126,17 @@ public class AuthorizationTest extends FrontendTestBase {
     AuthzOk("create table tpch.kudu_tbl (i int, j int, primary key (i))" +
         " PARTITION BY HASH (i) PARTITIONS 9 stored as kudu");
 
+    // IMPALA-6451: CTAS for Kudu tables on non-external tables and without
+    // TBLPROPERTIES ('kudu.master_addresses') should not require ALL 
privileges
+    // on SERVER.
+    // User has ALL privilege on tpch database and SELECT privilege on
+    // functional.alltypesagg table.
+    // The statement below causes the SQL statement to be rewritten.
+    AuthzOk("create table tpch.kudu_tbl primary key (bigint_col) stored as 
kudu as " +
+        "select bigint_col, string_col, current_timestamp() as ins_date " +
+        "from functional.alltypesagg " +
+        "where exists (select 1 from functional.alltypesagg)");
+
     // User does not have permission to create table at the specified 
location..
     AuthzError("create table tpch.new_table (i int) location " +
         "'hdfs://localhost:20500/test-warehouse/alltypes'",

Reply via email to