[
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: [email protected]
For additional commands, e-mail: [email protected]
