You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/04/13 13:51:10 UTC
[GitHub] [ozone] adoroszlai commented on a diff in pull request #3289: HDDS-6562. Exclude specific operations in Audit log
adoroszlai commented on code in PR #3289:
URL: https://github.com/apache/ozone/pull/3289#discussion_r849410323
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -3062,4 +3062,34 @@
will create intermediate directories.
</description>
</property>
+
+ <property>
+ <name>ozone.audit.log.debug.cmd.list.omaudit</name>
+ <value></value>
+ <tag>OM</tag>
+ <description>
+ A comma separated list of OzoneManager commands that are written to the OzoneManager audit logs only if the audit
+ log level is debug. Ex: "ALLOCATE_BLOCK,ALLOCATE_KEY,COMMIT_KEY".
+ </description>
+ </property>
+
+ <property>
+ <name>ozone.audit.log.debug.cmd.list.scmaudit</name>
+ <value></value>
+ <tag>SCM</tag>
+ <description>
+ A comma separated list of OzoneManager commands that are written to the SCM audit logs only if the audit
Review Comment:
```suggestion
A comma separated list of SCM commands that are written to the SCM audit logs only if the audit
```
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -3062,4 +3062,34 @@
will create intermediate directories.
</description>
</property>
+
+ <property>
+ <name>ozone.audit.log.debug.cmd.list.omaudit</name>
+ <value></value>
+ <tag>OM</tag>
+ <description>
+ A comma separated list of OzoneManager commands that are written to the OzoneManager audit logs only if the audit
+ log level is debug. Ex: "ALLOCATE_BLOCK,ALLOCATE_KEY,COMMIT_KEY".
+ </description>
+ </property>
+
+ <property>
+ <name>ozone.audit.log.debug.cmd.list.scmaudit</name>
+ <value></value>
+ <tag>SCM</tag>
+ <description>
+ A comma separated list of OzoneManager commands that are written to the SCM audit logs only if the audit
+ log level is debug. Ex: "GET_VERSION,REGISTER,SEND_HEARTBEAT".
+ </description>
+ </property>
+
+ <property>
+ <name>ozone.audit.log.debug.cmd.list.dnaudit</name>
+ <value></value>
+ <tag>DN</tag>
+ <description>
+ A comma separated list of OzoneManager commands that are written to the DN audit logs only if the audit
Review Comment:
```suggestion
A comma separated list of Datanode commands that are written to the DN audit logs only if the audit
```
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java:
##########
@@ -152,6 +160,19 @@ public void notLogReadEvents() throws IOException {
verifyNoLog();
}
+ /**
+ * Test to verify no WRITE event is logged.
+ */
+ @Test
+ public void notLogWriteEvents() throws IOException {
Review Comment:
I think `notLogExcludedEvent` or `excludedEventNotLogged` would better reflect the purpose of the test.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/audit/AuditLogger.java:
##########
@@ -18,21 +18,38 @@
package org.apache.hadoop.ozone.audit;
import com.google.common.annotations.VisibleForTesting;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Marker;
import org.apache.logging.log4j.spi.ExtendedLogger;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
/**
* Class to define Audit Logger for Ozone.
*/
public class AuditLogger {
+ private static final Logger LOG = LoggerFactory.getLogger(AuditLogger.class);
+
private ExtendedLogger logger;
private static final String FQCN = AuditLogger.class.getName();
private static final Marker WRITE_MARKER = AuditMarker.WRITE.getMarker();
private static final Marker READ_MARKER = AuditMarker.READ.getMarker();
+ private AtomicReference<Set<String>> debugCmdSetRef =
Review Comment:
```suggestion
private final AtomicReference<Set<String>> debugCmdSetRef =
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/audit/AuditLogger.java:
##########
@@ -56,7 +75,11 @@ public ExtendedLogger getLogger() {
}
public void logWriteSuccess(AuditMessage msg) {
- this.logger.logIfEnabled(FQCN, Level.INFO, WRITE_MARKER, msg, null);
+ if (debugCmdSetRef.get().contains(msg.getOp().toLowerCase(Locale.ROOT))) {
Review Comment:
Please extract a method (e.g. `shouldLogAtDebug(msg)`) for this condition:
```
debugCmdSetRef.get().contains(msg.getOp().toLowerCase(Locale.ROOT))
```
to avoid duplication and make these methods easier to read.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/audit/AuditLogger.java:
##########
@@ -75,12 +102,31 @@ public void logReadFailure(AuditMessage msg) {
public void logWrite(AuditMessage auditMessage) {
if (auditMessage.getThrowable() == null) {
- this.logger.logIfEnabled(FQCN, Level.INFO, WRITE_MARKER, auditMessage,
- auditMessage.getThrowable());
+ if (debugCmdSetRef.get().contains(
+ auditMessage.getOp().toLowerCase(Locale.ROOT))) {
+ this.logger.logIfEnabled(FQCN, Level.DEBUG, WRITE_MARKER, auditMessage,
+ auditMessage.getThrowable());
+ } else {
+ this.logger.logIfEnabled(FQCN, Level.INFO, WRITE_MARKER, auditMessage,
+ auditMessage.getThrowable());
+ }
} else {
this.logger.logIfEnabled(FQCN, Level.ERROR, WRITE_MARKER, auditMessage,
auditMessage.getThrowable());
Review Comment:
and `logWriteFailure(auditMessage)` here.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/audit/AuditLogger.java:
##########
@@ -75,12 +102,31 @@ public void logReadFailure(AuditMessage msg) {
public void logWrite(AuditMessage auditMessage) {
if (auditMessage.getThrowable() == null) {
- this.logger.logIfEnabled(FQCN, Level.INFO, WRITE_MARKER, auditMessage,
- auditMessage.getThrowable());
+ if (debugCmdSetRef.get().contains(
+ auditMessage.getOp().toLowerCase(Locale.ROOT))) {
+ this.logger.logIfEnabled(FQCN, Level.DEBUG, WRITE_MARKER, auditMessage,
+ auditMessage.getThrowable());
+ } else {
+ this.logger.logIfEnabled(FQCN, Level.INFO, WRITE_MARKER, auditMessage,
+ auditMessage.getThrowable());
+ }
Review Comment:
Maybe I'm missing something, but I think we could call `logWriteSuccess(auditMessage)` here.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org