This is an automated email from the ASF dual-hosted git repository.
somandal pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 388497cabfc [audit] Implement support for comma-separated header
filtering (#16694)
388497cabfc is described below
commit 388497cabfceab313d31194b4e67b1626865f069
Author: Suvodeep Pyne <[email protected]>
AuthorDate: Wed Aug 27 11:28:20 2025 -0700
[audit] Implement support for comma-separated header filtering (#16694)
* [audit] Update captureRequestHeaders to accept comma-separated values and
filter headers accordingly
* [audit] Enhance header capture functionality with comprehensive tests for
various string formats and edge cases
---
.../org/apache/pinot/common/audit/AuditConfig.java | 8 +-
.../pinot/common/audit/AuditRequestProcessor.java | 72 +++++++++--
.../pinot/common/audit/AuditConfigManagerTest.java | 43 +++++--
.../common/audit/AuditRequestProcessorTest.java | 131 ++++++++++++++++++++-
4 files changed, 228 insertions(+), 26 deletions(-)
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfig.java
b/pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfig.java
index 957b2be5195..4de6a5848f0 100644
--- a/pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfig.java
+++ b/pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfig.java
@@ -36,7 +36,7 @@ public final class AuditConfig {
private boolean _captureRequestPayload = false;
@JsonProperty("capture.request.headers")
- private boolean _captureRequestHeaders = false;
+ private String _captureRequestHeaders = "";
@JsonProperty("payload.size.max.bytes")
private int _maxPayloadSize = 10_240;
@@ -60,11 +60,11 @@ public final class AuditConfig {
_captureRequestPayload = captureRequestPayload;
}
- public boolean isCaptureRequestHeaders() {
+ public String getCaptureRequestHeaders() {
return _captureRequestHeaders;
}
- public void setCaptureRequestHeaders(boolean captureRequestHeaders) {
+ public void setCaptureRequestHeaders(String captureRequestHeaders) {
_captureRequestHeaders = captureRequestHeaders;
}
@@ -87,7 +87,7 @@ public final class AuditConfig {
@Override
public String toString() {
return "AuditConfig{" + "enabled=" + _enabled + ", captureRequestPayload="
+ _captureRequestPayload
- + ", captureRequestHeaders=" + _captureRequestHeaders + ",
maxPayloadSize=" + _maxPayloadSize
+ + ", captureRequestHeaders='" + _captureRequestHeaders + "',
maxPayloadSize=" + _maxPayloadSize
+ ", excludedEndpoints='" + _excludedEndpoints + "'}";
}
}
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java
b/pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java
index fea1cd69840..d7a8ecbde2d 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java
@@ -18,10 +18,14 @@
*/
package org.apache.pinot.common.audit;
+import com.google.common.annotations.VisibleForTesting;
import java.time.Instant;
+import java.util.Collections;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import javax.inject.Inject;
import javax.inject.Singleton;
import javax.ws.rs.container.ContainerRequestContext;
@@ -52,19 +56,40 @@ public class AuditRequestProcessor {
* If a key has multiple values, the list of values is added instead.
*
* @param multimap the input MultivaluedMap containing keys and their
associated values
+ * @param allowedKeys optional set of allowed keys for case-insensitive
filtering.
+ * If null or empty, all keys are included
* @return a Map where each key is mapped to either a single value or a list
of values
*/
- private static Map<String, Object> toMap(MultivaluedMap<String, String>
multimap) {
- Map<String, Object> queryMap = new HashMap<>();
+ private static Map<String, Object> toMap(MultivaluedMap<String, String>
multimap, Set<String> allowedKeys) {
+ Map<String, Object> resultMap = new HashMap<>();
+ boolean filterKeys = allowedKeys != null && !allowedKeys.isEmpty();
+
for (Map.Entry<String, List<String>> entry : multimap.entrySet()) {
+ String key = entry.getKey();
+ // Skip if filtering is enabled and key is not in allowed list
(case-insensitive)
+ if (filterKeys && !allowedKeys.contains(key.toLowerCase())) {
+ continue;
+ }
+
List<String> values = entry.getValue();
if (values.size() == 1) {
- queryMap.put(entry.getKey(), values.get(0));
+ resultMap.put(key, values.get(0));
} else {
- queryMap.put(entry.getKey(), values);
+ resultMap.put(key, values);
}
}
- return queryMap;
+ return resultMap;
+ }
+
+ /**
+ * Converts a MultivaluedMap into a Map of query parameters without
filtering.
+ * Backward compatibility method.
+ *
+ * @param multimap the input MultivaluedMap containing keys and their
associated values
+ * @return a Map where each key is mapped to either a single value or a list
of values
+ */
+ private static Map<String, Object> toMap(MultivaluedMap<String, String>
multimap) {
+ return toMap(multimap, null);
}
public AuditEvent processRequest(ContainerRequestContext requestContext,
String remoteAddr) {
@@ -140,10 +165,15 @@ public class AuditRequestProcessor {
}
final AuditConfig config = _configManager.getCurrentConfig();
- if (config.isCaptureRequestHeaders()) {
- MultivaluedMap<String, String> headers = requestContext.getHeaders();
- if (!headers.isEmpty()) {
- payload.setHeaders(toMap(headers));
+
+ Set<String> allowedHeaders =
parseAllowedHeaders(config.getCaptureRequestHeaders());
+ if (!allowedHeaders.isEmpty()) {
+ MultivaluedMap<String, String> allHeaders =
requestContext.getHeaders();
+ if (!allHeaders.isEmpty()) {
+ Map<String, Object> filteredHeaders = toMap(allHeaders,
allowedHeaders);
+ if (!filteredHeaders.isEmpty()) {
+ payload.setHeaders(filteredHeaders);
+ }
}
}
@@ -174,6 +204,30 @@ public class AuditRequestProcessor {
* @param maxPayloadSize maximum bytes to read from the request body
* @return the request body as string (potentially truncated)
*/
+ /**
+ * Parses a comma-separated list of headers into a Set of lowercase header
names
+ * for case-insensitive comparison.
+ *
+ * @param headerList comma-separated list of header names
+ * @return Set of lowercase header names, empty if headerList is blank
+ */
+ @VisibleForTesting
+ static Set<String> parseAllowedHeaders(String headerList) {
+ if (StringUtils.isBlank(headerList)) {
+ return Collections.emptySet();
+ }
+
+ Set<String> headers = new HashSet<>();
+ for (String header : headerList.split(",")) {
+ String trimmed = header.trim();
+ if (!trimmed.isEmpty()) {
+ // Store as lowercase for case-insensitive comparison
+ headers.add(trimmed.toLowerCase());
+ }
+ }
+ return headers;
+ }
+
private String readRequestBody(ContainerRequestContext requestContext, int
maxPayloadSize) {
// TODO spyne to be implemented
return null;
diff --git
a/pinot-common/src/test/java/org/apache/pinot/common/audit/AuditConfigManagerTest.java
b/pinot-common/src/test/java/org/apache/pinot/common/audit/AuditConfigManagerTest.java
index 84dfbffc9a5..a3ebd0e3932 100644
---
a/pinot-common/src/test/java/org/apache/pinot/common/audit/AuditConfigManagerTest.java
+++
b/pinot-common/src/test/java/org/apache/pinot/common/audit/AuditConfigManagerTest.java
@@ -38,7 +38,7 @@ public class AuditConfigManagerTest {
Map<String, String> properties = new HashMap<>();
properties.put("pinot.audit.enabled", "true");
properties.put("pinot.audit.capture.request.payload.enabled", "true");
- properties.put("pinot.audit.capture.request.headers", "true");
+ properties.put("pinot.audit.capture.request.headers",
"Content-Type,X-Request-Id,Authorization");
properties.put("pinot.audit.payload.size.max.bytes", "20480");
properties.put("pinot.audit.excluded.endpoints", "/health,/metrics");
properties.put("some.other.config", "value");
@@ -53,7 +53,7 @@ public class AuditConfigManagerTest {
AuditConfig config = manager.getCurrentConfig();
assertThat(config.isEnabled()).isTrue();
assertThat(config.isCaptureRequestPayload()).isTrue();
- assertThat(config.isCaptureRequestHeaders()).isTrue();
+
assertThat(config.getCaptureRequestHeaders()).isEqualTo("Content-Type,X-Request-Id,Authorization");
assertThat(config.getMaxPayloadSize()).isEqualTo(20480);
assertThat(config.getExcludedEndpoints()).isEqualTo("/health,/metrics");
}
@@ -78,7 +78,7 @@ public class AuditConfigManagerTest {
assertThat(config.getMaxPayloadSize()).isEqualTo(5000);
// Verify defaults for unspecified configs
assertThat(config.isCaptureRequestPayload()).isFalse();
- assertThat(config.isCaptureRequestHeaders()).isFalse();
+ assertThat(config.getCaptureRequestHeaders()).isEmpty();
assertThat(config.getExcludedEndpoints()).isEmpty();
}
@@ -97,7 +97,7 @@ public class AuditConfigManagerTest {
AuditConfig config = manager.getCurrentConfig();
assertThat(config.isEnabled()).isFalse();
assertThat(config.isCaptureRequestPayload()).isFalse();
- assertThat(config.isCaptureRequestHeaders()).isFalse();
+ assertThat(config.getCaptureRequestHeaders()).isEmpty();
assertThat(config.getMaxPayloadSize()).isEqualTo(10240);
assertThat(config.getExcludedEndpoints()).isEmpty();
}
@@ -132,7 +132,7 @@ public class AuditConfigManagerTest {
Map<String, String> properties = new HashMap<>();
properties.put("pinot.audit.enabled", "true");
properties.put("pinot.audit.capture.request.payload.enabled", "false");
- properties.put("pinot.audit.capture.request.headers", "true");
+ properties.put("pinot.audit.capture.request.headers",
"X-User-Id,X-Session-Token");
properties.put("some.other.config", "value");
properties.put("another.config", "123");
@@ -142,7 +142,7 @@ public class AuditConfigManagerTest {
// Then
assertThat(config.isEnabled()).isTrue();
assertThat(config.isCaptureRequestPayload()).isFalse();
- assertThat(config.isCaptureRequestHeaders()).isTrue();
+
assertThat(config.getCaptureRequestHeaders()).isEqualTo("X-User-Id,X-Session-Token");
// Verify defaults for unspecified fields
assertThat(config.getMaxPayloadSize()).isEqualTo(10240);
assertThat(config.getExcludedEndpoints()).isEmpty();
@@ -211,7 +211,7 @@ public class AuditConfigManagerTest {
Map<String, String> customProperties = new HashMap<>();
customProperties.put("pinot.audit.enabled", "true");
customProperties.put("pinot.audit.capture.request.payload.enabled",
"true");
- customProperties.put("pinot.audit.capture.request.headers", "true");
+ customProperties.put("pinot.audit.capture.request.headers",
"X-Trace-Id,X-Correlation-Id");
customProperties.put("pinot.audit.payload.size.max.bytes", "50000");
customProperties.put("pinot.audit.excluded.endpoints", "/test,/debug");
manager.onChange(customProperties.keySet(), customProperties);
@@ -220,7 +220,7 @@ public class AuditConfigManagerTest {
AuditConfig customConfig = manager.getCurrentConfig();
assertThat(customConfig.isEnabled()).isTrue();
assertThat(customConfig.isCaptureRequestPayload()).isTrue();
- assertThat(customConfig.isCaptureRequestHeaders()).isTrue();
+
assertThat(customConfig.getCaptureRequestHeaders()).isEqualTo("X-Trace-Id,X-Correlation-Id");
assertThat(customConfig.getMaxPayloadSize()).isEqualTo(50000);
assertThat(customConfig.getExcludedEndpoints()).isEqualTo("/test,/debug");
@@ -233,8 +233,33 @@ public class AuditConfigManagerTest {
AuditConfig defaultConfig = manager.getCurrentConfig();
assertThat(defaultConfig.isEnabled()).isFalse();
assertThat(defaultConfig.isCaptureRequestPayload()).isFalse();
- assertThat(defaultConfig.isCaptureRequestHeaders()).isFalse();
+ assertThat(defaultConfig.getCaptureRequestHeaders()).isEmpty();
assertThat(defaultConfig.getMaxPayloadSize()).isEqualTo(10240);
assertThat(defaultConfig.getExcludedEndpoints()).isEmpty();
}
+
+ @Test
+ public void testConfigurationStringFormats() {
+ Map<String, String> properties = new HashMap<>();
+
+ // Test various string formats
+ properties.put("pinot.audit.capture.request.headers",
"Header1,Header2,Header3");
+ AuditConfig config1 =
AuditConfigManager.buildFromClusterConfig(properties);
+
assertThat(config1.getCaptureRequestHeaders()).isEqualTo("Header1,Header2,Header3");
+
+ // Test empty string
+ properties.put("pinot.audit.capture.request.headers", "");
+ AuditConfig config2 =
AuditConfigManager.buildFromClusterConfig(properties);
+ assertThat(config2.getCaptureRequestHeaders()).isEmpty();
+
+ // Test single header
+ properties.put("pinot.audit.capture.request.headers", "Content-Type");
+ AuditConfig config3 =
AuditConfigManager.buildFromClusterConfig(properties);
+ assertThat(config3.getCaptureRequestHeaders()).isEqualTo("Content-Type");
+
+ // Test headers with mixed case and special characters
+ properties.put("pinot.audit.capture.request.headers",
"Content-Type,X-Request-ID,User-Agent,X-Custom-123");
+ AuditConfig config4 =
AuditConfigManager.buildFromClusterConfig(properties);
+
assertThat(config4.getCaptureRequestHeaders()).isEqualTo("Content-Type,X-Request-ID,User-Agent,X-Custom-123");
+ }
}
diff --git
a/pinot-common/src/test/java/org/apache/pinot/common/audit/AuditRequestProcessorTest.java
b/pinot-common/src/test/java/org/apache/pinot/common/audit/AuditRequestProcessorTest.java
index 2e01c08a4d1..3e1edc9e332 100644
---
a/pinot-common/src/test/java/org/apache/pinot/common/audit/AuditRequestProcessorTest.java
+++
b/pinot-common/src/test/java/org/apache/pinot/common/audit/AuditRequestProcessorTest.java
@@ -21,6 +21,7 @@ package org.apache.pinot.common.audit;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.core.MultivaluedHashMap;
import javax.ws.rs.core.MultivaluedMap;
@@ -68,7 +69,7 @@ public class AuditRequestProcessorTest {
_defaultConfig = new AuditConfig();
_defaultConfig.setEnabled(true);
_defaultConfig.setCaptureRequestPayload(false);
- _defaultConfig.setCaptureRequestHeaders(false);
+ _defaultConfig.setCaptureRequestHeaders("");
_defaultConfig.setMaxPayloadSize(10240);
_defaultConfig.setExcludedEndpoints("");
@@ -139,7 +140,7 @@ public class AuditRequestProcessorTest {
@Test
public void testCaptureHeadersWhenEnabled() {
- _defaultConfig.setCaptureRequestHeaders(true);
+
_defaultConfig.setCaptureRequestHeaders("Content-Type,X-Custom-Header,Authorization,X-Password");
MultivaluedMap<String, String> headers =
createHeaders("Content-Type", "application/json", "X-Custom-Header",
"custom-value", "Authorization",
"Bearer token123", // Should be filtered out
@@ -167,7 +168,7 @@ public class AuditRequestProcessorTest {
@Test
public void testSkipHeadersWhenDisabled() {
- _defaultConfig.setCaptureRequestHeaders(false);
+ _defaultConfig.setCaptureRequestHeaders("");
MultivaluedMap<String, String> headers = createHeaders("Content-Type",
"application/json");
when(_requestContext.getUriInfo()).thenReturn(_uriInfo);
@@ -187,7 +188,8 @@ public class AuditRequestProcessorTest {
@Test
public void testFilterSensitiveHeaders() {
- _defaultConfig.setCaptureRequestHeaders(true);
+ _defaultConfig.setCaptureRequestHeaders(
+
"authorization,x-auth-token,password-header,api-secret,x-api-key,content-type");
MultivaluedMap<String, String> headers =
createHeaders("authorization", "Bearer token123", "x-auth-token",
"token456", "password-header", "pass123",
"api-secret", "secret789", "x-api-key", "key123",
@@ -240,4 +242,125 @@ public class AuditRequestProcessorTest {
// The main processRequest catches all exceptions and returns null, so
test for that
assertThat(result).isNull();
}
+
+ @Test
+ public void testParseAllowedHeadersEdgeCases() {
+ // Empty/null/whitespace
+ assertThat(AuditRequestProcessor.parseAllowedHeaders("")).isEmpty();
+ assertThat(AuditRequestProcessor.parseAllowedHeaders(" ")).isEmpty();
+ assertThat(AuditRequestProcessor.parseAllowedHeaders(null)).isEmpty();
+
+ // Single header
+ Set<String> singleHeader =
AuditRequestProcessor.parseAllowedHeaders("Content-Type");
+ assertThat(singleHeader).containsExactly("content-type");
+
+ // Malformed comma separation
+ Set<String> malformed1 =
AuditRequestProcessor.parseAllowedHeaders("Header1,,Header2");
+ assertThat(malformed1).containsExactlyInAnyOrder("header1", "header2");
+
+ Set<String> malformed2 =
AuditRequestProcessor.parseAllowedHeaders(",Header1,Header2,");
+ assertThat(malformed2).containsExactlyInAnyOrder("header1", "header2");
+
+ // Whitespace handling
+ Set<String> withWhitespace = AuditRequestProcessor.parseAllowedHeaders("
Content-Type , X-Custom ");
+ assertThat(withWhitespace).containsExactly("content-type", "x-custom");
+ }
+
+ @Test
+ public void testHeaderFilteringCaseInsensitive() {
+
_defaultConfig.setCaptureRequestHeaders("content-type,authorization,x-custom-header");
+
+ MultivaluedMap<String, String> headers = createHeaders(
+ "Content-Type", "application/json", // Different case
+ "AUTHORIZATION", "Bearer token", // All caps
+ "x-custom-header", "value", // All lower
+ "X-Ignored-Header", "ignored" // Not in config
+ );
+
+ when(_requestContext.getUriInfo()).thenReturn(_uriInfo);
+ when(_uriInfo.getQueryParameters()).thenReturn(new MultivaluedHashMap<>());
+ when(_uriInfo.getPath()).thenReturn("/test");
+ when(_requestContext.getMethod()).thenReturn("GET");
+ when(_requestContext.getHeaders()).thenReturn(headers);
+
+ AuditEvent result = _processor.processRequest(_requestContext, "10.0.0.1");
+
+ assertThat(result).isNotNull();
+ AuditEvent.AuditRequestPayload payload = result.getRequest();
+ assertThat(payload).isNotNull();
+ Map<String, Object> capturedHeaders = payload.getHeaders();
+
+ // Should capture first 3, ignore the 4th
+ assertThat(capturedHeaders).hasSize(3);
+ assertThat(capturedHeaders).containsKeys("Content-Type", "AUTHORIZATION",
"x-custom-header");
+ assertThat(capturedHeaders).doesNotContainKey("X-Ignored-Header");
+ }
+
+ @Test
+ public void testHeaderValueHandling() {
+
_defaultConfig.setCaptureRequestHeaders("single-value,multi-value,empty-value");
+
+ MultivaluedMap<String, String> headers = new MultivaluedHashMap<>();
+ headers.add("single-value", "value1");
+ headers.addAll("multi-value", Arrays.asList("val1", "val2", "val3"));
+ headers.add("empty-value", "");
+
+ when(_requestContext.getUriInfo()).thenReturn(_uriInfo);
+ when(_uriInfo.getQueryParameters()).thenReturn(new MultivaluedHashMap<>());
+ when(_uriInfo.getPath()).thenReturn("/test");
+ when(_requestContext.getMethod()).thenReturn("GET");
+ when(_requestContext.getHeaders()).thenReturn(headers);
+
+ AuditEvent result = _processor.processRequest(_requestContext, "10.0.0.1");
+
+ assertThat(result).isNotNull();
+ AuditEvent.AuditRequestPayload payload = result.getRequest();
+ assertThat(payload).isNotNull();
+ Map<String, Object> capturedHeaders = payload.getHeaders();
+
+ // Single value stored as String
+ assertThat(capturedHeaders.get("single-value")).isEqualTo("value1");
+
+ // Multiple values stored as List
+ @SuppressWarnings("unchecked")
+ List<String> multiValues = (List<String>)
capturedHeaders.get("multi-value");
+ assertThat(multiValues).containsExactly("val1", "val2", "val3");
+
+ // Empty value still captured
+ assertThat(capturedHeaders.get("empty-value")).isEqualTo("");
+ }
+
+ @Test
+ public void testCompleteHeaderCaptureFlow() {
+ // Configure specific headers
+
_defaultConfig.setCaptureRequestHeaders("Content-Type,X-Request-ID,User-Agent");
+
+ // Create request with mixed case headers + extras
+ MultivaluedMap<String, String> headers = createHeaders(
+ "content-type", "application/json",
+ "X-REQUEST-ID", "req-123",
+ "user-agent", "test-client",
+ "Cookie", "session=abc", // Should be ignored
+ "Accept", "application/json" // Should be ignored
+ );
+
+ when(_requestContext.getUriInfo()).thenReturn(_uriInfo);
+ when(_uriInfo.getQueryParameters()).thenReturn(new MultivaluedHashMap<>());
+ when(_uriInfo.getPath()).thenReturn("/test");
+ when(_requestContext.getMethod()).thenReturn("GET");
+ when(_requestContext.getHeaders()).thenReturn(headers);
+
+ AuditEvent result = _processor.processRequest(_requestContext, "10.0.0.1");
+
+ assertThat(result).isNotNull();
+ AuditEvent.AuditRequestPayload payload = result.getRequest();
+ assertThat(payload).isNotNull();
+ Map<String, Object> capturedHeaders = payload.getHeaders();
+
+ assertThat(capturedHeaders).hasSize(3);
+ assertThat(capturedHeaders).containsOnlyKeys("content-type",
"X-REQUEST-ID", "user-agent");
+ assertThat(capturedHeaders).containsEntry("content-type",
"application/json");
+ assertThat(capturedHeaders).containsEntry("X-REQUEST-ID", "req-123");
+ assertThat(capturedHeaders).containsEntry("user-agent", "test-client");
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
