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 03:56:36 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40315/#review107706
-----------------------------------------------------------



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2348)
<https://reviews.apache.org/r/40315/#comment166963>

    Description needs fixing.



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2351)
<https://reviews.apache.org/r/40315/#comment166964>

    Don't think the default value - "*" - has any significance here. Replace by null - to
avoid confusion.



llap-client/src/java/org/apache/hadoop/hive/llap/io/api/LlapIoProxy.java (line 76)
<https://reviews.apache.org/r/40315/#comment166965>

    Rename class to LLAPProxy ? It's no longer limited to IO only.



llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java (line 44)
<https://reviews.apache.org/r/40315/#comment166966>

    This and readFields - eventually should be implemented using a Protobuf payload. Allows
the token to evolve during rolling upgrades.
    Separate jira.



llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java (line 71)
<https://reviews.apache.org/r/40315/#comment166967>

    Does a renewer for a token type have to be specified ?



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolClientImpl.java
(line 128)
<https://reviews.apache.org/r/40315/#comment166968>

    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.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolClientImpl.java
(line 129)
<https://reviews.apache.org/r/40315/#comment166969>

    Rename to getDelegationToken ?



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java
(line 79)
<https://reviews.apache.org/r/40315/#comment166970>

    throws IOException not required.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java
(line 133)
<https://reviews.apache.org/r/40315/#comment166971>

    Sanity checks for the values. Empty strings are not allowed.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java
(line 157)
<https://reviews.apache.org/r/40315/#comment166972>

    Avoid using the ZK property names. Instead, the properties defined for LLAP should be
used.
    (ZK properties could leak in from some other service which uses the same SecretManager)



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java
(line 164)
<https://reviews.apache.org/r/40315/#comment166973>

    New instances of Configuration if doing a set (this config is shared by several services)



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemonProtocolServerImpl.java
(line 250)
<https://reviews.apache.org/r/40315/#comment166974>

    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.



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapDaemonPolicyProvider.java (line
26)
<https://reviews.apache.org/r/40315/#comment166975>

    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.



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapDaemonPolicyProvider.java (line
32)
<https://reviews.apache.org/r/40315/#comment166976>

    clone not required.



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 53)
<https://reviews.apache.org/r/40315/#comment166977>

    This would matter when running under HiveServer ? or is the synchronization in LlapIoProxy
taking care of this ?



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 59)
<https://reviews.apache.org/r/40315/#comment166978>

    final. Also retryPolicy.



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 67)
<https://reviews.apache.org/r/40315/#comment166979>

    make configurable ?



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 94)
<https://reviews.apache.org/r/40315/#comment166980>

    Lots of TODOs - fix / convert to jira with a reference to the jira number ?



llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java (line 96)
<https://reviews.apache.org/r/40315/#comment166981>

    Stop logging the token.



llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java (line 100)
<https://reviews.apache.org/r/40315/#comment166983>

    Can be accessed via TaskCommunicatorContext. More on this in SessionState.



llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java (line 105)
<https://reviews.apache.org/r/40315/#comment166982>

    Stop logging the token.



llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java (line 511)
<https://reviews.apache.org/r/40315/#comment166984>

    Required for each host separately ? Setting the host may not be required.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java (line 319)
<https://reviews.apache.org/r/40315/#comment166985>

    Emailing the token to the AM in the Configuration is very avoidable.
    
    The token can be provided to Tez while creating the TezClient. TezClient will make this
available to the TaskCommunicator via the TaskCommunicatorContext.getCredentials().
    
    See TokenCache.get/setSessionToken.
    The static string defined in HiveConf to send this token could be shortened and moved
out of HiveConf.


Haven't looked at the details of the ZKSecretManager - but it looks like the Tokens issued
by any of the LLAP instances can be used by an application to communicate with all other instances.
Also, are the tokens the same for different applications ?

- Siddharth Seth


On Nov. 16, 2015, 7:45 p.m., Sergey Shelukhin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40315/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2015, 7:45 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 838f25c 
>   llap-client/src/java/org/apache/hadoop/hive/llap/io/api/LlapIoProxy.java 4c31e32 
>   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 9ab3e98 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 07f26be 
>   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