HADOOP-9125. LdapGroupsMapping threw CommunicationException after some idle time. Contributed by Kai Zheng.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1461863 13f79535-47bb-0310-9956-ffa450edef68

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -598,6 +598,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

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java

@@ -24,6 +24,7 @@
 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,9 +181,42 @@ public class LdapGroupsMapping
    */
   @Override
   public synchronized List<String> getGroups(String user) throws IOException {
-    List<String> groups = new ArrayList<String>();
+    List<String> emptyResults = new ArrayList<String>();
+    /*
+     * 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 {
+      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 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<String> doGetGroups(String user) throws NamingException {
+    List<String> groups = new ArrayList<String>();
+
     DirContext ctx = getDirContext();
 
     // Search for the user. We'll only ever need to look at the first result
@@ -203,10 +239,6 @@ public synchronized List<String> getGroups(String user) throws IOException {
         groups.add(groupName.get().toString());
       }
     }
-    } catch (NamingException e) {
-      LOG.warn("Exception trying to get groups for user " + user, e);
-      return new ArrayList<String>();
-    }
 
     return groups;
   }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java

@@ -26,6 +26,7 @@
 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 void setupMocks() throws NamingException {
     
     // 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<String> 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 void testGetGroups() throws IOException, NamingException {
     // regardless of input
     List<String> 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));