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.
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.