Added optional X-Request-Id to RemoteIpFilter - #758
Conversation
Sorry, something went wrong.
|
Will happily review in the next couple of days. |
Sorry, something went wrong.
michael-o
left a comment
There was a problem hiding this comment.
This should also be in the valve to be added to a host element.
I also think, maybe with a second PR this value should be natively accessible in the access log valve.
Sorry, something went wrong.
| /** | ||
| * @see #setRequestIdHeader(String) | ||
| */ | ||
| private String requestIdHeader = ""; |
There was a problem hiding this comment.
Shouldn't it be null?
Sorry, something went wrong.
There was a problem hiding this comment.
In the current state of the implementation I made it "backwards compatible" in the sense that no header is set. Yes, we can set it to null but empty String does not add a risk of NPE so I'm not sure if using null adds value.
I'm happy to change it to null if others think that it's more appropriate.
Sorry, something went wrong.
|
I was hoping to reuse XForwardedRequest from RemoteIpFilter in RemoteIpValve, but while a Filter's doFilter() takes a jakarta.servlet.ServletRequest [1] as an argument, a Valve's invoke() takes a org.apache.catalina.connector.Request [2] which in turn implements HttpServletRequest and ServletRequest. Ideally, Valve's invoke() would take an interface like HttpServletRequest rather than the concrete implementation of connector.Request. What does connector.Request add on top of HttpServletRequest? Is it possible to change the Valve interface so that invoke() takes the HttpServletRequest interface? My objective is to be able to either set the requestId or override getRequestId() so another approach might be to not reuse XForwardedRequest and instead add a setter like setRequestId(). Another option might be to make XForwardedRequest extend connector.Request, but that would require a reference to the Connector and coyote.Request which I don't think is available as the original call of the filterChain is from StandardWrapperValve: filterChain.doFilter(request.getRequest(), response.getResponse()); So while request is connector.Request, the filter chain receives an instance of RequestFacade... Perhaps I shouldn't try to reuse XForwardedRequest after all? I will still need to expose setRequestId() though 🤔 Any thoughts? cc @rmaucher @markt-asf |
Sorry, something went wrong.
|
From memory of previous discussions, implementing Request/Response wrapping in Valves is non-trivial. I'd expose a setter for requestId. |
Sorry, something went wrong.
This patch adds an optional requestIdHeader configuration to RemoteIpFilter, that when used, sets the XForwardedRequest.requestId to the value passed with that header.
By default, requestIdHeader is set to an empty string and is effectively disabled, thus maintaining backwards compatibility. Setting a value in the filter config, e.g. to "X-Request-Id", allows proxy servers or the client application to specify a Request ID, e.g. X-Request-Id: abcd-1234 which is then returned via getReuqestId().
I wanted to do the same for RemoteIpValve but it looks like there we do not use the XForwardedRequest and other changes will be required.
Feedback welcome.