----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46956/#review134076 -----------------------------------------------------------
Think it will be useful to add some tests around 1) signing / validation 2) The config parameter (assuming it stays), and it behaving the way it's intended to make sure tokens are created with the correct parameters. There's a lot of ! of ! of ! checks happening. common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2698 - 2699) <https://reviews.apache.org/r/46956/#comment198707> Is this primarily for config ? Rename to have a positive connotation maybe ? common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2699) <https://reviews.apache.org/r/46956/#comment198708> Rename "user" to "llapuser"/"serviceowner" - something that implies this is only for the user owning the service. Maybe call the other two settings "always", "never" - instead of "true", "false" common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2700) <https://reviews.apache.org/r/46956/#comment198715> What should the default value here be ? "false" seems to imply sign alyway. In case the client config is setup to obtain tokens remotely - instead of directly from ZK on the client side in HS2 - Tez would end up obtaining tokens which require signing as well ? llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java (line 29) <https://reviews.apache.org/r/46956/#comment198722> I'm not sure this will actually be usable, given that what is being signed is a protobuf generated class. llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java (line 1) <https://reviews.apache.org/r/46956/#comment198710> Not used anywhere. Re-introduce in the patch where it's required ? llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java (line 126) <https://reviews.apache.org/r/46956/#comment198713> Can a second login be avoided. I'm guessing this is because the ZK principla may be different from the llap principla. What was the reason for them to be different again ? (Especially w.r.t the SecretManager). Not sure if the fallback to using the llap principal and keytab will work if they have to be different. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java (line 168) <https://reviews.apache.org/r/46956/#comment198718> Move this to after checking if vertexBinary is set ? Potentially error out if both are set. IIRC, vertexBinary will be set by external clients, and vertex will be set by Tez ? llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java (line 170) <https://reviews.apache.org/r/46956/#comment198721> Maybe move all of these checks into the RPC layers itself ... i.e. LlapServiceServerImpl. As early as possible. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java (line 262) <https://reviews.apache.org/r/46956/#comment198719> Why is this required ? The signature will only exist if vertexBinary is present ? llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java (line 267) <https://reviews.apache.org/r/46956/#comment198724> A follow up jira may be to limit the age of keys. i.e. if a keyId is older than a certain amount of time - fail the request. I'm not sure how ZKSecretManager rotates these keys, and when they are invalidated. A user can potentially use an old (presumably compromsied key) to generate requests - which will be valid if keys are not rotated/aged. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java (line 66) <https://reviews.apache.org/r/46956/#comment198711> The meaning is a little unclear, when considered along with the negative connotation of the config parameter. I don't actually know what a TRUE value here means. Even more so when considered alongside the parameter called "isNoSigning" llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java (line 144) <https://reviews.apache.org/r/46956/#comment198712> "user" llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java (line 284) <https://reviews.apache.org/r/46956/#comment198716> All of this logic should be invoked even when obtaining tokens from ZKSM directly. Whether Tez is being used, or an external client - as long as HS2 is obtaining a token, it can do it directly from ZK. This code path is not likely to be exercised a lot. Assuming that invocation (when it happens, and likely needs another jira) - will call in to LlapTokenLocalClient.createToken directly - and will send in isSigningRequired based on all of the same configs. Would be better to move the logic out of this function in that case. Maybe the config flag itself could be dropped. If Tez, no singing, if external - force signing. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java (line 287) <https://reviews.apache.org/r/46956/#comment198714> What user is expected over here. 1. In case of an invocation by HS2 to run a Tez query - I'm assuming this would be the HS2 service user (which is the same as the LLAP service user). (That needs to be validated) 2. In case of external services - would this be the HS2 service user or the user associated with the external service ? If it's the HS2 user each time, is the "user"/"realuser" field in the TokenIdentifier required ? That seems to be passed in as a null everywhere. Assuming the appId is what will be used to differentiate different external clients ? and that in case of Tez - there's no differentiation. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java <https://reviews.apache.org/r/46956/#comment198717> Think the patch which added Pair/ImmutablePair may have added a maven dependency. Should be removed if it was added explicitly for this. llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java (line 35) <https://reviews.apache.org/r/46956/#comment198725> Nit: final llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 1) <https://reviews.apache.org/r/46956/#comment198709> This entire class is not used anywhere. Can it be dropped, and re-introduced in another jira if it is actually required. llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java (line 61) <https://reviews.apache.org/r/46956/#comment198720> I don't think we need to create a new instance of the ZKDelegationTokenSecretManager. The one created earlier to generate tokens should be passed in. The KeySigner could be an interface instead, and SecretManager (extends ZKDelegationTokenSecretManager) can implement this. ACL checks etc are already setup there. There's no requirement to have two independent copies of the ZKSecretManager running in the same daemon. - Siddharth Seth On May 18, 2016, 8:36 p.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46956/ > ----------------------------------------------------------- > > (Updated May 18, 2016, 8:36 p.m.) > > > Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth. > > > Repository: hive-git > > > Description > ------- > > see jira > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java cbb3a72 > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java > f10351b > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java > PRE-CREATION > > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java > e28eddd > > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java > PRE-CREATION > > llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java > 465b204 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java > 2524dc2 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java > de817e3 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java > b94fc2e > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java > 03ee055 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java > 8abd198 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java > eac0e8f > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java > 74359fa > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java > PRE-CREATION > > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java > 279baf1 > > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java > aaaa762 > > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java > a250882 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b > > Diff: https://reviews.apache.org/r/46956/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
