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