From fad8620a71e12bd291c5ee346d34d7d0a24d2f36 Mon Sep 17 00:00:00 2001 From: Andrew Wang Date: Thu, 10 Apr 2014 22:35:07 +0000 Subject: [PATCH] HDFS-6224. Add a unit test to TestAuditLogger for file permissions passed to logAuditEvent. Contributed by Charles Lamb. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1586490 13f79535-47bb-0310-9956-ffa450edef68 --- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 3 ++ .../hdfs/server/namenode/FSNamesystem.java | 24 +++++++---- .../hdfs/server/namenode/TestAuditLogger.java | 42 +++++++++++++++++++ 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 655ec8ca124..b62f05fcc1c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -292,6 +292,9 @@ Release 2.5.0 - UNRELEASED HDFS-6225. Remove the o.a.h.hdfs.server.common.UpgradeStatusReport. (wheat9) + + HDFS-6224. Add a unit test to TestAuditLogger for file permissions + passed to logAuditEvent. (Charles Lamb via wang) OPTIMIZATIONS 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 16c2df9ce4f..f4d609fbe87 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 @@ -7336,6 +7336,7 @@ public class FSNamesystem implements Namesystem, FSClusterStats, cacheManager.waitForRescanIfNeeded(); } writeLock(); + String effectiveDirectiveStr = null; Long result = null; try { checkOperation(OperationCategory.WRITE); @@ -7347,11 +7348,12 @@ public class FSNamesystem implements Namesystem, FSClusterStats, throw new IOException("addDirective: you cannot specify an ID " + "for this operation."); } - CacheDirectiveInfo effectiveDirective = + CacheDirectiveInfo effectiveDirective = cacheManager.addDirective(directive, pc, flags); getEditLog().logAddCacheDirectiveInfo(effectiveDirective, cacheEntry != null); result = effectiveDirective.getId(); + effectiveDirectiveStr = effectiveDirective.toString(); success = true; } finally { writeUnlock(); @@ -7359,7 +7361,7 @@ public class FSNamesystem implements Namesystem, FSClusterStats, getEditLog().logSync(); } if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(success, "addCacheDirective", null, null, null); + logAuditEvent(success, "addCacheDirective", effectiveDirectiveStr, null, null); } RetryCache.setState(cacheEntry, success, result); } @@ -7396,7 +7398,8 @@ public class FSNamesystem implements Namesystem, FSClusterStats, getEditLog().logSync(); } if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(success, "modifyCacheDirective", null, null, null); + String idStr = "{id: " + directive.getId().toString() + "}"; + logAuditEvent(success, "modifyCacheDirective", idStr, directive.toString(), null); } RetryCache.setState(cacheEntry, success); } @@ -7424,7 +7427,8 @@ public class FSNamesystem implements Namesystem, FSClusterStats, } finally { writeUnlock(); if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(success, "removeCacheDirective", null, null, + String idStr = "{id: " + id.toString() + "}"; + logAuditEvent(success, "removeCacheDirective", idStr, null, null); } RetryCache.setState(cacheEntry, success); @@ -7449,7 +7453,7 @@ public class FSNamesystem implements Namesystem, FSClusterStats, } finally { readUnlock(); if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(success, "listCacheDirectives", null, null, + logAuditEvent(success, "listCacheDirectives", filter.toString(), null, null); } } @@ -7466,6 +7470,7 @@ public class FSNamesystem implements Namesystem, FSClusterStats, } writeLock(); boolean success = false; + String poolInfoStr = null; try { checkOperation(OperationCategory.WRITE); if (isInSafeMode()) { @@ -7476,12 +7481,13 @@ public class FSNamesystem implements Namesystem, FSClusterStats, pc.checkSuperuserPrivilege(); } CachePoolInfo info = cacheManager.addCachePool(req); + poolInfoStr = info.toString(); getEditLog().logAddCachePool(info, cacheEntry != null); success = true; } finally { writeUnlock(); if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(success, "addCachePool", req.getPoolName(), null, null); + logAuditEvent(success, "addCachePool", poolInfoStr, null, null); } RetryCache.setState(cacheEntry, success); } @@ -7514,7 +7520,8 @@ public class FSNamesystem implements Namesystem, FSClusterStats, } finally { writeUnlock(); if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(success, "modifyCachePool", req.getPoolName(), null, null); + String poolNameStr = "{poolName: " + req.getPoolName() + "}"; + logAuditEvent(success, "modifyCachePool", poolNameStr, req.toString(), null); } RetryCache.setState(cacheEntry, success); } @@ -7547,7 +7554,8 @@ public class FSNamesystem implements Namesystem, FSClusterStats, } finally { writeUnlock(); if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(success, "removeCachePool", cachePoolName, null, null); + String poolNameStr = "{poolName: " + cachePoolName + "}"; + logAuditEvent(success, "removeCachePool", poolNameStr, null, null); } RetryCache.setState(cacheEntry, success); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java index 3de27cb2746..f20d51a1dfd 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java @@ -29,6 +29,7 @@ import java.net.InetAddress; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hdfs.HdfsConfiguration; import org.apache.hadoop.hdfs.MiniDFSCluster; @@ -42,6 +43,8 @@ import org.junit.Test; */ public class TestAuditLogger { + private static final short TEST_PERMISSION = (short) 0654; + /** * Tests that AuditLogger works as expected. */ @@ -55,6 +58,7 @@ public class TestAuditLogger { try { cluster.waitClusterUp(); assertTrue(DummyAuditLogger.initialized); + DummyAuditLogger.resetLogCount(); FileSystem fs = cluster.getFileSystem(); long time = System.currentTimeMillis(); @@ -65,6 +69,36 @@ public class TestAuditLogger { } } + /** + * Minor test related to HADOOP-9155. Verify that during a + * FileSystem.setPermission() operation, the stat passed in during the + * logAuditEvent() call returns the new permission rather than the old + * permission. + */ + @Test + public void testAuditLoggerWithSetPermission() throws IOException { + Configuration conf = new HdfsConfiguration(); + conf.set(DFS_NAMENODE_AUDIT_LOGGERS_KEY, + DummyAuditLogger.class.getName()); + MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).build(); + + try { + cluster.waitClusterUp(); + assertTrue(DummyAuditLogger.initialized); + DummyAuditLogger.resetLogCount(); + + FileSystem fs = cluster.getFileSystem(); + long time = System.currentTimeMillis(); + final Path p = new Path("/"); + fs.setTimes(p, time, time); + fs.setPermission(p, new FsPermission(TEST_PERMISSION)); + assertEquals(TEST_PERMISSION, DummyAuditLogger.foundPermission); + assertEquals(2, DummyAuditLogger.logCount); + } finally { + cluster.shutdown(); + } + } + /** * Tests that a broken audit logger causes requests to fail. */ @@ -93,15 +127,23 @@ public class TestAuditLogger { static boolean initialized; static int logCount; + static short foundPermission; public void initialize(Configuration conf) { initialized = true; } + public static void resetLogCount() { + logCount = 0; + } + public void logAuditEvent(boolean succeeded, String userName, InetAddress addr, String cmd, String src, String dst, FileStatus stat) { logCount++; + if (stat != null) { + foundPermission = stat.getPermission().toShort(); + } } }