HADOOP-8088. User-group mapping cache incorrectly does negative caching on transient failures (Khiwal Lee via bobby)

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

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

@@ -384,6 +384,9 @@ Release 0.23.2 - UNRELEASED
     HADOOP-8176.  Disambiguate the destination of FsShell copies (Daryn Sharp
     via bobby)
 
+    HADOOP-8088. User-group mapping cache incorrectly does negative caching on
+    transient failures (Khiwal Lee via bobby)
+
 Release 0.23.1 - 2012-02-17 
 
   INCOMPATIBLE CHANGES

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

@@ -86,6 +86,9 @@ public List<String> getGroups(String user) throws IOException {
     
     // Create and cache user's groups
     groups = new CachedGroups(impl.getGroups(user));
+    if (groups.getGroups().isEmpty()) {
+      throw new IOException("No groups found for user " + user);
+    }
     userToGroupsMap.put(user, groups);
     if(LOG.isDebugEnabled()) {
       LOG.debug("Returning fetched groups for '" + user + "'");

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

@@ -0,0 +1,117 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.security;
+
+import java.io.IOException;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Set;
+
+import org.junit.Test;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeys;
+import org.apache.hadoop.security.Groups;
+import org.apache.hadoop.security.ShellBasedUnixGroupsMapping;
+
+
+public class TestGroupsCaching {
+  public static final Log LOG = LogFactory.getLog(TestGroupsCaching.class);
+  private static Configuration conf = new Configuration();
+  private static String[] myGroups = {"grp1", "grp2"};
+
+  static {
+    conf.setClass(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING,
+      FakeGroupMapping.class,
+      ShellBasedUnixGroupsMapping.class);
+  }
+
+  public static class FakeGroupMapping extends ShellBasedUnixGroupsMapping {
+    // any to n mapping
+    private static Set<String> allGroups = new HashSet<String>();
+    private static Set<String> blackList = new HashSet<String>();
+
+    public List<String> getGroups(String user) throws IOException {
+      LOG.info("Getting groups for " + user);
+      if (blackList.contains(user)) {
+        return new LinkedList<String>();
+      }
+      return new LinkedList<String>(allGroups);
+    }
+
+    public void cacheGroupsRefresh() throws IOException {
+      LOG.info("Cache is being refreshed.");
+      clearBlackList();
+      return;
+    }
+
+    public static void clearBlackList() throws IOException {
+      LOG.info("Clearing the blacklist");
+      blackList.clear();
+    }
+
+    public void cacheGroupsAdd(List<String> groups) throws IOException {
+      LOG.info("Adding " + groups + " to groups.");
+      allGroups.addAll(groups);
+    }
+
+    public static void addToBlackList(String user) throws IOException {
+      LOG.info("Adding " + user + " to the blacklist");
+      blackList.add(user);
+    }
+  }
+
+  @Test
+  public void TestGroupsCaching() throws Exception {
+    Groups groups = new Groups(conf);
+    groups.cacheGroupsAdd(Arrays.asList(myGroups));
+    groups.refresh();
+    FakeGroupMapping.clearBlackList();
+    FakeGroupMapping.addToBlackList("user1");
+
+    // regular entry
+    assertTrue(groups.getGroups("me").size() == 2);
+
+    // this must be cached. blacklisting should have no effect.
+    FakeGroupMapping.addToBlackList("me");
+    assertTrue(groups.getGroups("me").size() == 2);
+
+    // ask for a negative entry
+    try {
+      LOG.error("We are not supposed to get here." + groups.getGroups("user1").toString());
+      fail();
+    } catch (IOException ioe) {
+      if(!ioe.getMessage().startsWith("No groups found")) {
+        LOG.error("Got unexpected exception: " + ioe.getMessage());
+        fail();
+      }
+    }
+
+    // this shouldn't be cached. remove from the black list and retry.
+    FakeGroupMapping.clearBlackList();
+    assertTrue(groups.getGroups("user1").size() == 2);
+  }
+}