From 5343b43fd989ec596afed807ddce29ad96c23e2d Mon Sep 17 00:00:00 2001 From: Uma Maheswara Rao G Date: Wed, 23 Jul 2014 19:05:11 +0000 Subject: [PATCH] HDFS-6422. getfattr in CLI doesn't throw exception or return non-0 return code when xattr doesn't exist. (Charles Lamb via umamahesh) git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1612922 13f79535-47bb-0310-9956-ffa450edef68 --- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 3 + .../hadoop/hdfs/protocol/ClientProtocol.java | 2 +- .../hdfs/server/namenode/FSEditLog.java | 3 +- .../hdfs/server/namenode/FSEditLogLoader.java | 4 + .../hdfs/server/namenode/FSEditLogOp.java | 5 + .../hdfs/server/namenode/FSNamesystem.java | 62 +++- .../namenode/XAttrPermissionFilter.java | 17 +- .../hdfs/web/resources/XAttrNameParam.java | 4 +- .../org/apache/hadoop/hdfs/TestDFSShell.java | 69 ++++ .../hdfs/server/namenode/FSXAttrBaseTest.java | 351 ++++++++++++++++-- .../namenode/TestNamenodeRetryCache.java | 4 +- .../namenode/ha/TestRetryCacheWithHA.java | 54 ++- .../hadoop/hdfs/web/resources/TestParam.java | 6 - .../src/test/resources/editsStored | Bin 4970 -> 4992 bytes .../src/test/resources/editsStored.xml | 2 + 15 files changed, 534 insertions(+), 52 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 8c172bf03cf..4fbe4cfc283 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -892,6 +892,9 @@ Release 2.5.0 - UNRELEASED HDFS-6703. NFS: Files can be deleted from a read-only mount (Srikanth Upputuri via brandonli) + HDFS-6422. getfattr in CLI doesn't throw exception or return non-0 return code + when xattr doesn't exist. (Charles Lamb via umamahesh) + BREAKDOWN OF HDFS-2006 SUBTASKS AND RELATED JIRAS HDFS-6299. Protobuf for XAttr and client-side implementation. (Yi Liu via umamahesh) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java index ad331d1e755..9398c721a68 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java @@ -1337,6 +1337,6 @@ public List listXAttrs(String src) * @param xAttr XAttr to remove * @throws IOException */ - @Idempotent + @AtMostOnce public void removeXAttr(String src, XAttr xAttr) throws IOException; } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java index 85cfc1c7746..b2adcd455fc 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java @@ -1074,10 +1074,11 @@ void logSetXAttrs(String src, List xAttrs, boolean toLogRpcIds) { logEdit(op); } - void logRemoveXAttrs(String src, List xAttrs) { + void logRemoveXAttrs(String src, List xAttrs, boolean toLogRpcIds) { final RemoveXAttrOp op = RemoveXAttrOp.getInstance(); op.src = src; op.xAttrs = xAttrs; + logRpcIds(op, toLogRpcIds); logEdit(op); } 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 858cd57b23f..a721491948d 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 @@ -821,6 +821,10 @@ private long applyEditLogOp(FSEditLogOp op, FSDirectory fsDir, RemoveXAttrOp removeXAttrOp = (RemoveXAttrOp) op; fsDir.unprotectedRemoveXAttrs(removeXAttrOp.src, removeXAttrOp.xAttrs); + if (toAddRetryCache) { + fsNamesys.addCacheEntry(removeXAttrOp.rpcClientId, + removeXAttrOp.rpcCallId); + } break; } default: diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java index e972799b335..5543e0cb86e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java @@ -3551,6 +3551,7 @@ void readFields(DataInputStream in, int logVersion) throws IOException { XAttrEditLogProto p = XAttrEditLogProto.parseDelimitedFrom(in); src = p.getSrc(); xAttrs = PBHelper.convertXAttrs(p.getXAttrsList()); + readRpcIds(in, logVersion); } @Override @@ -3561,18 +3562,22 @@ public void writeFields(DataOutputStream out) throws IOException { } b.addAllXAttrs(PBHelper.convertXAttrProto(xAttrs)); b.build().writeDelimitedTo(out); + // clientId and callId + writeRpcIds(rpcClientId, rpcCallId, out); } @Override protected void toXml(ContentHandler contentHandler) throws SAXException { XMLUtils.addSaxString(contentHandler, "SRC", src); appendXAttrsToXml(contentHandler, xAttrs); + appendRpcIdsToXml(contentHandler, rpcClientId, rpcCallId); } @Override void fromXml(Stanza st) throws InvalidXmlException { src = st.getValue("SRC"); xAttrs = readXAttrsFromXml(st); + readRpcIdsFromXml(st); } } 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 3e922ba023c..6ce6a70ce2b 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 @@ -8279,11 +8279,12 @@ List getXAttrs(String src, List xAttrs) throws IOException { nnConf.checkXAttrsConfigFlag(); FSPermissionChecker pc = getPermissionChecker(); boolean getAll = xAttrs == null || xAttrs.isEmpty(); - List filteredXAttrs = null; if (!getAll) { - filteredXAttrs = XAttrPermissionFilter.filterXAttrsForApi(pc, xAttrs); - if (filteredXAttrs.isEmpty()) { - return filteredXAttrs; + try { + XAttrPermissionFilter.checkPermissionForApi(pc, xAttrs); + } catch (AccessControlException e) { + logAuditEvent(false, "getXAttrs", src); + throw e; } } checkOperation(OperationCategory.READ); @@ -8302,15 +8303,21 @@ List getXAttrs(String src, List xAttrs) throws IOException { if (filteredAll == null || filteredAll.isEmpty()) { return null; } - List toGet = Lists.newArrayListWithCapacity(filteredXAttrs.size()); - for (XAttr xAttr : filteredXAttrs) { + List toGet = Lists.newArrayListWithCapacity(xAttrs.size()); + for (XAttr xAttr : xAttrs) { + boolean foundIt = false; for (XAttr a : filteredAll) { if (xAttr.getNameSpace() == a.getNameSpace() && xAttr.getName().equals(a.getName())) { toGet.add(a); + foundIt = true; break; } } + if (!foundIt) { + throw new IOException( + "At least one of the attributes provided was not found."); + } } return toGet; } @@ -8344,17 +8351,42 @@ List listXAttrs(String src) throws IOException { readUnlock(); } } - + + /** + * Remove an xattr for a file or directory. + * + * @param src + * - path to remove the xattr from + * @param xAttr + * - xAttr to remove + * @throws AccessControlException + * @throws SafeModeException + * @throws UnresolvedLinkException + * @throws IOException + */ void removeXAttr(String src, XAttr xAttr) throws IOException { - nnConf.checkXAttrsConfigFlag(); - HdfsFileStatus resultingStat = null; - FSPermissionChecker pc = getPermissionChecker(); + CacheEntry cacheEntry = RetryCache.waitForCompletion(retryCache); + if (cacheEntry != null && cacheEntry.isSuccess()) { + return; // Return previous response + } + boolean success = false; try { - XAttrPermissionFilter.checkPermissionForApi(pc, xAttr); + removeXAttrInt(src, xAttr, cacheEntry != null); + success = true; } catch (AccessControlException e) { logAuditEvent(false, "removeXAttr", src); throw e; + } finally { + RetryCache.setState(cacheEntry, success); } + } + + void removeXAttrInt(String src, XAttr xAttr, boolean logRetryCache) + throws IOException { + nnConf.checkXAttrsConfigFlag(); + HdfsFileStatus resultingStat = null; + FSPermissionChecker pc = getPermissionChecker(); + XAttrPermissionFilter.checkPermissionForApi(pc, xAttr); checkOperation(OperationCategory.WRITE); byte[][] pathComponents = FSDirectory.getPathComponentsForReservedPath(src); writeLock(); @@ -8368,12 +8400,12 @@ void removeXAttr(String src, XAttr xAttr) throws IOException { xAttrs.add(xAttr); List removedXAttrs = dir.removeXAttrs(src, xAttrs); if (removedXAttrs != null && !removedXAttrs.isEmpty()) { - getEditLog().logRemoveXAttrs(src, removedXAttrs); + getEditLog().logRemoveXAttrs(src, removedXAttrs, logRetryCache); + } else { + throw new IOException( + "No matching attributes found for remove operation"); } resultingStat = getAuditFileInfo(src, false); - } catch (AccessControlException e) { - logAuditEvent(false, "removeXAttr", src); - throw e; } finally { writeUnlock(); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java index 47f29399e5a..98730142fbd 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/XAttrPermissionFilter.java @@ -26,6 +26,7 @@ import org.apache.hadoop.security.AccessControlException; import com.google.common.collect.Lists; +import com.google.common.base.Preconditions; /** * There are four types of extended attributes defined by the @@ -60,8 +61,20 @@ static void checkPermissionForApi(FSPermissionChecker pc, XAttr xAttr) throw new AccessControlException("User doesn't have permission for xattr: " + XAttrHelper.getPrefixName(xAttr)); } - - static List filterXAttrsForApi(FSPermissionChecker pc, + + static void checkPermissionForApi(FSPermissionChecker pc, + List xAttrs) throws AccessControlException { + Preconditions.checkArgument(xAttrs != null); + if (xAttrs.isEmpty()) { + return; + } + + for (XAttr xAttr : xAttrs) { + checkPermissionForApi(pc, xAttr); + } + } + + static List filterXAttrsForApi(FSPermissionChecker pc, List xAttrs) { assert xAttrs != null : "xAttrs can not be null"; if (xAttrs == null || xAttrs.isEmpty()) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/resources/XAttrNameParam.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/resources/XAttrNameParam.java index 3860f916e48..8137b4494f4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/resources/XAttrNameParam.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/resources/XAttrNameParam.java @@ -25,8 +25,8 @@ public class XAttrNameParam extends StringParam { /** Default parameter value. **/ public static final String DEFAULT = ""; - private static Domain DOMAIN = new Domain(NAME, - Pattern.compile("^(user\\.|trusted\\.|system\\.|security\\.).+")); + private static Domain DOMAIN = new Domain(NAME, + Pattern.compile(".*")); public XAttrNameParam(final String str) { super(DOMAIN, str == null || str.equals(DEFAULT) ? null : str); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java index 8eb1c41e052..aac16f4e5e3 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java @@ -2653,6 +2653,75 @@ public Object run() throws Exception { } } + /* + * 1. Test that CLI throws an exception and returns non-0 when user does + * not have permission to read an xattr. + * 2. Test that CLI throws an exception and returns non-0 when a non-existent + * xattr is requested. + */ + @Test (timeout = 120000) + public void testGetFAttrErrors() throws Exception { + final UserGroupInformation user = UserGroupInformation. + createUserForTesting("user", new String[] {"mygroup"}); + MiniDFSCluster cluster = null; + PrintStream bakErr = null; + try { + final Configuration conf = new HdfsConfiguration(); + cluster = new MiniDFSCluster.Builder(conf).numDataNodes(1).build(); + cluster.waitActive(); + + final FileSystem fs = cluster.getFileSystem(); + final Path p = new Path("/foo"); + fs.mkdirs(p); + bakErr = System.err; + + final FsShell fshell = new FsShell(conf); + final ByteArrayOutputStream out = new ByteArrayOutputStream(); + System.setErr(new PrintStream(out)); + + // No permission for "other". + fs.setPermission(p, new FsPermission((short) 0700)); + + { + final int ret = ToolRunner.run(fshell, new String[] { + "-setfattr", "-n", "user.a1", "-v", "1234", "/foo"}); + assertEquals("Returned should be 0", 0, ret); + out.reset(); + } + + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + int ret = ToolRunner.run(fshell, new String[] { + "-getfattr", "-n", "user.a1", "/foo"}); + String str = out.toString(); + assertTrue("xattr value was incorrectly returned", + str.indexOf("1234") == -1); + out.reset(); + return null; + } + }); + + { + final int ret = ToolRunner.run(fshell, new String[]{ + "-getfattr", "-n", "user.nonexistent", "/foo"}); + String str = out.toString(); + assertTrue("xattr value was incorrectly returned", + str.indexOf( + "getfattr: At least one of the attributes provided was not found") + >= 0); + out.reset(); + } + } finally { + if (bakErr != null) { + System.setErr(bakErr); + } + if (cluster != null) { + cluster.shutdown(); + } + } + } + /** * Test that the server trash configuration is respected when * the client configuration is not set. diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSXAttrBaseTest.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSXAttrBaseTest.java index 86f1ec90b8b..636ecc2417f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSXAttrBaseTest.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSXAttrBaseTest.java @@ -19,7 +19,6 @@ import java.io.FileNotFoundException; import java.io.IOException; -import java.io.FileNotFoundException; import java.security.PrivilegedExceptionAction; import java.util.EnumSet; import java.util.List; @@ -46,6 +45,7 @@ import static org.apache.hadoop.fs.permission.FsAction.ALL; import static org.apache.hadoop.fs.permission.FsAction.READ; import static org.apache.hadoop.hdfs.server.namenode.AclTestHelpers.aclEntry; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import org.junit.After; @@ -261,11 +261,12 @@ public void testSetXAttr() throws Exception { fs.setXAttr(path, "user.", value1, EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE)); Assert.fail("Setting xattr with empty name should fail."); + } catch (RemoteException e) { + assertEquals("Unexpected RemoteException: " + e, e.getClassName(), + HadoopIllegalArgumentException.class.getCanonicalName()); + GenericTestUtils.assertExceptionContains("XAttr name cannot be empty", e); } catch (HadoopIllegalArgumentException e) { GenericTestUtils.assertExceptionContains("XAttr name cannot be empty", e); - } catch (IllegalArgumentException e) { - GenericTestUtils.assertExceptionContains("Invalid value: \"user.\" does " + - "not belong to the domain ^(user\\.|trusted\\.|system\\.|security\\.).+", e); } // Set xattr with invalid name: "a1" @@ -274,11 +275,12 @@ public void testSetXAttr() throws Exception { XAttrSetFlag.REPLACE)); Assert.fail("Setting xattr with invalid name prefix or without " + "name prefix should fail."); + } catch (RemoteException e) { + assertEquals("Unexpected RemoteException: " + e, e.getClassName(), + HadoopIllegalArgumentException.class.getCanonicalName()); + GenericTestUtils.assertExceptionContains("XAttr name must be prefixed", e); } catch (HadoopIllegalArgumentException e) { GenericTestUtils.assertExceptionContains("XAttr name must be prefixed", e); - } catch (IllegalArgumentException e) { - GenericTestUtils.assertExceptionContains("Invalid value: \"a1\" does " + - "not belong to the domain ^(user\\.|trusted\\.|system\\.|security\\.).+", e); } // Set xattr without XAttrSetFlag @@ -341,9 +343,18 @@ public void testSetXAttr() throws Exception { } /** - * Tests for getting xattr - * 1. To get xattr which does not exist. - * 2. To get multiple xattrs. + * getxattr tests. Test that getxattr throws an exception if any of + * the following are true: + * an xattr that was requested doesn't exist + * the caller specifies an unknown namespace + * the caller doesn't have access to the namespace + * the caller doesn't have permission to get the value of the xattr + * the caller does not have search access to the parent directory + * the caller has only read access to the owning directory + * the caller has only search access to the owning directory and + * execute/search access to the actual entity + * the caller does not have search access to the owning directory and read + * access to the actual entity */ @Test(timeout = 120000) public void testGetXAttrs() throws Exception { @@ -351,21 +362,159 @@ public void testGetXAttrs() throws Exception { fs.setXAttr(path, name1, value1, EnumSet.of(XAttrSetFlag.CREATE)); fs.setXAttr(path, name2, value2, EnumSet.of(XAttrSetFlag.CREATE)); - // XAttr does not exist. - byte[] value = fs.getXAttr(path, name3); - Assert.assertEquals(value, null); + /* An XAttr that was requested does not exist. */ + try { + final byte[] value = fs.getXAttr(path, name3); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains( + "At least one of the attributes provided was not found.", e); + } - List names = Lists.newArrayList(); - names.add(name1); - names.add(name2); - names.add(name3); - Map xattrs = fs.getXAttrs(path, names); - Assert.assertEquals(xattrs.size(), 2); - Assert.assertArrayEquals(value1, xattrs.get(name1)); - Assert.assertArrayEquals(value2, xattrs.get(name2)); + /* Throw an exception if an xattr that was requested does not exist. */ + { + final List names = Lists.newArrayList(); + names.add(name1); + names.add(name2); + names.add(name3); + try { + final Map xattrs = fs.getXAttrs(path, names); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains( + "At least one of the attributes provided was not found.", e); + } + } fs.removeXAttr(path, name1); fs.removeXAttr(path, name2); + + /* Unknown namespace should throw an exception. */ + try { + final byte[] xattr = fs.getXAttr(path, "wackynamespace.foo"); + Assert.fail("expected IOException"); + } catch (Exception e) { + GenericTestUtils.assertExceptionContains + ("An XAttr name must be prefixed with user/trusted/security/system, " + + "followed by a '.'", + e); + } + + /* + * The 'trusted' namespace should not be accessible and should throw an + * exception. + */ + final UserGroupInformation user = UserGroupInformation. + createUserForTesting("user", new String[] {"mygroup"}); + fs.setXAttr(path, "trusted.foo", "1234".getBytes()); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + final byte[] xattr = userFs.getXAttr(path, "trusted.foo"); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("User doesn't have permission", e); + } + + fs.setXAttr(path, name1, "1234".getBytes()); + + /* + * Test that an exception is thrown if the caller doesn't have permission to + * get the value of the xattr. + */ + + /* Set access so that only the owner has access. */ + fs.setPermission(path, new FsPermission((short) 0700)); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + final byte[] xattr = userFs.getXAttr(path, name1); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("Permission denied", e); + } + + /* + * The caller must have search access to the parent directory. + */ + final Path childDir = new Path(path, "child" + pathCount); + /* Set access to parent so that only the owner has access. */ + FileSystem.mkdirs(fs, childDir, FsPermission.createImmutable((short)0700)); + fs.setXAttr(childDir, name1, "1234".getBytes()); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + final byte[] xattr = userFs.getXAttr(childDir, name1); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("Permission denied", e); + } + + /* Check that read access to the owning directory is not good enough. */ + fs.setPermission(path, new FsPermission((short) 0704)); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + final byte[] xattr = userFs.getXAttr(childDir, name1); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("Permission denied", e); + } + + /* + * Check that search access to the owning directory and search/execute + * access to the actual entity with extended attributes is not good enough. + */ + fs.setPermission(path, new FsPermission((short) 0701)); + fs.setPermission(childDir, new FsPermission((short) 0701)); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + final byte[] xattr = userFs.getXAttr(childDir, name1); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("Permission denied", e); + } + + /* + * Check that search access to the owning directory and read access to + * the actual entity with the extended attribute is good enough. + */ + fs.setPermission(path, new FsPermission((short) 0701)); + fs.setPermission(childDir, new FsPermission((short) 0704)); + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + final byte[] xattr = userFs.getXAttr(childDir, name1); + return null; + } + }); } /** @@ -402,6 +551,166 @@ public void testRemoveXAttr() throws Exception { fs.removeXAttr(path, name3); } + /** + * removexattr tests. Test that removexattr throws an exception if any of + * the following are true: + * an xattr that was requested doesn't exist + * the caller specifies an unknown namespace + * the caller doesn't have access to the namespace + * the caller doesn't have permission to get the value of the xattr + * the caller does not have "execute" (scan) access to the parent directory + * the caller has only read access to the owning directory + * the caller has only execute access to the owning directory and execute + * access to the actual entity + * the caller does not have execute access to the owning directory and write + * access to the actual entity + */ + @Test(timeout = 120000) + public void testRemoveXAttrPermissions() throws Exception { + FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short)0750)); + fs.setXAttr(path, name1, value1, EnumSet.of(XAttrSetFlag.CREATE)); + fs.setXAttr(path, name2, value2, EnumSet.of(XAttrSetFlag.CREATE)); + fs.setXAttr(path, name3, null, EnumSet.of(XAttrSetFlag.CREATE)); + + try { + fs.removeXAttr(path, name2); + fs.removeXAttr(path, name2); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("No matching attributes found", e); + } + + /* Unknown namespace should throw an exception. */ + final String expectedExceptionString = "An XAttr name must be prefixed " + + "with user/trusted/security/system, followed by a '.'"; + try { + fs.removeXAttr(path, "wackynamespace.foo"); + Assert.fail("expected IOException"); + } catch (RemoteException e) { + assertEquals("Unexpected RemoteException: " + e, e.getClassName(), + HadoopIllegalArgumentException.class.getCanonicalName()); + GenericTestUtils.assertExceptionContains(expectedExceptionString, e); + } catch (HadoopIllegalArgumentException e) { + GenericTestUtils.assertExceptionContains(expectedExceptionString, e); + } + + /* + * The 'trusted' namespace should not be accessible and should throw an + * exception. + */ + final UserGroupInformation user = UserGroupInformation. + createUserForTesting("user", new String[] {"mygroup"}); + fs.setXAttr(path, "trusted.foo", "1234".getBytes()); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + userFs.removeXAttr(path, "trusted.foo"); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("User doesn't have permission", e); + } finally { + fs.removeXAttr(path, "trusted.foo"); + } + + /* + * Test that an exception is thrown if the caller doesn't have permission to + * get the value of the xattr. + */ + + /* Set access so that only the owner has access. */ + fs.setPermission(path, new FsPermission((short) 0700)); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + userFs.removeXAttr(path, name1); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("Permission denied", e); + } + + /* + * The caller must have "execute" (scan) access to the parent directory. + */ + final Path childDir = new Path(path, "child" + pathCount); + /* Set access to parent so that only the owner has access. */ + FileSystem.mkdirs(fs, childDir, FsPermission.createImmutable((short)0700)); + fs.setXAttr(childDir, name1, "1234".getBytes()); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + userFs.removeXAttr(childDir, name1); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("Permission denied", e); + } + + /* Check that read access to the owning directory is not good enough. */ + fs.setPermission(path, new FsPermission((short) 0704)); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + userFs.removeXAttr(childDir, name1); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("Permission denied", e); + } + + /* + * Check that execute access to the owning directory and scan access to + * the actual entity with extended attributes is not good enough. + */ + fs.setPermission(path, new FsPermission((short) 0701)); + fs.setPermission(childDir, new FsPermission((short) 0701)); + try { + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + userFs.removeXAttr(childDir, name1); + return null; + } + }); + Assert.fail("expected IOException"); + } catch (IOException e) { + GenericTestUtils.assertExceptionContains("Permission denied", e); + } + + /* + * Check that execute access to the owning directory and write access to + * the actual entity with extended attributes is good enough. + */ + fs.setPermission(path, new FsPermission((short) 0701)); + fs.setPermission(childDir, new FsPermission((short) 0706)); + user.doAs(new PrivilegedExceptionAction() { + @Override + public Object run() throws Exception { + final FileSystem userFs = dfsCluster.getFileSystem(); + userFs.removeXAttr(childDir, name1); + return null; + } + }); + } + @Test(timeout = 120000) public void testRenameFileWithXAttr() throws Exception { FileSystem.mkdirs(fs, path, FsPermission.createImmutable((short)0750)); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNamenodeRetryCache.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNamenodeRetryCache.java index d6f38853474..a0ae43b57fc 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNamenodeRetryCache.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNamenodeRetryCache.java @@ -415,7 +415,7 @@ public void testRetryCacheRebuild() throws Exception { LightWeightCache cacheSet = (LightWeightCache) namesystem.getRetryCache().getCacheSet(); - assertEquals(22, cacheSet.size()); + assertEquals(23, cacheSet.size()); Map oldEntries = new HashMap(); @@ -434,7 +434,7 @@ public void testRetryCacheRebuild() throws Exception { assertTrue(namesystem.hasRetryCache()); cacheSet = (LightWeightCache) namesystem .getRetryCache().getCacheSet(); - assertEquals(22, cacheSet.size()); + assertEquals(23, cacheSet.size()); iter = cacheSet.iterator(); while (iter.hasNext()) { CacheEntry entry = iter.next(); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestRetryCacheWithHA.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestRetryCacheWithHA.java index a34a0365f99..77f7090e8f1 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestRetryCacheWithHA.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestRetryCacheWithHA.java @@ -160,7 +160,7 @@ public void testRetryCacheOnStandbyNN() throws Exception { FSNamesystem fsn0 = cluster.getNamesystem(0); LightWeightCache cacheSet = (LightWeightCache) fsn0.getRetryCache().getCacheSet(); - assertEquals(22, cacheSet.size()); + assertEquals(23, cacheSet.size()); Map oldEntries = new HashMap(); @@ -181,7 +181,7 @@ public void testRetryCacheOnStandbyNN() throws Exception { FSNamesystem fsn1 = cluster.getNamesystem(1); cacheSet = (LightWeightCache) fsn1 .getRetryCache().getCacheSet(); - assertEquals(22, cacheSet.size()); + assertEquals(23, cacheSet.size()); iter = cacheSet.iterator(); while (iter.hasNext()) { CacheEntry entry = iter.next(); @@ -1047,6 +1047,49 @@ Object getResult() { } } + /** removeXAttr */ + class RemoveXAttrOp extends AtMostOnceOp { + private final String src; + + RemoveXAttrOp(DFSClient client, String src) { + super("removeXAttr", client); + this.src = src; + } + + @Override + void prepare() throws Exception { + Path p = new Path(src); + if (!dfs.exists(p)) { + DFSTestUtil.createFile(dfs, p, BlockSize, DataNodes, 0); + client.setXAttr(src, "user.key", "value".getBytes(), + EnumSet.of(XAttrSetFlag.CREATE)); + } + } + + @Override + void invoke() throws Exception { + client.removeXAttr(src, "user.key"); + } + + @Override + boolean checkNamenodeBeforeReturn() throws Exception { + for (int i = 0; i < CHECKTIMES; i++) { + Map iter = dfs.getXAttrs(new Path(src)); + Set keySet = iter.keySet(); + if (!keySet.contains("user.key")) { + return true; + } + Thread.sleep(1000); + } + return false; + } + + @Override + Object getResult() { + return null; + } + } + @Test (timeout=60000) public void testCreateSnapshot() throws Exception { final DFSClient client = genClientWithDummyHandler(); @@ -1183,6 +1226,13 @@ public void testSetXAttr() throws Exception { testClientRetryWithFailover(op); } + @Test (timeout=60000) + public void testRemoveXAttr() throws Exception { + DFSClient client = genClientWithDummyHandler(); + AtMostOnceOp op = new RemoveXAttrOp(client, "/removexattr"); + testClientRetryWithFailover(op); + } + /** * When NN failover happens, if the client did not receive the response and * send a retry request to the other NN, the same response should be recieved diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/resources/TestParam.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/resources/TestParam.java index 1a20739a712..a84243e7d4a 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/resources/TestParam.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/resources/TestParam.java @@ -355,12 +355,6 @@ public void testAclPermissionParam() { public void testXAttrNameParam() { final XAttrNameParam p = new XAttrNameParam("user.a1"); Assert.assertEquals(p.getXAttrName(), "user.a1"); - try { - new XAttrNameParam("a1"); - Assert.fail(); - } catch (IllegalArgumentException e) { - LOG.info("EXPECTED: " + e); - } } @Test diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/editsStored b/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/editsStored index a3561cda528a9bae7b6fb6e16180ca9a9d344d7a..a134969ca79136819a5f1e2d5e16771d54aed04a 100644 GIT binary patch delta 27 jcmaE*)}X%Oj}W87USER a2 + e03f4a52-3d85-4e05-8942-286185e639bd + 82