Hadoop Common
  1. Hadoop Common
  2. HADOOP-7645

HTTP auth tests requiring Kerberos infrastructure are not disabled on branch-0.20-security

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.20.205.0
    • Fix Version/s: 0.20.205.0
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The back-port of HADOOP-7119 to branch-0.20-security included tests which require Kerberos infrastructure in order to run. In trunk and 0.23, these are disabled unless one enables the testKerberos maven profile. In branch-0.20-security, these tests are always run regardless, and so fail most of the time.

      See this Jenkins build for an example: https://builds.apache.org/view/G-L/view/Hadoop/job/Hadoop-0.20-security/26/

      1. HADOOP-7645-20s.2.patch
        2 kB
        Jitendra Nath Pandey
      2. HADOOP-7645-20s.1.patch
        2 kB
        Jitendra Nath Pandey
      3. hadoop-7645-022.patch
        2 kB
        Benoy Antony

        Issue Links

          Activity

          Hide
          Aaron T. Myers added a comment -

          The valid options to me seem to be:

          1. Disable these tests somehow such that they're only run when some specific flag is passed to ant. Is there any precedent for doing so with specific test cases?
          2. Remove these tests from branch-0.20-security. Assuming that anyone who works on the classes exercised by these tests will be doing the work on trunk and back-porting to branch-0.20-security, the classes shouldn't diverge much if any.

          Thoughts?

          Show
          Aaron T. Myers added a comment - The valid options to me seem to be: Disable these tests somehow such that they're only run when some specific flag is passed to ant. Is there any precedent for doing so with specific test cases? Remove these tests from branch-0.20-security. Assuming that anyone who works on the classes exercised by these tests will be doing the work on trunk and back-porting to branch-0.20-security, the classes shouldn't diverge much if any. Thoughts?
          Hide
          Kihwal Lee added a comment -

          I noticed the failures too. I am fine with the option 1. LTC tests are like that.
          In any case I think we need it fixed for 205 if it's not overly complicated.

          Show
          Kihwal Lee added a comment - I noticed the failures too. I am fine with the option 1. LTC tests are like that. In any case I think we need it fixed for 205 if it's not overly complicated.
          Hide
          Kihwal Lee added a comment -

          I added 0.205 to fix version.

          Show
          Kihwal Lee added a comment - I added 0.205 to fix version.
          Hide
          Ahmed Radwan added a comment -

          +1 for option #1. It will be consistent with current maven behavior on trunk and 0.23.

          Show
          Ahmed Radwan added a comment - +1 for option #1. It will be consistent with current maven behavior on trunk and 0.23.
          Hide
          Jitendra Nath Pandey added a comment -

          A patch that disables these tests.

          Show
          Jitendra Nath Pandey added a comment - A patch that disables these tests.
          Hide
          Aaron T. Myers added a comment -

          Jitendra, is there no way to do something with ant akin to what is done on trunk with maven? That is, don't run some specific tests unless, e.g. "-DkerbTests" is passed when invoking ant?

          Show
          Aaron T. Myers added a comment - Jitendra, is there no way to do something with ant akin to what is done on trunk with maven? That is, don't run some specific tests unless, e.g. "-DkerbTests" is passed when invoking ant?
          Hide
          Kihwal Lee added a comment -

          The approach in the patch is not exactly the solution people have agreed on. It still can be automated by specifying the test case with -Dtestcase, so there might be no problem in terms of running these tests. But I think the @Ignore annotation implies "obsolete" or "broken" and marking these tests with it can be misleading.

          Show
          Kihwal Lee added a comment - The approach in the patch is not exactly the solution people have agreed on. It still can be automated by specifying the test case with -Dtestcase, so there might be no problem in terms of running these tests. But I think the @Ignore annotation implies "obsolete" or "broken" and marking these tests with it can be misleading.
          Hide
          Jitendra Nath Pandey added a comment -

          Some mapreduce tests already use "@Ignore" in 20 branch. For example, TestMiniMRMapRedDebugScript. I just followed this because it was a very simple change.

          Show
          Jitendra Nath Pandey added a comment - Some mapreduce tests already use "@Ignore" in 20 branch. For example, TestMiniMRMapRedDebugScript. I just followed this because it was a very simple change.
          Hide
          Kihwal Lee added a comment -

          At the minimum, there should be some comments along with the new @Ignore annotations, explaining why they are ignored and what is needed to run them.

          Show
          Kihwal Lee added a comment - At the minimum, there should be some comments along with the new @Ignore annotations, explaining why they are ignored and what is needed to run them.
          Hide
          Suresh Srinivas added a comment -

          But I think the @Ignore annotation implies "obsolete" or "broken" and marking these tests with it can be misleading.

          @Ignore is for precisely this reason http://api.dpml.net/junit/4.2/org/junit/Ignore.html

          "Sometimes you want to temporarily disable a test. Methods annotated with Test that are also annotated with @Ignore will not be executed as tests. Native JUnit 4 test runners should report the number of ignored tests along with the number of tests that ran and the number of tests that failed."

          Show
          Suresh Srinivas added a comment - But I think the @Ignore annotation implies "obsolete" or "broken" and marking these tests with it can be misleading. @Ignore is for precisely this reason http://api.dpml.net/junit/4.2/org/junit/Ignore.html "Sometimes you want to temporarily disable a test. Methods annotated with Test that are also annotated with @Ignore will not be executed as tests. Native JUnit 4 test runners should report the number of ignored tests along with the number of tests that ran and the number of tests that failed."
          Hide
          Kihwal Lee added a comment -

          So I guess when we mark a broken test with @Ignore (not in this case. The test is not broken.), we intend to fix them later. Otherwise we should simply delete it. Correct?

          Show
          Kihwal Lee added a comment - So I guess when we mark a broken test with @Ignore (not in this case. The test is not broken.), we intend to fix them later. Otherwise we should simply delete it. Correct?
          Hide
          Suresh Srinivas added a comment -

          We should add comment about why the test is disabled. I think adding ignore for 205 branch is a good enough solution. This is already solved nicely on trunk. If any one strongly prefers -DkerbTests, they can fix it in 0.20-security to be picked up later.

          Show
          Suresh Srinivas added a comment - We should add comment about why the test is disabled. I think adding ignore for 205 branch is a good enough solution. This is already solved nicely on trunk. If any one strongly prefers -DkerbTests, they can fix it in 0.20-security to be picked up later.
          Hide
          Aaron T. Myers added a comment -

          I think adding ignore for 205 branch is a good enough solution. This is already solved nicely on trunk.

          If indeed one can still run the test using "-Dtestcase=..." as Kihwal points out, then I'm fine just adding the @Ignore annotations with a comment saying why.

          Show
          Aaron T. Myers added a comment - I think adding ignore for 205 branch is a good enough solution. This is already solved nicely on trunk. If indeed one can still run the test using "-Dtestcase=..." as Kihwal points out, then I'm fine just adding the @Ignore annotations with a comment saying why.
          Hide
          Jitendra Nath Pandey added a comment -

          Added comments.

          Show
          Jitendra Nath Pandey added a comment - Added comments.
          Hide
          Suresh Srinivas added a comment -

          +1 for the patch.

          Show
          Suresh Srinivas added a comment - +1 for the patch.
          Hide
          Aaron T. Myers added a comment -

          Before commit, could someone please test to make sure that "ant -Dtestcase=TestKerberosAuthenticationHandler test-core" does indeed work even with the @Ignore annotation in place? The only thing I really want to avoid is having to modify the test code in order to run it.

          Show
          Aaron T. Myers added a comment - Before commit, could someone please test to make sure that " ant -Dtestcase=TestKerberosAuthenticationHandler test-core " does indeed work even with the @Ignore annotation in place? The only thing I really want to avoid is having to modify the test code in order to run it.
          Hide
          Jitendra Nath Pandey added a comment -

          >Before commit, could someone please test to make sure that "ant -Dtestcase=TestKerberosAuthenticationHandler
          > test-core" does indeed work even with the @Ignore annotation in place?

          With ant version 1.8.2 the above command runs the test, but with ant 1.7.1 it doesn't. I suspect ant-1.8.2 is just ignoring @Ignore.

          Aaron,
          Given the above observation, do you think @Ignore is not acceptable for 205? Alternative approach is to commit this patch as there are many tests in mapreduce that use the same approach, and come with a better fix in 206?

          Show
          Jitendra Nath Pandey added a comment - >Before commit, could someone please test to make sure that "ant -Dtestcase=TestKerberosAuthenticationHandler > test-core" does indeed work even with the @Ignore annotation in place? With ant version 1.8.2 the above command runs the test, but with ant 1.7.1 it doesn't. I suspect ant-1.8.2 is just ignoring @Ignore. Aaron, Given the above observation, do you think @Ignore is not acceptable for 205? Alternative approach is to commit this patch as there are many tests in mapreduce that use the same approach, and come with a better fix in 206?
          Hide
          Aaron T. Myers added a comment -

          Alternative approach is to commit this patch as there are many tests in mapreduce that use the same approach, and come with a better fix in 206?

          That's fine by me. Please file a JIRA which will hopefully get fixed in 206.

          Show
          Aaron T. Myers added a comment - Alternative approach is to commit this patch as there are many tests in mapreduce that use the same approach, and come with a better fix in 206? That's fine by me. Please file a JIRA which will hopefully get fixed in 206.
          Hide
          Jitendra Nath Pandey added a comment -

          Created jira : HADOOP-7675
          Committed this to 205, 206.

          Show
          Jitendra Nath Pandey added a comment - Created jira : HADOOP-7675 Committed this to 205, 206.
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot, Jitendra. Marking this issue as resolved.

          Show
          Aaron T. Myers added a comment - Thanks a lot, Jitendra. Marking this issue as resolved.
          Hide
          Matt Foley added a comment -

          Closed upon release of 0.20.205.0

          Show
          Matt Foley added a comment - Closed upon release of 0.20.205.0
          Hide
          Benoy Antony added a comment -

          attached patch for 0.22

          Show
          Benoy Antony added a comment - attached patch for 0.22
          Hide
          Konstantin Shvachko added a comment -

          I just committed this to branch 0.22.1. Thank you Benoy.

          Show
          Konstantin Shvachko added a comment - I just committed this to branch 0.22.1. Thank you Benoy.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-22-branch #106 (See https://builds.apache.org/job/Hadoop-Common-22-branch/106/)
          HADOOP-7645. Disable TestKerberosAuthenticator and TestKerberosAuthenticationHandler. Contributed by Jitendra Pandey and Benoy Antony. (Revision 1346226)

          Result = SUCCESS
          shv : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1346226
          Files :

          • /hadoop/common/branches/branch-0.22/common/CHANGES.txt
          • /hadoop/common/branches/branch-0.22/common/src/test/core/org/apache/hadoop/security/authentication/client/TestKerberosAuthenticator.java
          • /hadoop/common/branches/branch-0.22/common/src/test/core/org/apache/hadoop/security/authentication/server/TestKerberosAuthenticationHandler.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-22-branch #106 (See https://builds.apache.org/job/Hadoop-Common-22-branch/106/ ) HADOOP-7645 . Disable TestKerberosAuthenticator and TestKerberosAuthenticationHandler. Contributed by Jitendra Pandey and Benoy Antony. (Revision 1346226) Result = SUCCESS shv : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1346226 Files : /hadoop/common/branches/branch-0.22/common/CHANGES.txt /hadoop/common/branches/branch-0.22/common/src/test/core/org/apache/hadoop/security/authentication/client/TestKerberosAuthenticator.java /hadoop/common/branches/branch-0.22/common/src/test/core/org/apache/hadoop/security/authentication/server/TestKerberosAuthenticationHandler.java
          Hide
          Matt Foley added a comment -

          Fixed in 20.205, which was a lineal predecessor of 1.0. No need to mark it fixed in 1.1.0.

          Show
          Matt Foley added a comment - Fixed in 20.205, which was a lineal predecessor of 1.0. No need to mark it fixed in 1.1.0.

            People

            • Assignee:
              Jitendra Nath Pandey
              Reporter:
              Aaron T. Myers
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development