You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Dapeng Sun <da...@intel.com> on 2015/01/14 10:43:20 UTC
Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/
-----------------------------------------------------------
Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Bugs: SENTRY-503
https://issues.apache.org/jira/browse/SENTRY-503
Repository: sentry
Description
-------
Integrate **AuthenticationFilter** for authentication
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithSimple.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
Diff: https://reviews.apache.org/r/29879/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Dapeng Sun <da...@intel.com>.
> On 一月 15, 2015, 1:31 p.m., Xiaomeng Huang wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java, line 53
> > <https://reviews.apache.org/r/29879/diff/2/?file=822373#file822373line53>
> >
> > looks like we don't need define a value "conf" in SentryWebServer?
Yes...but I think it's okay.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68204
-----------------------------------------------------------
On 一月 15, 2015, 11:06 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated 一月 15, 2015, 11:06 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Xiaomeng Huang <xi...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68204
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
<https://reviews.apache.org/r/29879/#comment112356>
looks like we don't need define a value "conf" in SentryWebServer?
- Xiaomeng Huang
On 一月 15, 2015, 3:06 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated 一月 15, 2015, 3:06 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by shen guoquan <gu...@intel.com>.
> On 一月 15, 2015, 6 a.m., shen guoquan wrote:
> > Ship It!
It looks good to me. It's a great work. dapeng.
- shen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68209
-----------------------------------------------------------
On 一月 15, 2015, 5:56 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated 一月 15, 2015, 5:56 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by shen guoquan <gu...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68209
-----------------------------------------------------------
Ship it!
Ship It!
- shen guoquan
On 一月 15, 2015, 5:56 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated 一月 15, 2015, 5:56 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/
-----------------------------------------------------------
(Updated 一月 20, 2015, 2:56 p.m.)
Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Add a comment about **AuthenticationFilter** to clarify the reason
Bugs: SENTRY-503
https://issues.apache.org/jira/browse/SENTRY-503
Repository: sentry
Description
-------
Integrate **AuthenticationFilter** for authentication
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
Diff: https://reviews.apache.org/r/29879/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Dapeng Sun <da...@intel.com>.
> On 一月 19, 2015, 3:29 p.m., Lenni Kuff wrote:
> > Chatted with the Hadoop team and while there are not guarantees that this this interface will not change, it is fairly stable and used by other projects (ie - Oozie).
Hi, Lenni, thank you very much for help to communicate with Hadoop team and confirm it.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68592
-----------------------------------------------------------
On 一月 15, 2015, 5 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated 一月 15, 2015, 5 p.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68592
-----------------------------------------------------------
Ship it!
Chatted with the Hadoop team and while there are not guarantees that this this interface will not change, it is fairly stable and used by other projects (ie - Oozie).
- Lenni Kuff
On Jan. 15, 2015, 9 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2015, 9 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/
-----------------------------------------------------------
(Updated 一月 15, 2015, 5 p.m.)
Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Bugs: SENTRY-503
https://issues.apache.org/jira/browse/SENTRY-503
Repository: sentry
Description
-------
Integrate **AuthenticationFilter** for authentication
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
Diff: https://reviews.apache.org/r/29879/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/
-----------------------------------------------------------
(Updated 一月 15, 2015, 1:56 p.m.)
Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Update patch according Xiaomeng's feedback
Bugs: SENTRY-503
https://issues.apache.org/jira/browse/SENTRY-503
Repository: sentry
Description
-------
Integrate **AuthenticationFilter** for authentication
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
Diff: https://reviews.apache.org/r/29879/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/
-----------------------------------------------------------
(Updated 一月 15, 2015, 11:06 a.m.)
Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Updated patch according Lenni's feedback.
Bugs: SENTRY-503
https://issues.apache.org/jira/browse/SENTRY-503
Repository: sentry
Description
-------
Integrate **AuthenticationFilter** for authentication
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
Diff: https://reviews.apache.org/r/29879/diff/
Testing
-------
Unit Tests passed
Thanks,
Dapeng Sun
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Dapeng Sun <da...@intel.com>.
> On 一月 15, 2015, 2:04 a.m., Lenni Kuff wrote:
> > Nice, I like how little code it took to add this support.
:) Thank you for your review.
> On 一月 15, 2015, 2:04 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java, line 116
> > <https://reviews.apache.org/r/29879/diff/1/?file=820782#file820782line116>
> >
> > Include the IOException, might help with debugging.
Good suggestion! I will fix it in next patch.
> On 一月 15, 2015, 2:04 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java, line 161
> > <https://reviews.apache.org/r/29879/diff/1/?file=820784#file820784line161>
> >
> > Use this when building the SECURITY_ENABLE config name?
Good suggestion! I will fix it in next patch.
> On 一月 15, 2015, 2:04 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java, line 162
> > <https://reviews.apache.org/r/29879/diff/1/?file=820784#file820784line162>
> >
> > Add comment on what the "type" config option is used for and the valid values. Consider removing the .enable field and instead allowing a "NONE" type.
Good suggestion! I will fix it in next patch.
> On 一月 15, 2015, 2:04 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java, line 163
> > <https://reviews.apache.org/r/29879/diff/1/?file=820784#file820784line163>
> >
> > What's the use case for having new config options for PRINCIPAL and KEYTAB? Seems easier to manage if we just use the existing sentry service principal and keytab.
We may can't use the server PRINCIPAL, SPNEGO can only use HTTP/HOSTNAME for web.
[KerberosAuthenticator](https://github.com/apache/hadoop/blob/f71eb51ab8109c14e8e921751dd5de603bdf2bde/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/client/KerberosAuthenticator.java#L272)
> On 一月 15, 2015, 2:04 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithSimple.java, line 38
> > <https://reviews.apache.org/r/29879/diff/1/?file=820786#file820786line38>
> >
> > Can you add a negative test that shows that the user is not able to connect to the webserver when they are not authenticated?
Good suggestion! I will fix it in next patch.
> On 一月 15, 2015, 2:04 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java, line 71
> > <https://reviews.apache.org/r/29879/diff/1/?file=820782#file820782line71>
> >
> > Note - AuthenticationFilter is marked as @InterfaceAudience.Private and @InterfaceStability.Unstable in hadoop-common. This might be okay, but let me follow up with the Hadoop team on their recommendation.
I'm agree with you, thank you for your communication with Hadoop team.
> On 一月 15, 2015, 2:04 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java, line 153
> > <https://reviews.apache.org/r/29879/diff/1/?file=820784#file820784line153>
> >
> > Is this the deafault port for Jenkins? If not, it would make things simpler to just keep the config default at 51000 and override it in whatever Jenkins environment is conflicting.
It may not the default port for Jenkins, will revert it in next patch.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68057
-----------------------------------------------------------
On 一月 14, 2015, 5:43 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated 一月 14, 2015, 5:43 p.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithSimple.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Lenni Kuff <ls...@cloudera.com>.
> On Jan. 14, 2015, 6:04 p.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java, line 71
> > <https://reviews.apache.org/r/29879/diff/1/?file=820782#file820782line71>
> >
> > Note - AuthenticationFilter is marked as @InterfaceAudience.Private and @InterfaceStability.Unstable in hadoop-common. This might be okay, but let me follow up with the Hadoop team on their recommendation.
>
> Dapeng Sun wrote:
> I'm agree with you, thank you for your communication with Hadoop team.
It would be good to call out that we are using Private/Unstable Hadoop APIs.
- Lenni
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68057
-----------------------------------------------------------
On Jan. 15, 2015, 9 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2015, 9 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithoutSecurity.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 29879: SENTRY-503: Add authentication support to
SentryWebserver
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29879/#review68057
-----------------------------------------------------------
Nice, I like how little code it took to add this support.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
<https://reviews.apache.org/r/29879/#comment112227>
Note - AuthenticationFilter is marked as @InterfaceAudience.Private and @InterfaceStability.Unstable in hadoop-common. This might be okay, but let me follow up with the Hadoop team on their recommendation.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java
<https://reviews.apache.org/r/29879/#comment112226>
Include the IOException, might help with debugging.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
<https://reviews.apache.org/r/29879/#comment112225>
Is this the deafault port for Jenkins? If not, it would make things simpler to just keep the config default at 51000 and override it in whatever Jenkins environment is conflicting.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
<https://reviews.apache.org/r/29879/#comment112221>
Use this when building the SECURITY_ENABLE config name?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
<https://reviews.apache.org/r/29879/#comment112222>
Add comment on what the "type" config option is used for and the valid values. Consider removing the .enable field and instead allowing a "NONE" type.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
<https://reviews.apache.org/r/29879/#comment112220>
What's the use case for having new config options for PRINCIPAL and KEYTAB? Seems easier to manage if we just use the existing sentry service principal and keytab.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithSimple.java
<https://reviews.apache.org/r/29879/#comment112224>
Can you add a negative test that shows that the user is not able to connect to the webserver when they are not authenticated?
- Lenni Kuff
On Jan. 14, 2015, 9:43 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29879/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2015, 9:43 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Arun Suresh, Colin Ma, shen guoquan, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-503
> https://issues.apache.org/jira/browse/SENTRY-503
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Integrate **AuthenticationFilter** for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java 090917c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 6d96565
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 47794bc
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryWebServerWithSimple.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 0bdc3a2
>
> Diff: https://reviews.apache.org/r/29879/diff/
>
>
> Testing
> -------
>
> Unit Tests passed
>
>
> Thanks,
>
> Dapeng Sun
>
>