> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java, > > line 134 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387295#file1387295line134> > > > > 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. > > Sergey Shelukhin wrote: > The same principal didn't work on the test cluster I had for some reason > that I no longer remember :(
The logic does a login with ZK keytab, otherwise fallback to the one used by LLAP. Either the LLAP keytab works - in which case the ZK login is unnecessary, OR the LLAP keytab does not work - in which case the fallback should not exist. If this is an unknown, should we create a follow up jira to investigate and fix, and meanwhile add a comment in the code ? > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java, > > line 170 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387296#file1387296line170> > > > > Maybe move all of these checks into the RPC layers itself ... i.e. > > LlapServiceServerImpl. As early as possible. > > Sergey Shelukhin wrote: > The permissions are checked in calls by ContainerRunner. RPC right now > just propagates the request... The suggestion is to move it up :) > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java, > > line 287 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387298#file1387298line287> > > > > 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. > > Sergey Shelukhin wrote: > this actually kind of orthogonal. This logic doesn't apply to when HS2 > creates the token preciusely because HS2 knows whether it's creating the > token for Tez or external, so it can set the flag accordingly. > When the method is called remotely, by default we always require signing, > but that can be disabled for CLI, or HS2 calling remotely (presumably under > the same user as LLAP). I think we need a ManagementInterface - which has the local and remote implementation. The if/else block in the core (TezSessionState) is fairly ugly. All of the implementation details and logic for the individual cases can hide in there. Likely a separate jira ? > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java, > > line 290 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387298#file1387298line290> > > > > 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. > > Sergey Shelukhin wrote: > This is the calling user in case of RPC. This goes back to whether it's invoked locally or remotely. Local and Remote calls by HS2 to obtain a token on behalf of an external client, will need to pass in the user name to generate the token correctly. What's obtained on the RPC call will almost always be the Hive user - at least for the remote call. Behaviour should not change if the flag to get the token locally / remotely is changed. > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java, > > line 61 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387304#file1387304line61> > > > > 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. > > Sergey Shelukhin wrote: > This one has completely different logic and even different template > parameter. Most of the logic is in the BaseSecretManager itself, correct ? The same instance can be used to generate tokens, as well as sign. Is there a downside to that ? Setting up two instances would create extra threads, and confusion while debugging; Also potentially additional load to ZK, additional logins, etc - Siddharth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46956/#review134076 ----------------------------------------------------------- On May 21, 2016, 12:07 a.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46956/ > ----------------------------------------------------------- > > (Updated May 21, 2016, 12:07 a.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 4cfa5f1 > > 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/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/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 > >
