[jira] [Comment Edited] (HDFS-11246) FSNameSystem#logAuditEvent should be called outside the read or write locks during operations like getContentSummary

[Brahma Reddy Battula (JIRA)]

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

Brahma Reddy Battula edited comment on HDFS-11246 at 7/19/17 2:09 PM:
----------------------------------------------------------------------

[~kshukla] thanks for working on this.

bq.That would mean we log for IOExceptions and others as well with 
allowed=false, which should ideally only be when we see an 
AccessControlException. This was a bug fixed thru HDFS-9395. We can still have 
the call in the finally block while making sure we don't log unconditionally.

 Instead of double try-finally blocks can we  log in finally block based on 
some boolean condition like below such that it can be logged only on ACE..?

*Option 1:*
{code}
       final String operationName = "setOwner";
-    FileStatus auditStat;
+    boolean logAudit = false;
+    boolean allow = true;
+    FileStatus auditStat=null;
     checkOperation(OperationCategory.WRITE);
     writeLock();
     try {
       checkOperation(OperationCategory.WRITE);
       checkNameNodeSafeMode("Cannot set owner for " + src);
       auditStat = FSDirAttrOp.setOwner(dir, src, username, group);
+      logAudit = true;
     } catch (AccessControlException e) {
-      logAuditEvent(false, operationName, src);
+      logAudit = true;
+      allow = false;
+
       throw e;
     } finally {
       writeUnlock(operationName);
+      if(logAudit)
+        logAuditEvent(allow, operationName, src, null, auditStat);
     }
     getEditLog().logSync();
-    logAuditEvent(true, operationName, src, null, auditStat);
{code}
*Option 2:*
{code}
+  public enum AuditCheck {
+    SUCCESS, FAILURE, NONE
+  }
   /**
    * Set owner for an existing file.
    * @throws IOException
@@ -1807,21 +1810,25 @@ void setPermission(String src, FsPermission permission) 
throws IOException {
   void setOwner(String src, String username, String group)
       throws IOException {
     final String operationName = "setOwner";
-    FileStatus auditStat;
+    AuditCheck auditCheck = AuditCheck.NONE;
+    FileStatus auditStat = null;
     checkOperation(OperationCategory.WRITE);
     writeLock();
     try {
       checkOperation(OperationCategory.WRITE);
       checkNameNodeSafeMode("Cannot set owner for " + src);
       auditStat = FSDirAttrOp.setOwner(dir, src, username, group);
+      auditCheck = AuditCheck.SUCCESS;
     } catch (AccessControlException e) {
-      logAuditEvent(false, operationName, src);
+      auditCheck = AuditCheck.FAILURE;
       throw e;
     } finally {
       writeUnlock(operationName);
+      if (auditCheck != AuditCheck.NONE)
+        logAuditEvent(auditCheck == AuditCheck.SUCCESS, operationName, src,
+            null, auditStat);
     }
     getEditLog().logSync();
-    logAuditEvent(true, operationName, src, null, auditStat);
{code}

I prefer *option-2.*


was (Author: brahmareddy):
[~kshukla] thanks for working on this.

bq.That would mean we log for IOExceptions and others as well with 
allowed=false, which should ideally only be when we see an 
AccessControlException. This was a bug fixed thru HDFS-9395. We can still have 
the call in the finally block while making sure we don't log unconditionally.

 Instead of double try-finally blocks can we  log in finally block based on 
some boolean condition like below such that it can be logged only on ACE..?

{code}
       final String operationName = "setOwner";
-    FileStatus auditStat;
+    boolean logAudit = false;
+    boolean allow = true;
+    FileStatus auditStat=null;
     checkOperation(OperationCategory.WRITE);
     writeLock();
     try {
       checkOperation(OperationCategory.WRITE);
       checkNameNodeSafeMode("Cannot set owner for " + src);
       auditStat = FSDirAttrOp.setOwner(dir, src, username, group);
+      logAudit = true;
     } catch (AccessControlException e) {
-      logAuditEvent(false, operationName, src);
+      logAudit = true;
+      allow = false;
+
       throw e;
     } finally {
       writeUnlock(operationName);
+      if(logAudit)
+        logAuditEvent(allow, operationName, src, null, auditStat);
     }
     getEditLog().logSync();
-    logAuditEvent(true, operationName, src, null, auditStat);
{code}

{code}

> FSNameSystem#logAuditEvent should be called outside the read or write locks 
> during operations like getContentSummary
> --------------------------------------------------------------------------------------------------------------------
>
>                 Key: HDFS-11246
>                 URL: https://issues.apache.org/jira/browse/HDFS-11246
>             Project: Hadoop HDFS
>          Issue Type: Bug
>    Affects Versions: 2.7.3
>            Reporter: Kuhu Shukla
>            Assignee: Kuhu Shukla
>         Attachments: HDFS-11246.001.patch, HDFS-11246.002.patch
>
>
> {code}
>     readLock();
>     boolean success = true;
>     ContentSummary cs;
>     try {
>       checkOperation(OperationCategory.READ);
>       cs = FSDirStatAndListingOp.getContentSummary(dir, src);
>     } catch (AccessControlException ace) {
>       success = false;
>       logAuditEvent(success, operationName, src);
>       throw ace;
>     } finally {
>       readUnlock(operationName);
>     }
> {code}
> It would be nice to have audit logging outside the lock esp. in scenarios 
> where applications hammer a given operation several times. 



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org