yuqi1129 commented on code in PR #11618: URL: https://github.com/apache/gravitino/pull/11618#discussion_r3411840785
########## server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java: ########## @@ -0,0 +1,295 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.server.web; + +import java.io.IOException; +import java.security.Principal; +import java.util.Optional; +import javax.annotation.Nullable; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpServletResponseWrapper; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.auth.AuthConstants; +import org.apache.gravitino.listener.EventBus; +import org.apache.gravitino.listener.api.event.EventSource; +import org.apache.gravitino.listener.api.event.server.HttpRequestFailureEvent; +import org.apache.gravitino.utils.RequestContext; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A servlet filter that emits an {@link HttpRequestFailureEvent} for every HTTP request that + * completes with a 4xx or 5xx status code and for which no operation-layer failure event was + * already dispatched on the same request thread. + * + * <p><strong>Filter chain position:</strong> this filter must be registered <em>after</em> {@link + * RequestContextFilter} (so the remote address is already populated) but <em>before</em> {@link + * org.apache.gravitino.server.authentication.AuthenticationFilter} (so it observes 401 + * authentication failures as well as downstream 403 authorization denials). It is safe to install + * on servers that do not use {@code RequestContextFilter} (Iceberg REST, Lance REST) because it + * resolves the client address independently. + * + * <p><strong>Double-logging prevention:</strong> when an operation-layer failure event (e.g. {@code + * LoadTableFailureEvent}, {@code AuthorizationDenialFailureEvent}) has already been dispatched via + * {@link EventBus}, {@link RequestContext#markOperationFailureFired()} is set on the request + * thread. This filter checks that flag in the {@code finally} block and skips emitting its own + * {@link HttpRequestFailureEvent} if the flag is set. + * + * <p><strong>Success + 5xx edge case:</strong> if an operation dispatcher emits a {@code + * SuccessEvent} (flag not set) but the HTTP layer subsequently fails with a 5xx (e.g. JSON + * serialization error), this filter will emit an {@link HttpRequestFailureEvent} in addition to the + * success event already in the audit log. Both entries are correct — the operation itself succeeded + * but the response delivery failed — and are intentionally preserved. + * + * <p><strong>Health check exclusion:</strong> requests matched by the configured {@link + * HealthCheckPathMatcher} are silently passed through without audit logging to avoid polluting the + * audit log with probe traffic. Pass a server-specific subclass (e.g. {@code + * IcebergHealthCheckPathMatcher}) when constructing this filter for a server that defines + * additional health endpoints. + * + * <p><strong>Exception escape:</strong> if an uncaught {@link Throwable} escapes from the filter + * chain and the captured status is still 200 (i.e. no downstream component set an error code), the + * captured status is promoted to 500 before the {@code finally} block runs, ensuring that the audit + * event reflects the actual error condition. + */ +public class HttpAuditFilter implements Filter { + + private static final Logger LOG = LoggerFactory.getLogger(HttpAuditFilter.class); + private static final String X_FORWARDED_FOR = "X-Forwarded-For"; + + private final Optional<EventBus> eventBus; + private final EventSource eventSource; + private final HealthCheckPathMatcher healthCheckMatcher; + + /** + * Constructs an {@code HttpAuditFilter} using the default {@link HealthCheckPathMatcher}. + * + * @param eventBus the event bus used to dispatch {@link HttpRequestFailureEvent}s; may be {@code + * null}, in which case the filter is a pass-through no-op (useful when no audit listener is + * configured). + * @param eventSource identifies which server this filter instance is installed on; included in + * every emitted {@link HttpRequestFailureEvent}. + */ + public HttpAuditFilter(@Nullable EventBus eventBus, EventSource eventSource) { + this(eventBus, eventSource, new HealthCheckPathMatcher()); + } + + /** + * Constructs an {@code HttpAuditFilter} with a custom {@link HealthCheckPathMatcher}. + * + * @param eventBus the event bus used to dispatch {@link HttpRequestFailureEvent}s; may be {@code + * null}, in which case the filter is a pass-through no-op. + * @param eventSource identifies which server this filter instance is installed on. + * @param healthCheckMatcher determines which URI paths are health check probes; those paths are + * excluded from audit logging to avoid polluting the audit log with probe traffic. + */ + public HttpAuditFilter( + @Nullable EventBus eventBus, + EventSource eventSource, + HealthCheckPathMatcher healthCheckMatcher) { + this.eventBus = Optional.ofNullable(eventBus); + this.eventSource = eventSource; + this.healthCheckMatcher = healthCheckMatcher; + } + + @Override + public void init(FilterConfig filterConfig) {} + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + // Pass through when audit is not configured or request is not HTTP. + if (eventBus.isEmpty() || !(request instanceof HttpServletRequest)) { + chain.doFilter(request, response); + return; + } + + HttpServletRequest httpRequest = (HttpServletRequest) request; + // Defensive cleanup at request entry in case a pooled thread leaked stale state. + RequestContext.resetOperationFailureFired(); + if (healthCheckMatcher.isHealthCheckPath(httpRequest.getRequestURI())) { + try { + chain.doFilter(request, response); + } finally { + RequestContext.resetOperationFailureFired(); + } + return; + } + + StatusCapturingResponseWrapper wrappedResponse = + new StatusCapturingResponseWrapper((HttpServletResponse) response); + Throwable chainException = null; + try { + chain.doFilter(httpRequest, wrappedResponse); + } catch (Throwable t) { + chainException = t; + // Promote to 500 so the finally block emits an event for this escaped exception. + if (wrappedResponse.getCapturedStatus() < 400) { + wrappedResponse.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } + } finally { + try { + if (!RequestContext.isOperationFailureFired()) { + int status = wrappedResponse.getCapturedStatus(); + if (status >= 400) { Review Comment: I see. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
