HIVE-24705: Create/Alter/Drop tables based on storage handlers in HS2… #1960

saihemanth-cloudera · 2021-02-08T21:17:12Z

… should be authorized by Ranger/Sentry

What changes were proposed in this pull request?

Built-in hive storage handlers like HbaseStorageHandler, KafkaStorageHandler e.t.c should implement a method getURIForAuthentication() which returns a URI that is formed from table properties.

Why are the changes needed?

With doAs=false in Hive3.x, whenever a user is trying to create a table based on storage handlers on external storage for ex: HBase table, the end user we are seeing is hive so we cannot really enforce the condition in Apache Ranger/Sentry on the end-user. So, we need to enforce this condition in the hive in the event of create/alter/drop tables based on storage handlers.

Does this PR introduce any user-facing change?

By enforcing authorization, only the users granted with create/alter/drop privileges in ranger/sentry can only do these operations on tables based on storage handlers.

How was this patch tested?

Local machine, Remote cluster.

nrg4878 · 2021-03-25T18:01:35Z

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveStorageAuthorizationHandler.java

+     * should return uri with table properties.
+     */
+    public URI getURIForAuth(Map<String, String> tableProperties) throws URISyntaxException;
+}


Nit: Add newline character on the last line.

nrg4878 · 2021-03-25T18:07:29Z

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveStorageAuthorizationHandler.java

+ */
+@InterfaceAudience.Public
+@InterfaceStability.Stable
+public interface HiveStorageAuthorizationHandler{


nit: the name "HiveStorageAuthorizationHandler" does not seem to fit the purpose of the interface. I am just throwing out names now. WOuld something like these make more sense? HiveURIBasedAuthorization or HiveUriBasedStorageHandler or something akin to this?

Initially, I was using HiveURIBasedAuthorization as class name but Thejas suggested me to use HiveStorageAuthorizationHandler as this class may be evolved into the future to handle other methods of authorization

Thats ok then. Thank you

nrg4878 · 2021-03-25T18:13:44Z

kafka-handler/src/java/org/apache/hadoop/hive/kafka/KafkaStorageHandler.java

@@ -65,13 +68,16 @@
 /**
 * Hive Kafka storage handler to allow user to read and write from/to Kafka message bus.
 */
-@SuppressWarnings("ALL") public class KafkaStorageHandler extends DefaultHiveMetaHook implements HiveStorageHandler {
+@SuppressWarnings("ALL") public class KafkaStorageHandler extends DefaultHiveMetaHook implements HiveStorageHandler, HiveStorageAuthorizationHandler {


I dont think the StorageHandlers are serializable classes. Can you confirm? If they are, then we should stamp these classes with serialVersionUID to avoid execution engines using the wrong jars.

No, All these storage handlers are not serializable.

nrg4878 · 2021-03-25T18:17:55Z

kafka-handler/src/java/org/apache/hadoop/hive/kafka/KafkaStorageHandler.java


  private static final Logger LOG = LoggerFactory.getLogger(KafkaStorageHandler.class);
  private static final String KAFKA_STORAGE_HANDLER = "org.apache.hadoop.hive.kafka.KafkaStorageHandler";

  private Configuration configuration;

+  /** Kafka prefix to form the URI for authentication */
+  private static final String KAFKA_PREFIX = "kafka:";


Nit: Should we call this constant the same across all handlers instead it being specific to each handler? Something like URI_PREFIX instead of KAFKA_PREFIX, HBASE_PREFIX etc.

This prefix is used by Ranger to determine which storage handler class does the URI corresponds to.

nrg4878 · 2021-03-25T18:24:48Z

...rg/apache/hadoop/hive/ql/security/authorization/plugin/metastore/events/AlterTableEvent.java

+      HiveStorageHandler hiveStorageHandler = null;
+      Configuration conf = new Configuration();
+      Map<String, String> tableProperties = new HashMap<>();
+      tableProperties.putAll(newTable.getSd().getSerdeInfo().getParameters());


I am not sure I understand this logic. Why are the SerDe.getParameters() being added to Table.getParameters()? Is this what we always do?

tableProperties is just a variable. I'm not setting this to the table parameters. The reason why I'm combining table parameters and serde parameters are, the user can give table properties like table name, column name or connection information in TBLPropperties or SERDE. So I need to put all the properties in a map and send the map to respective storage handler and extract table properties in it.

nrg4878 · 2021-03-25T18:54:11Z

ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java

+          tableProperties.putAll(table.getParameters());
+          try {
+            Method methodIsImplemented = table.getStorageHandler().getClass().getMethod("getURIForAuth", Map.class);
+            if(methodIsImplemented != null && table.getStorageHandler() instanceof DefaultStorageHandler) {


so DefaultStorageHandler implements the "HiveStorageAuthorizationHandler" interface right? what is the difference between these if else conditions.
Feels like the code should do something like this
if (table.getStorageHandler() instancefo HiveStorageAuthorizationHandler) {
storageuri = ((HiveStorageAuthorizationHandler) table.getStorageHandler()).getURIForAuth(tableProperties)'
}

nrg4878 · 2021-03-25T18:55:23Z

ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java

+            throw nullExp;
+          }catch(Exception ex){
+            //Custom storage handler that has not implemented the getURIForAuth()
+            storageuri = table.getStorageHandler().getClass().getName()+"://"+


if the above change makes sense, then this code should go into the else block instead of the exception handler.

nrg4878 · 2021-03-25T18:56:39Z

...rg/apache/hadoop/hive/ql/security/authorization/plugin/metastore/events/AlterTableEvent.java

+        }
+      }catch(Exception ex){
+        //Custom storage handler that has not implemented the getURIForAuth()
+        storageUri = hiveStorageHandler.getClass().getName()+"://"+


if above comment makes sense, then this should go into the else block of the condition instead of the exception handler.

nrg4878 · 2021-03-25T18:58:04Z

...rg/apache/hadoop/hive/ql/security/authorization/plugin/metastore/events/AlterTableEvent.java

@@ -116,4 +157,13 @@ private String buildCommandString(String cmdStr, Table tbl) {
    }
    return ret;
  }
+
+  private static String getTablePropsForCustomStorageHandler(Map<String, String> tableProperties){


should we consolidate these 2 implementations of this method into some utility class?

nrg4878 · 2021-03-25T18:59:28Z

ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java

+      System.out.println("Class not found. Storage handler will be set to null: "+ex);
+    }
+    t.setStorageHandler(storageHandler);
+    for(Map.Entry<String,String> serdeMap : storageFormat.getSerdeProps().entrySet()){


is there a reason we are setting serde params on the table params? can you give me an example of why we want to do this?

Currently, in the compilation phase we are not sending serde information for ranger auth. This storageFormat's serde has table mappings(eg: hbase table name, column family) of external that needs to be set to table serde.

nrg4878

Changes look good to me. +1

…STORAGEHANDLER_URI. Also added Owner information to these objects.

…n storage handlers

nrg4878 · 2021-08-04T15:14:00Z

fix has been merged to master. Please close this PR.

kgyrtkirk added tests pending tests failed and removed tests pending labels Feb 8, 2021

saihemanth-cloudera force-pushed the hive-24705 branch from 979434d to 95e9846 Compare February 8, 2021 22:53

kgyrtkirk added tests pending tests unstable and removed tests failed tests pending tests unstable labels Feb 9, 2021

kgyrtkirk added tests pending tests failed tests unstable and removed tests unstable tests pending tests failed labels Feb 22, 2021

nrg4878 requested changes Mar 25, 2021

View reviewed changes

kgyrtkirk added tests pending tests unstable and removed tests unstable tests pending labels Mar 29, 2021

kgyrtkirk added tests pending tests passed and removed tests passed tests pending labels May 21, 2021

kgyrtkirk added tests pending tests passed and removed tests passed tests pending labels Jun 22, 2021

nrg4878 approved these changes Aug 3, 2021

View reviewed changes

saihemanth-cloudera added 14 commits August 3, 2021 10:50

Drop condition is not required

becd122

Added HivePrivObjectActionType.OTHER to the HivePrivilegeObject type …

b47e04a

…STORAGEHANDLER_URI. Also added Owner information to these objects.

Added import for HivePrivObjectActionType

1ed672c

Throw exception when kafka table and server properties are not set

9f2dc25

NullPointerException handled for test case failures

1d52ab9

Nullpointer check for table paramters of storage handlers

b8d1332

Addressed review comments

e49c419

compilation issues

60d7afc

Added loggers instead of flushing to STDOUT

5369d4f

Indentation is done done properly.

248d7b8

Added custom storage URI method in default storage handler class

be49d52

Get simple name for storage handler class

22677b3

Custom storage handler class name should be lower case

cd5dacf

Ignore the COLUMN_STATS_ACCURATE property of table for Tables based o…

ac368ad

…n storage handlers

saihemanth-cloudera force-pushed the hive-24705 branch from 89bd108 to ac368ad Compare August 3, 2021 17:53

kgyrtkirk added tests pending tests passed and removed tests passed tests pending labels Aug 3, 2021

saihemanth-cloudera closed this Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-24705: Create/Alter/Drop tables based on storage handlers in HS2… #1960

HIVE-24705: Create/Alter/Drop tables based on storage handlers in HS2… #1960

saihemanth-cloudera commented Feb 8, 2021

nrg4878 Mar 25, 2021

nrg4878 Mar 25, 2021

saihemanth-cloudera Apr 1, 2021

nrg4878 Apr 5, 2021

nrg4878 Mar 25, 2021

saihemanth-cloudera Apr 1, 2021

nrg4878 Mar 25, 2021

saihemanth-cloudera Apr 1, 2021

nrg4878 Mar 25, 2021

saihemanth-cloudera Apr 1, 2021

nrg4878 Mar 25, 2021

nrg4878 Mar 25, 2021

nrg4878 Mar 25, 2021

nrg4878 Mar 25, 2021

nrg4878 Mar 25, 2021

saihemanth-cloudera Apr 1, 2021

nrg4878 left a comment

nrg4878 commented Aug 4, 2021

HIVE-24705: Create/Alter/Drop tables based on storage handlers in HS2… #1960

HIVE-24705: Create/Alter/Drop tables based on storage handlers in HS2… #1960

Conversation

saihemanth-cloudera commented Feb 8, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

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

nrg4878 left a comment

Choose a reason for hiding this comment

nrg4878 commented Aug 4, 2021