hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anu Engineer (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-12291) Add support for nested groups in LdapGroupsMapping
Date Fri, 22 Apr 2016 18:09:13 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-12291?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15254349#comment-15254349
] 

Anu Engineer commented on HADOOP-12291:
---------------------------------------

[~ekundin] Thank you very much for providing this patch and taking care of most
jenkins issues in patch 2. I have some minor comments on Patch 2.


# Do we need -1 at all? In most cases it will not work and really depends on the
size of directory we are operating against. Since we know that it is not going to 
work or too slow in most cases, why support it ? My worry is that this will be used by 
some customer and will create very slow clusters. Can we please reduce this to  positive key
depth only  ? 
# what would be the impact of DIRECTORY_SEARCH_TIMEOUT
with a positive depth? Does it bail after the time out seconds or does it measure
timeout independently for each recursive query? if so, could you please define 
what is the right semantics here? 
# In {{LdapGroupsMapping.java:line 312}} : We add the groups to a list for all queries, but
this is needed if the goUpHierarchy is != 0. Would you please add an if check? This is just
to make sure that this code change does not change the memory usage if this feature is not
enabled.
# In {{LdapGroupsMapping#goUpGroupHierarchy}}
nitpick: can we please remove the reference to the JIRA number? "for HADOOP-12291", when we
commit this patch, we will refer to it. So it may not be needed in comments
# nitpick: do you want to rewrite this to be 
	{code}
    int nextLevel = 0;
    if (goUpHierarchy == -1) {
      nextLevel = -1;
    }
    else {
      nextLevel = goUpHierarchy -1;
    }
    {code}
into 
   {code}
   int nextLevel = (goUpHierarchy == -1) ? -1: goUpHierarchy -1;
   {code}
  Plus , Can you please define -1 as const like INFINITE_RECURSE = -1, so that code reading
is easier ? or better just remove this INIFITE_RECURSE capability completely from code ? 
# nitpick : would you like to pull this out as a function ? 
   {code}
  while (groupResults.hasMoreElements()) {
          SearchResult groupResult = groupResults.nextElement();
          Attribute groupName = groupResult.getAttributes().get(groupNameAttr);
          groups.add(groupName.get().toString());
          groupDNs.add(groupResult.getNameInNamespace());
        }
   {code}
# Do you think we should check for the goUpHierarchy == 0 before doing a LDAP query since
queries are generally expensive. I may be mistaken but I think you can optimize away one query
call if you check for the value little earlier.
# nitpick : Please feel free to ignore this. But we seem to be mixing StringBuilder.append
and String Concat. If we are using StringBuilder could we possible use appends all along instead
of creating an unnecessary string. I know that this is the style used in this file and you
are just following it, thought I would flag it for your consideration.
{code}
filter.append("(&" + groupSearchFilter + "(|");
{code}
# In TestLadpGroupMapping, Can you please use {{conf.setInt(LdapGroupsMapping.GROUP_HIERARCHY_LEVELS_KEY,1);}}
instead of {{conf.set(LdapGroupsMapping.GROUP_HIERARCHY_LEVELS_KEY, "1");}}
# In the next patch would you please take care of this last checkstyle warning:
./hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java:368:
   }:5: '}' should be on the same line.


> Add support for nested groups in LdapGroupsMapping
> --------------------------------------------------
>
>                 Key: HADOOP-12291
>                 URL: https://issues.apache.org/jira/browse/HADOOP-12291
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: security
>    Affects Versions: 2.8.0
>            Reporter: Gautam Gopalakrishnan
>            Assignee: Esther Kundin
>              Labels: features, patch
>             Fix For: 2.8.0
>
>         Attachments: HADOOP-12291.001.patch, HADOOP-12291.002.patch
>
>
> When using {{LdapGroupsMapping}} with Hadoop, nested groups are not supported. So for
example if user {{jdoe}} is part of group A which is a member of group B, the group mapping
currently returns only group A.
> Currently this facility is available with {{ShellBasedUnixGroupsMapping}} and SSSD (or
similar tools) but would be good to have this feature as part of {{LdapGroupsMapping}} directly.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message