SourabhBadhya commented on code in PR #4821: URL: https://github.com/apache/hive/pull/4821#discussion_r1378448646
########## itests/hive-minikdc/src/test/java/org/apache/hadoop/hive/ql/session/TestAddMetastoreDelegationTokenToUGI.java: ########## @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hive.ql.session; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hive.metastore.HiveMetaStoreClient; +import org.apache.hadoop.hive.metastore.TestRemoteHiveMetaStore; +import org.apache.hadoop.hive.metastore.TestHiveMetaStore; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars; +import org.apache.thrift.transport.TTransportException; +import org.apache.hadoop.hive.cli.CliSessionState; +import org.apache.hadoop.hive.ql.session.SessionState; +import org.apache.hadoop.hive.metastore.utils.SecurityUtils; +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hive.minikdc.MiniHiveKdc; +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.assertNotNull; + + +import java.util.List; + +/** + * TestAddMetastoreDelegationTokenToUGI : Simple base test to add metastore delegation token to UserGroupInformation object + */ +public class TestAddMetastoreDelegationTokenToUGI extends TestRemoteHiveMetaStore { + + private static final String CLITOKEN = "HiveClientImpersonationToken"; + public static MiniHiveKdc miniKDC; + + @Before + public void setUp() throws Exception { + if (null == miniKDC) { + miniKDC = new MiniHiveKdc(); + String hiveMetastorePrincipal = + miniKDC.getFullyQualifiedServicePrincipal(miniKDC.getHiveMetastoreServicePrincipal()); + String hiveMetastoreKeytab = miniKDC.getKeyTabFile( + miniKDC.getServicePrincipalForUser(miniKDC.getHiveMetastoreServicePrincipal())); + + initConf(); + MetastoreConf.setBoolVar(conf, ConfVars.USE_THRIFT_SASL, true); + MetastoreConf.setVar(conf, ConfVars.KERBEROS_PRINCIPAL, hiveMetastorePrincipal); + MetastoreConf.setVar(conf, ConfVars.KERBEROS_KEYTAB_FILE, hiveMetastoreKeytab); + MetastoreConf.setBoolVar(conf, ConfVars.EXECUTE_SET_UGI, false); + } + super.setUp(); + } + + @Test + public void testGetDelegationTokenFromUGI() throws Exception { + HiveConf clientConf = new HiveConf(conf, TestAddMetastoreDelegationTokenToUGI.class); + HiveConf.setVar(clientConf, HiveConf.ConfVars.METASTOREURIS, "thrift://localhost:" + port); + HiveConf.setBoolVar(clientConf, HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL, true); + SessionState.start(new CliSessionState(clientConf)); + assertNotNull(SecurityUtils.getTokenStrForm(CLITOKEN)); Review Comment: Can we add another test that without the following setup, the CLITOKEN retrieved is null. ########## itests/hive-minikdc/src/test/java/org/apache/hadoop/hive/ql/session/TestAddMetastoreDelegationTokenToUGI.java: ########## @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hive.ql.session; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hive.metastore.HiveMetaStoreClient; +import org.apache.hadoop.hive.metastore.TestRemoteHiveMetaStore; +import org.apache.hadoop.hive.metastore.TestHiveMetaStore; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars; +import org.apache.thrift.transport.TTransportException; +import org.apache.hadoop.hive.cli.CliSessionState; +import org.apache.hadoop.hive.ql.session.SessionState; +import org.apache.hadoop.hive.metastore.utils.SecurityUtils; +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hive.minikdc.MiniHiveKdc; +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.assertNotNull; + + +import java.util.List; + +/** + * TestAddMetastoreDelegationTokenToUGI : Simple base test to add metastore delegation token to UserGroupInformation object + */ +public class TestAddMetastoreDelegationTokenToUGI extends TestRemoteHiveMetaStore { Review Comment: Can we move this test to itests/hive-minikdc? You seem to use that package a lot in this test and getting the utils will be easier. ########## ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java: ########## @@ -731,6 +734,24 @@ private static void start(SessionState startSs, boolean isAsync, LogHelper conso // that would cause ClassNoFoundException otherwise throw new RuntimeException(e); } + try { + if(!startSs.isHiveServerQuery){ + if (getSessionConf().getBoolVar(ConfVars.METASTORE_USE_THRIFT_SASL)) { + UserGroupInformation UGI = Utils.getUGI(); + String hmsDelegationTokenStr = Hive.get().getDelegationToken(UGI.getShortUserName(), UGI.getShortUserName()); + if (hmsDelegationTokenStr != null) { + getSessionConf().setVar(ConfVars.METASTORE_TOKEN_SIGNATURE, CLITOKEN); + try { + SecurityUtils.setTokenStr(UGI, hmsDelegationTokenStr, CLITOKEN); + } catch (IOException e) { + throw new IOException("Couldn't setup delegation token in the ugi: " + e, e); Review Comment: I think this is better - `throw new IOException("Couldn't set delegation token in the UGI due to: " + e.getMessage(), e);` ########## ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java: ########## @@ -731,6 +734,24 @@ private static void start(SessionState startSs, boolean isAsync, LogHelper conso // that would cause ClassNoFoundException otherwise throw new RuntimeException(e); } + try { + if(!startSs.isHiveServerQuery){ Review Comment: A lot of nested if conditionals. Can we move this inside a method and probably invert the if conditions so that if those are satisfied then return. Something like - ``` void methodName() { if (startSs.isHiveServerQuery) { return; } if (!getSessionConf().getBoolVar(ConfVars.METASTORE_USE_THRIFT_SASL) { return; } ...... (so on) } ``` ########## ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java: ########## @@ -731,6 +734,24 @@ private static void start(SessionState startSs, boolean isAsync, LogHelper conso // that would cause ClassNoFoundException otherwise throw new RuntimeException(e); } + try { + if(!startSs.isHiveServerQuery){ + if (getSessionConf().getBoolVar(ConfVars.METASTORE_USE_THRIFT_SASL)) { + UserGroupInformation UGI = Utils.getUGI(); + String hmsDelegationTokenStr = Hive.get().getDelegationToken(UGI.getShortUserName(), UGI.getShortUserName()); + if (hmsDelegationTokenStr != null) { + getSessionConf().setVar(ConfVars.METASTORE_TOKEN_SIGNATURE, CLITOKEN); + try { + SecurityUtils.setTokenStr(UGI, hmsDelegationTokenStr, CLITOKEN); + } catch (IOException e) { + throw new IOException("Couldn't setup delegation token in the ugi: " + e, e); + } + } + } + } + }catch (Exception e) { Review Comment: Nit: Space before catch. ########## itests/hive-minikdc/src/test/java/org/apache/hadoop/hive/ql/session/TestAddMetastoreDelegationTokenToUGI.java: ########## @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hive.ql.session; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hive.metastore.HiveMetaStoreClient; +import org.apache.hadoop.hive.metastore.TestRemoteHiveMetaStore; +import org.apache.hadoop.hive.metastore.TestHiveMetaStore; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars; +import org.apache.thrift.transport.TTransportException; +import org.apache.hadoop.hive.cli.CliSessionState; +import org.apache.hadoop.hive.ql.session.SessionState; +import org.apache.hadoop.hive.metastore.utils.SecurityUtils; +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hive.minikdc.MiniHiveKdc; +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.assertNotNull; + + +import java.util.List; + +/** + * TestAddMetastoreDelegationTokenToUGI : Simple base test to add metastore delegation token to UserGroupInformation object + */ +public class TestAddMetastoreDelegationTokenToUGI extends TestRemoteHiveMetaStore { + + private static final String CLITOKEN = "HiveClientImpersonationToken"; + public static MiniHiveKdc miniKDC; + + @Before + public void setUp() throws Exception { + if (null == miniKDC) { + miniKDC = new MiniHiveKdc(); + String hiveMetastorePrincipal = + miniKDC.getFullyQualifiedServicePrincipal(miniKDC.getHiveMetastoreServicePrincipal()); + String hiveMetastoreKeytab = miniKDC.getKeyTabFile( + miniKDC.getServicePrincipalForUser(miniKDC.getHiveMetastoreServicePrincipal())); + + initConf(); + MetastoreConf.setBoolVar(conf, ConfVars.USE_THRIFT_SASL, true); + MetastoreConf.setVar(conf, ConfVars.KERBEROS_PRINCIPAL, hiveMetastorePrincipal); + MetastoreConf.setVar(conf, ConfVars.KERBEROS_KEYTAB_FILE, hiveMetastoreKeytab); + MetastoreConf.setBoolVar(conf, ConfVars.EXECUTE_SET_UGI, false); + } + super.setUp(); Review Comment: Isn't this same code as what is present in `setup()` method of `TestRemoteHiveMetaStoreKerberos.java`? Consider reusing the setup method. Or better move the test there itself. ########## ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java: ########## @@ -131,6 +132,8 @@ public class SessionState implements ISessionAuthState{ private static final String LOCAL_SESSION_PATH_KEY = "_hive.local.session.path"; private static final String HDFS_SESSION_PATH_KEY = "_hive.hdfs.session.path"; private static final String TMP_TABLE_SPACE_KEY = "_hive.tmp_table_space"; + private static final String CLITOKEN = "HiveClientImpersonationToken"; Review Comment: Is there a reason why this constant has the given name? Or this is just a custom name? ########## itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/MiniHiveKdc.java: ########## @@ -160,7 +160,7 @@ public String getDefaultUserPrincipal() { return HIVE_TEST_USER_1; } - String getHiveMetastoreServicePrincipal() { + public String getHiveMetastoreServicePrincipal() { Review Comment: I dont think this might be necessary if we move the test to itests/hive-minikdc. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
