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