Return-Path: X-Original-To: apmail-hive-dev-archive@www.apache.org Delivered-To: apmail-hive-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 31CBB17293 for ; Tue, 24 Nov 2015 23:52:53 +0000 (UTC) Received: (qmail 23815 invoked by uid 500); 24 Nov 2015 23:52:52 -0000 Delivered-To: apmail-hive-dev-archive@hive.apache.org Received: (qmail 23736 invoked by uid 500); 24 Nov 2015 23:52:52 -0000 Mailing-List: contact dev-help@hive.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hive.apache.org Delivered-To: mailing list dev@hive.apache.org Received: (qmail 23688 invoked by uid 99); 24 Nov 2015 23:52:52 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 24 Nov 2015 23:52:52 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 081332E3DC3; Tue, 24 Nov 2015 23:52:51 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============4673899151297634739==" MIME-Version: 1.0 Subject: Re: Review Request 40315: HIVE-12341 LLAP security From: "Siddharth Seth" To: "Siddharth Seth" , "Gopal V" Cc: "hive" , "Sergey Shelukhin" Date: Tue, 24 Nov 2015 23:52:51 -0000 Message-ID: <20151124235251.24387.56763@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Siddharth Seth" X-ReviewGroup: hive X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/40315/ X-Sender: "Siddharth Seth" References: <20151124035636.26797.45671@reviews.apache.org> In-Reply-To: <20151124035636.26797.45671@reviews.apache.org> Reply-To: "Siddharth Seth" X-ReviewRequest-Repository: hive-git --===============4673899151297634739== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Nov. 24, 2015, 3:56 a.m., Siddharth Seth wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 2361 > > > > > > Don't think the default value - "*" - has any significance here. Replace by null - to avoid confusion. > > Sergey Shelukhin wrote: > Following in the footsteps of slider Can we drop the * in favor of a null. (or no default if Hive supports that). > On Nov. 24, 2015, 3:56 a.m., Siddharth Seth wrote: > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java, line 71 > > > > > > Does a renewer for a token type have to be specified ? > > Sergey Shelukhin wrote: > renewer is set elsewhere Not the user which does the renewal. Implementation of the Renewer interface. See TrivialRenewer in Token.java. Not sure if one is required for each token kind - would be safer to add one which accepts the LLAP token kind and says it's not managed for now. > On Nov. 24, 2015, 3:56 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolClientImpl.java, line 129 > > > > > > This could be moved into it's own protocol (but listening on the same server). > > > > The methods so far are for access from the AM. > > > > getTokens is to be used by Clients. > > > > What that also allows is for the annotations to change. > > getTokens() - protected by Kerberos, and cannot be obtained using a token. > > Remaining methods - require a token. > > Sergey Shelukhin wrote: > why add an extra protocol? it seems like most services don't handle tokens like this. HDFS gives out tokens as part of normal interface. HDFS also doesn't have a separation of API vs server side jars. Reason for separation is that they're very different operations - one relates to exuecuting and tracking work, the other to access. Consider the case where there is a central service which is responsible for handing out these delegation tokens (rotation, etc etc). That will definitely not implement the submitWork APIs, and the daemons cannot implement the getTokns at that point. Doesn't need to be done rightnow, and can be changed later since this API is private for the moment (It is private, right?) Re-opened primarily for the "Is this private" bit. > On Nov. 24, 2015, 3:56 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java, line 251 > > > > > > YARN can take care of renewing delegation tokens - assuming the service supports it (i.e. the ZKSecretManager on one of the LLAP instances or a direct connection to ZK from the RM - but that isn't a good idea). > > Eventually, I believe the renweer would need to change to the RM service user. > > Sergey Shelukhin wrote: > Hmm... not sure how this would work. Can you file a follow-up JIRA? By MR logic, the renewer would be a central job manager, e.g. HS2 YARN (The RM) is given tokens for a job. It checks whether these tokens are managed or not (via the Renewer interface), and takes care of renewing them while the job is alive. That's what is done for HDFS delegation tokens anyway - renewed by the RM every day while the job is running. The same could be done for LLAP - but this is for later. Opening a jira to track renewal of tokens. > On Nov. 24, 2015, 3:56 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapDaemonPolicyProvider.java, line 26 > > > > > > How is the default value picked up ? (definitely not from the hive conf) > > OR > > What is the default value - "*" or " ". > > I'm not sure how other services handle this - but this can be set to " " by default on secure clusters, and "*" on non-secure clusters. > > Sergey Shelukhin wrote: > From the conf passed to refreshServiceAcl it looks like. What if the fiels is not set ? > On Nov. 24, 2015, 3:56 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapDaemonPolicyProvider.java, line 32 > > > > > > clone not required. > > Sergey Shelukhin wrote: > that looks like what other services do Looking at yarn services, this isn't required. A final array is being returned - this was likely changed to get past findbugs warnings, and ignored in YARN > On Nov. 24, 2015, 3:56 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java, line 53 > > > > > > This would matter when running under HiveServer ? or is the synchronization in LlapIoProxy taking care of this ? > > Sergey Shelukhin wrote: > shouldn't matter why ? Multiple threads launching AMs and accessing Proxy.getTokens() - Siddharth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40315/#review107706 ----------------------------------------------------------- On Nov. 24, 2015, 11:11 p.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40315/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2015, 11:11 p.m.) > > > Review request for hive, Gopal V and Siddharth Seth. > > > Repository: hive-git > > > Description > ------- > > see JIRA > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java fffedd9 > llap-client/src/java/org/apache/hadoop/hive/llap/io/api/LlapIoProxy.java 4c31e32 > llap-client/src/java/org/apache/hadoop/hive/llap/io/api/LlapProxy.java PRE-CREATION > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java PRE-CREATION > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java PRE-CREATION > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/LlapDaemonProtocolBlockingPB.java 5ad2344 > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 98b1ccd > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolClientImpl.java 4b13277 > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java 784c631 > llap-server/src/java/org/apache/hadoop/hive/llap/protocol/LlapTaskUmbilicalProtocol.java fae7654 > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapDaemonPolicyProvider.java PRE-CREATION > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java PRE-CREATION > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapServerSecurityInfo.java PRE-CREATION > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapTokenSelector.java PRE-CREATION > llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskCommunicator.java d327fc0 > llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapUmbilicalPolicyProvider.java PRE-CREATION > llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java 33e998c > llap-server/src/main/resources/META-INF/services/org.apache.hadoop.security.SecurityInfo PRE-CREATION > llap-server/src/protobuf/LlapDaemonProtocol.proto 0ba6acf > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapDaemonProtocolServerImpl.java 8d45c95 > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java a210b95 > ql/src/java/org/apache/hadoop/hive/ql/exec/GlobalWorkMapFactory.java 59ee347 > ql/src/java/org/apache/hadoop/hive/ql/exec/ObjectCacheFactory.java 3d9771a > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/MapRecordProcessor.java 914b4e7 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/ReduceRecordProcessor.java efcf88c > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 07f26be > ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 3feab1a > serde/src/java/org/apache/hadoop/hive/serde2/AbstractSerDe.java 9269ff4 > > Diff: https://reviews.apache.org/r/40315/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > > --===============4673899151297634739==--