Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13558

UserGroupInformation created from a Subject incorrectly tries to renew the Kerberos ticket

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.2, 2.6.4, 3.0.0-alpha2
    • Fix Version/s: 2.8.0, 2.7.4, 3.0.0-alpha2
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The UGI checkTGTAndReloginFromKeytab() method checks certain conditions and if they are met it invokes the reloginFromKeytab(). The reloginFromKeytab() method then fails with an IOException "loginUserFromKeyTab must be done first" because there is no keytab associated with the UGI.

      The checkTGTAndReloginFromKeytab() method checks if there is a keytab (isKeytab UGI instance variable) associated with the UGI, if there is one it triggers a call to reloginFromKeytab(). The problem is that the keytabFile UGI instance variable is NULL, and that triggers the mentioned IOException.

      The root of the problem seems to be when creating a UGI via the UGI.loginUserFromSubject(Subject) method, this method uses the UserGroupInformation(Subject) constructor, and this constructor does the following to determine if there is a keytab or not.

        this.isKeytab = KerberosUtil.hasKerberosKeyTab(subject);
      

      If the Subject given had a keytab, then the UGI instance will have the isKeytab set to TRUE.

      It sets the UGI instance as it would have a keytab because the Subject has a keytab. This has 2 problems:

      First, it does not set the keytab file (and this, having the isKeytab set to TRUE and the keytabFile set to NULL) is what triggers the IOException in the method reloginFromKeytab().

      Second (and even if the first problem is fixed, this still is a problem), it assumes that because the subject has a keytab it is up to UGI to do the relogin using the keytab. This is incorrect if the UGI was created using the UGI.loginUserFromSubject(Subject) method. In such case, the owner of the Subject is not the UGI, but the caller, so the caller is responsible for renewing the Kerberos tickets and the UGI should not try to do so.

      1. HADOOP-13558.branch-2.7.patch
        4 kB
        Xiao Chen
      2. HADOOP-13558.02.patch
        4 kB
        Xiao Chen
      3. HADOOP-13558.01.patch
        4 kB
        Xiao Chen

        Issue Links

          Activity

          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Tucu for reporting the issue.

          IIUC, the concern is that, with loginUserFromSubject, checkTGTAndReloginFromKeytab always throws the IOE, since isKeytab == true and keytabFile == null?

          I'm not entirely familiar with this ground, so not sure how feasible / secure it is to set keytabFile from subject.... If not, maybe we should set isKeytab to false. This will also get rid of the second problem, since no renewal will be done.
          Alternate solution of not throwing IOE (just log and return) seems incompatible.

          I'm hoping Robert Joseph Evans or Daryn Sharp as the initial author to HADOOP-10164 can shed some lights on this, thanks in advance!

          Show
          xiaochen Xiao Chen added a comment - Thanks Tucu for reporting the issue. IIUC, the concern is that, with loginUserFromSubject , checkTGTAndReloginFromKeytab always throws the IOE, since isKeytab == true and keytabFile == null ? I'm not entirely familiar with this ground, so not sure how feasible / secure it is to set keytabFile from subject.... If not, maybe we should set isKeytab to false. This will also get rid of the second problem, since no renewal will be done. Alternate solution of not throwing IOE (just log and return) seems incompatible. I'm hoping Robert Joseph Evans or Daryn Sharp as the initial author to HADOOP-10164 can shed some lights on this, thanks in advance!
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Xiao Chen, i think, when given a Subject, the renewal of ticket is the responsibility of the owner of the Subject, so as you suggest isKeytab should be FALSE.

          Show
          tucu00 Alejandro Abdelnur added a comment - Xiao Chen , i think, when given a Subject, the renewal of ticket is the responsibility of the owner of the Subject, so as you suggest isKeytab should be FALSE.
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Looking a bit more I think Xiao Chen suggestion is the right one. While this could be seen as an incompatible change, it is not, because a relogin by UGI for a UGI created from a Subject it never could have work, there is no keytab file. This means there is no functional application using this. Thus we can safely make the change, simply not checking if a Subject has a keytab and not setting the flag either. We don't nee to check because UGI does not care, it is the responsibility of the Subject creator to renew its credentials.

          Thoughts?

          Show
          tucu00 Alejandro Abdelnur added a comment - Looking a bit more I think Xiao Chen suggestion is the right one. While this could be seen as an incompatible change, it is not, because a relogin by UGI for a UGI created from a Subject it never could have work, there is no keytab file. This means there is no functional application using this. Thus we can safely make the change, simply not checking if a Subject has a keytab and not setting the flag either. We don't nee to check because UGI does not care, it is the responsibility of the Subject creator to renew its credentials. Thoughts?
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks again Tucu. I think this makes sense for the case described, but I lack the knowledge to review isKeytab == false is expected in all other cases. I'm attaching a patch which minimally set this to false when loginUserFromSubject, to trigger a pre-commit. (I imagine coverage on these cases aren't great though...)

          I'm not sure what's the expected return for the helper functions too, such as hasKerberosCredentials, isFromKeytab and isLoginKeytabBased. Could you review?

          Also would love to get comments from Daryn Sharp and Steve Loughran. Thanks in advance.

          Show
          xiaochen Xiao Chen added a comment - Thanks again Tucu. I think this makes sense for the case described, but I lack the knowledge to review isKeytab == false is expected in all other cases. I'm attaching a patch which minimally set this to false when loginUserFromSubject , to trigger a pre-commit. (I imagine coverage on these cases aren't great though...) I'm not sure what's the expected return for the helper functions too, such as hasKerberosCredentials , isFromKeytab and isLoginKeytabBased . Could you review? Also would love to get comments from Daryn Sharp and Steve Loughran . Thanks in advance.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 8m 1s trunk passed
          +1 compile 8m 8s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 55s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 22s trunk passed
          +1 javadoc 0m 44s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 6m 51s the patch passed
          +1 javac 6m 51s the patch passed
          -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148)
          +1 mvnsite 0m 51s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 26s the patch passed
          +1 javadoc 0m 44s the patch passed
          -1 unit 7m 40s hadoop-common in the patch failed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          40m 37s



          Reason Tests
          Failed junit tests hadoop.security.ssl.TestSSLFactory



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13558
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12826335/HADOOP-13558.01.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux d65236200199 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 20ae1fa
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10425/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10425/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10425/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10425/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 8m 1s trunk passed +1 compile 8m 8s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 0m 44s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 6m 51s the patch passed +1 javac 6m 51s the patch passed -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148) +1 mvnsite 0m 51s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 26s the patch passed +1 javadoc 0m 44s the patch passed -1 unit 7m 40s hadoop-common in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 40m 37s Reason Tests Failed junit tests hadoop.security.ssl.TestSSLFactory Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13558 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12826335/HADOOP-13558.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux d65236200199 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 20ae1fa Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10425/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10425/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10425/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10425/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I deny all understanding of how Kerberos works, except for a deep fear of the UGI class. Maybe larry mccay could look at it from the perspective of somebody who understands this stuff

          1. I don'tt understand why way the current/new code reuses the realuser field. Unless the act of creating the UGI has side effects, the first assignment is a no-op. If it is done for side effects, comments should declare this, an separate field used. now is the time to do this.
          2. test-wise, before the patch to UGI goes in —did the new test case fail? As that's a good sign that the test can recreate the problem and show it is fixed.
          3. If possible, it'd be good to extend KDiag with more info here
          4. This really ought to go through a full build and test run against a kerberized cluster. For example: can a version of Hadoop built with this auth with HDFS using keytabs as well as tickets. Volunteers?
          Show
          stevel@apache.org Steve Loughran added a comment - I deny all understanding of how Kerberos works, except for a deep fear of the UGI class. Maybe larry mccay could look at it from the perspective of somebody who understands this stuff I don'tt understand why way the current/new code reuses the realuser field. Unless the act of creating the UGI has side effects, the first assignment is a no-op. If it is done for side effects, comments should declare this, an separate field used. now is the time to do this. test-wise, before the patch to UGI goes in —did the new test case fail? As that's a good sign that the test can recreate the problem and show it is fixed. If possible, it'd be good to extend KDiag with more info here This really ought to go through a full build and test run against a kerberized cluster. For example: can a version of Hadoop built with this auth with HDFS using keytabs as well as tickets. Volunteers?
          Hide
          lmccay Larry McCay added a comment -

          Hi Alejandro Abdelnur!

          I agree with Steve Loughran's point regarding the double instantiation of the realUser. In fact, it seems to me that the setting of the loginContext and authenticationMethod get lost when you create the second one. This really looks like a bug, if not, it needs to be made clear as to why it is doing that.

          Beyond that, the new ctor should call the existing one and override the setting of isKeytab afterward in order to pick up any changes to the behavior of the other ctor going forward.

          Tricking UGI into not renewing/reloggin by overriding this field is simple but really should be done in an OO way instead.
          UGI being what it is, this simple change is probably the safest way forward.
          Let's comment this clearly in the new ctor - so when someone is stepping through and see the keytab credential they don't see this as a bug.

          Show
          lmccay Larry McCay added a comment - Hi Alejandro Abdelnur ! I agree with Steve Loughran 's point regarding the double instantiation of the realUser. In fact, it seems to me that the setting of the loginContext and authenticationMethod get lost when you create the second one. This really looks like a bug, if not, it needs to be made clear as to why it is doing that. Beyond that, the new ctor should call the existing one and override the setting of isKeytab afterward in order to pick up any changes to the behavior of the other ctor going forward. Tricking UGI into not renewing/reloggin by overriding this field is simple but really should be done in an OO way instead. UGI being what it is, this simple change is probably the safest way forward. Let's comment this clearly in the new ctor - so when someone is stepping through and see the keytab credential they don't see this as a bug.
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Larry McCay, Steve Loughran, thanks for looking into this.

          Xiao Chen, thanks for putting up a patch. Regarding the new constructor, do we really need it? or we could retrofit the existing one (which is package private).

          Show
          tucu00 Alejandro Abdelnur added a comment - Larry McCay , Steve Loughran , thanks for looking into this. Xiao Chen , thanks for putting up a patch. Regarding the new constructor, do we really need it? or we could retrofit the existing one (which is package private).
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Steve, Larry and Tucu for the valuable comments!

          1. reuses the realuser field

          Tracing back in history I think this is a bug. It was originally added by HADOOP-6656 (search for {{loginuser = }}). So removed the first.
          The setters are needed, because in the ctor of UGI, User is retrieved from Subject, hence pertaining the previous set values.

          2. test-wise ...

          Yep, the test fails before the fix, and passes after.

          3. If possible, it'd be good to extend KDiag with more info here

          Not sure how to pull in KDiag here, suggestion appreciated. I added a debug log though.

          4. This really ought to go through a full build and test run against a kerberized cluster.

          Valid point... Tucu, would you be able to help on the test? Ideally we can test with your setup where this issue is reported, make sure it's fixed and no regression? I can test with a keytab-based env.

          retrofit the existing one (which is package private).

          I think technically we can. The reason I put up a new constructor is to reduce change scope (so the other 13 usages doesn't have to be modified). Can definitely update to use existing one if you feel strongly. I added javadoc to the new constructor as Larry suggested.

          Attached patch 2 to express the idea. Feedback appreciated!

          Show
          xiaochen Xiao Chen added a comment - Thanks Steve, Larry and Tucu for the valuable comments! 1. reuses the realuser field Tracing back in history I think this is a bug. It was originally added by HADOOP-6656 (search for {{loginuser = }}). So removed the first. The setters are needed, because in the ctor of UGI, User is retrieved from Subject , hence pertaining the previous set values. 2. test-wise ... Yep, the test fails before the fix, and passes after. 3. If possible, it'd be good to extend KDiag with more info here Not sure how to pull in KDiag here, suggestion appreciated. I added a debug log though. 4. This really ought to go through a full build and test run against a kerberized cluster. Valid point... Tucu, would you be able to help on the test? Ideally we can test with your setup where this issue is reported, make sure it's fixed and no regression? I can test with a keytab-based env. retrofit the existing one (which is package private). I think technically we can. The reason I put up a new constructor is to reduce change scope (so the other 13 usages doesn't have to be modified). Can definitely update to use existing one if you feel strongly. I added javadoc to the new constructor as Larry suggested. Attached patch 2 to express the idea. Feedback appreciated!
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 25s trunk passed
          +1 compile 6m 58s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 54s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 19s trunk passed
          +1 javadoc 0m 45s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 6m 44s the patch passed
          +1 javac 6m 44s the patch passed
          +1 checkstyle 0m 25s the patch passed
          +1 mvnsite 0m 51s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 27s the patch passed
          +1 javadoc 0m 44s the patch passed
          -1 unit 7m 59s hadoop-common in the patch failed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          39m 8s



          Reason Tests
          Failed junit tests hadoop.ha.TestZKFailoverController



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13558
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12826567/HADOOP-13558.02.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 017f6d0d81af 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6c60036
          Default Java 1.8.0_101
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10436/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10436/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10436/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 25s trunk passed +1 compile 6m 58s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 54s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 19s trunk passed +1 javadoc 0m 45s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 6m 44s the patch passed +1 javac 6m 44s the patch passed +1 checkstyle 0m 25s the patch passed +1 mvnsite 0m 51s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 27s the patch passed +1 javadoc 0m 44s the patch passed -1 unit 7m 59s hadoop-common in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 39m 8s Reason Tests Failed junit tests hadoop.ha.TestZKFailoverController Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13558 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12826567/HADOOP-13558.02.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 017f6d0d81af 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6c60036 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10436/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10436/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10436/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          LGTM; debug statement and cleanup of bug both improve code quality.

          what do others think?

          Show
          stevel@apache.org Steve Loughran added a comment - LGTM; debug statement and cleanup of bug both improve code quality. what do others think?
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          LGTM, still i'm struggling with the following:

          Why do we need a new private constructor private UserGroupInformation(Subject subject, final boolean externalKeyTab) that is called with false from a single place, the original private UserGroupInformation(Subject subject) constructor. It seems to me we should simply set isKeytab to false in the original constructor and we are done. I don't see how the UGI renewal would work if somebody sets externalKeytab to true as the keytabFile will be null and we'll run into the same issue this JIRA reports.

          What am I missing?

          Show
          tucu00 Alejandro Abdelnur added a comment - LGTM, still i'm struggling with the following: Why do we need a new private constructor private UserGroupInformation(Subject subject, final boolean externalKeyTab) that is called with false from a single place, the original private UserGroupInformation(Subject subject) constructor. It seems to me we should simply set isKeytab to false in the original constructor and we are done. I don't see how the UGI renewal would work if somebody sets externalKeytab to true as the keytabFile will be null and we'll run into the same issue this JIRA reports. What am I missing?
          Hide
          xiaochen Xiao Chen added a comment -

          Thank you both for the prompt reviews.

          Regarding the constructor, the intent is to keep current behavior (decide isKeytab from the passed in Subject), while changing loginUserFromSubject to no longer do so. The former is done by calling the new constructor with false, and the latter is done by calling with true.
          Other calls such as loginUserFromKeytab and loginUserFromKeytabAndReturnUGI still works because they set the keytabFile.

          Ideally the change could look better if isKeytab is not final, but IMHO leave it as final is better.

          Does this make sense?

          Show
          xiaochen Xiao Chen added a comment - Thank you both for the prompt reviews. Regarding the constructor, the intent is to keep current behavior (decide isKeytab from the passed in Subject ), while changing loginUserFromSubject to no longer do so. The former is done by calling the new constructor with false, and the latter is done by calling with true. Other calls such as loginUserFromKeytab and loginUserFromKeytabAndReturnUGI still works because they set the keytabFile . Ideally the change could look better if isKeytab is not final , but IMHO leave it as final is better. Does this make sense?
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          got it, thx. +1

          Show
          tucu00 Alejandro Abdelnur added a comment - got it, thx. +1
          Hide
          xiaochen Xiao Chen added a comment -

          FYI - I'm doing some final testing as Steve suggested in his earlier comments. Will update here later today.

          Thanks for reviewing!

          Show
          xiaochen Xiao Chen added a comment - FYI - I'm doing some final testing as Steve suggested in his earlier comments. Will update here later today. Thanks for reviewing!
          Hide
          xiaochen Xiao Chen added a comment -

          Did a full hadoop unit test, and a smoke test run in a kerberized cluster. No regression found.

          I plan to commit this on Tuesday (given Monday is a U.S. holiday). Feel free to comment if any objections.

          Show
          xiaochen Xiao Chen added a comment - Did a full hadoop unit test, and a smoke test run in a kerberized cluster. No regression found. I plan to commit this on Tuesday (given Monday is a U.S. holiday). Feel free to comment if any objections.
          Hide
          xiaochen Xiao Chen added a comment -

          The failed test looks unrelated and passed locally.

          Will commit patch 2 end of the today.

          Show
          xiaochen Xiao Chen added a comment - The failed test looks unrelated and passed locally. Will commit patch 2 end of the today.
          Hide
          xiaochen Xiao Chen added a comment -

          Committed this to trunk and branch-2.

          Thanks Alejandro Abdelnur for reporting the issue and the discussion. Also thanks Steve Loughran and Larry McCay for the review comments!

          Show
          xiaochen Xiao Chen added a comment - Committed this to trunk and branch-2. Thanks Alejandro Abdelnur for reporting the issue and the discussion. Also thanks Steve Loughran and Larry McCay for the review comments!
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10402 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10402/)
          HADOOP-13558. UserGroupInformation created from a Subject incorrectly (xiao: rev 680be58aac03a9ffab6b07c8fde9602ddb9dc858)

          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10402 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10402/ ) HADOOP-13558 . UserGroupInformation created from a Subject incorrectly (xiao: rev 680be58aac03a9ffab6b07c8fde9602ddb9dc858) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
          Hide
          zhz Zhe Zhang added a comment -

          Xiao Chen Thanks for the fix! Since it affects earlier versions do you mind porting it to branch-2.7? I can also help with that.

          Show
          zhz Zhe Zhang added a comment - Xiao Chen Thanks for the fix! Since it affects earlier versions do you mind porting it to branch-2.7? I can also help with that.
          Hide
          xiaochen Xiao Chen added a comment -

          Hi Zhe Zhang,
          Attached a branch-2.7 patch, please feel free to check it in if looks good to you.
          Branch-2.8 only has the test class portion of the conflicts in the 2.7 patch, and is hopefully easy to tackle with.

          Show
          xiaochen Xiao Chen added a comment - Hi Zhe Zhang , Attached a branch-2.7 patch, please feel free to check it in if looks good to you. Branch-2.8 only has the test class portion of the conflicts in the 2.7 patch, and is hopefully easy to tackle with.
          Hide
          xiaochen Xiao Chen added a comment -

          FYI - I have backported this to branch-2.8 and branch-2.7 as suggested. Compiled and ran the test locally before pushing. Thanks for the reminder, Zhe.

          Show
          xiaochen Xiao Chen added a comment - FYI - I have backported this to branch-2.8 and branch-2.7 as suggested. Compiled and ran the test locally before pushing. Thanks for the reminder, Zhe.
          Hide
          zhz Zhe Zhang added a comment -

          Thanks much Xiao!

          Show
          zhz Zhe Zhang added a comment - Thanks much Xiao!
          Hide
          tucu00 Alejandro Abdelnur added a comment - - edited

          Zhe Zhang, I'm still running into issues with HDFS/KMS because of this. The issue seems to be that UserGroupInformation.getCurrentUser() uses the old constructor of UserGroupInformation and it sets like the UGI has a keytab. And the KMSClientProvider uses the UserGroupInformation.getCurrentUser() method to get the UGI.

          I'll dig the exact issue and open a follow up JIRA.

          Show
          tucu00 Alejandro Abdelnur added a comment - - edited Zhe Zhang , I'm still running into issues with HDFS/KMS because of this. The issue seems to be that UserGroupInformation.getCurrentUser() uses the old constructor of UserGroupInformation and it sets like the UGI has a keytab. And the KMSClientProvider uses the UserGroupInformation.getCurrentUser() method to get the UGI. I'll dig the exact issue and open a follow up JIRA.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Alejandro Abdelnur for the heads up.

          May be unrelated, but KMSCP's UGI logic also has a recent fix HADOOP-13749 - not sure if that will help here. Sounds more than that... Will follow up in the new jira to figure out.

          Show
          xiaochen Xiao Chen added a comment - Thanks Alejandro Abdelnur for the heads up. May be unrelated, but KMSCP's UGI logic also has a recent fix HADOOP-13749 - not sure if that will help here. Sounds more than that... Will follow up in the new jira to figure out.

            People

            • Assignee:
              xiaochen Xiao Chen
              Reporter:
              tucu00 Alejandro Abdelnur
            • Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development