Mailing-List: contact common-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: common-issues@hadoop.apache.org
Date: Thu, 24 Jul 2014 16:50:39 +0000 (UTC)
From: "Alejandro Abdelnur (JIRA)" <jira@apache.org>
To: common-issues@hadoop.apache.org
Message-ID: <JIRA.12723989.1403821032467.38509.1406220639869@arcas>
In-Reply-To: <JIRA.12723989.1403821032467@arcas>
References: <JIRA.12723989.1403821032467@arcas>
Subject: [jira] [Commented] (HADOOP-10756) KMS audit log should consolidate
 successful similar requests
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable


    [ https://issues.apache.org/jira/browse/HADOOP-10756?page=3Dcom.atlassi=
an.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=3D14=
073375#comment-14073375 ]=20

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 sh=
ould be set in the {{MDC}} within the {{op()}} method instead in the 2 call=
er 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 e=
nd (if something fails after construction or early init, you don=E2=80=99t =
leave a dangling thread running.
* for testings, you should have a method to stop the executor to get rid of=
 the executor=E2=80=99s thread.
* default delay should be the same that in the kms-log4j.properties, 10000 =
(currently is 5000)
* {{append()}} method, I don=E2=80=99t think we need to check {{mdcStatus}}=
 is an AuditOptStatus instance, we can safely assume it is.
* {{append()}} you can directly use {{status =3D=3D AuditOpStatus.UNAUTHORI=
ZED}}, the beauty of enum.
* {{append()}}, why we don=E2=80=99t leverage the Cache entry creation logi=
c on get here, then we can avoid the race condition of creating/putting 2 e=
vents. having the {{accessCount}} set to -1 on creation would be the indica=
tor 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=E2=80=
=99s property setting and calling {{super.append()}}, and using it from {{h=
andleUnAuthorizedAccess()}} & {{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 w=
ired. 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 Delay=
ed Event from the cache. This will make the next authorized access log to f=
lush with accessCount 1 immediately. I think we should instead:

* On expiry/unauthorized, if accessCount is greater than 0, flush to log, s=
et 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 ti=
me. We could also log the time interval of the aggregated count (currentTim=
e - (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-1=
0756.3.patch, HADOOP-10756.4.patch, HADOOP-10756.5.patch
>
>
> Every rejected access should be audited, but successful accesses should b=
e consolidated within a given amount of time if the request is from the sam=
e user for he same key.=20


--
This message was sent by Atlassian JIRA
(v6.2#6252)