hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wang (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-13396) Add json format audit logging to KMS
Date Thu, 18 Aug 2016 23:02:21 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-13396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15427328#comment-15427328
] 

Andrew Wang commented on HADOOP-13396:
--------------------------------------

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 [~aw].

> Add json format audit logging to KMS
> ------------------------------------
>
>                 Key: HADOOP-13396
>                 URL: https://issues.apache.org/jira/browse/HADOOP-13396
>             Project: Hadoop Common
>          Issue Type: New Feature
>          Components: kms
>            Reporter: Xiao Chen
>            Assignee: Xiao Chen
>         Attachments: HADOOP-13396.01.patch, HADOOP-13396.02.patch, HADOOP-13396.03.patch,
HADOOP-13396.04.patch, HADOOP-13396.05.patch, HADOOP-13396.06.patch
>
>
> 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.
> A json format log extension is added using the refactored API, and being turned off by
default.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message