HDFS-10673. Optimize FSPermissionChecker's internal path usage. Contributed by Daryn Sharp.

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java

@@ -1527,10 +1527,7 @@ public class FSDirectory implements Closeable {
   FSPermissionChecker getPermissionChecker(String fsOwner, String superGroup,
       UserGroupInformation ugi) throws AccessControlException {
     return new FSPermissionChecker(
-        fsOwner, superGroup, ugi,
+        fsOwner, superGroup, ugi, attributeProvider);
-        attributeProvider == null ?
-            DefaultINodeAttributesProvider.DEFAULT_PROVIDER
-            : attributeProvider);
   }
 
   void checkOwner(FSPermissionChecker pc, INodesInPath iip)
@@ -1660,15 +1657,12 @@ public class FSDirectory implements Closeable {
 
   INodeAttributes getAttributes(String fullPath, byte[] path,
       INode node, int snapshot) {
-    INodeAttributes nodeAttrs;
+    INodeAttributes nodeAttrs = node.getSnapshotINode(snapshot);
     if (attributeProvider != null) {
-      nodeAttrs = node.getSnapshotINode(snapshot);
       fullPath = fullPath
           + (fullPath.endsWith(Path.SEPARATOR) ? "" : Path.SEPARATOR)
           + DFSUtil.bytes2String(path);
       nodeAttrs = attributeProvider.getAttributes(fullPath, nodeAttrs);
-    } else {
-      nodeAttrs = node.getSnapshotINode(snapshot);
     }
     return nodeAttrs;
   }

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSPermissionChecker.java

@@ -45,15 +45,23 @@ import org.apache.hadoop.security.UserGroupInformation;
 class FSPermissionChecker implements AccessControlEnforcer {
   static final Log LOG = LogFactory.getLog(UserGroupInformation.class);
 
+  private static String constructPath(INodeAttributes[] inodes, int end) {
+    byte[][] components = new byte[end+1][];
+    for (int i=0; i <= end; i++) {
+      components[i] = inodes[i].getLocalNameBytes();
+    }
+    return DFSUtil.byteArray2PathString(components);
+  }
+
   /** @return a string for throwing {@link AccessControlException} */
   private String toAccessControlString(INodeAttributes inodeAttrib, String path,
-      FsAction access, FsPermission mode) {
+      FsAction access) {
-    return toAccessControlString(inodeAttrib, path, access, mode, false);
+    return toAccessControlString(inodeAttrib, path, access, false);
   }
 
   /** @return a string for throwing {@link AccessControlException} */
   private String toAccessControlString(INodeAttributes inodeAttrib,
-      String path, FsAction access, FsPermission mode, boolean deniedFromAcl) {
+      String path, FsAction access, boolean deniedFromAcl) {
     StringBuilder sb = new StringBuilder("Permission denied: ")
       .append("user=").append(getUser()).append(", ")
       .append("access=").append(access).append(", ")
@@ -61,7 +69,7 @@ class FSPermissionChecker implements AccessControlEnforcer {
       .append(inodeAttrib.getUserName()).append(':')
       .append(inodeAttrib.getGroupName()).append(':')
       .append(inodeAttrib.isDirectory() ? 'd' : '-')
-      .append(mode);
+      .append(inodeAttrib.getFsPermission());
     if (deniedFromAcl) {
       sb.append("+");
     }
@@ -112,6 +120,11 @@ class FSPermissionChecker implements AccessControlEnforcer {
     return attributeProvider;
   }
 
+  private AccessControlEnforcer getAccessControlEnforcer() {
+    return (attributeProvider != null)
+        ? attributeProvider.getExternalAccessControlEnforcer(this) : this;
+  }
+
   /**
    * Verify if the caller has the required permission. This will result into 
    * an exception if the caller is not allowed to access the resource.
@@ -174,47 +187,24 @@ class FSPermissionChecker implements AccessControlEnforcer {
     final int snapshotId = inodesInPath.getPathSnapshotId();
     final INode[] inodes = inodesInPath.getINodesArray();
     final INodeAttributes[] inodeAttrs = new INodeAttributes[inodes.length];
-    final byte[][] pathByNameArr = new byte[inodes.length][];
+    final byte[][] components = inodesInPath.getPathComponents();
     for (int i = 0; i < inodes.length && inodes[i] != null; i++) {
-      if (inodes[i] != null) {
+      inodeAttrs[i] = getINodeAttrs(components, i, inodes[i], snapshotId);
-        pathByNameArr[i] = inodes[i].getLocalNameBytes();
-        inodeAttrs[i] = getINodeAttrs(pathByNameArr, i, inodes[i], snapshotId);
-      }
     }
 
     String path = inodesInPath.getPath();
     int ancestorIndex = inodes.length - 2;
 
-    AccessControlEnforcer enforcer =
+    AccessControlEnforcer enforcer = getAccessControlEnforcer();
-        getAttributesProvider().getExternalAccessControlEnforcer(this);
     enforcer.checkPermission(fsOwner, supergroup, callerUgi, inodeAttrs, inodes,
-        pathByNameArr, snapshotId, path, ancestorIndex, doCheckOwner,
+        components, snapshotId, path, ancestorIndex, doCheckOwner,
         ancestorAccess, parentAccess, access, subAccess, ignoreEmptyDir);
   }
 
-  /**
-   * Check whether exception e is due to an ancestor inode's not being
-   * directory.
-   */
-  private void checkAncestorType(INode[] inodes, int checkedAncestorIndex,
-      AccessControlException e) throws AccessControlException {
-    for (int i = 0; i <= checkedAncestorIndex; i++) {
-      if (inodes[i] == null) {
-        break;
-      }
-      if (!inodes[i].isDirectory()) {
-        throw new AccessControlException(
-            e.getMessage() + " (Ancestor " + inodes[i].getFullPathName()
-                + " is not a directory).");
-      }
-    }
-    throw e;
-  }
-
   @Override
   public void checkPermission(String fsOwner, String supergroup,
       UserGroupInformation callerUgi, INodeAttributes[] inodeAttrs,
-      INode[] inodes, byte[][] pathByNameArr, int snapshotId, String path,
+      INode[] inodes, byte[][] components, int snapshotId, String path,
       int ancestorIndex, boolean doCheckOwner, FsAction ancestorAccess,
       FsAction parentAccess, FsAction access, FsAction subAccess,
       boolean ignoreEmptyDir)
@@ -222,29 +212,29 @@ class FSPermissionChecker implements AccessControlEnforcer {
     for(; ancestorIndex >= 0 && inodes[ancestorIndex] == null;
         ancestorIndex--);
 
-    checkTraverse(inodeAttrs, inodes, path, ancestorIndex);
+    checkTraverse(inodeAttrs, ancestorIndex);
 
     final INodeAttributes last = inodeAttrs[inodeAttrs.length - 1];
     if (parentAccess != null && parentAccess.implies(FsAction.WRITE)
         && inodeAttrs.length > 1 && last != null) {
-      checkStickyBit(inodeAttrs[inodeAttrs.length - 2], last, path);
+      checkStickyBit(inodeAttrs, inodeAttrs.length - 2);
     }
     if (ancestorAccess != null && inodeAttrs.length > 1) {
-      check(inodeAttrs, path, ancestorIndex, ancestorAccess);
+      check(inodeAttrs, ancestorIndex, ancestorAccess);
     }
     if (parentAccess != null && inodeAttrs.length > 1) {
-      check(inodeAttrs, path, inodeAttrs.length - 2, parentAccess);
+      check(inodeAttrs, inodeAttrs.length - 2, parentAccess);
     }
     if (access != null) {
-      check(last, path, access);
+      check(inodeAttrs, inodeAttrs.length - 1, access);
     }
     if (subAccess != null) {
       INode rawLast = inodes[inodeAttrs.length - 1];
-      checkSubAccess(pathByNameArr, inodeAttrs.length - 1, rawLast,
+      checkSubAccess(components, inodeAttrs.length - 1, rawLast,
           snapshotId, subAccess, ignoreEmptyDir);
     }
     if (doCheckOwner) {
-      checkOwner(last);
+      checkOwner(inodeAttrs, inodeAttrs.length - 1);
     }
   }
 
@@ -262,32 +252,35 @@ class FSPermissionChecker implements AccessControlEnforcer {
   }
 
   /** Guarded by {@link FSNamesystem#readLock()} */
-  private void checkOwner(INodeAttributes inode
+  private void checkOwner(INodeAttributes[] inodes, int i)
-      ) throws AccessControlException {
+      throws AccessControlException {
-    if (getUser().equals(inode.getUserName())) {
+    if (getUser().equals(inodes[i].getUserName())) {
       return;
     }
     throw new AccessControlException(
-            "Permission denied. user="
+        "Permission denied. user=" + getUser() +
-            + getUser() + " is not the owner of inode=" + inode);
+        " is not the owner of inode=" + constructPath(inodes, i));
   }
 
   /** Guarded by {@link FSNamesystem#readLock()} */
-  private void checkTraverse(INodeAttributes[] inodeAttrs, INode[] inodes,
+  private void checkTraverse(INodeAttributes[] inodeAttrs, int last)
-      String path, int last) throws AccessControlException {
+      throws AccessControlException {
-    int j = 0;
+    for (int i=0; i <= last; i++) {
-    try {
+      INodeAttributes inode = inodeAttrs[i];
-      for (; j <= last; j++) {
+      if (!inode.isDirectory()) {
-        check(inodeAttrs[j], path, FsAction.EXECUTE);
+        throw new AccessControlException(
+            constructPath(inodeAttrs, i) + " (is not a directory)");
+      }
+      if (!hasPermission(inode, FsAction.EXECUTE)) {
+        throw new AccessControlException(toAccessControlString(
+            inode, constructPath(inodeAttrs, i), FsAction.EXECUTE));
       }
-    } catch (AccessControlException e) {
-      checkAncestorType(inodes, j, e);
     }
   }
 
   /** Guarded by {@link FSNamesystem#readLock()} */
-  private void checkSubAccess(byte[][] pathByNameArr, int pathIdx, INode inode,
+  private void checkSubAccess(byte[][] components, int pathIdx,
-      int snapshotId, FsAction access, boolean ignoreEmptyDir)
+      INode inode, int snapshotId, FsAction access, boolean ignoreEmptyDir)
       throws AccessControlException {
     if (inode == null || !inode.isDirectory()) {
       return;
@@ -299,8 +292,12 @@ class FSPermissionChecker implements AccessControlEnforcer {
       ReadOnlyList<INode> cList = d.getChildrenList(snapshotId);
       if (!(cList.isEmpty() && ignoreEmptyDir)) {
         //TODO have to figure this out with inodeattribute provider
-        check(getINodeAttrs(pathByNameArr, pathIdx, d, snapshotId),
+        INodeAttributes inodeAttr =
-            inode.getFullPathName(), access);
+            getINodeAttrs(components, pathIdx, d, snapshotId);
+        if (!hasPermission(inodeAttr, access)) {
+          throw new AccessControlException(
+              toAccessControlString(inodeAttr, d.getFullPathName(), access));
+        }
       }
 
       for(INode child : cList) {
@@ -312,15 +309,21 @@ class FSPermissionChecker implements AccessControlEnforcer {
   }
 
   /** Guarded by {@link FSNamesystem#readLock()} */
-  private void check(INodeAttributes[] inodes, String path, int i, FsAction access
+  private void check(INodeAttributes[] inodes, int i, FsAction access)
-      ) throws AccessControlException {
+      throws AccessControlException {
-    check(i >= 0 ? inodes[i] : null, path, access);
+    INodeAttributes inode = (i >= 0) ? inodes[i] : null;
+    if (inode != null && !hasPermission(inode, access)) {
+      throw new AccessControlException(
+          toAccessControlString(inode, constructPath(inodes, i), access));
+    }
   }
 
-  private void check(INodeAttributes inode, String path, FsAction access
+  // return whether access is permitted.  note it neither requires a path or
-      ) throws AccessControlException {
+  // throws so the caller can build the path only if required for an exception.
+  // very beneficial for subaccess checks!
+  private boolean hasPermission(INodeAttributes inode, FsAction access) {
     if (inode == null) {
-      return;
+      return true;
     }
     final FsPermission mode = inode.getFsPermission();
     final AclFeature aclFeature = inode.getAclFeature();
@@ -328,21 +331,18 @@ class FSPermissionChecker implements AccessControlEnforcer {
       // It's possible that the inode has a default ACL but no access ACL.
       int firstEntry = aclFeature.getEntryAt(0);
       if (AclEntryStatusFormat.getScope(firstEntry) == AclEntryScope.ACCESS) {
-        checkAccessAcl(inode, path, access, mode, aclFeature);
+        return hasAclPermission(inode, access, mode, aclFeature);
-        return;
       }
     }
+    final FsAction checkAction;
     if (getUser().equals(inode.getUserName())) { //user class
-      if (mode.getUserAction().implies(access)) { return; }
+      checkAction = mode.getUserAction();
+    } else if (getGroups().contains(inode.getGroupName())) { //group class
+      checkAction = mode.getGroupAction();
+    } else { //other class
+      checkAction = mode.getOtherAction();
     }
-    else if (getGroups().contains(inode.getGroupName())) { //group class
+    return checkAction.implies(access);
-      if (mode.getGroupAction().implies(access)) { return; }
-    }
-    else { //other class
-      if (mode.getOtherAction().implies(access)) { return; }
-    }
-    throw new AccessControlException(
-        toAccessControlString(inode, path, access, mode));
   }
 
   /**
@@ -368,15 +368,14 @@ class FSPermissionChecker implements AccessControlEnforcer {
    * @param aclFeature AclFeature of inode
    * @throws AccessControlException if the ACL denies permission
    */
-  private void checkAccessAcl(INodeAttributes inode, String path,
+  private boolean hasAclPermission(INodeAttributes inode,
-      FsAction access, FsPermission mode, AclFeature aclFeature)
+      FsAction access, FsPermission mode, AclFeature aclFeature) {
-      throws AccessControlException {
     boolean foundMatch = false;
 
     // Use owner entry from permission bits if user is owner.
     if (getUser().equals(inode.getUserName())) {
       if (mode.getUserAction().implies(access)) {
-        return;
+        return true;
       }
       foundMatch = true;
     }
@@ -397,7 +396,7 @@ class FSPermissionChecker implements AccessControlEnforcer {
             FsAction masked = AclEntryStatusFormat.getPermission(entry).and(
                 mode.getGroupAction());
             if (masked.implies(access)) {
-              return;
+              return true;
             }
             foundMatch = true;
             break;
@@ -412,7 +411,7 @@ class FSPermissionChecker implements AccessControlEnforcer {
             FsAction masked = AclEntryStatusFormat.getPermission(entry).and(
                 mode.getGroupAction());
             if (masked.implies(access)) {
-              return;
+              return true;
             }
             foundMatch = true;
           }
@@ -421,17 +420,13 @@ class FSPermissionChecker implements AccessControlEnforcer {
     }
 
     // Use other entry if user was not denied by an earlier match.
-    if (!foundMatch && mode.getOtherAction().implies(access)) {
+    return !foundMatch && mode.getOtherAction().implies(access);
-      return;
-    }
-
-    throw new AccessControlException(
-        toAccessControlString(inode, path, access, mode));
   }
 
   /** Guarded by {@link FSNamesystem#readLock()} */
-  private void checkStickyBit(INodeAttributes parent, INodeAttributes inode,
+  private void checkStickyBit(INodeAttributes[] inodes, int index)
-      String path) throws AccessControlException {
+      throws AccessControlException {
+    INodeAttributes parent = inodes[index];
     if (!parent.getFsPermission().getStickyBit()) {
       return;
     }
@@ -441,6 +436,7 @@ class FSPermissionChecker implements AccessControlEnforcer {
       return;
     }
 
+    INodeAttributes inode = inodes[index + 1];
     // if this user is the file owner, return
     if (inode.getUserName().equals(getUser())) {
       return;
@@ -449,9 +445,10 @@ class FSPermissionChecker implements AccessControlEnforcer {
     throw new AccessControlException(String.format(
         "Permission denied by sticky bit: user=%s, path=\"%s\":%s:%s:%s%s, " +
         "parent=\"%s\":%s:%s:%s%s", user,
-        path, inode.getUserName(), inode.getGroupName(),
+        constructPath(inodes, index + 1),
+        inode.getUserName(), inode.getGroupName(),
         inode.isDirectory() ? "d" : "-", inode.getFsPermission().toString(),
-        path.substring(0, path.length() - inode.toString().length() - 1 ),
+        constructPath(inodes, index),
         parent.getUserName(), parent.getGroupName(),
         parent.isDirectory() ? "d" : "-", parent.getFsPermission().toString()));
   }

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java

@@ -590,6 +590,18 @@ public abstract class INode implements INodeAttributes, Diff.Element<byte[]> {
     return DFSUtil.bytes2String(path);
   }
 
+  public byte[][] getPathComponents() {
+    int n = 0;
+    for (INode inode = this; inode != null; inode = inode.getParent()) {
+      n++;
+    }
+    byte[][] components = new byte[n][];
+    for (INode inode = this; inode != null; inode = inode.getParent()) {
+      components[--n] = inode.getLocalNameBytes();
+    }
+    return components;
+  }
+
   @Override
   public String toString() {
     return getLocalName();

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeAttributeProvider.java

@@ -29,6 +29,7 @@ import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.permission.FsAction;
+import org.apache.hadoop.hdfs.DFSUtil;
 import org.apache.hadoop.security.AccessControlException;
 import org.apache.hadoop.security.UserGroupInformation;
 
@@ -121,6 +122,15 @@ public abstract class INodeAttributeProvider {
   public abstract INodeAttributes getAttributes(String[] pathElements,
       INodeAttributes inode);
 
+  public INodeAttributes getAttributes(byte[][] components,
+      INodeAttributes inode) {
+    String[] elements = new String[components.length];
+    for (int i = 0; i < elements.length; i++) {
+      elements[i] = DFSUtil.bytes2String(components[i]);
+    }
+    return getAttributes(elements, inode);
+  }
+
   /**
    * Can be over-ridden by implementations to provide a custom Access Control
    * Enforcer that can provide an alternate implementation of the

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSPermission.java

@@ -566,11 +566,12 @@ public class TestDFSPermission {
       fs.exists(nfpath);
       fail("The exists call should have failed.");
     } catch (AccessControlException e) {
-      assertTrue("Permission denied messages must carry file path",
+      assertFalse("Permission denied messages must not carry full file path,"
+              + "since the user does not have permission on /p4: "
+              + e.getMessage(),
           e.getMessage().contains(fpath.getName()));
-      assertFalse("Permission denied messages should not specify existing_file"
+      assertFalse("Permission denied messages must not specify /p4"
-              + " is not a directory, since the user does not have permission"
+          + " is not a directory: " + e.getMessage(),
-              + " on /p4",
           e.getMessage().contains("is not a directory"));
     }
   }