Support setting the local address per connection with ConnectOptions - #6320
Open
jnbdz wants to merge 1 commit into
Open
Support setting the local address per connection with ConnectOptions#6320jnbdz wants to merge 1 commit into
jnbdz wants to merge 1 commit into
Conversation
Motivation: TCP based clients can only bind their connections to the local address configured at the client level. Binding a given connection to a local address, e.g. a specific network interface or port, requires a dedicated client. Fixes eclipse-vertx#2335. In addition, the local address configured with the legacy HttpClientOptions#setLocalAddress and WebSocketClientOptions#setLocalAddress was ignored: TcpClientConfig(ClientOptionsBase) does not carry it and the HTTP client is built from an HttpClientConfig. Changes: Add localAddress to ConnectOptions and HttpConnectOptions, when set the connection is bound to it before connecting, otherwise the client default local address is used, if any. Remove localAddress from TcpClientConfig, the client default local address is now held by NetClientImpl and set by NetClientBuilder, from the legacy NetClientOptions, HttpClientOptions and WebSocketClientOptions. Propagate the HttpConnectOptions local address to the TCP transport when connecting an HttpClient connection. Add NetClient and HTTP client tests binding connections to a specific local port, and a test for the legacy HttpClientOptions local address, along with a documentation example.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
TCP based clients can only bind their connections to the local address configured at the client level; binding a given connection to a specific interface or port requires a dedicated client. This implements the approach discussed in #2335: the local address is a per-connection property, the client-level value is only a default.
Fixes #2335.
Changes
Following the steps listed in the issue:
localAddressis removed fromTcpClientConfig—TcpClientConfig#setLocalAddresswas released in 5.1.x, so this is an API removal on 5.2; if you would rather keep it as@Deprecatedand ignored for one release, I can adjust.NetClientImpland set byNetClientBuilder, from the legacyNetClientOptions#setLocalAddress(String)(NetClientBuilder#localAddress(ClientOptionsBase)does the mapping to an inet socket address with an ephemeral port).localAddressis added as aSocketAddressonConnectOptions(domain socket addresses are rejected), and onHttpConnectOptionstoo since it does not extendConnectOptions.ConnectOptions#getLocalAddress()is used when set, otherwise the client default (which may benull).Bootstrap.For the HTTP client,
HttpClientImpl#connect(HttpConnectOptions)propagates the local address throughHttpConnectParamstoTcpHttpClientTransport, so an un-pooled connection can be bound to a local address; pooled requests keep using the client default. The QUIC transport is unchanged (it has its own configuration).Legacy HTTP / WebSocket client local address was ignored
While wiring this I found that
HttpClientOptions#setLocalAddressandWebSocketClientOptions#setLocalAddresswere silently ignored on 5.x:HttpClientConfig(HttpClientOptions)goes throughTcpClientConfig(ClientOptionsBase), which never copied the local address (only theNetClientOptionsconstructor did). WithsetLocalAddress("127.0.0.2")the server saw connections from127.0.0.1. The existingHttpTest#testClientLocalAddressdid not catch it because it binds to and asserts127.0.0.1. This is fixed by passing the legacy options local address toNetClientBuilderinHttpClientBuilderInternaland inVertxImpl#createWebSocketClientImpl.Tests
NetTest#testConnectOptionsLocalAddress: connect withConnectOptions#setLocalAddressbound to a specific free port, assert the server sees that source port andNetSocket#localAddress()reports it.NetTest#testConnectOptionsLocalAddressOverridesClientLocalAddress: the client is created withNetClientOptions#setLocalAddress, the connect options local address takes precedence.Http1xTest#testConnectOptionsLocalAddress: same throughHttpClient#connect(HttpConnectOptions).Http1xTest#testClientOptionsLocalAddress: legacyHttpClientOptions#setLocalAddress("127.0.0.2")is honoured (skipped withAssumeon platforms without that loopback address).NetTest#testClientLocalAddress/HttpTest#testClientLocalAddresskeep passing.Documentation: a paragraph and example in the TCP client section (
connectingToAServerFromALocalAddress).NetTest,Http1xTest,Http2Test,WebSocketTest, the proxy tests,SharedHttpClientTestand the net/options test packages pass locally.