This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new dcf2258749 [#11170] fix(audit): Fix file rotation, client IP capture,
and log format (#11191)
dcf2258749 is described below
commit dcf2258749bdcdb7820b17e46906a962e87b6a2e
Author: Jerry Shao <[email protected]>
AuthorDate: Mon May 25 14:30:07 2026 +0800
[#11170] fix(audit): Fix file rotation, client IP capture, and log format
(#11191)
## What changes were proposed in this pull request?
Fixes three audit log bugs from #11170:
**Problem 1 — FileAuditWriter has no file rotation, compression, or
retention**
- Rewrite `FileAuditWriter` to delegate all file management to Log4j2
via a
dedicated SLF4J logger named `gravitino.audit`
- Add `audit_file` rolling appender group to
`conf/log4j2.properties.template`:
256 MB or daily rotation, gzip compression, 30-day retention
- Deprecated writer properties (`fileName`, `append`,
`flushIntervalSecs`) now
emit a `WARN` log at startup and are otherwise ignored
- Update `gravitino-server-config.md` with migration guidance
**Problem 3 — remoteAddress() always returns "unknown" for Gravitino
server events**
- Add `RequestContext` ThreadLocal in `core/` to hold the client IP on
the servlet thread
- Read the value at `Event` construction time (not at listener
processing time),
so async listener threads safely read a `final` field instead of
ThreadLocal
- Add `RequestContextFilter` in `server/` to set/clear the value around
each
request, honoring `X-Forwarded-For` for reverse-proxy deployments
- Apply the same `X-Forwarded-For` logic to `IcebergRequestContext`
**Problem 4 — SimpleAuditLogV2 uses second-precision timestamps and
omits customInfo**
- Replace `new SimpleDateFormat(...)` per call with a static thread-safe
`DateTimeFormatter` at millisecond precision (`yyyy-MM-dd HH:mm:ss.SSS`)
- Append `customInfo()` as an 8th tab-separated field (empty string when
absent)
## How was this patch tested?
- `TestRequestContext` — ThreadLocal set/get/clear/thread-isolation
- `TestEventRemoteAddress` — value captured at construction time,
survives ThreadLocal clear
- `TestRequestContextFilter` — X-Forwarded-For (single/multi-entry),
fallback to `getRemoteAddr()`, cleanup in finally including on exception
- `TestSimpleAuditLogV2` — millisecond precision, 8-field output,
customInfo presence/absence
- `TestFileAuditWriter` — doWrite publishes to audit logger, deprecated
keys emit WARN, clean properties emit no WARN, close is no-op
- `TestAuditManager` — default writer/formatter types, batch dispatch
smoke test
---------
Co-authored-by: Claude Sonnet 4.6 <[email protected]>
---
conf/log4j2.properties.template | 29 ++++
.../apache/gravitino/audit/FileAuditWriter.java | 92 +++-------
.../gravitino/audit/v2/SimpleAuditLogV2.java | 16 +-
.../apache/gravitino/listener/api/event/Event.java | 25 ++-
.../org/apache/gravitino/utils/RequestContext.java | 62 +++++++
.../apache/gravitino/audit/TestAuditManager.java | 132 ++-------------
.../gravitino/audit/TestFileAuditWriter.java | 187 +++++++++++++++++++++
.../gravitino/audit/v2/TestSimpleAuditLogV2.java | 103 ++++++++++++
.../listener/api/event/TestEventRemoteAddress.java | 89 ++++++++++
.../apache/gravitino/utils/TestRequestContext.java | 70 ++++++++
docs/gravitino-server-config.md | 46 +++--
.../listener/api/event/IcebergRequestContext.java | 14 +-
.../gravitino/server/web/RequestContextFilter.java | 75 +++++++++
.../server/web/TestRequestContextFilter.java | 120 +++++++++++++
.../apache/gravitino/server/GravitinoServer.java | 2 +
15 files changed, 862 insertions(+), 200 deletions(-)
diff --git a/conf/log4j2.properties.template b/conf/log4j2.properties.template
index f0579d1265..ff27eb3e0b 100644
--- a/conf/log4j2.properties.template
+++ b/conf/log4j2.properties.template
@@ -72,6 +72,35 @@ logger.lineage.level = info
logger.lineage.appenderRef.lineage_file.ref = lineage_file
logger.lineage.additivity = false
+## use separate file for audit log
+appender.audit_file.type = RollingFile
+appender.audit_file.name = audit_file
+appender.audit_file.fileName = ${basePath}/gravitino_audit.log
+appender.audit_file.filePattern =
${basePath}/gravitino_audit_%d{yyyyMMdd}.%i.log.gz
+appender.audit_file.layout.type = PatternLayout
+appender.audit_file.layout.pattern = %msg%n
+appender.audit_file.policies.type = Policies
+appender.audit_file.policies.size.type = SizeBasedTriggeringPolicy
+appender.audit_file.policies.size.size = 256MB
+appender.audit_file.policies.time.type = TimeBasedTriggeringPolicy
+appender.audit_file.policies.time.interval = 1
+appender.audit_file.policies.time.modulate = true
+appender.audit_file.strategy.type = DefaultRolloverStrategy
+appender.audit_file.strategy.delete.type = Delete
+appender.audit_file.strategy.delete.basePath = ${basePath}
+appender.audit_file.strategy.delete.maxDepth = 10
+appender.audit_file.strategy.delete.ifAll.type = IfAll
+appender.audit_file.strategy.delete.ifAll.ifFileName.type = IfFileName
+appender.audit_file.strategy.delete.ifAll.ifFileName.regex =
gravitino_audit.*\.log\.gz
+# Delete all audit log files older than 30 days
+appender.audit_file.strategy.delete.ifAll.ifLastModified.type = IfLastModified
+appender.audit_file.strategy.delete.ifAll.ifLastModified.age = 30d
+
+logger.audit.name = gravitino.audit
+logger.audit.level = info
+logger.audit.appenderRef.audit_file.ref = audit_file
+logger.audit.additivity = false
+
logger.rest.name = org.apache.gravitino.server.web.rest
logger.rest.level = warn
logger.rest.appenderRef.rolling.ref = fileLogger
diff --git a/core/src/main/java/org/apache/gravitino/audit/FileAuditWriter.java
b/core/src/main/java/org/apache/gravitino/audit/FileAuditWriter.java
index a8cefea807..e1125a0049 100644
--- a/core/src/main/java/org/apache/gravitino/audit/FileAuditWriter.java
+++ b/core/src/main/java/org/apache/gravitino/audit/FileAuditWriter.java
@@ -19,37 +19,34 @@
package org.apache.gravitino.audit;
-import com.google.common.annotations.VisibleForTesting;
-import java.io.FileOutputStream;
-import java.io.OutputStream;
-import java.io.OutputStreamWriter;
-import java.io.Writer;
-import java.nio.charset.StandardCharsets;
-import java.time.Instant;
+import com.google.common.collect.ImmutableMap;
import java.util.Map;
-import org.apache.gravitino.exceptions.GravitinoRuntimeException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * DefaultFileAuditWriter is the default implementation of AuditLogWriter,
which writes audit logs
- * to a file.
+ * FileAuditWriter is the default implementation of {@link AuditLogWriter}. It
delegates all file
+ * management (rotation, compression, retention) to Log4j2 via a dedicated
logger named {@code
+ * gravitino.audit}. Configure the appender in {@code conf/log4j2.properties}.
*/
public class FileAuditWriter implements AuditLogWriter {
- private static final Logger Log =
LoggerFactory.getLogger(FileAuditWriter.class);
- private static final String AUDIT_LOG_FILE_NAME = "fileName";
- private static final String APPEND = "append";
- private static final String FLUSH_INTERVAL_SECS = "flushIntervalSecs";
- private static final String LINE_SEPARATOR = System.lineSeparator();
+ private static final Logger LOG =
LoggerFactory.getLogger(FileAuditWriter.class);
- @VisibleForTesting Writer outWriter;
- @VisibleForTesting String fileName;
+ /** Logger name that must match the {@code logger.audit.name} entry in
log4j2.properties. */
+ static final String AUDIT_LOGGER_NAME = "gravitino.audit";
+
+ private static final Map<String, String> DEPRECATED_KEYS =
+ ImmutableMap.of(
+ "fileName",
+ "configure appender.audit_file.fileName in log4j2.properties",
+ "append",
+ "configure appender.audit_file.append in log4j2.properties",
+ "flushIntervalSecs",
+ "configure immediateFlush or an async appender in
log4j2.properties");
private Formatter formatter;
- private boolean append;
- private int flushIntervalSecs;
- private Instant nextFlushTime = Instant.now();
+ private Logger auditLogger;
@Override
public Formatter getFormatter() {
@@ -59,63 +56,28 @@ public class FileAuditWriter implements AuditLogWriter {
@Override
public void init(Formatter formatter, Map<String, String> properties) {
this.formatter = formatter;
- this.fileName =
- System.getProperty("gravitino.log.path")
- + "/"
- + properties.getOrDefault(AUDIT_LOG_FILE_NAME,
"gravitino_audit.log");
- this.append = Boolean.parseBoolean(properties.getOrDefault(APPEND,
"true"));
- this.flushIntervalSecs =
Integer.parseInt(properties.getOrDefault(FLUSH_INTERVAL_SECS, "10"));
- try {
- OutputStream outputStream = new FileOutputStream(fileName, append);
- this.outWriter = new OutputStreamWriter(outputStream,
StandardCharsets.UTF_8);
- } catch (Exception e) {
- throw new GravitinoRuntimeException(
- e, "Init audit log writer fail, filename is %s", fileName);
- }
+ DEPRECATED_KEYS.forEach(
+ (key, hint) -> {
+ if (properties.containsKey(key)) {
+ LOG.warn(
+ "FileAuditWriter config key '{}' is deprecated and has no
effect. {}", key, hint);
+ }
+ });
+ this.auditLogger = LoggerFactory.getLogger(AUDIT_LOGGER_NAME);
}
@Override
public void doWrite(AuditLog auditLog) {
- String log = auditLog.toString();
- try {
- outWriter.write(log + LINE_SEPARATOR);
- tryFlush();
- } catch (Exception e) {
- Log.warn("Failed to write audit log: {}", log, e);
- }
+ auditLogger.info(auditLog.toString());
}
@Override
public void close() {
- if (outWriter != null) {
- try {
- outWriter.close();
- } catch (Exception e) {
- Log.warn("Failed to close writer", e);
- }
- }
+ // Log4j2 manages the appender lifecycle; nothing to close here.
}
@Override
public String name() {
return "file";
}
-
- private void tryFlush() {
- Instant now = Instant.now();
- if (now.isAfter(nextFlushTime)) {
- nextFlushTime = now.plusSeconds(flushIntervalSecs);
- doFlush();
- }
- }
-
- private void doFlush() {
- if (outWriter != null) {
- try {
- outWriter.flush();
- } catch (Exception e) {
- Log.warn("Flush audit log failed,", e);
- }
- }
- }
}
diff --git
a/core/src/main/java/org/apache/gravitino/audit/v2/SimpleAuditLogV2.java
b/core/src/main/java/org/apache/gravitino/audit/v2/SimpleAuditLogV2.java
index 177b034154..104e91a864 100644
--- a/core/src/main/java/org/apache/gravitino/audit/v2/SimpleAuditLogV2.java
+++ b/core/src/main/java/org/apache/gravitino/audit/v2/SimpleAuditLogV2.java
@@ -19,7 +19,9 @@
package org.apache.gravitino.audit.v2;
-import java.text.SimpleDateFormat;
+import java.time.Instant;
+import java.time.ZoneId;
+import java.time.format.DateTimeFormatter;
import java.util.Map;
import java.util.Optional;
import org.apache.gravitino.NameIdentifier;
@@ -35,6 +37,9 @@ import org.apache.gravitino.listener.api.event.OperationType;
*/
public class SimpleAuditLogV2 implements AuditLog {
+ private static final DateTimeFormatter TIMESTAMP_FORMATTER =
+ DateTimeFormatter.ofPattern("yyyy-MM-dd
HH:mm:ss.SSS").withZone(ZoneId.systemDefault());
+
private final BaseEvent event;
public SimpleAuditLogV2(BaseEvent event) {
@@ -95,14 +100,17 @@ public class SimpleAuditLogV2 implements AuditLog {
@Override
public String toString() {
+ Map<String, String> info = customInfo();
+ String customInfoStr = info != null && !info.isEmpty() ? info.toString() :
"";
return String.format(
- "[%s]\t%s\t%s\t%s\t%s\t%s\t%s",
- new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(timestamp()),
+ "[%s]\t%s\t%s\t%s\t%s\t%s\t%s\t%s",
+ TIMESTAMP_FORMATTER.format(Instant.ofEpochMilli(timestamp())),
user(),
operationType(),
identifier(),
operationStatus(),
eventSource(),
- remoteAddress());
+ remoteAddress(),
+ customInfoStr);
}
}
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/Event.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/Event.java
index 7dba616d42..cb2b0829b7 100644
--- a/core/src/main/java/org/apache/gravitino/listener/api/event/Event.java
+++ b/core/src/main/java/org/apache/gravitino/listener/api/event/Event.java
@@ -19,13 +19,36 @@
package org.apache.gravitino.listener.api.event;
+import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.annotation.DeveloperApi;
+import org.apache.gravitino.utils.RequestContext;
-/** Represents a post event. */
+/**
+ * Represents a post event for Gravitino server operations.
+ *
+ * <p>The client remote address is captured from {@link RequestContext} at
construction time on the
+ * servlet thread, so async listener threads can safely call {@link
#remoteAddress()} without
+ * accessing thread-local storage.
+ */
@DeveloperApi
public abstract class Event extends BaseEvent {
+
+ private final String remoteAddress;
+
protected Event(String user, NameIdentifier identifier) {
super(user, identifier);
+ String addr = RequestContext.getRemoteAddress();
+ this.remoteAddress = StringUtils.isNoneBlank(addr) ? addr : "unknown";
+ }
+
+ /**
+ * Returns the client remote address captured at event construction time.
+ *
+ * @return the client IP address, or {@code "unknown"} if the request
context was not set.
+ */
+ @Override
+ public String remoteAddress() {
+ return remoteAddress;
}
}
diff --git a/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
b/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
new file mode 100644
index 0000000000..9b37bdb942
--- /dev/null
+++ b/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.utils;
+
+/**
+ * Holds per-request context data (e.g. client IP) in a {@link ThreadLocal} so
that event classes
+ * constructed on the servlet thread can capture it without carrying a servlet
dependency.
+ *
+ * <p><b>Threading contract:</b> values must be set and cleared on the same
(servlet) thread. Event
+ * constructors read the value at construction time and store it as a field,
so async listener
+ * threads never access this class.
+ */
+public class RequestContext {
+
+ private static final ThreadLocal<String> REMOTE_ADDRESS = new
ThreadLocal<>();
+
+ private RequestContext() {}
+
+ /**
+ * Sets the client remote address for the current request thread.
+ *
+ * @param remoteAddress the client IP address (or the first entry of {@code
X-Forwarded-For}).
+ */
+ public static void setRemoteAddress(String remoteAddress) {
+ REMOTE_ADDRESS.set(remoteAddress);
+ }
+
+ /**
+ * Returns the client remote address previously set on this thread, or
{@code null} if none was
+ * set.
+ *
+ * @return the client remote address, or {@code null}.
+ */
+ public static String getRemoteAddress() {
+ return REMOTE_ADDRESS.get();
+ }
+
+ /**
+ * Removes the remote address binding from the current thread. Must be
called in a {@code finally}
+ * block after the request completes to prevent thread-pool leaks.
+ */
+ public static void clear() {
+ REMOTE_ADDRESS.remove();
+ }
+}
diff --git
a/core/src/test/java/org/apache/gravitino/audit/TestAuditManager.java
b/core/src/test/java/org/apache/gravitino/audit/TestAuditManager.java
index d4055ce35d..21ec1ca1c6 100644
--- a/core/src/test/java/org/apache/gravitino/audit/TestAuditManager.java
+++ b/core/src/test/java/org/apache/gravitino/audit/TestAuditManager.java
@@ -22,66 +22,24 @@ package org.apache.gravitino.audit;
import static org.apache.gravitino.audit.AuditLog.Operation;
import static org.apache.gravitino.audit.AuditLog.Status;
-import java.io.BufferedReader;
-import java.io.IOException;
-import java.nio.charset.StandardCharsets;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.nio.file.Paths;
import java.util.HashMap;
-import java.util.stream.Stream;
import org.apache.gravitino.Config;
import org.apache.gravitino.Configs;
import org.apache.gravitino.NameIdentifier;
-import org.apache.gravitino.audit.v2.SimpleAuditLogV2;
import org.apache.gravitino.audit.v2.SimpleFormatterV2;
-import org.apache.gravitino.config.ConfigBuilder;
import org.apache.gravitino.listener.EventBus;
import org.apache.gravitino.listener.EventListenerManager;
import org.apache.gravitino.listener.api.event.Event;
import org.apache.gravitino.listener.api.event.FailureEvent;
-import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public class TestAuditManager {
- private static final Logger LOG =
LoggerFactory.getLogger(TestAuditManager.class);
-
- private static final String DEFAULT_FILE_NAME = "gravitino_audit.log";
private static final int EVENT_NUM = 2000;
- private Path logPath;
-
- @BeforeAll
- public void setup() {
- String logDir = System.getProperty("gravitino.log.path");
- Path logDirPath = Paths.get(logDir);
- if (!Files.exists(logDirPath)) {
- try {
- Files.createDirectories(logDirPath);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }
- this.logPath = Paths.get(logDir + "/" + DEFAULT_FILE_NAME);
- if (Files.exists(logPath)) {
- LOG.warn(
- String.format("tmp audit log file: %s already exists, delete it",
DEFAULT_FILE_NAME));
- try {
- Files.delete(logPath);
- LOG.warn(String.format("delete tmp audit log file: %s success",
DEFAULT_FILE_NAME));
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }
- }
-
/** Test audit log with custom audit writer and formatter. */
@Test
public void testAuditLog() {
@@ -109,10 +67,10 @@ public class TestAuditManager {
DummyAuditLog formattedAuditLog = formatter.format(dummyEvent);
Assertions.assertNotNull(formattedAuditLog);
- Assertions.assertEquals(formattedAuditLog.operation(),
Operation.UNKNOWN_OPERATION);
- Assertions.assertEquals(formattedAuditLog.status(), Status.SUCCESS);
- Assertions.assertEquals(formattedAuditLog.user(), "user");
- Assertions.assertEquals(formattedAuditLog.identifier(), "a.b.c.d");
+ Assertions.assertEquals(Operation.UNKNOWN_OPERATION,
formattedAuditLog.operation());
+ Assertions.assertEquals(Status.SUCCESS, formattedAuditLog.status());
+ Assertions.assertEquals("user", formattedAuditLog.user());
+ Assertions.assertEquals("a.b.c.d", formattedAuditLog.identifier());
Assertions.assertEquals(formattedAuditLog.timestamp(),
dummyEvent.eventTime());
Assertions.assertEquals(formattedAuditLog,
dummyAuditWriter.getAuditLogs().get(0));
@@ -121,97 +79,41 @@ public class TestAuditManager {
eventBus.dispatchEvent(dummyFailEvent);
DummyAuditLog formattedFailAuditLog = formatter.format(dummyFailEvent);
Assertions.assertEquals(formattedFailAuditLog,
dummyAuditWriter.getAuditLogs().get(1));
- Assertions.assertEquals(formattedFailAuditLog.operation(),
Operation.UNKNOWN_OPERATION);
- Assertions.assertEquals(formattedFailAuditLog.status(), Status.FAILURE);
+ Assertions.assertEquals(Operation.UNKNOWN_OPERATION,
formattedFailAuditLog.operation());
+ Assertions.assertEquals(Status.FAILURE, formattedFailAuditLog.status());
Assertions.assertEquals(formattedFailAuditLog,
dummyAuditWriter.getAuditLogs().get(1));
}
- /** Test audit log with default audit writer and formatter. */
- @SuppressWarnings("deprecation")
+ /** Verify that FileAuditWriter is used by default and delegates to
SimpleFormatterV2. */
@Test
public void testFileAuditLog() {
Config config = new Config(false) {};
config.set(Configs.AUDIT_LOG_ENABLED_CONF, true);
- DummyEvent dummyEvent = mockDummyEvent();
EventListenerManager eventListenerManager = mockEventListenerManager();
AuditLogManager auditLogManager = mockAuditLogManager(config,
eventListenerManager);
EventBus eventBus = eventListenerManager.createEventBus();
- eventBus.dispatchEvent(dummyEvent);
- Assertions.assertInstanceOf(FileAuditWriter.class,
auditLogManager.getAuditLogWriter());
- Assertions.assertInstanceOf(
- SimpleFormatterV2.class,
(auditLogManager.getAuditLogWriter()).getFormatter());
-
- FileAuditWriter fileAuditWriter = (FileAuditWriter)
auditLogManager.getAuditLogWriter();
- String fileName = fileAuditWriter.fileName;
- try {
- fileAuditWriter.outWriter.flush();
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- String auditLog = readAuditLog(fileName);
- Formatter formatter = fileAuditWriter.getFormatter();
- SimpleAuditLogV2 formattedAuditLog = (SimpleAuditLogV2)
formatter.format(dummyEvent);
+ // Dispatching events must not throw even though no file appender is
configured in the test.
+ eventBus.dispatchEvent(mockDummyEvent());
- Assertions.assertNotNull(formattedAuditLog);
- Assertions.assertEquals(formattedAuditLog.toString(), auditLog);
+ Assertions.assertInstanceOf(FileAuditWriter.class,
auditLogManager.getAuditLogWriter());
+ Assertions.assertInstanceOf(
+ SimpleFormatterV2.class,
auditLogManager.getAuditLogWriter().getFormatter());
}
+ /** Verify that dispatching many events does not throw or deadlock. */
@Test
- public void testBathEvents() {
+ public void testBatchEvents() {
Config config = new Config(false) {};
config.set(Configs.AUDIT_LOG_ENABLED_CONF, true);
- // set immediate flush to true for testing, so that the audit log will be
read immediately
- config.set(
- new
ConfigBuilder("gravitino.audit.writer.file.immediateFlush").stringConf(),
"true");
-
EventListenerManager eventListenerManager = mockEventListenerManager();
- AuditLogManager auditLogManager = mockAuditLogManager(config,
eventListenerManager);
+ mockAuditLogManager(config, eventListenerManager);
EventBus eventBus = eventListenerManager.createEventBus();
for (int i = 0; i < EVENT_NUM; i++) {
- DummyEvent dummyEvent = mockDummyEvent();
- eventBus.dispatchEvent(dummyEvent);
- }
-
- FileAuditWriter fileAuditWriter = (FileAuditWriter)
auditLogManager.getAuditLogWriter();
- String fileName = fileAuditWriter.fileName;
- try {
- fileAuditWriter.outWriter.flush();
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- long auditSize = getAuditSize(fileName);
- Assertions.assertEquals(EVENT_NUM, auditSize);
- }
-
- @AfterEach
- public void cleanup() {
- try {
- if (Files.exists(logPath)) {
- Files.delete(logPath);
- LOG.warn(String.format("delete tmp audit log file: %s success",
DEFAULT_FILE_NAME));
- }
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }
-
- private String readAuditLog(String fileName) {
- try (BufferedReader reader =
- Files.newBufferedReader(Paths.get(fileName), StandardCharsets.UTF_8)) {
- return reader.readLine();
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }
-
- private long getAuditSize(String fileName) {
- try (Stream<String> lines = Files.lines(Paths.get(fileName))) {
- return lines.count();
- } catch (IOException e) {
- throw new RuntimeException(e);
+ eventBus.dispatchEvent(mockDummyEvent());
}
+ // No assertion needed — we just verify no exception is thrown for a large
batch.
}
private AuditLogManager mockAuditLogManager(
diff --git
a/core/src/test/java/org/apache/gravitino/audit/TestFileAuditWriter.java
b/core/src/test/java/org/apache/gravitino/audit/TestFileAuditWriter.java
new file mode 100644
index 0000000000..f117c649f4
--- /dev/null
+++ b/core/src/test/java/org/apache/gravitino/audit/TestFileAuditWriter.java
@@ -0,0 +1,187 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.audit;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.gravitino.audit.v2.SimpleFormatterV2;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.AbstractConfiguration;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.config.LoggerConfig;
+import org.apache.logging.log4j.core.layout.PatternLayout;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class TestFileAuditWriter {
+
+ /** Minimal in-memory appender for capturing log events in tests. */
+ static class CaptureAppender extends AbstractAppender {
+ private final List<LogEvent> events = new ArrayList<>();
+
+ CaptureAppender(String name) {
+ super(name, null, PatternLayout.createDefaultLayout(), true, null);
+ }
+
+ @Override
+ public void append(LogEvent event) {
+ events.add(event.toImmutable());
+ }
+
+ List<LogEvent> getEvents() {
+ return events;
+ }
+ }
+
+ private CaptureAppender auditCapture;
+ private CaptureAppender warnCapture;
+ private LoggerContext loggerContext;
+
+ @BeforeEach
+ public void setup() {
+ loggerContext =
+ (LoggerContext)
LogManager.getContext(FileAuditWriter.class.getClassLoader(), false);
+ Configuration config = loggerContext.getConfiguration();
+
+ auditCapture = new CaptureAppender("auditCapture");
+ auditCapture.start();
+ config.addAppender(auditCapture);
+
+ // Wire a dedicated logger for gravitino.audit so writes are captured in
tests.
+ LoggerConfig auditLoggerConfig =
+ new LoggerConfig(FileAuditWriter.AUDIT_LOGGER_NAME, Level.INFO, false);
+ auditLoggerConfig.addAppender(auditCapture, Level.INFO, null);
+ config.addLogger(FileAuditWriter.AUDIT_LOGGER_NAME, auditLoggerConfig);
+
+ // Capture WARN logs from FileAuditWriter itself (for deprecation warning
tests).
+ warnCapture = new CaptureAppender("warnCapture");
+ warnCapture.start();
+ config.addAppender(warnCapture);
+ String writerLoggerName = FileAuditWriter.class.getName();
+ LoggerConfig writerLoggerConfig = new LoggerConfig(writerLoggerName,
Level.WARN, false);
+ writerLoggerConfig.addAppender(warnCapture, Level.WARN, null);
+ config.addLogger(writerLoggerName, writerLoggerConfig);
+
+ loggerContext.updateLoggers();
+ }
+
+ @AfterEach
+ public void teardown() {
+ AbstractConfiguration config = (AbstractConfiguration)
loggerContext.getConfiguration();
+ config.removeLogger(FileAuditWriter.AUDIT_LOGGER_NAME);
+ config.removeLogger(FileAuditWriter.class.getName());
+ auditCapture.stop();
+ warnCapture.stop();
+ config.removeAppender(auditCapture.getName());
+ config.removeAppender(warnCapture.getName());
+ loggerContext.updateLoggers();
+ }
+
+ @Test
+ public void testDoWritePublishesToAuditLogger() {
+ FileAuditWriter writer = new FileAuditWriter();
+ writer.init(new SimpleFormatterV2(), new HashMap<>());
+
+ AuditLog log = new DummyAuditLog();
+ writer.doWrite(log);
+
+ Assertions.assertEquals(1, auditCapture.getEvents().size());
+ Assertions.assertEquals(
+ log.toString(),
auditCapture.getEvents().get(0).getMessage().getFormattedMessage());
+ }
+
+ @Test
+ public void testDeprecatedKeysProduceWarnLogs() {
+ FileAuditWriter writer = new FileAuditWriter();
+ Map<String, String> properties = new HashMap<>();
+ properties.put("fileName", "old-path.log");
+ properties.put("append", "true");
+ properties.put("flushIntervalSecs", "5");
+ writer.init(new SimpleFormatterV2(), properties);
+
+ List<LogEvent> warns = warnCapture.getEvents();
+ Assertions.assertEquals(3, warns.size(), "Expected one warning per
deprecated key");
+
+ List<String> messages = new ArrayList<>();
+ warns.forEach(e -> messages.add(e.getMessage().getFormattedMessage()));
+ Assertions.assertTrue(messages.stream().anyMatch(m ->
m.contains("fileName")));
+ Assertions.assertTrue(messages.stream().anyMatch(m ->
m.contains("append")));
+ Assertions.assertTrue(messages.stream().anyMatch(m ->
m.contains("flushIntervalSecs")));
+ }
+
+ @Test
+ public void testNoWarnForCleanProperties() {
+ FileAuditWriter writer = new FileAuditWriter();
+ writer.init(new SimpleFormatterV2(), new HashMap<>());
+ Assertions.assertTrue(warnCapture.getEvents().isEmpty());
+ }
+
+ @Test
+ public void testCloseIsNoOp() {
+ FileAuditWriter writer = new FileAuditWriter();
+ writer.init(new SimpleFormatterV2(), new HashMap<>());
+ // Must not throw.
+ Assertions.assertDoesNotThrow(writer::close);
+ }
+
+ // ---- helpers ----
+
+ static class DummyAuditLog implements AuditLog {
+ @Override
+ public String user() {
+ return "test-user";
+ }
+
+ @Override
+ @SuppressWarnings("deprecation")
+ public AuditLog.Operation operation() {
+ return AuditLog.Operation.UNKNOWN_OPERATION;
+ }
+
+ @Override
+ public String identifier() {
+ return "metalake.catalog";
+ }
+
+ @Override
+ public long timestamp() {
+ return System.currentTimeMillis();
+ }
+
+ @Override
+ @SuppressWarnings("deprecation")
+ public AuditLog.Status status() {
+ return AuditLog.Status.SUCCESS;
+ }
+
+ @Override
+ public String toString() {
+ return "dummy-audit-log";
+ }
+ }
+}
diff --git
a/core/src/test/java/org/apache/gravitino/audit/v2/TestSimpleAuditLogV2.java
b/core/src/test/java/org/apache/gravitino/audit/v2/TestSimpleAuditLogV2.java
new file mode 100644
index 0000000000..a96a4de2a3
--- /dev/null
+++ b/core/src/test/java/org/apache/gravitino/audit/v2/TestSimpleAuditLogV2.java
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.audit.v2;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.listener.api.event.Event;
+import org.apache.gravitino.listener.api.event.EventSource;
+import org.apache.gravitino.listener.api.event.OperationStatus;
+import org.apache.gravitino.listener.api.event.OperationType;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestSimpleAuditLogV2 {
+
+ @Test
+ public void testTimestampHasMillisecondPrecision() {
+ SimpleAuditLogV2 log = new SimpleAuditLogV2(new StubEvent());
+ String output = log.toString();
+ // Format is [yyyy-MM-dd HH:mm:ss.SSS] — the dot-separated millis must be
present.
+ Assertions.assertTrue(
+ output.matches("\\[\\d{4}-\\d{2}-\\d{2}
\\d{2}:\\d{2}:\\d{2}\\.\\d{3}\\].*"),
+ "Expected millisecond precision in: " + output);
+ }
+
+ @Test
+ public void testCustomInfoAppendedWhenPresent() {
+ SimpleAuditLogV2 log = new SimpleAuditLogV2(new StubEventWithCustomInfo());
+ String output = log.toString();
+ String[] fields = output.split("\t", -1);
+ // 8 tab-separated fields expected: timestamp, user, opType, id, status,
source, addr, custom
+ Assertions.assertEquals(8, fields.length, "Expected 8 tab-separated
fields, got: " + output);
+ Assertions.assertTrue(
+ fields[7].contains("k1"), "Last field should contain customInfo key: "
+ fields[7]);
+ }
+
+ @Test
+ public void testCustomInfoEmptyWhenAbsent() {
+ SimpleAuditLogV2 log = new SimpleAuditLogV2(new StubEvent());
+ String output = log.toString();
+ String[] fields = output.split("\t", -1);
+ Assertions.assertEquals(8, fields.length);
+ Assertions.assertEquals("", fields[7], "Last field should be empty when
customInfo is absent");
+ }
+
+ @Test
+ public void testOutputContainsAllCoreFields() {
+ SimpleAuditLogV2 log = new SimpleAuditLogV2(new StubEvent());
+ String output = log.toString();
+ Assertions.assertTrue(output.contains("test-user"));
+ Assertions.assertTrue(output.contains("LIST_TABLE"));
+ Assertions.assertTrue(output.contains("metalake.catalog"));
+ Assertions.assertTrue(output.contains("SUCCESS"));
+ }
+
+ // ---- stubs ----
+
+ static class StubEvent extends Event {
+ StubEvent() {
+ super("test-user", NameIdentifier.of("metalake", "catalog"));
+ }
+
+ @Override
+ public OperationType operationType() {
+ return OperationType.LIST_TABLE;
+ }
+
+ @Override
+ public OperationStatus operationStatus() {
+ return OperationStatus.SUCCESS;
+ }
+
+ @Override
+ public EventSource eventSource() {
+ return EventSource.GRAVITINO_SERVER;
+ }
+ }
+
+ static class StubEventWithCustomInfo extends StubEvent {
+ @Override
+ public Map<String, String> customInfo() {
+ return ImmutableMap.of("k1", "v1");
+ }
+ }
+}
diff --git
a/core/src/test/java/org/apache/gravitino/listener/api/event/TestEventRemoteAddress.java
b/core/src/test/java/org/apache/gravitino/listener/api/event/TestEventRemoteAddress.java
new file mode 100644
index 0000000000..5ff669bb18
--- /dev/null
+++
b/core/src/test/java/org/apache/gravitino/listener/api/event/TestEventRemoteAddress.java
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.listener.api.event;
+
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.utils.RequestContext;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestEventRemoteAddress {
+
+ @AfterEach
+ public void cleanup() {
+ RequestContext.clear();
+ }
+
+ @Test
+ public void testRemoteAddressCapturedAtConstructionTime() {
+ RequestContext.setRemoteAddress("10.0.0.1");
+ Event event = new StubEvent();
+ // Clear the ThreadLocal after construction to simulate what the servlet
filter does.
+ RequestContext.clear();
+
+ Assertions.assertEquals(
+ "10.0.0.1",
+ event.remoteAddress(),
+ "remoteAddress() must return the value captured at construction, not
from ThreadLocal");
+ }
+
+ @Test
+ public void testRemoteAddressDefaultsToUnknownWhenNotSet() {
+ // No RequestContext set — must fall back to "unknown".
+ Event event = new StubEvent();
+ Assertions.assertEquals("unknown", event.remoteAddress());
+ }
+
+ @Test
+ public void testRemoteAddressIsolatedAcrossInstances() {
+ RequestContext.setRemoteAddress("1.2.3.4");
+ Event first = new StubEvent();
+
+ RequestContext.setRemoteAddress("5.6.7.8");
+ Event second = new StubEvent();
+
+ Assertions.assertEquals("1.2.3.4", first.remoteAddress());
+ Assertions.assertEquals("5.6.7.8", second.remoteAddress());
+ }
+
+ // ---- stub ----
+
+ static class StubEvent extends Event {
+ StubEvent() {
+ super("test-user", NameIdentifier.of("metalake", "catalog"));
+ }
+
+ @Override
+ public OperationType operationType() {
+ return OperationType.LIST_TABLE;
+ }
+
+ @Override
+ public OperationStatus operationStatus() {
+ return OperationStatus.SUCCESS;
+ }
+
+ @Override
+ public EventSource eventSource() {
+ return EventSource.GRAVITINO_SERVER;
+ }
+ }
+}
diff --git
a/core/src/test/java/org/apache/gravitino/utils/TestRequestContext.java
b/core/src/test/java/org/apache/gravitino/utils/TestRequestContext.java
new file mode 100644
index 0000000000..5c70284446
--- /dev/null
+++ b/core/src/test/java/org/apache/gravitino/utils/TestRequestContext.java
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.utils;
+
+import java.util.concurrent.atomic.AtomicReference;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestRequestContext {
+
+ @AfterEach
+ public void cleanup() {
+ RequestContext.clear();
+ }
+
+ @Test
+ public void testSetAndGet() {
+ RequestContext.setRemoteAddress("192.168.1.1");
+ Assertions.assertEquals("192.168.1.1", RequestContext.getRemoteAddress());
+ }
+
+ @Test
+ public void testClearRemovesValue() {
+ RequestContext.setRemoteAddress("10.0.0.1");
+ RequestContext.clear();
+ Assertions.assertNull(RequestContext.getRemoteAddress());
+ }
+
+ @Test
+ public void testGetReturnsNullWhenNotSet() {
+ Assertions.assertNull(RequestContext.getRemoteAddress());
+ }
+
+ @Test
+ public void testThreadIsolation() throws InterruptedException {
+ RequestContext.setRemoteAddress("main-thread-ip");
+ AtomicReference<String> childValue = new AtomicReference<>();
+
+ Thread child =
+ new Thread(
+ () -> {
+ // Child thread has its own ThreadLocal slot — must not see the
main-thread value.
+ childValue.set(RequestContext.getRemoteAddress());
+ });
+ child.start();
+ child.join();
+
+ Assertions.assertNull(childValue.get(), "Child thread should not inherit
parent ThreadLocal");
+ Assertions.assertEquals(
+ "main-thread-ip", RequestContext.getRemoteAddress(), "Main thread
value unchanged");
+ }
+}
diff --git a/docs/gravitino-server-config.md b/docs/gravitino-server-config.md
index 65d549d0a5..cf6501fc43 100644
--- a/docs/gravitino-server-config.md
+++ b/docs/gravitino-server-config.md
@@ -245,29 +245,47 @@ When processing pre-event, you could throw a
`ForbiddenException` to skip the fo
The audit log framework defines how audit logs are formatted and written to
various storages. The formatter defines an interface that transforms different
`Event` types into a unified `AuditLog`. The writer defines an interface to
writing AuditLog to different storages.
-Gravitino provides a default implement to log basic audit information to a
file, you could extend the audit system by implementation corresponding
interfaces.
+Gravitino provides a default implementation to log basic audit information to
a file. You can extend the audit system by implementing the corresponding
interfaces.
-| Property name | Description
| Default value | Required | Since Version
|
-|---------------------------------------|----------------------------------------|---------------------------------------------|----------|----------------------------|
-| `gravitino.audit.enabled` | The audit log enable flag.
| false | NO | 0.7.0-incubating
|
-| `gravitino.audit.writer.className` | The class name of audit log writer.
| org.apache.gravitino.audit.FileAuditWriter | NO | 0.7.0-incubating
|
-| `gravitino.audit.formatter.className` | The class name of audit log
formatter. | org.apache.gravitino.audit.SimpleFormatter | NO |
0.7.0-incubating |
+| Property name | Description
| Default value | Required | Since Version
|
+|---------------------------------------|----------------------------------------|------------------------------------------------|----------|------------------|
+| `gravitino.audit.enabled` | The audit log enable flag.
| false | NO |
0.7.0-incubating |
+| `gravitino.audit.writer.className` | The class name of audit log writer.
| org.apache.gravitino.audit.FileAuditWriter | NO |
0.7.0-incubating |
+| `gravitino.audit.formatter.className` | The class name of audit log
formatter. | org.apache.gravitino.audit.v2.SimpleFormatterV2 | NO |
0.7.0-incubating |
#### Audit log formatter
-The Formatter defines an interface that formats metadata audit logs into a
unified format. `SimpleFormatter` is a default implement to format audit
information, you don't need to do extra configurations.
+The `Formatter` interface transforms an `Event` into an `AuditLog`.
`SimpleFormatterV2` is the default implementation and requires no extra
configuration. It produces a tab-separated line with the following fields:
timestamp, user, operation type, identifier, operation status, event source,
remote address, and custom info.
#### Audit log writer
-The `AuditLogWriter` defines an interface that enables the writing of metadata
audit logs to different storage mediums such as files, databases, etc.
+The `AuditLogWriter` interface enables writing audit logs to different storage
mediums (files, databases, etc.).
-Writer configuration begins with `gravitino.audit.writer.${name}`, where
`${name}` is replaced with the actual writer name defined in method `name()`.
`FileAuditWriter` is a default implement to log audit information, whose name
is `file`.
+`FileAuditWriter` is the default implementation. It delegates all file
management — rotation, compression, and retention — to Log4j2 via a dedicated
logger named `gravitino.audit`. The appender is configured in
`conf/log4j2.properties` (see the `audit_file` appender group). The default
configuration rotates daily and on 256 MB, compresses rotated files with gzip,
and deletes files older than 30 days.
-| Property name | Description
| Default value |
Required | Since Version |
-|-------------------------------------------------|-------------------------------------------------------------------------------|---------------------|----------|------------------|
-| `gravitino.audit.writer.file.fileName` | The audit log file name,
the path is `${sys:gravitino.log.path}/${fileName}`. | gravitino_audit.log | NO
| 0.7.0-incubating |
-| `gravitino.audit.writer.file.flushIntervalSecs` | The flush interval time of
the audit file in seconds. | 10 | NO
| 0.7.0-incubating |
-| `gravitino.audit.writer.file.append` | Whether the log will be
written to the end or the beginning of the file. | true |
NO | 0.7.0-incubating |
+##### Deprecated FileAuditWriter properties
+
+The following `gravitino.audit.writer.file.*` properties were accepted in
earlier versions but are now **deprecated** and have no effect.
`FileAuditWriter` emits a `WARN` log at startup if any of them are present.
Configure the equivalent behavior directly in `conf/log4j2.properties` instead.
+
+| Deprecated property | Migration: configure in
`conf/log4j2.properties` |
+|-------------------------------------------------|--------------------------------------------------------------------|
+| `gravitino.audit.writer.file.fileName` |
`appender.audit_file.fileName` |
+| `gravitino.audit.writer.file.append` |
`appender.audit_file.append` |
+| `gravitino.audit.writer.file.flushIntervalSecs` | Use `immediateFlush` on
the appender or an async appender wrapper |
+
+Example — change the audit log path:
+
+```properties
+# conf/log4j2.properties
+appender.audit_file.fileName = /var/log/gravitino/my_audit.log
+appender.audit_file.filePattern =
/var/log/gravitino/my_audit_%d{yyyyMMdd}.%i.log.gz
+```
+
+Example — adjust retention to 90 days:
+
+```properties
+appender.audit_file.strategy.delete.ifLastModified.age = 90d
+```
### Security configuration
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java
index c6cf52661c..4881257805 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java
@@ -21,6 +21,7 @@ package org.apache.gravitino.listener.api.event;
import java.util.Map;
import javax.servlet.http.HttpServletRequest;
+import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
import org.apache.gravitino.utils.PrincipalUtils;
@@ -58,13 +59,24 @@ public class IcebergRequestContext {
public IcebergRequestContext(
HttpServletRequest httpRequest, String catalogName, boolean
requestCredentialVending) {
this.httpServletRequest = httpRequest;
- this.remoteHostName = httpRequest.getRemoteHost();
+ this.remoteHostName = resolveClientAddress(httpRequest);
this.httpHeaders = IcebergRESTUtils.getHttpHeaders(httpRequest);
this.catalogName = catalogName;
this.userName = PrincipalUtils.getCurrentUserName();
this.requestCredentialVending = requestCredentialVending;
}
+ private static String resolveClientAddress(HttpServletRequest request) {
+ // X-Forwarded-For is trusted unconditionally; callers in environments
where the server is
+ // reachable directly (not only via a trusted proxy) should be aware that
this header can be
+ // spoofed by clients.
+ String xForwardedFor = request.getHeader("X-Forwarded-For");
+ if (StringUtils.isNotBlank(xForwardedFor)) {
+ return xForwardedFor.split(",")[0].trim();
+ }
+ return request.getRemoteHost();
+ }
+
/**
* Returns the catalog name.
*
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
b/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
new file mode 100644
index 0000000000..f5de5185bf
--- /dev/null
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.web;
+
+import java.io.IOException;
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.utils.RequestContext;
+
+/**
+ * A servlet filter that captures the client remote address from each HTTP
request and stores it in
+ * {@link RequestContext} so that audit event constructors can read it on the
same thread.
+ *
+ * <p>When a reverse proxy is in use, the real client IP is taken from the
first entry of the {@code
+ * X-Forwarded-For} header (a de-facto standard header set by reverse proxies;
note that it is
+ * trusted unconditionally — deployments where the server is reachable
directly, without a trusted
+ * reverse proxy, should be aware that clients can spoof this header). If the
header is absent,
+ * {@link HttpServletRequest#getRemoteAddr()} is used instead.
+ *
+ * <p>The stored value is always cleared in a {@code finally} block to prevent
thread-pool leaks.
+ */
+public class RequestContextFilter implements Filter {
+
+ private static final String X_FORWARDED_FOR = "X-Forwarded-For";
+
+ @Override
+ public void init(FilterConfig filterConfig) {}
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain)
+ throws IOException, ServletException {
+ try {
+ if (request instanceof HttpServletRequest) {
+
RequestContext.setRemoteAddress(resolveClientAddress((HttpServletRequest)
request));
+ }
+ chain.doFilter(request, response);
+ } finally {
+ RequestContext.clear();
+ }
+ }
+
+ @Override
+ public void destroy() {}
+
+ private String resolveClientAddress(HttpServletRequest request) {
+ String xForwardedFor = request.getHeader(X_FORWARDED_FOR);
+ if (StringUtils.isNotBlank(xForwardedFor)) {
+ return xForwardedFor.split(",")[0].trim();
+ }
+ return request.getRemoteAddr();
+ }
+}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
new file mode 100644
index 0000000000..1b69f03ac4
--- /dev/null
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.web;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.util.concurrent.atomic.AtomicReference;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.gravitino.utils.RequestContext;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestRequestContextFilter {
+
+ private final RequestContextFilter filter = new RequestContextFilter();
+
+ @AfterEach
+ public void cleanup() {
+ RequestContext.clear();
+ }
+
+ @Test
+ public void testSetsRemoteAddrFromRequest() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("192.168.1.1");
+
+ AtomicReference<String> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRemoteAddress());
+
+ filter.doFilter(req, resp, chain);
+
+ Assertions.assertEquals("192.168.1.1", captured.get());
+ }
+
+ @Test
+ public void testXForwardedForSingleEntry() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn("203.0.113.5");
+ when(req.getRemoteAddr()).thenReturn("10.0.0.1");
+
+ AtomicReference<String> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRemoteAddress());
+
+ filter.doFilter(req, resp, chain);
+
+ Assertions.assertEquals("203.0.113.5", captured.get());
+ }
+
+ @Test
+ public void testXForwardedForMultipleEntriesUsesFirst() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn("203.0.113.5, 10.1.1.1,
10.2.2.2");
+ when(req.getRemoteAddr()).thenReturn("10.0.0.1");
+
+ AtomicReference<String> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRemoteAddress());
+
+ filter.doFilter(req, resp, chain);
+
+ Assertions.assertEquals("203.0.113.5", captured.get());
+ }
+
+ @Test
+ public void testThreadLocalClearedAfterChain() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+
+ filter.doFilter(req, resp, (request, response) -> {});
+
+ Assertions.assertNull(
+ RequestContext.getRemoteAddress(), "ThreadLocal must be cleared after
chain completes");
+ }
+
+ @Test
+ public void testThreadLocalClearedEvenOnChainException() throws IOException,
ServletException {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(null);
+ when(req.getRemoteAddr()).thenReturn("1.2.3.4");
+
+ FilterChain throwingChain =
+ (request, response) -> {
+ throw new ServletException("simulated error");
+ };
+
+ Assertions.assertThrows(
+ ServletException.class, () -> filter.doFilter(req, resp,
throwingChain));
+ Assertions.assertNull(
+ RequestContext.getRemoteAddress(), "ThreadLocal must be cleared even
when chain throws");
+ }
+}
diff --git
a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
index 91630cfe5d..2b2f424c5a 100644
--- a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
+++ b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
@@ -53,6 +53,7 @@ import
org.apache.gravitino.server.web.HttpServerMetricsSource;
import org.apache.gravitino.server.web.JettyServer;
import org.apache.gravitino.server.web.JettyServerConfig;
import org.apache.gravitino.server.web.ObjectMapperProvider;
+import org.apache.gravitino.server.web.RequestContextFilter;
import org.apache.gravitino.server.web.VersioningFilter;
import org.apache.gravitino.server.web.filter.AccessControlNotAllowedFilter;
import org.apache.gravitino.server.web.filter.GravitinoInterceptionService;
@@ -186,6 +187,7 @@ public class GravitinoServer extends ResourceConfig {
server.addServlet(new HealthAliasServlet(), "/health.html");
server.addCustomFilters(API_ANY_PATH);
+ server.addFilter(new RequestContextFilter(), API_ANY_PATH);
server.addFilter(new VersioningFilter(), API_ANY_PATH);
server.addSystemFilters(API_ANY_PATH);
if (server.isWebUiEnabled()) {
