Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-alpha1
    • Component/s: kms
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      Currently, KMS audit log is using log4j, to write a text format log.
      We should refactor this, so that people can easily add new format audit logs. The current text format log should be the default, and all of its behavior should remain compatible.

      1. HADOOP-13396.01.patch
        26 kB
        Xiao Chen
      2. HADOOP-13396.02.patch
        46 kB
        Xiao Chen
      3. HADOOP-13396.03.patch
        46 kB
        Xiao Chen
      4. HADOOP-13396.04.patch
        42 kB
        Xiao Chen
      5. HADOOP-13396.05.patch
        43 kB
        Xiao Chen
      6. HADOOP-13396.06.patch
        43 kB
        Xiao Chen
      7. HADOOP-13396.07.patch
        26 kB
        Xiao Chen
      8. HADOOP-13396.08.patch
        28 kB
        Xiao Chen
      9. HADOOP-13396.09.patch
        27 kB
        Xiao Chen

        Issue Links

          Activity

          Hide
          busbey Sean Busbey added a comment -

          if we want a structured audit log, why not something more compact and precise like Avro?

          Show
          busbey Sean Busbey added a comment - if we want a structured audit log, why not something more compact and precise like Avro?
          Hide
          aw Allen Wittenauer added a comment -

          Binary formats are pretty useless without tools written to actually process them. So to me, the #1 requirement here is that the standard sysadmin toolkit needs to be usable here, e.g., grep. Being a fixed field format was one of the absolute keys to success of the HDFS audit log and one of the reasons why people still use it over other solutions like the weirdo notification thing.

          With that in mind, if JSON is really wanted, then it needs to get printed on a single line.

          Show
          aw Allen Wittenauer added a comment - Binary formats are pretty useless without tools written to actually process them. So to me, the #1 requirement here is that the standard sysadmin toolkit needs to be usable here, e.g., grep. Being a fixed field format was one of the absolute keys to success of the HDFS audit log and one of the reasons why people still use it over other solutions like the weirdo notification thing. With that in mind, if JSON is really wanted, then it needs to get printed on a single line.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Sean and Allen for the comments.

          The requirement for this comes from a post-processing tool, which accepts only json format logs. I'm aware there's an option to let that tool to do the conversion from current text format audit-log to json by itself. However, adding a plugin to the KMS audit also feels reasonable to me. With the plugin logger, one can easily extend the current logging with the formats needed, while maintaining backwards-compatibility.

          I'm working on the patch, will post it soon to express the idea.

          Show
          xiaochen Xiao Chen added a comment - Thanks Sean and Allen for the comments. The requirement for this comes from a post-processing tool, which accepts only json format logs. I'm aware there's an option to let that tool to do the conversion from current text format audit-log to json by itself. However, adding a plugin to the KMS audit also feels reasonable to me. With the plugin logger, one can easily extend the current logging with the formats needed, while maintaining backwards-compatibility. I'm working on the patch, will post it soon to express the idea.
          Hide
          aw Allen Wittenauer added a comment -

          The requirement for this comes from a post-processing tool, which accepts only json format logs.

          Why can't we spit out fixed field and then use a tool to convert that to JSON so that this other mystery tool can process it?

          However, adding a plugin to the KMS audit

          Is the intent to output multiple format types then?

          one can easily extend the current logging with the formats needed, while maintaining backwards-compatibility.

          This isn't really true in practice. See, e.g., http://linuxjedi.co.uk/posts/2014/Oct/31/why-json-is-bad-for-applications/ . Lots of other articles on this topic elsewhere.

          Show
          aw Allen Wittenauer added a comment - The requirement for this comes from a post-processing tool, which accepts only json format logs. Why can't we spit out fixed field and then use a tool to convert that to JSON so that this other mystery tool can process it? However, adding a plugin to the KMS audit Is the intent to output multiple format types then? one can easily extend the current logging with the formats needed, while maintaining backwards-compatibility. This isn't really true in practice. See, e.g., http://linuxjedi.co.uk/posts/2014/Oct/31/why-json-is-bad-for-applications/ . Lots of other articles on this topic elsewhere.
          Hide
          busbey Sean Busbey added a comment -

          If it's pluggable with multiple format types, then that sounds fine by me so long as the default is a plain text greppable option as Allen mentions. I'd be happy to make an Avro one to provide a third implementation example for the pluggable API.

          Show
          busbey Sean Busbey added a comment - If it's pluggable with multiple format types, then that sounds fine by me so long as the default is a plain text greppable option as Allen mentions. I'd be happy to make an Avro one to provide a third implementation example for the pluggable API.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks again Sean and Allen for the valuable comments.

          For this jira, the goal is to:

          1. add a json format audit logger to the KMS
          2. without impacting the existing plain text audit logging output.
          3. with the required fields for the json format logging (than the current plain text).

          I'm aware json is not the magical solution, but what I'm trying to do here is to abstract the actual logging format from the existing audit log calls, so one can add other types as a plugin, such as Avro like Sean has generously offered.

          The difficulty, IMHO, is that the existing audit log doesn't have a well-defined API, or even consistent format for different result codes (I'll attach an example log). Attached a patch to express the idea: #1 and #2 are done via extraction, and #3 are only changed in the new json format logger. Existing format is verified by the current tests, which I enhanced in the dependent jira HADOOP-13395.

          Appreciate your review and feedback.

          Show
          xiaochen Xiao Chen added a comment - Thanks again Sean and Allen for the valuable comments. For this jira, the goal is to: add a json format audit logger to the KMS without impacting the existing plain text audit logging output. with the required fields for the json format logging (than the current plain text). I'm aware json is not the magical solution, but what I'm trying to do here is to abstract the actual logging format from the existing audit log calls, so one can add other types as a plugin, such as Avro like Sean has generously offered. The difficulty, IMHO, is that the existing audit log doesn't have a well-defined API, or even consistent format for different result codes (I'll attach an example log). Attached a patch to express the idea: #1 and #2 are done via extraction, and #3 are only changed in the new json format logger. Existing format is verified by the current tests, which I enhanced in the dependent jira HADOOP-13395 . Appreciate your review and feedback.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 7m 50s trunk passed
          +1 compile 8m 40s trunk passed
          +1 checkstyle 0m 14s trunk passed
          +1 mvnsite 0m 21s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 0m 28s trunk passed
          +1 javadoc 0m 13s trunk passed
          -1 mvninstall 0m 12s hadoop-kms in the patch failed.
          -1 compile 1m 19s root in the patch failed.
          -1 javac 1m 19s root in the patch failed.
          -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 17 new + 16 unchanged - 4 fixed = 33 total (was 20)
          -1 mvnsite 0m 13s hadoop-kms in the patch failed.
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 0m 13s hadoop-kms in the patch failed.
          -1 javadoc 0m 11s hadoop-common-project_hadoop-kms generated 19 new + 0 unchanged - 0 fixed = 19 total (was 0)
          -1 unit 0m 13s hadoop-kms in the patch failed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          22m 35s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820114/HADOOP-13396.01.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit compile javac javadoc mvninstall findbugs checkstyle
          uname Linux 44c6417a5ec9 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 / 7cac765
          Default Java 1.8.0_101
          findbugs v3.0.0
          mvninstall https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-kms.txt
          compile https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-compile-root.txt
          javac https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-compile-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          mvnsite https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-kms.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-kms.txt
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-kms.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 7m 50s trunk passed +1 compile 8m 40s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 21s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 0m 28s trunk passed +1 javadoc 0m 13s trunk passed -1 mvninstall 0m 12s hadoop-kms in the patch failed. -1 compile 1m 19s root in the patch failed. -1 javac 1m 19s root in the patch failed. -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 17 new + 16 unchanged - 4 fixed = 33 total (was 20) -1 mvnsite 0m 13s hadoop-kms in the patch failed. +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 0m 13s hadoop-kms in the patch failed. -1 javadoc 0m 11s hadoop-common-project_hadoop-kms generated 19 new + 0 unchanged - 0 fixed = 19 total (was 0) -1 unit 0m 13s hadoop-kms in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 22m 35s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820114/HADOOP-13396.01.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit compile javac javadoc mvninstall findbugs checkstyle uname Linux 44c6417a5ec9 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 / 7cac765 Default Java 1.8.0_101 findbugs v3.0.0 mvninstall https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-kms.txt compile https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-compile-root.txt javac https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-compile-root.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt mvnsite https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-kms.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-kms.txt javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-kms.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10082/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Oops, missed the newly added files when git add... Attaching a complete patch 2.

          Show
          xiaochen Xiao Chen added a comment - Oops, missed the newly added files when git add ... Attaching a complete patch 2.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s 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.
          +1 mvninstall 6m 45s trunk passed
          +1 compile 6m 45s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 18s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 20s trunk passed
          +1 javadoc 0m 12s trunk passed
          +1 mvninstall 0m 15s the patch passed
          +1 compile 6m 42s the patch passed
          +1 javac 6m 42s the patch passed
          -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 16 new + 15 unchanged - 5 fixed = 31 total (was 20)
          +1 mvnsite 0m 18s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 1s The patch has no whitespace issues.
          -1 findbugs 0m 31s hadoop-common-project/hadoop-kms generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0)
          -1 javadoc 0m 12s hadoop-common-project_hadoop-kms generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
          +1 unit 2m 15s hadoop-kms in the patch passed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          27m 24s



          Reason Tests
          FindBugs module:hadoop-common-project/hadoop-kms
            Found reliance on default encoding in org.apache.hadoop.crypto.key.kms.server.KMSAudit$JsonKMSAuditLogger.logEvent(String, String, String, KMS$KMSOp, long, boolean, Map):in org.apache.hadoop.crypto.key.kms.server.KMSAudit$JsonKMSAuditLogger.logEvent(String, String, String, KMS$KMSOp, long, boolean, Map): java.io.ByteArrayOutputStream.toString() At KMSAudit.java:[line 219]
            Should org.apache.hadoop.crypto.key.kms.server.KMSAudit$JsonKMSAuditLogger be a static inner class? At KMSAudit.java:inner class? At KMSAudit.java:[lines 125-234]
            Should org.apache.hadoop.crypto.key.kms.server.KMSAudit$SimpleKMSAuditLogger be a static inner class? At KMSAudit.java:inner class? At KMSAudit.java:[lines 63-119]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820340/HADOOP-13396.02.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit compile javac javadoc mvninstall findbugs checkstyle
          uname Linux 9f74682bf619 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 / d84ab8a
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-kms.html
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s 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. +1 mvninstall 6m 45s trunk passed +1 compile 6m 45s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 20s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 15s the patch passed +1 compile 6m 42s the patch passed +1 javac 6m 42s the patch passed -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 16 new + 15 unchanged - 5 fixed = 31 total (was 20) +1 mvnsite 0m 18s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 1s The patch has no whitespace issues. -1 findbugs 0m 31s hadoop-common-project/hadoop-kms generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0) -1 javadoc 0m 12s hadoop-common-project_hadoop-kms generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) +1 unit 2m 15s hadoop-kms in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 27m 24s Reason Tests FindBugs module:hadoop-common-project/hadoop-kms   Found reliance on default encoding in org.apache.hadoop.crypto.key.kms.server.KMSAudit$JsonKMSAuditLogger.logEvent(String, String, String, KMS$KMSOp, long, boolean, Map):in org.apache.hadoop.crypto.key.kms.server.KMSAudit$JsonKMSAuditLogger.logEvent(String, String, String, KMS$KMSOp, long, boolean, Map): java.io.ByteArrayOutputStream.toString() At KMSAudit.java: [line 219]   Should org.apache.hadoop.crypto.key.kms.server.KMSAudit$JsonKMSAuditLogger be a static inner class? At KMSAudit.java:inner class? At KMSAudit.java: [lines 125-234]   Should org.apache.hadoop.crypto.key.kms.server.KMSAudit$SimpleKMSAuditLogger be a static inner class? At KMSAudit.java:inner class? At KMSAudit.java: [lines 63-119] Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820340/HADOOP-13396.02.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit compile javac javadoc mvninstall findbugs checkstyle uname Linux 9f74682bf619 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 / d84ab8a Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-kms.html javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10094/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Patch 3 to satisfy pre-commit.

          Show
          xiaochen Xiao Chen added a comment - Patch 3 to satisfy pre-commit.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 4 new or modified test files.
          +1 mvninstall 7m 34s trunk passed
          +1 compile 7m 3s trunk passed
          +1 checkstyle 0m 13s trunk passed
          +1 mvnsite 0m 18s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 22s trunk passed
          +1 javadoc 0m 12s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 7m 12s the patch passed
          +1 javac 7m 12s the patch passed
          -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 11 new + 14 unchanged - 5 fixed = 25 total (was 19)
          +1 mvnsite 0m 19s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 0m 32s hadoop-common-project/hadoop-kms generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
          +1 javadoc 0m 12s the patch passed
          +1 unit 2m 22s hadoop-kms in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          29m 7s



          Reason Tests
          FindBugs module:hadoop-common-project/hadoop-kms
            Should org.apache.hadoop.crypto.key.kms.server.KMSAudit$JsonKMSAuditLogger be a static inner class? At KMSAudit.java:inner class? At KMSAudit.java:[lines 125-235]
            Should org.apache.hadoop.crypto.key.kms.server.KMSAudit$SimpleKMSAuditLogger be a static inner class? At KMSAudit.java:inner class? At KMSAudit.java:[lines 63-119]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12821182/HADOOP-13396.03.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit compile javac javadoc mvninstall findbugs checkstyle
          uname Linux 7b94ec29c7a2 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 / ef501b1
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10129/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10129/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-kms.html
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10129/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10129/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s 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. +1 mvninstall 7m 34s trunk passed +1 compile 7m 3s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 22s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 7m 12s the patch passed +1 javac 7m 12s the patch passed -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 11 new + 14 unchanged - 5 fixed = 25 total (was 19) +1 mvnsite 0m 19s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 0m 32s hadoop-common-project/hadoop-kms generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) +1 javadoc 0m 12s the patch passed +1 unit 2m 22s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 29m 7s Reason Tests FindBugs module:hadoop-common-project/hadoop-kms   Should org.apache.hadoop.crypto.key.kms.server.KMSAudit$JsonKMSAuditLogger be a static inner class? At KMSAudit.java:inner class? At KMSAudit.java: [lines 125-235]   Should org.apache.hadoop.crypto.key.kms.server.KMSAudit$SimpleKMSAuditLogger be a static inner class? At KMSAudit.java:inner class? At KMSAudit.java: [lines 63-119] Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12821182/HADOOP-13396.03.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit compile javac javadoc mvninstall findbugs checkstyle uname Linux 7b94ec29c7a2 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 / ef501b1 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10129/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10129/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-kms.html Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10129/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10129/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Had an offline chat with Wei-Chiu Chuang, and got some valuable feedback:

          • I will update the description for more context
          • Should document this better, perhaps in a new jira
          • Added the configuration item into kms-site.xml
          • Rebased on HADOOP-13395.
          Show
          xiaochen Xiao Chen added a comment - Had an offline chat with Wei-Chiu Chuang , and got some valuable feedback: I will update the description for more context Should document this better, perhaps in a new jira Added the configuration item into kms-site.xml Rebased on HADOOP-13395 .
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 4 new or modified test files.
          +1 mvninstall 6m 35s trunk passed
          +1 compile 6m 49s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 18s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 20s trunk passed
          +1 javadoc 0m 12s trunk passed
          +1 mvninstall 0m 15s the patch passed
          +1 compile 6m 40s the patch passed
          +1 javac 6m 40s the patch passed
          -0 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 4 new + 21 unchanged - 4 fixed = 25 total (was 25)
          +1 mvnsite 0m 19s the patch passed
          +1 mvneclipse 0m 12s 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 0m 29s the patch passed
          +1 javadoc 0m 12s the patch passed
          +1 unit 2m 12s hadoop-kms in the patch passed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          27m 9s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12822883/HADOOP-13396.04.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle
          uname Linux 7518c7814b33 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 / 85422bb
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10213/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10213/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10213/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s 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. +1 mvninstall 6m 35s trunk passed +1 compile 6m 49s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 20s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 15s the patch passed +1 compile 6m 40s the patch passed +1 javac 6m 40s the patch passed -0 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 4 new + 21 unchanged - 4 fixed = 25 total (was 25) +1 mvnsite 0m 19s the patch passed +1 mvneclipse 0m 12s 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 0m 29s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 12s hadoop-kms in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 27m 9s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12822883/HADOOP-13396.04.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle uname Linux 7518c7814b33 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 / 85422bb Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10213/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10213/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10213/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Hi Xiao Chen thanks for the updated patch. Overall looks good to me. I have a few comments, and most are cosmetic:

          • KMSAudit.JsonKMSAuditLogger#logAuditEvent
            LOG.error("Exception caught when logging {}", event, e);
            

            This won’t print the exception nor its stack trace. The similar kind of change is needed for handling other exceptions.

          • In KMSAuditLogger.AuditEvent#toString
            you can actually do: sb.append(X).append(Y).append(Z);
          • TestKMSAuditJson
            please import classes explicitly instead of wildcard import.
            import static org.apache.hadoop.crypto.key.kms.server.KMSAudit.JsonKMSAuditLogger.*;
            
          • KMSAudit#error:
            I noticed the url parameter of this method is not used. Would it make sense to append the url to extraMsg?
          • KMSAudit#initializeAuditLoggers
            I suppose the ‘continue’ is redundant?
            for (String l : loggers) {
              if (l.equals(SimpleKMSAuditLogger.TYPE)) {
                auditLoggers.add(new SimpleKMSAuditLogger());
              } else if (l.equals(JsonKMSAuditLogger.TYPE)) {
                auditLoggers.add(new JsonKMSAuditLogger());
                continue;
              } else {
                LOG.warn("Ignored unknown audit logger type {}", l);
              }
            
          • KMSAudit#op:
            It does not make sense to me that the code throws a RuntimeException when an ExecutionException is thrown. It transforms a checked exception to an unchecked one, and if the exception is thrown, the entire process is terminated if I understand it correctly.
          Show
          jojochuang Wei-Chiu Chuang added a comment - Hi Xiao Chen thanks for the updated patch. Overall looks good to me. I have a few comments, and most are cosmetic: KMSAudit.JsonKMSAuditLogger#logAuditEvent LOG.error( "Exception caught when logging {}" , event, e); This won’t print the exception nor its stack trace. The similar kind of change is needed for handling other exceptions. In KMSAuditLogger.AuditEvent#toString you can actually do: sb.append(X).append(Y).append(Z); TestKMSAuditJson please import classes explicitly instead of wildcard import. import static org.apache.hadoop.crypto.key.kms.server.KMSAudit.JsonKMSAuditLogger.*; KMSAudit#error: I noticed the url parameter of this method is not used. Would it make sense to append the url to extraMsg? KMSAudit#initializeAuditLoggers I suppose the ‘continue’ is redundant? for ( String l : loggers) { if (l.equals(SimpleKMSAuditLogger.TYPE)) { auditLoggers.add( new SimpleKMSAuditLogger()); } else if (l.equals(JsonKMSAuditLogger.TYPE)) { auditLoggers.add( new JsonKMSAuditLogger()); continue ; } else { LOG.warn( "Ignored unknown audit logger type {}" , l); } KMSAudit#op: It does not make sense to me that the code throws a RuntimeException when an ExecutionException is thrown. It transforms a checked exception to an unchecked one, and if the exception is thrown, the entire process is terminated if I understand it correctly.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Wei-Chiu Chuang for the review and comments. Patch 5 addressed all except:

          KMSAudit.JsonKMSAuditLogger#logAuditEvent This won’t print the exception nor its stack trace. The similar kind of change is needed for handling other exceptions.

          Not sure what exactly the concern is. Per http://www.slf4j.org/apidocs/org/slf4j/Logger.html, the throwable works. Also verified locally.

          KMSAudit#op: RuntimeException v.s. ExecutionException

          Thanks for pointing me offline to http://google.github.io/guava/releases/snapshot/api/docs/com/google/common/cache/LoadingCache.html#getUnchecked(K) . Here it's calling get(K, Callable) though, I didn't find a replacement. It's out of this jira's scope too, so let's follow up in new jira.

          Show
          xiaochen Xiao Chen added a comment - Thanks Wei-Chiu Chuang for the review and comments. Patch 5 addressed all except: KMSAudit.JsonKMSAuditLogger#logAuditEvent This won’t print the exception nor its stack trace. The similar kind of change is needed for handling other exceptions. Not sure what exactly the concern is. Per http://www.slf4j.org/apidocs/org/slf4j/Logger.html , the throwable works. Also verified locally. KMSAudit#op: RuntimeException v.s. ExecutionException Thanks for pointing me offline to http://google.github.io/guava/releases/snapshot/api/docs/com/google/common/cache/LoadingCache.html#getUnchecked(K ) . Here it's calling get(K, Callable) though, I didn't find a replacement. It's out of this jira's scope too, so let's follow up in new jira.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 4 new or modified test files.
          +1 mvninstall 7m 59s trunk passed
          +1 compile 8m 6s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 19s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 21s trunk passed
          +1 javadoc 0m 12s trunk passed
          +1 mvninstall 0m 15s the patch passed
          +1 compile 6m 50s the patch passed
          +1 javac 6m 50s the patch passed
          -0 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 4 new + 21 unchanged - 4 fixed = 25 total (was 25)
          +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 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 0m 29s the patch passed
          +1 javadoc 0m 12s the patch passed
          -1 unit 2m 13s hadoop-kms in the patch failed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          30m 2s



          Reason Tests
          Failed junit tests hadoop.crypto.key.kms.server.TestKMSAudit
            hadoop.crypto.key.kms.server.TestKMSAuditJson



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12822957/HADOOP-13396.05.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle
          uname Linux 1641fc79ce0c 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 / d00d3ad
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10218/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10218/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10218/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10218/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s 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. +1 mvninstall 7m 59s trunk passed +1 compile 8m 6s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 21s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 15s the patch passed +1 compile 6m 50s the patch passed +1 javac 6m 50s the patch passed -0 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 4 new + 21 unchanged - 4 fixed = 25 total (was 25) +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 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 0m 29s the patch passed +1 javadoc 0m 12s the patch passed -1 unit 2m 13s hadoop-kms in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 30m 2s Reason Tests Failed junit tests hadoop.crypto.key.kms.server.TestKMSAudit   hadoop.crypto.key.kms.server.TestKMSAuditJson Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12822957/HADOOP-13396.05.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle uname Linux 1641fc79ce0c 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 / d00d3ad Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10218/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10218/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10218/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10218/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          KMSAudit.JsonKMSAuditLogger#logAuditEvent This won’t print the exception nor its stack trace. The similar kind of change is needed for handling other exceptions.

          My bad. I was under impression slf4j logger does not permit printing exception and stack trace.

          Show
          jojochuang Wei-Chiu Chuang added a comment - KMSAudit.JsonKMSAuditLogger#logAuditEvent This won’t print the exception nor its stack trace. The similar kind of change is needed for handling other exceptions. My bad. I was under impression slf4j logger does not permit printing exception and stack trace.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Hi Xiao Chen thanks for the updated patch and thanks for clarification. I made another round of review, and here's my comment:

          • There is a test failure due to the change in audit message.
          • user and impersonator could be null string. Would JSONGenerator#writeStartObject get an NPE if the string is null? If not, and if it prints "null" for null user/null impersonator, it could be hard to tell whether it is a user/impersonator named 'null' or a null string.
          • SimpleKMSAuditLogger#logAuditEvent:
            if the status is UNAUTHORIZED, I think you can also use logAuditSimpleFormat() to print audit log as well.
            In fact, it looks like you only need special logic when status is OK.
          • JsonKMSAuditLogger#logAuditEvent:
            I think the same applies to this audit logger class. You should be able to call logAuditSimpleFormat(). This will help simplify the logic.
            Or if it can't, see if you can extract the lines in each switch-case, because they are relatively long.
          • The parameter opStatus in private void op(OpStatus opStatus, final KMS.KMSOp op, final UserGroupInformation ugi, final String key, final String remoteHost, final String extraMsg) { should be declared as final.
          • With regard to the default logger, can we stick to the typical Hadoop convention where we define
            public static final String KMS_AUDIT_LOGGER_KEY_DEFAULT = “simple";
            This simplifies the code in KMSAudit#initializeAuditLoggers().
          Show
          jojochuang Wei-Chiu Chuang added a comment - Hi Xiao Chen thanks for the updated patch and thanks for clarification. I made another round of review, and here's my comment: There is a test failure due to the change in audit message. user and impersonator could be null string. Would JSONGenerator#writeStartObject get an NPE if the string is null? If not, and if it prints "null" for null user/null impersonator, it could be hard to tell whether it is a user/impersonator named 'null' or a null string. SimpleKMSAuditLogger#logAuditEvent: if the status is UNAUTHORIZED, I think you can also use logAuditSimpleFormat() to print audit log as well. In fact, it looks like you only need special logic when status is OK. JsonKMSAuditLogger#logAuditEvent: I think the same applies to this audit logger class. You should be able to call logAuditSimpleFormat(). This will help simplify the logic. Or if it can't, see if you can extract the lines in each switch-case, because they are relatively long. The parameter opStatus in private void op(OpStatus opStatus, final KMS.KMSOp op, final UserGroupInformation ugi, final String key, final String remoteHost, final String extraMsg) { should be declared as final. With regard to the default logger, can we stick to the typical Hadoop convention where we define public static final String KMS_AUDIT_LOGGER_KEY_DEFAULT = “simple"; This simplifies the code in KMSAudit#initializeAuditLoggers().
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Wei-Chiu. Patch 6 to address the comments. Some explanations:

          user and impersonator could be null string.

          As discussed offline, this won't cause NPE. But it's a valid point to distinguish between a user named 'null' and a null object. So using a POSIX in-compliant message to print when the latter happens.

          This simplifies the code in KMSAudit#initializeAuditLoggers().

          To keep compatibility when users run new software with old configuration, I think we need to keep it this way.

          Show
          xiaochen Xiao Chen added a comment - Thanks Wei-Chiu. Patch 6 to address the comments. Some explanations: user and impersonator could be null string. As discussed offline, this won't cause NPE. But it's a valid point to distinguish between a user named 'null' and a null object. So using a POSIX in-compliant message to print when the latter happens. This simplifies the code in KMSAudit#initializeAuditLoggers(). To keep compatibility when users run new software with old configuration, I think we need to keep it this way.
          Hide
          hadoopqa Hadoop QA added a comment -
          -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 appears to include 4 new or modified test files.
          +1 mvninstall 6m 44s trunk passed
          +1 compile 7m 34s trunk passed
          +1 checkstyle 0m 13s trunk passed
          +1 mvnsite 0m 19s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 23s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 19s the patch passed
          +1 compile 8m 24s the patch passed
          +1 javac 8m 24s the patch passed
          -0 checkstyle 0m 14s hadoop-common-project/hadoop-kms: The patch generated 4 new + 21 unchanged - 4 fixed = 25 total (was 25)
          +1 mvnsite 0m 19s the patch passed
          +1 mvneclipse 0m 13s 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 0m 34s hadoop-common-project/hadoop-kms generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 0m 12s the patch passed
          +1 unit 2m 17s hadoop-kms in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          30m 15s



          Reason Tests
          FindBugs module:hadoop-common-project/hadoop-kms
            Unwritten field:KMSAudit.java:[line 200]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823643/HADOOP-13396.06.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle
          uname Linux 630830768f2a 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 / d677b68
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10243/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10243/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-kms.html
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10243/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10243/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s 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. +1 mvninstall 6m 44s trunk passed +1 compile 7m 34s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 23s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 19s the patch passed +1 compile 8m 24s the patch passed +1 javac 8m 24s the patch passed -0 checkstyle 0m 14s hadoop-common-project/hadoop-kms: The patch generated 4 new + 21 unchanged - 4 fixed = 25 total (was 25) +1 mvnsite 0m 19s the patch passed +1 mvneclipse 0m 13s 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 0m 34s hadoop-common-project/hadoop-kms generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 0m 12s the patch passed +1 unit 2m 17s hadoop-kms in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 30m 15s Reason Tests FindBugs module:hadoop-common-project/hadoop-kms   Unwritten field:KMSAudit.java: [line 200] Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823643/HADOOP-13396.06.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle uname Linux 630830768f2a 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 / d677b68 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10243/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10243/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-kms.html Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10243/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10243/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s 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.
          +1 mvninstall 8m 4s trunk passed
          +1 compile 8m 18s trunk passed
          +1 checkstyle 0m 13s trunk passed
          +1 mvnsite 0m 17s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 22s trunk passed
          +1 javadoc 0m 11s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 6m 45s the patch passed
          +1 javac 6m 45s the patch passed
          -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 4 new + 21 unchanged - 4 fixed = 25 total (was 25)
          +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 xml 0m 1s The patch has no ill-formed XML file.
          -1 findbugs 0m 31s hadoop-common-project/hadoop-kms generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 0m 12s the patch passed
          +1 unit 2m 12s hadoop-kms in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          30m 16s



          Reason Tests
          FindBugs module:hadoop-common-project/hadoop-kms
            Unwritten field:KMSAudit.java:[line 200]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823644/HADOOP-13396.06.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle
          uname Linux b99f6546a660 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 / d677b68
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10244/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10244/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-kms.html
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10244/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10244/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s 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. +1 mvninstall 8m 4s trunk passed +1 compile 8m 18s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 17s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 22s trunk passed +1 javadoc 0m 11s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 6m 45s the patch passed +1 javac 6m 45s the patch passed -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 4 new + 21 unchanged - 4 fixed = 25 total (was 25) +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 xml 0m 1s The patch has no ill-formed XML file. -1 findbugs 0m 31s hadoop-common-project/hadoop-kms generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 0m 12s the patch passed +1 unit 2m 12s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 30m 16s Reason Tests FindBugs module:hadoop-common-project/hadoop-kms   Unwritten field:KMSAudit.java: [line 200] Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823644/HADOOP-13396.06.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle uname Linux b99f6546a660 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 / d677b68 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10244/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10244/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-kms.html Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10244/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10244/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Hi Xiao Chen thanks again for the new patch.

          1.

          auditLog.info(new String(output.toByteArray(), "UTF-8"));
          

          can be changed to

          auditLog.info(output.toString("UTF-8"));
          

          2. KMSAudit#initializeAuditLoggers I think it's fine to keep it as is. But if we ever want to add new type of audit loggers, I would suggest to change this to a factory method. I don't feel strongly about this, so this is just a note.

          Other than this, I think this patch is ready.

          Show
          jojochuang Wei-Chiu Chuang added a comment - Hi Xiao Chen thanks again for the new patch. 1. auditLog.info( new String (output.toByteArray(), "UTF-8" )); can be changed to auditLog.info(output.toString( "UTF-8" )); 2. KMSAudit#initializeAuditLoggers I think it's fine to keep it as is. But if we ever want to add new type of audit loggers, I would suggest to change this to a factory method. I don't feel strongly about this, so this is just a note. Other than this, I think this patch is ready.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Xiao, thanks for working on this. Had some review comments:

          • Is KMSAuditLogger interface really InterfaceAudience.Public? i.e. do we allow users to code against this interface by providing their own audit logger implementations? I'm guessing the answer is no, since we do not use reflection to instantiate the logger. We might still consider using the classname as configuration though, in case we want to add support for user-provided loggers later. In this case, I'd recommend splitting out each audit logger implementation as a separate class.
          • Could we add a big warning about the importance of audit logger output compatibility to KMSAuditLogger's class javadoc? We could use similar reminders in the logger implementations. One difference is that since we output a JSON dictionary, there are no guarantees about the ordering of the KV pairs.
          • kms-site.xml description should say how this takes a comma-separated list. Are multiple audit loggers unit tested? What happens if the same value (e.g. "simple") is specified multiple times?
          • KMSAudit#error, we added logging the URL, but used a different capitalization than #unauthenticated. It's also somewhat inconsistent that we put URL after the Exception while #unauthenticated puts it before the ErrorMsg. Not sure if we can change this one though. On the whole I don't think we should be making this possibly incompatible change in this JIRA, could you split it out so we can discuss separately?
          • The multiple uses of System.currentTimeMillis() is suspect. It means with multiple audit loggers, they could have different times. A similar issue can also happen within a single call to the JSON logger's logAuditEvent.
          • I think it's confusing how there's a logAuditSimpleFormat in the JSON logger. The term is now overloaded since we use "simple" to configure the current audit logger. So, we should rename one or the other.
          • Could we make an effort to use the same key names as the current audit logger? e.g. "op" instead of "operation", "user" instead of "username". This will make life easier for consumers.
          • Could you provide a small snippet of what the JSON output and textual output looks like for the same events? Hopefully we can get a quick gut check from Allen Wittenauer.
          Show
          andrew.wang Andrew Wang added a comment - Hi Xiao, thanks for working on this. Had some review comments: Is KMSAuditLogger interface really InterfaceAudience.Public? i.e. do we allow users to code against this interface by providing their own audit logger implementations? I'm guessing the answer is no, since we do not use reflection to instantiate the logger. We might still consider using the classname as configuration though, in case we want to add support for user-provided loggers later. In this case, I'd recommend splitting out each audit logger implementation as a separate class. Could we add a big warning about the importance of audit logger output compatibility to KMSAuditLogger's class javadoc? We could use similar reminders in the logger implementations. One difference is that since we output a JSON dictionary, there are no guarantees about the ordering of the KV pairs. kms-site.xml description should say how this takes a comma-separated list. Are multiple audit loggers unit tested? What happens if the same value (e.g. "simple") is specified multiple times? KMSAudit#error, we added logging the URL, but used a different capitalization than #unauthenticated . It's also somewhat inconsistent that we put URL after the Exception while #unauthenticated puts it before the ErrorMsg. Not sure if we can change this one though. On the whole I don't think we should be making this possibly incompatible change in this JIRA, could you split it out so we can discuss separately? The multiple uses of System.currentTimeMillis() is suspect. It means with multiple audit loggers, they could have different times. A similar issue can also happen within a single call to the JSON logger's logAuditEvent. I think it's confusing how there's a logAuditSimpleFormat in the JSON logger. The term is now overloaded since we use "simple" to configure the current audit logger. So, we should rename one or the other. Could we make an effort to use the same key names as the current audit logger? e.g. "op" instead of "operation", "user" instead of "username". This will make life easier for consumers. Could you provide a small snippet of what the JSON output and textual output looks like for the same events? Hopefully we can get a quick gut check from Allen Wittenauer .
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Wei-Chiu and Andrew for the great reviews!!
          I will need more time to come back with the comments, but here's a sample output, pending to roll back the URL changes. (Getting from running the testAuditLogFormat tests from both, which is exactly the same as what it would show in actual audit log files.)

          Text:

          OK[op=GENERATE_EEK, key=k4, user=luser, accessCount=1, interval=1ms] testmsg
          OK[op=GENERATE_EEK, user=luser] testmsg
          OK[op=GENERATE_EEK, key=k4, user=luser, accessCount=1, interval=5ms] testmsg
          UNAUTHORIZED[op=DECRYPT_EEK, key=k4, user=luser] 
          ERROR[user=luser] Method:'method' Exception:'testmsg' url:'url'
          UNAUTHENTICATED RemoteHost:remotehost Method:method URL:url ErrorMsg:'testmsg'
          

          Json:

          {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"GENERATE_EEK","eventTime":1471583567510,"allowed":true,"result":"OK","accessCount":"1","extraMessage":"testmsg","interval":"2","key":"k4"}
           {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"GENERATE_EEK","eventTime":1471583567538,"allowed":true,"result":"OK","extraMessage":"testmsg"}
           {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"GENERATE_EEK","eventTime":1471583568543,"allowed":true,"result":"OK","accessCount":"1","extraMessage":"testmsg","interval":"1035","key":"k4"}
           {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"DECRYPT_EEK","eventTime":1471583568544,"allowed":false,"result":"UNAUTHORIZED","extraMessage":"","key":"k4"}
           {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"Unknown","eventTime":1471583568544,"allowed":false,"result":"ERROR","extraMessage":"Method:'method' Exception:'testmsg' url:'url'"}
           {"username":"null!","impersonator":"null!","ipAddress":"remotehost","operation":"Unknown","eventTime":1471583568545,"allowed":false,"result":"UNAUTHENTICATED","extraMessage":"RemoteHost:remotehost Method:method URL:url ErrorMsg:'testmsg'"}
          
          Show
          xiaochen Xiao Chen added a comment - Thanks Wei-Chiu and Andrew for the great reviews!! I will need more time to come back with the comments, but here's a sample output, pending to roll back the URL changes. (Getting from running the testAuditLogFormat tests from both, which is exactly the same as what it would show in actual audit log files.) Text: OK[op=GENERATE_EEK, key=k4, user=luser, accessCount=1, interval=1ms] testmsg OK[op=GENERATE_EEK, user=luser] testmsg OK[op=GENERATE_EEK, key=k4, user=luser, accessCount=1, interval=5ms] testmsg UNAUTHORIZED[op=DECRYPT_EEK, key=k4, user=luser] ERROR[user=luser] Method:'method' Exception:'testmsg' url:'url' UNAUTHENTICATED RemoteHost:remotehost Method:method URL:url ErrorMsg:'testmsg' Json: {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"GENERATE_EEK","eventTime":1471583567510,"allowed":true,"result":"OK","accessCount":"1","extraMessage":"testmsg","interval":"2","key":"k4"} {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"GENERATE_EEK","eventTime":1471583567538,"allowed":true,"result":"OK","extraMessage":"testmsg"} {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"GENERATE_EEK","eventTime":1471583568543,"allowed":true,"result":"OK","accessCount":"1","extraMessage":"testmsg","interval":"1035","key":"k4"} {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"DECRYPT_EEK","eventTime":1471583568544,"allowed":false,"result":"UNAUTHORIZED","extraMessage":"","key":"k4"} {"username":"luser","impersonator":"null!","ipAddress":"Unknown","operation":"Unknown","eventTime":1471583568544,"allowed":false,"result":"ERROR","extraMessage":"Method:'method' Exception:'testmsg' url:'url'"} {"username":"null!","impersonator":"null!","ipAddress":"remotehost","operation":"Unknown","eventTime":1471583568545,"allowed":false,"result":"UNAUTHENTICATED","extraMessage":"RemoteHost:remotehost Method:method URL:url ErrorMsg:'testmsg'"}
          Hide
          xiaochen Xiao Chen added a comment -

          As you can see, the text format audit log really has minimum information .... (shrug), but I understand compat is compat.

          Show
          xiaochen Xiao Chen added a comment - As you can see, the text format audit log really has minimum information .... (shrug), but I understand compat is compat.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks again for the comments, Andrew and Wei-Chiu!
          As chatted offline, I'd like to make this patch simple, to solely refactor the kmsaudit logger to be pluggable. Will create a new jira for the Json addition. Patch 7 is toward this direction.

          I will address all json related comments in the new jira, this leaves us with:

          Is KMSAuditLogger interface really InterfaceAudience.Public?

          Changed it to Private.

          We might still consider using the classname as configuration though.... I'd recommend splitting out each audit logger implementation as a separate class.

          Separated the classes, with initialization done by reflection.

          Could we add a big warning about the importance of audit logger output compatibility to KMSAuditLogger's class javadoc? We could use similar reminders in the logger implementations.

          I added warning notes I feel pretty serious, to both the interface's and implementation's header.

          kms-site.xml description should say how this takes a comma-separated list.

          done.

          Are multiple audit loggers unit tested?

          Manually tested, I think they're separate enough to be tested on their own. Feel free to speak out what testing is in your thoughts though, we could add.

          What happens if the same value (e.g. "simple") is specified multiple times?

          Configuration#getTrimmedStringCollection calls into StringUtils#getTrimmedStringCollection which removes duplicates. Added a comment in KMSAudit for this.

          adding url in the message, and it being inconsistent

          I reverted any change to the message. I plan to see what the recent (audit) log compatibility thread concludes, and take actions based on that. Will assume nothing can be changed for now.

          The multiple uses of System.currentTimeMillis() is suspect. It means with multiple audit loggers, they could have different times.

          Good catch, added an endTime field on the event to address.

          Could we make an effort to use the same key names as the current audit logger? e.g. "op" instead of "operation", "user" instead of "username". This will make life easier for consumers.

          This is for the json logger right? I'll defer this to the new jira.

          Show
          xiaochen Xiao Chen added a comment - Thanks again for the comments, Andrew and Wei-Chiu! As chatted offline, I'd like to make this patch simple, to solely refactor the kmsaudit logger to be pluggable. Will create a new jira for the Json addition. Patch 7 is toward this direction. I will address all json related comments in the new jira, this leaves us with: Is KMSAuditLogger interface really InterfaceAudience.Public? Changed it to Private. We might still consider using the classname as configuration though.... I'd recommend splitting out each audit logger implementation as a separate class. Separated the classes, with initialization done by reflection. Could we add a big warning about the importance of audit logger output compatibility to KMSAuditLogger's class javadoc? We could use similar reminders in the logger implementations. I added warning notes I feel pretty serious, to both the interface's and implementation's header. kms-site.xml description should say how this takes a comma-separated list. done. Are multiple audit loggers unit tested? Manually tested, I think they're separate enough to be tested on their own. Feel free to speak out what testing is in your thoughts though, we could add. What happens if the same value (e.g. "simple") is specified multiple times? Configuration#getTrimmedStringCollection calls into StringUtils#getTrimmedStringCollection which removes duplicates. Added a comment in KMSAudit for this. adding url in the message, and it being inconsistent I reverted any change to the message. I plan to see what the recent (audit) log compatibility thread concludes, and take actions based on that. Will assume nothing can be changed for now. The multiple uses of System.currentTimeMillis() is suspect. It means with multiple audit loggers, they could have different times. Good catch, added an endTime field on the event to address. Could we make an effort to use the same key names as the current audit logger? e.g. "op" instead of "operation", "user" instead of "username". This will make life easier for consumers. This is for the json logger right? I'll defer this to the new jira.
          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 2 new or modified test files.
          +1 mvninstall 6m 37s trunk passed
          +1 compile 6m 49s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 19s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 21s trunk passed
          +1 javadoc 0m 12s trunk passed
          +1 mvninstall 0m 15s the patch passed
          +1 compile 6m 43s the patch passed
          +1 javac 6m 43s the patch passed
          -0 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 44 new + 21 unchanged - 4 fixed = 65 total (was 25)
          +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 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 0m 29s the patch passed
          +1 javadoc 0m 12s the patch passed
          +1 unit 2m 6s hadoop-kms in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          27m 18s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824662/HADOOP-13396.07.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle
          uname Linux 585f219e5be2 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 / 723facf
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10317/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10317/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10317/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 2 new or modified test files. +1 mvninstall 6m 37s trunk passed +1 compile 6m 49s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 21s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 15s the patch passed +1 compile 6m 43s the patch passed +1 javac 6m 43s the patch passed -0 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 44 new + 21 unchanged - 4 fixed = 65 total (was 25) +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 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 0m 29s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 6s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 27m 18s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824662/HADOOP-13396.07.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle uname Linux 585f219e5be2 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 / 723facf Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10317/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10317/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10317/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Patch 8 to fix the styles and added a unit test for the reflection initialization.

          Show
          xiaochen Xiao Chen added a comment - Patch 8 to fix the styles and added a unit test for the reflection initialization.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 6m 39s trunk passed
          +1 compile 6m 44s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 18s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 22s trunk passed
          +1 javadoc 0m 12s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 6m 39s the patch passed
          +1 javac 6m 39s the patch passed
          -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 2 new + 21 unchanged - 4 fixed = 23 total (was 25)
          +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 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 0m 29s the patch passed
          +1 javadoc 0m 12s the patch passed
          +1 unit 2m 6s hadoop-kms in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          27m 9s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824895/HADOOP-13396.08.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle
          uname Linux d7b18cb94a2c 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 / 115ecb5
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10333/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10333/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10333/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 6m 39s trunk passed +1 compile 6m 44s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 22s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 6m 39s the patch passed +1 javac 6m 39s the patch passed -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 2 new + 21 unchanged - 4 fixed = 23 total (was 25) +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 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 0m 29s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 6s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 27m 9s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12824895/HADOOP-13396.08.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle uname Linux d7b18cb94a2c 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 / 115ecb5 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10333/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10333/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10333/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          Splitting the patch sounds good to me. Few more comments:

          • Still some lingering JSON references in the two log4j properties files
          • Can we make the default value of the config key the classname, rather than empty? This way users have an example.
          • Should abort if a specified audit logger cannot be configured. Remember that the audit logger is important for security, so we don't want to accidentally not log in the case of misconfiguration.
          • I really like that comment on SimpleKMSAuditLogger. One grammar nit, "and will be haunted by the consumer tools / developers", maybe you meant "will haunt consumer tools / developers" ?
          Show
          andrew.wang Andrew Wang added a comment - Splitting the patch sounds good to me. Few more comments: Still some lingering JSON references in the two log4j properties files Can we make the default value of the config key the classname, rather than empty? This way users have an example. Should abort if a specified audit logger cannot be configured. Remember that the audit logger is important for security, so we don't want to accidentally not log in the case of misconfiguration. I really like that comment on SimpleKMSAuditLogger. One grammar nit, "and will be haunted by the consumer tools / developers", maybe you meant "will haunt consumer tools / developers" ?
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Andrew for the continued reviews.

          Patch 9 addressed all comments, and updated the logger loading/initialization to fail on any failures. IMO ideally we should ExitUtils#terminate it, but not sure how to unit test that though. Throwing a RTE is more consistent with current kms code...

          Also, for the grammar issue, I meant to say 'if you modify log format, downstream people will haunt you'. But updated with your suggestion.

          Show
          xiaochen Xiao Chen added a comment - Thanks Andrew for the continued reviews. Patch 9 addressed all comments, and updated the logger loading/initialization to fail on any failures. IMO ideally we should ExitUtils#terminate it, but not sure how to unit test that though. Throwing a RTE is more consistent with current kms code... Also, for the grammar issue, I meant to say 'if you modify log format, downstream people will haunt you'. But updated with your suggestion.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 6m 38s trunk passed
          +1 compile 6m 50s trunk passed
          +1 checkstyle 0m 13s trunk passed
          +1 mvnsite 0m 18s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 22s trunk passed
          +1 javadoc 0m 12s trunk passed
          +1 mvninstall 0m 15s the patch passed
          +1 compile 6m 45s the patch passed
          +1 javac 6m 45s the patch passed
          +1 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 0 new + 21 unchanged - 4 fixed = 21 total (was 25)
          +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 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 0m 29s the patch passed
          +1 javadoc 0m 12s the patch passed
          +1 unit 2m 7s hadoop-kms in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          27m 19s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12825080/HADOOP-13396.09.patch
          JIRA Issue HADOOP-13396
          Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle
          uname Linux a9cd18342c16 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 / 8aae8d6
          Default Java 1.8.0_101
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10345/testReport/
          modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10345/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 6m 38s trunk passed +1 compile 6m 50s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 22s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 15s the patch passed +1 compile 6m 45s the patch passed +1 javac 6m 45s the patch passed +1 checkstyle 0m 13s hadoop-common-project/hadoop-kms: The patch generated 0 new + 21 unchanged - 4 fixed = 21 total (was 25) +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 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 0m 29s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 7s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 27m 19s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12825080/HADOOP-13396.09.patch JIRA Issue HADOOP-13396 Optional Tests asflicense mvnsite unit xml compile javac javadoc mvninstall findbugs checkstyle uname Linux a9cd18342c16 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 / 8aae8d6 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10345/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10345/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          LGTM +1, thanks Xiao. Looking forward to your JSON logger followup.

          Show
          andrew.wang Andrew Wang added a comment - LGTM +1, thanks Xiao. Looking forward to your JSON logger followup.
          Hide
          xiaochen Xiao Chen added a comment -

          I have committed this to trunk and branch-2. There're trivial conflicts backporting to branch-2 due to HADOOP-12615. Compiled and passed TestKMSAudit before pushing.

          Thanks Allen, Sean, Wei-Chiu and Andrew for the discussions and reviews! Json logger will be done via HADOOP-13523.

          Show
          xiaochen Xiao Chen added a comment - I have committed this to trunk and branch-2. There're trivial conflicts backporting to branch-2 due to HADOOP-12615 . Compiled and passed TestKMSAudit before pushing. Thanks Allen, Sean, Wei-Chiu and Andrew for the discussions and reviews! Json logger will be done via HADOOP-13523 .
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10338 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10338/)
          HADOOP-13396. Allow pluggable audit loggers in KMS. Contributed by Xiao (xiao: rev 3476156807733505746951f0c9346592742bbbb2)

          • (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java
          • (add) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAuditLogger.java
          • (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSWebApp.java
          • (add) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/SimpleKMSAuditLogger.java
          • (edit) hadoop-common-project/hadoop-kms/src/test/resources/log4j-kmsaudit.properties
          • (edit) hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
          • (edit) hadoop-common-project/hadoop-kms/src/main/conf/kms-site.xml
          • (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10338 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10338/ ) HADOOP-13396 . Allow pluggable audit loggers in KMS. Contributed by Xiao (xiao: rev 3476156807733505746951f0c9346592742bbbb2) (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java (add) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAuditLogger.java (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSWebApp.java (add) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/SimpleKMSAuditLogger.java (edit) hadoop-common-project/hadoop-kms/src/test/resources/log4j-kmsaudit.properties (edit) hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java (edit) hadoop-common-project/hadoop-kms/src/main/conf/kms-site.xml (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java

            People

            • Assignee:
              xiaochen Xiao Chen
              Reporter:
              xiaochen Xiao Chen
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development