You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2017/05/16 18:21:59 UTC

Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

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

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

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

> 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.
> 
> kalyan kumar kalvagadda wrote:
>     Are you saying that you are fine if a comment is added in sentry client implimentation before getLoginUser() method invocation?

Yes but please give a complete description on the issues one would notice if the UGI is not instantiated before.


- Vamsee


-----------------------------------------------------------
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
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

> 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
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

> 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.

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.


- Vamsee


-----------------------------------------------------------
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
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175135
-----------------------------------------------------------




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/#comment248508>

    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.


- Vamsee Yarlagadda


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
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175430
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Line 77 (original), 77 (patched)
<https://reviews.apache.org/r/59317/#comment248879>

    Could you, please, elaborate on why you ended up placing UGI initialization here, as opposed to SentryGenericServiceClientDefaultImpl ctor, as in the previous revision?



sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java
Lines 34 (patched)
<https://reviews.apache.org/r/59317/#comment248878>

    The idea is right, but the implementation also needs to be thread-safe.
    
    Standard solution would be using AtomicBoolean instead of boolean:
    
    private static final AtomicBoolean isInitialized = new AtomicBoolean(false);
    
    and do checking as this:
    
    if (isInitialized.compareAndSet(false, true)) {
    // existing initialization logic goes here
    }
    
    This is supposedly faster than just declaring the entire initialize() method synchronized (which would be another solution).



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Line 66 (original), 64 (patched)
<https://reviews.apache.org/r/59317/#comment248875>

    "this.conf" is only initialized on the next line; should use "conf" instead of "this.conf" - or swap these two lines.


- Vadim Spector


On May 18, 2017, 8:57 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 8:57 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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/3/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

> On May 18, 2017, 10:21 p.m., Vamsee Yarlagadda wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java
> > Lines 43-45 (patched)
> > <https://reviews.apache.org/r/59317/diff/3/?file=1724152#file1724152line43>
> >
> >     We should only do this if conf contains any of the property sentry.service.security.mode or  sentry.hdfs.service.security.mode is set to kerberos?
> 
> kalyan kumar kalvagadda wrote:
>     All of that is implicitly checked. UgiSaslClientTransport in transport factory will be called only when sentry.service.security.mode is kerberos and sentry.service.security.use.ugi are enabled.

Oh yeah you are right. This is taken care of by transportFactory.isKerberosEnabled() check.


- Vamsee


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


On May 18, 2017, 10:35 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 10:35 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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/4/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

> On May 18, 2017, 10:21 p.m., Vamsee Yarlagadda wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java
> > Lines 43-45 (patched)
> > <https://reviews.apache.org/r/59317/diff/3/?file=1724152#file1724152line43>
> >
> >     We should only do this if conf contains any of the property sentry.service.security.mode or  sentry.hdfs.service.security.mode is set to kerberos?
> 
> kalyan kumar kalvagadda wrote:
>     All of that is implicitly checked. UgiSaslClientTransport in transport factory will be called only when sentry.service.security.mode is kerberos and sentry.service.security.use.ugi are enabled.
> 
> Vamsee Yarlagadda wrote:
>     Oh yeah you are right. This is taken care of by transportFactory.isKerberosEnabled() check.

yes


- kalyan kumar


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


On May 18, 2017, 10:35 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 10:35 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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/4/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175440
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java
Lines 43-45 (patched)
<https://reviews.apache.org/r/59317/#comment248887>

    We should only do this if conf contains any of the property sentry.service.security.mode or  sentry.hdfs.service.security.mode is set to kerberos?


- Vamsee Yarlagadda


On May 18, 2017, 8:57 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 8:57 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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/3/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175448
-----------------------------------------------------------


Ship it!




Ship It!

- Alexander Kolbasov


On May 18, 2017, 10:35 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 10:35 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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/4/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175444
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java
Lines 37-45 (patched)
<https://reviews.apache.org/r/59317/#comment248893>

    Patch looks really good!
    
    One small concern, if multiple threads are trying to access this function at the same time, only one of them would go inside the "if" block. But how would we block other threads from simply returning before the thread handling the "if" block is complete?


- Vamsee Yarlagadda


On May 18, 2017, 10:35 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 10:35 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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/4/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175460
-----------------------------------------------------------


Ship it!




Ship It!

- Vamsee Yarlagadda


On May 19, 2017, 1:18 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 19, 2017, 1:18 a.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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/5/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175553
-----------------------------------------------------------


Ship it!




Ship It!

- Vadim Spector


On May 19, 2017, 1:18 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 19, 2017, 1:18 a.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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/5/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

> On May 19, 2017, 6:26 p.m., Vadim Spector wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java
> > Line 36 (original), 40 (patched)
> > <https://reviews.apache.org/r/59317/diff/4-5/?file=1724185#file1724185line42>
> >
> >     Hmm ... how dit it end up reverting the fix?

It had a much bigger potential problem:

"One small concern, if multiple threads are trying to access this function at the same time, only one of them would go inside the "if (isInitialized.compareAndSet(false, true))" block. But how would we block other threads from simply returning before the thread handling the "if" block is complete?"


- Vamsee


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


On May 19, 2017, 1:18 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 19, 2017, 1:18 a.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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/5/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vadim Spector <vs...@cloudera.com>.

> On May 19, 2017, 6:26 p.m., Vadim Spector wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java
> > Line 36 (original), 40 (patched)
> > <https://reviews.apache.org/r/59317/diff/4-5/?file=1724185#file1724185line42>
> >
> >     Hmm ... how dit it end up reverting the fix?
> 
> Vamsee Yarlagadda wrote:
>     It had a much bigger potential problem:
>     
>     "One small concern, if multiple threads are trying to access this function at the same time, only one of them would go inside the "if (isInitialized.compareAndSet(false, true))" block. But how would we block other threads from simply returning before the thread handling the "if" block is complete?"

You are right, this case is possible. I've noticed that you declared isInitialized volatile, which should take care of cross-thread visibility, so it is fine now.


- Vadim


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


On May 19, 2017, 1:18 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 19, 2017, 1:18 a.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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/5/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vadim Spector <vs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175543
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java
Line 36 (original), 40 (patched)
<https://reviews.apache.org/r/59317/#comment248946>

    Hmm ... how dit it end up reverting the fix?


- Vadim Spector


On May 19, 2017, 1:18 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 19, 2017, 1:18 a.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
> -------
> 
> 1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
> 2. Make sure that the update is not done for every connection towards sentry server.
> 3. Don't update the configuration object that we passed. Instead use a local version to update.
> 
> 
> Diffs
> -----
> 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
>   sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
>   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/5/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/
-----------------------------------------------------------

(Updated May 19, 2017, 1:18 a.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Made changes to avaoid issues that might happen when multiple threads try to call initialize() initially at the same time. one of the threads might return with out initializing UserGroupInformation.


Bugs: SENTRY-1736
    https://issues.apache.org/jira/browse/SENTRY-1736


Repository: sentry


Description
-------

1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
2. Make sure that the update is not done for every connection towards sentry server.
3. Don't update the configuration object that we passed. Instead use a local version to update.


Diffs (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
  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/5/

Changes: https://reviews.apache.org/r/59317/diff/4-5/


Testing
-------

Made sure that Solr client is able to talk to sentry with kerberos enabled.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/
-----------------------------------------------------------

(Updated May 18, 2017, 10:35 p.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Addressed review comments from Vadim. I'm working on a tests to test the new class


Bugs: SENTRY-1736
    https://issues.apache.org/jira/browse/SENTRY-1736


Repository: sentry


Description
-------

1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
2. Make sure that the update is not done for every connection towards sentry server.
3. Don't update the configuration object that we passed. Instead use a local version to update.


Diffs (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
  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/4/

Changes: https://reviews.apache.org/r/59317/diff/3-4/


Testing
-------

Made sure that Solr client is able to talk to sentry with kerberos enabled.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/
-----------------------------------------------------------

(Updated May 18, 2017, 8:57 p.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Description changed based on the new patch updated.


Bugs: SENTRY-1736
    https://issues.apache.org/jira/browse/SENTRY-1736


Repository: sentry


Description (updated)
-------

1. Update the UserGroupInformation with HADOOP_SECURITY_AUTHENTICATION for the client when kerberos is enabled.
2. Make sure that the update is not done for every connection towards sentry server.
3. Don't update the configuration object that we passed. Instead use a local version to update.


Diffs
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
  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/3/


Testing
-------

Made sure that Solr client is able to talk to sentry with kerberos enabled.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/
-----------------------------------------------------------

(Updated May 18, 2017, 8:49 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 (updated)
-----

  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java 9b9f9e8 
  sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java PRE-CREATION 
  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/3/

Changes: https://reviews.apache.org/r/59317/diff/2-3/


Testing
-------

Made sure that Solr client is able to talk to sentry with kerberos enabled.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175339
-----------------------------------------------------------


Ship it!




Ship It!

- Vamsee Yarlagadda


On May 18, 2017, 12:06 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 12:06 a.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/2/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

> On May 18, 2017, 12:37 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
> > Line 74 (original), 69 (patched)
> > <https://reviews.apache.org/r/59317/diff/2/?file=1723582#file1723582line76>
> >
> >     1) Why is it only done for Generic clients but not for other clients? What is the difference? I'd rather see the common solution that works for everyone.
> >     2) Since you are modifying config that you got from someone else, IMO you should get a private copy to use for modifications.

bq. 2) Since you are modifying config that you got from someone else, IMO you should get a private copy to use for modifications.
I suggested editing the supplied conf directly so that any further usages of that conf/ (by the caller) will reflect the right updated settings.


- Vamsee


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


On May 18, 2017, 12:06 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 12:06 a.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/2/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175341
-----------------------------------------------------------




sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryTransportFactory.java
Line 79 (original), 79 (patched)
<https://reviews.apache.org/r/59317/#comment248794>

    Why are you removing this line?
    The comment says that "Static information in UserGroupInformation should have been initialized by this time" - initialized by whom? Where? Do all components actually do what you describe here?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Line 74 (original), 69 (patched)
<https://reviews.apache.org/r/59317/#comment248795>

    1) Why is it only done for Generic clients but not for other clients? What is the difference? I'd rather see the common solution that works for everyone.
    2) Since you are modifying config that you got from someone else, IMO you should get a private copy to use for modifications.


- Alexander Kolbasov


On May 18, 2017, 12:06 a.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59317/
> -----------------------------------------------------------
> 
> (Updated May 18, 2017, 12:06 a.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/2/
> 
> 
> Testing
> -------
> 
> Made sure that Solr client is able to talk to sentry with kerberos enabled.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/
-----------------------------------------------------------

(Updated May 18, 2017, 12:06 a.m.)


Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Addressed review comments.


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 (updated)
-----

  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/2/

Changes: https://reviews.apache.org/r/59317/diff/1-2/


Testing
-------

Made sure that Solr client is able to talk to sentry with kerberos enabled.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

> On May 16, 2017, 6:41 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
> > Line 67 (original), 69 (patched)
> > <https://reviews.apache.org/r/59317/diff/1/?file=1721745#file1721745line69>
> >
> >     Why can't we change the supplied conf itself? That way if the clients (SentryShellSolr, SentryShellKafka) need to use their conf for something else, they will have all the updates.
> 
> kalyan kumar kalvagadda wrote:
>     If there are not updating HADOOP_SECURITY_AUTHENTICATION how would changing the supplied copy help. I'm not sure is there would any other issues if we do so.
>     
>     If you are sure it doesn't create any issues, will update the patch.
> 
> Vamsee Yarlagadda wrote:
>     What I am saying is as a client (SentrySehllSolr), they could be using that conf for other purposes too right? If they use the same conf everywhere they can get the advantage of changes being done to it.

I will update the code to use supplied conf.


- kalyan kumar


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


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
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.

> On May 16, 2017, 6:41 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
> > Line 67 (original), 69 (patched)
> > <https://reviews.apache.org/r/59317/diff/1/?file=1721745#file1721745line69>
> >
> >     Why can't we change the supplied conf itself? That way if the clients (SentryShellSolr, SentryShellKafka) need to use their conf for something else, they will have all the updates.
> 
> kalyan kumar kalvagadda wrote:
>     If there are not updating HADOOP_SECURITY_AUTHENTICATION how would changing the supplied copy help. I'm not sure is there would any other issues if we do so.
>     
>     If you are sure it doesn't create any issues, will update the patch.

What I am saying is as a client (SentrySehllSolr), they could be using that conf for other purposes too right? If they use the same conf everywhere they can get the advantage of changes being done to it.


- Vamsee


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


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
> 
>

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59317/#review175137
-----------------------------------------------------------




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
Line 67 (original), 69 (patched)
<https://reviews.apache.org/r/59317/#comment248511>

    Why can't we change the supplied conf itself? That way if the clients (SentryShellSolr, SentryShellKafka) need to use their conf for something else, they will have all the updates.


- Vamsee Yarlagadda


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
> 
>