hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Siddharth Seth" <ss...@apache.org>
Subject Re: Review Request 40315: HIVE-12341 LLAP security
Date Tue, 24 Nov 2015 23:52:51 GMT


> On Nov. 24, 2015, 3:56 a.m., Siddharth Seth wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 2361
> > <https://reviews.apache.org/r/40315/diff/2/?file=1127510#file1127510line2361>
> >
> >     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
> > <https://reviews.apache.org/r/40315/diff/2/?file=1127512#file1127512line71>
> >
> >     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
> > <https://reviews.apache.org/r/40315/diff/2/?file=1127516#file1127516line129>
> >
> >     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
> > <https://reviews.apache.org/r/40315/diff/2/?file=1127517#file1127517line251>
> >
> >     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
> > <https://reviews.apache.org/r/40315/diff/2/?file=1127519#file1127519line26>
> >
> >     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
> > <https://reviews.apache.org/r/40315/diff/2/?file=1127519#file1127519line32>
> >
> >     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
> > <https://reviews.apache.org/r/40315/diff/2/?file=1127520#file1127520line53>
> >
> >     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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message