You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Chaoyu Tang <ct...@gmail.com> on 2016/06/16 02:33:02 UTC
Review Request 48771: HIVE-13590: Kerberized HS2 with LDAP auth
enabled fails in multi-domain LDAP case
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48771/
-----------------------------------------------------------
Review request for hive.
Bugs: HIVE-13590
https://issues.apache.org/jira/browse/HIVE-13590
Repository: hive-git
Description
-------
Hive should not use Hadoop security (e.g. kerberos) related APIs such as KerberosName etc to process user logged in via other SASL mechanism such as LDAP.
Diffs
-----
itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcNonKrbSASLWithMiniKdc.java 1c1beda
service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java ab8806c
service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 8bc3d94
shims/common/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java 8a4786c
Diff: https://reviews.apache.org/r/48771/diff/
Testing
-------
Manual test
PreCommit test
Thanks,
Chaoyu Tang
Re: Review Request 48771: HIVE-13590: Kerberized HS2 with LDAP auth
enabled fails in multi-domain LDAP case
Posted by Chaoyu Tang <ct...@gmail.com>.
> On June 16, 2016, 3:01 p.m., Sergio Pena wrote:
> > The code looks good for me.
> >
> > What other tests did you do to validate this code?
> > - HS2 with KERBEROS + LDAP?
> > - HS2 with KERBEROS only?
> > - HS2 with LDAP only?
The unit tests cover the cases:
1. HS2 with kerberos + SASL non-kerberos authentication. I used the CUSTOM authentication since Hive does not have LDAP end-to-end unit test framework and CUSTOM authentication shares the same code path with LDAP except the detail how LDAP does its authentication, which is not the scope of this JIRA. So in term of this JIRA, testing with LDAP and CUSTOM authentication is equalivent and actually LDAP is a specially case of CUSTOM.
2. HS2 with kerberos only
3. HS2 with only SASL non-kerberos authentication (CUSTOM) for same reason as item 1.
> On June 16, 2016, 3:01 p.m., Sergio Pena wrote:
> > service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java, line 231
> > <https://reviews.apache.org/r/48771/diff/1/?file=1420643#file1420643line231>
> >
> > Should we return empty strings instead of nulls? That way we avoid that any other future developer uses this method without validating a null pointer, that could cause NPE if they do not do that.
Thanks Sergio for review. yeah, it is debatable which way is better, returning null or empty object (e.g. String), please see http://stackoverflow.com/questions/1626597/should-functions-return-null-or-an-empty-object
Thinking over, I tend to still return null for this API because
1. There is not an "empty" authentication mechanism. It either exists like TOKEN, KERBEROS, or does not exist (which null will be more informative)
2. Be consistent with other APIs like getRemoteUser(), getIpAddress()
I wonder if it makes sense. Thanks
- Chaoyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48771/#review137988
-----------------------------------------------------------
On June 16, 2016, 2:33 a.m., Chaoyu Tang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48771/
> -----------------------------------------------------------
>
> (Updated June 16, 2016, 2:33 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-13590
> https://issues.apache.org/jira/browse/HIVE-13590
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Hive should not use Hadoop security (e.g. kerberos) related APIs such as KerberosName etc to process user logged in via other SASL mechanism such as LDAP.
>
>
> Diffs
> -----
>
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcNonKrbSASLWithMiniKdc.java 1c1beda
> service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java ab8806c
> service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 8bc3d94
> shims/common/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java 8a4786c
>
> Diff: https://reviews.apache.org/r/48771/diff/
>
>
> Testing
> -------
>
> Manual test
> PreCommit test
>
>
> Thanks,
>
> Chaoyu Tang
>
>
Re: Review Request 48771: HIVE-13590: Kerberized HS2 with LDAP auth
enabled fails in multi-domain LDAP case
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48771/#review137988
-----------------------------------------------------------
Fix it, then Ship it!
The code looks good for me.
What other tests did you do to validate this code?
- HS2 with KERBEROS + LDAP?
- HS2 with KERBEROS only?
- HS2 with LDAP only?
service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java (line 231)
<https://reviews.apache.org/r/48771/#comment203196>
Should we return empty strings instead of nulls? That way we avoid that any other future developer uses this method without validating a null pointer, that could cause NPE if they do not do that.
- Sergio Pena
On June 16, 2016, 2:33 a.m., Chaoyu Tang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48771/
> -----------------------------------------------------------
>
> (Updated June 16, 2016, 2:33 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-13590
> https://issues.apache.org/jira/browse/HIVE-13590
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Hive should not use Hadoop security (e.g. kerberos) related APIs such as KerberosName etc to process user logged in via other SASL mechanism such as LDAP.
>
>
> Diffs
> -----
>
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcNonKrbSASLWithMiniKdc.java 1c1beda
> service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java ab8806c
> service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 8bc3d94
> shims/common/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java 8a4786c
>
> Diff: https://reviews.apache.org/r/48771/diff/
>
>
> Testing
> -------
>
> Manual test
> PreCommit test
>
>
> Thanks,
>
> Chaoyu Tang
>
>
Re: Review Request 48771: HIVE-13590: Kerberized HS2 with LDAP auth
enabled fails in multi-domain LDAP case
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48771/#review138670
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On June 16, 2016, 2:33 a.m., Chaoyu Tang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48771/
> -----------------------------------------------------------
>
> (Updated June 16, 2016, 2:33 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-13590
> https://issues.apache.org/jira/browse/HIVE-13590
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Hive should not use Hadoop security (e.g. kerberos) related APIs such as KerberosName etc to process user logged in via other SASL mechanism such as LDAP.
>
>
> Diffs
> -----
>
> itests/hive-minikdc/src/test/java/org/apache/hive/minikdc/TestJdbcNonKrbSASLWithMiniKdc.java 1c1beda
> service/src/java/org/apache/hive/service/auth/HiveAuthFactory.java ab8806c
> service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 8bc3d94
> shims/common/src/main/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java 8a4786c
>
> Diff: https://reviews.apache.org/r/48771/diff/
>
>
> Testing
> -------
>
> Manual test
> PreCommit test
>
>
> Thanks,
>
> Chaoyu Tang
>
>