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

Support MSI and DeviceCode token provider in ADLS

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-beta1, 2.8.3
    • Component/s: fs/adl
    • Labels:
      None
    • Environment:

      MSI Change applies only to Hadoop running in an Azure VM

      Description

      This change is to upgrade the Hadoop ADLS connector to enable new auth features exposed by the ADLS Java SDK.

      Specifically:
      MSI Tokens: MSI (Managed Service Identity) is a way to provide an identity to an Azure Service. In the case of VMs, they can be used to give an identity to a VM deployment. This simplifies managing Service Principals, since the creds don’t have to be managed in core-site files anymore. The way this works is that during VM deployment, the ARM (Azure Resource Manager) template needs to be modified to enable MSI. Once deployed, the MSI extension runs a service on the VM that exposes a token endpoint to http://localhost at a port specified in the template. The SDK has a new TokenProvider to fetch the token from this local endpoint. This change would expose that TokenProvider as an auth option.

      DeviceCode auth: This enables a token to be obtained from an interactive login. The user is given a URL and a token to use on the login screen. User can use the token to login from any device. Once the login is done, the token that is obtained is in the name of the user who logged in. Note that because of the interactive login involved, this is not very suitable for job scenarios, but can work for ad-hoc scenarios like running “hdfs dfs” commands.

      1. HADOOP-14627.002.patch
        15 kB
        Atul Sikaria
      2. HADOOP-14627.003.patch
        15 kB
        John Zhuge
      3. HADOOP-14627.004.patch
        15 kB
        John Zhuge
      4. HADOOP-14627-001.patch
        10 kB
        Atul Sikaria

        Issue Links

          Activity

          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12175 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12175/)
          HADOOP-14627. Support MSI and DeviceCode token provider in ADLS. (jzhuge: rev 7769e9614956283a86eda9e4e69aaa592c0ca960)

          • (edit) hadoop-tools/hadoop-azure-datalake/src/test/java/org/apache/hadoop/fs/adl/TestAzureADTokenProvider.java
          • (edit) hadoop-tools/hadoop-azure-datalake/src/site/markdown/index.md
          • (edit) hadoop-tools/hadoop-azure-datalake/src/main/java/org/apache/hadoop/fs/adl/AdlConfKeys.java
          • (edit) hadoop-tools/hadoop-azure-datalake/src/main/java/org/apache/hadoop/fs/adl/AdlFileSystem.java
          • (edit) hadoop-tools/hadoop-azure-datalake/src/main/java/org/apache/hadoop/fs/adl/TokenProviderType.java
          • (edit) hadoop-tools/hadoop-azure-datalake/pom.xml
          • (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12175 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12175/ ) HADOOP-14627 . Support MSI and DeviceCode token provider in ADLS. (jzhuge: rev 7769e9614956283a86eda9e4e69aaa592c0ca960) (edit) hadoop-tools/hadoop-azure-datalake/src/test/java/org/apache/hadoop/fs/adl/TestAzureADTokenProvider.java (edit) hadoop-tools/hadoop-azure-datalake/src/site/markdown/index.md (edit) hadoop-tools/hadoop-azure-datalake/src/main/java/org/apache/hadoop/fs/adl/AdlConfKeys.java (edit) hadoop-tools/hadoop-azure-datalake/src/main/java/org/apache/hadoop/fs/adl/AdlFileSystem.java (edit) hadoop-tools/hadoop-azure-datalake/src/main/java/org/apache/hadoop/fs/adl/TokenProviderType.java (edit) hadoop-tools/hadoop-azure-datalake/pom.xml (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
          Hide
          jzhuge John Zhuge added a comment -

          Committed to trunk, branch-2, and branch-2.8.

          Thanks Atul Sikaria for the contribution!

          Show
          jzhuge John Zhuge added a comment - Committed to trunk, branch-2, and branch-2.8. Thanks Atul Sikaria for the contribution!
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +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.
                trunk Compile Tests
          0 mvndep 1m 27s Maven dependency ordering for branch
          +1 mvninstall 17m 30s trunk passed
          +1 compile 18m 31s trunk passed
          +1 checkstyle 2m 25s trunk passed
          +1 mvnsite 2m 16s trunk passed
          +1 findbugs 2m 36s trunk passed
          +1 javadoc 1m 27s trunk passed
                Patch Compile Tests
          0 mvndep 0m 22s Maven dependency ordering for patch
          +1 mvninstall 1m 15s the patch passed
          +1 compile 13m 42s the patch passed
          +1 javac 13m 42s the patch passed
          +1 checkstyle 2m 24s the patch passed
          +1 mvnsite 2m 16s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 3s The patch has no ill-formed XML file.
          +1 findbugs 2m 51s the patch passed
          +1 javadoc 1m 30s the patch passed
                Other Tests
          +1 unit 9m 32s hadoop-common in the patch passed.
          +1 unit 3m 56s hadoop-azure-datalake in the patch passed.
          +1 asflicense 0m 41s The patch does not generate ASF License warnings.
          107m 7s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14627
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881595/HADOOP-14627.004.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux f6d77ee875f3 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 28d97b7
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13013/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure-datalake U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13013/console
          Powered by Apache Yetus 0.6.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.       Prechecks +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.       trunk Compile Tests 0 mvndep 1m 27s Maven dependency ordering for branch +1 mvninstall 17m 30s trunk passed +1 compile 18m 31s trunk passed +1 checkstyle 2m 25s trunk passed +1 mvnsite 2m 16s trunk passed +1 findbugs 2m 36s trunk passed +1 javadoc 1m 27s trunk passed       Patch Compile Tests 0 mvndep 0m 22s Maven dependency ordering for patch +1 mvninstall 1m 15s the patch passed +1 compile 13m 42s the patch passed +1 javac 13m 42s the patch passed +1 checkstyle 2m 24s the patch passed +1 mvnsite 2m 16s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 3s The patch has no ill-formed XML file. +1 findbugs 2m 51s the patch passed +1 javadoc 1m 30s the patch passed       Other Tests +1 unit 9m 32s hadoop-common in the patch passed. +1 unit 3m 56s hadoop-azure-datalake in the patch passed. +1 asflicense 0m 41s The patch does not generate ASF License warnings. 107m 7s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14627 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881595/HADOOP-14627.004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux f6d77ee875f3 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 28d97b7 Default Java 1.8.0_144 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13013/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure-datalake U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13013/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jzhuge John Zhuge added a comment -

          Patch 004

          Show
          jzhuge John Zhuge added a comment - Patch 004 Trivial fix in index.md to take care of HADOOP-14438
          Hide
          jzhuge John Zhuge added a comment -

          Thanks Mingliang Liu.

          Show
          jzhuge John Zhuge added a comment - Thanks Mingliang Liu .
          Hide
          liuml07 Mingliang Liu added a comment -

          The change looks good to me overall, but I need more time to give another +1. As I am working on something else recently, please go ahead w/o me. Thanks John Zhuge.

          Show
          liuml07 Mingliang Liu added a comment - The change looks good to me overall, but I need more time to give another +1. As I am working on something else recently, please go ahead w/o me. Thanks John Zhuge .
          Hide
          jzhuge John Zhuge added a comment -

          Committing in a few days unless there is an objection.

          Show
          jzhuge John Zhuge added a comment - Committing in a few days unless there is an objection.
          Hide
          jzhuge John Zhuge added a comment -

          TestKDiag and TestDNS failures are unrelated.

          +1 LGTM

          Mingliang Liu Could you please take a look?

          Show
          jzhuge John Zhuge added a comment - TestKDiag and TestDNS failures are unrelated. +1 LGTM Mingliang Liu Could you please take a look?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
                Prechecks
          +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.
                trunk Compile Tests
          0 mvndep 0m 15s Maven dependency ordering for branch
          +1 mvninstall 14m 14s trunk passed
          +1 compile 14m 47s trunk passed
          +1 checkstyle 1m 57s trunk passed
          +1 mvnsite 1m 55s trunk passed
          +1 findbugs 1m 59s trunk passed
          +1 javadoc 1m 14s trunk passed
                Patch Compile Tests
          0 mvndep 0m 18s Maven dependency ordering for patch
          +1 mvninstall 0m 59s the patch passed
          +1 compile 11m 15s the patch passed
          +1 javac 11m 15s the patch passed
          +1 checkstyle 2m 0s the patch passed
          +1 mvnsite 1m 55s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 3s The patch has no ill-formed XML file.
          +1 findbugs 2m 23s the patch passed
          +1 javadoc 1m 18s the patch passed
                Other Tests
          -1 unit 8m 15s hadoop-common in the patch failed.
          +1 unit 3m 44s hadoop-azure-datalake in the patch passed.
          +1 asflicense 0m 33s The patch does not generate ASF License warnings.
          91m 16s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag
            hadoop.net.TestDNS



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14627
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881161/HADOOP-14627.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux 17e81f374afd 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 8d953c2
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12999/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12999/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure-datalake U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12999/console
          Powered by Apache Yetus 0.6.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.       Prechecks +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.       trunk Compile Tests 0 mvndep 0m 15s Maven dependency ordering for branch +1 mvninstall 14m 14s trunk passed +1 compile 14m 47s trunk passed +1 checkstyle 1m 57s trunk passed +1 mvnsite 1m 55s trunk passed +1 findbugs 1m 59s trunk passed +1 javadoc 1m 14s trunk passed       Patch Compile Tests 0 mvndep 0m 18s Maven dependency ordering for patch +1 mvninstall 0m 59s the patch passed +1 compile 11m 15s the patch passed +1 javac 11m 15s the patch passed +1 checkstyle 2m 0s the patch passed +1 mvnsite 1m 55s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 3s The patch has no ill-formed XML file. +1 findbugs 2m 23s the patch passed +1 javadoc 1m 18s the patch passed       Other Tests -1 unit 8m 15s hadoop-common in the patch failed. +1 unit 3m 44s hadoop-azure-datalake in the patch passed. +1 asflicense 0m 33s The patch does not generate ASF License warnings. 91m 16s Reason Tests Failed junit tests hadoop.security.TestKDiag   hadoop.net.TestDNS Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14627 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881161/HADOOP-14627.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux 17e81f374afd 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8d953c2 Default Java 1.8.0_131 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12999/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12999/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure-datalake U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12999/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jzhuge John Zhuge added a comment -

          Patch 003

          • Fix TestAzureADTokenProvider.testMSITokenProvider failure
          • My last review comments
          • Formatting in core-default.xml and index.md

          Testing Done

          • Passed live unit tests
          Show
          jzhuge John Zhuge added a comment - Patch 003 Fix TestAzureADTokenProvider.testMSITokenProvider failure My last review comments Formatting in core-default.xml and index.md Testing Done Passed live unit tests
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +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.
                trunk Compile Tests
          0 mvndep 1m 26s Maven dependency ordering for branch
          +1 mvninstall 20m 20s trunk passed
          +1 compile 20m 54s trunk passed
          +1 checkstyle 2m 38s trunk passed
          +1 mvnsite 2m 34s trunk passed
          +1 findbugs 2m 49s trunk passed
          +1 javadoc 1m 33s trunk passed
                Patch Compile Tests
          0 mvndep 0m 27s Maven dependency ordering for patch
          +1 mvninstall 1m 28s the patch passed
          +1 compile 16m 12s the patch passed
          +1 javac 16m 11s the patch passed
          -0 checkstyle 2m 38s root: The patch generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
          +1 mvnsite 2m 47s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 3s The patch has no ill-formed XML file.
          +1 findbugs 2m 52s the patch passed
          +1 javadoc 1m 30s the patch passed
                Other Tests
          -1 unit 9m 33s hadoop-common in the patch failed.
          -1 unit 3m 57s hadoop-azure-datalake in the patch failed.
          +1 asflicense 0m 40s The patch does not generate ASF License warnings.
          117m 24s



          Reason Tests
          Failed junit tests hadoop.fs.shell.TestCopyFromLocal
            hadoop.fs.adl.TestGetFileStatus
            hadoop.fs.adl.TestAzureADTokenProvider



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14627
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880330/HADOOP-14627.002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux af35720f8a10 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 024c3ec
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/artifact/patchprocess/patch-unit-hadoop-tools_hadoop-azure-datalake.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure-datalake U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/console
          Powered by Apache Yetus 0.6.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.       Prechecks +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.       trunk Compile Tests 0 mvndep 1m 26s Maven dependency ordering for branch +1 mvninstall 20m 20s trunk passed +1 compile 20m 54s trunk passed +1 checkstyle 2m 38s trunk passed +1 mvnsite 2m 34s trunk passed +1 findbugs 2m 49s trunk passed +1 javadoc 1m 33s trunk passed       Patch Compile Tests 0 mvndep 0m 27s Maven dependency ordering for patch +1 mvninstall 1m 28s the patch passed +1 compile 16m 12s the patch passed +1 javac 16m 11s the patch passed -0 checkstyle 2m 38s root: The patch generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) +1 mvnsite 2m 47s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 3s The patch has no ill-formed XML file. +1 findbugs 2m 52s the patch passed +1 javadoc 1m 30s the patch passed       Other Tests -1 unit 9m 33s hadoop-common in the patch failed. -1 unit 3m 57s hadoop-azure-datalake in the patch failed. +1 asflicense 0m 40s The patch does not generate ASF License warnings. 117m 24s Reason Tests Failed junit tests hadoop.fs.shell.TestCopyFromLocal   hadoop.fs.adl.TestGetFileStatus   hadoop.fs.adl.TestAzureADTokenProvider Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14627 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880330/HADOOP-14627.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux af35720f8a10 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 024c3ec Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/artifact/patchprocess/patch-unit-hadoop-tools_hadoop-azure-datalake.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure-datalake U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12966/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jzhuge John Zhuge added a comment - - edited

          Thanks Atul Sikaria for the new rev. Looking great except a few minors:

          • Use all upper case MSI except in config property names.
          • Convert fs.adl.oauth2.devicecode.ClientAppId to all lower case.
          • At AdlConfKeys.java#61, need a space after "//" in "//DeviceCode Auth configuration".
          • AdlFileSystem#getAdlStoreClient is unused.
          Show
          jzhuge John Zhuge added a comment - - edited Thanks Atul Sikaria for the new rev. Looking great except a few minors: Use all upper case MSI except in config property names. Convert fs.adl.oauth2.devicecode.ClientAppId to all lower case. At AdlConfKeys.java#61, need a space after "//" in "//DeviceCode Auth configuration". AdlFileSystem#getAdlStoreClient is unused.
          Hide
          ASikaria Atul Sikaria added a comment -

          John Zhuge, Steve Loughran: addressed all your concerns in the comments above.

          Also updating patch to not be dependent on the preview version of SDK (since released version 2.2.1 is now available).

          +Chris Douglas as FYI

          Show
          ASikaria Atul Sikaria added a comment - John Zhuge , Steve Loughran : addressed all your concerns in the comments above. Also updating patch to not be dependent on the preview version of SDK (since released version 2.2.1 is now available). + Chris Douglas as FYI
          Hide
          jzhuge John Zhuge added a comment - - edited
          • Name the patch in the format of HADOOP-14627.00X.patch.
          • Add all new properties and their default values to core-default.xml.
          • Update SDK version whenever it is GA.
          • Add unit tests for the new token providers to TestAzureADTokenProvider
          Show
          jzhuge John Zhuge added a comment - - edited Name the patch in the format of HADOOP-14627 .00X.patch. Add all new properties and their default values to core-default.xml. Update SDK version whenever it is GA. Add unit tests for the new token providers to TestAzureADTokenProvider
          Hide
          jzhuge John Zhuge added a comment -

          Atul Sikaria It does not hurt to call ```getPaswordString``` for non-secret properties. It is nice to group all related properties together in one place, the cred store. Of course, the downside is that non-secret properties are placed into cred store which can be confusing.

          Steve Loughran What do you think?

          Show
          jzhuge John Zhuge added a comment - Atul Sikaria It does not hurt to call ```getPaswordString``` for non-secret properties. It is nice to group all related properties together in one place, the cred store. Of course, the downside is that non-secret properties are placed into cred store which can be confusing. Steve Loughran What do you think?
          Hide
          ASikaria Atul Sikaria added a comment -

          Thanks Steve Loughran for reviewing. Will do a test, and fix the casing on the property name. Note on the test that it is only meaningful if run from within an Azure VM - MSI service will not be present anywhere else.

          The properties are not secrets - ok to have in cleartext.

          Show
          ASikaria Atul Sikaria added a comment - Thanks Steve Loughran for reviewing. Will do a test, and fix the casing on the property name. Note on the test that it is only meaningful if run from within an Azure VM - MSI service will not be present anywhere else. The properties are not secrets - ok to have in cleartext.
          Hide
          stevel@apache.org Steve Loughran added a comment -
          • A test would still be good, if just to verify that attempting to use the new auth mechanism fails if the configuration is missing any required property.
          • New fs.adl.oauth2.msi.TenantGuid should be all lower case, for consistency with (nearly) everything else
          • Is this property a secret which should be stored in hadoop credentials files & retrieved with Configuration.getPassword()?
          Show
          stevel@apache.org Steve Loughran added a comment - A test would still be good, if just to verify that attempting to use the new auth mechanism fails if the configuration is missing any required property. New fs.adl.oauth2.msi.TenantGuid should be all lower case, for consistency with (nearly) everything else Is this property a secret which should be stored in hadoop credentials files & retrieved with Configuration.getPassword()?
          Hide
          liuml07 Mingliang Liu added a comment -

          Current change is good. I just propose the general idea. Thanks!

          Show
          liuml07 Mingliang Liu added a comment - Current change is good. I just propose the general idea. Thanks!
          Hide
          ASikaria Atul Sikaria added a comment -

          Thanks Mingliang Liu, I thought about that too. However, I went with one Jira for this time because 1) The combined change is small (~30-40 lines of code) so it was small enough already. 2) The changes for the two auth methods were very similar, so I thought it would make it easier to review them together. 3) The biggest change is to the doc file (index.md), which would be easier to see as a final doc containing both, rather than individual isolated changes for each.

          Having said that, this is my perspective (from the patch creator's side), so I am only guessing at your usability in reading the change. Let me know if the current change is not as easy to review as I thought; if so, I can break it up.

          Show
          ASikaria Atul Sikaria added a comment - Thanks Mingliang Liu , I thought about that too. However, I went with one Jira for this time because 1) The combined change is small (~30-40 lines of code) so it was small enough already. 2) The changes for the two auth methods were very similar, so I thought it would make it easier to review them together. 3) The biggest change is to the doc file (index.md), which would be easier to see as a final doc containing both, rather than individual isolated changes for each. Having said that, this is my perspective (from the patch creator's side), so I am only guessing at your usability in reading the change. Let me know if the current change is not as easy to review as I thought; if so, I can break it up.
          Hide
          liuml07 Mingliang Liu added a comment -

          If there are multiple changes that are separable, we can make this one a Uber JIRA and create subtasks for it. Small and individual changes are better for review, test and release. Thanks,

          Show
          liuml07 Mingliang Liu added a comment - If there are multiple changes that are separable, we can make this one a Uber JIRA and create subtasks for it. Small and individual changes are better for review, test and release. Thanks,
          Hide
          ASikaria Atul Sikaria added a comment -

          Attached patch with the current code. This is to provide early opportunity to review, it should not be checked in until the SDK version it is based on removes the preview label.

          Show
          ASikaria Atul Sikaria added a comment - Attached patch with the current code. This is to provide early opportunity to review, it should not be checked in until the SDK version it is based on removes the preview label.

            People

            • Assignee:
              ASikaria Atul Sikaria
              Reporter:
              ASikaria Atul Sikaria
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development