[GitHub] [kafka] curie71 commented on a diff in pull request #13090: KAFKA-14605 Change the log level to info when logIfAllowed is set, warn when logIfDenied is set.

[GitBox <gi...@apache.org>]

curie71 commented on code in PR #13090:
URL: https://github.com/apache/kafka/pull/13090#discussion_r1070537837


##########
metadata/src/main/java/org/apache/kafka/metadata/authorizer/StandardAuthorizerData.java:
##########
@@ -329,7 +329,7 @@ private void logAuditMessage(
                 // authorized operations or a filter (e.g for regex subscriptions) to filter out authorized resources.
                 // In this case, log only at trace level.
                 if (action.logIfDenied()) {
-                    auditLog.info(buildAuditMessage(principal, requestContext, action, rule));
+                    auditLog.warn(buildAuditMessage(principal, requestContext, action, rule));

Review Comment:
   Thanks for your reply!
   
   Firstly, audit log is **security log**, not ~debug log~. Security log should be enabled by default during production such as fatal, error, warn, info verbosity levels. But now, we only enable **deny** log by default in KAFKA.
   ```java
   case DENIED:
          if (action.logIfDenied()) {
                 auditLog.info(......); // deny log at verbose level
           } else if (auditLog.isTraceEnabled()) {
                 auditLog.trace(buildAuditMessage(principal, requestContext, action, rule)); // debug log for deny
           }
   ```
   
   Secondly, the debug level is close to trace level, the `if (action.logIfAllowed)` branch is **redundant**. 
   ```java
   case ALLOWED:
           if (action.logIfAllowed() && auditLog.isDebugEnabled()) { // redundant branch
                 auditLog.debug(......); // debug log 
           } else if (auditLog.isTraceEnabled()) {
                 auditLog.trace(buildAuditMessage(principal, requestContext, action, rule)); // debug log again
           }
   ```
   
   In my opinion, if we don' t think "allow log" is as important as "deny log", we should **delete** the redundant branch here. 
   
   Or we could log allow at **info** level with a switch `action.logIfAllowed = false/true` for performance consideration.
   ```java
       private void logAuditMessage(
           ...... ) {
           switch (rule.result()) {
               case ALLOWED:
                   if (action.logIfAllowed() && auditLog.isDebugEnabled()) {
                       auditLog.debug(......); // info maybe better
                   } else if (auditLog.isTraceEnabled()) {
                       auditLog.trace(buildAuditMessage(principal, requestContext, action, rule));
                   }
                   return;
   
               case DENIED:
                   if (action.logIfDenied()) {
                       auditLog.info(......); // warn maybe better
                   } else if (auditLog.isTraceEnabled()) {
                       auditLog.trace(buildAuditMessage(principal, requestContext, action, rule));
                   }
           }
       }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org