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

UGI.getCurrentUser() fails if user does not have a keytab associated

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.8.0, 2.9.0, 3.0.0-alpha2
    • Fix Version/s: 3.0.0-alpha4
    • Component/s: security
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      Due to a remaining issue after HADOOP-13558, an UGI may still try to renew the TGT even though the UGI is created from an existing Subject. The renewal would fail because of non-existing keytab.

      Fixing the issue means different behavior which is incompatible, however, configuration property "hadoop.treat.subject.external" is introduced to enable the fix (disabled by default). The behavior is the same as before when the fix is not enabled.
      Show
      Due to a remaining issue after HADOOP-13558 , an UGI may still try to renew the TGT even though the UGI is created from an existing Subject. The renewal would fail because of non-existing keytab. Fixing the issue means different behavior which is incompatible, however, configuration property "hadoop.treat.subject.external" is introduced to enable the fix (disabled by default). The behavior is the same as before when the fix is not enabled.

      Description

      HADOOP-13558 intention was to avoid UGI from trying to renew the TGT when the UGI is created from an existing Subject as in that case the keytab is not 'own' by UGI but by the creator of the Subject.

      In HADOOP-13558 we introduced a new private UGI constructor UserGroupInformation(Subject subject, final boolean externalKeyTab) and we use with TRUE only when doing a UGI.loginUserFromSubject().

      The problem is, when we call UGI.getCurrentUser(), and UGI was created via a Subject (via the UGI.loginUserFromSubject() method), we call new UserGroupInformation(subject) which will delegate to UserGroupInformation(Subject subject, final boolean externalKeyTab) and that will use externalKeyTab == FALSE.

      Then the UGI returned by UGI.getCurrentUser() will attempt to login using a non-existing keytab if the TGT expired.

      This problem is experienced in KMSClientProvider when used by the HDFS filesystem client accessing an an encryption zone.

      1. HADOOP-13805.01.patch
        2 kB
        Xiao Chen
      2. HADOOP-13805.02.patch
        6 kB
        Xiao Chen
      3. HADOOP-13805.03.patch
        5 kB
        Xiao Chen
      4. HADOOP-13805.04.patch
        8 kB
        Xiao Chen
      5. HADOOP-13805.05.patch
        4 kB
        Xiao Chen
      6. HADOOP-13805.006.patch
        9 kB
        Yongjun Zhang
      7. HADOOP-13805.007.patch
        14 kB
        Yongjun Zhang
      8. HADOOP-13805.008.patch
        16 kB
        Yongjun Zhang
      9. HADOOP-13805.009.patch
        17 kB
        Yongjun Zhang
      10. HADOOP-13805.010.patch
        17 kB
        Yongjun Zhang

        Issue Links

          Activity

          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Hi Daryn Sharp,

          The exception we were hitting was:

          Caused by: java.io.IOException: loginUserFromKeyTab must be done first
          at org.apache.hadoop.security.UserGroupInformation.reloginFromKeytab(UserGroupInformation.java:1055)
          at org.apache.hadoop.security.UserGroupInformation.checkTGTAndReloginFromKeytab(UserGroupInformation.java:1020)
          at org.apache.hadoop.crypto.key.kms.KMSClientProvider.createConnection(KMSClientProvider.java:478)
          at org.apache.hadoop.crypto.key.kms.KMSClientProvider.decryptEncryptedKey(KMSClientProvider.java:771)
          at org.apache.hadoop.crypto.key.kms.LoadBalancingKMSClientProvider$3.call(LoadBalancingKMSClientProvider.java:185)
          at org.apache.hadoop.crypto.key.kms.LoadBalancingKMSClientProvider$3.call(LoadBalancingKMSClientProvider.java:181)
          at org.apache.hadoop.crypto.key.kms.LoadBalancingKMSClientProvider.doOp(LoadBalancingKMSClientProvider.java:94)
          at org.apache.hadoop.crypto.key.kms.LoadBalancingKMSClientProvider.decryptEncryptedKey(LoadBalancingKMSClientProvider.java:181)
          at org.apache.hadoop.crypto.key.KeyProviderCryptoExtension.decryptEncryptedKey(KeyProviderCryptoExtension.java:388)
          at org.apache.hadoop.hdfs.DFSClient.decryptEncryptedDataEncryptionKey(DFSClient.java:1420)
          

          The UGI as created using a Subject. The UGI used by KMS client is obtained from UGI.getCurrentUser().

          Regarding 'Any UGI should be able to relogin a subject regardless of who created it'. It may be the case in a conventional app, in our case, the app (StreamSets Data Collector) is a server app that is using classloaders to be able to interact with different versions of Hadoop clusters. Each classloader has its own Hadoop classes (diff versions of it). And Tthe renewal of the Kerberos credentials in the seed Subject is done from code in the bootstrap classloader.

          All this has worked fine for almost 2 years until HDFS encryption has been switched on and we run int the above exception.

          Said this, if you have a better idea how to solve this problem I'm all for it.

          Thanks.

          Show
          tucu00 Alejandro Abdelnur added a comment - Hi Daryn Sharp , The exception we were hitting was: Caused by: java.io.IOException: loginUserFromKeyTab must be done first at org.apache.hadoop.security.UserGroupInformation.reloginFromKeytab(UserGroupInformation.java:1055) at org.apache.hadoop.security.UserGroupInformation.checkTGTAndReloginFromKeytab(UserGroupInformation.java:1020) at org.apache.hadoop.crypto.key.kms.KMSClientProvider.createConnection(KMSClientProvider.java:478) at org.apache.hadoop.crypto.key.kms.KMSClientProvider.decryptEncryptedKey(KMSClientProvider.java:771) at org.apache.hadoop.crypto.key.kms.LoadBalancingKMSClientProvider$3.call(LoadBalancingKMSClientProvider.java:185) at org.apache.hadoop.crypto.key.kms.LoadBalancingKMSClientProvider$3.call(LoadBalancingKMSClientProvider.java:181) at org.apache.hadoop.crypto.key.kms.LoadBalancingKMSClientProvider.doOp(LoadBalancingKMSClientProvider.java:94) at org.apache.hadoop.crypto.key.kms.LoadBalancingKMSClientProvider.decryptEncryptedKey(LoadBalancingKMSClientProvider.java:181) at org.apache.hadoop.crypto.key.KeyProviderCryptoExtension.decryptEncryptedKey(KeyProviderCryptoExtension.java:388) at org.apache.hadoop.hdfs.DFSClient.decryptEncryptedDataEncryptionKey(DFSClient.java:1420) The UGI as created using a Subject . The UGI used by KMS client is obtained from UGI.getCurrentUser() . Regarding 'Any UGI should be able to relogin a subject regardless of who created it'. It may be the case in a conventional app, in our case, the app (StreamSets Data Collector) is a server app that is using classloaders to be able to interact with different versions of Hadoop clusters. Each classloader has its own Hadoop classes (diff versions of it). And Tthe renewal of the Kerberos credentials in the seed Subject is done from code in the bootstrap classloader. All this has worked fine for almost 2 years until HDFS encryption has been switched on and we run int the above exception. Said this, if you have a better idea how to solve this problem I'm all for it. Thanks.
          Hide
          daryn Daryn Sharp added a comment -

          What exactly broke in EZ because as best I can tell the whole premise of this change and the "external keytab" concept is completely broken. I think there's a deeper bug this is attempting to mask because I've seen the EZ UGI handling - it is completely screwed up and on my to-do blockers for EZ deployment.

          Internally we hacked out the external ugi changes because it broke us.

          If you create a UGI from another UGI, ie via getCurrentUser(), the created UGI should not relogin from keytab, the relogin should be done by the creator UGI if it has a keytab.

          My point is, any UGI created from a Subject (directly or via another UGI) should never attempt to relogin, it is the creator of the responsibility to do so.

          Wrong. Any UGI should be able to relogin a subject regardless of who created it. Why should any number of threads be dead in the water waiting for the "owner" to relogin in? It's a shared resource.

          The bug i'm hitting now is that UGI.getCurrentUser() creates a new UGI and this tries to do relogin from keytab even if there is no keytab associated to the current UGI. This happens when HDFS client is accessing encryption zones, specifically the HDFS client interacting with the KMS client to get encryption keys.

          Whoa. Let's step back for a minute. The ugi knows if it's from a keytab based on whether there's a KeyTab instance present. Why does the ugi think it has a keytab but doesn't have a keytab? That makes no sense and is an indicator that the ugi is being grossly misused.

          Alejandro Please provide more details or a stack trace.

          Show
          daryn Daryn Sharp added a comment - What exactly broke in EZ because as best I can tell the whole premise of this change and the "external keytab" concept is completely broken. I think there's a deeper bug this is attempting to mask because I've seen the EZ UGI handling - it is completely screwed up and on my to-do blockers for EZ deployment. Internally we hacked out the external ugi changes because it broke us. If you create a UGI from another UGI, ie via getCurrentUser(), the created UGI should not relogin from keytab, the relogin should be done by the creator UGI if it has a keytab. My point is, any UGI created from a Subject (directly or via another UGI) should never attempt to relogin, it is the creator of the responsibility to do so. Wrong. Any UGI should be able to relogin a subject regardless of who created it. Why should any number of threads be dead in the water waiting for the "owner" to relogin in? It's a shared resource. The bug i'm hitting now is that UGI.getCurrentUser() creates a new UGI and this tries to do relogin from keytab even if there is no keytab associated to the current UGI. This happens when HDFS client is accessing encryption zones, specifically the HDFS client interacting with the KMS client to get encryption keys. Whoa. Let's step back for a minute. The ugi knows if it's from a keytab based on whether there's a KeyTab instance present. Why does the ugi think it has a keytab but doesn't have a keytab? That makes no sense and is an indicator that the ugi is being grossly misused. Alejandro Please provide more details or a stack trace.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Good point Wei-Chiu Chuang. Just added it.

          Though it's incompatible when the fix is enabled, the behavior is the same as before without enabling the fix. Well, I still marked it as incompatible and made it clear in the release notes.

          Show
          yzhangal Yongjun Zhang added a comment - Good point Wei-Chiu Chuang . Just added it. Though it's incompatible when the fix is enabled, the behavior is the same as before without enabling the fix. Well, I still marked it as incompatible and made it clear in the release notes.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          I didn't follow up to the end when this was resolve. Is this an incompatible change? If so this deserves a release note and we need to flag it as an incompatible change.

          Show
          jojochuang Wei-Chiu Chuang added a comment - I didn't follow up to the end when this was resolve. Is this an incompatible change? If so this deserves a release note and we need to flag it as an incompatible change.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Yongjun Zhang, Wei-Chiu Chuang for covering me up and working on this, and thanks Alejandro Abdelnur for reporting and reviewing!

          Show
          xiaochen Xiao Chen added a comment - Thanks Yongjun Zhang , Wei-Chiu Chuang for covering me up and working on this, and thanks Alejandro Abdelnur for reporting and reviewing!
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #11273 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11273/)
          HADOOP-13805. UGI.getCurrentUser() fails if user does not have a keytab (yzhang: rev 4c26c241ad2b907dc02cecefa9846cbe2b0465ba)

          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUGIWithMiniKdc.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #11273 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11273/ ) HADOOP-13805 . UGI.getCurrentUser() fails if user does not have a keytab (yzhang: rev 4c26c241ad2b907dc02cecefa9846cbe2b0465ba) (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUGIWithMiniKdc.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
          Hide
          yzhangal Yongjun Zhang added a comment -

          I committed to trunk.

          Thanks Xiao Chen / Wei-Chiu Chuang for the earlier work, and Alejandro Abdelnur for the review.

          Show
          yzhangal Yongjun Zhang added a comment - I committed to trunk. Thanks Xiao Chen / Wei-Chiu Chuang for the earlier work, and Alejandro Abdelnur for the review.
          Hide
          yzhangal Yongjun Zhang added a comment -

          The test failures are pre-existing and reported as HADOOP-14030.

          Show
          yzhangal Yongjun Zhang added a comment - The test failures are pre-existing and reported as HADOOP-14030 .
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 12m 29s trunk passed
          +1 compile 13m 53s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 1m 8s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 1m 27s trunk passed
          +1 javadoc 0m 49s trunk passed
          +1 mvninstall 0m 39s the patch passed
          +1 compile 11m 22s the patch passed
          +1 javac 11m 22s the patch passed
          +1 checkstyle 0m 36s the patch passed
          +1 mvnsite 1m 1s the patch passed
          +1 mvneclipse 0m 17s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 49s the patch passed
          +1 javadoc 0m 48s the patch passed
          -1 unit 8m 58s hadoop-common in the patch failed.
          +1 asflicense 0m 35s The patch does not generate ASF License warnings.
          58m 58s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12853225/HADOOP-13805.010.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux bdec1aea006f 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 02c5494
          Default Java 1.8.0_121
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11651/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11651/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11651/console
          Powered by Apache Yetus 0.5.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 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 12m 29s trunk passed +1 compile 13m 53s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 1m 8s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 27s trunk passed +1 javadoc 0m 49s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 11m 22s the patch passed +1 javac 11m 22s the patch passed +1 checkstyle 0m 36s the patch passed +1 mvnsite 1m 1s the patch passed +1 mvneclipse 0m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 49s the patch passed +1 javadoc 0m 48s the patch passed -1 unit 8m 58s hadoop-common in the patch failed. +1 asflicense 0m 35s The patch does not generate ASF License warnings. 58m 58s Reason Tests Failed junit tests hadoop.security.TestKDiag Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12853225/HADOOP-13805.010.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux bdec1aea006f 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 02c5494 Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11651/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11651/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11651/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Alejandro Abdelnur. Rebased and added an INFO message when the config is enabled, as rev10.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Alejandro Abdelnur . Rebased and added an INFO message when the config is enabled, as rev10.
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          I've got confirmation the patch is working as expected on a live cluster. IMO, we can go for it. +1 again.

          Show
          tucu00 Alejandro Abdelnur added a comment - I've got confirmation the patch is working as expected on a live cluster. IMO, we can go for it. +1 again.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Xiao,

          I'm just assisting in this jira, it's still yours

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Xiao, I'm just assisting in this jira, it's still yours Thanks.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Hi, I am currently on PTO until Feburary 5th. While on PTO I will have limited access to Internet.
          For anything urgent or customer escalations, please contact cce-hdfs@, Yongjun/Xiao/John Zhuge. For any technical inquires pertains to Hadoop or HDFS,  please contact Aaron T. Myer (atm@)

          Thanks


          A very happy Clouderan

          Show
          jojochuang Wei-Chiu Chuang added a comment - Hi, I am currently on PTO until Feburary 5th. While on PTO I will have limited access to Internet. For anything urgent or customer escalations, please contact cce-hdfs@, Yongjun/Xiao/John Zhuge. For any technical inquires pertains to Hadoop or HDFS,  please contact Aaron T. Myer (atm@) Thanks – A very happy Clouderan
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          patch9 lgtm, +1. It would be great if you can verify it in a live cluster before committing.

          Show
          tucu00 Alejandro Abdelnur added a comment - patch9 lgtm, +1. It would be great if you can verify it in a live cluster before committing.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 22s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 15m 15s trunk passed
          +1 compile 13m 24s trunk passed
          +1 checkstyle 0m 31s trunk passed
          +1 mvnsite 1m 4s trunk passed
          +1 mvneclipse 0m 18s trunk passed
          +1 findbugs 1m 29s trunk passed
          +1 javadoc 0m 50s trunk passed
          +1 mvninstall 0m 38s the patch passed
          +1 compile 10m 50s the patch passed
          +1 javac 10m 50s the patch passed
          +1 checkstyle 0m 30s hadoop-common-project/hadoop-common: The patch generated 0 new + 220 unchanged - 4 fixed = 220 total (was 224)
          +1 mvnsite 1m 1s the patch passed
          +1 mvneclipse 0m 19s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 36s the patch passed
          +1 javadoc 0m 49s the patch passed
          +1 unit 8m 38s hadoop-common in the patch passed.
          +1 asflicense 0m 33s The patch does not generate ASF License warnings.
          60m 2s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12849351/HADOOP-13805.009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux f228227cb6f8 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / b782bf2
          Default Java 1.8.0_121
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11508/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11508/console
          Powered by Apache Yetus 0.5.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 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 15m 15s trunk passed +1 compile 13m 24s trunk passed +1 checkstyle 0m 31s trunk passed +1 mvnsite 1m 4s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 29s trunk passed +1 javadoc 0m 50s trunk passed +1 mvninstall 0m 38s the patch passed +1 compile 10m 50s the patch passed +1 javac 10m 50s the patch passed +1 checkstyle 0m 30s hadoop-common-project/hadoop-common: The patch generated 0 new + 220 unchanged - 4 fixed = 220 total (was 224) +1 mvnsite 1m 1s the patch passed +1 mvneclipse 0m 19s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 36s the patch passed +1 javadoc 0m 49s the patch passed +1 unit 8m 38s hadoop-common in the patch passed. +1 asflicense 0m 33s The patch does not generate ASF License warnings. 60m 2s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12849351/HADOOP-13805.009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux f228227cb6f8 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / b782bf2 Default Java 1.8.0_121 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11508/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11508/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Alejandro Abdelnur for the review again! Good comments, and here is rev 9 to address them!

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Alejandro Abdelnur for the review again! Good comments, and here is rev 9 to address them!
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          patch8 looks good.

          A few things regarding the new enableRenewThreadCreationForTest, can we make the methods package private (the testcases using them are in the same package, and having them package private will avoid an app setting them by mistake). Can we also log a WARN message in the spawnAutoRenewalThreadForUserCreds() if in test mode?

          Show
          tucu00 Alejandro Abdelnur added a comment - patch8 looks good. A few things regarding the new enableRenewThreadCreationForTest , can we make the methods package private (the testcases using them are in the same package, and having them package private will avoid an app setting them by mistake). Can we also log a WARN message in the spawnAutoRenewalThreadForUserCreds() if in test mode?
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 13m 46s trunk passed
          +1 compile 12m 58s trunk passed
          +1 checkstyle 0m 31s trunk passed
          +1 mvnsite 1m 14s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 1m 30s trunk passed
          +1 javadoc 0m 46s trunk passed
          +1 mvninstall 0m 39s the patch passed
          +1 compile 12m 21s the patch passed
          +1 javac 12m 21s the patch passed
          -0 checkstyle 0m 28s hadoop-common-project/hadoop-common: The patch generated 1 new + 220 unchanged - 4 fixed = 221 total (was 224)
          +1 mvnsite 1m 2s the patch passed
          +1 mvneclipse 0m 17s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 53s the patch passed
          +1 javadoc 0m 51s the patch passed
          +1 unit 10m 12s hadoop-common in the patch passed.
          +1 asflicense 0m 37s The patch does not generate ASF License warnings.
          61m 24s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12849224/HADOOP-13805.008.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a2b6c9c5d44c 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 9c0a4d3
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11505/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11505/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11505/console
          Powered by Apache Yetus 0.5.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 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 13m 46s trunk passed +1 compile 12m 58s trunk passed +1 checkstyle 0m 31s trunk passed +1 mvnsite 1m 14s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 30s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 12m 21s the patch passed +1 javac 12m 21s the patch passed -0 checkstyle 0m 28s hadoop-common-project/hadoop-common: The patch generated 1 new + 220 unchanged - 4 fixed = 221 total (was 224) +1 mvnsite 1m 2s the patch passed +1 mvneclipse 0m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 53s the patch passed +1 javadoc 0m 51s the patch passed +1 unit 10m 12s hadoop-common in the patch passed. +1 asflicense 0m 37s The patch does not generate ASF License warnings. 61m 24s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12849224/HADOOP-13805.008.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a2b6c9c5d44c 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 9c0a4d3 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11505/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11505/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11505/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks a lot for the review Alejandro Abdelnur.

          The failed test is actually an interesting one. It loginuser from subject, and expects the renewal thread to be created to renew the credential. Because of the fix, no matter whether we disable or enable the config, the condition for creating renewal thread is always false, thus the test failed. The reason is that this test is created after HADOOP-13558, and it depends on the behaviour of HADOOP-13558. Disabling the config will disable the HADOOP-13558 change, enabling it will fix the wrong behavior, that's why this test can't work by simply disabling or enabling the config.

          Discussed with Xiao Chen who originally created the testcase, we agreed upon a solution that introduce a special field to allow the renewal thread be created for testing purpose. Uploaded rev 008 with this solution. Would you please take a look again?

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks a lot for the review Alejandro Abdelnur . The failed test is actually an interesting one. It loginuser from subject, and expects the renewal thread to be created to renew the credential. Because of the fix, no matter whether we disable or enable the config, the condition for creating renewal thread is always false, thus the test failed. The reason is that this test is created after HADOOP-13558 , and it depends on the behaviour of HADOOP-13558 . Disabling the config will disable the HADOOP-13558 change, enabling it will fix the wrong behavior, that's why this test can't work by simply disabling or enabling the config. Discussed with Xiao Chen who originally created the testcase, we agreed upon a solution that introduce a special field to allow the renewal thread be created for testing purpose. Uploaded rev 008 with this solution. Would you please take a look again? Thanks.
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Yongjun Zhang, patch 7 looks goods, though there is a related test failure you should look at it. Also, it would be great to test this in real deployment.

          Show
          tucu00 Alejandro Abdelnur added a comment - Yongjun Zhang , patch 7 looks goods, though there is a related test failure you should look at it. Also, it would be great to test this in real deployment.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 39s 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 13m 38s trunk passed
          +1 compile 13m 36s trunk passed
          +1 checkstyle 0m 32s trunk passed
          +1 mvnsite 1m 3s trunk passed
          +1 mvneclipse 0m 19s trunk passed
          +1 findbugs 1m 29s trunk passed
          +1 javadoc 0m 48s trunk passed
          +1 mvninstall 0m 38s the patch passed
          +1 compile 12m 9s the patch passed
          +1 javac 12m 9s the patch passed
          -0 checkstyle 0m 31s hadoop-common-project/hadoop-common: The patch generated 2 new + 216 unchanged - 3 fixed = 218 total (was 219)
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 34s the patch passed
          +1 javadoc 0m 48s the patch passed
          -1 unit 7m 22s hadoop-common in the patch failed.
          +1 asflicense 0m 38s The patch does not generate ASF License warnings.
          58m 53s



          Reason Tests
          Failed junit tests hadoop.security.TestUGIWithMiniKdc



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12849054/HADOOP-13805.007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 954e5d04c131 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 0101267
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11502/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11502/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11502/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11502/console
          Powered by Apache Yetus 0.5.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 39s 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 13m 38s trunk passed +1 compile 13m 36s trunk passed +1 checkstyle 0m 32s trunk passed +1 mvnsite 1m 3s trunk passed +1 mvneclipse 0m 19s trunk passed +1 findbugs 1m 29s trunk passed +1 javadoc 0m 48s trunk passed +1 mvninstall 0m 38s the patch passed +1 compile 12m 9s the patch passed +1 javac 12m 9s the patch passed -0 checkstyle 0m 31s hadoop-common-project/hadoop-common: The patch generated 2 new + 216 unchanged - 3 fixed = 218 total (was 219) +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 34s the patch passed +1 javadoc 0m 48s the patch passed -1 unit 7m 22s hadoop-common in the patch failed. +1 asflicense 0m 38s The patch does not generate ASF License warnings. 58m 53s Reason Tests Failed junit tests hadoop.security.TestUGIWithMiniKdc Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12849054/HADOOP-13805.007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 954e5d04c131 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0101267 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11502/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11502/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11502/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11502/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks for the discussion Alejandro Abdelnur.

          Submitted rev 007 for a solution we agreed: introduce a config to resolve the compatibility issue, that is, we retain the old behavior prior to the fix of HADOOP-13558 and HADOOP-13805 at default (the config is not set or is set to false); otherwise, the fix of HADOOP-13558 and HADOOP-13805 will be enabled.

          At one point, this config should be deprecated when relevant code is fixed.

          Would you please help taking a look?

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks for the discussion Alejandro Abdelnur . Submitted rev 007 for a solution we agreed: introduce a config to resolve the compatibility issue, that is, we retain the old behavior prior to the fix of HADOOP-13558 and HADOOP-13805 at default (the config is not set or is set to false); otherwise, the fix of HADOOP-13558 and HADOOP-13805 will be enabled. At one point, this config should be deprecated when relevant code is fixed. Would you please help taking a look? Thanks.
          Hide
          yzhangal Yongjun Zhang added a comment - - edited

          Thanks Alejandro Abdelnur.

          I think you listed two options, one is to make this potentially incompatible change; the other is to create a new UGI and obsolete the old/incorrect implementation later on.

          It may not be too bad to go with option one. Say, with option two, we may hit the issue reported here. With option one, we need to watch out how things are broken due to the incompatible change, and fix accordingly.

          If we go with option one, if client code is broken, the client code need to be changed to do the renewal. Would you please help putting together a recommended client code change as part of the release notes of this jira? Or any suggestion about documenting it so user can use it as a reference.

          If we go with option one, I'm +1 on Wei-Chiu's rev6 (I found that it may not be easy to add the test you proposed as a unit test due to the run time) Would you please also take a look at rev6?

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - - edited Thanks Alejandro Abdelnur . I think you listed two options, one is to make this potentially incompatible change; the other is to create a new UGI and obsolete the old/incorrect implementation later on. It may not be too bad to go with option one. Say, with option two, we may hit the issue reported here. With option one, we need to watch out how things are broken due to the incompatible change, and fix accordingly. If we go with option one, if client code is broken, the client code need to be changed to do the renewal. Would you please help putting together a recommended client code change as part of the release notes of this jira? Or any suggestion about documenting it so user can use it as a reference. If we go with option one, I'm +1 on Wei-Chiu's rev6 (I found that it may not be easy to add the test you proposed as a unit test due to the run time) Would you please also take a look at rev6? Thanks.
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Yongjun Zhang, I understand your concern on potentially breaking existing usages if fixing the current wrong behavior. If you don't want to fix the behavior because it would account as an incompatible change, then the only option I see is a new API that will allow to create a UGI with an externalLogin; else I don't see how to create such UGI. Any suggestion?

          Show
          tucu00 Alejandro Abdelnur added a comment - Yongjun Zhang , I understand your concern on potentially breaking existing usages if fixing the current wrong behavior. If you don't want to fix the behavior because it would account as an incompatible change, then the only option I see is a new API that will allow to create a UGI with an externalLogin; else I don't see how to create such UGI. Any suggestion?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 12m 27s trunk passed
          +1 compile 9m 31s trunk passed
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 0m 59s trunk passed
          +1 mvneclipse 0m 18s trunk passed
          +1 findbugs 1m 21s trunk passed
          +1 javadoc 0m 47s trunk passed
          +1 mvninstall 0m 35s the patch passed
          +1 compile 9m 14s the patch passed
          +1 javac 9m 14s the patch passed
          -0 checkstyle 0m 30s hadoop-common-project/hadoop-common: The patch generated 2 new + 94 unchanged - 2 fixed = 96 total (was 96)
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 30s the patch passed
          +1 javadoc 0m 46s the patch passed
          -1 unit 8m 1s hadoop-common in the patch failed.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          50m 16s



          Reason Tests
          Failed junit tests hadoop.security.TestUGIWithMiniKdc



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12847530/HADOOP-13805.006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux bc2685988b5a 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / ed09c14
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11442/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11442/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11442/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11442/console
          Powered by Apache Yetus 0.5.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 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 12m 27s trunk passed +1 compile 9m 31s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 0m 59s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 21s trunk passed +1 javadoc 0m 47s trunk passed +1 mvninstall 0m 35s the patch passed +1 compile 9m 14s the patch passed +1 javac 9m 14s the patch passed -0 checkstyle 0m 30s hadoop-common-project/hadoop-common: The patch generated 2 new + 94 unchanged - 2 fixed = 96 total (was 96) +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 30s the patch passed +1 javadoc 0m 46s the patch passed -1 unit 8m 1s hadoop-common in the patch failed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 50m 16s Reason Tests Failed junit tests hadoop.security.TestUGIWithMiniKdc Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12847530/HADOOP-13805.006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux bc2685988b5a 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ed09c14 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11442/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11442/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11442/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11442/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          I have been looking at rev006 quite a bit, it looks good to me, except for two things:

          1. The change to constructor

           UserGroupInformation(Subject subject) {
              this(subject, true);
            }
          

          now changed the original behavior, even though it's really fixing a wrong behavior, it's an incompatible change. Other application use this API may break. Hi Alejandro Abdelnur, thanks for reporting the issue and review so far. How do you think we should address that?

          2. The test suggested by Alejandro at
          https://issues.apache.org/jira/browse/HADOOP-13805?focusedCommentId=15653489&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15653489
          is better included with the patch.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - I have been looking at rev006 quite a bit, it looks good to me, except for two things: 1. The change to constructor UserGroupInformation(Subject subject) { this (subject, true ); } now changed the original behavior, even though it's really fixing a wrong behavior, it's an incompatible change. Other application use this API may break. Hi Alejandro Abdelnur , thanks for reporting the issue and review so far. How do you think we should address that? 2. The test suggested by Alejandro at https://issues.apache.org/jira/browse/HADOOP-13805?focusedCommentId=15653489&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15653489 is better included with the patch. Thanks.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks for the email.
          I'm currently on PTO and will return on Jan. 23. I will have limited email access during this time.
          For Escalations, please contact cce-hdfs@, HDFS-CCE HipChat room ,or Yongjun Zhang (yzhang@).For HDFS/KMS issues, please contact int-hdfs@, HDFS HipChat room, or ATM (atm@).


          -Xiao

          Show
          xiaochen Xiao Chen added a comment - Thanks for the email. I'm currently on PTO and will return on Jan. 23. I will have limited email access during this time. For Escalations, please contact cce-hdfs@, HDFS-CCE HipChat room ,or Yongjun Zhang (yzhang@).For HDFS/KMS issues, please contact int-hdfs@, HDFS HipChat room, or ATM (atm@). – -Xiao
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks all for the work here!

          Xiao Chen is unavailable for some time and Wei-Chiu Chuang worked out a new rev 006, as I'm posting it now.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks all for the work here! Xiao Chen is unavailable for some time and Wei-Chiu Chuang worked out a new rev 006, as I'm posting it now.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Alejandro Abdelnur I am not expert in UGI and user authentication. But looking at this jira and the patch, I think you are right. UGI should not use isKeytab to determine if it should renew.

          Show
          jojochuang Wei-Chiu Chuang added a comment - Alejandro Abdelnur I am not expert in UGI and user authentication. But looking at this jira and the patch, I think you are right. UGI should not use isKeytab to determine if it should renew.
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Xiao Chen,

          Unless I'm missing something, I don't think patch 5 will do the trick for the scenario where I've seen this issue to pop up.

          getCurrentUser() creates a new UGI using the UserGroupInformation(Subject) constructor, in your proposed change, the availability of the keytab is determined by inspecting the given subject. If the given Subject has a keytab but is not owned/created to the UGI (my usecase) it will create UGI with the isKeytab flag set to true and this will fail in the same way as without patch 5.

          Somehow, we have to make sure that if a UGI is created with an external Subject, any UGI derived from it it should not say that it has a keytab.

          Show
          tucu00 Alejandro Abdelnur added a comment - Xiao Chen , Unless I'm missing something, I don't think patch 5 will do the trick for the scenario where I've seen this issue to pop up. getCurrentUser() creates a new UGI using the UserGroupInformation(Subject) constructor, in your proposed change, the availability of the keytab is determined by inspecting the given subject. If the given Subject has a keytab but is not owned/created to the UGI (my usecase) it will create UGI with the isKeytab flag set to true and this will fail in the same way as without patch 5. Somehow, we have to make sure that if a UGI is created with an external Subject, any UGI derived from it it should not say that it has a keytab.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 12s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 12m 41s trunk passed
          +1 compile 9m 28s trunk passed
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 0m 59s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 1m 37s trunk passed
          +1 javadoc 0m 50s trunk passed
          +1 mvninstall 0m 42s the patch passed
          +1 compile 10m 20s the patch passed
          +1 javac 10m 20s the patch passed
          +1 checkstyle 0m 29s the patch passed
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 1s The patch has no whitespace issues.
          +1 findbugs 1m 47s the patch passed
          +1 javadoc 0m 46s the patch passed
          +1 unit 9m 8s hadoop-common in the patch passed.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          53m 22s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845597/HADOOP-13805.05.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 88ec3ca88dad 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / a0a2761
          Default Java 1.8.0_111
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11358/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11358/console
          Powered by Apache Yetus 0.5.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 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 12m 41s trunk passed +1 compile 9m 28s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 0m 59s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 37s trunk passed +1 javadoc 0m 50s trunk passed +1 mvninstall 0m 42s the patch passed +1 compile 10m 20s the patch passed +1 javac 10m 20s the patch passed +1 checkstyle 0m 29s the patch passed +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 1s The patch has no whitespace issues. +1 findbugs 1m 47s the patch passed +1 javadoc 0m 46s the patch passed +1 unit 9m 8s hadoop-common in the patch passed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 53m 22s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845597/HADOOP-13805.05.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 88ec3ca88dad 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / a0a2761 Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11358/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11358/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks for the comment Alejandro Abdelnur.

          kinit -R assumes the TGT can still be renewed, if it reached it max life time it is not. So this will delay the failure until the TGT cannot be renewed anymore;

          Looking at the initial commit, I think that's what this renewal thread is supposed to do - kinit -R until the TGT reach its max lifetime. After that, it will fail and seems current code isn't considering it - no -kt <keytab> is provided to the command.
          Verifying this from shell, kinit -R -kt <keytab> will get a new TGT to keep credentials updated, but feels like we should split that improvement to a new jira.
          Let's move this part to HADOOP-13807 if you're comfortable.

          Back to this jira, I think the issue can be fixed in another way. Current patch breaks TestKMS#testTGTRenewal, and the usage there seems reasonable.
          So maybe we can fix it this way - the state of whether a login is external shouldn't be a instance variable of the UGI, but a static variable reflecting what loginFromXXX was performed to log the user in. Therefore, it can track the initial login and perform relogins accordingly. This is different than current patch, because UGI itself instantiates new UGI objects in various calls (e.g. getCurrentUser), and performs loginUserFromSubject internally (e.g. getLoginUser(null)). Having a static variable to reflect the static loginUserFromXXX methods feels cleaner.

          Patch has the proposed fix, and didn't have to change any unit test. Appreciate your continued feedback!

          Show
          xiaochen Xiao Chen added a comment - Thanks for the comment Alejandro Abdelnur . kinit -R assumes the TGT can still be renewed, if it reached it max life time it is not. So this will delay the failure until the TGT cannot be renewed anymore; Looking at the initial commit , I think that's what this renewal thread is supposed to do - kinit -R until the TGT reach its max lifetime. After that, it will fail and seems current code isn't considering it - no -kt <keytab> is provided to the command. Verifying this from shell, kinit -R -kt <keytab> will get a new TGT to keep credentials updated, but feels like we should split that improvement to a new jira. Let's move this part to HADOOP-13807 if you're comfortable. Back to this jira, I think the issue can be fixed in another way. Current patch breaks TestKMS#testTGTRenewal , and the usage there seems reasonable. So maybe we can fix it this way - the state of whether a login is external shouldn't be a instance variable of the UGI, but a static variable reflecting what loginFromXXX was performed to log the user in. Therefore, it can track the initial login and perform relogins accordingly. This is different than current patch, because UGI itself instantiates new UGI objects in various calls (e.g. getCurrentUser ), and performs loginUserFromSubject internally (e.g. getLoginUser(null) ). Having a static variable to reflect the static loginUserFromXXX methods feels cleaner. Patch has the proposed fix, and didn't have to change any unit test. Appreciate your continued feedback!
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Xiao Chen,

          kinit -R assumes the TGT can still be renewed, if it reached it max life time it is not. So this will delay the failure until the TGT cannot be renewed anymore; at that point it will fail as you'll need to use the keytab which you don't have.

          Regardless, even if kinit -R would do the trick, it is not correct for UGI to take over renewal responsibilities when the TGT has not been obtained by UGI.

          Show
          tucu00 Alejandro Abdelnur added a comment - Xiao Chen , kinit -R assumes the TGT can still be renewed, if it reached it max life time it is not. So this will delay the failure until the TGT cannot be renewed anymore; at that point it will fail as you'll need to use the keytab which you don't have. Regardless, even if kinit -R would do the trick, it is not correct for UGI to take over renewal responsibilities when the TGT has not been obtained by UGI.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 12s 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 13m 35s trunk passed
          +1 compile 9m 38s trunk passed
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 1m 1s trunk passed
          +1 mvneclipse 0m 18s trunk passed
          +1 findbugs 1m 22s trunk passed
          +1 javadoc 0m 48s trunk passed
          +1 mvninstall 0m 36s the patch passed
          +1 compile 9m 10s the patch passed
          +1 javac 9m 10s the patch passed
          -0 checkstyle 0m 30s hadoop-common-project/hadoop-common: The patch generated 9 new + 101 unchanged - 0 fixed = 110 total (was 101)
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 32s the patch passed
          +1 javadoc 0m 46s the patch passed
          +1 unit 8m 27s hadoop-common in the patch passed.
          +1 asflicense 0m 32s The patch does not generate ASF License warnings.
          52m 0s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845453/HADOOP-13805.04.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 7a10fcdf0400 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 8fadd69
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11347/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11347/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11347/console
          Powered by Apache Yetus 0.5.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 12s 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 13m 35s trunk passed +1 compile 9m 38s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 0m 48s trunk passed +1 mvninstall 0m 36s the patch passed +1 compile 9m 10s the patch passed +1 javac 9m 10s the patch passed -0 checkstyle 0m 30s hadoop-common-project/hadoop-common: The patch generated 9 new + 101 unchanged - 0 fixed = 110 total (was 101) +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 32s the patch passed +1 javadoc 0m 46s the patch passed +1 unit 8m 27s hadoop-common in the patch passed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 52m 0s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845453/HADOOP-13805.04.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7a10fcdf0400 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8fadd69 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11347/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11347/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11347/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Patch 4 attached.

          Tested this by running downstream smokes etc, didn't see any failure. But found out there's a existing test TestUGIWithMiniKdc that needs update.

          The renewal thread should not be started if there is no keytab, there is no point to do so because it will not have the credentials (the info in the keytab) at renewal time.

          Alejandro Abdelnur please correct me if I'm wrong, the renewal thread is doing kinit -R so a TGT would be sufficient, and keytab doesn't need to be renewed or present for the tgt renewal, right? In any case, I agree with your initial proposal of having this done in HADOOP-13807 - feels cleaner and more separated

          Show
          xiaochen Xiao Chen added a comment - Patch 4 attached. Tested this by running downstream smokes etc, didn't see any failure. But found out there's a existing test TestUGIWithMiniKdc that needs update. The renewal thread should not be started if there is no keytab, there is no point to do so because it will not have the credentials (the info in the keytab) at renewal time. Alejandro Abdelnur please correct me if I'm wrong, the renewal thread is doing kinit -R so a TGT would be sufficient, and keytab doesn't need to be renewed or present for the tgt renewal, right? In any case, I agree with your initial proposal of having this done in HADOOP-13807 - feels cleaner and more separated
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Xiao Chen, sorry missed your NOV18 comment. The renewal thread should not be started if there is no keytab, there is no point to do so because it will not have the credentials (the info in the keytab) at renewal time.

          Show
          tucu00 Alejandro Abdelnur added a comment - Xiao Chen , sorry missed your NOV18 comment. The renewal thread should not be started if there is no keytab, there is no point to do so because it will not have the credentials (the info in the keytab) at renewal time.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks for the ping, I think this is major, but should target the same as HADOOP-13558. Tucu, please feel free to modify if you disagree.

          Show
          xiaochen Xiao Chen added a comment - Thanks for the ping, I think this is major, but should target the same as HADOOP-13558 . Tucu, please feel free to modify if you disagree.
          Hide
          andrew.wang Andrew Wang added a comment -

          Is this a release blocker? Also HADOOP-13558 has fix versions of 2.7.4 and 2.8.0, is this targeted at those releases as well?

          Show
          andrew.wang Andrew Wang added a comment - Is this a release blocker? Also HADOOP-13558 has fix versions of 2.7.4 and 2.8.0, is this targeted at those releases as well?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 8m 18s trunk passed
          +1 compile 11m 6s trunk passed
          +1 checkstyle 0m 30s trunk passed
          +1 mvnsite 1m 7s trunk passed
          +1 mvneclipse 0m 18s trunk passed
          +1 findbugs 1m 26s trunk passed
          +1 javadoc 0m 47s trunk passed
          +1 mvninstall 0m 46s the patch passed
          +1 compile 10m 19s the patch passed
          +1 javac 10m 19s the patch passed
          +1 checkstyle 0m 31s the patch passed
          +1 mvnsite 1m 10s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 45s the patch passed
          +1 javadoc 0m 49s the patch passed
          +1 unit 8m 42s hadoop-common in the patch passed.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          50m 28s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12839605/HADOOP-13805.03.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux ffb9eb3b4c18 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 / f6ffa11
          Default Java 1.8.0_111
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11101/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11101/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 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 8m 18s trunk passed +1 compile 11m 6s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 1m 7s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 26s trunk passed +1 javadoc 0m 47s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 10m 19s the patch passed +1 javac 10m 19s the patch passed +1 checkstyle 0m 31s the patch passed +1 mvnsite 1m 10s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 45s the patch passed +1 javadoc 0m 49s the patch passed +1 unit 8m 42s hadoop-common in the patch passed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 50m 28s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12839605/HADOOP-13805.03.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux ffb9eb3b4c18 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 / f6ffa11 Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11101/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11101/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Patch 3 uploaded. Turns out that unit test was depending on an incorrect behavior.

          I don't understand why the renewal thread should depend on whether there's a keytab or not. Isn't it supposed to be renewing the ticket?

            private void spawnAutoRenewalThreadForUserCreds() {
              if (!isSecurityEnabled()
                  || user.getAuthenticationMethod() != AuthenticationMethod.KERBEROS
                  || isKeytab) {
                return;
              }
          

          Made this change in patch 3, but if desired, can separate that to HADOOP-13807 and workaround the test in this jira. Also, Alejandro Abdelnur could you share more info on HADOOP-13807? Left a comment there.

          Thank you.

          Show
          xiaochen Xiao Chen added a comment - Patch 3 uploaded. Turns out that unit test was depending on an incorrect behavior. I don't understand why the renewal thread should depend on whether there's a keytab or not. Isn't it supposed to be renewing the ticket? private void spawnAutoRenewalThreadForUserCreds() { if (!isSecurityEnabled() || user.getAuthenticationMethod() != AuthenticationMethod.KERBEROS || isKeytab) { return ; } Made this change in patch 3, but if desired, can separate that to HADOOP-13807 and workaround the test in this jira. Also, Alejandro Abdelnur could you share more info on HADOOP-13807 ? Left a comment there. Thank you.
          Hide
          xiaochen Xiao Chen added a comment -

          Hm, clearly unit test caught me. Let me look more into this...

          Show
          xiaochen Xiao Chen added a comment - Hm, clearly unit test caught me. Let me look more into this...
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 7m 36s trunk passed
          +1 compile 10m 37s trunk passed
          +1 checkstyle 0m 30s trunk passed
          +1 mvnsite 1m 7s trunk passed
          +1 mvneclipse 0m 19s trunk passed
          +1 findbugs 1m 25s trunk passed
          +1 javadoc 0m 49s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 9m 13s the patch passed
          +1 javac 9m 13s the patch passed
          +1 checkstyle 0m 30s the patch passed
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 32s the patch passed
          +1 javadoc 0m 46s the patch passed
          -1 unit 7m 57s hadoop-common in the patch failed.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          46m 49s



          Reason Tests
          Failed junit tests hadoop.security.TestUGIWithMiniKdc



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13805
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12839492/HADOOP-13805.02.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux eab41fcf714b 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 / 140b993
          Default Java 1.8.0_111
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11096/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11096/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11096/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 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 7m 36s trunk passed +1 compile 10m 37s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 1m 7s trunk passed +1 mvneclipse 0m 19s trunk passed +1 findbugs 1m 25s trunk passed +1 javadoc 0m 49s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 9m 13s the patch passed +1 javac 9m 13s the patch passed +1 checkstyle 0m 30s the patch passed +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 32s the patch passed +1 javadoc 0m 46s the patch passed -1 unit 7m 57s hadoop-common in the patch failed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 46m 49s Reason Tests Failed junit tests hadoop.security.TestUGIWithMiniKdc Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13805 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12839492/HADOOP-13805.02.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux eab41fcf714b 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 / 140b993 Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11096/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11096/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11096/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Alejandro Abdelnur for the review. I thought more about this, and feels like we should add an orthogonal parameter to control whether to relogin/spawn renew thread or not.

          Patch 2 attached, I believe this should take care of both this and HADOOP-13807 correctly.

          Show
          xiaochen Xiao Chen added a comment - Thanks Alejandro Abdelnur for the review. I thought more about this, and feels like we should add an orthogonal parameter to control whether to relogin/spawn renew thread or not. Patch 2 attached, I believe this should take care of both this and HADOOP-13807 correctly.
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Patch LGTM, I was missing the point that when login from keytab the subject is created outside of the resulting UGI. I see now why you need the private constructor introduced in HADOOP-13558.

          Show
          tucu00 Alejandro Abdelnur added a comment - Patch LGTM, I was missing the point that when login from keytab the subject is created outside of the resulting UGI. I see now why you need the private constructor introduced in HADOOP-13558 .
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Alejandro Abdelnur for the explanation.

          It seems we should eliminate the scenarios that would make isKeytab true and keytabFile null. I couldn't think of a better solution than you proposed - UGI class is LimitedPrivate and the reloginFromXXX are public, so not much room for the change...

          I'm attaching a preliminary patch 1 to collect feedback, will run a full unit test for better coverage. Will also look for corner cases in the meantime.

          Show
          xiaochen Xiao Chen added a comment - Thanks Alejandro Abdelnur for the explanation. It seems we should eliminate the scenarios that would make isKeytab true and keytabFile null. I couldn't think of a better solution than you proposed - UGI class is LimitedPrivate and the reloginFromXXX are public, so not much room for the change... I'm attaching a preliminary patch 1 to collect feedback, will run a full unit test for better coverage. Will also look for corner cases in the meantime.
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          Xiao Chen,

          If you create a UGI from Subject externally, this can done only via the getUGIFromSubject(Subject) method, and in that case the UGI should not relogin from keytab as we already discussed.

          If you create a UGI from another UGI, ie via getCurrentUser(), the created UGI should not relogin from keytab, the relogin should be done by the creator UGI if it has a keytab.

          My point is, any UGI created from a Subject (directly or via another UGI) should never attempt to relogin, it is the creator of the responsibility to do so.

          The bug i'm hitting now is that UGI.getCurrentUser() creates a new UGI and this tries to do relogin from keytab even if there is no keytab associated to the current UGI. This happens when HDFS client is accessing encryption zones, specifically the HDFS client interacting with the KMS client to get encryption keys.

          Show
          tucu00 Alejandro Abdelnur added a comment - Xiao Chen , If you create a UGI from Subject externally, this can done only via the getUGIFromSubject(Subject) method, and in that case the UGI should not relogin from keytab as we already discussed. If you create a UGI from another UGI , ie via getCurrentUser() , the created UGI should not relogin from keytab, the relogin should be done by the creator UGI if it has a keytab. My point is, any UGI created from a Subject (directly or via another UGI ) should never attempt to relogin, it is the creator of the responsibility to do so. The bug i'm hitting now is that UGI.getCurrentUser() creates a new UGI and this tries to do relogin from keytab even if there is no keytab associated to the current UGI. This happens when HDFS client is accessing encryption zones, specifically the HDFS client interacting with the KMS client to get encryption keys.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Alejandro Abdelnur for creating this with great details / reproduction / solution.

          Could you help me understand more? Specifically this:
          currently UGI is always created with the UGI(Subject) constructor. If we set the default to true, then it seems the spawn logic won't be triggered ever. When should hadoop spawn the background thread to renew? Is the proposal to make false the default and only true when loginUserFromKeytab / loginUserFromKeytabAndReturnUGI / loginUserFromTicketCache ?
          Also, what about createRemoteUser and createProxyUser?

          Show
          xiaochen Xiao Chen added a comment - Thanks Alejandro Abdelnur for creating this with great details / reproduction / solution. Could you help me understand more? Specifically this: currently UGI is always created with the UGI(Subject) constructor. If we set the default to true, then it seems the spawn logic won't be triggered ever. When should hadoop spawn the background thread to renew? Is the proposal to make false the default and only true when loginUserFromKeytab / loginUserFromKeytabAndReturnUGI / loginUserFromTicketCache ? Also, what about createRemoteUser and createProxyUser ?
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          A quick way to verify the proposed solution work is using running the following snippet and wait until the TGT expires:

              UserGroupInformation.setShouldRenewImmediatelyForTests(true);
          
              UserGroupInformation.loginUserFromSubject(subject);
              UserGroupInformation ugi = UserGroupInformation.getLoginUser();
              for (int i = 0; i < 100; i++) {
                System.out.printf("*******Into %dsecs\n", i * 30*1000);
                ugi.doAs(new PrivilegedExceptionAction<Object>() {
                  @Override
                  public Object run() throws Exception {
                    URI uri = new URI("kms://http@localhost:16000/kms");
                    KMSClientProvider provider = (KMSClientProvider) new KMSClientProvider.Factory().createProvider(uri, hConf);
                    System.out.println(provider.getKeys());
                    return null;
                  }
                });
                for (int j = 0; j < 30; j++) {
                  System.out.println("! " + j);
                  System.out.flush();
                  Thread.sleep(1000);
                }
                System.out.println();
              }
          
          Show
          tucu00 Alejandro Abdelnur added a comment - A quick way to verify the proposed solution work is using running the following snippet and wait until the TGT expires: UserGroupInformation.setShouldRenewImmediatelyForTests( true ); UserGroupInformation.loginUserFromSubject(subject); UserGroupInformation ugi = UserGroupInformation.getLoginUser(); for ( int i = 0; i < 100; i++) { System .out.printf( "*******Into %dsecs\n" , i * 30*1000); ugi.doAs( new PrivilegedExceptionAction< Object >() { @Override public Object run() throws Exception { URI uri = new URI( "kms: //http@localhost:16000/kms" ); KMSClientProvider provider = (KMSClientProvider) new KMSClientProvider.Factory().createProvider(uri, hConf); System .out.println(provider.getKeys()); return null ; } }); for ( int j = 0; j < 30; j++) { System .out.println( "! " + j); System .out.flush(); Thread .sleep(1000); } System .out.println(); }
          Hide
          tucu00 Alejandro Abdelnur added a comment -

          The solution, and I think is a safe one, is that the UGI(Subject) constructor to set isExternalKeytab to TRUE. If an UGI is being created from a Subject, the keytab is always external and it is the Subject creator responsibility to keep the credentials valid.

          Show
          tucu00 Alejandro Abdelnur added a comment - The solution, and I think is a safe one, is that the UGI(Subject) constructor to set isExternalKeytab to TRUE . If an UGI is being created from a Subject, the keytab is always external and it is the Subject creator responsibility to keep the credentials valid.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development