hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ecl...@apache.org
Subject hbase git commit: HBASE-14597 Fix Groups cache in multi-threaded env
Date Fri, 16 Oct 2015 22:21:16 GMT
Repository: hbase
Updated Branches:
  refs/heads/master 6076ed2a1 -> 0ed8b0e5d


HBASE-14597 Fix Groups cache in multi-threaded env


Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/0ed8b0e5
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/0ed8b0e5
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/0ed8b0e5

Branch: refs/heads/master
Commit: 0ed8b0e5d559e0499a85f8b1fbb3d3925a611688
Parents: 6076ed2
Author: Elliott Clark <eclark@apache.org>
Authored: Tue Oct 13 08:30:12 2015 -0700
Committer: Elliott Clark <eclark@apache.org>
Committed: Fri Oct 16 15:15:26 2015 -0700

----------------------------------------------------------------------
 .../org/apache/hadoop/hbase/security/User.java  | 44 +++++++++++-
 .../hadoop/hbase/security/UserProvider.java     | 40 ++++++++---
 .../apache/hadoop/hbase/security/TestUser.java  | 74 ++++++++++++++++++--
 3 files changed, 142 insertions(+), 16 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hbase/blob/0ed8b0e5/hbase-common/src/main/java/org/apache/hadoop/hbase/security/User.java
----------------------------------------------------------------------
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..c480dad 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
@@ -22,13 +22,18 @@ package org.apache.hadoop.hbase.security;
 import java.io.IOException;
 import java.security.PrivilegedAction;
 import java.security.PrivilegedExceptionAction;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
 import java.util.concurrent.ExecutionException;
 import com.google.common.cache.LoadingCache;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.classification.InterfaceAudience;
 import org.apache.hadoop.hbase.classification.InterfaceStability;
 import org.apache.hadoop.hbase.util.Methods;
+import org.apache.hadoop.security.Groups;
 import org.apache.hadoop.security.SecurityUtil;
 import org.apache.hadoop.security.UserGroupInformation;
 import org.apache.hadoop.security.token.Token;
@@ -255,7 +260,7 @@ public abstract class User {
   @InterfaceAudience.Private
    public static final class SecureHadoopUser extends User {
     private String shortName;
-    private LoadingCache<UserGroupInformation, String[]> cache;
+    private LoadingCache<String, String[]> cache;
 
     public SecureHadoopUser() throws IOException {
       ugi = UserGroupInformation.getCurrentUser();
@@ -268,7 +273,7 @@ public abstract class User {
     }
 
     public SecureHadoopUser(UserGroupInformation ugi,
-                            LoadingCache<UserGroupInformation, String[]> cache) {
+                            LoadingCache<String, String[]> cache) {
       this.ugi = ugi;
       this.cache = cache;
     }
@@ -289,7 +294,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];
         }
@@ -311,6 +316,13 @@ public abstract class User {
     /** @see User#createUserForTesting(org.apache.hadoop.conf.Configuration, String, String[])
*/
     public static User createUserForTesting(Configuration conf,
         String name, String[] groups) {
+      synchronized (UserProvider.class) {
+        if (!(UserProvider.groups instanceof TestingGroups)) {
+          UserProvider.groups = new TestingGroups(UserProvider.groups);
+        }
+      }
+
+      ((TestingGroups)UserProvider.groups).setUserGroups(name, groups);
       return new SecureHadoopUser(UserGroupInformation.createUserForTesting(name, groups));
     }
 
@@ -340,4 +352,30 @@ public abstract class User {
       return UserGroupInformation.isSecurityEnabled();
     }
   }
+
+  static class TestingGroups extends Groups {
+    private final Map<String, List<String>> userToGroupsMapping =
+        new HashMap<String,List<String>>();
+    private Groups underlyingImplementation;
+
+    TestingGroups(Groups underlyingImplementation) {
+      super(new Configuration());
+      this.underlyingImplementation = underlyingImplementation;
+    }
+
+    @Override
+    public List<String> getGroups(String user) throws IOException {
+      List<String> result = userToGroupsMapping.get(user);
+
+      if (result == null) {
+        result = underlyingImplementation.getGroups(user);
+      }
+
+      return result;
+    }
+
+    private void setUserGroups(String user, String[] groups) {
+      userToGroupsMapping.put(user, Arrays.asList(groups));
+    }
+  }
 }

http://git-wip-us.apache.org/repos/asf/hbase/blob/0ed8b0e5/hbase-common/src/main/java/org/apache/hadoop/hbase/security/UserProvider.java
----------------------------------------------------------------------
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..43b1c89 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,12 +51,20 @@ public class UserProvider extends BaseConfigurable {
           1,
           new ThreadFactoryBuilder().setDaemon(true).setNameFormat("group-cache-%d").build()));
 
-  private LoadingCache<UserGroupInformation, String[]> groupCache = null;
+  private LoadingCache<String, String[]> groupCache = null;
 
+  static Groups groups = Groups.getUserToGroupsMappingService();
 
   @Override
-  public void setConf(Configuration conf) {
+  public void setConf(final Configuration conf) {
     super.setConf(conf);
+
+    synchronized (UserProvider.class) {
+      if (!(groups instanceof User.TestingGroups)) {
+        groups = Groups.getUserToGroupsMappingService(conf);
+      }
+    }
+
     long cacheTimeout =
         getConf().getLong(CommonConfigurationKeys.HADOOP_SECURITY_GROUPS_CACHE_SECS,
             CommonConfigurationKeys.HADOOP_SECURITY_GROUPS_CACHE_SECS_DEFAULT) * 1000;
@@ -67,21 +78,34 @@ public class UserProvider extends BaseConfigurable {
         .concurrencyLevel(20)
             // create the loader
             // This just delegates to UGI.
-        .build(new CacheLoader<UserGroupInformation, String[]>() {
+        .build(new CacheLoader<String, String[]>() {
+
+          // 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.
           @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<String> result = new LinkedHashSet<String>(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<String[]> reload(final UserGroupInformation k,
+          public ListenableFuture<String[]> reload(final String k,
                                                    String[] oldValue) throws Exception {
 
             return executor.submit(new Callable<String[]>() {
-              UserGroupInformation userGroupInformation = k;
+
               @Override
               public String[] call() throws Exception {
-                return userGroupInformation.getGroupNames();
+                return getGroupStrings(k);
               }
             });
           }

http://git-wip-us.apache.org/repos/asf/hbase/blob/0ed8b0e5/hbase-server/src/test/java/org/apache/hadoop/hbase/security/TestUser.java
----------------------------------------------------------------------
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..709e796 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,84 @@ 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 testCreateUserForTestingGroupCache() throws Exception {
+    Configuration conf = HBaseConfiguration.create();
+    User uCreated = User.createUserForTesting(conf, "group_user", new String[] { "MYGROUP"
});
+    UserProvider up = UserProvider.instantiate(conf);
+    User uProvided = up.create(UserGroupInformation.createRemoteUser("group_user"));
+    assertArrayEquals(uCreated.getGroupNames(), uProvided.getGroupNames());
+
+  }
+
+  @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"});


Mime
View raw message