Hadoop Common
  1. Hadoop Common
  2. HADOOP-6527

UserGroupInformation::createUserForTesting clobbers already defined group mappings

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.0, 1-win
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In UserGroupInformation::createUserForTesting the follow code creates a new groups instance, obliterating any groups that have been previously defined in the static groups field.

          if (!(groups instanceof TestingGroups)) {
            groups = new TestingGroups();
          }
      

      This becomes a problem in tests that start a Mini

      {DFS,MR}

      Cluster and then create a testing user. The user that started the user (generally the real user running the test) immediately has their groups wiped out and is prevented from accessing files/folders/queues they should be able to. Before the UserGroupInformation.createRemoteUserForTesting, calls to userA.getGroups may return

      {"a", "b", "c"}

      and immediately after the new fake user is created, the same call will return an empty array.

        Issue Links

          Activity

          Hide
          Matt Foley added a comment -

          Closed upon release of Hadoop-1.1.0.

          Show
          Matt Foley added a comment - Closed upon release of Hadoop-1.1.0.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Ivan!

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Ivan!
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good.
          Hide
          Ivan Mitic added a comment -

          I attached the patch to HADOOP-7389 so that someone can backport the fix. I guess it should be fine to resolve this Jira as a duplicate of HADOOP-7389.

          Show
          Ivan Mitic added a comment - I attached the patch to HADOOP-7389 so that someone can backport the fix. I guess it should be fine to resolve this Jira as a duplicate of HADOOP-7389 .
          Hide
          Ivan Mitic added a comment -

          Just to add, I still think it would be useful to remove comments from TestQueueManager.java that refer to this Jira, as this is no longer needed.

          Show
          Ivan Mitic added a comment - Just to add, I still think it would be useful to remove comments from TestQueueManager.java that refer to this Jira, as this is no longer needed.
          Hide
          Ivan Mitic added a comment -

          Thanks Nicholas, Bikas!

          The patch for HADOOP-7389 works fine, so I would like to propose to port it to branch-1-win (ported patch attached). Please let me know if I can do something to help with the process besides providing the patch.

          PS.

          I tried to add only the new test in TestUserGroupInformation without other changes to branch-1. The new test still passed.

          You are right, my bad. The new test would have to be the first in the list, otherwise the real user groups are already cleaned up.

          Show
          Ivan Mitic added a comment - Thanks Nicholas, Bikas! The patch for HADOOP-7389 works fine, so I would like to propose to port it to branch-1-win (ported patch attached). Please let me know if I can do something to help with the process besides providing the patch. PS. I tried to add only the new test in TestUserGroupInformation without other changes to branch-1. The new test still passed. You are right, my bad. The new test would have to be the first in the list, otherwise the real user groups are already cleaned up.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The underlyingImplementation code was introduced by HADOOP-7389.

          Show
          Tsz Wo Nicholas Sze added a comment - The underlyingImplementation code was introduced by HADOOP-7389 .
          Hide
          Bikas Saha added a comment -

          Additionally, I think Nicholas also mentions that the new test seems to be passing even without the new code changes. Would be worth double checking that.

          Show
          Bikas Saha added a comment - Additionally, I think Nicholas also mentions that the new test seems to be passing even without the new code changes. Would be worth double checking that.
          Hide
          Ivan Mitic added a comment -

          Thanks for bringing this up Nicholas! I have not tested this in trunk, just scanned through the trunk implementation of UserGroupInformation.java (but missed the underlyingImplementation detail). Let me take a look if the trunk fix addresses the problem completely, and if yes, I guess we can back port the fix to branch-1 if needed.

          Show
          Ivan Mitic added a comment - Thanks for bringing this up Nicholas! I have not tested this in trunk, just scanned through the trunk implementation of UserGroupInformation.java (but missed the underlyingImplementation detail). Let me take a look if the trunk fix addresses the problem completely, and if yes, I guess we can back port the fix to branch-1 if needed.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Hi Ivan,

          Is this still a problem in trunk? TestingGroups in trunk has underlyingImplementation as a field and uses it when the result is null.

          BTW, I tried to add only the new test in TestUserGroupInformation without other changes to branch-1. The new test still passed.

          Show
          Tsz Wo Nicholas Sze added a comment - Hi Ivan, Is this still a problem in trunk? TestingGroups in trunk has underlyingImplementation as a field and uses it when the result is null. BTW, I tried to add only the new test in TestUserGroupInformation without other changes to branch-1. The new test still passed.
          Hide
          Bikas Saha added a comment -

          lgtm. Looks good to go unless more experienced folks have concerns.

          Show
          Bikas Saha added a comment - lgtm. Looks good to go unless more experienced folks have concerns.
          Hide
          Ivan Mitic added a comment -

          Thanks for reviewing Bikas!

          Have tests been run on both Windows and Linux to verify?

          Yes, I've the tests on both Windows and Linux.

          Show
          Ivan Mitic added a comment - Thanks for reviewing Bikas! Have tests been run on both Windows and Linux to verify? Yes, I've the tests on both Windows and Linux.
          Hide
          Bikas Saha added a comment -

          Have tests been run on both Windows and Linux to verify?

          Show
          Bikas Saha added a comment - Have tests been run on both Windows and Linux to verify?
          Hide
          Ivan Mitic added a comment -

          Attaching a proposal for the fix. This is a branch-1 compatible change. Will prepare a trunk patch once everyone is happy with the fix.

          Show
          Ivan Mitic added a comment - Attaching a proposal for the fix. This is a branch-1 compatible change. Will prepare a trunk patch once everyone is happy with the fix.

            People

            • Assignee:
              Ivan Mitic
              Reporter:
              Jakob Homan
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development