From a52dd427f15d3440deef2054ab4c74e60987cf8e Mon Sep 17 00:00:00 2001 From: Elliott Clark Date: Tue, 13 Oct 2015 08:30:12 -0700 Subject: [PATCH] HBASE-14597 Fix Groups cache in multi-threaded env --- .../org/apache/hadoop/hbase/security/User.java | 6 ++--- .../apache/hadoop/hbase/security/UserProvider.java | 30 ++++++++++++++++------ .../org/apache/hadoop/hbase/security/TestUser.java | 18 +++++++++---- 3 files changed, 38 insertions(+), 16 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/User.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/User.java index 93ce003..cfa2f04 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/User.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/User.java @@ -255,7 +255,7 @@ public abstract class User { @InterfaceAudience.Private public static final class SecureHadoopUser extends User { private String shortName; - private LoadingCache cache; + private LoadingCache cache; public SecureHadoopUser() throws IOException { ugi = UserGroupInformation.getCurrentUser(); @@ -268,7 +268,7 @@ public abstract class User { } public SecureHadoopUser(UserGroupInformation ugi, - LoadingCache cache) { + LoadingCache cache) { this.ugi = ugi; this.cache = cache; } @@ -289,7 +289,7 @@ public abstract class User { public String[] getGroupNames() { if (cache != null) { try { - return this.cache.get(ugi); + return this.cache.get(ugi.getShortUserName()); } catch (ExecutionException e) { return new String[0]; } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/UserProvider.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/UserProvider.java index 8ffa753..5717563 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/UserProvider.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/UserProvider.java @@ -18,6 +18,8 @@ package org.apache.hadoop.hbase.security; import java.io.IOException; +import java.util.LinkedHashSet; +import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; @@ -33,6 +35,7 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.CommonConfigurationKeys; import org.apache.hadoop.hbase.BaseConfigurable; import org.apache.hadoop.hbase.classification.InterfaceAudience; +import org.apache.hadoop.security.Groups; import org.apache.hadoop.security.UserGroupInformation; import org.apache.hadoop.util.ReflectionUtils; @@ -48,11 +51,11 @@ public class UserProvider extends BaseConfigurable { 1, new ThreadFactoryBuilder().setDaemon(true).setNameFormat("group-cache-%d").build())); - private LoadingCache groupCache = null; + private LoadingCache groupCache = null; @Override - public void setConf(Configuration conf) { + public void setConf(final Configuration conf) { super.setConf(conf); long cacheTimeout = getConf().getLong(CommonConfigurationKeys.HADOOP_SECURITY_GROUPS_CACHE_SECS, @@ -67,21 +70,32 @@ public class UserProvider extends BaseConfigurable { .concurrencyLevel(20) // create the loader // This just delegates to UGI. - .build(new CacheLoader() { + .build(new CacheLoader() { + Groups groups = Groups.getUserToGroupsMappingService(conf); @Override - public String[] load(UserGroupInformation ugi) throws Exception { - return ugi.getGroupNames(); + public String[] load(String ugi) throws Exception { + return getGroupStrings(ugi); + } + + private String[] getGroupStrings(String ugi) { + try { + Set result = new LinkedHashSet + (groups.getGroups(ugi)); + return result.toArray(new String[result.size()]); + } catch (Exception e) { + return new String[0]; + } } // Provide the reload function that uses the executor thread. - public ListenableFuture reload(final UserGroupInformation k, + public ListenableFuture reload(final String k, String[] oldValue) throws Exception { return executor.submit(new Callable() { - UserGroupInformation userGroupInformation = k; + @Override public String[] call() throws Exception { - return userGroupInformation.getGroupNames(); + return getGroupStrings(k); } }); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/TestUser.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/TestUser.java index 02b7f9a..0d65e31 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/TestUser.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/TestUser.java @@ -18,11 +18,6 @@ */ package org.apache.hadoop.hbase.security; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; - import java.io.IOException; import java.security.PrivilegedAction; import java.security.PrivilegedExceptionAction; @@ -34,16 +29,29 @@ import org.apache.hadoop.fs.CommonConfigurationKeys; import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.testclassification.SecurityTests; import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.apache.hadoop.security.UserGroupInformation; import org.junit.Test; import org.junit.experimental.categories.Category; import com.google.common.collect.ImmutableSet; +import static org.junit.Assert.*; + @Category({SecurityTests.class, SmallTests.class}) public class TestUser { private static final Log LOG = LogFactory.getLog(TestUser.class); @Test + public void testGetGroups() throws Exception { + Configuration conf = HBaseConfiguration.create(); + UserProvider up = UserProvider.instantiate(conf); + UserGroupInformation ugiOne = UserGroupInformation.createRemoteUser("notonthisbox"); + UserGroupInformation ugiTwo = UserGroupInformation.createRemoteUser("notonthisbox"); + User uOne = up.create(ugiOne); + User uTwo = up.create(ugiTwo); + assertArrayEquals(uOne.getGroupNames(),uTwo.getGroupNames()); + } + @Test public void testBasicAttributes() throws Exception { Configuration conf = HBaseConfiguration.create(); User user = User.createUserForTesting(conf, "simple", new String[]{"foo"}); -- 2.6.1