Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new API AccessLogArgProvider#connectionInformation() for retrieving information related to host/remote address/scheme #2606

Merged
merged 8 commits into from Dec 12, 2022
Merged

Add new API AccessLogArgProvider#connectionInformation() for retrieving information related to host/remote address/scheme #2606

merged 8 commits into from Dec 12, 2022

Conversation

pderop
Copy link
Member

@pderop pderop commented Dec 8, 2022 •
edited

This PR addresses #2603 issue, and allows any custom AccessLogFactory to get more informations about the connection on which the request has been received. The AccessLogArgProvider interface has a new connectionInformation() method which returns a new ConnectionInformationinterface, which has the following methods:

  • hostAddress()/remoteAddress()/scheme(): provides the host address/remote address/scheme that is derived from all proxies (using Forwarded/X-Fowarded-* headers)
  • connectionHostAddress()/connectionRemoteAddress()/connectionScheme(): provides the host/remote address/scheme of the connection from which the request is received

Fixes #2603

@pderop pderop added the type/bug A general bug label Dec 8, 2022
@pderop pderop self-assigned this Dec 8, 2022
@pderop pderop mentioned this pull request Dec 8, 2022
Closed
@pderop
Copy link
Member Author

pderop commented Dec 8, 2022

@violetagg ,

Can you take a look ?

PS: maybe we should not include this PR for the next upcoming release, that's why I did not add a milestone.

@pderop pderop requested a review from violetagg December 8, 2022 12:27
@violetagg
Copy link
Member

violetagg commented Dec 8, 2022 •
edited

@pderop Now with this change we have the other problem - always log what is provided with X-Forwarded/Forwarded header.

We need to be able to log either with client or connection peer.

@pderop
Added remotePeer method in AccessLogArgProvider class, to allow peopl…
0e158c3 
…e to choose between remoteAddress vs remotePeer.
@pderop
Copy link
Member Author

pderop commented Dec 9, 2022

@violetagg,

indeed, the PR breaks something: if the HttpServer forwarded mode is enabled, then accesslog will now log the forwarded client address which is a breaking change, so people may complain about that.

second attempt:

To let people choose either connection peer address, or forwarded remote client address, the last commit is adding a new forwardedAddress method in the AccessLogArgProvider interface. So if the forwarded client address needs to be displayed in the accesslog, then the user will need to use the AccessLogFactory and invoke the new forwardedAddress() method from the AccessLogArgProvider argument passed to the factory, and the original AccessLogFactory.remoteAddress is left unchanged (the forwarded mode must be enabled on the HttpServer).

please take a look ?

@pderop
Copy link
Member Author

pderop commented Dec 9, 2022

mmm, please wait, sanity checks are failing.

@pderop
Copy link
Member Author

pderop commented Dec 9, 2022

The sanity check error is now fixed.

@pderop pderop removed the request for review from violetagg December 9, 2022 15:28
@pderop pderop added this to the 1.0.27 milestone Dec 9, 2022
@pderop pderop requested a review from violetagg December 9, 2022 15:28
@pderop
Copy link
Member Author

pderop commented Dec 9, 2022

Let's just follow the suggestion done in the original #2603: simply expose the ConnectionInfo from the AccessLogFactory

@violetagg violetagg modified the milestones: 1.0.27, 1.0.26 Dec 12, 2022
violetagg
violetagg requested changes Dec 12, 2022
View reviewed changes
Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small fixes for the javadoc

reactor-netty-http/src/main/java/reactor/netty/http/server/logging/AccessLogArgProvider.java Outdated
@@ -16,12 +16,15 @@
package reactor.netty.http.server.logging;

import io.netty.handler.codec.http.cookie.Cookie;
import reactor.netty.http.server.ConnectionInfo;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import reactor.netty.http.server.ConnectionInfo;

reactor-netty-http/src/main/java/reactor/netty/http/server/logging/AccessLogArgProvider.java Outdated
@@ -54,10 +57,26 @@ public interface AccessLogArgProvider {
* Returns the address of the remote peer or {@code null} in case of Unix Domain Sockets.
*
* @return the peer's address
* @deprecated Use {@link ConnectionInformation#connectionRemoteAddress()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @deprecated Use {@link ConnectionInformation#connectionRemoteAddress()}
* @deprecated as of 1.0.26. Use {@link ConnectionInformation#connectionRemoteAddress()}

reactor-netty-http/src/main/java/reactor/netty/http/server/logging/AccessLogArgProvider.java Outdated
Comment on lines 67 to 76
/**
* Returns the information about the current connection.
* <p> Note that the {@link ConnectionInfo#getRemoteAddress()} will return the forwarded
* remote client address if the server is configured in forwarded mode.
*
* @since 1.0.26
* @return the connection info
* @see reactor.netty.http.server.HttpServer#forwarded(BiFunction)
* @see reactor.netty.http.server.HttpServer#forwarded(BiFunction)
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Returns the information about the current connection.
* <p> Note that the {@link ConnectionInfo#getRemoteAddress()} will return the forwarded
* remote client address if the server is configured in forwarded mode.
*
* @since 1.0.26
* @return the connection info
* @see reactor.netty.http.server.HttpServer#forwarded(BiFunction)
* @see reactor.netty.http.server.HttpServer#forwarded(BiFunction)
*/
/**
* Returns the information about the current connection.
* <p> Note that the {@link ConnectionInformation#remoteAddress()} will return the forwarded
* remote client address if the server is configured in forwarded mode.
*
* @since 1.0.26
* @return the connection info
* @see reactor.netty.http.server.HttpServer#forwarded(BiFunction)
*/

@pderop pderop merged commit 76e1fcf into reactor:1.0.x Dec 12, 2022
pderop added a commit that referenced this pull request Dec 12, 2022
@pderop
Merge #2606 into 1.1.1
a74c684
pderop added a commit that referenced this pull request Dec 12, 2022
@pderop
Merge #2606 into 2.0.0-M4
b277785
@violetagg violetagg added type/enhancement A general enhancement and removed type/bug A general bug labels Dec 13, 2022
@violetagg violetagg changed the title Include forwarded client IP address in access log Add new API AccessLogArgProvider#connectionInformation() for retrieving information related to host/remote address/scheme Dec 13, 2022
@pderop pderop deleted the 1.0.x-include-forwarded-client-ip-in-accesslog branch December 14, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@violetagg violetagg violetagg approved these changes

Assignees

@pderop pderop

Labels
type/enhancement A general enhancement
Projects
None yet
Milestone
1.0.26
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pderop @violetagg