Details

    • Type: Test
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.1
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: test
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      HDFS-9295 adds an end-to-end test for KMS, but it is missing a dimension. The tests added in that JIRA hold the configuration constant and test that all operations succeed or fail as expected. More tests are needed that hold the operation constant and test that all possible configurations cause the operations to succeed or fail as expected. This JIRA is to add those tests.

      1. HDFS-9339.002.patch
        33 kB
        Daniel Templeton
      2. HDFS-9339.001.patch
        32 kB
        Daniel Templeton

        Activity

        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #565 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/565/)
        HDFS-9339. Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #565 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/565/ ) HDFS-9339 . Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #2560 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2560/)
        HDFS-9339. Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #2560 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2560/ ) HDFS-9339 . Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2501 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2501/)
        HDFS-9339. Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2501 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2501/ ) HDFS-9339 . Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #1352 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1352/)
        HDFS-9339. Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #1352 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1352/ ) HDFS-9339 . Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #629 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/629/)
        HDFS-9339. Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #629 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/629/ ) HDFS-9339 . Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #618 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/618/)
        HDFS-9339. Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #618 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/618/ ) HDFS-9339 . Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #8744 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8744/)
        HDFS-9339. Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #8744 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8744/ ) HDFS-9339 . Extend full test of KMS ACLs. Contributed by Daniel (zhz: rev 78d6890865424db850faecfc5c76f14c64925063) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestAclsEndToEnd.java
        Hide
        zhz Zhe Zhang added a comment -

        Thanks Daniel for exploring this change. I agree that keeping the individual try-finally structures makes each subtest look cleaner. I was suggesting the consolidation to avoid duplicate code. So each option has its merits and I'm with you to use the current structure. +1 on the patch, I just committed to trunk and branch-2. Thanks for adding this thorough test!

        Show
        zhz Zhe Zhang added a comment - Thanks Daniel for exploring this change. I agree that keeping the individual try-finally structures makes each subtest look cleaner. I was suggesting the consolidation to avoid duplicate code. So each option has its merits and I'm with you to use the current structure. +1 on the patch, I just committed to trunk and branch-2. Thanks for adding this thorough test!
        Hide
        templedf Daniel Templeton added a comment -

        Zhe Zhang, I tried making the tests use one big try-finally wrapper, and I just didn't like it as much. One thing the repeated try-finally blocks do is make it really obvious where the subsections of the tests are. Taking the blocks out leaves the tests rambling a bit. Unless its a big deal for you, I think the code is actually clearer with the try-finally blocks.

        I did happen across a bug in HDFS-9295 that I fixed in the patch I just posted.

        Show
        templedf Daniel Templeton added a comment - Zhe Zhang , I tried making the tests use one big try-finally wrapper, and I just didn't like it as much. One thing the repeated try-finally blocks do is make it really obvious where the subsections of the tests are. Taking the blocks out leaves the tests rambling a bit. Unless its a big deal for you, I think the code is actually clearer with the try-finally blocks. I did happen across a bug in HDFS-9295 that I fixed in the patch I just posted.
        Hide
        zhz Zhe Zhang added a comment -

        Ah thanks, my bad. Then maybe we should try-finally for the entire test?

        Show
        zhz Zhe Zhang added a comment - Ah thanks, my bad. Then maybe we should try-finally for the entire test?
        Hide
        templedf Daniel Templeton added a comment -

        Maybe remove the try-finally and let the entire test fail if setup fails?

        If setup throws an exception, the exception isn't caught, so after executing the finally block, the exception will continue being passed up the stack, eventually causing the test to error.

        Show
        templedf Daniel Templeton added a comment - Maybe remove the try-finally and let the entire test fail if setup fails? If setup throws an exception, the exception isn't caught, so after executing the finally block, the exception will continue being passed up the stack, eventually causing the test to error.
        Hide
        zhz Zhe Zhang added a comment -

        The createKey method is a local method that wraps the KeyProvider method. One of the things it does is return false if there's an exception.

        I see, so we try for setup. But should we consider the test pass if setup fails from an exception (and therefore the assertTrue is not visited)? Maybe remove the try-finally and let the entire test fail if setup fails?

        It would be possible to abstract it out, but it would involve another anonymous inner class

        I see, thanks for explaining.

        They aren't actually repeated.

        My bad, spotted the differences now.

        +1 pending a conclusion on the first point.

        Show
        zhz Zhe Zhang added a comment - The createKey method is a local method that wraps the KeyProvider method. One of the things it does is return false if there's an exception. I see, so we try for setup . But should we consider the test pass if setup fails from an exception (and therefore the assertTrue is not visited)? Maybe remove the try-finally and let the entire test fail if setup fails? It would be possible to abstract it out, but it would involve another anonymous inner class I see, thanks for explaining. They aren't actually repeated. My bad, spotted the differences now. +1 pending a conclusion on the first point.
        Hide
        templedf Daniel Templeton added a comment -

        1. The createKey method is a local method that wraps the KeyProvider method. One of the things it does is return false if there's an exception.
        2. It would be possible to abstract it out, but it would involve another anonymous inner class to perform the operations. I'm not sure that's an improvement. That's the way it's done in TestKMS, but I don't like it.
        3. They aren't actually repeated. There are two different controlling ACLs, so one tests one ACL, and the other tests the other.

        Show
        templedf Daniel Templeton added a comment - 1. The createKey method is a local method that wraps the KeyProvider method. One of the things it does is return false if there's an exception. 2. It would be possible to abstract it out, but it would involve another anonymous inner class to perform the operations. I'm not sure that's an improvement. That's the way it's done in TestKMS, but I don't like it. 3. They aren't actually repeated. There are two different controlling ACLs, so one tests one ACL, and the other tests the other.
        Hide
        zhz Zhe Zhang added a comment -

        Thanks Daniel! The patch looks pretty good. A few minor comments:

        1. Maybe we should improve the criteria in assertions?
          try {
            setup(conf);
          
            assertTrue("Exception during key creation with correct config"
                + " using whitelist key ACLs", createKey(realUgi, KEY1, conf));
          } finally {
            teardown();
          }
          

          The test will pass if createKey throws an exception. A simple fix is to fail() in the finally statement. But maybe there's a way to avoid adding it in every finally statement.

        2. Since the above try-finally logic repeats many times we can also abstract it out as a method.
        3. Looks like the // Correct config with blacklist //Missing GET_METADATA KMS ACL sections are both repeated twice?
        Show
        zhz Zhe Zhang added a comment - Thanks Daniel! The patch looks pretty good. A few minor comments: Maybe we should improve the criteria in assertions? try { setup(conf); assertTrue( "Exception during key creation with correct config" + " using whitelist key ACLs" , createKey(realUgi, KEY1, conf)); } finally { teardown(); } The test will pass if createKey throws an exception. A simple fix is to fail() in the finally statement. But maybe there's a way to avoid adding it in every finally statement. Since the above try-finally logic repeats many times we can also abstract it out as a method. Looks like the // Correct config with blacklist //Missing GET_METADATA KMS ACL sections are both repeated twice?
        Hide
        templedf Daniel Templeton added a comment -

        Zhe Zhang, can you take a look when you get a chance?

        Show
        templedf Daniel Templeton added a comment - Zhe Zhang , can you take a look when you get a chance?

          People

          • Assignee:
            templedf Daniel Templeton
            Reporter:
            templedf Daniel Templeton
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development