Wei-Chiu Chuang added a comment - 21/Sep/16 21:07 v01. a one-liner fixer to set UGI configuration correctly when KMS starts. I don't think it's easy to unit test it, but I've manually verified this fix works on a CDH 5.7.0 cluster.

Xiao Chen added a comment - 21/Sep/16 21:19 +1 pending jenkins. Nice work Wei-Chiu! This is clearly a bug and I think manual testing is fine. Could you elaborate a little bit on the manual testings you've done? Thanks.

Wei-Chiu Chuang added a comment - 21/Sep/16 22:11 Here's what I did to verify the patch: Configure a CDH Hadoop cluster using LdapGroupsMapping and KMS. The KMS ACL rule denies "group1" from decrypting the key. I added additional log at Groups#<init> to print the class name of the GroupMapping resolution object. Subsequently, I started KMS and do a few operations in a HDFS encryption zone to observe the class name printed.

Hadoop QA added a comment - 21/Sep/16 22:14 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 6m 52s trunk passed +1 compile 7m 19s trunk passed +1 checkstyle 0m 11s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 11s trunk passed +1 findbugs 0m 22s trunk passed +1 javadoc 0m 11s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 7m 13s the patch passed +1 javac 7m 13s the patch passed +1 checkstyle 0m 13s the patch passed +1 mvnsite 0m 18s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 31s the patch passed +1 javadoc 0m 14s the patch passed -1 unit 1m 32s hadoop-kms in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 27m 52s Reason Tests Failed junit tests hadoop.crypto.key.kms.server.TestKMS Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13638 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829690/HADOOP-13638.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 9ddaad62c252 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 / 964e546 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10559/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10559/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10559/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Xiao Chen added a comment - 22/Sep/16 06:14 Thanks for the explanation Wei-Chiu, sgtm. Failed test looks related.

Wei-Chiu Chuang added a comment - 22/Sep/16 06:48 The tests failed because UserGroupInformation.setConfiguration(conf) sets a global variable, and each test has a client side configuration which is overwritten by it. Need to find a way to avoid interference between client and server.

Xiao Chen added a comment - 22/Sep/16 17:12 Thanks for looking into the test Wei-Chiu. Looking forward to the fix. This will also give audience more time to review this.

Wei-Chiu Chuang added a comment - 22/Sep/16 18:38 - edited v02: The Configuration object is shared by both KMS client and server in unit tests because UGI gets/sets it to a static variable. I don't see a way to isolate client's configuration from server. Fortunately, UGI-sensitive configuration names are independent between KMS client and server. As a workaround, make sure the client configurations are copied to the server's so that client can read them.

Hadoop QA added a comment - 22/Sep/16 19:54 +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 1 new or modified test files. +1 mvninstall 7m 43s trunk passed +1 compile 7m 32s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 23s trunk passed +1 javadoc 0m 11s trunk passed +1 mvninstall 0m 15s the patch passed +1 compile 7m 5s the patch passed +1 javac 7m 5s the patch passed -0 checkstyle 0m 14s hadoop-common-project/hadoop-kms: The patch generated 3 new + 114 unchanged - 3 fixed = 117 total (was 117) +1 mvnsite 0m 18s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 29s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 8s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 29m 36s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13638 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829906/HADOOP-13638.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4f77a4ead812 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 40acace Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10574/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10574/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10574/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Wei-Chiu Chuang added a comment - 22/Sep/16 21:09 Checkstyle warnings can't be removed unless we refactor test methods.

Xiao Chen added a comment - 23/Sep/16 23:32 Thanks Wei-Chiu Chuang for the new patch. Could you explain why do we need newConf = new Configuration(false); instead of the default ctor?

Wei-Chiu Chuang added a comment - 26/Sep/16 18:46 That was copied from an existing test case.

Xiao Chen added a comment - 26/Sep/16 20:02 Checkstyle is out-of-scope as Wei-Chiu mentioned. +1 to patch 2, committing this.

Xiao Chen added a comment - 26/Sep/16 20:12 I have committed this to trunk, branch-2 and branch-2.8. Let me know if you feel this should go to earlier branches, from the Target Versions field. Thanks for the contribution Wei-Chiu Chuang !

Hudson added a comment - 26/Sep/16 21:34 SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10489 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10489/ ) HADOOP-13638 . KMS should set UGI's Configuration object properly. (xiao: rev fa397e74fe988bcbb05c816de73eb738794ace4b) (edit) hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMS.java (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSWebApp.java

Wei-Chiu Chuang added a comment - 26/Sep/16 22:10 Thanks very much Xiao Chen for reviewing and committing the patch!