From ebb72865ea753b39efc4c8cf1fecbbb74efbbf11 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Wed, 18 Sep 2019 21:03:15 -0700 Subject: [PATCH 01/12] Fetch ACL while scanning external store --- .../org/apache/hadoop/hdfs/DFSConfigKeys.java | 2 + .../src/main/resources/hdfs-default.xml | 11 ++ .../hdfs/server/namenode/FSTreeWalk.java | 52 ++++++- .../hadoop/hdfs/server/namenode/TreePath.java | 45 ++++-- .../hadoop/hdfs/server/namenode/TreeWalk.java | 4 +- .../hdfs/server/namenode/UGIResolver.java | 142 +++++++++++++++++- .../hdfs/server/namenode/RandomTreeWalk.java | 6 +- 7 files changed, 241 insertions(+), 21 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java index 3b776bab109d3..3a44fc6d52eea 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java @@ -379,6 +379,8 @@ public class DFSConfigKeys extends CommonConfigurationKeys { public static final String DFS_PROVIDED_ALIASMAP_TEXT_WRITE_DIR_DEFAULT = "file:///tmp/"; public static final String DFS_PROVIDED_ALIASMAP_LEVELDB_PATH = "dfs.provided.aliasmap.leveldb.path"; + public static final String DFS_NAMENODE_MOUNT_ACLS_ENABLED = "dfs.namenode.mount.acls.enabled"; + public static final boolean DFS_NAMENODE_MOUNT_ACLS_ENABLED_DEFAULT = false; public static final String DFS_LIST_LIMIT = "dfs.ls.limit"; public static final int DFS_LIST_LIMIT_DEFAULT = 1000; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml b/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml index 9b999643f7ed3..4ba0c97018dc9 100755 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml @@ -5357,6 +5357,17 @@ + + dfs.namenode.mount.acls.enabled + false + + Set to true to inherit ACLs (Access Control Lists) from remote stores + during mount. Disabled by default, i.e., ACLs are not inherited from + remote stores. Note had HDFS ACLs have to be enabled + (dfs.namenode.acls.enabled must be set to true) for this to take effect. + + + dfs.provided.aliasmap.load.retries 0 diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java index 2d865037d50fb..4755ba544a789 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java @@ -29,6 +29,14 @@ import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.AclStatus; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_DEFAULT; +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_KEY; +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_MOUNT_ACLS_ENABLED; +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_MOUNT_ACLS_ENABLED_DEFAULT; /** * Traversal of an external FileSystem. @@ -37,12 +45,28 @@ @InterfaceStability.Unstable public class FSTreeWalk extends TreeWalk { + public static final Logger LOG = + LoggerFactory.getLogger(FSTreeWalk.class); + private final Path root; private final FileSystem fs; + private final boolean enableACLs; public FSTreeWalk(Path root, Configuration conf) throws IOException { this.root = root; fs = root.getFileSystem(conf); + + boolean mountACLsEnabled = conf.getBoolean(DFS_NAMENODE_MOUNT_ACLS_ENABLED, + DFS_NAMENODE_MOUNT_ACLS_ENABLED_DEFAULT); + boolean localACLsEnabled = conf.getBoolean(DFS_NAMENODE_ACLS_ENABLED_KEY, + DFS_NAMENODE_ACLS_ENABLED_DEFAULT); + if (!localACLsEnabled && mountACLsEnabled) { + LOG.warn("Mount ACLs have been enabled but HDFS ACLs are not. " + + "Disabling ACLs on the mount {}", root); + this.enableACLs = false; + } else { + this.enableACLs = mountACLsEnabled; + } } @Override @@ -55,7 +79,7 @@ protected Iterable getChildren(TreePath path, long id, try { ArrayList ret = new ArrayList<>(); for (FileStatus s : fs.listStatus(path.getFileStatus().getPath())) { - ret.add(new TreePath(s, id, i, fs)); + ret.add(new TreePath(s, id, i, fs, getAclStatus(fs, s.getPath()))); } return ret; } catch (FileNotFoundException e) { @@ -71,14 +95,24 @@ private FSTreeIterator() { } FSTreeIterator(TreePath p) { + AclStatus acls = null; + Path remotePath = p.getFileStatus().getPath(); + try { + acls = getAclStatus(fs, remotePath); + } catch (IOException e) { + LOG.warn( + "Got exception when trying to get remote acls for path {} : {}", + remotePath, e.getMessage()); + } getPendingQueue().addFirst( - new TreePath(p.getFileStatus(), p.getParentId(), this, fs)); + new TreePath(p.getFileStatus(), p.getParentId(), this, fs, acls)); } FSTreeIterator(Path p) throws IOException { try { FileStatus s = fs.getFileStatus(root); - getPendingQueue().addFirst(new TreePath(s, -1L, this, fs)); + AclStatus acls = getAclStatus(fs, s.getPath()); + getPendingQueue().addFirst(new TreePath(s, -1L, this, fs, acls)); } catch (FileNotFoundException e) { if (p.equals(root)) { throw e; @@ -97,6 +131,17 @@ public TreeIterator fork() { } + private AclStatus getAclStatus(FileSystem fs, Path path) throws IOException { + if (enableACLs) { + try { + return fs.getAclStatus(path); + } catch (UnsupportedOperationException e) { + LOG.warn("Remote filesystem {} doesn't support ACLs", fs); + } + } + return null; + } + @Override public TreeIterator iterator() { try { @@ -105,5 +150,4 @@ public TreeIterator iterator() { throw new RuntimeException(e); } } - } diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java index fd4dbff144e74..7f1190d874c65 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java @@ -19,6 +19,7 @@ import java.io.IOException; +import com.google.common.annotations.VisibleForTesting; import com.google.protobuf.ByteString; import org.apache.hadoop.classification.InterfaceAudience; @@ -27,6 +28,7 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Options; import org.apache.hadoop.fs.PathHandle; +import org.apache.hadoop.fs.permission.AclStatus; import org.apache.hadoop.hdfs.protocol.HdfsConstants; import org.apache.hadoop.hdfs.protocol.proto.HdfsProtos.BlockProto; import org.apache.hadoop.hdfs.server.common.FileRegion; @@ -52,23 +54,38 @@ public class TreePath { private final FileStatus stat; private final TreeWalk.TreeIterator i; private final FileSystem fs; + private final AclStatus acls; - protected TreePath(FileStatus stat, long parentId, TreeWalk.TreeIterator i, - FileSystem fs) { + @VisibleForTesting + public TreePath(FileStatus stat, long parentId, TreeWalk.TreeIterator i) { + this(stat, parentId, i, null, null); + } + + public TreePath(FileStatus stat, long parentId, TreeWalk.TreeIterator i, + FileSystem fs, AclStatus acls) { this.i = i; this.stat = stat; this.parentId = parentId; this.fs = fs; + this.acls = acls; } public FileStatus getFileStatus() { return stat; } + public AclStatus getAclStatus() { + return acls; + } + public long getParentId() { return parentId; } + public TreeWalk.TreeIterator getIterator() { + return i; + } + public long getId() { if (id < 0) { throw new IllegalStateException(); @@ -76,7 +93,7 @@ public long getId() { return id; } - void accept(long id) { + public void accept(long id) { this.id = id; i.onAccept(this, id); } @@ -121,14 +138,14 @@ void writeBlock(long blockId, long offset, long length, long genStamp, INode toFile(UGIResolver ugi, BlockResolver blk, BlockAliasMap.Writer out) throws IOException { final FileStatus s = getFileStatus(); - ugi.addUser(s.getOwner()); - ugi.addGroup(s.getGroup()); + final AclStatus aclStatus = getAclStatus(); + long permissions = ugi.getPermissionsProto(s, aclStatus); INodeFile.Builder b = INodeFile.newBuilder() .setReplication(blk.getReplication(s)) .setModificationTime(s.getModificationTime()) .setAccessTime(s.getAccessTime()) .setPreferredBlockSize(blk.preferredBlockSize(s)) - .setPermission(ugi.resolve(s)) + .setPermission(permissions) .setStoragePolicyID(HdfsConstants.PROVIDED_STORAGE_POLICY_ID); // pathhandle allows match as long as the file matches exactly. @@ -141,7 +158,11 @@ INode toFile(UGIResolver ugi, BlockResolver blk, "Exact path handle not supported by filesystem " + fs.toString()); } } - // TODO: storage policy should be configurable per path; use BlockResolver + if (aclStatus != null) { + throw new UnsupportedOperationException( + "Acls not supported by ImageWriter"); + } + //TODO: storage policy should be configurable per path; use BlockResolver long off = 0L; for (BlockProto block : blk.resolve(s)) { b.addBlocks(block); @@ -159,13 +180,17 @@ INode toFile(UGIResolver ugi, BlockResolver blk, INode toDirectory(UGIResolver ugi) { final FileStatus s = getFileStatus(); - ugi.addUser(s.getOwner()); - ugi.addGroup(s.getGroup()); + final AclStatus aclStatus = getAclStatus(); + long permissions = ugi.getPermissionsProto(s, aclStatus); INodeDirectory.Builder b = INodeDirectory.newBuilder() .setModificationTime(s.getModificationTime()) .setNsQuota(DEFAULT_NAMESPACE_QUOTA) .setDsQuota(DEFAULT_STORAGE_SPACE_QUOTA) - .setPermission(ugi.resolve(s)); + .setPermission(permissions); + if (aclStatus != null) { + throw new UnsupportedOperationException( + "Acls not supported by ImageWriter"); + } INode.Builder ib = INode.newBuilder() .setType(INode.Type.DIRECTORY) .setId(id) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreeWalk.java index 03675ecc925c0..b5360be16903e 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreeWalk.java @@ -17,6 +17,8 @@ */ package org.apache.hadoop.hdfs.server.namenode; +import org.apache.hadoop.fs.Path; + import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; @@ -50,7 +52,7 @@ public abstract class TreeIterator implements Iterator { private final Deque pending; - TreeIterator() { + public TreeIterator() { this(new ArrayDeque()); } diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java index 72d1fa866d561..aa59d718d4da7 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java @@ -17,14 +17,20 @@ */ package org.apache.hadoop.hdfs.server.namenode; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.permission.AclEntry; +import org.apache.hadoop.fs.permission.AclEntryType; +import org.apache.hadoop.fs.permission.AclStatus; import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.fs.permission.PermissionStatus; /** * Pluggable class for mapping ownership and permissions from an external @@ -34,9 +40,9 @@ @InterfaceStability.Unstable public abstract class UGIResolver { - static final int USER_STRID_OFFSET = 40; - static final int GROUP_STRID_OFFSET = 16; - static final long USER_GROUP_STRID_MASK = (1 << 24) - 1; + public static final int USER_STRID_OFFSET = 40; + public static final int GROUP_STRID_OFFSET = 16; + public static final long USER_GROUP_STRID_MASK = (1 << 24) - 1; /** * Permission is serialized as a 64-bit long. [0:24):[25:48):[48:64) (in Big @@ -132,4 +138,134 @@ public FsPermission permission(FileStatus s) { return s.getPermission(); } + private long resolve(AclStatus aclStatus) { + return buildPermissionStatus( + user(aclStatus), group(aclStatus), permission(aclStatus).toShort()); + } + + /** + * Get the locally mapped user for external {@link AclStatus}. + * + * @param aclStatus AclStatus on external store. + * @return locally mapped user name. + */ + public String user(AclStatus aclStatus) { + return aclStatus.getOwner(); + } + + /** + * Get the locally mapped group for the external {@link AclStatus}. + * + * @param aclStatus AclStatus on the external store. + * @return locally mapped group name. + */ + public String group(AclStatus aclStatus) { + return aclStatus.getGroup(); + } + + /** + * Get the locally mapped {@link FsPermission} for the external + * {@link AclStatus}. + * + * @param aclStatus AclStatus on the external store. + * @return local {@link FsPermission} the external AclStatus maps to. + */ + public FsPermission permission(AclStatus aclStatus) { + return aclStatus.getPermission(); + } + + /** + * Returns list of Acl entries to apply to local paths based on remote acls. + * + * @param aclStatus remote ACL status. + * @return local HDFS ACL entries. + */ + public List aclEntries(AclStatus aclStatus) { + List newAclEntries = new ArrayList<>(); + for (AclEntry entry : aclStatus.getEntries()) { + newAclEntries.add(getLocalAclEntry(entry)); + } + return newAclEntries; + } + + /** + * Map the {@link AclEntry} on the remote store to one on the local HDFS. + * @param remoteEntry the {@link AclEntry} on the remote store. + * @return the {@link AclEntry} on the local HDFS. + */ + protected AclEntry getLocalAclEntry(AclEntry remoteEntry) { + return new AclEntry.Builder() + .setType(remoteEntry.getType()) + .setName(remoteEntry.getName()) + .setPermission(remoteEntry.getPermission()) + .setScope(remoteEntry.getScope()) + .build(); + } + + /** + * Get the local {@link PermissionStatus} the external {@link FileStatus} or + * {@link AclStatus} map to. {@code remoteAcl} is used when it + * is not null, otherwise {@code remoteStatus} is used. + * + * @param remoteStatus FileStatus on remote store. + * @param remoteAcl AclStatus on external store. + * @return the local {@link PermissionStatus}. + */ + public PermissionStatus getPermissions(FileStatus remoteStatus, + AclStatus remoteAcl) { + addUGI(remoteStatus, remoteAcl); + if (remoteAcl == null) { + return new PermissionStatus(user(remoteStatus), + group(remoteStatus), permission(remoteStatus)); + } else { + return new PermissionStatus(user(remoteAcl), + group(remoteAcl), permission(remoteAcl)); + } + } + + /** + * Get the serialized, local permissions for the external + * {@link FileStatus} or {@link AclStatus}. {@code remoteAcl} is used when it + * is not null, otherwise {@code remoteStatus} is used. + * + * @param remoteStatus FileStatus on remote store. + * @param remoteAcl AclStatus on external store. + * @return serialized, local permissions the FileStatus or AclStatus map to. + */ + public long getPermissionsProto(FileStatus remoteStatus, + AclStatus remoteAcl) { + addUGI(remoteStatus, remoteAcl); + if (remoteAcl == null) { + return resolve(remoteStatus); + } else { + return resolve(remoteAcl); + } + } + + /** + * Add the users and groups specified by the given {@link FileStatus} and + * {@link AclStatus}. + * + * @param remoteStatus + * @param remoteAcl + */ + private void addUGI(FileStatus remoteStatus, AclStatus remoteAcl) { + if (remoteAcl != null) { + addUser(remoteAcl.getOwner()); + addGroup(remoteAcl.getGroup()); + for (AclEntry entry : remoteAcl.getEntries()) { + // add the users and groups in this acl entry to ugi + if (entry.getName() != null) { + if (entry.getType() == AclEntryType.USER) { + addUser(entry.getName()); + } else if (entry.getType() == AclEntryType.GROUP) { + addGroup(entry.getName()); + } + } + } + } else { + addUser(remoteStatus.getOwner()); + addGroup(remoteStatus.getGroup()); + } + } } diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/RandomTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/RandomTreeWalk.java index 6e5b1663cc89b..b7d2225f59cb4 100644 --- a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/RandomTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/RandomTreeWalk.java @@ -95,7 +95,7 @@ protected Iterable getChildren(TreePath p, long id, int nChildren = r.nextInt(children); ArrayList ret = new ArrayList(); for (int i = 0; i < nChildren; ++i) { - ret.add(new TreePath(genFileStatus(p, r), p.getId(), walk, null)); + ret.add(new TreePath(genFileStatus(p, r), p.getId(), walk)); } return ret; } @@ -163,12 +163,12 @@ class RandomTreeIterator extends TreeIterator { RandomTreeIterator(long seed) { Random r = new Random(seed); FileStatus iroot = genFileStatus(null, r); - getPendingQueue().addFirst(new TreePath(iroot, -1, this, null)); + getPendingQueue().addFirst(new TreePath(iroot, -1, this)); } RandomTreeIterator(TreePath p) { getPendingQueue().addFirst( - new TreePath(p.getFileStatus(), p.getParentId(), this, null)); + new TreePath(p.getFileStatus(), p.getParentId(), this)); } @Override From 7ad6623e65f6e43f94c662b14b8d46b9ec304b96 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Fri, 20 Sep 2019 11:49:15 -0700 Subject: [PATCH 02/12] Add unit test to verify remote acl import --- .../hdfs/server/namenode/TestFSTreeWalk.java | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java new file mode 100644 index 0000000000000..08b96c4219df6 --- /dev/null +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java @@ -0,0 +1,77 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdfs.server.namenode; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.AclStatus; +import org.apache.hadoop.hdfs.DFSConfigKeys; +import org.junit.Test; + +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Validate FSTreeWalk specific behavior + */ +public class TestFSTreeWalk { + // verify that the ACLs are fetched when configured + @Test + public void testImportAcl() throws Exception { + Configuration conf = new Configuration(); + conf.setBoolean(DFSConfigKeys.DFS_NAMENODE_MOUNT_ACLS_ENABLED, true); + + FileSystem fs = mock(FileSystem.class); + Path root = mock(Path.class); + when(root.getFileSystem(conf)).thenReturn(fs); + + Map expectedChildren = new HashMap<>(); + FileStatus child1 = new FileStatus(0, true, 0, 0, 1, new Path("/a")); + FileStatus child2 = new FileStatus(0, true, 0, 0, 1, new Path("/b")); + expectedChildren.put(child1.getPath(), child1); + expectedChildren.put(child2.getPath(), child2); + when(fs.listStatus(root)).thenReturn(expectedChildren.values().toArray(new FileStatus[1])); + + AclStatus expectedAcls = mock(AclStatus.class); + when(fs.getAclStatus(any(Path.class))).thenReturn(expectedAcls); + + FSTreeWalk fsTreeWalk = new FSTreeWalk(root, conf); + + FileStatus rootFileStatus = new FileStatus(0, true, 0, 0, 1, root); + TreePath treePath = new TreePath(rootFileStatus, 1, null); + + Iterable result = fsTreeWalk.getChildren(treePath, 1, null); + for (TreePath path : result) { + FileStatus expectedChildStatus = expectedChildren.remove(path.getFileStatus().getPath()); + assertNotNull(expectedChildStatus); + + AclStatus childAcl = path.getAclStatus(); + assertEquals(expectedAcls, childAcl); + } + + assertEquals(0, expectedChildren.size()); + } +} From 04d0a8b463c8ee5962f5e18c1e2aa5f5c86e45a3 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Fri, 20 Sep 2019 10:37:34 -0700 Subject: [PATCH 03/12] Address review comments --- .../hdfs/server/namenode/FSTreeWalk.java | 8 +- .../hadoop/hdfs/server/namenode/TreePath.java | 6 +- .../hadoop/hdfs/server/namenode/TreeWalk.java | 2 - .../hdfs/server/namenode/UGIResolver.java | 96 ++----------------- 4 files changed, 14 insertions(+), 98 deletions(-) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java index 4755ba544a789..2fbcc064e4953 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java @@ -79,7 +79,8 @@ protected Iterable getChildren(TreePath path, long id, try { ArrayList ret = new ArrayList<>(); for (FileStatus s : fs.listStatus(path.getFileStatus().getPath())) { - ret.add(new TreePath(s, id, i, fs, getAclStatus(fs, s.getPath()))); + AclStatus aclStatus = getAclStatus(fs, s.getPath()); + ret.add(new TreePath(s, id, i, fs, aclStatus)); } return ret; } catch (FileNotFoundException e) { @@ -100,9 +101,7 @@ private FSTreeIterator() { try { acls = getAclStatus(fs, remotePath); } catch (IOException e) { - LOG.warn( - "Got exception when trying to get remote acls for path {} : {}", - remotePath, e.getMessage()); + throw new RuntimeException(e); } getPendingQueue().addFirst( new TreePath(p.getFileStatus(), p.getParentId(), this, fs, acls)); @@ -150,4 +149,5 @@ public TreeIterator iterator() { throw new RuntimeException(e); } } + } diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java index 7f1190d874c65..dcd8bf0cbc689 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java @@ -159,8 +159,7 @@ INode toFile(UGIResolver ugi, BlockResolver blk, } } if (aclStatus != null) { - throw new UnsupportedOperationException( - "Acls not supported by ImageWriter"); + throw new UnsupportedOperationException("ACLs not supported by ImageWriter"); } //TODO: storage policy should be configurable per path; use BlockResolver long off = 0L; @@ -188,8 +187,7 @@ INode toDirectory(UGIResolver ugi) { .setDsQuota(DEFAULT_STORAGE_SPACE_QUOTA) .setPermission(permissions); if (aclStatus != null) { - throw new UnsupportedOperationException( - "Acls not supported by ImageWriter"); + throw new UnsupportedOperationException("ACLs not supported by ImageWriter"); } INode.Builder ib = INode.newBuilder() .setType(INode.Type.DIRECTORY) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreeWalk.java index b5360be16903e..af801c82c41f8 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreeWalk.java @@ -17,8 +17,6 @@ */ package org.apache.hadoop.hdfs.server.namenode; -import org.apache.hadoop.fs.Path; - import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java index aa59d718d4da7..a972d95f7af95 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java @@ -17,10 +17,8 @@ */ package org.apache.hadoop.hdfs.server.namenode; -import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; -import java.util.List; import java.util.Map; import org.apache.hadoop.classification.InterfaceAudience; @@ -30,7 +28,6 @@ import org.apache.hadoop.fs.permission.AclEntryType; import org.apache.hadoop.fs.permission.AclStatus; import org.apache.hadoop.fs.permission.FsPermission; -import org.apache.hadoop.fs.permission.PermissionStatus; /** * Pluggable class for mapping ownership and permissions from an external @@ -139,88 +136,10 @@ public FsPermission permission(FileStatus s) { } private long resolve(AclStatus aclStatus) { - return buildPermissionStatus( - user(aclStatus), group(aclStatus), permission(aclStatus).toShort()); - } - - /** - * Get the locally mapped user for external {@link AclStatus}. - * - * @param aclStatus AclStatus on external store. - * @return locally mapped user name. - */ - public String user(AclStatus aclStatus) { - return aclStatus.getOwner(); - } - - /** - * Get the locally mapped group for the external {@link AclStatus}. - * - * @param aclStatus AclStatus on the external store. - * @return locally mapped group name. - */ - public String group(AclStatus aclStatus) { - return aclStatus.getGroup(); - } - - /** - * Get the locally mapped {@link FsPermission} for the external - * {@link AclStatus}. - * - * @param aclStatus AclStatus on the external store. - * @return local {@link FsPermission} the external AclStatus maps to. - */ - public FsPermission permission(AclStatus aclStatus) { - return aclStatus.getPermission(); - } - - /** - * Returns list of Acl entries to apply to local paths based on remote acls. - * - * @param aclStatus remote ACL status. - * @return local HDFS ACL entries. - */ - public List aclEntries(AclStatus aclStatus) { - List newAclEntries = new ArrayList<>(); - for (AclEntry entry : aclStatus.getEntries()) { - newAclEntries.add(getLocalAclEntry(entry)); - } - return newAclEntries; - } - - /** - * Map the {@link AclEntry} on the remote store to one on the local HDFS. - * @param remoteEntry the {@link AclEntry} on the remote store. - * @return the {@link AclEntry} on the local HDFS. - */ - protected AclEntry getLocalAclEntry(AclEntry remoteEntry) { - return new AclEntry.Builder() - .setType(remoteEntry.getType()) - .setName(remoteEntry.getName()) - .setPermission(remoteEntry.getPermission()) - .setScope(remoteEntry.getScope()) - .build(); - } - - /** - * Get the local {@link PermissionStatus} the external {@link FileStatus} or - * {@link AclStatus} map to. {@code remoteAcl} is used when it - * is not null, otherwise {@code remoteStatus} is used. - * - * @param remoteStatus FileStatus on remote store. - * @param remoteAcl AclStatus on external store. - * @return the local {@link PermissionStatus}. - */ - public PermissionStatus getPermissions(FileStatus remoteStatus, - AclStatus remoteAcl) { - addUGI(remoteStatus, remoteAcl); - if (remoteAcl == null) { - return new PermissionStatus(user(remoteStatus), - group(remoteStatus), permission(remoteStatus)); - } else { - return new PermissionStatus(user(remoteAcl), - group(remoteAcl), permission(remoteAcl)); - } + String owner = aclStatus.getOwner(); + String group = aclStatus.getGroup(); + short permission = aclStatus.getPermission().toShort(); + return buildPermissionStatus(owner, group, permission); } /** @@ -255,11 +174,12 @@ private void addUGI(FileStatus remoteStatus, AclStatus remoteAcl) { addGroup(remoteAcl.getGroup()); for (AclEntry entry : remoteAcl.getEntries()) { // add the users and groups in this acl entry to ugi - if (entry.getName() != null) { + String name = entry.getName(); + if (name != null) { if (entry.getType() == AclEntryType.USER) { - addUser(entry.getName()); + addUser(name); } else if (entry.getType() == AclEntryType.GROUP) { - addGroup(entry.getName()); + addGroup(name); } } } From 85c832857a5cc0f0ef7cbe1e44b7c4f2fcdd3e81 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Fri, 20 Sep 2019 15:52:41 -0700 Subject: [PATCH 04/12] Make name of acl-import config consistent with the rest --- .../main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java | 4 ++-- .../hadoop-hdfs/src/main/resources/hdfs-default.xml | 2 +- .../apache/hadoop/hdfs/server/namenode/FSTreeWalk.java | 8 ++++---- .../hadoop/hdfs/server/namenode/TestFSTreeWalk.java | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java index 3a44fc6d52eea..40d0abf613c0c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java @@ -379,8 +379,8 @@ public class DFSConfigKeys extends CommonConfigurationKeys { public static final String DFS_PROVIDED_ALIASMAP_TEXT_WRITE_DIR_DEFAULT = "file:///tmp/"; public static final String DFS_PROVIDED_ALIASMAP_LEVELDB_PATH = "dfs.provided.aliasmap.leveldb.path"; - public static final String DFS_NAMENODE_MOUNT_ACLS_ENABLED = "dfs.namenode.mount.acls.enabled"; - public static final boolean DFS_NAMENODE_MOUNT_ACLS_ENABLED_DEFAULT = false; + public static final String DFS_PROVIDED_ACLS_IMPORT_ENABLED = "dfs.provided.acls.import.enabled"; + public static final boolean DFS_PROVIDED_ACLS_IMPORT_ENABLED_DEFAULT = false; public static final String DFS_LIST_LIMIT = "dfs.ls.limit"; public static final int DFS_LIST_LIMIT_DEFAULT = 1000; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml b/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml index 4ba0c97018dc9..27b3fe60c4935 100755 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml @@ -5358,7 +5358,7 @@ - dfs.namenode.mount.acls.enabled + dfs.provided.acls.import.enabled false Set to true to inherit ACLs (Access Control Lists) from remote stores diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java index 2fbcc064e4953..70778231866aa 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java @@ -35,8 +35,8 @@ import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_DEFAULT; import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_KEY; -import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_MOUNT_ACLS_ENABLED; -import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_MOUNT_ACLS_ENABLED_DEFAULT; +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_PROVIDED_ACLS_IMPORT_ENABLED; +import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_PROVIDED_ACLS_IMPORT_ENABLED_DEFAULT; /** * Traversal of an external FileSystem. @@ -56,8 +56,8 @@ public FSTreeWalk(Path root, Configuration conf) throws IOException { this.root = root; fs = root.getFileSystem(conf); - boolean mountACLsEnabled = conf.getBoolean(DFS_NAMENODE_MOUNT_ACLS_ENABLED, - DFS_NAMENODE_MOUNT_ACLS_ENABLED_DEFAULT); + boolean mountACLsEnabled = conf.getBoolean(DFS_PROVIDED_ACLS_IMPORT_ENABLED, + DFS_PROVIDED_ACLS_IMPORT_ENABLED_DEFAULT); boolean localACLsEnabled = conf.getBoolean(DFS_NAMENODE_ACLS_ENABLED_KEY, DFS_NAMENODE_ACLS_ENABLED_DEFAULT); if (!localACLsEnabled && mountACLsEnabled) { diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java index 08b96c4219df6..71d367ac12ca0 100644 --- a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java @@ -42,7 +42,7 @@ public class TestFSTreeWalk { @Test public void testImportAcl() throws Exception { Configuration conf = new Configuration(); - conf.setBoolean(DFSConfigKeys.DFS_NAMENODE_MOUNT_ACLS_ENABLED, true); + conf.setBoolean(DFSConfigKeys.DFS_PROVIDED_ACLS_IMPORT_ENABLED, true); FileSystem fs = mock(FileSystem.class); Path root = mock(Path.class); From 84df44c44a886f0868776b37425c40ddc2ea58c1 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Fri, 20 Sep 2019 16:19:41 -0700 Subject: [PATCH 05/12] Refactor code based on review comments --- .../org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java index 70778231866aa..187c15014f070 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java @@ -97,14 +97,15 @@ private FSTreeIterator() { FSTreeIterator(TreePath p) { AclStatus acls = null; - Path remotePath = p.getFileStatus().getPath(); + FileStatus fileStatus = p.getFileStatus(); + Path remotePath = fileStatus.getPath(); try { acls = getAclStatus(fs, remotePath); } catch (IOException e) { throw new RuntimeException(e); } - getPendingQueue().addFirst( - new TreePath(p.getFileStatus(), p.getParentId(), this, fs, acls)); + TreePath treePath = new TreePath(fileStatus, p.getParentId(), this, fs, acls); + getPendingQueue().addFirst(treePath); } FSTreeIterator(Path p) throws IOException { From e12a30fe28b95a1515ebd55a36e5e3cdbba0f110 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Mon, 23 Sep 2019 11:44:09 -0700 Subject: [PATCH 06/12] Refactor common code and exception handling --- .../hdfs/server/namenode/FSTreeWalk.java | 30 +++++++------------ .../hdfs/server/namenode/TestFSTreeWalk.java | 4 ++- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java index 187c15014f070..3a870208b4b47 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java @@ -96,29 +96,18 @@ private FSTreeIterator() { } FSTreeIterator(TreePath p) { - AclStatus acls = null; - FileStatus fileStatus = p.getFileStatus(); - Path remotePath = fileStatus.getPath(); - try { - acls = getAclStatus(fs, remotePath); - } catch (IOException e) { - throw new RuntimeException(e); - } - TreePath treePath = new TreePath(fileStatus, p.getParentId(), this, fs, acls); - getPendingQueue().addFirst(treePath); + this(p.getFileStatus(), p.getParentId()); } - FSTreeIterator(Path p) throws IOException { + FSTreeIterator(FileStatus fileStatus, long parentId) { + Path path = fileStatus.getPath(); + AclStatus acls; try { - FileStatus s = fs.getFileStatus(root); - AclStatus acls = getAclStatus(fs, s.getPath()); - getPendingQueue().addFirst(new TreePath(s, -1L, this, fs, acls)); - } catch (FileNotFoundException e) { - if (p.equals(root)) { - throw e; - } - throw new ConcurrentModificationException("FS modified"); + acls = getAclStatus(fs, path); + } catch (IOException e) { + throw new RuntimeException(e); } + getPendingQueue().addFirst(new TreePath(fileStatus, parentId, this, fs, acls)); } @Override @@ -145,7 +134,8 @@ private AclStatus getAclStatus(FileSystem fs, Path path) throws IOException { @Override public TreeIterator iterator() { try { - return new FSTreeIterator(root); + FileStatus s = fs.getFileStatus(root); + return new FSTreeIterator(s, -1L); } catch (IOException e) { throw new RuntimeException(e); } diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java index 71d367ac12ca0..b02f6ebee624d 100644 --- a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java @@ -38,7 +38,9 @@ * Validate FSTreeWalk specific behavior */ public class TestFSTreeWalk { - // verify that the ACLs are fetched when configured + /** + * Verify that the ACLs are fetched when configured + */ @Test public void testImportAcl() throws Exception { Configuration conf = new Configuration(); From 5f1b53d1a9b3e363514a3cf40111188494561e62 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Mon, 23 Sep 2019 15:05:10 -0700 Subject: [PATCH 07/12] Add unit test to verify ugi resolution --- .../server/namenode/SingleUGIResolver.java | 4 +-- .../hdfs/server/namenode/UGIResolver.java | 30 +++++++++-------- .../namenode/TestSingleUGIResolver.java | 32 +++++++++++++++++++ 3 files changed, 50 insertions(+), 16 deletions(-) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/SingleUGIResolver.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/SingleUGIResolver.java index 9c42c11fff4fb..dca665e13750e 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/SingleUGIResolver.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/SingleUGIResolver.java @@ -73,12 +73,12 @@ public Configuration getConf() { } @Override - public String user(FileStatus s) { + public String user(String s) { return user; } @Override - public String group(FileStatus s) { + public String group(String s) { return group; } diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java index a972d95f7af95..dfd2029e2593e 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java @@ -120,26 +120,29 @@ protected void resetUGInfo() { } public long resolve(FileStatus s) { - return buildPermissionStatus(user(s), group(s), permission(s).toShort()); + String resolvedGroup = group(s.getGroup()); + String resolvedOwner = user(s.getOwner()); + FsPermission resolvedPermission = permission(s.getPermission()); + return buildPermissionStatus(resolvedOwner, resolvedGroup, resolvedPermission.toShort()); } - public String user(FileStatus s) { - return s.getOwner(); + private long resolve(AclStatus aclStatus) { + String resolvedOwner = user(aclStatus.getOwner()); + String resolvedGroup = group(aclStatus.getGroup()); + FsPermission resolvedPermision = permission(aclStatus.getPermission()); + return buildPermissionStatus(resolvedOwner, resolvedGroup, resolvedPermision.toShort()); } - public String group(FileStatus s) { - return s.getGroup(); + protected String user(String s) { + return s; } - public FsPermission permission(FileStatus s) { - return s.getPermission(); + protected String group(String s) { + return s; } - private long resolve(AclStatus aclStatus) { - String owner = aclStatus.getOwner(); - String group = aclStatus.getGroup(); - short permission = aclStatus.getPermission().toShort(); - return buildPermissionStatus(owner, group, permission); + public FsPermission permission(FsPermission s) { + return s; } /** @@ -151,8 +154,7 @@ private long resolve(AclStatus aclStatus) { * @param remoteAcl AclStatus on external store. * @return serialized, local permissions the FileStatus or AclStatus map to. */ - public long getPermissionsProto(FileStatus remoteStatus, - AclStatus remoteAcl) { + public long getPermissionsProto(FileStatus remoteStatus, AclStatus remoteAcl) { addUGI(remoteStatus, remoteAcl); if (remoteAcl == null) { return resolve(remoteStatus); diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestSingleUGIResolver.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestSingleUGIResolver.java index 9aef10637160e..c242c30faaff1 100644 --- a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestSingleUGIResolver.java +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestSingleUGIResolver.java @@ -23,6 +23,11 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.permission.AclEntry; +import org.apache.hadoop.fs.permission.AclEntryScope; +import org.apache.hadoop.fs.permission.AclEntryType; +import org.apache.hadoop.fs.permission.AclStatus; +import org.apache.hadoop.fs.permission.FsAction; import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.security.UserGroupInformation; @@ -93,6 +98,33 @@ public void testDefault() { assertEquals(user, ids.get(1)); } + @Test + public void testAclResolution() { + long perm; + + FsPermission p1 = new FsPermission((short)0755); + FileStatus fileStatus = file("dingo", "dingo", p1); + perm = ugi.getPermissionsProto(fileStatus, null); + match(perm, p1); + + AclEntry aclEntry = new AclEntry.Builder() + .setType(AclEntryType.USER) + .setScope(AclEntryScope.ACCESS) + .setPermission(FsAction.ALL) + .setName("dingo") + .build(); + + AclStatus aclStatus = new AclStatus.Builder() + .owner("dingo") + .group(("dingo")) + .addEntry(aclEntry) + .setPermission(p1) + .build(); + + perm = ugi.getPermissionsProto(null, aclStatus); + match(perm, p1); + } + @Test(expected=IllegalArgumentException.class) public void testInvalidUid() { Configuration conf = ugi.getConf(); From 3afebacae6579d02e286b3635c2fb5b8584c27df Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Mon, 23 Sep 2019 15:13:24 -0700 Subject: [PATCH 08/12] Fix checkstyle issues --- .../org/apache/hadoop/hdfs/server/namenode/TreePath.java | 6 ++++-- .../apache/hadoop/hdfs/server/namenode/UGIResolver.java | 9 ++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java index dcd8bf0cbc689..f4737ab1dd1ce 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java @@ -159,7 +159,8 @@ INode toFile(UGIResolver ugi, BlockResolver blk, } } if (aclStatus != null) { - throw new UnsupportedOperationException("ACLs not supported by ImageWriter"); + throw new UnsupportedOperationException( + "ACLs not supported by ImageWriter"); } //TODO: storage policy should be configurable per path; use BlockResolver long off = 0L; @@ -187,7 +188,8 @@ INode toDirectory(UGIResolver ugi) { .setDsQuota(DEFAULT_STORAGE_SPACE_QUOTA) .setPermission(permissions); if (aclStatus != null) { - throw new UnsupportedOperationException("ACLs not supported by ImageWriter"); + throw new UnsupportedOperationException( + "ACLs not supported by ImageWriter"); } INode.Builder ib = INode.newBuilder() .setType(INode.Type.DIRECTORY) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java index dfd2029e2593e..43ae1bbed61b9 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java @@ -123,14 +123,16 @@ public long resolve(FileStatus s) { String resolvedGroup = group(s.getGroup()); String resolvedOwner = user(s.getOwner()); FsPermission resolvedPermission = permission(s.getPermission()); - return buildPermissionStatus(resolvedOwner, resolvedGroup, resolvedPermission.toShort()); + return buildPermissionStatus( + resolvedOwner, resolvedGroup, resolvedPermission.toShort()); } private long resolve(AclStatus aclStatus) { String resolvedOwner = user(aclStatus.getOwner()); String resolvedGroup = group(aclStatus.getGroup()); FsPermission resolvedPermision = permission(aclStatus.getPermission()); - return buildPermissionStatus(resolvedOwner, resolvedGroup, resolvedPermision.toShort()); + return buildPermissionStatus( + resolvedOwner, resolvedGroup, resolvedPermision.toShort()); } protected String user(String s) { @@ -154,7 +156,8 @@ public FsPermission permission(FsPermission s) { * @param remoteAcl AclStatus on external store. * @return serialized, local permissions the FileStatus or AclStatus map to. */ - public long getPermissionsProto(FileStatus remoteStatus, AclStatus remoteAcl) { + public long getPermissionsProto(FileStatus remoteStatus, + AclStatus remoteAcl) { addUGI(remoteStatus, remoteAcl); if (remoteAcl == null) { return resolve(remoteStatus); From c148e4ed4fcaa75bf71e8cbfc22565db5bf8dac5 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Mon, 23 Sep 2019 18:20:24 -0700 Subject: [PATCH 09/12] Terminate mounting if desired acls are unavailable --- .../hdfs/server/namenode/FSTreeWalk.java | 11 ++--------- .../hdfs/server/namenode/TestFSTreeWalk.java | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java index 3a870208b4b47..d5f7f3258ba00 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java @@ -57,7 +57,7 @@ public FSTreeWalk(Path root, Configuration conf) throws IOException { fs = root.getFileSystem(conf); boolean mountACLsEnabled = conf.getBoolean(DFS_PROVIDED_ACLS_IMPORT_ENABLED, - DFS_PROVIDED_ACLS_IMPORT_ENABLED_DEFAULT); + DFS_PROVIDED_ACLS_IMPORT_ENABLED_DEFAULT); boolean localACLsEnabled = conf.getBoolean(DFS_NAMENODE_ACLS_ENABLED_KEY, DFS_NAMENODE_ACLS_ENABLED_DEFAULT); if (!localACLsEnabled && mountACLsEnabled) { @@ -121,14 +121,7 @@ public TreeIterator fork() { } private AclStatus getAclStatus(FileSystem fs, Path path) throws IOException { - if (enableACLs) { - try { - return fs.getAclStatus(path); - } catch (UnsupportedOperationException e) { - LOG.warn("Remote filesystem {} doesn't support ACLs", fs); - } - } - return null; + return enableACLs ? fs.getAclStatus(path) : null; } @Override diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java index b02f6ebee624d..4c81fbe225dc6 100644 --- a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java @@ -76,4 +76,22 @@ public void testImportAcl() throws Exception { assertEquals(0, expectedChildren.size()); } + + @Test(expected = UnsupportedOperationException.class) + public void testACLNotSupported() throws Exception { + Configuration conf = new Configuration(); + conf.setBoolean(DFSConfigKeys.DFS_PROVIDED_ACLS_IMPORT_ENABLED, true); + + FileSystem fs = mock(FileSystem.class); + when(fs.getAclStatus(any())).thenThrow(new UnsupportedOperationException()); + Path root = mock(Path.class); + when(root.getFileSystem(conf)).thenReturn(fs); + FileStatus rootFileStatus = new FileStatus(0, true, 0, 0, 1, root); + when(fs.getFileStatus(root)).thenReturn(rootFileStatus); + + FSTreeWalk fsTreeWalk = new FSTreeWalk(root, conf); + for (TreePath treePath : fsTreeWalk) { + System.out.println(treePath.getAclStatus()); + } + } } From f2f32876145d7763d55915afd64275d583b26769 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Thu, 26 Sep 2019 11:31:39 -0700 Subject: [PATCH 10/12] Fix checkstyle issues --- .../java/org/apache/hadoop/hdfs/DFSConfigKeys.java | 3 ++- .../hadoop/hdfs/server/namenode/FSTreeWalk.java | 8 +++++--- .../hdfs/server/namenode/SingleUGIResolver.java | 1 - .../hadoop/hdfs/server/namenode/TreePath.java | 4 ++-- .../hadoop/hdfs/server/namenode/TestFSTreeWalk.java | 13 ++++++++----- 5 files changed, 17 insertions(+), 12 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java index 40d0abf613c0c..47a507dbaff8b 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java @@ -379,7 +379,8 @@ public class DFSConfigKeys extends CommonConfigurationKeys { public static final String DFS_PROVIDED_ALIASMAP_TEXT_WRITE_DIR_DEFAULT = "file:///tmp/"; public static final String DFS_PROVIDED_ALIASMAP_LEVELDB_PATH = "dfs.provided.aliasmap.leveldb.path"; - public static final String DFS_PROVIDED_ACLS_IMPORT_ENABLED = "dfs.provided.acls.import.enabled"; + public static final String DFS_PROVIDED_ACLS_IMPORT_ENABLED = + "dfs.provided.acls.import.enabled"; public static final boolean DFS_PROVIDED_ACLS_IMPORT_ENABLED_DEFAULT = false; public static final String DFS_LIST_LIMIT = "dfs.ls.limit"; diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java index d5f7f3258ba00..c1ef2312d2525 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSTreeWalk.java @@ -107,7 +107,8 @@ private FSTreeIterator() { } catch (IOException e) { throw new RuntimeException(e); } - getPendingQueue().addFirst(new TreePath(fileStatus, parentId, this, fs, acls)); + TreePath treePath = new TreePath(fileStatus, parentId, this, fs, acls); + getPendingQueue().addFirst(treePath); } @Override @@ -120,8 +121,9 @@ public TreeIterator fork() { } - private AclStatus getAclStatus(FileSystem fs, Path path) throws IOException { - return enableACLs ? fs.getAclStatus(path) : null; + private AclStatus getAclStatus(FileSystem fileSystem, Path path) + throws IOException { + return enableACLs ? fileSystem.getAclStatus(path) : null; } @Override diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/SingleUGIResolver.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/SingleUGIResolver.java index dca665e13750e..e8c7e2b5446b3 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/SingleUGIResolver.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/SingleUGIResolver.java @@ -23,7 +23,6 @@ import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.conf.Configurable; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.security.UserGroupInformation; /** diff --git a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java index f4737ab1dd1ce..0f1eda1f6b845 100644 --- a/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java +++ b/hadoop-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/TreePath.java @@ -93,8 +93,8 @@ public long getId() { return id; } - public void accept(long id) { - this.id = id; + public void accept(long pathId) { + this.id = pathId; i.onAccept(this, id); } diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java index 4c81fbe225dc6..b1ae54252fde3 100644 --- a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java @@ -30,16 +30,17 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; /** - * Validate FSTreeWalk specific behavior + * Validate FSTreeWalk specific behavior. */ public class TestFSTreeWalk { /** - * Verify that the ACLs are fetched when configured + * Verify that the ACLs are fetched when configured. */ @Test public void testImportAcl() throws Exception { @@ -55,7 +56,8 @@ public void testImportAcl() throws Exception { FileStatus child2 = new FileStatus(0, true, 0, 0, 1, new Path("/b")); expectedChildren.put(child1.getPath(), child1); expectedChildren.put(child2.getPath(), child2); - when(fs.listStatus(root)).thenReturn(expectedChildren.values().toArray(new FileStatus[1])); + when(fs.listStatus(root)) + .thenReturn(expectedChildren.values().toArray(new FileStatus[1])); AclStatus expectedAcls = mock(AclStatus.class); when(fs.getAclStatus(any(Path.class))).thenReturn(expectedAcls); @@ -67,7 +69,8 @@ public void testImportAcl() throws Exception { Iterable result = fsTreeWalk.getChildren(treePath, 1, null); for (TreePath path : result) { - FileStatus expectedChildStatus = expectedChildren.remove(path.getFileStatus().getPath()); + FileStatus expectedChildStatus + = expectedChildren.remove(path.getFileStatus().getPath()); assertNotNull(expectedChildStatus); AclStatus childAcl = path.getAclStatus(); @@ -91,7 +94,7 @@ public void testACLNotSupported() throws Exception { FSTreeWalk fsTreeWalk = new FSTreeWalk(root, conf); for (TreePath treePath : fsTreeWalk) { - System.out.println(treePath.getAclStatus()); + fail("Unexpected successful traversal of remote FS: " + treePath); } } } From 1bd5ee0d43bbb8fb49c23dfd632d8b3361e23aef Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Thu, 10 Oct 2019 21:49:17 -0700 Subject: [PATCH 11/12] Add unit test for image generation tool with acls --- .../hdfs/server/namenode/TestFSTreeWalk.java | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java index b1ae54252fde3..932ef57bc78fc 100644 --- a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java @@ -80,6 +80,10 @@ public void testImportAcl() throws Exception { assertEquals(0, expectedChildren.size()); } + /** + * Verify ACL enabled TreeWalk iterator throws an error if the external file + * system does not support ACLs. + */ @Test(expected = UnsupportedOperationException.class) public void testACLNotSupported() throws Exception { Configuration conf = new Configuration(); @@ -93,8 +97,24 @@ public void testACLNotSupported() throws Exception { when(fs.getFileStatus(root)).thenReturn(rootFileStatus); FSTreeWalk fsTreeWalk = new FSTreeWalk(root, conf); - for (TreePath treePath : fsTreeWalk) { - fail("Unexpected successful traversal of remote FS: " + treePath); - } + TreeWalk.TreeIterator iter = fsTreeWalk.iterator(); + fail("Unexpected successful creation of iter: " + iter); + } + + /** + * Verify creation of INode for ACL enabled TreePath throws an error. + */ + @Test(expected = UnsupportedOperationException.class) + public void testToINodeACLNotSupported() throws Exception { + BlockResolver blockResolver = new FixedBlockResolver(); + Path root = new Path("/"); + FileStatus rootFileStatus = new FileStatus(0, false, 0, 0, 1, root); + + AclStatus acls = mock(AclStatus.class); + TreePath treePath = new TreePath(rootFileStatus, 1, null, null, acls); + + UGIResolver ugiResolver = mock(UGIResolver.class); + when(ugiResolver.getPermissionsProto(null, acls)).thenReturn(1l); + treePath.toINode(ugiResolver, blockResolver, null); } } From 0a3d26247e3ef6f994933badd6744c8a6851a9f9 Mon Sep 17 00:00:00 2001 From: Ashvin Agrawal Date: Fri, 11 Oct 2019 15:33:26 -0700 Subject: [PATCH 12/12] Fix minor checkstyle issue --- .../org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java index 932ef57bc78fc..19e52cf43a9ee 100644 --- a/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java +++ b/hadoop-tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java @@ -114,7 +114,7 @@ public void testToINodeACLNotSupported() throws Exception { TreePath treePath = new TreePath(rootFileStatus, 1, null, null, acls); UGIResolver ugiResolver = mock(UGIResolver.class); - when(ugiResolver.getPermissionsProto(null, acls)).thenReturn(1l); + when(ugiResolver.getPermissionsProto(null, acls)).thenReturn(1L); treePath.toINode(ugiResolver, blockResolver, null); } }