> On May 16, 2017, 6:32 p.m., Vamsee Yarlagadda wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java > > Lines 77-79 (original) > > <https://reviews.apache.org/r/59317/diff/1/?file=1721744#file1721744line77> > > > > I think it is not a good idea to remove this section. This patch > > handles it well for Generic clients but for HDFS and Policy client the > > behavior is unexpected. > > > > The reason why we are getting the right UGI for HDFS, Hive is that > > their codebases are doing some UGI related stuff before starting the Sentry > > client connection and that is why we are getting that context for free. And > > Hive, HDFS logic of setting UGI is not aware that a sentry client is > > depenedent on it. In future, if they restructure code it is very likely > > that they will break us and a more diffucult problem would be to debug this > > and fix it. So i would rather suggest the client do all the things (setting > > UGI) it needs so that these long chain of dependencies could be avoided. > > kalyan kumar kalvagadda wrote: > From what I understand, In hadoop if one component wants to talk to other > component securely using kerberos, source component should call > UserGroupInformation.setConfiguration(conf); before calling the thift client > of destination component. setConfiguration would update the static data in > class UserGroupInformation so that appropriate information is avaialble when > UserGroupInformation is instantiated. I have checked Hive, sentry code to > confirm the behavior. > > For generic clients, things are done differently as the configuraion > provided to sentry doesn't have HADOOP_SECURITY_AUTHENTICATION propery. > Sentry generic policy client updates the configuration and performs > setConfiguration explicitly. > > > If sentry client calls setConfiguration on UserGroupInformation for every > client, this update would have impact on other secured connections that > source component initiates. I'm sure if we should be doing it. If you are > sure of it, i will update the patch. > > Vamsee Yarlagadda wrote: > I understand your argument but IMO from the debugging perspective, it is > good that the client does all the things it needs for proper execution rather > than assuming some other precursor process would take care of it. > > bq. this update would have impact on other secured connections that > source component initiates. > I doubt that it does given that it is the same conf being used for all > invocations. > > I am also ok with the proposed solution provided we give a good set of > comments before we call the loginUser() that the sentry client assumes UGI > has been set properly before the line gets executed.
Are you saying that you are fine if a comment is added in sentry client implimentation before getLoginUser() method invocation? - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59317/#review175135 ----------------------------------------------------------- On May 16, 2017, 6:21 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59317/ > ----------------------------------------------------------- > > (Updated May 16, 2017, 6:21 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, > Sergio Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1736 > https://issues.apache.org/jira/browse/SENTRY-1736 > > > Repository: sentry > > > Description > ------- > > As part of SENTRY-1593, code in SentryGenericServiceClientDefaultImpl which > set HADOOP_SECURITY_AUTHENTICATION property and update UserGroupInformation > class is missed. > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java > 9b9f9e8 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > b7ac640 > > > Diff: https://reviews.apache.org/r/59317/diff/1/ > > > Testing > ------- > > Made sure that Solr client is able to talk to sentry with kerberos enabled. > > > Thanks, > > kalyan kumar kalvagadda > >
