You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@pinot.apache.org by "suvodeep-pyne (via GitHub)" <gi...@apache.org> on 2025/08/15 20:12:13 UTC

[PR] Implement initial stub for audit logging framework in Pinot [pinot]

suvodeep-pyne opened a new pull request, #16616:
URL: https://github.com/apache/pinot/pull/16616

   # Implement initial stub for audit logging framework in Pinot
   
   ## Summary
   
   This PR introduces the initial framework stub for audit logging in Apache Pinot Controller API requests. **The audit logging framework is disabled by default** and serves as the foundational infrastructure that will be enhanced in subsequent pull requests.
   
   The framework provides configurable audit logging capabilities with structured JSON output, dynamic configuration management, and request filtering - ready for future activation and extension.
   
   ## Changes Included
   
   ### Core Audit Framework Components
   
   **New Classes Added:**
   - `AuditConfig.java` - Configuration data class with Jackson annotations for JSON mapping
   - `AuditConfigManager.java` - Thread-safe configuration manager with endpoint filtering logic
   - `AuditEvent.java` - Data class representing audit events with all required fields
   - `AuditLogger.java` - Utility class for structured JSON audit logging using SLF4J
   - `AuditRequestProcessor.java` - Request processing logic for extracting audit information
   - `AuditLogFilter.java` - Jersey filter for intercepting API requests
   
   ### Configuration Properties
   
   The audit logging framework supports the following configurable properties:
   - `enabled` - Enable/disable audit logging (default: false)
   - `capture.request.payload.enabled` - Capture request payloads (default: false)
   - `capture.request.headers` - Capture request headers (default: false)
   - `max.payload.size` - Maximum payload size in bytes (default: 10,240)
   - `excluded.endpoints` - Comma-separated list of endpoints to exclude from auditing
   
   ### Integration Points
   
   **Controller Integration:**
   - Modified `BaseControllerStarter.java` to register audit components with dependency injection
   - Added `AuditConfigManager` and `AuditRequestProcessor` to Guice bindings
   - Updated `ControllerAdminApiApplication.java` to register the `AuditLogFilter`
   
   **Logging Configuration:**
   - Updated `log4j2.xml` configurations to include dedicated audit logger (`org.apache.pinot.audit`)
   - Audit logs are structured JSON messages logged at INFO level
   
   ### Security and Privacy Features
   
   - **Sensitive Header Filtering**: Automatically excludes headers containing "auth", "password", "token", or "secret"
   - **Payload Size Limiting**: Configurable maximum payload size to prevent memory issues
   - **Endpoint Exclusion**: Wildcard-based endpoint filtering (supports patterns like `/health/*`)
   - **Graceful Degradation**: Audit logging failures never affect main request processing
   
   ### Request Processing Capabilities
   
   - **IP Address Extraction**: Supports proxy headers (X-Forwarded-For, X-Real-IP)
   - **User Identification**: Extracts user ID from Authorization headers and custom headers
   - **Service Identification**: Reads service ID from X-Service-ID or X-Service-Name headers
   - **Query Parameter Capture**: Always captures query parameters (lightweight operation)
   - **Request Body Capture**: Optional, configurable request body capture with size limits
   
   ## Implementation Choices
   
   ### Design Decisions
   
   1. **Dependency Injection Architecture**: Used Guice for dependency injection to enable easy testing and configuration management
   2. **Jersey Filter Integration**: Implemented as a JAX-RS ContainerRequestFilter for seamless integration with existing API infrastructure
   3. **SLF4J Logging**: Leveraged existing logging infrastructure for consistent log management
   4. **JSON Structured Logging**: Used Jackson ObjectMapper for consistent JSON serialization
   5. **Thread-Safe Configuration**: Implemented lock-free configuration management for high performance
   
   ### Configuration Strategy
   
   - **Dynamic Configuration Support**: Framework designed to support runtime configuration updates (foundation for future cluster config integration)
   - **Sensible Defaults**: Conservative defaults prioritize performance and security
   - **Flexible Filtering**: Wildcard-based endpoint exclusion for operational flexibility
   
   ### Error Handling
   
   - **Graceful Degradation**: Audit logging failures are logged separately and never impact request processing
   - **Defensive Programming**: Null checks and exception handling throughout the audit pipeline
   - **Fallback Values**: Default values for all extracted audit fields when extraction fails
   
   ## Evidence of Sufficient Testing
   
   ### Manual Testing Performed
   
   1. **Basic Functionality Testing**:
      - Verified audit logs are generated for Controller API requests
      - Confirmed JSON structure matches specification requirements
      - Tested endpoint exclusion with wildcard patterns
   
   2. **Configuration Testing**:
      - Validated all configuration properties work as expected
      - Tested enabling/disabling various capture options
      - Verified payload size limiting functionality
   
   3. **Security Testing**:
      - Confirmed sensitive headers are filtered out
      - Tested payload truncation with large request bodies
      - Verified graceful degradation when audit logging fails
   
   ### Integration Testing
   
   - **Controller Startup**: Verified controller starts successfully with audit framework enabled
   - **API Request Processing**: Confirmed normal API functionality is unaffected
   - **Log Output**: Validated structured JSON audit logs are written to configured appenders
   
   ### Build Testing
   
   ```bash
   # Clean build and test execution
   ./mvnw clean install -Pbin-dist
   # Passed all existing tests
   # No new test failures introduced
   ```
   
   ### Code Quality Checks
   
   ```bash
   # Code style verification
   ./mvnw checkstyle:check
   # License formatting
   ./mvnw license:format
   # Code formatting
   ./mvnw spotless:apply
   ```
   
   ## Technical Details
   
   ### Audit Event Schema
   
   Each audit event contains the following fields as JSON:
   ```json
   {
     "timestamp": "2025-01-15T10:30:00.000Z",
     "service_id": "pinot-controller",
     "endpoint": "/tables",
     "method": "POST",
     "origin_ip_address": "192.168.1.100",
     "user_id": "admin",
     "request": {
       "queryParameters": {...},
       "body": "...",
       "headers": {...}
     }
   }
   ```
   
   ### Performance Considerations
   
   - **Minimal Overhead**: Audit processing occurs only when enabled
   - **Efficient Filtering**: Endpoint exclusion check happens early to avoid unnecessary processing
   - **Configurable Payload Capture**: Request body capture is optional and size-limited
   - **Non-Blocking**: Audit logging uses existing SLF4J infrastructure for async processing
   
   ### Future Extension Points
   
   - **Cluster Configuration Integration**: Framework ready for dynamic config updates via ZooKeeper
   - **Additional Audit Fields**: Easy to extend AuditEvent class for additional metadata
   - **Custom Processors**: Pluggable audit processor architecture for custom audit logic
   - **Response Auditing**: Framework can be extended to audit response data
   
   ## Constants Added
   
   Added audit-related constants to `CommonConstants.java`:
   - `DEFAULT_AUDIT_LOG_MAX_PAYLOAD_SIZE`
   - `AUDIT_CONFIG_PREFIX` 
   - Additional configuration keys for future use
   
   ## Compatibility
   
   - **Backward Compatible**: No breaking changes to existing APIs
   - **Optional Feature**: Audit logging can be disabled without affecting functionality  
   - **Configuration Driven**: All audit behavior controlled through configuration
   - **Zero Impact**: When disabled, introduces negligible performance overhead
   
   ## Future Work
   
   This initial framework stub provides the foundation for audit logging in Apache Pinot. Subsequent pull requests will:
   
   - Enable the audit logging framework by default with appropriate configuration
   - Integrate with cluster configuration management for dynamic updates
   - Add comprehensive unit and integration tests
   - Enhance audit event fields based on operational requirements
   - Add performance optimizations and monitoring capabilities
   
   This implementation establishes the infrastructure while maintaining complete backward compatibility and zero impact on existing functionality.


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286748783


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();
+
+  /**
+   * Checks if the given endpoint should be excluded from audit logging.
+   * Supports simple wildcard matching with '*' character.
+   */
+  public static boolean isEndpointExcluded(String endpoint, String excludedEndpointsString) {
+    if (StringUtils.isBlank(endpoint) || StringUtils.isBlank(excludedEndpointsString)) {
+      return false;
+    }
+
+    Set<String> excludedEndpoints = parseExcludedEndpoints(excludedEndpointsString);
+    if (excludedEndpoints.isEmpty()) {
+      return false;
+    }
+
+    // Check for exact matches first
+    if (excludedEndpoints.contains(endpoint)) {
+      return true;
+    }
+
+    // Check for wildcard matches
+    for (String excluded : excludedEndpoints) {
+      if (excluded.contains("*")) {
+        if (matchesWildcard(endpoint, excluded)) {
+          return true;
+        }
+      }
+    }
+
+    return false;
+  }
+
+  private static Set<String> parseExcludedEndpoints(String excludedEndpointsString) {
+    Set<String> excludedEndpoints = new HashSet<>();
+    if (StringUtils.isNotBlank(excludedEndpointsString)) {
+      String[] endpoints = excludedEndpointsString.split(",");
+      for (String endpoint : endpoints) {
+        String trimmed = endpoint.trim();
+        if (StringUtils.isNotBlank(trimmed)) {
+          excludedEndpoints.add(trimmed);
+        }
+      }
+    }
+    return excludedEndpoints;
+  }
+
+  private static boolean matchesWildcard(String endpoint, String pattern) {

Review Comment:
   I think regex match might be an overkill. However, we can look into glob matching. However, I'd take that up in a separate PR in that case. Would that work? Else I can again, simplify this piece and remove it for now?
   
   Lmk what would what better 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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286638562


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();
+
+  /**
+   * Checks if the given endpoint should be excluded from audit logging.
+   * Supports simple wildcard matching with '*' character.
+   */
+  public static boolean isEndpointExcluded(String endpoint, String excludedEndpointsString) {

Review Comment:
   yes. The design is that Cluster Config is the default and only way of configuring audit logs.



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286173012


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();
+
+  /**
+   * Checks if the given endpoint should be excluded from audit logging.
+   * Supports simple wildcard matching with '*' character.
+   */
+  public static boolean isEndpointExcluded(String endpoint, String excludedEndpointsString) {

Review Comment:
   So the expectation here is that we'll expose these configurations through the cluster config. A user should be able to change these configurations on the fly. The cluster config will be the configuration that will be respected finally.



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on PR #16616:
URL: https://github.com/apache/pinot/pull/16616#issuecomment-3207529584

   @somandal done.


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286823667


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();
+
+  /**
+   * Checks if the given endpoint should be excluded from audit logging.
+   * Supports simple wildcard matching with '*' character.
+   */
+  public static boolean isEndpointExcluded(String endpoint, String excludedEndpointsString) {
+    if (StringUtils.isBlank(endpoint) || StringUtils.isBlank(excludedEndpointsString)) {
+      return false;
+    }
+
+    Set<String> excludedEndpoints = parseExcludedEndpoints(excludedEndpointsString);
+    if (excludedEndpoints.isEmpty()) {
+      return false;
+    }
+
+    // Check for exact matches first
+    if (excludedEndpoints.contains(endpoint)) {
+      return true;
+    }
+
+    // Check for wildcard matches
+    for (String excluded : excludedEndpoints) {
+      if (excluded.contains("*")) {
+        if (matchesWildcard(endpoint, excluded)) {
+          return true;
+        }
+      }
+    }
+
+    return false;
+  }
+
+  private static Set<String> parseExcludedEndpoints(String excludedEndpointsString) {
+    Set<String> excludedEndpoints = new HashSet<>();
+    if (StringUtils.isNotBlank(excludedEndpointsString)) {
+      String[] endpoints = excludedEndpointsString.split(",");
+      for (String endpoint : endpoints) {
+        String trimmed = endpoint.trim();
+        if (StringUtils.isNotBlank(trimmed)) {
+          excludedEndpoints.add(trimmed);
+        }
+      }
+    }
+    return excludedEndpoints;
+  }
+
+  private static boolean matchesWildcard(String endpoint, String pattern) {

Review Comment:
   yeah sure, feel free to address as a future change depending on how the requirements evolve. even just keeping it simple is fine, as long as the limitations are well documented 



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on PR #16616:
URL: https://github.com/apache/pinot/pull/16616#issuecomment-3207534430

   The integration test set error does not seem related to this change.
   
   https://productionresultssa7.blob.core.windows.net/actions-results/e64d0c48-8d40-402e-bde3-82ad08ce4438/workflow-job-run-f0fdf0c6-32f9-5bc9-bada-62d39548d2f6/logs/job/job-logs.txt?rsct=text%2Fplain&se=2025-08-20T18%3A19%3A53Z&sig=MkCEbmqk4GtoXxMImrvX5YrDvVeXbzeUu%2B0tSOhgqlM%3D&ske=2025-08-21T03%3A51%3A13Z&skoid=ca7593d4-ee42-46cd-af88-8b886a2f84eb&sks=b&skt=2025-08-20T15%3A51%3A13Z&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skv=2025-05-05&sp=r&spr=https&sr=b&st=2025-08-20T18%3A09%3A48Z&sv=2025-05-05
   
   <img width="1428" height="207" alt="Screenshot 2025-08-20 at 11 10 32 AM" src="https://github.com/user-attachments/assets/880e6afb-56a8-4539-963a-faed7a2f128c" />
   


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2285885394


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditLogger.java:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.pinot.common.audit;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for audit logging in Pinot components.
+ * Uses SLF4J with structured JSON logging format and supports dynamic configuration.
+ */
+public final class AuditLogger {
+
+  private static final String PINOT_AUDIT_LOGGER_NAME = "org.apache.pinot.audit";
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+  // Default Pinot logger. For logging failures in audit logging itself
+  private static final Logger LOG = LoggerFactory.getLogger(AuditLogger.class);
+  private static final Logger AUDIT_LOGGER = LoggerFactory.getLogger(PINOT_AUDIT_LOGGER_NAME);
+
+  private AuditLogger() {
+    // Private constructor to prevent instantiation
+  }
+
+  /**
+   * Logs an audit event as structured JSON at INFO level.
+   * Implements graceful degradation - audit logging failures will not propagate
+   * and will be logged separately for monitoring.
+   *
+   * @param auditEvent the audit event to log
+   */
+  public static void auditLog(AuditEvent auditEvent) {
+    if (auditEvent == null) {
+      return;
+    }
+
+    try {
+      String jsonLog = OBJECT_MAPPER.writeValueAsString(auditEvent);

Review Comment:
   though pretty-printing is probably more generic as it evolves, plus the request is an object



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286226067


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditLogger.java:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.pinot.common.audit;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for audit logging in Pinot components.
+ * Uses SLF4J with structured JSON logging format and supports dynamic configuration.
+ */
+public final class AuditLogger {
+
+  private static final String PINOT_AUDIT_LOGGER_NAME = "org.apache.pinot.audit";
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+  // Default Pinot logger. For logging failures in audit logging itself
+  private static final Logger LOG = LoggerFactory.getLogger(AuditLogger.class);
+  private static final Logger AUDIT_LOGGER = LoggerFactory.getLogger(PINOT_AUDIT_LOGGER_NAME);
+
+  private AuditLogger() {
+    // Private constructor to prevent instantiation
+  }
+
+  /**
+   * Logs an audit event as structured JSON at INFO level.
+   * Implements graceful degradation - audit logging failures will not propagate
+   * and will be logged separately for monitoring.
+   *
+   * @param auditEvent the audit event to log
+   */
+  public static void auditLog(AuditEvent auditEvent) {
+    if (auditEvent == null) {
+      return;
+    }
+
+    try {
+      String jsonLog = OBJECT_MAPPER.writeValueAsString(auditEvent);

Review Comment:
   got it, do you plan to make that change in this PR?



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on PR #16616:
URL: https://github.com/apache/pinot/pull/16616#issuecomment-3203756621

   @somandal 
   decided to remove pieces of individual logic which kinda need their own attention. I'll add them back in separate PRs. 
   Responded to your comments and resolved the ones in which I have removed the feature. 
   
   Thanks again for your thorough review.


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286168177


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();

Review Comment:
   Yes, that is basically the next PR. In this case, it's hard-coded config and it's set to false. This is the initial stub; subsequently, I’ll probably add a configuration listener so that we can change this on the fly with the configuration listener infra.



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal commented on PR #16616:
URL: https://github.com/apache/pinot/pull/16616#issuecomment-3203946999

   @suvodeep-pyne can you fix the test failures? thanks!


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286639696


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";

Review Comment:
   Updated the code. I am moving handling these cases to a separate PR. I want to keep this focussed on the audit log infra stub.



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";
+        }
+      }
+
+      // Check for custom user headers
+      String userHeader = requestContext.getHeaderString("X-User-ID");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      userHeader = requestContext.getHeaderString("X-Username");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      return ANONYMOUS;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract user ID", e);

Review Comment:
   Updated the code. I am moving handling these cases to a separate PR. I want to keep this focussed on the audit log infra stub.



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286636816


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditLogger.java:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.pinot.common.audit;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for audit logging in Pinot components.
+ * Uses SLF4J with structured JSON logging format and supports dynamic configuration.
+ */
+public final class AuditLogger {
+
+  private static final String PINOT_AUDIT_LOGGER_NAME = "org.apache.pinot.audit";
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+  // Default Pinot logger. For logging failures in audit logging itself
+  private static final Logger LOG = LoggerFactory.getLogger(AuditLogger.class);
+  private static final Logger AUDIT_LOGGER = LoggerFactory.getLogger(PINOT_AUDIT_LOGGER_NAME);
+
+  private AuditLogger() {
+    // Private constructor to prevent instantiation
+  }
+
+  /**
+   * Logs an audit event as structured JSON at INFO level.
+   * Implements graceful degradation - audit logging failures will not propagate
+   * and will be logged separately for monitoring.
+   *
+   * @param auditEvent the audit event to log
+   */
+  public static void auditLog(AuditEvent auditEvent) {
+    if (auditEvent == null) {
+      return;
+    }
+
+    try {
+      String jsonLog = OBJECT_MAPPER.writeValueAsString(auditEvent);

Review Comment:
   That is how it currently is actually. Here's a screenshot.
   
   <img width="1107" height="517" alt="Screenshot 2025-08-19 at 4 47 03 PM" src="https://github.com/user-attachments/assets/8b724d5a-d780-4114-bd3c-e3afef558d74" />
   



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.

codecov-commenter commented on PR #16616:
URL: https://github.com/apache/pinot/pull/16616#issuecomment-3192745815

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16616?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `0%` with `229 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 56.32%. Comparing base ([`d9e7317`](https://app.codecov.io/gh/apache/pinot/commit/d9e731782afc62149b487c27e80de4d809673853?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f09bce9`](https://app.codecov.io/gh/apache/pinot/commit/f09bce97b84e295d5436f9e36819777f87288c49?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 2 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16616?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...ache/pinot/common/audit/AuditRequestProcessor.java](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Faudit%2FAuditRequestProcessor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXVkaXQvQXVkaXRSZXF1ZXN0UHJvY2Vzc29yLmphdmE=) | 0.00% | [117 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [.../apache/pinot/common/audit/AuditConfigManager.java](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Faudit%2FAuditConfigManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXVkaXQvQXVkaXRDb25maWdNYW5hZ2VyLmphdmE=) | 0.00% | [36 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...java/org/apache/pinot/common/audit/AuditEvent.java](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Faudit%2FAuditEvent.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXVkaXQvQXVkaXRFdmVudC5qYXZh) | 0.00% | [32 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...ava/org/apache/pinot/common/audit/AuditConfig.java](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Faudit%2FAuditConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXVkaXQvQXVkaXRDb25maWcuamF2YQ==) | 0.00% | [22 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...ava/org/apache/pinot/common/audit/AuditLogger.java](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Faudit%2FAuditLogger.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXVkaXQvQXVkaXRMb2dnZXIuamF2YQ==) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [.../org/apache/pinot/common/audit/AuditLogFilter.java](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Faudit%2FAuditLogFilter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXVkaXQvQXVkaXRMb2dGaWx0ZXIuamF2YQ==) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Futils%2FCommonConstants.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (d9e7317) and HEAD (f09bce9). Click for more details.
   > 
   > <details><summary>HEAD has 32 uploads less than BASE</summary>
   >
   >| Flag | BASE (d9e7317) | HEAD (f09bce9) |
   >|------|------|------|
   >|unittests2|2|0|
   >|unittests|4|1|
   >|java-21|5|1|
   >|temurin|9|1|
   >|integration|5|0|
   >|integration1|2|0|
   >|custom-integration1|2|0|
   >|java-11|4|0|
   >|integration2|1|0|
   >|unittests1|2|1|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #16616      +/-   ##
   ============================================
   - Coverage     63.34%   56.32%   -7.02%     
   + Complexity     1379      699     -680     
   ============================================
     Files          3019     2370     -649     
     Lines        175600   130910   -44690     
     Branches      26918    20850    -6068     
   ============================================
   - Hits         111236    73737   -37499     
   + Misses        55874    51099    -4775     
   + Partials       8490     6074    -2416     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration1](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration2](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-11](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-21](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.32% <0.00%> (-6.99%)` | :arrow_down: |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.32% <0.00%> (-7.02%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.32% <0.00%> (-7.02%)` | :arrow_down: |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.32% <0.00%> (-0.12%)` | :arrow_down: |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16616/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16616?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286176663


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditLogger.java:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.pinot.common.audit;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for audit logging in Pinot components.
+ * Uses SLF4J with structured JSON logging format and supports dynamic configuration.
+ */
+public final class AuditLogger {
+
+  private static final String PINOT_AUDIT_LOGGER_NAME = "org.apache.pinot.audit";
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+  // Default Pinot logger. For logging failures in audit logging itself
+  private static final Logger LOG = LoggerFactory.getLogger(AuditLogger.class);
+  private static final Logger AUDIT_LOGGER = LoggerFactory.getLogger(PINOT_AUDIT_LOGGER_NAME);
+
+  private AuditLogger() {
+    // Private constructor to prevent instantiation
+  }
+
+  /**
+   * Logs an audit event as structured JSON at INFO level.
+   * Implements graceful degradation - audit logging failures will not propagate
+   * and will be logged separately for monitoring.
+   *
+   * @param auditEvent the audit event to log
+   */
+  public static void auditLog(AuditEvent auditEvent) {
+    if (auditEvent == null) {
+      return;
+    }
+
+    try {
+      String jsonLog = OBJECT_MAPPER.writeValueAsString(auditEvent);

Review Comment:
   The convention I'm seeing is to log these into an NDJSON structure where it's basically a single JSON per line.



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286228146


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();
+
+  /**
+   * Checks if the given endpoint should be excluded from audit logging.
+   * Supports simple wildcard matching with '*' character.
+   */
+  public static boolean isEndpointExcluded(String endpoint, String excludedEndpointsString) {

Review Comment:
   got it, so in that case do we need to make this public facing? we'd expect users to always update the cluster config, right?



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286664388


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";
+        }
+      }
+
+      // Check for custom user headers
+      String userHeader = requestContext.getHeaderString("X-User-ID");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      userHeader = requestContext.getHeaderString("X-Username");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      return ANONYMOUS;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract user ID", e);
+      return ANONYMOUS;
+    }
+  }
+
+  /**
+   * Extracts service ID from request headers.
+   * Service ID should be provided by the client in headers, not from configuration.
+   *
+   * @param requestContext the container request context
+   * @return the service ID or "unknown" if not found
+   */
+  private String extractServiceId(ContainerRequestContext requestContext) {
+    try {
+      // Check for custom service ID headers
+      String serviceId = requestContext.getHeaderString("X-Service-ID");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      serviceId = requestContext.getHeaderString("X-Service-Name");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      return "unknown";
+    } catch (Exception e) {
+      LOG.debug("Failed to extract service ID", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Captures the request payload for audit logging based on configuration.
+   * Uses dynamic configuration to control what data is captured.
+   *
+   * @param requestContext the request context
+   * @return the captured request payload
+   */
+  private Object captureRequestPayload(ContainerRequestContext requestContext) {
+    // Get current configuration (fallback to defaults if no config manager)
+    AuditConfig config = _configManager != null ? _configManager.getCurrentConfig() : new AuditConfig();

Review Comment:
   No. fixed this. ConfigManager should be present always.



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2285860930


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";
+        }
+      }
+
+      // Check for custom user headers
+      String userHeader = requestContext.getHeaderString("X-User-ID");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      userHeader = requestContext.getHeaderString("X-Username");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      return ANONYMOUS;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract user ID", e);
+      return ANONYMOUS;
+    }
+  }
+
+  /**
+   * Extracts service ID from request headers.
+   * Service ID should be provided by the client in headers, not from configuration.
+   *
+   * @param requestContext the container request context
+   * @return the service ID or "unknown" if not found
+   */
+  private String extractServiceId(ContainerRequestContext requestContext) {
+    try {
+      // Check for custom service ID headers
+      String serviceId = requestContext.getHeaderString("X-Service-ID");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      serviceId = requestContext.getHeaderString("X-Service-Name");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      return "unknown";
+    } catch (Exception e) {
+      LOG.debug("Failed to extract service ID", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Captures the request payload for audit logging based on configuration.
+   * Uses dynamic configuration to control what data is captured.
+   *
+   * @param requestContext the request context
+   * @return the captured request payload
+   */
+  private Object captureRequestPayload(ContainerRequestContext requestContext) {
+    // Get current configuration (fallback to defaults if no config manager)
+    AuditConfig config = _configManager != null ? _configManager.getCurrentConfig() : new AuditConfig();
+
+    Map<String, Object> payload = new HashMap<>();
+
+    try {
+      // Always capture query parameters (lightweight)
+      UriInfo uriInfo = requestContext.getUriInfo();
+      MultivaluedMap<String, String> queryParams = uriInfo.getQueryParameters();
+      if (!queryParams.isEmpty()) {
+        Map<String, Object> queryMap = new HashMap<>();
+        for (Map.Entry<String, List<String>> entry : queryParams.entrySet()) {
+          List<String> values = entry.getValue();
+          if (values.size() == 1) {
+            queryMap.put(entry.getKey(), values.get(0));
+          } else {
+            queryMap.put(entry.getKey(), values);
+          }
+        }
+        payload.put("queryParameters", queryMap);
+      }
+
+      // Conditionally capture request body based on configuration
+      if (config.isCaptureRequestPayload() && requestContext.hasEntity()) {
+        String requestBody = readRequestBody(requestContext, config.getMaxPayloadSize());
+        if (StringUtils.isNotBlank(requestBody)) {
+          payload.put("body", requestBody);
+        }
+      }
+
+      // Conditionally capture headers based on configuration
+      if (config.isCaptureRequestHeaders()) {
+        MultivaluedMap<String, String> headers = requestContext.getHeaders();
+        if (!headers.isEmpty()) {
+          Map<String, String> headerMap = new HashMap<>();
+          for (Map.Entry<String, List<String>> entry : headers.entrySet()) {
+            String headerName = entry.getKey().toLowerCase();
+            // Skip sensitive headers
+            if (!headerName.contains("auth") && !headerName.contains("password") && !headerName.contains("token")
+                && !headerName.contains("secret")) {
+              List<String> values = entry.getValue();
+              if (!values.isEmpty()) {
+                headerMap.put(entry.getKey(), values.get(0));
+              }
+            }
+          }
+          if (!headerMap.isEmpty()) {
+            payload.put("headers", headerMap);
+          }
+        }
+      }
+    } catch (Exception e) {
+      LOG.debug("Failed to capture request payload", e);
+      payload.put("error", "Failed to capture payload: " + e.getMessage());
+    }
+
+    return payload.isEmpty() ? null : payload;
+  }
+
+  /**
+   * Reads the request body from the entity input stream.
+   * Restores the input stream for downstream processing.
+   * Limits the amount of data read based on configuration.
+   *
+   * @param requestContext the request context
+   * @param maxPayloadSize maximum bytes to read from the request body
+   * @return the request body as string (potentially truncated)
+   */
+  private String readRequestBody(ContainerRequestContext requestContext, int maxPayloadSize) {
+    try {
+      InputStream entityStream = requestContext.getEntityStream();
+      if (entityStream == null) {
+        return null;
+      }
+
+      // Read the stream content with size limit
+      ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+      byte[] data = new byte[8192];
+      int bytesRead;
+      int totalBytesRead = 0;
+      boolean truncated = false;
+
+      while ((bytesRead = entityStream.read(data, 0, data.length)) != -1) {
+        if (totalBytesRead + bytesRead > maxPayloadSize) {
+          // Truncate to max payload size
+          int remainingBytes = maxPayloadSize - totalBytesRead;
+          if (remainingBytes > 0) {
+            buffer.write(data, 0, remainingBytes);
+          }
+          truncated = true;
+          break;
+        }
+        buffer.write(data, 0, bytesRead);
+        totalBytesRead += bytesRead;
+      }
+
+      byte[] requestBodyBytes = buffer.toByteArray();
+      String requestBody = new String(requestBodyBytes, StandardCharsets.UTF_8);
+
+      // Add truncation indicator if needed
+      if (truncated) {
+        requestBody += " [TRUNCATED - exceeds " + maxPayloadSize + " byte limit]";
+      }
+
+      // Restore the input stream for downstream processing
+      // Need to read the entire original stream to restore it properly
+      if (truncated) {
+        // Read remaining bytes to fully consume the original stream
+        while (entityStream.read(data) != -1) {
+          // Consume remaining bytes
+        }
+        // Create new stream with original data (this is complex, for now keep the truncated version)
+        requestContext.setEntityStream(new ByteArrayInputStream(requestBodyBytes));
+      } else {
+        requestContext.setEntityStream(new ByteArrayInputStream(requestBodyBytes));
+      }
+
+      return requestBody;
+    } catch (IOException e) {
+      LOG.debug("Failed to read request body", e);
+      return "Failed to read request body: " + e.getMessage();

Review Comment:
   nit: can avoid duplication by storing the string in a variable and reusing it



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";
+        }
+      }
+
+      // Check for custom user headers
+      String userHeader = requestContext.getHeaderString("X-User-ID");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      userHeader = requestContext.getHeaderString("X-Username");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      return ANONYMOUS;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract user ID", e);
+      return ANONYMOUS;
+    }
+  }
+
+  /**
+   * Extracts service ID from request headers.
+   * Service ID should be provided by the client in headers, not from configuration.
+   *
+   * @param requestContext the container request context
+   * @return the service ID or "unknown" if not found
+   */
+  private String extractServiceId(ContainerRequestContext requestContext) {
+    try {
+      // Check for custom service ID headers
+      String serviceId = requestContext.getHeaderString("X-Service-ID");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      serviceId = requestContext.getHeaderString("X-Service-Name");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      return "unknown";
+    } catch (Exception e) {
+      LOG.debug("Failed to extract service ID", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Captures the request payload for audit logging based on configuration.
+   * Uses dynamic configuration to control what data is captured.
+   *
+   * @param requestContext the request context
+   * @return the captured request payload
+   */
+  private Object captureRequestPayload(ContainerRequestContext requestContext) {
+    // Get current configuration (fallback to defaults if no config manager)
+    AuditConfig config = _configManager != null ? _configManager.getCurrentConfig() : new AuditConfig();

Review Comment:
   do we expect `_configManager` to be null? if so, won't other functions in this class need null handling as well?



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();
+
+  /**
+   * Checks if the given endpoint should be excluded from audit logging.
+   * Supports simple wildcard matching with '*' character.
+   */
+  public static boolean isEndpointExcluded(String endpoint, String excludedEndpointsString) {

Review Comment:
   do we expect use cases where callers pass their own `excludedEndpointsString` rather than using the one in the `AuditConfig`?



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();

Review Comment:
   is my understanding correct that this is just doing this for now as the code is disabled? in the future this will be registered with the config change listener with Helix and then the AuditConfig may be changeable. And that this will also be initialized with the controller config on start up?



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfig.java:
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.pinot.common.audit;
+
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+
+/**
+ * Pure data class for audit logging configuration.
+ * Uses Jackson annotations for automatic JSON mapping.
+ */
+@JsonIgnoreProperties(ignoreUnknown = true)
+public final class AuditConfig {

Review Comment:
   do you want to add a constructor for this class?



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";
+        }
+      }
+
+      // Check for custom user headers
+      String userHeader = requestContext.getHeaderString("X-User-ID");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      userHeader = requestContext.getHeaderString("X-Username");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      return ANONYMOUS;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract user ID", e);
+      return ANONYMOUS;
+    }
+  }
+
+  /**
+   * Extracts service ID from request headers.
+   * Service ID should be provided by the client in headers, not from configuration.
+   *
+   * @param requestContext the container request context
+   * @return the service ID or "unknown" if not found
+   */
+  private String extractServiceId(ContainerRequestContext requestContext) {
+    try {
+      // Check for custom service ID headers
+      String serviceId = requestContext.getHeaderString("X-Service-ID");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      serviceId = requestContext.getHeaderString("X-Service-Name");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      return "unknown";
+    } catch (Exception e) {
+      LOG.debug("Failed to extract service ID", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Captures the request payload for audit logging based on configuration.
+   * Uses dynamic configuration to control what data is captured.
+   *
+   * @param requestContext the request context
+   * @return the captured request payload
+   */
+  private Object captureRequestPayload(ContainerRequestContext requestContext) {
+    // Get current configuration (fallback to defaults if no config manager)
+    AuditConfig config = _configManager != null ? _configManager.getCurrentConfig() : new AuditConfig();
+
+    Map<String, Object> payload = new HashMap<>();
+
+    try {
+      // Always capture query parameters (lightweight)
+      UriInfo uriInfo = requestContext.getUriInfo();
+      MultivaluedMap<String, String> queryParams = uriInfo.getQueryParameters();
+      if (!queryParams.isEmpty()) {
+        Map<String, Object> queryMap = new HashMap<>();
+        for (Map.Entry<String, List<String>> entry : queryParams.entrySet()) {
+          List<String> values = entry.getValue();
+          if (values.size() == 1) {
+            queryMap.put(entry.getKey(), values.get(0));
+          } else {
+            queryMap.put(entry.getKey(), values);
+          }
+        }
+        payload.put("queryParameters", queryMap);
+      }
+
+      // Conditionally capture request body based on configuration
+      if (config.isCaptureRequestPayload() && requestContext.hasEntity()) {
+        String requestBody = readRequestBody(requestContext, config.getMaxPayloadSize());
+        if (StringUtils.isNotBlank(requestBody)) {
+          payload.put("body", requestBody);
+        }
+      }
+
+      // Conditionally capture headers based on configuration
+      if (config.isCaptureRequestHeaders()) {
+        MultivaluedMap<String, String> headers = requestContext.getHeaders();
+        if (!headers.isEmpty()) {
+          Map<String, String> headerMap = new HashMap<>();
+          for (Map.Entry<String, List<String>> entry : headers.entrySet()) {
+            String headerName = entry.getKey().toLowerCase();
+            // Skip sensitive headers
+            if (!headerName.contains("auth") && !headerName.contains("password") && !headerName.contains("token")
+                && !headerName.contains("secret")) {
+              List<String> values = entry.getValue();
+              if (!values.isEmpty()) {
+                headerMap.put(entry.getKey(), values.get(0));
+              }
+            }
+          }
+          if (!headerMap.isEmpty()) {
+            payload.put("headers", headerMap);
+          }
+        }
+      }
+    } catch (Exception e) {
+      LOG.debug("Failed to capture request payload", e);
+      payload.put("error", "Failed to capture payload: " + e.getMessage());
+    }
+
+    return payload.isEmpty() ? null : payload;
+  }
+
+  /**
+   * Reads the request body from the entity input stream.
+   * Restores the input stream for downstream processing.
+   * Limits the amount of data read based on configuration.
+   *
+   * @param requestContext the request context
+   * @param maxPayloadSize maximum bytes to read from the request body
+   * @return the request body as string (potentially truncated)
+   */
+  private String readRequestBody(ContainerRequestContext requestContext, int maxPayloadSize) {
+    try {
+      InputStream entityStream = requestContext.getEntityStream();
+      if (entityStream == null) {
+        return null;
+      }
+
+      // Read the stream content with size limit
+      ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+      byte[] data = new byte[8192];
+      int bytesRead;
+      int totalBytesRead = 0;
+      boolean truncated = false;
+
+      while ((bytesRead = entityStream.read(data, 0, data.length)) != -1) {
+        if (totalBytesRead + bytesRead > maxPayloadSize) {
+          // Truncate to max payload size
+          int remainingBytes = maxPayloadSize - totalBytesRead;
+          if (remainingBytes > 0) {
+            buffer.write(data, 0, remainingBytes);
+          }
+          truncated = true;
+          break;
+        }
+        buffer.write(data, 0, bytesRead);
+        totalBytesRead += bytesRead;
+      }
+
+      byte[] requestBodyBytes = buffer.toByteArray();
+      String requestBody = new String(requestBodyBytes, StandardCharsets.UTF_8);
+
+      // Add truncation indicator if needed
+      if (truncated) {
+        requestBody += " [TRUNCATED - exceeds " + maxPayloadSize + " byte limit]";
+      }

Review Comment:
   nit: can this be moved into the next if block which checks for "if (truncated)" anyways?



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfigManager.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.util.HashSet;
+import java.util.Set;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Thread-safe configuration manager for audit logging settings.
+ * Handles dynamic configuration updates from cluster configuration changes.
+ * Self-registers with the provided cluster config provider.
+ */
+@Singleton
+public final class AuditConfigManager {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditConfigManager.class);
+
+  private final AuditConfig _currentConfig = new AuditConfig();
+
+  /**
+   * Checks if the given endpoint should be excluded from audit logging.
+   * Supports simple wildcard matching with '*' character.
+   */
+  public static boolean isEndpointExcluded(String endpoint, String excludedEndpointsString) {
+    if (StringUtils.isBlank(endpoint) || StringUtils.isBlank(excludedEndpointsString)) {
+      return false;
+    }
+
+    Set<String> excludedEndpoints = parseExcludedEndpoints(excludedEndpointsString);
+    if (excludedEndpoints.isEmpty()) {
+      return false;
+    }
+
+    // Check for exact matches first
+    if (excludedEndpoints.contains(endpoint)) {
+      return true;
+    }
+
+    // Check for wildcard matches
+    for (String excluded : excludedEndpoints) {
+      if (excluded.contains("*")) {
+        if (matchesWildcard(endpoint, excluded)) {
+          return true;
+        }
+      }
+    }
+
+    return false;
+  }
+
+  private static Set<String> parseExcludedEndpoints(String excludedEndpointsString) {
+    Set<String> excludedEndpoints = new HashSet<>();
+    if (StringUtils.isNotBlank(excludedEndpointsString)) {
+      String[] endpoints = excludedEndpointsString.split(",");
+      for (String endpoint : endpoints) {
+        String trimmed = endpoint.trim();
+        if (StringUtils.isNotBlank(trimmed)) {
+          excludedEndpoints.add(trimmed);
+        }
+      }
+    }
+    return excludedEndpoints;
+  }
+
+  private static boolean matchesWildcard(String endpoint, String pattern) {

Review Comment:
   does this mean that the wildcard can't be somewhere in the middle? like "end*point"? Maybe have a comment somewhere that calls out allowed patterns?
   
   Just wondering if it makes sense to use the `Pattern` class here to compile the pattern and then search for the match? It might be overkill if we don't expect matches other than what's mentioned here.



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";

Review Comment:
   nit: create static variables for these?



##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";
+        }
+      }
+
+      // Check for custom user headers
+      String userHeader = requestContext.getHeaderString("X-User-ID");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      userHeader = requestContext.getHeaderString("X-Username");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      return ANONYMOUS;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract user ID", e);

Review Comment:
   should this be warn? same question for other functions



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2285879099


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditLogger.java:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.pinot.common.audit;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for audit logging in Pinot components.
+ * Uses SLF4J with structured JSON logging format and supports dynamic configuration.
+ */
+public final class AuditLogger {
+
+  private static final String PINOT_AUDIT_LOGGER_NAME = "org.apache.pinot.audit";
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+  // Default Pinot logger. For logging failures in audit logging itself
+  private static final Logger LOG = LoggerFactory.getLogger(AuditLogger.class);
+  private static final Logger AUDIT_LOGGER = LoggerFactory.getLogger(PINOT_AUDIT_LOGGER_NAME);
+
+  private AuditLogger() {
+    // Private constructor to prevent instantiation
+  }
+
+  /**
+   * Logs an audit event as structured JSON at INFO level.
+   * Implements graceful degradation - audit logging failures will not propagate
+   * and will be logged separately for monitoring.
+   *
+   * @param auditEvent the audit event to log
+   */
+  public static void auditLog(AuditEvent auditEvent) {
+    if (auditEvent == null) {
+      return;
+    }
+
+    try {
+      String jsonLog = OBJECT_MAPPER.writeValueAsString(auditEvent);

Review Comment:
   i think this is okay for now. do we need to pretty-print the JSON? just wondering if it'll be more efficient to add a "toString" function there and use that to print instead?



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286166338


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditConfig.java:
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.pinot.common.audit;
+
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+
+/**
+ * Pure data class for audit logging configuration.
+ * Uses Jackson annotations for automatic JSON mapping.
+ */
+@JsonIgnoreProperties(ignoreUnknown = true)
+public final class AuditConfig {

Review Comment:
   Plan is to not have any constructors here; rather, rely on utilities to translate the `ClusterConfiguration` to the audit config class.



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286822663


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditLogger.java:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.pinot.common.audit;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for audit logging in Pinot components.
+ * Uses SLF4J with structured JSON logging format and supports dynamic configuration.
+ */
+public final class AuditLogger {
+
+  private static final String PINOT_AUDIT_LOGGER_NAME = "org.apache.pinot.audit";
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+  // Default Pinot logger. For logging failures in audit logging itself
+  private static final Logger LOG = LoggerFactory.getLogger(AuditLogger.class);
+  private static final Logger AUDIT_LOGGER = LoggerFactory.getLogger(PINOT_AUDIT_LOGGER_NAME);
+
+  private AuditLogger() {
+    // Private constructor to prevent instantiation
+  }
+
+  /**
+   * Logs an audit event as structured JSON at INFO level.
+   * Implements graceful degradation - audit logging failures will not propagate
+   * and will be logged separately for monitoring.
+   *
+   * @param auditEvent the audit event to log
+   */
+  public static void auditLog(AuditEvent auditEvent) {
+    if (auditEvent == null) {
+      return;
+    }
+
+    try {
+      String jsonLog = OBJECT_MAPPER.writeValueAsString(auditEvent);

Review Comment:
   cool, thanks for the screenshot



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "somandal (via GitHub)" <gi...@apache.org>.

somandal merged PR #16616:
URL: https://github.com/apache/pinot/pull/16616


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add initial configs, classes and stubs for audit logging support. Disabled by default [pinot]

Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.

suvodeep-pyne commented on code in PR #16616:
URL: https://github.com/apache/pinot/pull/16616#discussion_r2286663230


##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -0,0 +1,317 @@
+/**
+ * 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.pinot.common.audit;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class for extracting audit information from Jersey HTTP requests.
+ * Handles all the complex logic for IP address extraction, user identification,
+ * and request payload capture for audit logging purposes.
+ * Uses dynamic configuration to control audit behavior.
+ */
+@Singleton
+public class AuditRequestProcessor {
+
+  private static final Logger LOG = LoggerFactory.getLogger(AuditRequestProcessor.class);
+  private static final String ANONYMOUS = "anonymous";
+
+  @Inject
+  private AuditConfigManager _configManager;
+
+  public AuditEvent processRequest(ContainerRequestContext requestContext, String remoteAddr) {
+    // Check if auditing is enabled (if config manager is available)
+    if (!isEnabled()) {
+      return null;
+    }
+
+    try {
+      UriInfo uriInfo = requestContext.getUriInfo();
+      String endpoint = uriInfo.getPath();
+
+      // Check endpoint exclusions
+      if (_configManager.isEndpointExcluded(endpoint)) {
+        return null;
+      }
+
+      String method = requestContext.getMethod();
+      String originIpAddress = extractClientIpAddress(requestContext, remoteAddr);
+      String userId = extractUserId(requestContext);
+
+      // Capture request payload based on configuration
+      Object requestPayload = captureRequestPayload(requestContext);
+
+      // Log the audit event (service ID will be extracted from headers, not config)
+      return new AuditEvent(extractServiceId(requestContext), endpoint, method, originIpAddress, userId,
+          requestPayload);
+    } catch (Exception e) {
+      // Graceful degradation: Never let audit logging failures affect the main request
+      LOG.warn("Failed to process audit logging for request", e);
+    }
+    return null;
+  }
+
+  public boolean isEnabled() {
+    return _configManager.isEnabled();
+  }
+
+  /**
+   * Extracts the client IP address from the request.
+   * Checks common proxy headers before falling back to remote address.
+   *
+   * @param requestContext the container request context
+   * @param remoteAddr the remote address from the underlying request
+   * @return the client IP address
+   */
+  private String extractClientIpAddress(ContainerRequestContext requestContext, String remoteAddr) {
+    try {
+      // Check for proxy headers first
+      String xForwardedFor = requestContext.getHeaderString("X-Forwarded-For");
+      if (StringUtils.isNotBlank(xForwardedFor)) {
+        // X-Forwarded-For can contain multiple IPs, take the first one
+        return xForwardedFor.split(",")[0].trim();
+      }
+
+      String xRealIp = requestContext.getHeaderString("X-Real-IP");
+      if (StringUtils.isNotBlank(xRealIp)) {
+        return xRealIp.trim();
+      }
+
+      // Fall back to remote address
+      return remoteAddr;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract client IP address", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Extracts user ID from request headers.
+   * Looks for common authentication headers.
+   *
+   * @param requestContext the container request context
+   * @return the user ID or "anonymous" if not found
+   */
+  private String extractUserId(ContainerRequestContext requestContext) {
+    try {
+      // Check for common user identification headers
+      String authHeader = requestContext.getHeaderString("Authorization");
+      if (StringUtils.isNotBlank(authHeader)) {
+        // For basic auth, extract username; for bearer tokens, use a placeholder
+        if (authHeader.startsWith("Basic ")) {
+          // Could decode basic auth to get username, but for security keep it as placeholder
+          return "basic-auth-user";
+        } else if (authHeader.startsWith("Bearer ")) {
+          return "bearer-token-user";
+        }
+      }
+
+      // Check for custom user headers
+      String userHeader = requestContext.getHeaderString("X-User-ID");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      userHeader = requestContext.getHeaderString("X-Username");
+      if (StringUtils.isNotBlank(userHeader)) {
+        return userHeader.trim();
+      }
+
+      return ANONYMOUS;
+    } catch (Exception e) {
+      LOG.debug("Failed to extract user ID", e);
+      return ANONYMOUS;
+    }
+  }
+
+  /**
+   * Extracts service ID from request headers.
+   * Service ID should be provided by the client in headers, not from configuration.
+   *
+   * @param requestContext the container request context
+   * @return the service ID or "unknown" if not found
+   */
+  private String extractServiceId(ContainerRequestContext requestContext) {
+    try {
+      // Check for custom service ID headers
+      String serviceId = requestContext.getHeaderString("X-Service-ID");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      serviceId = requestContext.getHeaderString("X-Service-Name");
+      if (StringUtils.isNotBlank(serviceId)) {
+        return serviceId.trim();
+      }
+
+      return "unknown";
+    } catch (Exception e) {
+      LOG.debug("Failed to extract service ID", e);
+      return "unknown";
+    }
+  }
+
+  /**
+   * Captures the request payload for audit logging based on configuration.
+   * Uses dynamic configuration to control what data is captured.
+   *
+   * @param requestContext the request context
+   * @return the captured request payload
+   */
+  private Object captureRequestPayload(ContainerRequestContext requestContext) {
+    // Get current configuration (fallback to defaults if no config manager)
+    AuditConfig config = _configManager != null ? _configManager.getCurrentConfig() : new AuditConfig();
+
+    Map<String, Object> payload = new HashMap<>();
+
+    try {
+      // Always capture query parameters (lightweight)
+      UriInfo uriInfo = requestContext.getUriInfo();
+      MultivaluedMap<String, String> queryParams = uriInfo.getQueryParameters();
+      if (!queryParams.isEmpty()) {
+        Map<String, Object> queryMap = new HashMap<>();
+        for (Map.Entry<String, List<String>> entry : queryParams.entrySet()) {
+          List<String> values = entry.getValue();
+          if (values.size() == 1) {
+            queryMap.put(entry.getKey(), values.get(0));
+          } else {
+            queryMap.put(entry.getKey(), values);
+          }
+        }
+        payload.put("queryParameters", queryMap);
+      }
+
+      // Conditionally capture request body based on configuration
+      if (config.isCaptureRequestPayload() && requestContext.hasEntity()) {
+        String requestBody = readRequestBody(requestContext, config.getMaxPayloadSize());
+        if (StringUtils.isNotBlank(requestBody)) {
+          payload.put("body", requestBody);
+        }
+      }
+
+      // Conditionally capture headers based on configuration
+      if (config.isCaptureRequestHeaders()) {
+        MultivaluedMap<String, String> headers = requestContext.getHeaders();
+        if (!headers.isEmpty()) {
+          Map<String, String> headerMap = new HashMap<>();
+          for (Map.Entry<String, List<String>> entry : headers.entrySet()) {
+            String headerName = entry.getKey().toLowerCase();
+            // Skip sensitive headers
+            if (!headerName.contains("auth") && !headerName.contains("password") && !headerName.contains("token")
+                && !headerName.contains("secret")) {
+              List<String> values = entry.getValue();
+              if (!values.isEmpty()) {
+                headerMap.put(entry.getKey(), values.get(0));
+              }
+            }
+          }
+          if (!headerMap.isEmpty()) {
+            payload.put("headers", headerMap);
+          }
+        }
+      }
+    } catch (Exception e) {
+      LOG.debug("Failed to capture request payload", e);
+      payload.put("error", "Failed to capture payload: " + e.getMessage());
+    }
+
+    return payload.isEmpty() ? null : payload;
+  }
+
+  /**
+   * Reads the request body from the entity input stream.
+   * Restores the input stream for downstream processing.
+   * Limits the amount of data read based on configuration.
+   *
+   * @param requestContext the request context
+   * @param maxPayloadSize maximum bytes to read from the request body
+   * @return the request body as string (potentially truncated)
+   */
+  private String readRequestBody(ContainerRequestContext requestContext, int maxPayloadSize) {
+    try {
+      InputStream entityStream = requestContext.getEntityStream();
+      if (entityStream == null) {
+        return null;
+      }
+
+      // Read the stream content with size limit
+      ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+      byte[] data = new byte[8192];
+      int bytesRead;
+      int totalBytesRead = 0;
+      boolean truncated = false;
+
+      while ((bytesRead = entityStream.read(data, 0, data.length)) != -1) {
+        if (totalBytesRead + bytesRead > maxPayloadSize) {
+          // Truncate to max payload size
+          int remainingBytes = maxPayloadSize - totalBytesRead;
+          if (remainingBytes > 0) {
+            buffer.write(data, 0, remainingBytes);
+          }
+          truncated = true;
+          break;
+        }
+        buffer.write(data, 0, bytesRead);
+        totalBytesRead += bytesRead;
+      }
+
+      byte[] requestBodyBytes = buffer.toByteArray();
+      String requestBody = new String(requestBodyBytes, StandardCharsets.UTF_8);
+
+      // Add truncation indicator if needed
+      if (truncated) {
+        requestBody += " [TRUNCATED - exceeds " + maxPayloadSize + " byte limit]";
+      }

Review Comment:
   There are actually some nuances here that is worth heavy testing. Planning to merge in the structure first before tackling this.



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org