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