log ip #811
log ip #811
Conversation
There was a problem hiding this comment.
This PR also triggers a compilation error in the test cases.
I am minded to close this PR due to the low quality, particularly that it doesn't compile.
Even with the compilation and other issues fixed, I am not convinced this is the right approach or even necessary. The access log is where I'd expect the underlying requirement to be handled.
| Http2AsyncParser(String connectionId, Input input, Output output, SocketWrapperBase<?> socketWrapper, | ||
| Http2AsyncUpgradeHandler upgradeHandler) { | ||
| Http2AsyncUpgradeHandler upgradeHandler,String rip) { | ||
| super(connectionId, input, output); |
| @Override | ||
| protected Http2Parser getParser(String connectionId) { | ||
| protected Http2Parser getParser(String connectionId, String rip) { | ||
| return new Http2AsyncParser(connectionId, this, this, socketWrapper, this); |
| if (key == Setting.UNKNOWN) { | ||
| log.warn(sm.getString("connectionSettings.unknown", connectionId, Integer.toString(id), | ||
| log.warn(sm.getString("connectionSettings.unknown", rip + "_" + connectionId, Integer.toString(id), | ||
| Long.toString(value))); |
There was a problem hiding this comment.
The right way to add information to a log message is by adding an additional argument to the i18n string.
| String rip = "-"; | ||
| if (webConnection != null) { | ||
| try { | ||
| stream = getStream(1, true); | ||
| rip = stream.getCoyoteRequest().getRequestProcessor().getRemoteAddr(); | ||
| } catch (Http2Exception e) { | ||
| throw new ProtocolException(sm.getString("upgradeHandler.upgrade.fail", connectionId)); | ||
| } | ||
| } | ||
|
|
||
| parser = getParser(connectionId, rip); | ||
|
|
There was a problem hiding this comment.
Duplicates code later in the method.
| if (key == Setting.UNKNOWN) { | ||
| log.warn(sm.getString("connectionSettings.unknown", connectionId, Integer.toString(id), | ||
| String rip = stream.getCoyoteRequest().getRequestProcessor().getRemoteAddr(); | ||
| log.warn(sm.getString("connectionSettings.unknown", rip + "_" + connectionId, Integer.toString(id), | ||
| Long.toString(value))); |
Is there a way to log connectionId in the access log with something like %{xxx}r ? |
|
I have relatively little tolerance for PRs that fail to compile. I have even less tolerance for PRs that still fail to compile after this problem has been pointed out once. Logging the connection ID in the access log looks like a candidate for an addition to the If a request gets as far as sending at least one byte to Tomcat it should appear in the access log. Dedicated, separate loggers are provided for TLS handshake failures |
Every log relating to data sent from a connection should log the IP for fail2ban etc.