hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Illya Yalovyy <yalov...@amazon.com>
Subject Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests
Date Tue, 20 Sep 2016 17:32:43 GMT


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, line
37
> > <https://reviews.apache.org/r/51694/diff/1/?file=1492945#file1492945line37>
> >
> >     Do we really need an extra factory layer and have a factory for each filter?
> >     In Hive, actaully each session instantiates its own LdapAuthenticationProviderImpl,
which now contains different factories with each one generating only one instance of its filter.

I think so. Factories encapsulate logic related to choosing whether particular filter is required
or not based on the provided configuration.


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java, line 108
> > <https://reviews.apache.org/r/51694/diff/1/?file=1492946#file1492946line108>
> >
> >     can getSingleLdapName be used to enforce only one returned entry? that API in
SearachResultHandler is never used.

I was trying to copy existing logic. At the moment, I don't want to do any changes to that.
This particular code should be improved in separate CR. Does it make sense?


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java, line 105
> > <https://reviews.apache.org/r/51694/diff/1/?file=1492948#file1492948line105>
> >
> >     This method might throw out runtime exception such as NPE, IndexOutOfBoundsException,
should we check the passed in parameter rdn? 
> >     We might not run into this situation in old code, but since this line of code
is refactored as a separate API, I think we should do the check. Same for the other methods
like patternToBaseDn etc.

Agree. The DN parsing in general implemented quite poorly. I have a task already to re-implement
it completely. There are many problems with current one. Handling incorrect format or invalid
input is only one of them. My intention is to use RDN java class to do a correct parsing.


https://docs.oracle.com/javase/7/docs/api/javax/naming/ldap/Rdn.html

I think we can leave it for now, and I will submit another CR that addresses this concern
sortly.


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java, line 159
> > <https://reviews.apache.org/r/51694/diff/1/?file=1492948#file1492948line159>
> >
> >     I am not sure if there is any precedence for these configurations, but here
it seems that the GUIDKEY/BASEDN takes precedence over DNPATTERN, which is different from
the existing implementation and cause the behavior change.

Could you please give more details on the case when the behavior will be different. 
The logic seems to be same: It uses GUIDKEY/BASEDN only when DNPATTERN is not configured:

if (StringUtils.isBlank(patternsString)) {
...

Which means *only* if patternsString is blank, try to use GUIDKEY/BASEDN.

Please let me know if I did not get it correctly.


> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/Query.java, line 122
> > <https://reviews.apache.org/r/51694/diff/1/?file=1492949#file1492949line122>
> >
> >     Will it improve the performance to set the search limit? I did not see it is
used.

I will be used for different filters. Do you think we should use it for existing filters?
Which one in particular? Or you would prefer me to remove this option?

Please keep in mind that this CR is not about performance.


- Illya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51694/#review148634
-----------------------------------------------------------


On Sept. 7, 2016, 2:24 p.m., Illya Yalovyy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2016, 2:24 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Currently LdapAuthenticationProviderImpl class is not covered with unit tests. To make
this class testable some minor refactoring will be required.
> 
> 
> Diffs
> -----
> 
>   service/pom.xml ecea719 
>   service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java efd5393

>   service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java PRE-CREATION

>   service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java PRE-CREATION

>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java PRE-CREATION

>   service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java PRE-CREATION

>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java PRE-CREATION

>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java PRE-CREATION

>   service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java PRE-CREATION

>   service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java PRE-CREATION

>   service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java 089a059

>   service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
f276906 
>   service/src/test/org/apache/hive/service/auth/ldap/Credentials.java PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java PRE-CREATION 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java PRE-CREATION

>   service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/51694/diff/
> 
> 
> Testing
> -------
> 
> ...hive/service> mvn clean test
> 
> ...
> 
> Results :
> 
> Tests run: 123, Failures: 0, Errors: 0, Skipped: 0
> 
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 04:18 min
> [INFO] Finished at: 2016-09-06T08:46:04-07:00
> [INFO] Final Memory: 66M/984M
> [INFO] ------------------------------------------------------------------------
> 
> 
> Thanks,
> 
> Illya Yalovyy
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message