[
https://issues.apache.org/jira/browse/HADOOP-10756?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14073375#comment-14073375
]
Alejandro Abdelnur commented on HADOOP-10756:
---------------------------------------------
Nice, I like it much better using Guava Cache.
*kms-log4j.properties*:
* name of appender should be 'aggregated-audit', 'delayed-audit' gives the
wrong impression.
*KMSAudit.java*:
* the {{op()}} method should receive as param the AuditOpStatus and that should
be set in the {{MDC}} within the {{op()}} method instead in the 2 caller
methods. Then all is in one place, it is more symmetric.
*KMSAuditAppender.java*:
* the {{AtomicInteger}} can be created in the constructor of {{DelayedEvent}}
* the {{executor}} should be created in the {{initialize()}}, at the very end
(if something fails after construction or early init, you don’t leave a
dangling thread running.
* for testings, you should have a method to stop the executor to get rid of the
executor’s thread.
* default delay should be the same that in the kms-log4j.properties, 10000
(currently is 5000)
* {{append()}} method, I don’t think we need to check {{mdcStatus}} is an
AuditOptStatus instance, we can safely assume it is.
* {{append()}} you can directly use {{status == AuditOpStatus.UNAUTHORIZED}},
the beauty of enum.
* {{append()}}, why we don’t leverage the Cache entry creation logic on get
here, then we can avoid the race condition of creating/putting 2 events. having
the {{accessCount}} set to -1 on creation would be the indicator that is new.
you would do an incrementAndGet, if you get zero it means it is first
occurrence (more on this a bit later)
* how about having a {{dispatchAppend()}} method doing all the event’s property
setting and calling {{super.append()}}, and using it from
{{handleUnAuthorizedAccess()}} & {{append()}}
*TestKMSAuditAppender.java*:
* we should, somehow, shutdown the appender executor (see comment above)
* the current test should be used to simply verify the append is properly
wired. you could use Mockito to easier test all code paths in the appender.
A couple of things we missed:
1. On unauthorized access and on cache expiration we are removing the Delayed
Event from the cache. This will make the next authorized access log to flush
with accessCount 1 immediately. I think we should instead:
* On expiry/unauthorized, if accessCount is greater than 0, flush to log, set
accessCount to 0, set entry in cache again.
* On expiry/unauthorized, if acccessCount is zero, remove from cache.
please check if this makes sense.
2. on flushing we should make sure we set the event time to the flushing time.
We could also log the time interval of the aggregated count (currentTime -
(timeOfFirstAccess or timeOfLastFlush)).
> KMS audit log should consolidate successful similar requests
> ------------------------------------------------------------
>
> Key: HADOOP-10756
> URL: https://issues.apache.org/jira/browse/HADOOP-10756
> Project: Hadoop Common
> Issue Type: Bug
> Components: security
> Affects Versions: 3.0.0
> Reporter: Alejandro Abdelnur
> Assignee: Arun Suresh
> Attachments: HADOOP-10756.1.patch, HADOOP-10756.2.patch,
> HADOOP-10756.3.patch, HADOOP-10756.4.patch, HADOOP-10756.5.patch
>
>
> Every rejected access should be audited, but successful accesses should be
> consolidated within a given amount of time if the request is from the same
> user for he same key.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
