From c427540ced4b9ac751f9ba89cba64b3e243fe4f2 Mon Sep 17 00:00:00 2001 From: Aaron Myers Date: Wed, 27 Mar 2013 21:52:52 +0000 Subject: [PATCH] HADOOP-9125. LdapGroupsMapping threw CommunicationException after some idle time. Contributed by Kai Zheng. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-2@1461864 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-common/CHANGES.txt | 3 + .../hadoop/security/LdapGroupsMapping.java | 84 +++++++++++++------ .../security/TestLdapGroupsMapping.java | 59 +++++++++---- 3 files changed, 105 insertions(+), 41 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index c1526be418f..c7bc33dfbe1 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -95,6 +95,9 @@ Release 2.0.5-beta - UNRELEASED HADOOP-9430. TestSSLFactory fails on IBM JVM. (Amir Sanjar via suresh) + HADOOP-9125. LdapGroupsMapping threw CommunicationException after some + idle time. (Kai Zheng via atm) + Release 2.0.4-alpha - UNRELEASED INCOMPATIBLE CHANGES diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java index bcccb198ebc..eda711547c3 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java @@ -24,6 +24,7 @@ import java.util.ArrayList; import java.util.Hashtable; import java.util.List; +import javax.naming.CommunicationException; import javax.naming.Context; import javax.naming.NamingEnumeration; import javax.naming.NamingException; @@ -166,6 +167,8 @@ public class LdapGroupsMapping private String groupMemberAttr; private String groupNameAttr; + public static int RECONNECT_RETRY_COUNT = 3; + /** * Returns list of groups for a user. * @@ -178,34 +181,63 @@ public class LdapGroupsMapping */ @Override public synchronized List getGroups(String user) throws IOException { - List groups = new ArrayList(); - + List emptyResults = new ArrayList(); + /* + * Normal garbage collection takes care of removing Context instances when they are no longer in use. + * Connections used by Context instances being garbage collected will be closed automatically. + * So in case connection is closed and gets CommunicationException, retry some times with new new DirContext/connection. + */ try { - DirContext ctx = getDirContext(); - - // Search for the user. We'll only ever need to look at the first result - NamingEnumeration results = ctx.search(baseDN, - userSearchFilter, - new Object[]{user}, - SEARCH_CONTROLS); - if (results.hasMoreElements()) { - SearchResult result = results.nextElement(); - String userDn = result.getNameInNamespace(); - - NamingEnumeration groupResults = - ctx.search(baseDN, - "(&" + groupSearchFilter + "(" + groupMemberAttr + "={0}))", - new Object[]{userDn}, - SEARCH_CONTROLS); - while (groupResults.hasMoreElements()) { - SearchResult groupResult = groupResults.nextElement(); - Attribute groupName = groupResult.getAttributes().get(groupNameAttr); - groups.add(groupName.get().toString()); - } - } + return doGetGroups(user); + } catch (CommunicationException e) { + LOG.warn("Connection is closed, will try to reconnect"); } catch (NamingException e) { LOG.warn("Exception trying to get groups for user " + user, e); - return new ArrayList(); + return emptyResults; + } + + int retryCount = 0; + while (retryCount ++ < RECONNECT_RETRY_COUNT) { + //reset ctx so that new DirContext can be created with new connection + this.ctx = null; + + try { + return doGetGroups(user); + } catch (CommunicationException e) { + LOG.warn("Connection being closed, reconnecting failed, retryCount = " + retryCount); + } catch (NamingException e) { + LOG.warn("Exception trying to get groups for user " + user, e); + return emptyResults; + } + } + + return emptyResults; + } + + List doGetGroups(String user) throws NamingException { + List groups = new ArrayList(); + + DirContext ctx = getDirContext(); + + // Search for the user. We'll only ever need to look at the first result + NamingEnumeration results = ctx.search(baseDN, + userSearchFilter, + new Object[]{user}, + SEARCH_CONTROLS); + if (results.hasMoreElements()) { + SearchResult result = results.nextElement(); + String userDn = result.getNameInNamespace(); + + NamingEnumeration groupResults = + ctx.search(baseDN, + "(&" + groupSearchFilter + "(" + groupMemberAttr + "={0}))", + new Object[]{userDn}, + SEARCH_CONTROLS); + while (groupResults.hasMoreElements()) { + SearchResult groupResult = groupResults.nextElement(); + Attribute groupName = groupResult.getAttributes().get(groupNameAttr); + groups.add(groupName.get().toString()); + } } return groups; @@ -236,7 +268,7 @@ public class LdapGroupsMapping return ctx; } - + /** * Caches groups, no need to do that for this provider */ diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java index 19ac7b68211..331e288e8ba 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java @@ -26,6 +26,7 @@ import java.io.Writer; import java.util.Arrays; import java.util.List; +import javax.naming.CommunicationException; import javax.naming.NamingEnumeration; import javax.naming.NamingException; import javax.naming.directory.Attribute; @@ -46,21 +47,15 @@ public class TestLdapGroupsMapping { private DirContext mockContext; private LdapGroupsMapping mappingSpy = spy(new LdapGroupsMapping()); + private NamingEnumeration mockUserNamingEnum = mock(NamingEnumeration.class); + private NamingEnumeration mockGroupNamingEnum = mock(NamingEnumeration.class); + private String[] testGroups = new String[] {"group1", "group2"}; @Before public void setupMocks() throws NamingException { mockContext = mock(DirContext.class); doReturn(mockContext).when(mappingSpy).getDirContext(); - - NamingEnumeration mockUserNamingEnum = mock(NamingEnumeration.class); - NamingEnumeration mockGroupNamingEnum = mock(NamingEnumeration.class); - - // The search functionality of the mock context is reused, so we will - // return the user NamingEnumeration first, and then the group - when(mockContext.search(anyString(), anyString(), any(Object[].class), - any(SearchControls.class))) - .thenReturn(mockUserNamingEnum, mockGroupNamingEnum); - + SearchResult mockUserResult = mock(SearchResult.class); // We only ever call hasMoreElements once for the user NamingEnum, so // we can just have one return value @@ -76,23 +71,57 @@ public class TestLdapGroupsMapping { // Define the attribute for the name of the first group Attribute group1Attr = new BasicAttribute("cn"); - group1Attr.add("group1"); + group1Attr.add(testGroups[0]); Attributes group1Attrs = new BasicAttributes(); group1Attrs.put(group1Attr); // Define the attribute for the name of the second group Attribute group2Attr = new BasicAttribute("cn"); - group2Attr.add("group2"); + group2Attr.add(testGroups[1]); Attributes group2Attrs = new BasicAttributes(); group2Attrs.put(group2Attr); // This search result gets reused, so return group1, then group2 when(mockGroupResult.getAttributes()).thenReturn(group1Attrs, group2Attrs); - } @Test public void testGetGroups() throws IOException, NamingException { + // The search functionality of the mock context is reused, so we will + // return the user NamingEnumeration first, and then the group + when(mockContext.search(anyString(), anyString(), any(Object[].class), + any(SearchControls.class))) + .thenReturn(mockUserNamingEnum, mockGroupNamingEnum); + + doTestGetGroups(Arrays.asList(testGroups), 2); + } + + @Test + public void testGetGroupsWithConnectionClosed() throws IOException, NamingException { + // The case mocks connection is closed/gc-ed, so the first search call throws CommunicationException, + // then after reconnected return the user NamingEnumeration first, and then the group + when(mockContext.search(anyString(), anyString(), any(Object[].class), + any(SearchControls.class))) + .thenThrow(new CommunicationException("Connection is closed")) + .thenReturn(mockUserNamingEnum, mockGroupNamingEnum); + + // Although connection is down but after reconnected it still should retrieve the result groups + doTestGetGroups(Arrays.asList(testGroups), 1 + 2); // 1 is the first failure call + } + + @Test + public void testGetGroupsWithLdapDown() throws IOException, NamingException { + // This mocks the case where Ldap server is down, and always throws CommunicationException + when(mockContext.search(anyString(), anyString(), any(Object[].class), + any(SearchControls.class))) + .thenThrow(new CommunicationException("Connection is closed")); + + // Ldap server is down, no groups should be retrieved + doTestGetGroups(Arrays.asList(new String[] {}), + 1 + LdapGroupsMapping.RECONNECT_RETRY_COUNT); // 1 is the first normal call + } + + private void doTestGetGroups(List expectedGroups, int searchTimes) throws IOException, NamingException { Configuration conf = new Configuration(); // Set this, so we don't throw an exception conf.set(LdapGroupsMapping.LDAP_URL_KEY, "ldap://test"); @@ -102,10 +131,10 @@ public class TestLdapGroupsMapping { // regardless of input List groups = mappingSpy.getGroups("some_user"); - Assert.assertEquals(Arrays.asList("group1", "group2"), groups); + Assert.assertEquals(expectedGroups, groups); // We should have searched for a user, and then two groups - verify(mockContext, times(2)).search(anyString(), + verify(mockContext, times(searchTimes)).search(anyString(), anyString(), any(Object[].class), any(SearchControls.class));