Repository: hadoop Updated Branches: refs/heads/trunk 94d342e60 -> 53caeaa16
HADOOP-11402. Negative user-to-group cache entries are never cleared for never-again-accessed users. Contributed by Varun Saxena. Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/53caeaa1 Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/53caeaa1 Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/53caeaa1 Branch: refs/heads/trunk Commit: 53caeaa16b1450b54e994c77f5d0c8a767b88d57 Parents: 94d342e Author: Benoy Antony <be...@apache.org> Authored: Mon Jan 5 15:06:46 2015 -0800 Committer: Benoy Antony <be...@apache.org> Committed: Mon Jan 5 15:06:46 2015 -0800 ---------------------------------------------------------------------- .../java/org/apache/hadoop/security/Groups.java | 36 +++++++++------ .../hadoop/security/TestGroupsCaching.java | 48 ++++++++++++++++++++ 2 files changed, 71 insertions(+), 13 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/53caeaa1/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java index f3c5094..9fd39b0 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java @@ -23,12 +23,14 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; +import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Ticker; import com.google.common.cache.CacheBuilder; +import com.google.common.cache.Cache; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import org.apache.hadoop.HadoopIllegalArgumentException; @@ -60,14 +62,13 @@ public class Groups { private final GroupMappingServiceProvider impl; private final LoadingCache<String, List<String>> cache; - private final ConcurrentHashMap<String, Long> negativeCacheMask = - new ConcurrentHashMap<String, Long>(); private final Map<String, List<String>> staticUserToGroupsMap = new HashMap<String, List<String>>(); private final long cacheTimeout; private final long negativeCacheTimeout; private final long warningDeltaMs; private final Timer timer; + private Set<String> negativeCache; public Groups(Configuration conf) { this(conf, new Timer()); @@ -99,11 +100,24 @@ public class Groups { .expireAfterWrite(10 * cacheTimeout, TimeUnit.MILLISECONDS) .build(new GroupCacheLoader()); + if(negativeCacheTimeout > 0) { + Cache<String, Boolean> tempMap = CacheBuilder.newBuilder() + .expireAfterWrite(negativeCacheTimeout, TimeUnit.MILLISECONDS) + .ticker(new TimerToTickerAdapter(timer)) + .build(); + negativeCache = Collections.newSetFromMap(tempMap.asMap()); + } + if(LOG.isDebugEnabled()) LOG.debug("Group mapping impl=" + impl.getClass().getName() + "; cacheTimeout=" + cacheTimeout + "; warningDeltaMs=" + warningDeltaMs); } + + @VisibleForTesting + Set<String> getNegativeCache() { + return negativeCache; + } /* * Parse the hadoop.user.group.static.mapping.overrides configuration to @@ -159,13 +173,8 @@ public class Groups { // Check the negative cache first if (isNegativeCacheEnabled()) { - Long expirationTime = negativeCacheMask.get(user); - if (expirationTime != null) { - if (timer.monotonicNow() < expirationTime) { - throw noGroupsForUser(user); - } else { - negativeCacheMask.remove(user, expirationTime); - } + if (negativeCache.contains(user)) { + throw noGroupsForUser(user); } } @@ -212,8 +221,7 @@ public class Groups { if (groups.isEmpty()) { if (isNegativeCacheEnabled()) { - long expirationTime = timer.monotonicNow() + negativeCacheTimeout; - negativeCacheMask.put(user, expirationTime); + negativeCache.add(user); } // We throw here to prevent Cache from retaining an empty group @@ -252,7 +260,9 @@ public class Groups { LOG.warn("Error refreshing groups cache", e); } cache.invalidateAll(); - negativeCacheMask.clear(); + if(isNegativeCacheEnabled()) { + negativeCache.clear(); + } } /** http://git-wip-us.apache.org/repos/asf/hadoop/blob/53caeaa1/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestGroupsCaching.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestGroupsCaching.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestGroupsCaching.java index 89e5b2d..3686694 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestGroupsCaching.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestGroupsCaching.java @@ -454,5 +454,53 @@ public class TestGroupsCaching { } assertEquals(startingRequestCount + 1, FakeGroupMapping.getRequestCount()); + } + + @Test + public void testNegativeCacheEntriesExpire() throws Exception { + conf.setLong( + CommonConfigurationKeys.HADOOP_SECURITY_GROUPS_NEGATIVE_CACHE_SECS, 2); + FakeTimer timer = new FakeTimer(); + // Ensure that stale entries are removed from negative cache every 2 seconds + Groups groups = new Groups(conf, timer); + groups.cacheGroupsAdd(Arrays.asList(myGroups)); + groups.refresh(); + // Add both these users to blacklist so that they + // can be added to negative cache + FakeGroupMapping.addToBlackList("user1"); + FakeGroupMapping.addToBlackList("user2"); + + // Put user1 in negative cache. + try { + groups.getGroups("user1"); + fail("Did not throw IOException : Failed to obtain groups" + + " from FakeGroupMapping."); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("No groups found for user", e); + } + // Check if user1 exists in negative cache + assertTrue(groups.getNegativeCache().contains("user1")); + + // Advance fake timer + timer.advance(1000); + // Put user2 in negative cache + try { + groups.getGroups("user2"); + fail("Did not throw IOException : Failed to obtain groups" + + " from FakeGroupMapping."); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("No groups found for user", e); + } + // Check if user2 exists in negative cache + assertTrue(groups.getNegativeCache().contains("user2")); + + // Advance timer. Only user2 should be present in negative cache. + timer.advance(1100); + assertFalse(groups.getNegativeCache().contains("user1")); + assertTrue(groups.getNegativeCache().contains("user2")); + + // Advance timer. Even user2 should not be present in negative cache. + timer.advance(1000); + assertFalse(groups.getNegativeCache().contains("user2")); } }
