Xiao Chen added a comment - 21/Mar/16 22:44 Patch 1 explains the current relation between KMS ACL and Key ACL. Also corrected some outdated texts regarding delegation tokens. I have to admit that my understanding towards delegation token is limited, but IIUC, we don't need extra configs for it on HA since the configurations in 'HTTP Authentication Signature' already explains it.

Hadoop QA added a comment - 22/Mar/16 01:16 +1 overall Vote Subsystem Runtime Comment 0 reexec 12m 56s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. 0 mvndep 0m 18s Maven dependency ordering for branch +1 mvninstall 9m 47s trunk passed +1 mvnsite 0m 58s trunk passed 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvnsite 0m 50s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 asflicense 0m 24s Patch does not generate ASF License warnings. 25m 49s Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12794624/HADOOP-12951.01.patch JIRA Issue HADOOP-12951 Optional Tests asflicense mvnsite uname Linux 70631512b8d0 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e7ed05e modules C: hadoop-common-project/hadoop-auth hadoop-common-project/hadoop-kms U: hadoop-common-project Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/8888/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.

Andrew Wang added a comment - 23/Mar/16 19:45 The delegation token discussion is pretty important to understand KMS HA, so let's talk about that a little. A delegation token is essentially a time-bounded authentication mechanism, which is cryptographically signed and verified via a shared secret. In the case of KMS HA, we have multiple KMS instances, all of which need to be able to verify delegation tokens given out by another KMS. This means the shared secret needs to be shared, which is done this by retrieving the shared secret from ZooKeeper. So, if you configure KMS HA, and you have security turned on (which you should), you need to use ZooKeeper secret storage. I think this is what the "TBD" section was meant to cover. Hopefully that's enough to get started. I think I can dig up more references on delegation tokens and KMS HA if that will help, DTs in particular should already be covered in some part of the Hadoop docs. Otherwise looks good!

Xiao Chen added a comment - 24/Mar/16 03:56 Thanks for the explanation Andrew Wang ! Sorry I didn't ask the correct question. On KMS documentation page, it first explains KMS delegation token configuration, then talks about HA in another section, where it's called 'Using Multiple Instances of KMS Behind a Load-Balancer or VIP' . Since authentication is done by KMSAuthenticationFilter , which inherits from DelegationTokenAuthenticationFilter which inherits from AuthenticationFilter , I think from configuration and example point of view, they're the same as those given in 'HTTP Authentication Signature' in the same KMS HA section. I also found that the Hadoop Auth page describes about the signer in details, and the last example being configuring multiple ZKs ( here ). So I'm thinking of just add some descriptive text on the delegation tokens HA section, and point to the Auth page. (Auth page seems a bit out dated, will modify as well.) One confusion though is the xml format is different, but I think that can be easily explained, and better than having 2 places showing similar examples. Does this sound right to you? Please correct me if I misunderstood anything.

Andrew Wang added a comment - 24/Mar/16 22:06 Sure, that sounds good to me. We can address auth doc improvements in this JIRA too if you want.

Xiao Chen added a comment - 01/Apr/16 06:46 Sorry about the delay here Andrew Wang . I've updated all related parts in patch 2, please let me know what you think. Thanks. The auth page is majorly copied from the javadoc at the beginning of the classes ( AuthenticationFilter , ZKSignerSecretProvider ), with corrections by checking the actual code. I hope we could have a way to auto-generate docs from class headers, but maybe that's not realistic at this point, unless we standardize that for all hadoop classes. Anyways, although the auth page has some redundant information, but I think its structure is good and easy to understand/find. So I manually fixed/updated related docs.

Hadoop QA added a comment - 01/Apr/16 08:32 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 14s Maven dependency ordering for branch +1 mvninstall 11m 7s trunk passed +1 compile 15m 11s trunk passed with JDK v1.8.0_74 +1 compile 11m 49s trunk passed with JDK v1.7.0_95 +1 checkstyle 0m 40s trunk passed +1 mvnsite 1m 7s trunk passed +1 mvneclipse 0m 42s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 0m 53s trunk passed with JDK v1.8.0_74 +1 javadoc 0m 46s trunk passed with JDK v1.7.0_95 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 0m 54s the patch passed +1 compile 15m 5s the patch passed with JDK v1.8.0_74 +1 javac 15m 5s the patch passed +1 compile 11m 59s the patch passed with JDK v1.7.0_95 +1 javac 11m 59s the patch passed -1 checkstyle 0m 40s hadoop-common-project: patch generated 2 new + 53 unchanged - 0 fixed = 55 total (was 53) +1 mvnsite 1m 6s the patch passed +1 mvneclipse 0m 45s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 1m 54s the patch passed +1 javadoc 0m 57s the patch passed with JDK v1.8.0_74 +1 javadoc 0m 49s the patch passed with JDK v1.7.0_95 +1 unit 4m 42s hadoop-auth in the patch passed with JDK v1.8.0_74. +1 unit 1m 56s hadoop-kms in the patch passed with JDK v1.8.0_74. +1 unit 4m 25s hadoop-auth in the patch passed with JDK v1.7.0_95. -1 unit 1m 53s hadoop-kms in the patch failed with JDK v1.7.0_95. +1 asflicense 0m 31s Patch does not generate ASF License warnings. 94m 9s Reason Tests JDK v1.7.0_95 Failed junit tests hadoop.crypto.key.kms.server.TestKMS Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12796481/HADOOP-12951.02.patch JIRA Issue HADOOP-12951 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b51895152cfd 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 3488c4f Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9003/artifact/patchprocess/diff-checkstyle-hadoop-common-project.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9003/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-kms-jdk1.7.0_95.txt unit test logs https://builds.apache.org/job/PreCommit-HADOOP-Build/9003/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-kms-jdk1.7.0_95.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9003/testReport/ modules C: hadoop-common-project/hadoop-auth hadoop-common-project/hadoop-kms U: hadoop-common-project Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9003/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.

Xiao Chen added a comment - 01/Apr/16 16:34 Patch 3 fixes the 80 chars.

Hadoop QA added a comment - 01/Apr/16 17:30 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 19s Maven dependency ordering for branch +1 mvninstall 6m 59s trunk passed +1 compile 5m 52s trunk passed with JDK v1.8.0_74 +1 compile 6m 33s trunk passed with JDK v1.7.0_95 +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 26s trunk passed +1 findbugs 0m 55s trunk passed +1 javadoc 0m 24s trunk passed with JDK v1.8.0_74 +1 javadoc 0m 28s trunk passed with JDK v1.7.0_95 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 0m 33s the patch passed +1 compile 5m 32s the patch passed with JDK v1.8.0_74 +1 javac 5m 32s the patch passed +1 compile 6m 31s the patch passed with JDK v1.7.0_95 +1 javac 6m 31s the patch passed +1 checkstyle 0m 25s the patch passed +1 mvnsite 0m 40s the patch passed +1 mvneclipse 0m 26s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 1m 16s the patch passed +1 javadoc 0m 23s the patch passed with JDK v1.8.0_74 +1 javadoc 0m 28s the patch passed with JDK v1.7.0_95 +1 unit 3m 36s hadoop-auth in the patch passed with JDK v1.8.0_74. +1 unit 1m 30s hadoop-kms in the patch passed with JDK v1.8.0_74. +1 unit 4m 1s hadoop-auth in the patch passed with JDK v1.7.0_95. +1 unit 1m 35s hadoop-kms in the patch passed with JDK v1.7.0_95. +1 asflicense 0m 23s Patch does not generate ASF License warnings. 52m 1s Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12796549/HADOOP-12951.03.patch JIRA Issue HADOOP-12951 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 3a1bf10f3724 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 256c82f Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9006/testReport/ modules C: hadoop-common-project/hadoop-auth hadoop-common-project/hadoop-kms U: hadoop-common-project Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9006/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.

Andrew Wang added a comment - 02/Apr/16 01:01 This looks great. Only one comment, related to the AuthenticationFilter javadoc. As you say, the user doc and the javadoc are very very similar. I noticed that the javadoc is still incorrectly referring to "string" rather than "file". As a fix, how about we delete the javadoc except for the first paragraph, and add a reference to the md file? This is a private class so devs can find the md file, and this way we aren't duplicating the information in two places.

Xiao Chen added a comment - 02/Apr/16 01:46 Good idea Andrew Wang ! That level of details can be gained by either reading the docs page, or just reading the code itself. Patch 4 removes the javadocs in that 2 classes that I think is not necessary. (I left more than 1 paragraph - those left could be useful IMO, and does not exist on the doc). Verified the link to the configuration works, after building the docs then javadoc.

Hadoop QA added a comment - 02/Apr/16 02:40 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 8s Maven dependency ordering for branch +1 mvninstall 6m 36s trunk passed +1 compile 5m 52s trunk passed with JDK v1.8.0_77 +1 compile 6m 48s trunk passed with JDK v1.7.0_95 +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 27s trunk passed +1 findbugs 0m 52s trunk passed +1 javadoc 0m 24s trunk passed with JDK v1.8.0_77 +1 javadoc 0m 28s trunk passed with JDK v1.7.0_95 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 0m 33s the patch passed +1 compile 5m 44s the patch passed with JDK v1.8.0_77 +1 javac 5m 44s the patch passed +1 compile 6m 46s the patch passed with JDK v1.7.0_95 +1 javac 6m 46s the patch passed -1 checkstyle 0m 26s hadoop-common-project: patch generated 2 new + 46 unchanged - 6 fixed = 48 total (was 52) +1 mvnsite 0m 42s the patch passed +1 mvneclipse 0m 26s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 1m 19s the patch passed +1 javadoc 0m 24s the patch passed with JDK v1.8.0_77 +1 javadoc 0m 28s the patch passed with JDK v1.7.0_95 +1 unit 3m 35s hadoop-auth in the patch passed with JDK v1.8.0_77. +1 unit 1m 30s hadoop-kms in the patch passed with JDK v1.8.0_77. +1 unit 4m 3s hadoop-auth in the patch passed with JDK v1.7.0_95. +1 unit 1m 37s hadoop-kms in the patch passed with JDK v1.7.0_95. +1 asflicense 0m 23s Patch does not generate ASF License warnings. 52m 14s Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12796638/HADOOP-12951.04.patch JIRA Issue HADOOP-12951 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux be0b8289edab 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 81d04ca Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9010/artifact/patchprocess/diff-checkstyle-hadoop-common-project.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9010/testReport/ modules C: hadoop-common-project/hadoop-auth hadoop-common-project/hadoop-kms U: hadoop-common-project Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9010/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.

Andrew Wang added a comment - 08/Apr/16 06:52 This looks great. Thanks for the thoroughness here Xiao. Committed to trunk, branch-2, branch-2.8.

Hudson added a comment - 08/Apr/16 07:32 FAILURE: Integrated in Hadoop-trunk-Commit #9578 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9578/ ) HADOOP-12951 . Improve documentation on KMS ACLs and delegation tokens. (wang: rev 594c70f779b277bd0b9d0a5dc98c3e9cc49b7e91) hadoop-common-project/hadoop-auth/src/site/markdown/Configuration.md hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/ZKSignerSecretProvider.java hadoop-common-project/hadoop-kms/src/site/markdown/index.md.vm hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/server/AuthenticationFilter.java

Xiao Chen added a comment - 08/Apr/16 16:39 Thanks Andrew Wang !