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

Client should always ask namenode for kms provider path.

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-alpha4, 2.8.2
    • Component/s: kms
    • Labels:
      None

      Description

      According to current implementation of kms provider in client conf, there can only be one kms.
      In multi-cluster environment, if a client is reading encrypted data from multiple clusters it will only get kms token for local cluster.
      Not sure whether the target version is correct or not.

      1. HADOOP-14104-branch-2.8.patch
        38 kB
        Rushabh S Shah
      2. HADOOP-14104-branch-2.patch
        38 kB
        Rushabh S Shah
      3. HADOOP-14104-trunk.patch
        28 kB
        Rushabh S Shah
      4. HADOOP-14104-trunk-v1.patch
        30 kB
        Rushabh S Shah
      5. HADOOP-14104-trunk-v2.patch
        39 kB
        Rushabh S Shah
      6. HADOOP-14104-trunk-v3.patch
        38 kB
        Rushabh S Shah
      7. HADOOP-14104-trunk-v4.patch
        38 kB
        Rushabh S Shah
      8. HADOOP-14104-trunk-v5.patch
        38 kB
        Rushabh S Shah

        Issue Links

          Activity

          Hide
          djp Junping Du added a comment -

          The patch cause incompatible API change here as omit previous constructor of FsServerDefaults which is a public API. Just filed HADOOP-14814 and put up a fix there. Please help to review and commit/fix this issue.

          Show
          djp Junping Du added a comment - The patch cause incompatible API change here as omit previous constructor of FsServerDefaults which is a public API. Just filed HADOOP-14814 and put up a fix there. Please help to review and commit/fix this issue.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          2.8.1 became a security release. Moving fix-version to 2.8.2 after the fact.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - 2.8.1 became a security release. Moving fix-version to 2.8.2 after the fact.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hive code

          private boolean isEncryptionEnabled(DFSClient client, Configuration conf) {
            try {
              DFSClient.class.getMethod("isHDFSEncryptionEnabled");
            } catch (NoSuchMethodException e) {
              // the method is available since Hadoop-2.7.1
              // if we run with an older Hadoop, check this ourselves
              return !conf.getTrimmed(DFSConfigKeys.DFS_ENCRYPTION_KEY_PROVIDER_URI, "").isEmpty();
            }
            return client.isHDFSEncryptionEnabled();
          }
          
          Show
          yzhangal Yongjun Zhang added a comment - Hive code private boolean isEncryptionEnabled(DFSClient client, Configuration conf) { try { DFSClient.class.getMethod( "isHDFSEncryptionEnabled" ); } catch (NoSuchMethodException e) { // the method is available since Hadoop-2.7.1 // if we run with an older Hadoop, check this ourselves return !conf.getTrimmed(DFSConfigKeys.DFS_ENCRYPTION_KEY_PROVIDER_URI, "").isEmpty(); } return client.isHDFSEncryptionEnabled(); }
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks a lot Rushabh S Shah.

          I created HADOOP-14333. This is a temp solution, and we are requesting hive to make changes to use public API, we can add public APIs if they are not there. I hope we can get this in asap today.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks a lot Rushabh S Shah . I created HADOOP-14333 . This is a temp solution, and we are requesting hive to make changes to use public API, we can add public APIs if they are not there. I hope we can get this in asap today.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Since the impact is wide (many different versions of Hiv

          Yongjun Zhang which version of hive is calling ?
          I tried to search on grepcode but couldn't find any hit.

          Show
          shahrs87 Rushabh S Shah added a comment - Since the impact is wide (many different versions of Hiv Yongjun Zhang which version of hive is calling ? I tried to search on grepcode but couldn't find any hit.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Yongjun Zhang thanks for reporting !
          I will discuss with Daryn Sharp and will update this jira soon.

          Show
          shahrs87 Rushabh S Shah added a comment - Yongjun Zhang thanks for reporting ! I will discuss with Daryn Sharp and will update this jira soon.
          Hide
          yzhangal Yongjun Zhang added a comment -

          HI Rushabh Shah, Andrew Wang, Daryn Sharp,

          Just noticed that he change of adding "throws" in the public API of DFSClient breaks hive, which accesses DFSClient, thought it should not because DFSClient is private.

          public boolean isHDFSEncryptionEnabled() throws IOException{

           * Hadoop DFS users should obtain an instance of 
           * DistributedFileSystem, which uses DFSClient to handle
           * filesystem tasks.
           *
           ********************************************************/
          @InterfaceAudience.Private
          public class DFSClient implements java.io.Closeable, RemotePeerFactory,
              DataEncryptionKeyFactory {
          

          Since the impact is wide (many different versions of Hive), we can remove the throws from this API as a temporary solution. And simply return false when it throws, do you guys agree?

          We should also let hive to fix their code not to access private interface of Hadoop.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - HI Rushabh Shah , Andrew Wang , Daryn Sharp , Just noticed that he change of adding "throws" in the public API of DFSClient breaks hive, which accesses DFSClient, thought it should not because DFSClient is private. public boolean isHDFSEncryptionEnabled() throws IOException{ * Hadoop DFS users should obtain an instance of * DistributedFileSystem, which uses DFSClient to handle * filesystem tasks. * ********************************************************/ @InterfaceAudience.Private public class DFSClient implements java.io.Closeable, RemotePeerFactory, DataEncryptionKeyFactory { Since the impact is wide (many different versions of Hive), we can remove the throws from this API as a temporary solution. And simply return false when it throws, do you guys agree? We should also let hive to fix their code not to access private interface of Hadoop. Thanks.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11591 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11591/)
          HADOOP-14104. Client should always ask namenode for kms provider path. (wang: rev 18432130a7f580f206adf023507678c534487f2e)

          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/KMSUtil.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsServerDefaults.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/LocalConfigKeys.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/TransparentEncryption.md
          • (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/hdfs.proto
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestKeyProviderCache.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/KeyProviderCache.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FtpConfigKeys.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11591 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11591/ ) HADOOP-14104 . Client should always ask namenode for kms provider path. (wang: rev 18432130a7f580f206adf023507678c534487f2e) (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/KMSUtil.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsServerDefaults.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/LocalConfigKeys.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/TransparentEncryption.md (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/hdfs.proto (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestKeyProviderCache.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/KeyProviderCache.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FtpConfigKeys.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Thanks all !

          Show
          shahrs87 Rushabh S Shah added a comment - Thanks all !
          Hide
          andrew.wang Andrew Wang added a comment -

          Committed to branch-2 and branch-2.8 as well, thanks again for working on this Rushabh, as well as Yongjun and Daryn for reviews.

          Show
          andrew.wang Andrew Wang added a comment - Committed to branch-2 and branch-2.8 as well, thanks again for working on this Rushabh, as well as Yongjun and Daryn for reviews.
          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 3 new or modified test files.
          0 mvndep 1m 10s Maven dependency ordering for branch
          +1 mvninstall 6m 37s branch-2 passed
          +1 compile 5m 38s branch-2 passed with JDK v1.8.0_121
          +1 compile 6m 38s branch-2 passed with JDK v1.7.0_121
          +1 checkstyle 1m 33s branch-2 passed
          +1 mvnsite 2m 25s branch-2 passed
          +1 mvneclipse 0m 46s branch-2 passed
          +1 findbugs 5m 21s branch-2 passed
          +1 javadoc 2m 3s branch-2 passed with JDK v1.8.0_121
          +1 javadoc 3m 1s branch-2 passed with JDK v1.7.0_121
          0 mvndep 0m 16s Maven dependency ordering for patch
          +1 mvninstall 2m 1s the patch passed
          +1 compile 5m 32s the patch passed with JDK v1.8.0_121
          +1 cc 5m 32s the patch passed
          +1 javac 5m 32s the patch passed
          +1 compile 6m 40s the patch passed with JDK v1.7.0_121
          +1 cc 6m 40s the patch passed
          +1 javac 6m 40s the patch passed
          -0 checkstyle 1m 34s root: The patch generated 6 new + 457 unchanged - 2 fixed = 463 total (was 459)
          +1 mvnsite 2m 35s the patch passed
          +1 mvneclipse 0m 53s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 0s The patch has no ill-formed XML file.
          +1 findbugs 6m 31s the patch passed
          +1 javadoc 2m 15s the patch passed with JDK v1.8.0_121
          +1 javadoc 3m 11s the patch passed with JDK v1.7.0_121
          +1 unit 8m 18s hadoop-common in the patch passed with JDK v1.7.0_121.
          +1 unit 1m 19s hadoop-hdfs-client in the patch passed with JDK v1.7.0_121.
          -1 unit 49m 57s hadoop-hdfs in the patch failed with JDK v1.7.0_121.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          216m 39s



          Reason Tests
          JDK v1.8.0_121 Failed junit tests hadoop.tracing.TestTracing
            hadoop.hdfs.server.datanode.TestFsDatasetCache
          JDK v1.7.0_121 Failed junit tests hadoop.hdfs.server.blockmanagement.TestReplicationPolicyWithUpgradeDomain



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:b59b8b7
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12862314/HADOOP-14104-branch-2.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc
          uname Linux 1825e1ad1689 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 branch-2 / 9016614
          Default Java 1.7.0_121
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_121 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12042/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12042/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_121.txt
          JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12042/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12042/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 3 new or modified test files. 0 mvndep 1m 10s Maven dependency ordering for branch +1 mvninstall 6m 37s branch-2 passed +1 compile 5m 38s branch-2 passed with JDK v1.8.0_121 +1 compile 6m 38s branch-2 passed with JDK v1.7.0_121 +1 checkstyle 1m 33s branch-2 passed +1 mvnsite 2m 25s branch-2 passed +1 mvneclipse 0m 46s branch-2 passed +1 findbugs 5m 21s branch-2 passed +1 javadoc 2m 3s branch-2 passed with JDK v1.8.0_121 +1 javadoc 3m 1s branch-2 passed with JDK v1.7.0_121 0 mvndep 0m 16s Maven dependency ordering for patch +1 mvninstall 2m 1s the patch passed +1 compile 5m 32s the patch passed with JDK v1.8.0_121 +1 cc 5m 32s the patch passed +1 javac 5m 32s the patch passed +1 compile 6m 40s the patch passed with JDK v1.7.0_121 +1 cc 6m 40s the patch passed +1 javac 6m 40s the patch passed -0 checkstyle 1m 34s root: The patch generated 6 new + 457 unchanged - 2 fixed = 463 total (was 459) +1 mvnsite 2m 35s the patch passed +1 mvneclipse 0m 53s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 0s The patch has no ill-formed XML file. +1 findbugs 6m 31s the patch passed +1 javadoc 2m 15s the patch passed with JDK v1.8.0_121 +1 javadoc 3m 11s the patch passed with JDK v1.7.0_121 +1 unit 8m 18s hadoop-common in the patch passed with JDK v1.7.0_121. +1 unit 1m 19s hadoop-hdfs-client in the patch passed with JDK v1.7.0_121. -1 unit 49m 57s hadoop-hdfs in the patch failed with JDK v1.7.0_121. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 216m 39s Reason Tests JDK v1.8.0_121 Failed junit tests hadoop.tracing.TestTracing   hadoop.hdfs.server.datanode.TestFsDatasetCache JDK v1.7.0_121 Failed junit tests hadoop.hdfs.server.blockmanagement.TestReplicationPolicyWithUpgradeDomain Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12862314/HADOOP-14104-branch-2.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc uname Linux 1825e1ad1689 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 branch-2 / 9016614 Default Java 1.7.0_121 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_121 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12042/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12042/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_121.txt JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12042/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12042/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Hi Rushabh S Shah, I think you attached a wrong branch-2 patch which has 0.8 K only?

          Thanks Junping Du !
          I attached branch-2 version of another jira.
          Attached the right branch-2 patch now.

          Show
          shahrs87 Rushabh S Shah added a comment - Hi Rushabh S Shah, I think you attached a wrong branch-2 patch which has 0.8 K only? Thanks Junping Du ! I attached branch-2 version of another jira. Attached the right branch-2 patch now.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 14m 30s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 14s Maven dependency ordering for branch
          +1 mvninstall 9m 7s branch-2.8 passed
          +1 compile 6m 3s branch-2.8 passed with JDK v1.8.0_121
          +1 compile 7m 0s branch-2.8 passed with JDK v1.7.0_121
          +1 checkstyle 1m 12s branch-2.8 passed
          +1 mvnsite 2m 30s branch-2.8 passed
          +1 mvneclipse 0m 46s branch-2.8 passed
          +1 findbugs 5m 15s branch-2.8 passed
          +1 javadoc 2m 4s branch-2.8 passed with JDK v1.8.0_121
          +1 javadoc 2m 59s branch-2.8 passed with JDK v1.7.0_121
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 57s the patch passed
          +1 compile 7m 4s the patch passed with JDK v1.8.0_121
          +1 cc 7m 4s the patch passed
          +1 javac 7m 4s the patch passed
          +1 compile 7m 8s the patch passed with JDK v1.7.0_121
          +1 cc 7m 8s the patch passed
          +1 javac 7m 8s the patch passed
          -0 checkstyle 1m 14s root: The patch generated 9 new + 502 unchanged - 7 fixed = 511 total (was 509)
          +1 mvnsite 2m 33s the patch passed
          +1 mvneclipse 0m 54s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 0s The patch has no ill-formed XML file.
          +1 findbugs 6m 5s the patch passed
          +1 javadoc 2m 14s the patch passed with JDK v1.8.0_121
          +1 javadoc 3m 11s the patch passed with JDK v1.7.0_121
          +1 unit 8m 27s hadoop-common in the patch passed with JDK v1.7.0_121.
          +1 unit 1m 6s hadoop-hdfs-client in the patch passed with JDK v1.7.0_121.
          +1 unit 47m 19s hadoop-hdfs in the patch passed with JDK v1.7.0_121.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          226m 25s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:5af2af1
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12862096/HADOOP-14104-branch-2.8.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc
          uname Linux 83ede633861d 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 branch-2.8 / 9c1cf63
          Default Java 1.7.0_121
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_121 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12030/artifact/patchprocess/diff-checkstyle-root.txt
          JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12030/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12030/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 14m 30s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 14s Maven dependency ordering for branch +1 mvninstall 9m 7s branch-2.8 passed +1 compile 6m 3s branch-2.8 passed with JDK v1.8.0_121 +1 compile 7m 0s branch-2.8 passed with JDK v1.7.0_121 +1 checkstyle 1m 12s branch-2.8 passed +1 mvnsite 2m 30s branch-2.8 passed +1 mvneclipse 0m 46s branch-2.8 passed +1 findbugs 5m 15s branch-2.8 passed +1 javadoc 2m 4s branch-2.8 passed with JDK v1.8.0_121 +1 javadoc 2m 59s branch-2.8 passed with JDK v1.7.0_121 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 57s the patch passed +1 compile 7m 4s the patch passed with JDK v1.8.0_121 +1 cc 7m 4s the patch passed +1 javac 7m 4s the patch passed +1 compile 7m 8s the patch passed with JDK v1.7.0_121 +1 cc 7m 8s the patch passed +1 javac 7m 8s the patch passed -0 checkstyle 1m 14s root: The patch generated 9 new + 502 unchanged - 7 fixed = 511 total (was 509) +1 mvnsite 2m 33s the patch passed +1 mvneclipse 0m 54s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 0s The patch has no ill-formed XML file. +1 findbugs 6m 5s the patch passed +1 javadoc 2m 14s the patch passed with JDK v1.8.0_121 +1 javadoc 3m 11s the patch passed with JDK v1.7.0_121 +1 unit 8m 27s hadoop-common in the patch passed with JDK v1.7.0_121. +1 unit 1m 6s hadoop-hdfs-client in the patch passed with JDK v1.7.0_121. +1 unit 47m 19s hadoop-hdfs in the patch passed with JDK v1.7.0_121. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 226m 25s Subsystem Report/Notes Docker Image:yetus/hadoop:5af2af1 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12862096/HADOOP-14104-branch-2.8.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc uname Linux 83ede633861d 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 branch-2.8 / 9c1cf63 Default Java 1.7.0_121 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_121 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12030/artifact/patchprocess/diff-checkstyle-root.txt JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12030/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12030/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          djp Junping Du added a comment -

          Hi Rushabh S Shah, I think you attached a wrong branch-2 patch which has 0.8 K only?

          Show
          djp Junping Du added a comment - Hi Rushabh S Shah , I think you attached a wrong branch-2 patch which has 0.8 K only?
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Andrew Wang Thanks for reviewing and committing to trunk.
          Yongjun Zhang Daryn Sharp: Thanks for reviews.

          Show
          shahrs87 Rushabh S Shah added a comment - Andrew Wang Thanks for reviewing and committing to trunk. Yongjun Zhang Daryn Sharp : Thanks for reviews.
          Hide
          shahrs87 Rushabh S Shah added a comment - - edited

          Attaching branch-2 and branch-2.8 patch.
          No code change compared to trunk.

          Show
          shahrs87 Rushabh S Shah added a comment - - edited Attaching branch-2 and branch-2.8 patch. No code change compared to trunk.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Andrew Wang for committing. Nice work Rushabh S Shah!

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Andrew Wang for committing. Nice work Rushabh S Shah !
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11521 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11521/)
          HADOOP-14104. Client should always ask namenode for kms provider path. (wang: rev 18432130a7f580f206adf023507678c534487f2e)

          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/KMSUtil.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FtpConfigKeys.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/TransparentEncryption.md
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/hdfs.proto
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/LocalConfigKeys.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestKeyProviderCache.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsServerDefaults.java
          • (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/KeyProviderCache.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11521 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11521/ ) HADOOP-14104 . Client should always ask namenode for kms provider path. (wang: rev 18432130a7f580f206adf023507678c534487f2e) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/KMSUtil.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ftp/FtpConfigKeys.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/TransparentEncryption.md (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/hdfs.proto (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/local/LocalConfigKeys.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestKeyProviderCache.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FsServerDefaults.java (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/KeyProviderCache.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          Hide
          andrew.wang Andrew Wang added a comment -

          Committed to trunk, leaving this open for the branch-2 and branch-2.8 patches. Thanks again Rushabh!

          Show
          andrew.wang Andrew Wang added a comment - Committed to trunk, leaving this open for the branch-2 and branch-2.8 patches. Thanks again Rushabh!
          Hide
          andrew.wang Andrew Wang added a comment -

          +1 LGTM, Yongjun also pinged me to say it looked good to him. I'll commit this shortly.

          Show
          andrew.wang Andrew Wang added a comment - +1 LGTM, Yongjun also pinged me to say it looked good to him. I'll commit this shortly.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 23s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 13s Maven dependency ordering for branch
          +1 mvninstall 12m 47s trunk passed
          +1 compile 16m 16s trunk passed
          +1 checkstyle 1m 59s trunk passed
          +1 mvnsite 2m 40s trunk passed
          +1 mvneclipse 0m 59s trunk passed
          +1 findbugs 4m 54s trunk passed
          +1 javadoc 2m 8s trunk passed
          0 mvndep 0m 16s Maven dependency ordering for patch
          +1 mvninstall 2m 1s the patch passed
          +1 compile 14m 28s the patch passed
          +1 cc 14m 28s the patch passed
          +1 javac 14m 28s the patch passed
          -0 checkstyle 2m 11s root: The patch generated 6 new + 475 unchanged - 2 fixed = 481 total (was 477)
          +1 mvnsite 2m 55s the patch passed
          +1 mvneclipse 1m 8s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 5m 28s the patch passed
          +1 javadoc 2m 16s the patch passed
          -1 unit 7m 51s hadoop-common in the patch failed.
          +1 unit 1m 8s hadoop-hdfs-client in the patch passed.
          +1 unit 69m 18s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 37s The patch does not generate ASF License warnings.
          176m 50s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12861904/HADOOP-14104-trunk-v5.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc
          uname Linux b68e09d7a21e 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 / 6eba792
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12021/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12021/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12021/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12021/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 23s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 13s Maven dependency ordering for branch +1 mvninstall 12m 47s trunk passed +1 compile 16m 16s trunk passed +1 checkstyle 1m 59s trunk passed +1 mvnsite 2m 40s trunk passed +1 mvneclipse 0m 59s trunk passed +1 findbugs 4m 54s trunk passed +1 javadoc 2m 8s trunk passed 0 mvndep 0m 16s Maven dependency ordering for patch +1 mvninstall 2m 1s the patch passed +1 compile 14m 28s the patch passed +1 cc 14m 28s the patch passed +1 javac 14m 28s the patch passed -0 checkstyle 2m 11s root: The patch generated 6 new + 475 unchanged - 2 fixed = 481 total (was 477) +1 mvnsite 2m 55s the patch passed +1 mvneclipse 1m 8s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 5m 28s the patch passed +1 javadoc 2m 16s the patch passed -1 unit 7m 51s hadoop-common in the patch failed. +1 unit 1m 8s hadoop-hdfs-client in the patch passed. +1 unit 69m 18s hadoop-hdfs in the patch passed. +1 asflicense 0m 37s The patch does not generate ASF License warnings. 176m 50s Reason Tests Failed junit tests hadoop.security.TestKDiag Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12861904/HADOOP-14104-trunk-v5.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc uname Linux b68e09d7a21e 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 / 6eba792 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12021/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12021/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12021/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12021/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Hopefully the last patch for trunk.
          Andrew Wang Yongjun Zhang: I really appreciate your feedback.
          Thanks for being extremely patient with me.
          Attaching a new patch addressing your comments and fixing some checkstyle issues.
          Please review.
          If this patch is good for trunk, I will then attach branch-2 and branch-2.8 patch immediately.

          Test failures from previous precommit build. Both tests ran fine on my machine.
          1. hadoop.security.TestKDiag: Known to be flaky. Tracked via HADOOP-14030
          2. hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting:
          This also is known to be flaky. Tried to fix via HDFS-11316, HDFS-11398. But still flaky.

          Show
          shahrs87 Rushabh S Shah added a comment - Hopefully the last patch for trunk. Andrew Wang Yongjun Zhang : I really appreciate your feedback. Thanks for being extremely patient with me. Attaching a new patch addressing your comments and fixing some checkstyle issues. Please review. If this patch is good for trunk, I will then attach branch-2 and branch-2.8 patch immediately. Test failures from previous precommit build. Both tests ran fine on my machine. 1. hadoop.security.TestKDiag: Known to be flaky. Tracked via HADOOP-14030 2. hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting: This also is known to be flaky. Tried to fix via HDFS-11316 , HDFS-11398 . But still flaky.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Thanks Andrew Wang and Yongjun Zhang for reviewing and for your valubale feedback.
          Will upload a new patch by EOB tomorrow addressing your review comments.

          Show
          shahrs87 Rushabh S Shah added a comment - Thanks Andrew Wang and Yongjun Zhang for reviewing and for your valubale feedback. Will upload a new patch by EOB tomorrow addressing your review comments.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Rushabh, thanks for revving. Yongjun and I reviewed this together, posted here are our combined review comments. Looks really good overall!

          Nits:

          • Change DFS_KMS_PREFIX to private
          • Rename getKmsSecretKey to getKeyProviderMapKey (included in item below), since "SecretKey" sounds like an encryption key, a javadoc would also help

          Bigger things:

          In DistributedFileSystem, this changes the uri passed to DFSClient:

             this.dfs = new DFSClient(uri, conf, statistics);
             this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority());
          

          to

             this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority());
             this.dfs = new DFSClient(uri, conf, statistics);
          

          To be safe, I'd suggest that we don't change the order of the above code, and instead change the method in DFSClient.java to just grab the scheme and authority:

           public Text getKmsSecretKey() {
              return new Text(DFS_KMS_PREFIX + namenodeUri.toString());
            }
          

          to

            public Text getKeyProviderMapKey() {
              return new Text(DFS_KMS_PREFIX + nnUri.getScheme()
                  + "://" + nnUri.getAuthority());
            }
          
          Show
          andrew.wang Andrew Wang added a comment - Hi Rushabh, thanks for revving. Yongjun and I reviewed this together, posted here are our combined review comments. Looks really good overall! Nits: Change DFS_KMS_PREFIX to private Rename getKmsSecretKey to getKeyProviderMapKey (included in item below), since "SecretKey" sounds like an encryption key, a javadoc would also help Bigger things: In DistributedFileSystem, this changes the uri passed to DFSClient: this .dfs = new DFSClient(uri, conf, statistics); this .uri = URI.create(uri.getScheme()+ ": //" +uri.getAuthority()); to this .uri = URI.create(uri.getScheme()+ ": //" +uri.getAuthority()); this .dfs = new DFSClient(uri, conf, statistics); To be safe, I'd suggest that we don't change the order of the above code, and instead change the method in DFSClient.java to just grab the scheme and authority: public Text getKmsSecretKey() { return new Text(DFS_KMS_PREFIX + namenodeUri.toString()); } to public Text getKeyProviderMapKey() { return new Text(DFS_KMS_PREFIX + nnUri.getScheme() + ": //" + nnUri.getAuthority()); }
          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 3 new or modified test files.
          0 mvndep 1m 33s Maven dependency ordering for branch
          +1 mvninstall 14m 14s trunk passed
          +1 compile 15m 26s trunk passed
          +1 checkstyle 2m 1s trunk passed
          +1 mvnsite 2m 41s trunk passed
          +1 mvneclipse 0m 58s trunk passed
          +1 findbugs 4m 50s trunk passed
          +1 javadoc 2m 8s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 2m 2s the patch passed
          +1 compile 13m 5s the patch passed
          +1 cc 13m 5s the patch passed
          +1 javac 13m 5s the patch passed
          -0 checkstyle 2m 3s root: The patch generated 9 new + 475 unchanged - 2 fixed = 484 total (was 477)
          +1 mvnsite 2m 47s the patch passed
          +1 mvneclipse 1m 8s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 5m 31s the patch passed
          +1 javadoc 2m 17s the patch passed
          -1 unit 7m 57s hadoop-common in the patch failed.
          +1 unit 1m 7s hadoop-hdfs-client in the patch passed.
          -1 unit 71m 4s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 49s The patch does not generate ASF License warnings.
          179m 11s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12861654/HADOOP-14104-trunk-v4.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc
          uname Linux 49d97b6d6637 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 / 845529b
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/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 3 new or modified test files. 0 mvndep 1m 33s Maven dependency ordering for branch +1 mvninstall 14m 14s trunk passed +1 compile 15m 26s trunk passed +1 checkstyle 2m 1s trunk passed +1 mvnsite 2m 41s trunk passed +1 mvneclipse 0m 58s trunk passed +1 findbugs 4m 50s trunk passed +1 javadoc 2m 8s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 2m 2s the patch passed +1 compile 13m 5s the patch passed +1 cc 13m 5s the patch passed +1 javac 13m 5s the patch passed -0 checkstyle 2m 3s root: The patch generated 9 new + 475 unchanged - 2 fixed = 484 total (was 477) +1 mvnsite 2m 47s the patch passed +1 mvneclipse 1m 8s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 5m 31s the patch passed +1 javadoc 2m 17s the patch passed -1 unit 7m 57s hadoop-common in the patch failed. +1 unit 1m 7s hadoop-hdfs-client in the patch passed. -1 unit 71m 4s hadoop-hdfs in the patch failed. +1 asflicense 0m 49s The patch does not generate ASF License warnings. 179m 11s Reason Tests Failed junit tests hadoop.security.TestKDiag   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12861654/HADOOP-14104-trunk-v4.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc uname Linux 49d97b6d6637 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 / 845529b Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12008/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Addressed some of the checkstyle comments.
          Since the config key name "CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH" is so big, it can't fit in a line within 80 characters and remaining are not added by this patch.
          Test failures:
          1. TestWebHdfsTimeouts - Fails consistently with and without the patch.
          Tracked via HDFS-11043
          2. TestDataNodeVolumeFailureReporting - Passes locally on my box. Ran multiple times. Seems like a flaky test.
          3. TestDFSZKFailoverController - Passes locally on my box. Proven to be a flaky test. We tried to fix it via HDFS-9333 but still failing. Left a comment on HDFS-9333.

          Show
          shahrs87 Rushabh S Shah added a comment - Addressed some of the checkstyle comments. Since the config key name "CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH" is so big, it can't fit in a line within 80 characters and remaining are not added by this patch. Test failures: 1. TestWebHdfsTimeouts - Fails consistently with and without the patch. Tracked via HDFS-11043 2. TestDataNodeVolumeFailureReporting - Passes locally on my box. Ran multiple times. Seems like a flaky test. 3. TestDFSZKFailoverController - Passes locally on my box. Proven to be a flaky test. We tried to fix it via HDFS-9333 but still failing. Left a comment on HDFS-9333 .
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Removing the patch to address checkstyle comments.

          Show
          shahrs87 Rushabh S Shah added a comment - Removing the patch to address checkstyle comments.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 1m 47s Maven dependency ordering for branch
          +1 mvninstall 12m 56s trunk passed
          +1 compile 14m 46s trunk passed
          +1 checkstyle 1m 54s trunk passed
          +1 mvnsite 2m 39s trunk passed
          +1 mvneclipse 1m 5s trunk passed
          +1 findbugs 4m 39s trunk passed
          +1 javadoc 1m 51s trunk passed
          0 mvndep 0m 13s Maven dependency ordering for patch
          +1 mvninstall 1m 58s the patch passed
          +1 compile 13m 11s the patch passed
          +1 cc 13m 11s the patch passed
          +1 javac 13m 11s the patch passed
          -0 checkstyle 1m 42s root: The patch generated 16 new + 475 unchanged - 2 fixed = 491 total (was 477)
          +1 mvnsite 2m 31s the patch passed
          +1 mvneclipse 0m 47s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 5m 8s the patch passed
          +1 javadoc 2m 27s the patch passed
          +1 unit 7m 15s hadoop-common in the patch passed.
          +1 unit 1m 13s hadoop-hdfs-client in the patch passed.
          -1 unit 61m 32s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 44s The patch does not generate ASF License warnings.
          165m 25s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting
            hadoop.hdfs.web.TestWebHdfsTimeouts
            hadoop.hdfs.tools.TestDFSZKFailoverController



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12861492/HADOOP-14104-trunk-v3.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc
          uname Linux f9179210540c 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 318bfb0
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12002/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12002/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12002/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12002/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 21s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 1m 47s Maven dependency ordering for branch +1 mvninstall 12m 56s trunk passed +1 compile 14m 46s trunk passed +1 checkstyle 1m 54s trunk passed +1 mvnsite 2m 39s trunk passed +1 mvneclipse 1m 5s trunk passed +1 findbugs 4m 39s trunk passed +1 javadoc 1m 51s trunk passed 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 1m 58s the patch passed +1 compile 13m 11s the patch passed +1 cc 13m 11s the patch passed +1 javac 13m 11s the patch passed -0 checkstyle 1m 42s root: The patch generated 16 new + 475 unchanged - 2 fixed = 491 total (was 477) +1 mvnsite 2m 31s the patch passed +1 mvneclipse 0m 47s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 5m 8s the patch passed +1 javadoc 2m 27s the patch passed +1 unit 7m 15s hadoop-common in the patch passed. +1 unit 1m 13s hadoop-hdfs-client in the patch passed. -1 unit 61m 32s hadoop-hdfs in the patch failed. +1 asflicense 0m 44s The patch does not generate ASF License warnings. 165m 25s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting   hadoop.hdfs.web.TestWebHdfsTimeouts   hadoop.hdfs.tools.TestDFSZKFailoverController Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12861492/HADOOP-14104-trunk-v3.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc uname Linux f9179210540c 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 318bfb0 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12002/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12002/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12002/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12002/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Thanks Daryn Sharp Andrew Wang Yongjun Zhang for your valuable reviews.
          I tried to address most of the review comments.
          For the following comment:

          basically I wanted a unit test that did an encrypted read/write using the KP URI from the credentials.

          I added another unit test TestEncryptionZones#testEncryptedReadWriteUsingDiffKeyProvider
          This test tried to read/write a file from/to encrypted zone.
          The test adds key provider uri to credentials object and then unsets the local conf.
          While reading the file, it gets the key provider uri from the credentials object.
          The way we try to resolve the provider uri is first credentials map, then from namenode and then from conf.
          So among this chain, the testcase can get the key provider uri from namenode but I verified by adding a log line that it got from credentials map.

          Note: I haven't resolved the checkstyle warnings from the previous patch since the precommit build logs were removed from jenkins server.
          Once the precommit build runs, I will fix all the checkstyle warnings and then upload another patch.
          Request for reviews.

          Show
          shahrs87 Rushabh S Shah added a comment - Thanks Daryn Sharp Andrew Wang Yongjun Zhang for your valuable reviews. I tried to address most of the review comments. For the following comment: basically I wanted a unit test that did an encrypted read/write using the KP URI from the credentials. I added another unit test TestEncryptionZones#testEncryptedReadWriteUsingDiffKeyProvider This test tried to read/write a file from/to encrypted zone. The test adds key provider uri to credentials object and then unsets the local conf. While reading the file, it gets the key provider uri from the credentials object. The way we try to resolve the provider uri is first credentials map, then from namenode and then from conf. So among this chain, the testcase can get the key provider uri from namenode but I verified by adding a log line that it got from credentials map. Note: I haven't resolved the checkstyle warnings from the previous patch since the precommit build logs were removed from jenkins server. Once the precommit build runs, I will fix all the checkstyle warnings and then upload another patch. Request for reviews.
          Hide
          andrew.wang Andrew Wang added a comment -

          I think a full YARN job is a bit much; basically I wanted a unit test that did an encrypted read/write using the KP URI from the credentials.

          This might be partially addressed by an existing HDFS encryption test, but I'd like to see the same verification as in the newly added unit tests that the value from the credentials overrides the value in the config.

          Show
          andrew.wang Andrew Wang added a comment - I think a full YARN job is a bit much; basically I wanted a unit test that did an encrypted read/write using the KP URI from the credentials. This might be partially addressed by an existing HDFS encryption test, but I'd like to see the same verification as in the newly added unit tests that the value from the credentials overrides the value in the config.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks much Rushabh S Shah!

          Hi Andrew Wang, about your comment Rushabh was asking, would you please elaborate? seems a more comprehensive test, do you agree that we could address it in later jira? Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks much Rushabh S Shah ! Hi Andrew Wang , about your comment Rushabh was asking, would you please elaborate? seems a more comprehensive test, do you agree that we could address it in later jira? Thanks.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Wonder if you have chance to look at Daryn's comments?

          I was out last week and before that busy working on internal stuff.
          I should be able to upload a new patch today.

          An actual mini-cluster test that mimics an MR job submitter and task's call pattern would also be good.

          I was able to address all the review comments except the above one.
          Only way I can think to mimic job submitter is by creating MiniYARNCluster. I don't have any experience writing yarn tests. I may take additional time if you want me to write miniyarn cluster test.
          Yongjun Zhang, Andrew Wang: Any pointers would be appreciated.

          Show
          shahrs87 Rushabh S Shah added a comment - Wonder if you have chance to look at Daryn's comments? I was out last week and before that busy working on internal stuff. I should be able to upload a new patch today. An actual mini-cluster test that mimics an MR job submitter and task's call pattern would also be good. I was able to address all the review comments except the above one. Only way I can think to mimic job submitter is by creating MiniYARNCluster. I don't have any experience writing yarn tests. I may take additional time if you want me to write miniyarn cluster test. Yongjun Zhang , Andrew Wang : Any pointers would be appreciated.
          Hide
          yzhangal Yongjun Zhang added a comment - - edited

          Thanks Daryn Sharp a lot for the review and comments.

          Hi Rushabh S Shah,

          Wonder if you have chance to look at Daryn's comments?

          Many thanks!

          Show
          yzhangal Yongjun Zhang added a comment - - edited Thanks Daryn Sharp a lot for the review and comments. Hi Rushabh S Shah , Wonder if you have chance to look at Daryn's comments? Many thanks!
          Hide
          daryn Daryn Sharp added a comment -

          Still scrutinizing, initial comments:

          Regarding need for constants, in general, would be preferable for “null” to mean no provider instead of both empty string and null meaning no provider – in which case a constant is not necessary.

          Regarding concerns over getServerDefaults being called via webhdfs: if you are using EZ with webhdfs, one little getServerDefaults call is the least of your worries... You should terrified that apparently one compromised DN renders encryption moot. At any rate, we'll see what we can do to reduce the number of calls from webhdfs.

          DFSClient

          1. get/setKeyProviderUri methods should use URI instances, not strings, as name implies. The caller should have already done the string to uri conversions.
          2. getServerDefaults never returns null, so don’t null check. Just assign keyProviderUri and check for null.

          KMSUtil

          1. If createKeyProviderFromUri(Configuration, String) fails to create a client, it throws with a hardcoded CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH which may not be the key that was used. Although see comments in KeyProviderCache that may negate need for these changes at all.

          KeyProviderCache

          1. The new get(Configuration,String) should take a URI, not String, and just look it up. No need to perform all the String -> URI logic for every lookup since DFSClient should call with a computed URI.
          2. Consider just calling KeyProviderFactory.get(uri, conf) directly instead of roundabout trip through DFSUtilClient/KMSUtil. Then you probably don’t need to change those classes since their methods are all about getting the key provider from the config. We know what the URI is.
          3. Minor, in get, leave the unwrapping of the ExecutionException in place.

          DFSClient

          1. In isHDFSEncryptionEnabled, it’s completely broken to swallow exceptions and return false… It’s not just a StandbyException, per the comment, that may bubble out. It could be the retry proxy giving up, connect refused, etc. Change this method to allow IOE because DFSClient is a private interface. Then the filesystem’s getTrashRoot can do the wrong thing by swallowing the exception.

          DistributedFileSystem

          1. I’d rather see addDelegationTokens not call the problematic isHDFSEncryptionEnabled method, but instead call dfs.getKeyProvider(). If not null, do the block of code to get a kms token.

          I'll double check that token selection works correctly.

          Show
          daryn Daryn Sharp added a comment - Still scrutinizing, initial comments: Regarding need for constants, in general, would be preferable for “null” to mean no provider instead of both empty string and null meaning no provider – in which case a constant is not necessary. Regarding concerns over getServerDefaults being called via webhdfs: if you are using EZ with webhdfs, one little getServerDefaults call is the least of your worries... You should terrified that apparently one compromised DN renders encryption moot. At any rate, we'll see what we can do to reduce the number of calls from webhdfs. DFSClient get/setKeyProviderUri methods should use URI instances, not strings, as name implies. The caller should have already done the string to uri conversions. getServerDefaults never returns null, so don’t null check. Just assign keyProviderUri and check for null. KMSUtil If createKeyProviderFromUri(Configuration, String) fails to create a client, it throws with a hardcoded CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH which may not be the key that was used. Although see comments in KeyProviderCache that may negate need for these changes at all. KeyProviderCache The new get(Configuration,String) should take a URI, not String, and just look it up. No need to perform all the String -> URI logic for every lookup since DFSClient should call with a computed URI. Consider just calling KeyProviderFactory.get(uri, conf) directly instead of roundabout trip through DFSUtilClient/KMSUtil. Then you probably don’t need to change those classes since their methods are all about getting the key provider from the config. We know what the URI is. Minor, in get , leave the unwrapping of the ExecutionException in place. DFSClient In isHDFSEncryptionEnabled , it’s completely broken to swallow exceptions and return false… It’s not just a StandbyException, per the comment, that may bubble out. It could be the retry proxy giving up, connect refused, etc. Change this method to allow IOE because DFSClient is a private interface. Then the filesystem’s getTrashRoot can do the wrong thing by swallowing the exception. DistributedFileSystem I’d rather see addDelegationTokens not call the problematic isHDFSEncryptionEnabled method, but instead call dfs.getKeyProvider() . If not null, do the block of code to get a kms token. I'll double check that token selection works correctly.
          Hide
          daryn Daryn Sharp added a comment -

          Sorry for the delay, I've been OOO on and off the past week. Reviewing this morning.

          Show
          daryn Daryn Sharp added a comment - Sorry for the delay, I've been OOO on and off the past week. Reviewing this morning.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Rushabh Shah,

          Thanks for pointing it out. Because DFSClient#isHDFSEncryptionEnabled is a public method of DFSClient, I thought anyone could call it including any mapreduce task.

          Not an ask for this jira, - we can do it later - If this method is only intended to be called by DistributedFileSystem#addDelegationTokens and DistributedFileSystem.getTrashRoot, maybe we can add a javadoc to indicate that. BTW, One could create FsShell object and call shell from any java code too, we can worry about that later.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Rushabh Shah , Thanks for pointing it out. Because DFSClient#isHDFSEncryptionEnabled is a public method of DFSClient , I thought anyone could call it including any mapreduce task. Not an ask for this jira, - we can do it later - If this method is only intended to be called by DistributedFileSystem#addDelegationTokens and DistributedFileSystem.getTrashRoot , maybe we can add a javadoc to indicate that. BTW, One could create FsShell object and call shell from any java code too, we can worry about that later. Thanks.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          This means, If encryption is not enabled, then we don't put any entry into the secretMap, and each task of mapreduce job will always call getServerDefaults, which we try to avoid.

          I don't follow this.
          DFSClient#isHDFSEncryptionEnabled is being only called by DistributedFileSystem#addDelegationTokens (job submission) and DistributedFileSystem.getTrashRoot (FSShell commands).
          DFSClient#getKeyProvider is being called by DistributedFileSystem#addDelegationTokens (again job submission) and DFSClient#decryptEncryptedDataEncryptionKey.
          When calling via DFSClient#decryptEncryptedDataEncryptionKey, we already know that this directory or parent directory is already encrypted.
          So I don't understand why each mapreduce task which is accessing non-encrypted directory will call getKeyProvider and in turn call serverDefaults ?
          Am I missing something ?

          Show
          shahrs87 Rushabh S Shah added a comment - This means, If encryption is not enabled, then we don't put any entry into the secretMap, and each task of mapreduce job will always call getServerDefaults, which we try to avoid. I don't follow this. DFSClient#isHDFSEncryptionEnabled is being only called by DistributedFileSystem#addDelegationTokens (job submission) and DistributedFileSystem.getTrashRoot (FSShell commands). DFSClient#getKeyProvider is being called by DistributedFileSystem#addDelegationTokens (again job submission) and DFSClient#decryptEncryptedDataEncryptionKey . When calling via DFSClient#decryptEncryptedDataEncryptionKey , we already know that this directory or parent directory is already encrypted. So I don't understand why each mapreduce task which is accessing non-encrypted directory will call getKeyProvider and in turn call serverDefaults ? Am I missing something ?
          Hide
          yzhangal Yongjun Zhang added a comment -

          HI Rushabh Shah,

          To avoid confusions to other reviewers, I'm taking out rev3. Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - HI Rushabh Shah , To avoid confusions to other reviewers, I'm taking out rev3. Thanks.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Andrew Wang.

          The FileEncryptionInfo has a cipher type field. If the client doesn't support the cipher, then it can't read/write the file, and will throw an exception

          Does the exception get thrown when we do final CryptoCodec codec = getCryptoCodec(conf, feInfo); or later when we write file? I prefer the former.

          BTW Rushabh Shah,

          In rev2, DistributedFileSyste#addDelegationTokens put the keyProvider entry to scretMap only when dfs.isHDFSEncryptionEnabled() is true. dfs.isHDFSEncryptionEnabled() calls getServerDefaults to find out whether encryption is enabled when there is no keyProvider entry in the secretMap. This means, If encryption is not enabled, then we don't put any entry into the secretMap, and each task of mapreduce job will always call getServerDefaults, which we try to avoid.

          I suggest to add an empty entry to the secretMap after we call getServerDefaults for the first time and find encryption is disabled. Then later if we find an entry in the map, and if it's empty, we know encryption is not enabled, if it's non-empty, it's enabled. This is what I tried to do in rev3 to avoid the extra getServerDefaults call for the case when encryption is disabled. For your reference.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Andrew Wang . The FileEncryptionInfo has a cipher type field. If the client doesn't support the cipher, then it can't read/write the file, and will throw an exception Does the exception get thrown when we do final CryptoCodec codec = getCryptoCodec(conf, feInfo); or later when we write file? I prefer the former. BTW Rushabh Shah , In rev2, DistributedFileSyste#addDelegationTokens put the keyProvider entry to scretMap only when dfs.isHDFSEncryptionEnabled() is true. dfs.isHDFSEncryptionEnabled() calls getServerDefaults to find out whether encryption is enabled when there is no keyProvider entry in the secretMap. This means, If encryption is not enabled, then we don't put any entry into the secretMap, and each task of mapreduce job will always call getServerDefaults, which we try to avoid. I suggest to add an empty entry to the secretMap after we call getServerDefaults for the first time and find encryption is disabled. Then later if we find an entry in the map, and if it's empty, we know encryption is not enabled, if it's non-empty, it's enabled. This is what I tried to do in rev3 to avoid the extra getServerDefaults call for the case when encryption is disabled. For your reference. Thanks.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Yongjun,

          When the target is the remote cluster, could we fail to find Codec info or wrong codec info because we don't have remote cluster's configuration?

          The FileEncryptionInfo has a cipher type field. If the client doesn't support the cipher, then it can't read/write the file, and will throw an exception. If it does support that cipher, it uses its local config to determine the correct codec implementation to use (i.e. java or native).

          From a safety point of view, we're okay to use the local config. If the client is too old and doesn't understand a new cipher type, it'll abort. Supporting a new cipher necessarily requires upgrading the client (and potentially also installing native libraries), so I think this behavior is okay.

          Show
          andrew.wang Andrew Wang added a comment - Hi Yongjun, When the target is the remote cluster, could we fail to find Codec info or wrong codec info because we don't have remote cluster's configuration? The FileEncryptionInfo has a cipher type field. If the client doesn't support the cipher, then it can't read/write the file, and will throw an exception. If it does support that cipher, it uses its local config to determine the correct codec implementation to use (i.e. java or native). From a safety point of view, we're okay to use the local config. If the client is too old and doesn't understand a new cipher type, it'll abort. Supporting a new cipher necessarily requires upgrading the client (and potentially also installing native libraries), so I think this behavior is okay.
          Hide
          yzhangal Yongjun Zhang added a comment -

          HI Rushabh Shah,

          Thanks again for your work. Below are my comments in reply to yours. Just saw Andrew Wang's, thanks Andrew!

          1.

          final FileEncryptionInfo feInfo = dfsos.getFileEncryptionInfo();
          final CryptoCodec codec = getCryptoCodec(conf, feInfo);
          in createWrappedOutputStream, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here.

          Sorry I was a bit unclear earlier, this comment is not about the change of this jira, but about the existing implementation. My concern is about Codec rather than keyProvider here. We get feInfo from the target file, getting Codec based on conf and feInfo. The conf here is the configuration of the local cluster. When the target is the remote cluster, could we fail to find Codec info or wrong codec info because we don't have remote cluster's configuration? That's what wanted to say. So I hope Andrew Wang who was involved in the original development of encryption feature could comment. (Andrew, saw you above comment, but would you please look at my comment here again and see if it makes sense?

          2. It looks having a HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT constant would help making the code more consistent, even for the patch here, but having a separate jira works for me too.

          3. Keeping the name keyProviderUri instead of keyProviderPath is actually fine. I did some study before creating patch rev3, the format appears to be Uri. I wish HDFS-10489 made it uri instead of path. So I did not change uri to path in rev3.

          4. About adding two methods to add/get from credentials, it's a way of encapsulating how it's handled in one place, and share the way how the key in the map is generated (e.g. uri.getScheme()+"://"+uri.getAuthority()). You can see my example in rev3. These methodd are also called in Test code.

          5.

          I don't think key provider is used by WebHDFSFileSystem. Maybe I'm missing something.
          Can you please elaborate your comment ?

          I was just guessing, but I'm not so sure, but hope Daryn Sharp can comment.

          6.

          7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this.

          For HA cluster, we can access NN via nameservice (for example, hadoop distcp hdfs://nameservice1:/xyz hdfs://nameservice2:/abc) , so the StandbyException can be detected and different NN will be tried automatically. See https://issues.apache.org/jira/browse/HDFS-6376. We actually see a non-intrusive way to do that without using dfs.internal.nameservices", that is, we copy the local cluster's conf to a new dir, and append the nameservice portion of the remote cluster's conf to the hdfs-site.xml in the new dir. Then pass the new dir to distcp.

          Show
          yzhangal Yongjun Zhang added a comment - HI Rushabh Shah , Thanks again for your work. Below are my comments in reply to yours. Just saw Andrew Wang 's, thanks Andrew! 1. final FileEncryptionInfo feInfo = dfsos.getFileEncryptionInfo(); final CryptoCodec codec = getCryptoCodec(conf, feInfo); in createWrappedOutputStream, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here. Sorry I was a bit unclear earlier, this comment is not about the change of this jira, but about the existing implementation. My concern is about Codec rather than keyProvider here. We get feInfo from the target file, getting Codec based on conf and feInfo. The conf here is the configuration of the local cluster. When the target is the remote cluster, could we fail to find Codec info or wrong codec info because we don't have remote cluster's configuration? That's what wanted to say. So I hope Andrew Wang who was involved in the original development of encryption feature could comment. (Andrew, saw you above comment, but would you please look at my comment here again and see if it makes sense? 2. It looks having a HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT constant would help making the code more consistent, even for the patch here, but having a separate jira works for me too. 3. Keeping the name keyProviderUri instead of keyProviderPath is actually fine. I did some study before creating patch rev3, the format appears to be Uri. I wish HDFS-10489 made it uri instead of path. So I did not change uri to path in rev3. 4. About adding two methods to add/get from credentials, it's a way of encapsulating how it's handled in one place, and share the way how the key in the map is generated (e.g. uri.getScheme()+"://"+uri.getAuthority()). You can see my example in rev3. These methodd are also called in Test code. 5. I don't think key provider is used by WebHDFSFileSystem. Maybe I'm missing something. Can you please elaborate your comment ? I was just guessing, but I'm not so sure, but hope Daryn Sharp can comment. 6. 7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this. For HA cluster, we can access NN via nameservice (for example, hadoop distcp hdfs://nameservice1:/xyz hdfs://nameservice2:/abc) , so the StandbyException can be detected and different NN will be tried automatically. See https://issues.apache.org/jira/browse/HDFS-6376 . We actually see a non-intrusive way to do that without using dfs.internal.nameservices", that is, we copy the local cluster's conf to a new dir, and append the nameservice portion of the remote cluster's conf to the hdfs-site.xml in the new dir. Then pass the new dir to distcp.
          Hide
          andrew.wang Andrew Wang added a comment -

          Here's my review on v2, along with responses to some of Yongjun's questions. Thanks for working on this Rushabh!

          in createWrappedOutputStream(, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here.

          The conf is only used to configure the CryptoCodec, which does not need a KMS URI. I think this is okay, all the CC parameters are included in the feInfo.

          @awang would you please confirm if it's ok to do so since this class is public), and use this constant at multiple places that current uses ""

          Yea it's fine. I would like to improve this in this patch if possible, since it removes redundancies.

          3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489). So suggest to replace variable name keyProviderUri with keyProviderPath

          I think we can ignore this for now like Rushabh said, can handle it in another JIRA if necessary.

          Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens

          The DN does the encryption/decryption in WebHDFS, so the client doesn't need to do any KMS communication.

          It does bring up a question regarding the DN DFSClient though. It looks like WebHdfsHandler creates a new DFSClient each time, which means we won't benefit from getServerDefaults caching.

          Is the fix to make config preferred over getServerDefaults?

          <StandbyException>

          Does this exception actually make it to clients? The HA RPC proxy normally catches the StandbyException and fails over to the other NN. We can write a unit test for this to verify if we're unsure.

          My additional review comments:

          • typo nameonde
          • ServerDefaults#getKeyProviderUri needs javadoc explaining how to interpret null and empty (IIUC null means not set, empty means means set but not enabled)
          • In docs, "DFSClients" is an internal name. Please rename to say "HDFS clients" or similar. Same for "dfs clients" in core-default.xml
          • There are a lot of whitespace errors, please take a look at what's flagged by checkstyle. Recommend using IDE autoformatting in the future.
          • An actual mini-cluster test that mimics an MR job submitter and task's call pattern would also be good.

          TestEncryptionZones:

          • Would like to see the test reversed so it covers the fallback behavior, i.e.
          • set client config with kp1, check that it returns kp1
          • mock getServerDefaults() with kp2, check it returns kp2
          • set Credentials with kp3, check it returns kp3
          • typo originalNameodeUri
          • String lookUpKey = DFSClient.DFS_KMS_PREFIX + originalNameodeUri.toString(); should this be a getKey helper method in DFSClient rather than having the test code also construct the key?
          Show
          andrew.wang Andrew Wang added a comment - Here's my review on v2, along with responses to some of Yongjun's questions. Thanks for working on this Rushabh! in createWrappedOutputStream(, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here. The conf is only used to configure the CryptoCodec, which does not need a KMS URI. I think this is okay, all the CC parameters are included in the feInfo . @awang would you please confirm if it's ok to do so since this class is public), and use this constant at multiple places that current uses "" Yea it's fine. I would like to improve this in this patch if possible, since it removes redundancies. 3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489 ). So suggest to replace variable name keyProviderUri with keyProviderPath I think we can ignore this for now like Rushabh said, can handle it in another JIRA if necessary. Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens The DN does the encryption/decryption in WebHDFS, so the client doesn't need to do any KMS communication. It does bring up a question regarding the DN DFSClient though. It looks like WebHdfsHandler creates a new DFSClient each time, which means we won't benefit from getServerDefaults caching. Is the fix to make config preferred over getServerDefaults? <StandbyException> Does this exception actually make it to clients? The HA RPC proxy normally catches the StandbyException and fails over to the other NN. We can write a unit test for this to verify if we're unsure. My additional review comments: typo nameonde ServerDefaults#getKeyProviderUri needs javadoc explaining how to interpret null and empty (IIUC null means not set, empty means means set but not enabled) In docs, "DFSClients" is an internal name. Please rename to say "HDFS clients" or similar. Same for "dfs clients" in core-default.xml There are a lot of whitespace errors, please take a look at what's flagged by checkstyle. Recommend using IDE autoformatting in the future. An actual mini-cluster test that mimics an MR job submitter and task's call pattern would also be good. TestEncryptionZones: Would like to see the test reversed so it covers the fallback behavior, i.e. set client config with kp1, check that it returns kp1 mock getServerDefaults() with kp2, check it returns kp2 set Credentials with kp3, check it returns kp3 typo originalNameodeUri String lookUpKey = DFSClient.DFS_KMS_PREFIX + originalNameodeUri.toString(); should this be a getKey helper method in DFSClient rather than having the test code also construct the key?
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks for the update Rushabh Shah, sorry about that, but please be assured that I did not mean to intrude, my sincere apology if you felt so.

          I should have given some background, I was looking into HDFS-9868 earlier because we need a solution very soon to let distcp to be able to see the keyProvider of the remote cluster, then we found that HADOOP-14104 may be a better solution. As far as I know, the existing "providing key provider path via conf " implementation doesn't support external cluster, we could do an implementation to extend the conf support for external cluster for keyprovider, as an alternative solution for HADOOP-14104.

          Will comment on your other points soon.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks for the update Rushabh Shah , sorry about that, but please be assured that I did not mean to intrude, my sincere apology if you felt so. I should have given some background, I was looking into HDFS-9868 earlier because we need a solution very soon to let distcp to be able to see the keyProvider of the remote cluster, then we found that HADOOP-14104 may be a better solution. As far as I know, the existing "providing key provider path via conf " implementation doesn't support external cluster, we could do an implementation to extend the conf support for external cluster for keyprovider, as an alternative solution for HADOOP-14104 . Will comment on your other points soon.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Andrew Wang Daryn Sharp: Just FYI, please review HADOOP-14104-trunk-v2.patch patch.

          Show
          shahrs87 Rushabh S Shah added a comment - Andrew Wang Daryn Sharp : Just FYI, please review HADOOP-14104 -trunk-v2.patch patch.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          final CryptoCodec codec = getCryptoCodec(conf, feInfo);
          in createWrappedOutputStream(, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here.

          We are not reading the key provider path from conf within getCryptoCodec method. So I don't think my change (form v2 patch) will fail anything.

          public satic final String HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT = "";

          Before the v2 of patch, there was no default value for HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT and I kept it the same way.
          I think this change is out of the scope of this jira. Suggest you to create a new jira for this change. Let's not mix multiple things in one jira.

          3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489). So suggest to replace variable name keyProviderUri with keyProviderPath

          I didn't even know this config used to be called as dfs.encryption.key.provider.uri.
          But this hadoop.security.key.provider.path on the client side is just an URI and not path by any way. We convert this to a path via KMSClientProvider(URI uri, Configuration conf) where we extract the path via KMSClientProvider#extractKMSPath(URI uri). Thats why I named it keyProviderUri.
          But if you feel that strongly about the variable name, I can change it to provider path in my next revision.

          4. Suggest to add two methods of package scope in DFSClient
          void addKmsKeyProviderPath(...)
          String getKmsKeyProviderPath(...)

          I think we use add method if that data structure is going to contain more than one entry. I don't think the provider uri on the client side is going to contain more than one entry.
          I already have getKeyProviderUri in v2 of patch.

          5.The uri used in DistributedFileSystem and DFSClient may be different, see DistributedFileSystem#initialize below

          This is very good observation. I think its safe to flip these 2 statements in DistributedFilesystem class.
          Will do it in next revision.

            this.dfs = new DFSClient(uri, conf, statistics);
              this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority());
          

          6. Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens

          I don't think key provider is used by WebHDFSFileSystem. Maybe I'm missing something.
          Can you please elaborate your comment ?

          7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this.

          I didn't understand this comment also. Please elaborate.

          We need a solution soon, so I hope you don't mind I just uploaded a patch to address most of my comments.

          I don't see any sense of urgency from the community since providing key provider path via conf was the standard way since the feature was introduced since hadoop 2.6 (Aug 2014).
          Having said that I was waiting for comments from Daryn Sharp and Andrew Wang before putting up a new patch.
          I know Daryn Sharp was out of office for last couple of days and Andrew Wang was involved in 2.8.0 release work so I thought to wait for few days before pinging them.
          Yongjun Zhang: Since I am the assignee of the jira and I am updating patches regularly, please allow me to upload the patches henceforth. I would like to get valuable review comments from you.
          I hope you don't mind that.

          Show
          shahrs87 Rushabh S Shah added a comment - final CryptoCodec codec = getCryptoCodec(conf, feInfo); in createWrappedOutputStream(, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here. We are not reading the key provider path from conf within getCryptoCodec method. So I don't think my change (form v2 patch) will fail anything. public satic final String HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT = ""; Before the v2 of patch, there was no default value for HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT and I kept it the same way. I think this change is out of the scope of this jira. Suggest you to create a new jira for this change. Let's not mix multiple things in one jira. 3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489 ). So suggest to replace variable name keyProviderUri with keyProviderPath I didn't even know this config used to be called as dfs.encryption.key.provider.uri . But this hadoop.security.key.provider.path on the client side is just an URI and not path by any way. We convert this to a path via KMSClientProvider(URI uri, Configuration conf) where we extract the path via KMSClientProvider#extractKMSPath(URI uri) . Thats why I named it keyProviderUri . But if you feel that strongly about the variable name, I can change it to provider path in my next revision. 4. Suggest to add two methods of package scope in DFSClient void addKmsKeyProviderPath(...) String getKmsKeyProviderPath(...) I think we use add method if that data structure is going to contain more than one entry. I don't think the provider uri on the client side is going to contain more than one entry. I already have getKeyProviderUri in v2 of patch. 5.The uri used in DistributedFileSystem and DFSClient may be different, see DistributedFileSystem#initialize below This is very good observation. I think its safe to flip these 2 statements in DistributedFilesystem class. Will do it in next revision. this.dfs = new DFSClient(uri, conf, statistics); this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority()); 6. Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens I don't think key provider is used by WebHDFSFileSystem. Maybe I'm missing something. Can you please elaborate your comment ? 7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this. I didn't understand this comment also. Please elaborate. We need a solution soon, so I hope you don't mind I just uploaded a patch to address most of my comments. I don't see any sense of urgency from the community since providing key provider path via conf was the standard way since the feature was introduced since hadoop 2.6 (Aug 2014). Having said that I was waiting for comments from Daryn Sharp and Andrew Wang before putting up a new patch. I know Daryn Sharp was out of office for last couple of days and Andrew Wang was involved in 2.8.0 release work so I thought to wait for few days before pinging them. Yongjun Zhang : Since I am the assignee of the jira and I am updating patches regularly, please allow me to upload the patches henceforth. I would like to get valuable review comments from you. I hope you don't mind that.
          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 4 new or modified test files.
          0 mvndep 1m 50s Maven dependency ordering for branch
          +1 mvninstall 13m 48s trunk passed
          +1 compile 11m 17s trunk passed
          +1 checkstyle 2m 7s trunk passed
          +1 mvnsite 3m 5s trunk passed
          +1 mvneclipse 1m 13s trunk passed
          +1 findbugs 5m 31s trunk passed
          +1 javadoc 2m 24s trunk passed
          0 mvndep 0m 16s Maven dependency ordering for patch
          +1 mvninstall 2m 10s the patch passed
          +1 compile 10m 14s the patch passed
          +1 cc 10m 14s the patch passed
          +1 javac 10m 14s the patch passed
          -0 checkstyle 2m 21s root: The patch generated 43 new + 735 unchanged - 2 fixed = 778 total (was 737)
          +1 mvnsite 3m 8s the patch passed
          +1 mvneclipse 1m 24s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 5m 46s the patch passed
          +1 javadoc 2m 32s the patch passed
          -1 unit 8m 30s hadoop-common in the patch failed.
          +1 unit 1m 13s hadoop-hdfs-client in the patch passed.
          -1 unit 64m 13s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 53s The patch does not generate ASF License warnings.
          169m 18s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag
            hadoop.hdfs.server.namenode.ha.TestDelegationTokensWithHA
            hadoop.hdfs.security.TestDelegationToken
            hadoop.hdfs.tools.offlineImageViewer.TestOfflineImageViewer
            hadoop.hdfs.security.TestDelegationTokenForProxyUser



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856909/HADOOP-14104-trunk-v3.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc
          uname Linux 434822a71527 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 / 33a38a5
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/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 4 new or modified test files. 0 mvndep 1m 50s Maven dependency ordering for branch +1 mvninstall 13m 48s trunk passed +1 compile 11m 17s trunk passed +1 checkstyle 2m 7s trunk passed +1 mvnsite 3m 5s trunk passed +1 mvneclipse 1m 13s trunk passed +1 findbugs 5m 31s trunk passed +1 javadoc 2m 24s trunk passed 0 mvndep 0m 16s Maven dependency ordering for patch +1 mvninstall 2m 10s the patch passed +1 compile 10m 14s the patch passed +1 cc 10m 14s the patch passed +1 javac 10m 14s the patch passed -0 checkstyle 2m 21s root: The patch generated 43 new + 735 unchanged - 2 fixed = 778 total (was 737) +1 mvnsite 3m 8s the patch passed +1 mvneclipse 1m 24s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 5m 46s the patch passed +1 javadoc 2m 32s the patch passed -1 unit 8m 30s hadoop-common in the patch failed. +1 unit 1m 13s hadoop-hdfs-client in the patch passed. -1 unit 64m 13s hadoop-hdfs in the patch failed. +1 asflicense 0m 53s The patch does not generate ASF License warnings. 169m 18s Reason Tests Failed junit tests hadoop.security.TestKDiag   hadoop.hdfs.server.namenode.ha.TestDelegationTokensWithHA   hadoop.hdfs.security.TestDelegationToken   hadoop.hdfs.tools.offlineImageViewer.TestOfflineImageViewer   hadoop.hdfs.security.TestDelegationTokenForProxyUser Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856909/HADOOP-14104-trunk-v3.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc uname Linux 434822a71527 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 / 33a38a5 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11785/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 -

          HI Rushabh Shah,

          We need a solution soon, so I hope you don't mind I just uploaded a patch to address most of my comments.

          I also included some changes to avoid overhead for cluster that doesn't have encryption enabled, by inserting an empty entry into the secret map.

          Would you please take a look?

          Hi Daryn Sharp and Andrew Wang, would you please help taking a look? thanks a lot.

          Show
          yzhangal Yongjun Zhang added a comment - HI Rushabh Shah , We need a solution soon, so I hope you don't mind I just uploaded a patch to address most of my comments. I also included some changes to avoid overhead for cluster that doesn't have encryption enabled, by inserting an empty entry into the secret map. Would you please take a look? Hi Daryn Sharp and Andrew Wang , would you please help taking a look? thanks a lot.
          Hide
          yzhangal Yongjun Zhang added a comment -

          HI Rushabh Shah,

          Thanks again for your updated patch. Below are my comments:

          1. Currently we do

                final CryptoCodec codec = getCryptoCodec(conf, feInfo);
                KeyVersion decrypted = decryptEncryptedDataEncryptionKey(feInfo);
          

          in createWrappedOutputStream(, where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here.

          2. suggest to introduce

          public satic final String HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT = "";
          

          in parallel to HADOOP_SECURITY_KEY_PROVIDER_PATH in CommonConfigurationKeysPublic.java (@awang would you please confirm if it's ok to do so since this class is public), and use this constant at multiple places that current uses "".

          3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489). So suggest to replace variable name keyProviderUri with keyProviderPath

          4. Suggest to add two methods of package scope in DFSClient

            void addKmsKeyProviderPath(...)
            String getKmsKeyProviderPath(...)
          

          and call them from needed places.

          5.The uri used in DistributedFileSystem and DFSClient may be different, see DistributedFileSystem#initialize below

            public void initialize(URI uri, Configuration conf) throws IOException {
              ...
              this.dfs = new DFSClient(uri, conf, statistics);
              this.uri = URI.create(uri.getScheme()+"://"+uri.getAuthority());
              this.workingDir = getHomeDirectory();
          

          So the update and retrieve of fs/keyProvider from the credentials may be mismatching, because uri.toString() is used as the key in the credentials map. We may just use (uri.getScheme()+"://"+uri.getAuthority() as the key and encapsulate this in the add/get methods I proposed in 4.

          6. Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens

          7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this.

          Hi Daryn Sharp and Andrew Wang, would you please help doing a review too? and see if my above comments make sense to you?

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - HI Rushabh Shah , Thanks again for your updated patch. Below are my comments: 1. Currently we do final CryptoCodec codec = getCryptoCodec(conf, feInfo); KeyVersion decrypted = decryptEncryptedDataEncryptionKey(feInfo); in createWrappedOutputStream( , where conf is the configuration of local cluster. There is a possibility that the local configuration is different than remote cluster's. So it's possible to fail here. 2. suggest to introduce public satic final String HADOOP_SECURITY_KEY_PROVIDER_PATH_DEFAULT = ""; in parallel to HADOOP_SECURITY_KEY_PROVIDER_PATH in CommonConfigurationKeysPublic.java (@awang would you please confirm if it's ok to do so since this class is public), and use this constant at multiple places that current uses "". 3. Notice that "dfs.encryption.key.provider.uri" is deprecated and replaced with hadoop.security.key.provider.path (see HDFS-10489 ). So suggest to replace variable name keyProviderUri with keyProviderPath 4. Suggest to add two methods of package scope in DFSClient void addKmsKeyProviderPath(...) String getKmsKeyProviderPath(...) and call them from needed places. 5.The uri used in DistributedFileSystem and DFSClient may be different, see DistributedFileSystem#initialize below public void initialize(URI uri, Configuration conf) throws IOException { ... this .dfs = new DFSClient(uri, conf, statistics); this .uri = URI.create(uri.getScheme()+ ": //" +uri.getAuthority()); this .workingDir = getHomeDirectory(); So the update and retrieve of fs/keyProvider from the credentials may be mismatching, because uri.toString() is used as the key in the credentials map. We may just use (uri.getScheme()+"://"+uri.getAuthority() as the key and encapsulate this in the add/get methods I proposed in 4. 6. Seems we need a similar change in WebHdfsFileSystem when calling addDelegationTokens 7. About your question w.r.t. public boolean isHDFSEncryptionEnabled() throwing StandbyException. There is a solution, that is, we need to incorporate remote's cluster's nameservices configurations in the client (distcp for example) configuration, and let the client handle the NN failover and retry. We need to document this. Hi Daryn Sharp and Andrew Wang , would you please help doing a review too? and see if my above comments make sense to you? Thanks.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 26s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 4 new or modified test files.
          0 mvndep 1m 58s Maven dependency ordering for branch
          +1 mvninstall 15m 1s trunk passed
          +1 compile 12m 8s trunk passed
          +1 checkstyle 2m 20s trunk passed
          +1 mvnsite 3m 17s trunk passed
          +1 mvneclipse 1m 16s trunk passed
          +1 findbugs 5m 39s trunk passed
          +1 javadoc 2m 41s trunk passed
          0 mvndep 0m 18s Maven dependency ordering for patch
          +1 mvninstall 2m 25s the patch passed
          +1 compile 12m 32s the patch passed
          +1 cc 12m 32s the patch passed
          +1 javac 12m 32s the patch passed
          -0 checkstyle 2m 24s root: The patch generated 41 new + 575 unchanged - 2 fixed = 616 total (was 577)
          +1 mvnsite 3m 7s the patch passed
          +1 mvneclipse 1m 21s the patch passed
          -1 whitespace 0m 0s The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 5m 57s the patch passed
          +1 javadoc 2m 29s the patch passed
          -1 unit 8m 23s hadoop-common in the patch failed.
          +1 unit 1m 13s hadoop-hdfs-client in the patch passed.
          -1 unit 66m 36s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 54s The patch does not generate ASF License warnings.
          177m 23s



          Reason Tests
          Failed junit tests hadoop.ipc.TestRPC
            hadoop.hdfs.TestDFSStripedOutputStreamWithFailure100



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856391/HADOOP-14104-trunk-v2.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc
          uname Linux f938878faaed 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 / 5e74196
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/artifact/patchprocess/diff-checkstyle-root.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/artifact/patchprocess/whitespace-eol.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/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 26s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 4 new or modified test files. 0 mvndep 1m 58s Maven dependency ordering for branch +1 mvninstall 15m 1s trunk passed +1 compile 12m 8s trunk passed +1 checkstyle 2m 20s trunk passed +1 mvnsite 3m 17s trunk passed +1 mvneclipse 1m 16s trunk passed +1 findbugs 5m 39s trunk passed +1 javadoc 2m 41s trunk passed 0 mvndep 0m 18s Maven dependency ordering for patch +1 mvninstall 2m 25s the patch passed +1 compile 12m 32s the patch passed +1 cc 12m 32s the patch passed +1 javac 12m 32s the patch passed -0 checkstyle 2m 24s root: The patch generated 41 new + 575 unchanged - 2 fixed = 616 total (was 577) +1 mvnsite 3m 7s the patch passed +1 mvneclipse 1m 21s the patch passed -1 whitespace 0m 0s The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 5m 57s the patch passed +1 javadoc 2m 29s the patch passed -1 unit 8m 23s hadoop-common in the patch failed. +1 unit 1m 13s hadoop-hdfs-client in the patch passed. -1 unit 66m 36s hadoop-hdfs in the patch failed. +1 asflicense 0m 54s The patch does not generate ASF License warnings. 177m 23s Reason Tests Failed junit tests hadoop.ipc.TestRPC   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure100 Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856391/HADOOP-14104-trunk-v2.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc uname Linux f938878faaed 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 / 5e74196 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/artifact/patchprocess/diff-checkstyle-root.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/artifact/patchprocess/whitespace-eol.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11767/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 much for your quick turnaround, Rushabh Shah, I will review it tonight.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks much for your quick turnaround, Rushabh Shah , I will review it tonight.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          This patch addresses most of the previous comments.
          What changed in this patch compared to previous ?
          1. During job submission, added a new secret in the credential's secret map.
          DFS-KMS-<namenodeUri> --> keyproviderUri
          This mapping captures the namenode's keyProviderUri at the time of job submission.
          Every task that is scheduled to run will contact this key provider uri for decrypting EDEK's.
          2. The key provider uri will be searched in the following order.

          • from the credentials secrets map.
          • Query the namenode via server defaults.
          • Local conf.

          Previous concerns:
          1. From Andrew Wang

          I like that getServerDefaults is lock-free, but I'm still worried about the overhead.

          The namenode#getServerDefaults will be queried only once at the time of job submission.

          2. From Yongjun Zhang

          Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour,
          client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again?

          This was a very good question which I didn't think while writing previous patch. Thanks !
          We glue the namenode uri to key provider uri at the time of job submission and persist in ugi's credentials object.
          The task will find it in credentials object and no longer need to contact namenode.
          If there is an update(hardware update or put in maintenance mode ) for key provider, we plan to keep in decommission mode for 7 days.
          So all the tokens which were given out while it was still active will be valid for 7 days and then new key provider will issue tokens for newly submitted jobs.

          I tried to incorporate all the previous comments in the current patch (v2) but let me know if I missed any.

          I need one suggestion.

          DFSClient.java
            public boolean isHDFSEncryptionEnabled() {
              try {
                return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri());
              } catch (IOException ioe) {
                // This means the ClientProtocol#getServerDefautls threw StandbyException
                return false;
              }
            }
          

          getKeyProviderUri is calling NamenodeRpcServer#getServerDefaults and it can throw an Standby Exception in which case, I am returning false.
          I don't know what is the right thing to do.
          DFSClient.isHDFSEncryptionEnabled() is being called by DistributedFileSystem.getTrashRoot(Path path) which doesn't throw any IOException so I need to take some decision if an Exception is encountered.
          Your help is much appreciated.
          Please review.

          Show
          shahrs87 Rushabh S Shah added a comment - This patch addresses most of the previous comments. What changed in this patch compared to previous ? 1. During job submission, added a new secret in the credential's secret map. DFS-KMS-<namenodeUri> --> keyproviderUri This mapping captures the namenode's keyProviderUri at the time of job submission. Every task that is scheduled to run will contact this key provider uri for decrypting EDEK's. 2. The key provider uri will be searched in the following order. from the credentials secrets map. Query the namenode via server defaults. Local conf. Previous concerns: 1. From Andrew Wang I like that getServerDefaults is lock-free, but I'm still worried about the overhead. The namenode#getServerDefaults will be queried only once at the time of job submission. 2. From Yongjun Zhang Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour, client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again? This was a very good question which I didn't think while writing previous patch. Thanks ! We glue the namenode uri to key provider uri at the time of job submission and persist in ugi's credentials object. The task will find it in credentials object and no longer need to contact namenode. If there is an update(hardware update or put in maintenance mode ) for key provider, we plan to keep in decommission mode for 7 days. So all the tokens which were given out while it was still active will be valid for 7 days and then new key provider will issue tokens for newly submitted jobs. I tried to incorporate all the previous comments in the current patch (v2) but let me know if I missed any. I need one suggestion. DFSClient.java public boolean isHDFSEncryptionEnabled() { try { return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri()); } catch (IOException ioe) { // This means the ClientProtocol#getServerDefautls threw StandbyException return false ; } } getKeyProviderUri is calling NamenodeRpcServer#getServerDefaults and it can throw an Standby Exception in which case, I am returning false. I don't know what is the right thing to do. DFSClient.isHDFSEncryptionEnabled() is being called by DistributedFileSystem.getTrashRoot(Path path) which doesn't throw any IOException so I need to take some decision if an Exception is encountered. Your help is much appreciated. Please review.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Daryn Sharp,

          Took a further look, my understanding is:

          Credentials is an object stored in UGI, and it is passed around to various components such as mappers and reducers to get a job done. The Credentials object contains two maps:

            private  Map<Text, byte[]> secretKeysMap = new HashMap<Text, byte[]>();
            private  Map<Text, Token<? extends TokenIdentifier>> tokenMap =
                new HashMap<Text, Token<? extends TokenIdentifier>>();
          

          When initializing the Credentials object for the client, the token map is populated by asking NN for the tokens with FSNamesystem#getDelegationToken(Text renewer)

          The secretKeysMap is populated by UserProvider.

          To add fs/keyProvider entries to secretKeysMap, we call getServerDefaults once to get back the keyProvider info, and update secretKeysMap with entries <fs, keyProvider>.

          Is that understanding correct?

          Thanks.

          --Yongjun

          Show
          yzhangal Yongjun Zhang added a comment - Hi Daryn Sharp , Took a further look, my understanding is: Credentials is an object stored in UGI, and it is passed around to various components such as mappers and reducers to get a job done. The Credentials object contains two maps: private Map<Text, byte []> secretKeysMap = new HashMap<Text, byte []>(); private Map<Text, Token<? extends TokenIdentifier>> tokenMap = new HashMap<Text, Token<? extends TokenIdentifier>>(); When initializing the Credentials object for the client, the token map is populated by asking NN for the tokens with FSNamesystem#getDelegationToken(Text renewer) The secretKeysMap is populated by UserProvider. To add fs/keyProvider entries to secretKeysMap, we call getServerDefaults once to get back the keyProvider info, and update secretKeysMap with entries <fs, keyProvider>. Is that understanding correct? Thanks. --Yongjun
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thank you guys very much Daryn Sharp and Rushabh Shah!

          Much appreciated that you will provide the revised patch Rushabh!

          Show
          yzhangal Yongjun Zhang added a comment - Thank you guys very much Daryn Sharp and Rushabh Shah ! Much appreciated that you will provide the revised patch Rushabh!
          Hide
          daryn Daryn Sharp added a comment -

          Credentials are simply a container in the ugi for tokens and/or secrets. There is no notion of client, server, etc. Credentials are the mechanism by which tokens are propagated throughout a job.

          I may be understanding the EZ w/o security question (which seems an entirely contrived use case), but regardless: if tokens are available, so are the secrets since they are both packaged in the credentials object. If this use case works today then it will continue to work with the mappings based approach.

          Show
          daryn Daryn Sharp added a comment - Credentials are simply a container in the ugi for tokens and/or secrets. There is no notion of client, server, etc. Credentials are the mechanism by which tokens are propagated throughout a job. I may be understanding the EZ w/o security question (which seems an entirely contrived use case), but regardless: if tokens are available, so are the secrets since they are both packaged in the credentials object. If this use case works today then it will continue to work with the mappings based approach.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Thanks Daryn Sharp Andrew Wang Yongjun Zhang Wei-Chiu Chuang for the discussion and reviews.
          I will put up a patch in couple of days with test cases implementing Daryn's idea from this comment.
          We can then discuss further ahead.

          Show
          shahrs87 Rushabh S Shah added a comment - Thanks Daryn Sharp Andrew Wang Yongjun Zhang Wei-Chiu Chuang for the discussion and reviews. I will put up a patch in couple of days with test cases implementing Daryn's idea from this comment . We can then discuss further ahead.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Daryn Sharp!

          Some follow-up questions:

          My understanding is that the credential map exists at the server side (NN of a cluster), thus NN should populate the fs/kms provider mapping, is that understanding correct? If so, where and when does NN populate the mapping?

          How the client gets the credential map?

          Does this work only for kerberized cluster, but not non-kerberized cluster? If that's the case, how we solve the problem of non-kerberized cluster with encryption zone?

          Thanks much.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Daryn Sharp ! Some follow-up questions: My understanding is that the credential map exists at the server side (NN of a cluster), thus NN should populate the fs/kms provider mapping, is that understanding correct? If so, where and when does NN populate the mapping? How the client gets the credential map? Does this work only for kerberized cluster, but not non-kerberized cluster? If that's the case, how we solve the problem of non-kerberized cluster with encryption zone? Thanks much.
          Hide
          daryn Daryn Sharp added a comment -

          Goal is naturally no incompatibility but expanded functionality. To that end, the general principal when acquiring tokens:

          1. If getServerDefaults doesn't return a keyprovider field: it's a previous version to the client, don't know if kms is applicable, so fallback to config just as today.
          2. If getServerDefaults does return a keyprovider: it's authoritative, add a kvp to the credentials mapping the fs uri to the kms uri.

          When the dfsclient needs a key provider, check credentials:

          1. If fs/kms provider mapping is present, use the value as authoritative since a token in the credentials was obtained from that provider.
          2. If no mapping, check getServerDefaults, fallback to config.
          Show
          daryn Daryn Sharp added a comment - Goal is naturally no incompatibility but expanded functionality. To that end, the general principal when acquiring tokens: If getServerDefaults doesn't return a keyprovider field: it's a previous version to the client, don't know if kms is applicable, so fallback to config just as today. If getServerDefaults does return a keyprovider: it's authoritative, add a kvp to the credentials mapping the fs uri to the kms uri. When the dfsclient needs a key provider, check credentials: If fs/kms provider mapping is present, use the value as authoritative since a token in the credentials was obtained from that provider. If no mapping, check getServerDefaults, fallback to config.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Andrew Wang, good comments.

          Hi Daryn Sharp,

          I like the sound of your proposal too

          I think the cleanest/most-compatible way is leveraging the Credentials instead of the config. We could inject a mapping of filesystem uri to kms uri via the secrets map. So now when the client needs to talk to the kms it can check the map, else fallback to getServerDefaults.

          Did you mean to use the following UserProvider method

            @Override
            public synchronized CredentialEntry createCredentialEntry(String name, char[] credential) 
                throws IOException {
              Text nameT = new Text(name);
              if (credentials.getSecretKey(nameT) != null) {
                throw new IOException("Credential " + name + 
                    " already exists in " + this);
              }
              credentials.addSecretKey(new Text(name), 
                  new String(credential).getBytes("UTF-8"));
              return new CredentialEntry(name, credential);
            }
          

          to add <fs-uri, keyProvider> mapping to the credential map? This mapping info for a remote cluster need to come from either the remote cluster conf, or the NN of the remote cluster, what's your thinking here?

          Would you please elaborate this approach? Is there any in-compatibility here?

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Andrew Wang , good comments. Hi Daryn Sharp , I like the sound of your proposal too I think the cleanest/most-compatible way is leveraging the Credentials instead of the config. We could inject a mapping of filesystem uri to kms uri via the secrets map. So now when the client needs to talk to the kms it can check the map, else fallback to getServerDefaults. Did you mean to use the following UserProvider method @Override public synchronized CredentialEntry createCredentialEntry( String name, char [] credential) throws IOException { Text nameT = new Text(name); if (credentials.getSecretKey(nameT) != null ) { throw new IOException( "Credential " + name + " already exists in " + this ); } credentials.addSecretKey( new Text(name), new String (credential).getBytes( "UTF-8" )); return new CredentialEntry(name, credential); } to add <fs-uri, keyProvider> mapping to the credential map? This mapping info for a remote cluster need to come from either the remote cluster conf, or the NN of the remote cluster, what's your thinking here? Would you please elaborate this approach? Is there any in-compatibility here? Thanks.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Daryn, thanks for commenting! Looks like you have a more ambitious implementation in mind, since your usecases include dynamic configuration changes without client restarts (something not possible with the current config-based approach).

          Generally speaking, I think it's pretty rare to change the KMS URI. I think the two situations are:

          • Enabling HDFS encryption for the first time. This currently requires a client restart.
          • Enabling KMS HA. As long as the old KMS is part of the HA group, then clients with the old value will still work.

          Since the KMS is just a proxy, you can swap out the backing KeyProvider implementation without changing the URI.

          I'm not familiar with the Credentials APIs, but I like the sound of your proposal. It lets most clients avoid calling getServerDefaults, which was my main concern about the current patch.

          Since we're very interested in a NN-specified KMS URI but less interested in dynamic refresh, so if it's reasonable to do refresh as a follow-on JIRA, that'd be optimal from our perspective.

          Show
          andrew.wang Andrew Wang added a comment - Hi Daryn, thanks for commenting! Looks like you have a more ambitious implementation in mind, since your usecases include dynamic configuration changes without client restarts (something not possible with the current config-based approach). Generally speaking, I think it's pretty rare to change the KMS URI. I think the two situations are: Enabling HDFS encryption for the first time. This currently requires a client restart. Enabling KMS HA. As long as the old KMS is part of the HA group, then clients with the old value will still work. Since the KMS is just a proxy, you can swap out the backing KeyProvider implementation without changing the URI. I'm not familiar with the Credentials APIs, but I like the sound of your proposal. It lets most clients avoid calling getServerDefaults, which was my main concern about the current patch. Since we're very interested in a NN-specified KMS URI but less interested in dynamic refresh, so if it's reasonable to do refresh as a follow-on JIRA, that'd be optimal from our perspective.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hm, my jira window was not refreshed so I missed updates from you guys when I did my last one. Thanks for further discussions.

          Show
          yzhangal Yongjun Zhang added a comment - Hm, my jira window was not refreshed so I missed updates from you guys when I did my last one. Thanks for further discussions.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Discussed with Wei-Chiu Chuang and John Zhuge, we looked at the code together and saw KMS provider already support multiple key provider servers in its configuration, for example:

           <property>
              <name>hadoop.security.key.provider.path</name>
              <value>kms://https@kms01.example.com;kms02.example.com:16000/kms</value>
            </property>
          

          and

               * If multiple hosts are provider, the Factory will create a
               * {@link LoadBalancingKMSClientProvider} that round-robins requests
               * across the provided list of hosts.
          

          This is a form of KeyProvider HA handling (also handles load balancing).

          Andrew Wang,

          I like that getServerDefaults is lock-free, but I'm still worried about the overhead. MR tasks are short lived and thus don't benefit from the caching. This also affects all clients, on both encrypted and unencrypted clusters. I think getServerDefault is also currently only called when SASL is enabled. Have you done any performance testing of this RPC?

          getServerDefaults mechanism has been there and the patch here just included an additional field. Calling it once an hour should not be a problem to me, at least not a regression. It's just that if things changed within the hour, the errors may not be handled correctly (for example, all old key provider hosts are replaced with new ones), but it's less of a concern assuming that's rare. I don't see that sasl is checked when getServerDefaults is called.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Discussed with Wei-Chiu Chuang and John Zhuge , we looked at the code together and saw KMS provider already support multiple key provider servers in its configuration, for example: <property> <name>hadoop.security.key.provider.path</name> <value>kms: //https@kms01.example.com;kms02.example.com:16000/kms</value> </property> and * If multiple hosts are provider, the Factory will create a * {@link LoadBalancingKMSClientProvider} that round-robins requests * across the provided list of hosts. This is a form of KeyProvider HA handling (also handles load balancing). Andrew Wang , I like that getServerDefaults is lock-free, but I'm still worried about the overhead. MR tasks are short lived and thus don't benefit from the caching. This also affects all clients, on both encrypted and unencrypted clusters. I think getServerDefault is also currently only called when SASL is enabled. Have you done any performance testing of this RPC? getServerDefaults mechanism has been there and the patch here just included an additional field. Calling it once an hour should not be a problem to me, at least not a regression. It's just that if things changed within the hour, the errors may not be handled correctly (for example, all old key provider hosts are replaced with new ones), but it's less of a concern assuming that's rare. I don't see that sasl is checked when getServerDefaults is called. Thanks.
          Hide
          daryn Daryn Sharp added a comment -

          ... how client handle namenode HA ... we specify keyproviderservice in config file ...

          The need for configs is the problem, so more configs is not the answer (aside: the NN HA token handling is an example of exactly what not to do).

          One thing I might recommend is that we don't query getServerDefaults after we get the KP initially.

          Enabling EZ on a cluster must not require a restart of all daemon and proxy services that communicate with said cluster. It can't be cached forever.

          ––

          I reviewed Rushabh's approach with him this morning. The main goal should be a config-free token acquisition and selection. How do we get there?

          The first challenge is how does a client intelligently request a kms token, when needed, and from the right kms? The NN is the authoritative and dynamic source for the correct kms, ala this patch.. Token acquisition should use the kp uri provided by the NN, and I'm not too worried about caching when a typical cluster has a few dozen app submits/sec (equaling token requests) vs 10s of thousand of NN ops/sec. This is only a small part of the problem.

          The second challenge is how does a client select the correct kms for a given NN? The client could again ask the NN but you stumble into the morass of caching. However as soon as the NN reports a different key provider than when a job launched, clients won't be able to find a token for the new kms - even when the old one is still legit. Now jobs fail that should/could have completed. It's very messy. The simpler answer is a client should always use the key provider for a given NN as it existed when the token was acquired (ie. job submit).

          So how do we implement a config-free mapping of NN to key provider? When the hdfs and kms tokens are acquired we need a way to later associate them as a pair. I think the cleanest/most-compatible way is leveraging the Credentials instead of the config. We could inject a mapping of filesystem uri to kms uri via the secrets map. So now when the client needs to talk to the kms it can check the map, else fallback to getServerDefaults.

          Show
          daryn Daryn Sharp added a comment - ... how client handle namenode HA ... we specify keyproviderservice in config file ... The need for configs is the problem, so more configs is not the answer (aside: the NN HA token handling is an example of exactly what not to do). One thing I might recommend is that we don't query getServerDefaults after we get the KP initially. Enabling EZ on a cluster must not require a restart of all daemon and proxy services that communicate with said cluster. It can't be cached forever. –– I reviewed Rushabh's approach with him this morning. The main goal should be a config-free token acquisition and selection. How do we get there? The first challenge is how does a client intelligently request a kms token, when needed, and from the right kms? The NN is the authoritative and dynamic source for the correct kms, ala this patch.. Token acquisition should use the kp uri provided by the NN, and I'm not too worried about caching when a typical cluster has a few dozen app submits/sec (equaling token requests) vs 10s of thousand of NN ops/sec. This is only a small part of the problem. The second challenge is how does a client select the correct kms for a given NN? The client could again ask the NN but you stumble into the morass of caching. However as soon as the NN reports a different key provider than when a job launched, clients won't be able to find a token for the new kms - even when the old one is still legit. Now jobs fail that should/could have completed. It's very messy. The simpler answer is a client should always use the key provider for a given NN as it existed when the token was acquired (ie. job submit). So how do we implement a config-free mapping of NN to key provider? When the hdfs and kms tokens are acquired we need a way to later associate them as a pair. I think the cleanest/most-compatible way is leveraging the Credentials instead of the config. We could inject a mapping of filesystem uri to kms uri via the secrets map. So now when the client needs to talk to the kms it can check the map, else fallback to getServerDefaults.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Yongjun, what you describe is how the existing KMS HA already works.

          One thing I might recommend is that we don't query getServerDefaults after we get the KP initially. This way we don't need to worry about the value changing later (or an exception being thrown).

          Show
          andrew.wang Andrew Wang added a comment - Hi Yongjun, what you describe is how the existing KMS HA already works. One thing I might recommend is that we don't query getServerDefaults after we get the KP initially. This way we don't need to worry about the value changing later (or an exception being thrown).
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Andrew Wang.

          I gave some more thoughts, I think a better solution instead of the period polling is, just like how client handle namenode HA, we can do the same for KeyProvider. Say, if we specify keyproviderservice in config file to associate with a list of KeyProviders, if one keyProvider is down, the client can try to access the next one in the list (client failover). This is essentially KeyProvider HA. But this would be a larger scope solution. Does this make sense to you?

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Andrew Wang . I gave some more thoughts, I think a better solution instead of the period polling is, just like how client handle namenode HA, we can do the same for KeyProvider. Say, if we specify keyproviderservice in config file to associate with a list of KeyProviders, if one keyProvider is down, the client can try to access the next one in the list (client failover). This is essentially KeyProvider HA. But this would be a larger scope solution. Does this make sense to you?
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for reviewing Wei-chiu and Yongjun, let me reply to a few comments of Rushabh:

          This is not a precise statement though, considering that multiple kms instances can be added for load balancing purposes. Would you mind to update the release note once this patch gets committed?

          By this, I think you mean we should update the JIRA summary and related to say "for the KeyProvider URI"?

          Can we also add a test to ensure clients can access files in an encrypted remote cluster using the token obtained from the remote NameNode?

          What constitutes "remote" here? Clients fetch delegation tokens based on the specified filesystem(s), so I don't think it matters where the client or cluster are located.

          5. Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour, client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again?

          What kind of error handling do you recommend here? I do think at least some a log message would be nice.

          Some new questions of my own:

          • I like that getServerDefaults is lock-free, but I'm still worried about the overhead. MR tasks are short lived and thus don't benefit from the caching. This also affects all clients, on both encrypted and unencrypted clusters. I think getServerDefault is also currently only called when SASL is enabled. Have you done any performance testing of this RPC?
          • Another option is to put this behind a config key which defaults off, and file a new JIRA to track defaulting it on in a later release.
          Show
          andrew.wang Andrew Wang added a comment - Thanks for reviewing Wei-chiu and Yongjun, let me reply to a few comments of Rushabh: This is not a precise statement though, considering that multiple kms instances can be added for load balancing purposes. Would you mind to update the release note once this patch gets committed? By this, I think you mean we should update the JIRA summary and related to say "for the KeyProvider URI"? Can we also add a test to ensure clients can access files in an encrypted remote cluster using the token obtained from the remote NameNode? What constitutes "remote" here? Clients fetch delegation tokens based on the specified filesystem(s), so I don't think it matters where the client or cluster are located. 5. Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour, client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again? What kind of error handling do you recommend here? I do think at least some a log message would be nice. Some new questions of my own: I like that getServerDefaults is lock-free, but I'm still worried about the overhead. MR tasks are short lived and thus don't benefit from the caching. This also affects all clients, on both encrypted and unencrypted clusters. I think getServerDefault is also currently only called when SASL is enabled. Have you done any performance testing of this RPC? Another option is to put this behind a config key which defaults off, and file a new JIRA to track defaulting it on in a later release.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Looked closer, I think #5 of my prior comment is indeed a potential problem because I don't see the kind of handling I was looking for in the current implementation.

          Show
          yzhangal Yongjun Zhang added a comment - Looked closer, I think #5 of my prior comment is indeed a potential problem because I don't see the kind of handling I was looking for in the current implementation.
          Hide
          yzhangal Yongjun Zhang added a comment -

          HI Rushabh S Shah,

          Thanks much for working on this issue!

          I had a few nits and question about the patch here:

          1. Since this is public API, we should introduce a new constructor with the addiitional parameter keyProviderUri to be backward compatible, instead of just modifying the existing one.

          @InterfaceAudience.Public
          @InterfaceStability.Evolving
          public class FsServerDefaults implements Writable {
          ......
            public FsServerDefaults(long blockSize, int bytesPerChecksum,
                int writePacketSize, short replication, int fileBufferSize,
                boolean encryptDataTransfer, long trashInterval,
                DataChecksum.Type checksumType,
                String keyProviderUri) {
          

          2. Suggest to add a KEY_PROVIDER_URI_DEFAULT to replce the "" here (in both FtpConfigKeys.java and LocalConfigKeys.java):

            protected static FsServerDefaults getServerDefaults() throws IOException {
              return new FsServerDefaults(
                  BLOCK_SIZE_DEFAULT,
                  ......
                  CHECKSUM_TYPE_DEFAULT,
                  "");
          

          3. the following method swallows IOException and return false. Suggest to remove the @throws IOException and add a comment in the catch block about why it can be iegnored and false should be returned here. And a
          dd a space after the word catrch.

            /**
             * Probe for encryption enabled on this filesystem.
             * See {@link DFSUtilClient#isHDFSEncryptionEnabled(Configuration)}
             * @return true if encryption is enabled
             * @throws IOException
             */
            public boolean isHDFSEncryptionEnabled() {
              try {
                return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri());
              } catch(IOException ioe) {
                return false;
              }
            }
          

          4. Line 84 of KeyProviderCache.java (not introduced by your changei)

          LOG.error("Could not create KeyProvider for DFSClient !!", e.getCause());
          

          suggest to replace e.getCause() with e, so we can see the full stack.

          5. Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour,
          client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again?

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - HI Rushabh S Shah , Thanks much for working on this issue! I had a few nits and question about the patch here: 1. Since this is public API, we should introduce a new constructor with the addiitional parameter keyProviderUri to be backward compatible, instead of just modifying the existing one. @InterfaceAudience.Public @InterfaceStability.Evolving public class FsServerDefaults implements Writable { ...... public FsServerDefaults( long blockSize, int bytesPerChecksum, int writePacketSize, short replication, int fileBufferSize, boolean encryptDataTransfer, long trashInterval, DataChecksum.Type checksumType, String keyProviderUri) { 2. Suggest to add a KEY_PROVIDER_URI_DEFAULT to replce the "" here (in both FtpConfigKeys.java and LocalConfigKeys.java): protected static FsServerDefaults getServerDefaults() throws IOException { return new FsServerDefaults( BLOCK_SIZE_DEFAULT, ...... CHECKSUM_TYPE_DEFAULT, ""); 3. the following method swallows IOException and return false. Suggest to remove the @throws IOException and add a comment in the catch block about why it can be iegnored and false should be returned here. And a dd a space after the word catrch . /** * Probe for encryption enabled on this filesystem. * See {@link DFSUtilClient#isHDFSEncryptionEnabled(Configuration)} * @ return true if encryption is enabled * @ throws IOException */ public boolean isHDFSEncryptionEnabled() { try { return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri()); } catch (IOException ioe) { return false ; } } 4. Line 84 of KeyProviderCache.java (not introduced by your changei) LOG.error( "Could not create KeyProvider for DFSClient !!" , e.getCause()); suggest to replace e.getCause() with e, so we can see the full stack. 5. Currently getServerDefaults() contact NN every hour, to find if there is any update of keyprovider. If keyprovider changed within the hour, client code may get into exception, wonder if we have mechanism to handle the exception and update the keyprovider and try again? Thanks.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Hi Rushabh S Shah thanks much for your patch. Looks good overall but I still have a few questions.

          According to current implementation of kms provider in client conf, there can only be one kms.

          This is not a precise statement though, considering that multiple kms instances can be added for load balancing purposes. Would you mind to update the release note once this patch gets committed?

          Preferably also state (in the doc, maybe?) that this only works if both client and namenodes are on a supported version, otherwise the client's local kms config is used.

          Can we also add a test to ensure clients can access files in an encrypted remote cluster using the token obtained from the remote NameNode?

          Show
          jojochuang Wei-Chiu Chuang added a comment - Hi Rushabh S Shah thanks much for your patch. Looks good overall but I still have a few questions. According to current implementation of kms provider in client conf, there can only be one kms. This is not a precise statement though, considering that multiple kms instances can be added for load balancing purposes. Would you mind to update the release note once this patch gets committed? Preferably also state (in the doc, maybe?) that this only works if both client and namenodes are on a supported version, otherwise the client's local kms config is used. Can we also add a test to ensure clients can access files in an encrypted remote cluster using the token obtained from the remote NameNode?
          Hide
          shahrs87 Rushabh S Shah added a comment -

          FtpConfigKeys and LocalConfigKeys, would still be nice to introduce a new variable for documentation purposes

          Will update in the next revision.

          I dislike non-trivial ternary statements, mind rewriting DFSClient#getKeyProviderUri a bit?

          Will update in next revision.

          We can also dedupe the call to getKeyProviderUri for clarity.

          Can you please elaborate more ?

          Still could use a doc update in TransparentEncryption.md about how this is fetched from server-side defaults.

          Will update in next revision.

          Could you comment on potential additional overheads from invoking the NNto query this config value? I don't see getServerDefaults used much right now, so it looks like this will add another NN RPC to many client operations, for both unencrypted and encrypted clusters. This is concerning.

          The results of namenode#getServerDefaults on DFSClient are cached for 1 hour.
          On the namenode side also, this rpc call doesn't need any lock. So I don't think this should add any significant load on Namenode.
          For DFSInputStream, we already do call severDefaults to get DFSClient#newDataEncryptionKey

          Show
          shahrs87 Rushabh S Shah added a comment - FtpConfigKeys and LocalConfigKeys, would still be nice to introduce a new variable for documentation purposes Will update in the next revision. I dislike non-trivial ternary statements, mind rewriting DFSClient#getKeyProviderUri a bit? Will update in next revision. We can also dedupe the call to getKeyProviderUri for clarity. Can you please elaborate more ? Still could use a doc update in TransparentEncryption.md about how this is fetched from server-side defaults. Will update in next revision. Could you comment on potential additional overheads from invoking the NNto query this config value? I don't see getServerDefaults used much right now, so it looks like this will add another NN RPC to many client operations, for both unencrypted and encrypted clusters. This is concerning. The results of namenode#getServerDefaults on DFSClient are cached for 1 hour. On the namenode side also, this rpc call doesn't need any lock. So I don't think this should add any significant load on Namenode. For DFSInputStream, we already do call severDefaults to get DFSClient#newDataEncryptionKey
          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 4 new or modified test files.
          0 mvndep 1m 49s Maven dependency ordering for branch
          +1 mvninstall 14m 53s trunk passed
          +1 compile 15m 58s trunk passed
          +1 checkstyle 2m 13s trunk passed
          +1 mvnsite 3m 0s trunk passed
          +1 mvneclipse 1m 2s trunk passed
          +1 findbugs 5m 37s trunk passed
          +1 javadoc 2m 44s trunk passed
          0 mvndep 0m 23s Maven dependency ordering for patch
          +1 mvninstall 2m 35s the patch passed
          +1 compile 13m 42s the patch passed
          +1 cc 13m 42s the patch passed
          +1 javac 13m 42s the patch passed
          +1 checkstyle 2m 21s the patch passed
          +1 mvnsite 3m 39s the patch passed
          +1 mvneclipse 1m 30s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 6m 30s the patch passed
          -1 javadoc 0m 40s hadoop-hdfs-project_hadoop-hdfs-client generated 1 new + 1 unchanged - 0 fixed = 2 total (was 1)
          -1 unit 8m 39s hadoop-common in the patch failed.
          +1 unit 1m 10s hadoop-hdfs-client in the patch passed.
          -1 unit 92m 51s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 45s The patch does not generate ASF License warnings.
          210m 16s



          Reason Tests
          Failed junit tests hadoop.fs.sftp.TestSFTPFileSystem
            hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855020/HADOOP-14104-trunk-v1.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc
          uname Linux cdb23fbd7c99 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 5f5b031
          Default Java 1.8.0_121
          findbugs v3.0.0
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/artifact/patchprocess/diff-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-client.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/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 4 new or modified test files. 0 mvndep 1m 49s Maven dependency ordering for branch +1 mvninstall 14m 53s trunk passed +1 compile 15m 58s trunk passed +1 checkstyle 2m 13s trunk passed +1 mvnsite 3m 0s trunk passed +1 mvneclipse 1m 2s trunk passed +1 findbugs 5m 37s trunk passed +1 javadoc 2m 44s trunk passed 0 mvndep 0m 23s Maven dependency ordering for patch +1 mvninstall 2m 35s the patch passed +1 compile 13m 42s the patch passed +1 cc 13m 42s the patch passed +1 javac 13m 42s the patch passed +1 checkstyle 2m 21s the patch passed +1 mvnsite 3m 39s the patch passed +1 mvneclipse 1m 30s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 6m 30s the patch passed -1 javadoc 0m 40s hadoop-hdfs-project_hadoop-hdfs-client generated 1 new + 1 unchanged - 0 fixed = 2 total (was 1) -1 unit 8m 39s hadoop-common in the patch failed. +1 unit 1m 10s hadoop-hdfs-client in the patch passed. -1 unit 92m 51s hadoop-hdfs in the patch failed. +1 asflicense 0m 45s The patch does not generate ASF License warnings. 210m 16s Reason Tests Failed junit tests hadoop.fs.sftp.TestSFTPFileSystem   hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855020/HADOOP-14104-trunk-v1.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml cc uname Linux cdb23fbd7c99 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5f5b031 Default Java 1.8.0_121 findbugs v3.0.0 javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/artifact/patchprocess/diff-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-client.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11726/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for working on this Rushabh, patch looks pretty close, just some nits:

          • FtpConfigKeys and LocalConfigKeys, would still be nice to introduce a new variable for documentation purposes
          • I dislike non-trivial ternary statements, mind rewriting DFSClient#getKeyProviderUri a bit? We can also dedupe the call to getKeyProviderUri for clarity.
          • Still could use a doc update in TransparentEncryption.md about how this is fetched from server-side defaults.

          Could you comment on potential additional overheads from invoking the NNto query this config value? I don't see getServerDefaults used much right now, so it looks like this will add another NN RPC to many client operations, for both unencrypted and encrypted clusters. This is concerning.

          Show
          andrew.wang Andrew Wang added a comment - Thanks for working on this Rushabh, patch looks pretty close, just some nits: FtpConfigKeys and LocalConfigKeys, would still be nice to introduce a new variable for documentation purposes I dislike non-trivial ternary statements, mind rewriting DFSClient#getKeyProviderUri a bit? We can also dedupe the call to getKeyProviderUri for clarity. Still could use a doc update in TransparentEncryption.md about how this is fetched from server-side defaults. Could you comment on potential additional overheads from invoking the NNto query this config value? I don't see getServerDefaults used much right now, so it looks like this will add another NN RPC to many client operations, for both unencrypted and encrypted clusters. This is concerning.
          Hide
          shahrs87 Rushabh S Shah added a comment - - edited

          Attached a new patch which removed PB boolean.
          Updated documentation and core-default.xml

          Test Failures from the previous patch:
          hadoop.security.TestKDiag – Ran fine on my node.
          hadoop.hdfs.TestDFSUtil – Was actually a bug in my patch. Fixed in the latest revision.
          Please review.

          Show
          shahrs87 Rushabh S Shah added a comment - - edited Attached a new patch which removed PB boolean. Updated documentation and core-default.xml Test Failures from the previous patch: hadoop.security.TestKDiag – Ran fine on my node. hadoop.hdfs.TestDFSUtil – Was actually a bug in my patch. Fixed in the latest revision. Please review.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 13s Maven dependency ordering for branch
          +1 mvninstall 12m 25s trunk passed
          +1 compile 13m 50s trunk passed
          +1 checkstyle 2m 23s trunk passed
          +1 mvnsite 2m 53s trunk passed
          +1 mvneclipse 0m 55s trunk passed
          +1 findbugs 5m 10s trunk passed
          +1 javadoc 2m 27s trunk passed
          0 mvndep 0m 19s Maven dependency ordering for patch
          +1 mvninstall 2m 28s the patch passed
          +1 compile 14m 32s the patch passed
          +1 cc 14m 32s the patch passed
          +1 javac 14m 32s the patch passed
          +1 checkstyle 2m 14s the patch passed
          +1 mvnsite 3m 21s the patch passed
          +1 mvneclipse 1m 5s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 6m 22s the patch passed
          -1 javadoc 0m 33s hadoop-hdfs-project_hadoop-hdfs-client generated 1 new + 1 unchanged - 0 fixed = 2 total (was 1)
          -1 unit 9m 27s hadoop-common in the patch failed.
          +1 unit 1m 20s hadoop-hdfs-client in the patch passed.
          -1 unit 66m 14s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 39s The patch does not generate ASF License warnings.
          175m 56s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag
            hadoop.hdfs.TestDFSUtil



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-14104
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12854608/HADOOP-14104-trunk.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc
          uname Linux 6936cba05be6 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 / d2b3ba9
          Default Java 1.8.0_121
          findbugs v3.0.0
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/artifact/patchprocess/diff-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-client.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/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 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 13s Maven dependency ordering for branch +1 mvninstall 12m 25s trunk passed +1 compile 13m 50s trunk passed +1 checkstyle 2m 23s trunk passed +1 mvnsite 2m 53s trunk passed +1 mvneclipse 0m 55s trunk passed +1 findbugs 5m 10s trunk passed +1 javadoc 2m 27s trunk passed 0 mvndep 0m 19s Maven dependency ordering for patch +1 mvninstall 2m 28s the patch passed +1 compile 14m 32s the patch passed +1 cc 14m 32s the patch passed +1 javac 14m 32s the patch passed +1 checkstyle 2m 14s the patch passed +1 mvnsite 3m 21s the patch passed +1 mvneclipse 1m 5s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 6m 22s the patch passed -1 javadoc 0m 33s hadoop-hdfs-project_hadoop-hdfs-client generated 1 new + 1 unchanged - 0 fixed = 2 total (was 1) -1 unit 9m 27s hadoop-common in the patch failed. +1 unit 1m 20s hadoop-hdfs-client in the patch passed. -1 unit 66m 14s hadoop-hdfs in the patch failed. +1 asflicense 0m 39s The patch does not generate ASF License warnings. 175m 56s Reason Tests Failed junit tests hadoop.security.TestKDiag   hadoop.hdfs.TestDFSUtil Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-14104 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12854608/HADOOP-14104-trunk.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc uname Linux 6936cba05be6 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 / d2b3ba9 Default Java 1.8.0_121 findbugs v3.0.0 javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/artifact/patchprocess/diff-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-client.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11714/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Canceling the patch to address review comments.
          Will post a new patch shortly.

          Show
          shahrs87 Rushabh S Shah added a comment - Canceling the patch to address review comments. Will post a new patch shortly.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          PB can differentiate between not specified, empty string, and set.

          Please ignore my previous comment. I understood what you actually meant.
          I will update the patch by monday.

          Show
          shahrs87 Rushabh S Shah added a comment - PB can differentiate between not specified, empty string, and set. Please ignore my previous comment. I understood what you actually meant. I will update the patch by monday.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Andrew Wang: Thanks for quick review.

          Could you update the documentation and core-default.xml about this new feature?

          Will update in next revision.

          Do we actually need the PB boolean? PB can differentiate between not specified, empty string, and set

          The default value for string format in protobuf is "" (empty string).
          If the client receives an empty string, it either means its talking to an old namenode or the namenode has provider path config not set.
          Maybe if the namenode doesn't have security path set, then I can return null value and on the client side, I have to differentiate between null and empty string. But need to test if protobuf convert null string to empty string in response or not ?

          Show
          shahrs87 Rushabh S Shah added a comment - Andrew Wang : Thanks for quick review. Could you update the documentation and core-default.xml about this new feature? Will update in next revision. Do we actually need the PB boolean? PB can differentiate between not specified, empty string, and set The default value for string format in protobuf is "" (empty string). If the client receives an empty string, it either means its talking to an old namenode or the namenode has provider path config not set. Maybe if the namenode doesn't have security path set, then I can return null value and on the client side, I have to differentiate between null and empty string. But need to test if protobuf convert null string to empty string in response or not ?
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for posting the patch Rushabh! I like this idea, it'll simplify client configuration.

          I took a quick look, few questions:

          • Could you update the documentation and core-default.xml about this new feature?
          • Do we actually need the PB boolean? PB can differentiate between not specified, empty string, and set. IIUC it's not specified, then it's an old NN, and we can fallback.
          Show
          andrew.wang Andrew Wang added a comment - Thanks for posting the patch Rushabh! I like this idea, it'll simplify client configuration. I took a quick look, few questions: Could you update the documentation and core-default.xml about this new feature? Do we actually need the PB boolean? PB can differentiate between not specified, empty string, and set. IIUC it's not specified, then it's an old NN, and we can fallback.
          Hide
          shahrs87 Rushabh S Shah added a comment -

          Added 2 member variable (optional in protobuf format) in FsServerDefaults:
          1. namenodeSupportsProviderUri: to differentiate between empty keyProviderUri and namenode not supporting encrpytion zones and supporting backward compatibility.
          2. keyProviderUri:

          New namenode returns namenodeSupportsProviderUri as true.
          If namenodeSupportsProviderUri is true, then the client will always trust whatever namenode returned in keyProviderUri.
          If namenodeSupportsProviderUri is false (that means the namenode is not upgraded), then the client will use it own conf as before thereby supporting backwards compatibility.

          DFSClient.java
            public boolean isHDFSEncryptionEnabled() {
              try {
                return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri());
              } catch(IOException ioe) {
                return false;
              }
            }
          
          DFSClient.java
            private String getKeyProviderUri() throws IOException {
              FsServerDefaults serverDefaults = getServerDefaults();
              return serverDefaults.getNamenodeSupportsProviderUri() ?
                  serverDefaults.getKeyProviderUri() : conf.getTrimmed(
                  CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH, "");
            }
          

          DFSClient#getServerDefaults throws IOExcpetion (StandbyException)
          If DFSUtilClient#isHDFSEncryptionEnabled throws IOException then I am returning false.
          Not sure what should I return.
          Comments are welcome.

          Please review.

          Show
          shahrs87 Rushabh S Shah added a comment - Added 2 member variable (optional in protobuf format) in FsServerDefaults: 1. namenodeSupportsProviderUri: to differentiate between empty keyProviderUri and namenode not supporting encrpytion zones and supporting backward compatibility. 2. keyProviderUri: New namenode returns namenodeSupportsProviderUri as true. If namenodeSupportsProviderUri is true, then the client will always trust whatever namenode returned in keyProviderUri. If namenodeSupportsProviderUri is false (that means the namenode is not upgraded), then the client will use it own conf as before thereby supporting backwards compatibility. DFSClient.java public boolean isHDFSEncryptionEnabled() { try { return DFSUtilClient.isHDFSEncryptionEnabled(getKeyProviderUri()); } catch (IOException ioe) { return false ; } } DFSClient.java private String getKeyProviderUri() throws IOException { FsServerDefaults serverDefaults = getServerDefaults(); return serverDefaults.getNamenodeSupportsProviderUri() ? serverDefaults.getKeyProviderUri() : conf.getTrimmed( CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH, ""); } DFSClient#getServerDefaults throws IOExcpetion (StandbyException) If DFSUtilClient#isHDFSEncryptionEnabled throws IOException then I am returning false. Not sure what should I return. Comments are welcome. Please review.

            People

            • Assignee:
              shahrs87 Rushabh S Shah
              Reporter:
              shahrs87 Rushabh S Shah
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development