Proposal - 8209137: Add ability to bind to specific local address to HTTP client

[Michael McMahon]

Yes, I agree with this. It's a niche use case which should be made clear 
in the docs.

The new method should have a default implementation.

I think I prefer also to use a SocketAddress in case there is some 
future need to also
specify the local port. The InetSocketAddress constructors allow a 
default value of zero
to mean any port.

Thanks,

Michael.

On 03/12/2021 12:29, Daniel Fuchs wrote:
> Hi Jaikiran,
>
> A couple of remarks:
>
> > However, in this specific case the Builder is expected
> > to be created using the static java.net.http.HTTPClient.newBuilder()
> > method and that implementation always returns an internal 
> implementation
> > of the Builder interface. That effectively means that the only
> > implementation impacted by this new method addition would be the JDK
> > internal jdk.internal.net.http.HttpClientBuilderImpl. Because of 
> this, I
> > decided not to create this above new method as a "default" one.
>
> I do not think this is right. Even though the only implementation
> is in the JDK, libraries that provides adds-on on top of the JDK API
> could very well provide their own builder implementation - obtained
> by other means than calling HttpClient::newBuilder().
>
> In this case I would recommend having a default body that
> throws UnsupportedOperationException - and adding the @throws
> to the API documentation of the new method.
>
> You will also need an @implSpec to document the default body,
> and possibly say that builders obtained from HttpClient::newBuilder
> provide an implementation of this method.
>
> There will also be some security checks that need to be performed
> probably in the builder implementation, either when passing the
> address or possibly when build() is called (or maybe both?).
> See SocketChannel::bind. @throws SecurityException with appropriate
> documentation will need to be added where needed.
> The HttpClient does these checks before hand - so I believe a
> security check needs to happen in build() at the latest.
>
> There's also the question on whether we should take a SocketAddress
> here, even though at this time we only accept InetAddress.
> There might be pro and cons.
>
> > In addition to the above method on the Builder interface, a new method
> > has also been introduced on the java.net.http.HttpClient class:
> >
> >      /**
> >       * Returns an {@link InetAddress} which will be used by this 
> client
> >       * to bind a socket when creating connections. If no
> >       * {@link Builder#localAddress() local address} was set in the
> >       * client's builder, then this method returns null, in which case
> >       * the sockets created by this client will be bound to 
> automatically
> >       * assigned socket addresses.
> >       *
> >       * @return the local address that will be used by this client when
> >       *         creating connections
> >       * @since 19
> >       */
> >      public InetAddress localAddress() {
> >          return null;
> >      }
>
> I am not sure we should add that.
> If we do add this method, then it should probably
> return Optional<InetAddress> like other methods in this
> class (e.g. proxy(), authenticator(), selector())
>
> WRT to SocketChannel::bind I believe we should treat that as a non
> blocking call - as it shouldn't involve any network operation.
>
> Finally, supplying a local address to the HttpClient builder is really
> for advanced users. Only hosts reachable through that interface will
> be connectable. This means that providing a "bad" address there
> can prevent accessing the servers targetted by the request URI.
> In pathological cases it could also prevent accessing the proxy
> returned by the proxy selector.
>
> The documentation of Builder::localAddress should probably have
> more information about the use cases for supplying a local address
> and some warning about the consequences of doing so.
>
> Michael may have more insight/comments to provide.
>
> best regards,
>
> -- daniel
>
> On 03/12/2021 06:21, Jaikiran Pai wrote:
>> I've been working on adding support for the enhancement noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8209137. As noted in that 
>> issue, right now the java.net.http.HTTPClient doesn't have a way to 
>> allow applications to specify which local address to use while 
>> connecting to target hosts during requests >
>> The proposed enhancement will introduce an API to allow applications 
>> to configure a particular local address that the HTTPClient will use 
>> when creating Socket(s) during connection. I've an initial version of 
>> this enhancement that I've created as a draft PR here 
>> https://github.com/openjdk/jdk/pull/6690 for initial discussion. I 
>> would like some inputs on these changes.
>>
>> User facing API:
>> ---------------
>> A new API has been introduced on the java.net.http.HTTPClient.Builder 
>> interface:
>>
>>
>>          /**
>>           * Binds the socket to this local address when creating
>>           * connections for sending requests.
>>           *
>>           * <p> If no local address is set or {@code null} is passed
>>           * to this method then sockets created by the
>>           * HTTP client will be bound to an automatically
>>           * assigned socket address.
>>           *
>>           * @param localAddr The local socket address. Can be null.
>>           * @return this builder
>>           * @since 19
>>           */
>>          public Builder localAddress(InetAddress localAddr);
>>
>> The java.net.http.HTTPClient.Builder is a public API and typically 
>> any new method additions to such interfaces would mean using a 
>> "default" method so as to prevent and breakages of existing 
>> implementations of that interface. However, in this specific case the 
>> Builder is expected to be created using the static 
>> java.net.http.HTTPClient.newBuilder() method and that implementation 
>> always returns an internal implementation of the Builder interface. 
>> That effectively means that the only implementation impacted by this 
>> new method addition would be the JDK internal 
>> jdk.internal.net.http.HttpClientBuilderImpl. Because of this, I 
>> decided not to create this above new method as a "default" one.
>>
>> This new locaAddress method accepts an instance of 
>> java.net.InetAddress. Calling applications are expected to create the 
>> InetAddress using different available methods. I paid some thought to 
>> see if it's worth allowing calling applications to pass a 
>> "hostname/IP" String as a parameter to this new method instead of 
>> accepting a InetAddress, but decided to expect a pre-constructed 
>> InetAddress to keep it simple (for the HTTPClient) as well as to let 
>> the application/callers deal with the InetAddress creation.
>>
>> Calling this method is optional and as noted in the javadoc of this 
>> method, passing null or not calling this method will have the same 
>> result - the connections created by the HTTPClient will end up using 
>> an automatically assigned socket address, just like it does in the 
>> current version.
>>
>> Furthermore, the Builder, neither in the implementation of this 
>> method nor during the build() call will do any validations on the 
>> passed InetAddress. In other words, the localAddress that's set on 
>> the HTTPClient will just be used as-is during connection creation and 
>> any failures to bind due to the passed InetAddress will be propagated 
>> back as a IOException. I spent some thought on whether to do any kind 
>> of validations, like whether the host of the InetAddress is 
>> resolvable and such, during the Builder.build() but decided not to. 
>> One reason for that is, the HTTPClient is a long lived object and, as 
>> far as I remember, is encouraged to be used as one single instance 
>> for the lifetime of an application. The address resolution result of 
>> an InetAddress (as per its javadoc) are cachable values and are 
>> refreshed from time to time (based on internal implementation details 
>> of that class). As such, there's no guarantee that a "valid" 
>> InetAddress during Builder.build() would still be "valid" when the 
>> connection is being established by the HTTPClient at some later point 
>> in time (maybe some hours later). So doing such validations at 
>> Builder.build() time, I thought, would add no real value.
>>
>> In addition to the above method on the Builder interface, a new 
>> method has also been introduced on the java.net.http.HttpClient class:
>>
>>      /**
>>       * Returns an {@link InetAddress} which will be used by this client
>>       * to bind a socket when creating connections. If no
>>       * {@link Builder#localAddress() local address} was set in the
>>       * client's builder, then this method returns null, in which case
>>       * the sockets created by this client will be bound to 
>> automatically
>>       * assigned socket addresses.
>>       *
>>       * @return the local address that will be used by this client when
>>       *         creating connections
>>       * @since 19
>>       */
>>      public InetAddress localAddress() {
>>          return null;
>>      }
>>
>>   This method just returns whatever was set as the localAddress on 
>> the Builder, using which this instance of HTTPClient was built.
>>
>>
>>   (Internal) Implementation:
>>   --------------------------
>>   Within the java.net.http.HTTPClient implementation, 
>> jdk.internal.net.http.HttpConnection is where the underlying 
>> SocketChannel connections are handled. There are multiple sub-classes 
>> of the jdk.internal.net.http.HttpConnection abstract class. However 
>> each of those sub-classes ultimately ends up calling the 
>> jdk.internal.net.http.PlainHttpConnection class for doing the actual 
>> connection on the SocketChannel. The implementation in this class has 
>> now been enhanced to take into account any localAddress that might 
>> have been set on the HTTPClient. In the presence of such a configured 
>> InetAddress, this implementation now calls a SocketChannel.bind() 
>> before calling the SocketChannel.connect().
>>
>>   More precisely, the PlainHttpConnection.connectAsync(Exchange<?> 
>> exchange) method has been enhanced to call the bind() before calling 
>> the connect() on the channel. This 
>> PlainHttpConnection.connectAsync(), as the name suggests, isn't 
>> supposed to have any blocking calls. My knowledge in this area isn't 
>> vast, so I was unsure whether a SocketChannel.bind() call is a 
>> blocking call, even when the SocketChannel is configured to be 
>> non-blocking. The SocketChannel.bind() doesn't state much about this 
>> semantic (unlike the SocketChannel.connect()). Looking at the API and 
>> the implementation (which ends up calling the native layer for the 
>> socket bind), I think a call to bind() would be a blocking one. More 
>> so in the case where it perhaps has to find a free/available port 
>> (when the passed port to the socket address is 0, like in the 
>> implementation here). As such, in this proposed implementation, I've 
>> considered SocketChannel.bind() to be blocking and thus wrapped it 
>> around in a async call that needs to complete successfully before an 
>> async connect is issued. I look forward to inputs around this part on 
>> whether or not this is the right way to go about it.
>>
>> Testing:
>> -------
>> A new jtreg test has been introduced to test this new method. The 
>> test is configured to run with both IPv6 and IPv4. tier1 testing with 
>> these changes have passed on a personal run of GitHub Actions and a 
>> local test run of "test/jdk/java/net/httpclient/" has also passed 
>> with these changes. I'll run tier2 tests locally once we have 
>> finalized on the API part of this change.
>>
>> The PR is currently in draft and I will move it out of draft after 
>> the API is finalized. I don't plan to rush this into JDK 18, so the 
>> new APIs have all been marked for @since 19.
>>
>> -Jaikiran
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20211203/f2c4cf98/attachment-0001.htm>