hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aihua Xu <...@cloudera.com>
Subject Re: Review Request 53204: HIVE-15076 Improve scalability of LDAP authentication provider group filter
Date Fri, 09 Dec 2016 19:58:15 GMT


> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 2426
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579539#file1579539line2426>
> >
> >     Just curious why we don't just put the constant string "hive.server2.authentication.ldap.userMembershipKey"
here like most of other entries?
> 
> Illya Yalovyy wrote:
>     Because it uses in several places. In particular in documentation. Putting a string
in documentation is not maintainable, because later someone can change the string and forget
to update in in all places. Documentation would become stale. In such a big project in will
be a problem. JavaDoc has means to prevent that from happening by using string constants in
documentation sections.

I got what you mean. But you can define as HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY("the
string", null) and then refer the string as HiveConf.ConfVars.HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY.varname.
Is that what you want?


> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, line
90
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line90>
> >
> >     This seems to be a useful info that will help in diagnostics. Wondering why
changes from info to debug level?
> 
> Illya Yalovyy wrote:
>     I totally agree, but Naveen doesn't want to expose group names in logs. It is a questionable
concern, but moving it to DEBUG may be a good compromise.

I see. That makes sense. Then maybe we can have both, so sensitive group only printed during
debug level and we will still see authetication succuss message during info level? How do
you think? 

LOG.debug("GroupMembershipKeyFilter passes: user '{}' is a member of '{}' group",...
LOG.info("Authentication succeeded based on group membership");


> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, line
115
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line115>
> >
> >     This should be info level which will be consistent with GroupMembershipKeyFilter
class.
> 
> Illya Yalovyy wrote:
>     Ok. I'll generate 2 log entries then: 1. INFO without group information; 2. DEBUG
with full information. 
>     
>     Does it make sense?
>     
>     See Naveen's comments for more details.
> 
> Illya Yalovyy wrote:
>     Actually this is a bit different. I'll change it to INFO.

Now I'm convinced for having 2 entries, 1. INFO without group information; 2. DEBUG with full
information. 

What do you mean it's different?


> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, line
124
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line124>
> >
> >     Seems 'warn' is not necessary since that could be expected in the for loop,
right?
> 
> Illya Yalovyy wrote:
>     It means we have a group in configuration that doesn't exist... Would you recommend
log it at DEBUG level?

I see. So we configure several groups in the configuration and it's possible that we configure
some incorrectly which are not in LDAP. The normal logic is to except no NamingException,
right? Actually based on that, "warn" is correct level. Sorry, I was misunderstanding the
logic. 

      for (String groupId : groupFilter) {
        try {
          String groupDn = ldap.findGroupDn(groupId);
          groupDns.add(groupDn);
        } catch (NamingException e) {
          LOG.debug("Cannot find DN for group " + groupId, e);
        }
      }


> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, line
132
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line132>
> >
> >     Since we are throwing the exception, I guess such debug may be redundant. We
should display such exception in the caller somewhere.
> 
> Illya Yalovyy wrote:
>     Exception message has a different (less descriptive) message. Please see Naveen's
comments for more details.

Got it.


> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, line
139
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line139>
> >
> >     Seems this could be a info level message.
> 
> Illya Yalovyy wrote:
>     Same here.

Same as previous comment. Maybe print info level without sensitive info and debug with?


> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, line
145
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line145>
> >
> >     You may need to change message since it's expected that the user is not in some
groups. Probably change to "Cannot match user ... and group ..." since "Failed to" seems to
be an error.
> 
> Illya Yalovyy wrote:
>     I will update the message.
> 
> Illya Yalovyy wrote:
>     Usually it should just return true or false. If it fails with exception then something
is wrong. That was reflected in the message. I noticed that I'm hiding the exception, which
is a very bad practice. Will fix it as well. May be even WARN log message with exception details
is required here. What you think? Again it should not happen usually, if it does - something
wrong.

Seems I misunderstood here again. yeah. You are right. WARN with details looks good to me.


> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java,
line 265
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579546#file1579546line265>
> >
> >     You may need to add some tests for the default configuraiton which is null for
HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY.
> 
> Illya Yalovyy wrote:
>     If HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY is NULL this filter will not be used.
I think we have enough test for this case. Did I get you correctly? Could you please provide
more details about the test case you have in mind?
> 
> Illya Yalovyy wrote:
>     I think this use case is tested in #testAuthenticateWhenGroupFilterPasses(). Probably
I should rename other tests to make it clear.

Looks good. :)


- Aihua


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


On Dec. 9, 2016, 1:03 a.m., Illya Yalovyy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53204/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2016, 1:03 a.m.)
> 
> 
> Review request for hive, Aihua Xu, Ashutosh Chauhan, Chaoyu Tang, and Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-15076 Improve scalability of LDAP authentication provider group filter
> 
> https://issues.apache.org/jira/browse/HIVE-15076
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 5ea9751 
>   service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 33b6088 
>   service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java 152c4b2

>   service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 65076ea 
>   service/src/java/org/apache/hive/service/auth/ldap/Query.java b8bf938 
>   service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java e9172d3 
>   service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java cd62935

>   service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
4fad755 
>   service/src/test/org/apache/hive/service/auth/ldap/LdapAuthenticationTestCase.java
acde8c1 
>   service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java 0cc2ead 
>   service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java 499b624 
>   service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java 3054e33 
>   service/src/test/resources/ldap/ad.example.com.ldif PRE-CREATION 
>   service/src/test/resources/ldap/microsoft.schema.ldif PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53204/diff/
> 
> 
> Testing
> -------
> 
> Build succeeded.
> 
> Test results:
> 
> Tests run: 149, Failures: 0, Errors: 0, Skipped: 0
> 
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 03:14 min
> [INFO] Finished at: 2016-10-26T13:53:15-07:00
> [INFO] Final Memory: 36M/1091M
> [INFO] ------------------------------------------------------------------------
> 
> 
> Thanks,
> 
> Illya Yalovyy
> 
>


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