diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java index 88fe3eba458..d79419b0263 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java @@ -26,7 +26,6 @@ import java.net.URI; import java.net.URISyntaxException; import java.security.PrivilegedExceptionAction; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.EnumSet; import java.util.HashMap; @@ -2222,12 +2221,11 @@ public abstract class FileSystem extends Configured implements Closeable { FsPermission perm = stat.getPermission(); UserGroupInformation ugi = UserGroupInformation.getCurrentUser(); String user = ugi.getShortUserName(); - List groups = Arrays.asList(ugi.getGroupNames()); if (user.equals(stat.getOwner())) { if (perm.getUserAction().implies(mode)) { return; } - } else if (groups.contains(stat.getGroup())) { + } else if (ugi.getGroups().contains(stat.getGroup())) { if (perm.getGroupAction().implies(mode)) { return; } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java index 5f42e705bcf..37893d7ab58 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java @@ -862,7 +862,7 @@ public class ViewFileSystem extends FileSystem { public FileStatus getFileStatus(Path f) throws IOException { checkPathIsSlash(f); return new FileStatus(0, true, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), new Path(theInternalDir.fullPath).makeQualified( myUri, ROOT_PATH)); @@ -883,7 +883,7 @@ public class ViewFileSystem extends FileSystem { result[i++] = new FileStatus(0, false, 0, 0, creationTime, creationTime, PERMISSION_555, - ugi.getUserName(), ugi.getGroupNames()[0], + ugi.getUserName(), ugi.getPrimaryGroupName(), link.getTargetLink(), new Path(inode.fullPath).makeQualified( myUri, null)); @@ -1015,7 +1015,7 @@ public class ViewFileSystem extends FileSystem { public AclStatus getAclStatus(Path path) throws IOException { checkPathIsSlash(path); return new AclStatus.Builder().owner(ugi.getUserName()) - .group(ugi.getGroupNames()[0]) + .group(ugi.getPrimaryGroupName()) .addEntries(AclUtil.getMinimalAcl(PERMISSION_555)) .stickyBit(false).build(); } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java index 2f932969502..23465a678ff 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java @@ -843,14 +843,14 @@ public class ViewFs extends AbstractFileSystem { public FileStatus getFileStatus(final Path f) throws IOException { checkPathIsSlash(f); return new FileStatus(0, true, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), new Path(theInternalDir.fullPath).makeQualified( myUri, null)); } @Override public FileStatus getFileLinkStatus(final Path f) - throws FileNotFoundException { + throws IOException { // look up i internalDirs children - ignore first Slash INode inode = theInternalDir.children.get(f.toUri().toString().substring(1)); @@ -863,13 +863,13 @@ public class ViewFs extends AbstractFileSystem { INodeLink inodelink = (INodeLink) inode; result = new FileStatus(0, false, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), inodelink.getTargetLink(), new Path(inode.fullPath).makeQualified( myUri, null)); } else { result = new FileStatus(0, true, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), new Path(inode.fullPath).makeQualified( myUri, null)); } @@ -908,7 +908,7 @@ public class ViewFs extends AbstractFileSystem { result[i++] = new FileStatus(0, false, 0, 0, creationTime, creationTime, - PERMISSION_555, ugi.getUserName(), ugi.getGroupNames()[0], + PERMISSION_555, ugi.getUserName(), ugi.getPrimaryGroupName(), link.getTargetLink(), new Path(inode.fullPath).makeQualified( myUri, null)); @@ -1042,7 +1042,7 @@ public class ViewFs extends AbstractFileSystem { public AclStatus getAclStatus(Path path) throws IOException { checkPathIsSlash(path); return new AclStatus.Builder().owner(ugi.getUserName()) - .group(ugi.getGroupNames()[0]) + .group(ugi.getPrimaryGroupName()) .addEntries(AclUtil.getMinimalAcl(PERMISSION_555)) .stickyBit(false).build(); } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java index 1b092ec8da1..ad238e6b9f9 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SecureIOUtils.java @@ -23,7 +23,6 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.RandomAccessFile; -import java.util.Arrays; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FSDataInputStream; @@ -276,7 +275,7 @@ public class SecureIOUtils { UserGroupInformation.createRemoteUser(expectedOwner); final String adminsGroupString = "Administrators"; success = owner.equals(adminsGroupString) - && Arrays.asList(ugi.getGroupNames()).contains(adminsGroupString); + && ugi.getGroups().contains(adminsGroupString); } else { success = false; } 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 5414ba5530e..df32bb3ebf6 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 @@ -18,9 +18,11 @@ package org.apache.hadoop.security; import java.io.IOException; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -31,6 +33,7 @@ import java.util.concurrent.ThreadFactory; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import org.apache.htrace.core.TraceScope; import org.apache.htrace.core.Tracer; @@ -74,8 +77,8 @@ public class Groups { private final GroupMappingServiceProvider impl; private final LoadingCache> cache; - private final Map> staticUserToGroupsMap = - new HashMap>(); + private final AtomicReference>> staticMapRef = + new AtomicReference<>(); private final long cacheTimeout; private final long negativeCacheTimeout; private final long warningDeltaMs; @@ -164,6 +167,8 @@ public class Groups { CommonConfigurationKeys.HADOOP_USER_GROUP_STATIC_OVERRIDES_DEFAULT); Collection mappings = StringUtils.getStringCollection( staticMapping, ";"); + Map> staticUserToGroupsMap = + new HashMap>(); for (String users : mappings) { Collection userToGroups = StringUtils.getStringCollection(users, "="); @@ -182,6 +187,8 @@ public class Groups { } staticUserToGroupsMap.put(user, groups); } + staticMapRef.set( + staticUserToGroupsMap.isEmpty() ? null : staticUserToGroupsMap); } private boolean isNegativeCacheEnabled() { @@ -201,9 +208,12 @@ public class Groups { */ public List getGroups(final String user) throws IOException { // No need to lookup for groups of static users - List staticMapping = staticUserToGroupsMap.get(user); - if (staticMapping != null) { - return staticMapping; + Map> staticUserToGroupsMap = staticMapRef.get(); + if (staticUserToGroupsMap != null) { + List staticMapping = staticUserToGroupsMap.get(user); + if (staticMapping != null) { + return staticMapping; + } } // Check the negative cache first @@ -322,7 +332,9 @@ public class Groups { throw noGroupsForUser(user); } - return groups; + // return immutable de-duped list + return Collections.unmodifiableList( + new ArrayList<>(new LinkedHashSet<>(groups))); } /** diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java index 370f92ff4d6..e12c9a3ef6d 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java @@ -38,7 +38,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -1499,11 +1498,11 @@ public class UserGroupInformation { } public String getPrimaryGroupName() throws IOException { - String[] groups = getGroupNames(); - if (groups.length == 0) { + List groups = getGroups(); + if (groups.isEmpty()) { throw new IOException("There is no primary group for UGI " + this); } - return groups[0]; + return groups.get(0); } /** @@ -1615,27 +1614,36 @@ public class UserGroupInformation { return credentials; } + /** + * Get the group names for this user. {@ #getGroups(String)} is less + * expensive alternative when checking for a contained element. + * @return the list of users with the primary group first. If the command + * fails, it returns an empty list. + */ + public String[] getGroupNames() { + List groups = getGroups(); + return groups.toArray(new String[groups.size()]); + } + /** * Get the group names for this user. * @return the list of users with the primary group first. If the command * fails, it returns an empty list. */ - public synchronized String[] getGroupNames() { + public List getGroups() { ensureInitialized(); try { - Set result = new LinkedHashSet - (groups.getGroups(getShortUserName())); - return result.toArray(new String[result.size()]); + return groups.getGroups(getShortUserName()); } catch (IOException ie) { if (LOG.isDebugEnabled()) { LOG.debug("Failed to get groups for user " + getShortUserName() + " by " + ie); LOG.trace("TRACE", ie); } - return StringUtils.emptyStringArray; + return Collections.emptyList(); } } - + /** * Return the username. */ diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java index b1b474b5a8d..a32a5fe74b0 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/authorize/AccessControlList.java @@ -231,7 +231,7 @@ public class AccessControlList implements Writable { if (allAllowed || users.contains(ugi.getShortUserName())) { return true; } else if (!groups.isEmpty()) { - for(String group: ugi.getGroupNames()) { + for (String group : ugi.getGroups()) { if (groups.contains(group)) { return true; } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java index 838d4311df6..189a5a83f26 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java @@ -443,8 +443,10 @@ public class TestUserGroupInformation { UserGroupInformation uugi = UserGroupInformation.createUserForTesting(USER_NAME, GROUP_NAMES); assertEquals(USER_NAME, uugi.getUserName()); - assertArrayEquals(new String[]{GROUP1_NAME, GROUP2_NAME, GROUP3_NAME}, - uugi.getGroupNames()); + String[] expected = new String[]{GROUP1_NAME, GROUP2_NAME, GROUP3_NAME}; + assertArrayEquals(expected, uugi.getGroupNames()); + assertArrayEquals(expected, uugi.getGroups().toArray(new String[0])); + assertEquals(GROUP1_NAME, uugi.getPrimaryGroupName()); } @SuppressWarnings("unchecked") // from Mockito mocks