HDFS-12336. Listing encryption zones still fails when deleted EZ is not a direct child of snapshottable directory. Contributed by Wellington Chevreuil.

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

@@ -373,7 +373,7 @@ public class EncryptionZoneManager {
       final String pathName = getFullPathName(ezi);
       INode inode = dir.getInode(ezi.getINodeId());
       INode lastINode = null;
-      if (inode.getParent() != null || inode.isRoot()) {
+      if (INode.isValidAbsolutePath(pathName)) {
         INodesInPath iip = dir.getINodesInPath(pathName, DirOp.READ_LINK);
         lastINode = iip.getLastINode();
       }

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

@@ -782,8 +782,17 @@ public abstract class INode implements INodeAttributes, Diff.Element<byte[]> {
     return StringUtils.split(path, Path.SEPARATOR_CHAR);
   }
 
+  /**
+   * Verifies if the path informed is a valid absolute path.
+   * @param path the absolute path to validate.
+   * @return true if the path is valid.
+   */
+  static boolean isValidAbsolutePath(final String path){
+    return path != null && path.startsWith(Path.SEPARATOR);
+  }
+
   private static void checkAbsolutePath(final String path) {
-    if (path == null || !path.startsWith(Path.SEPARATOR)) {
+    if (!isValidAbsolutePath(path)) {
       throw new AssertionError("Absolute path required, but got '"
           + path + "'");
     }

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

@@ -1885,4 +1885,51 @@ public class TestEncryptionZones {
     // Read them back in and compare byte-by-byte
     verifyFilesEqual(fs, baseFile, encFile1, len);
   }
+
+  /**
+   * Test listing encryption zones after zones had been deleted,
+   * but still exist under snapshots. This test first moves EZs
+   * to trash folder, so that an inodereference is created for the EZ,
+   * then it removes the EZ from trash folder to emulate condition where
+   * the EZ inode will not be complete.
+   */
+  @Test
+  public void testListEncryptionZonesWithSnapshots() throws Exception {
+    final Path snapshottable = new Path("/zones");
+    final Path zoneDirectChild = new Path(snapshottable, "zone1");
+    final Path snapshottableChild = new Path(snapshottable, "child");
+    final Path zoneSubChild = new Path(snapshottableChild, "zone2");
+    fsWrapper.mkdir(zoneDirectChild, FsPermission.getDirDefault(),
+        true);
+    fsWrapper.mkdir(zoneSubChild, FsPermission.getDirDefault(),
+        true);
+    dfsAdmin.allowSnapshot(snapshottable);
+    dfsAdmin.createEncryptionZone(zoneDirectChild, TEST_KEY, NO_TRASH);
+    dfsAdmin.createEncryptionZone(zoneSubChild, TEST_KEY, NO_TRASH);
+    final Path snap1 = fs.createSnapshot(snapshottable, "snap1");
+    Configuration clientConf = new Configuration(conf);
+    clientConf.setLong(FS_TRASH_INTERVAL_KEY, 1);
+    FsShell shell = new FsShell(clientConf);
+    //will "trash" the zone under subfolder of snapshottable directory
+    verifyShellDeleteWithTrash(shell, snapshottableChild);
+    //permanently remove zone under subfolder of snapshottable directory
+    fsWrapper.delete(shell.getCurrentTrashDir(snapshottableChild),
+        true);
+    final RemoteIterator<EncryptionZone> it = dfsAdmin.listEncryptionZones();
+    boolean match = false;
+    while (it.hasNext()) {
+      EncryptionZone ez = it.next();
+      assertNotEquals("EncryptionZone " + zoneSubChild.toString() +
+          " should not be listed.",
+          ez.getPath(), zoneSubChild.toString());
+    }
+    //will "trash" the zone direct child of snapshottable directory
+    verifyShellDeleteWithTrash(shell, zoneDirectChild);
+    //permanently remove zone direct child of snapshottable directory
+    fsWrapper.delete(shell.getCurrentTrashDir(zoneDirectChild), true);
+    assertFalse("listEncryptionZones should not return anything, " +
+            "since both EZs were deleted.",
+        dfsAdmin.listEncryptionZones().hasNext());
+  }
+
 }

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEncryptionZoneManager.java

@@ -135,4 +135,30 @@ public class TestEncryptionZoneManager {
     assertEquals(0L, result.get(0).getId());
     assertEquals("/", result.get(0).getPath());
   }
+
+  @Test
+  public void testListEncryptionZonesSubDirInvalid() throws Exception{
+    INodeDirectory thirdINode = new INodeDirectory(3L, "third".getBytes(),
+        defaultPermission, System.currentTimeMillis());
+    when(this.mockedDir.getInode(3L)).thenReturn(thirdINode);
+    //sets "second" as parent
+    thirdINode.setParent(this.secondINode);
+    this.ezManager = new EncryptionZoneManager(mockedDir, new Configuration());
+    this.ezManager.addEncryptionZone(1L, CipherSuite.AES_CTR_NOPADDING,
+        CryptoProtocolVersion.ENCRYPTION_ZONES, "test_key");
+    this.ezManager.addEncryptionZone(3L, CipherSuite.AES_CTR_NOPADDING,
+        CryptoProtocolVersion.ENCRYPTION_ZONES, "test_key");
+    // sets root as proper parent for firstINode only,
+    // leave secondINode with no parent
+    this.firstINode.setParent(rootINode);
+    when(mockedDir.getINodesInPath("/first", DirOp.READ_LINK)).
+        thenReturn(mockedINodesInPath);
+    when(mockedINodesInPath.getLastINode()).
+        thenReturn(firstINode);
+    BatchedListEntries<EncryptionZone> result = ezManager.
+        listEncryptionZones(0);
+    assertEquals(1, result.size());
+    assertEquals(1L, result.get(0).getId());
+    assertEquals("/first", result.get(0).getPath());
+  }
 }

hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCryptoConf.xml

@@ -593,6 +593,7 @@
         <crypto-admin-command>-listZones</crypto-admin-command>
       </test-commands>
       <cleanup-commands>
+        <command>-fs NAMENODE -deleteSnapshot /snapshotable snapshot1</command>
         <command>-fs NAMENODE -rm -r /snapshotable</command>
         <command>-fs NAMENODE -rm -r .Trash/Current/*</command>
       </cleanup-commands>
@@ -603,5 +604,37 @@
         </comparator>
       </comparators>
     </test>
+
+    <test>
+      <description>Test adding two zones to a snapshotable directory;
+        The second zone is not a direct child of snapshottable directory;
+        Take snapshot, permanently delete second EZ, then list zones;</description>
+      <test-commands>
+        <command>-fs NAMENODE -rm -r .Trash/Current/*</command>
+        <command>-fs NAMENODE -mkdir /snapshotable</command>
+        <command>-fs NAMENODE -mkdir /snapshotable/test1</command>
+        <command>-fs NAMENODE -mkdir /snapshotable/test1/test2</command>
+        <command>-fs NAMENODE -mkdir /snapshotable/test3</command>
+        <dfs-admin-command>-fs NAMENODE -allowSnapshot /snapshotable</dfs-admin-command>
+        <crypto-admin-command>-createZone -path /snapshotable/test1/test2 -keyName myKey</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /snapshotable/test3 -keyName myKey</crypto-admin-command>
+        <command>-fs NAMENODE -createSnapshot /snapshotable snapshot1</command>
+        <command>-fs NAMENODE -rm -r /snapshotable/test1</command>
+        <command>-fs NAMENODE -rm -r .Trash/Current/*</command>
+        <crypto-admin-command>-listZones</crypto-admin-command>
+      </test-commands>
+      <cleanup-commands>
+        <command>-fs NAMENODE -deleteSnapshot /snapshotable snapshot1</command>
+        <command>-fs NAMENODE -rm -r /snapshotable</command>
+        <command>-fs NAMENODE -rm -r .Trash/Current/*</command>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>RegexpAcrossOutputComparator</type>
+          <expected-output>(/test3)\s*(myKey)\s*</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+
   </tests>
 </configuration>