You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2015/06/22 22:53:17 UTC
Review Request 35741: SENTRY-776: Sentry client should support cache
based kerberos ticket for secure zookeeper connection
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35741/
-----------------------------------------------------------
Review request for sentry, Dapeng Sun and Sravya Tirukkovalur.
Bugs: SENTRY-776
https://issues.apache.org/jira/browse/SENTRY-776
Repository: sentry
Description
-------
Currently the Sentry client requires principal/keytab for ZK connection. Given that it's a common client for various downstream project, we should also support ticket cache based connect to ZK. The patch supports both options for creating JAAS context for secure ZK connection. The method can be specified by a Sentry config property.
Also fixed the missing default for RPC address property resolving _HOST part of the kerberos principal.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 71935b1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java d5f55fe
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 54dbac5
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 1b9691e
Diff: https://reviews.apache.org/r/35741/diff/
Testing
-------
Thanks,
Prasad Mujumdar
Re: Review Request 35741: SENTRY-776: Sentry client should support
cache based kerberos ticket for secure zookeeper connection
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35741/#review89322
-----------------------------------------------------------
Ship it!
Ship It!
- Dapeng Sun
On 六月 25, 2015, 2:14 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35741/
> -----------------------------------------------------------
>
> (Updated 六月 25, 2015, 2:14 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-776
> https://issues.apache.org/jira/browse/SENTRY-776
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently the Sentry client requires principal/keytab for ZK connection. Given that it's a common client for various downstream project, we should also support ticket cache based connect to ZK. The patch supports both options for creating JAAS context for secure ZK connection. The method can be specified by a Sentry config property.
> Also fixed the missing default for RPC address property resolving _HOST part of the kerberos principal.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 71935b1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java d5f55fe
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 54dbac5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 1b9691e
>
> Diff: https://reviews.apache.org/r/35741/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 35741: SENTRY-776: Sentry client should support
cache based kerberos ticket for secure zookeeper connection
Posted by Prasad Mujumdar <pr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35741/
-----------------------------------------------------------
(Updated June 25, 2015, 6:14 a.m.)
Review request for sentry, Dapeng Sun and Sravya Tirukkovalur.
Changes
-------
Changes per review feedback
Bugs: SENTRY-776
https://issues.apache.org/jira/browse/SENTRY-776
Repository: sentry
Description
-------
Currently the Sentry client requires principal/keytab for ZK connection. Given that it's a common client for various downstream project, we should also support ticket cache based connect to ZK. The patch supports both options for creating JAAS context for secure ZK connection. The method can be specified by a Sentry config property.
Also fixed the missing default for RPC address property resolving _HOST part of the kerberos principal.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 71935b1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java d5f55fe
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 54dbac5
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 1b9691e
Diff: https://reviews.apache.org/r/35741/diff/
Testing
-------
Thanks,
Prasad Mujumdar
Re: Review Request 35741: SENTRY-776: Sentry client should support
cache based kerberos ticket for secure zookeeper connection
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On June 23, 2015, 10:52 a.m., Dapeng Sun wrote:
> > Hi Prasad, I'm agree with this change. But I have a question: Since sentry clients are always bound with component services, I'm curious which scene don't have keytab file.
I have seen this in my testing with Impala client. In general it's better to cover that case to make Sentry RPC client more generic.
> On June 23, 2015, 10:52 a.m., Dapeng Sun wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java, line 249
> > <https://reviews.apache.org/r/35741/diff/1/?file=989910#file989910line249>
> >
> > Nit: It may be better to add some Unit Tests for it, or open a ticket to track it.
Created SENTRY-782 to track new unit test case.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35741/#review88931
-----------------------------------------------------------
On June 22, 2015, 8:53 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35741/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 8:53 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-776
> https://issues.apache.org/jira/browse/SENTRY-776
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently the Sentry client requires principal/keytab for ZK connection. Given that it's a common client for various downstream project, we should also support ticket cache based connect to ZK. The patch supports both options for creating JAAS context for secure ZK connection. The method can be specified by a Sentry config property.
> Also fixed the missing default for RPC address property resolving _HOST part of the kerberos principal.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 71935b1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java d5f55fe
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 54dbac5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 1b9691e
>
> Diff: https://reviews.apache.org/r/35741/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 35741: SENTRY-776: Sentry client should support
cache based kerberos ticket for secure zookeeper connection
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35741/#review88931
-----------------------------------------------------------
Hi Prasad, I'm agree with this change. But I have a question: Since sentry clients are always bound with component services, I'm curious which scene don't have keytab file.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 249)
<https://reviews.apache.org/r/35741/#comment141556>
Nit: It may be better to add some Unit Tests for it, or open a ticket to track it.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java (line 328)
<https://reviews.apache.org/r/35741/#comment141547>
I think here should use **SENTRY_ZK_JAAS_NAME**: "SentryClient"
- Dapeng Sun
On 六月 23, 2015, 4:53 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35741/
> -----------------------------------------------------------
>
> (Updated 六月 23, 2015, 4:53 a.m.)
>
>
> Review request for sentry, Dapeng Sun and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-776
> https://issues.apache.org/jira/browse/SENTRY-776
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently the Sentry client requires principal/keytab for ZK connection. Given that it's a common client for various downstream project, we should also support ticket cache based connect to ZK. The patch supports both options for creating JAAS context for secure ZK connection. The method can be specified by a Sentry config property.
> Also fixed the missing default for RPC address property resolving _HOST part of the kerberos principal.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 71935b1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java d5f55fe
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 54dbac5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 1b9691e
>
> Diff: https://reviews.apache.org/r/35741/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 35741: SENTRY-776: Sentry client should support
cache based kerberos ticket for secure zookeeper connection
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35741/#review89259
-----------------------------------------------------------
Same concerns as Dapeng. Otherwise looks good.
- Sravya Tirukkovalur
On June 22, 2015, 8:53 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35741/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 8:53 p.m.)
>
>
> Review request for sentry, Dapeng Sun and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-776
> https://issues.apache.org/jira/browse/SENTRY-776
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently the Sentry client requires principal/keytab for ZK connection. Given that it's a common client for various downstream project, we should also support ticket cache based connect to ZK. The patch supports both options for creating JAAS context for secure ZK connection. The method can be specified by a Sentry config property.
> Also fixed the missing default for RPC address property resolving _HOST part of the kerberos principal.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 71935b1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java d5f55fe
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 54dbac5
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 1b9691e
>
> Diff: https://reviews.apache.org/r/35741/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Prasad Mujumdar
>
>