From 69ba48e72740b80079972ee4323025d84facc0cc 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 | 35 +++++++++--- .../org/apache/hadoop/hbase/security/TestUser.java | 64 ++++++++++++++++++++-- 3 files changed, 89 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..80f330d 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(getShortName()); } 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..b6c37f9 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,37 @@ public class UserProvider extends BaseConfigurable { .concurrencyLevel(20) // create the loader // This just delegates to UGI. - .build(new CacheLoader() { + .build(new CacheLoader() { + + // Since UGI's don't hash based on the user id + // The cache needs to be keyed on the same thing that Hadoop's Groups class + // uses. So this cache uses shortname. + 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..b224764 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,15 +18,11 @@ */ 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; +import org.apache.commons.lang.SystemUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; @@ -34,16 +30,74 @@ 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 testCacheGetGroups() throws Exception { + Configuration conf = HBaseConfiguration.create(); + UserProvider up = UserProvider.instantiate(conf); + + // VERY unlikely that this user will exist on the box. + // This should mean the user has no groups. + String nonUser = "kklvfnvhdhcenfnniilggljhdecjhidkle"; + + // Create two UGI's for this username + UserGroupInformation ugiOne = UserGroupInformation.createRemoteUser(nonUser); + UserGroupInformation ugiTwo = UserGroupInformation.createRemoteUser(nonUser); + + // Now try and get the user twice. + User uOne = up.create(ugiOne); + User uTwo = up.create(ugiTwo); + + // Make sure that we didn't break groups and everything worked well. + assertArrayEquals(uOne.getGroupNames(),uTwo.getGroupNames()); + + // Check that they are referentially equal. + // Since getting a group for a users that doesn't exist creates a new string array + // the only way that they should be referentially equal is if the cache worked and + // made sure we didn't go to hadoop's script twice. + assertTrue(uOne.getGroupNames() == uTwo.getGroupNames()); + assertEquals(0, ugiOne.getGroupNames().length); + } + + @Test + public void testCacheGetGroupsRoot() throws Exception { + // Windows users don't have a root user. + // However pretty much every other *NIX os will have root. + if (!SystemUtils.IS_OS_WINDOWS) { + Configuration conf = HBaseConfiguration.create(); + UserProvider up = UserProvider.instantiate(conf); + + + String rootUserName = "root"; + + // Create two UGI's for this username + UserGroupInformation ugiOne = UserGroupInformation.createRemoteUser(rootUserName); + UserGroupInformation ugiTwo = UserGroupInformation.createRemoteUser(rootUserName); + + // Now try and get the user twice. + User uOne = up.create(ugiOne); + User uTwo = up.create(ugiTwo); + + // Make sure that we didn't break groups and everything worked well. + assertArrayEquals(uOne.getGroupNames(),uTwo.getGroupNames()); + String[] groupNames = ugiOne.getGroupNames(); + assertTrue(groupNames.length > 0); + } + } + + + @Test public void testBasicAttributes() throws Exception { Configuration conf = HBaseConfiguration.create(); User user = User.createUserForTesting(conf, "simple", new String[]{"foo"}); -- 2.6.1