-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HIVE-28356: HMS’s Authorizer for the CREATE_TABLE event doesn’t handle HivePrivilegeObjectType.STORAGEHANDLER_URI #5343
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,10 +19,7 @@ | |
|
||
package org.apache.hadoop.hive.ql.security.authorization.plugin.metastore; | ||
|
||
import org.apache.hadoop.hive.metastore.api.DataConnector; | ||
import org.apache.hadoop.hive.metastore.api.Database; | ||
import org.apache.hadoop.hive.metastore.api.StorageDescriptor; | ||
import org.apache.hadoop.hive.metastore.api.Table; | ||
import org.apache.hadoop.hive.metastore.api.*; | ||
import org.apache.hadoop.hive.metastore.events.PreEventContext; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HivePrivilegeObject; | ||
|
||
|
@@ -64,6 +61,12 @@ protected HivePrivilegeObject getHivePrivilegeObject(Table table) { | |
table.getOwner(), table.getOwnerType()); | ||
} | ||
|
||
protected HivePrivilegeObject getHivePrivilegeObjectStorageHandlerUri(String storagehandler_uri, Table table) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method is not used anymore now. |
||
return new HivePrivilegeObject(HivePrivilegeObject.HivePrivilegeObjectType.STORAGEHANDLER_URI, null, storagehandler_uri, | ||
null, null, HivePrivilegeObject.HivePrivObjectActionType.OTHER, null, null, | ||
table.getOwner(), table.getOwnerType()); | ||
} | ||
|
||
protected HivePrivilegeObject getHivePrivilegeObjectDfsUri(String uri) { | ||
return new HivePrivilegeObject(HivePrivilegeObject.HivePrivilegeObjectType.DFS_URI, null, uri); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,18 +23,22 @@ | |
import org.apache.hadoop.hive.metastore.api.Table; | ||
import org.apache.hadoop.hive.metastore.api.Database; | ||
import org.apache.hadoop.hive.metastore.TableType; | ||
import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants; | ||
import org.apache.hadoop.hive.metastore.events.PreCreateTableEvent; | ||
import org.apache.hadoop.hive.metastore.events.PreEventContext; | ||
import org.apache.hadoop.hive.ql.metadata.HiveStorageHandler; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveOperationType; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HivePrivilegeObject; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HivePrivilegeObject.HivePrivilegeObjectType; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.metastore.HiveMetaStoreAuthorizableEvent; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.metastore.HiveMetaStoreAuthzInfo; | ||
import org.apache.hadoop.conf.Configuration; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HivePrivilegeObject.HivePrivObjectActionType; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.apache.hadoop.util.ReflectionUtils; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: All the newly added imports are in an incorrect order. They should follow alphabetical order. |
||
|
||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
/* | ||
|
@@ -79,6 +83,24 @@ private List<HivePrivilegeObject> getOutputHObjs() { | |
Database database = event.getDatabase(); | ||
String uri = getSdLocation(table.getSd()); | ||
|
||
// if the table is an iceberg storagehandler based table , need storageuri as hive privilege objects | ||
// format: storagehandler type + cluster + location | ||
LOG.debug("<== CreateTableEvent.getOutputHObjs(): ret={}", ret); | ||
if (table.getParameters().containsKey(hive_metastoreConstants.META_TABLE_STORAGE)) { | ||
Configuration conf = new Configuration(); | ||
try { | ||
HiveStorageHandler hiveStorageHandler = (HiveStorageHandler) ReflectionUtils.newInstance( | ||
conf.getClassByName(table.getParameters().get(hive_metastoreConstants.META_TABLE_STORAGE)), event.getHandler().getConf()); | ||
String storageUri = hiveStorageHandler.getURIForAuth(table).toString(); | ||
saihemanth-cloudera marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (storageUri.contains("iceberg")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need to check for iceberg? |
||
ret.add(new HivePrivilegeObject(HivePrivilegeObjectType.STORAGEHANDLER_URI, null, storageUri, null, null, | ||
HivePrivObjectActionType.OTHER, null, table.getParameters().get(hive_metastoreConstants.META_TABLE_STORAGE), table.getOwner(), table.getOwnerType())); | ||
} | ||
} catch (Exception ex) { | ||
LOG.error("Exception occurred while getting the URI from storage handler: " + ex.getMessage(), ex); | ||
} | ||
} | ||
|
||
ret.add(getHivePrivilegeObject(database)); | ||
ret.add(getHivePrivilegeObject(table)); | ||
|
||
|
@@ -88,7 +110,9 @@ private List<HivePrivilegeObject> getOutputHObjs() { | |
|
||
COMMAND_STR = buildCommandString(COMMAND_STR,table); | ||
|
||
LOG.debug("<== CreateTableEvent.getOutputHObjs(): ret={}", ret); | ||
if (LOG.isDebugEnabled()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor, but why do we need this check? |
||
LOG.debug("<== CreateTableEvent.getOutputHObjs(): ret=" + ret); | ||
} | ||
|
||
return ret; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,9 +19,12 @@ | |
package org.apache.hadoop.hive.ql.security.authorization.plugin.metastore; | ||
|
||
import org.apache.commons.lang3.StringUtils; | ||
import org.apache.commons.lang3.tuple.ImmutablePair; | ||
import org.apache.commons.lang3.tuple.Pair; | ||
import org.apache.commons.lang3.exception.ExceptionUtils; | ||
import org.apache.hadoop.conf.Configuration; | ||
import org.apache.hadoop.hive.common.FileUtils; | ||
import org.apache.hadoop.hive.conf.HiveConf; | ||
import org.apache.hadoop.hive.metastore.ColumnType; | ||
import org.apache.hadoop.hive.metastore.*; | ||
import org.apache.hadoop.hive.metastore.MetaStoreTestUtils; | ||
|
@@ -31,21 +34,36 @@ | |
import org.apache.hadoop.hive.metastore.client.builder.*; | ||
import org.apache.hadoop.hive.metastore.events.*; | ||
import org.apache.hadoop.hive.ql.security.HadoopDefaultMetastoreAuthenticator; | ||
import org.apache.hadoop.hive.ql.security.HiveAuthenticationProvider; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAccessControlException; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthorizer; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthorizerFactory; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthzContext; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthzPluginException; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthzSessionContext; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveMetastoreClientFactory; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HivePrivilegeObject; | ||
import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveOperationType; | ||
import org.apache.hadoop.security.UserGroupInformation; | ||
import org.junit.FixMethodOrder; | ||
import org.junit.runners.MethodSorters; | ||
import org.junit.Before; | ||
import org.junit.Test; | ||
import org.mockito.ArgumentCaptor; | ||
import org.mockito.Mockito; | ||
|
||
import java.util.HashMap; | ||
import java.util.Map; | ||
import java.io.File; | ||
import java.util.Arrays; | ||
import java.util.Map; | ||
import java.util.List; | ||
|
||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertTrue; | ||
import static org.junit.Assert.fail; | ||
import static org.mockito.ArgumentMatchers.any; | ||
import static org.mockito.Mockito.reset; | ||
import static org.mockito.Mockito.verify; | ||
|
||
/* | ||
Test whether HiveAuthorizer for MetaStore operation is trigger and HiveMetaStoreAuthzInfo is created by HiveMetaStoreAuthorizer | ||
|
@@ -69,6 +87,8 @@ public class TestHiveMetaStoreAuthorizer { | |
private Configuration conf; | ||
private HMSHandler hmsHandler; | ||
|
||
static HiveAuthorizer dummyHiveAuthorizer; | ||
|
||
@Before | ||
public void setUp() throws Exception { | ||
conf = MetastoreConf.newMetastoreConf(); | ||
|
@@ -78,7 +98,7 @@ public void setUp() throws Exception { | |
MetastoreConf.setVar(conf, ConfVars.PARTITION_NAME_WHITELIST_PATTERN, metaConfVal); | ||
MetastoreConf.setLongVar(conf, ConfVars.THRIFT_CONNECTION_RETRIES, 3); | ||
MetastoreConf.setBoolVar(conf, ConfVars.HIVE_SUPPORT_CONCURRENCY, false); | ||
MetastoreConf.setVar(conf, ConfVars.HIVE_AUTHORIZATION_MANAGER, DummyHiveAuthorizerFactory.class.getName()); | ||
MetastoreConf.setVar(conf, ConfVars.HIVE_AUTHORIZATION_MANAGER, DummyHmsAuthorizerFactory.class.getName()); | ||
MetastoreConf.setVar(conf, ConfVars.PRE_EVENT_LISTENERS, HiveMetaStoreAuthorizer.class.getName()); | ||
MetastoreConf.setVar(conf, ConfVars.HIVE_METASTORE_AUTHENTICATOR_MANAGER, HadoopDefaultMetastoreAuthenticator.class.getName() ); | ||
conf.set("hadoop.proxyuser.hive.groups", "*"); | ||
|
@@ -105,6 +125,17 @@ public void setUp() throws Exception { | |
} catch (Exception e) { | ||
// NoSuchObjectException will be ignored if the step objects are not there | ||
} | ||
dummyHiveAuthorizer = Mockito.mock(HiveAuthorizer.class); | ||
} | ||
|
||
static class DummyHmsAuthorizerFactory implements HiveAuthorizerFactory { | ||
@Override | ||
public HiveAuthorizer createHiveAuthorizer(HiveMetastoreClientFactory metastoreClientFactory, | ||
HiveConf conf, HiveAuthenticationProvider hiveAuthenticator, HiveAuthzSessionContext ctx) | ||
throws HiveAuthzPluginException { | ||
TestHiveMetaStoreAuthorizer.dummyHiveAuthorizer = Mockito.mock(HiveAuthorizer.class); | ||
return TestHiveMetaStoreAuthorizer.dummyHiveAuthorizer; | ||
} | ||
} | ||
|
||
@Test | ||
|
@@ -439,4 +470,66 @@ public void testUnAuthorizedCause() { | |
.anyMatch(stack -> stack.contains(DummyHiveAuthorizer.class.getName()))); | ||
} | ||
} | ||
|
||
/** | ||
* @return pair with left value as inputs and right value as outputs, | ||
* passed in current call to authorizer.checkPrivileges | ||
* @throws HiveAuthzPluginException | ||
* @throws HiveAccessControlException | ||
*/ | ||
private Pair<List<HivePrivilegeObject>, List<HivePrivilegeObject>> getHivePrivilegeObjectInputs() throws HiveAuthzPluginException, | ||
HiveAccessControlException { | ||
// Create argument capturer | ||
// a class variable cast to this generic of generic class | ||
Class<List<HivePrivilegeObject>> class_listPrivObjects = (Class) List.class; | ||
ArgumentCaptor<List<HivePrivilegeObject>> inputsCapturer = ArgumentCaptor | ||
.forClass(class_listPrivObjects); | ||
ArgumentCaptor<List<HivePrivilegeObject>> outputsCapturer = ArgumentCaptor | ||
.forClass(class_listPrivObjects); | ||
|
||
verify(dummyHiveAuthorizer).checkPrivileges(any(HiveOperationType.class), | ||
inputsCapturer.capture(), outputsCapturer.capture(), | ||
any(HiveAuthzContext.class)); | ||
|
||
return new ImmutablePair<List<HivePrivilegeObject>, List<HivePrivilegeObject>>( | ||
inputsCapturer.getValue(), outputsCapturer.getValue()); | ||
} | ||
|
||
@Test | ||
public void testCreateTab() { | ||
reset(dummyHiveAuthorizer); | ||
UserGroupInformation.setLoginUser(UserGroupInformation.createRemoteUser(authorizedUser)); | ||
try { | ||
Map<String, String> tableParams = new HashMap<String, String>(); | ||
tableParams.putIfAbsent("owner", "systest"); | ||
tableParams.putIfAbsent("external.table.purge", "true"); | ||
tableParams.putIfAbsent("current-schema", "{\"type\" :\"struct\", \"schema-id\":0, \"fields\":[{\"id\": 1, \"name\" :\"id\", \"required\":false, \"type\":\"int\"}, {\"id\":2, \"name\":\"txt\", \"required\":false, \"type\":\"string\"}]}"); | ||
tableParams.putIfAbsent("storage_handler", "org.apache.iceberg.mr.hive.HivelcebergStorageHandler"); | ||
tableParams.putIfAbsent("uuid", "c229e4b5-d1f8-4239-adeb-cb43d0f1d209"); | ||
tableParams.putIfAbsent("EXTERNAL", "TRUE"); | ||
tableParams.putIfAbsent("metadata_location", "hdfs://clustername/warehouse/tablespace/external/hive/icespark/metadata/00000-fa77f11c-6b5d-4da3-ae99-de907e525fbb.metadata.json"); | ||
tableParams.putIfAbsent("snapshot-count", "0"); | ||
tableParams.putIfAbsent("table_type", "ICEBERG"); | ||
Table table = new TableBuilder() | ||
.setTableName(tblName) | ||
.addCol("name", ColumnType.STRING_TYPE_NAME) | ||
.setOwner(authorizedUser) | ||
.setTableParams(tableParams) | ||
.build(conf); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a normal table, not a storage handler-based table. We need to verify a storage handler-based table. |
||
hmsHandler.create_table(table); | ||
Pair<List<HivePrivilegeObject>, List<HivePrivilegeObject>> io = getHivePrivilegeObjectInputs(); | ||
List<HivePrivilegeObject> outputs = io.getRight(); | ||
List<HivePrivilegeObject> inputs = io.getLeft(); | ||
assertEquals("No outputs for a select", 2, outputs.size()); | ||
assertEquals("One input for this select", 0, inputs.size()); | ||
for (HivePrivilegeObject hivePrivilegeObject : outputs){ | ||
assertTrue(hivePrivilegeObject.getObjectName().contains("storage_handler")); | ||
} | ||
|
||
} catch (Exception ex) { | ||
String[] rootCauseStackTrace = ExceptionUtils.getRootCauseStackTrace(ex); | ||
assertTrue(Arrays.stream(rootCauseStackTrace) | ||
.anyMatch(stack -> stack.contains(DummyHiveAuthorizer.class.getName()))); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: incorrect imports