Fixes #4421 - HttpClient support for PROXY protocol. #4424
Conversation
Implemented support for the PROXY protocol in HttpClient. Introduced Request.tag(Object) to tag requests that belong to the same group (e.g. a client address) so that they can generate a different destination. The tag object may implement ClientConnectionFactory.Decorator so that it can decorate the HttpDestination ClientConnectionFactory and therefore work both with and without forward proxy configuration. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
Don't like this at all!
The Tag mechanism appears loosely typed but will fail with a class cast exception if a tag of some random type is assigned.
The Info classes are poorly named and appear to have duplicate information in them (the server IP/port). It is not clear in their usage that they are describing a proxy tunnel.
I think the language of the API needs to be strongly typed and explicitly about Proxy, Tunnel, Connection, ForwardFor or something better than Tag and Info, which could be anything.
Does this need to be linked somehow with setting the Forwarded headers?
If this design is indeed the only viable solution, then it needs a lot more explanation what the Info and Tag are communicating and how they should be used.
| @@ -48,6 +60,11 @@ public Address getAddress() | |||
| return address; | |||
| } | |||
|
|
|||
| public Object getTag() | |||
There was a problem hiding this comment.
Is javadoc needed here to indicate that this is the same tag from the associated request?
jetty-client/src/main/java/org/eclipse/jetty/client/ProxyProtocolClientConnectionFactory.java
Show resolved
Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/ProxyProtocolClientConnectionFactory.java
Outdated
Show resolved
Hide resolved
| int serverPort = connector.getLocalPort(); | ||
|
|
||
| int clientPort = ThreadLocalRandom.current().nextInt(1024, 65536); | ||
| V1.Info info = new V1.Info("TCP4", "127.0.0.1", clientPort, "127.0.0.1", serverPort); |
There was a problem hiding this comment.
This example makes no sense to me? The user should not be dealing with IP addresses and ports like this. It is not entirely clean which client this is talking about - specifically it looks like I need to know my own client port before it has even been allocated (which it is not as I guess it is the port that we are forwarding??).
Why must the user specify information that is in the request anyway, such as the server and serverPort? Does it have to be server IP or can it be server name? What about servers with multiple IPs and DNS balancing?
Updates after review. Renamed Info -> Tag. Added convenience constructors to class Tag. Added test and server support for PROXY V1 UNKNOWN. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Personally, I like the tag. It's intentionally loosely typed, and can apply to anything related to destination resolution. I could use it to tag requests with a common object that my application knows and uses. I could use the tag to section off destinations by something arbitrary (like load clients and a minimum physical connections config). Just because the tag can also apply to a proxy is a curiosity and isn't in conflict with the tag idea. |
|
@joakime during the review, the name 'Tag' grew on me, specially with |
| this(null, 0); | ||
| } | ||
|
|
||
| public Tag(String srcIP, int srcPort) |
There was a problem hiding this comment.
probably should javadoc these constructors to say that null values mean that the proxy protocol will take the value from the EndPoint when then tag is applied to a specific connection (and not defaulted at construction).
| { | ||
| try | ||
| { | ||
| InetSocketAddress localAddress = endPoint.getLocalAddress(); |
There was a problem hiding this comment.
Do all our endpoints return non null InetAddresses? What if this is applied to a UDP endpoint?
Updates after review. Improved javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Implemented support for the PROXY protocol in HttpClient.
Introduced Request.tag(Object) to tag requests that belong
to the same group (e.g. a client address) so that they can
generate a different destination.
The tag object may implement ClientConnectionFactory.Decorator
so that it can decorate the HttpDestination ClientConnectionFactory
and therefore work both with and without forward proxy configuration.
Signed-off-by: Simone Bordet simone.bordet@gmail.com