diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt index 0df3d2013b6..4e1dd750d0b 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES-HDFS-4685.txt @@ -72,6 +72,9 @@ HDFS-4685 (Unreleased) HDFS-5625. Write end user documentation for HDFS ACLs. (cnauroth) + HDFS-5925. ACL configuration flag must only reject ACL API calls, not ACLs + present in fsimage or edits. (cnauroth) + OPTIMIZATIONS BUG FIXES diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclConfigFlag.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclConfigFlag.java index d23771aaa9f..bfc6b4d7a4c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclConfigFlag.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclConfigFlag.java @@ -24,8 +24,7 @@ import org.apache.hadoop.hdfs.protocol.AclException; /** * Support for ACLs is controlled by a configuration flag. If the configuration - * flag is false, then the NameNode will reject all ACL-related operations and - * refuse to load an fsimage or edit log containing ACLs. + * flag is false, then the NameNode will reject all ACL-related operations. */ final class AclConfigFlag { private final boolean enabled; @@ -47,28 +46,11 @@ final class AclConfigFlag { * @throws AclException if ACLs are disabled */ public void checkForApiCall() throws AclException { - check("The ACL operation has been rejected."); - } - - /** - * Checks the flag on behalf of edit log loading. - * - * @throws AclException if ACLs are disabled - */ - public void checkForEditLog() throws AclException { - check("Cannot load edit log containing an ACL."); - } - - /** - * Common check method. - * - * @throws AclException if ACLs are disabled - */ - private void check(String reason) throws AclException { if (!enabled) { throw new AclException(String.format( - "%s Support for ACLs has been disabled by setting %s to false.", - reason, DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_KEY)); + "The ACL operation has been rejected. " + + "Support for ACLs has been disabled by setting %s to false.", + DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_KEY)); } } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java index 8f7bc0650a1..557b7f021f7 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java @@ -294,9 +294,6 @@ public class FSEditLogLoader { switch (op.opCode) { case OP_ADD: { AddCloseOp addCloseOp = (AddCloseOp)op; - if (addCloseOp.aclEntries != null) { - fsNamesys.getAclConfigFlag().checkForEditLog(); - } final String path = renameReservedPathsOnUpgrade(addCloseOp.path, logVersion); if (FSNamesystem.LOG.isDebugEnabled()) { @@ -485,9 +482,6 @@ public class FSEditLogLoader { } case OP_MKDIR: { MkdirOp mkdirOp = (MkdirOp)op; - if (mkdirOp.aclEntries != null) { - fsNamesys.getAclConfigFlag().checkForEditLog(); - } inodeId = getAndUpdateLastInodeId(mkdirOp.inodeId, logVersion, lastInodeId); fsDir.unprotectedMkdir(inodeId, @@ -749,7 +743,6 @@ public class FSEditLogLoader { break; } case OP_SET_ACL: { - fsNamesys.getAclConfigFlag().checkForEditLog(); SetAclOp setAclOp = (SetAclOp) op; fsDir.unprotectedSetAcl(setAclOp.src, setAclOp.aclEntries); break; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index 51ee21ce204..5ebe47968b3 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java @@ -7393,10 +7393,6 @@ public class FSNamesystem implements Namesystem, FSClusterStats, return results; } - AclConfigFlag getAclConfigFlag() { - return aclConfigFlag; - } - void modifyAclEntries(String src, List aclSpec) throws IOException { aclConfigFlag.checkForApiCall(); HdfsFileStatus resultingStat = null; 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 3bbc69f240e..86222422a9e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml @@ -351,11 +351,7 @@ Set to true to enable support for HDFS ACLs (Access Control Lists). By default, ACLs are disabled. When ACLs are disabled, the NameNode rejects - all attempts to set an ACL. An fsimage containing an ACL will cause the - NameNode to abort during startup, and ACLs present in the edit log will - cause the NameNode to abort. To transition from ACLs enabled to ACLs - disabled, restart the NameNode with ACLs enabled, remove all ACLs, save a - new checkpoint, and then restart the NameNode with ACLs disabled. + all RPCs related to setting or getting ACLs. diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAclConfigFlag.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAclConfigFlag.java index 0739a533efa..afb7dc528ce 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAclConfigFlag.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAclConfigFlag.java @@ -23,8 +23,6 @@ import static org.apache.hadoop.fs.permission.AclEntryType.*; import static org.apache.hadoop.fs.permission.FsAction.*; import static org.junit.Assert.*; -import java.io.IOException; - import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hdfs.DFSConfigKeys; @@ -34,7 +32,6 @@ import org.apache.hadoop.hdfs.protocol.AclException; import org.apache.hadoop.hdfs.server.namenode.NameNode; import org.apache.hadoop.hdfs.server.namenode.NameNodeAdapter; import org.apache.hadoop.io.IOUtils; -import org.apache.hadoop.test.GenericTestUtils; import org.junit.After; import org.junit.Rule; import org.junit.Test; @@ -44,9 +41,8 @@ import com.google.common.collect.Lists; /** * Tests that the configuration flag that controls support for ACLs is off by - * default and causes all attempted operations related to ACLs to fail. This - * includes the API calls, ACLs found while loading fsimage and ACLs found while - * applying edit log ops. + * default and causes all attempted operations related to ACLs to fail. The + * NameNode can still load ACLs from fsimage or edits. */ public class TestAclConfigFlag { private static final Path PATH = new Path("/path"); @@ -125,20 +121,23 @@ public class TestAclConfigFlag { fs.setAcl(PATH, Lists.newArrayList( aclEntry(DEFAULT, USER, "foo", READ_WRITE))); - // Attempt restart with ACLs disabled. - try { - restart(false, false); - fail("expected IOException"); - } catch (IOException e) { - GenericTestUtils.assertExceptionContains( - DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_KEY, e); - } + // Restart with ACLs disabled. Expect successful restart. + restart(false, false); + } - // Recover by restarting with ACLs enabled, deleting the ACL, saving a new - // checkpoint, and then restarting with ACLs disabled. - restart(false, true); - fs.removeAcl(PATH); - restart(true, false); + @Test + public void testFsImage() throws Exception { + // With ACLs enabled, set an ACL. + initCluster(true, true); + fs.mkdirs(PATH); + fs.setAcl(PATH, Lists.newArrayList( + aclEntry(DEFAULT, USER, "foo", READ_WRITE))); + + // Save a new checkpoint and restart with ACLs still enabled. + restart(true, true); + + // Restart with ACLs disabled. Expect successful restart. + restart(false, false); } /**