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.

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

8209137: Add ability to bind to specific local address to HTTP client #6690

Closed
wants to merge 41 commits into from

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Dec 3, 2021

This change proposes to implement the enhancement noted in https://bugs.openjdk.java.net/browse/JDK-8209137.

The change introduces a new API to allow applications to build a java.net.http.HTTPClient configured with a specific local address that will be used while creating Socket(s) for connections.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires a CSR request to be approved
  • Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issues

  • JDK-8209137: Add ability to bind to specific local address to HTTP client
  • JDK-8286583: Add ability to bind to specific local address to HTTP client (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6690/head:pull/6690
$ git checkout pull/6690

Update a local copy of the PR:
$ git checkout pull/6690
$ git pull https://git.openjdk.java.net/jdk pull/6690/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6690

View PR using the GUI difftool:
$ git pr show -t 6690

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6690.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 3, 2021

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Dec 3, 2021

@jaikiran The following label will be automatically applied to this pull request:

  • net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the net net-dev@openjdk.org label Dec 3, 2021
private static URI rootURI;

@BeforeClass
public static void beforeClass() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

For HttpClient tests - unless they are specific to HTTP/1.1 I'd recommend making the test class implement HttpServerAdapters and test will all versions of the protocol (http, https, http/2, and https/2).
An example can be found in e.g. test/jdk/java/net/httpclient/CancelRequestTest.java

Copy link
Member

Choose a reason for hiding this comment

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

Also it's better if the test is IP version agnostic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hello Daniel, I will update the test accordingly in the next round of updates.

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest HttpClientLocalAddrTest implements HttpServerAdapters like other tests do.

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

Generally it would be good if the test did not make any assumption on the presence - or absence - of either IPv4 or IPv6 on the tested machine.
The IPSupport test library class has methods that allows a test to inquire whether IPv6 or IPv4 are available - I would recommend using them to figure out which test cases can be tested on the machine the test runs on.

.build(),
requestURI,
loopbackAddr
});
// anyAddress
if (Boolean.getBoolean("java.net.preferIPv6Addresses")) {
// ipv6 wildcard
Copy link
Member

Choose a reason for hiding this comment

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

::1 is the ipv6 loopback - not the ipv6 wildcard

Copy link
Member

Choose a reason for hiding this comment

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

You may encounter some issues when testing with https and IPv6 - Michael is working on a fix to add the loopback addresses as SNI aliases to the SimpleSSLContext certificate:
https://bugs.openjdk.java.net/browse/JDK-8278312

Copy link
Member

Choose a reason for hiding this comment

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

You could also replace Boolean.getBoolean("java.net.preferIPv6Addresses") with a call to IPSupport::preferIPv6Addresses()

Copy link
Member Author

@jaikiran jaikiran Dec 8, 2021

Choose a reason for hiding this comment

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

::1 is the ipv6 loopback - not the ipv6 wildcard

Oops, that's embarrassing and it isn't even a copy/paste mistake. I have been in this part of the test so many times while cleaning up/debugging and yet didn't notice this. Fixed it in the latest PR update now.

I'll update the rest of this test with your suggestions shortly. Thank you Daniel for the reviews.

Copy link
Member Author

Choose a reason for hiding this comment

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

You may encounter some issues when testing with https and IPv6 - Michael is working on a fix to add the loopback addresses as SNI aliases to the SimpleSSLContext certificate: https://bugs.openjdk.java.net/browse/JDK-8278312

I had a look at this one. Turns out my test wasn't impacted by it because the test ends up using a hostname based URLs (localhost) for requests and the issue only impacts requests that use IP addresses for the requests (where the server then presents a hostname based cert causing the cert validation failure.).

@jaikiran jaikiran marked this pull request as ready for review December 10, 2021 09:27
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 10, 2021
@jaikiran
Copy link
Member Author

Any further reviews on this PR, please?

@jaikiran
Copy link
Member Author

I'd suggest HttpClientLocalAddrTest implements HttpServerAdapters like other tests do.

Done. Updated the PR to implement the HttpServerAdapters.

@bridgekeeper
Copy link

bridgekeeper bot commented May 9, 2022

@jaikiran This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

* @implSpec If the {@link #localAddress(SocketAddress) local address} is a non-null
* <i>Internet Protocol</i> socket address and a security manager is installed, then
* @implSpec If the {@link #localAddress(InetAddress) local address} is a non-null
* <i>Internet Protocol</i> address and a security manager is installed, then
Copy link
Member

Choose a reason for hiding this comment

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

Here and in the @throws bellow we could now remove mention of <i>Internet Protocol</i>.
For instance, here we could say

... is a non-null address and a security ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Updated this and the other javadoc comment to follow this suggestion.

Comment on lines 409 to 411
* the {@link #localAddress(InetAddress) local address} is an
* <i>Internet Protocol</i> address and the
* security manager's {@link SecurityManager#checkListen checkListen}
Copy link
Member

Choose a reason for hiding this comment

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

... has been installed and the security manager's {@link SecurityManager#checkListen checkListen} ...

builder.localAddress(new InetSocketAddress(0));
builder.localAddress(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0));
builder.localAddress(new JustAnotherInetSocketAddress(0));
builder.localAddress(InetAddress.getLoopbackAddress());
Copy link
Member

Choose a reason for hiding this comment

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

We probably also need a MockHttpClientBuilder to test the behaviour of the default implementation (check that it throws UOE).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've now updated this PR to add a new test method which tests the default method implementation of localAddress.

Comment on lines 267 to 268
* Tests the {@link HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method
* behaviour when that method is called on a builder returned by {@link HttpClient#newBuilder()}
Copy link
Member

Choose a reason for hiding this comment

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

/**
  * Tests the {@link HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method

This {@link looks broken - the HttpClient, prefix probably need to be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. That prefix wasn't intentional. I'm not sure how I ended up with that. I just pushed an update to fix this. Thank you for flagging this.

Comment on lines 282 to 283
* {@link HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} throws
* an {@link UnsupportedOperationException}
Copy link
Member

Choose a reason for hiding this comment

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

Same remark here

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise things look good to me - you should update the CSR if it needs to be updated.

@jaikiran
Copy link
Member Author

I've triggered some internal CI runs against the current state of PR to see all works fine. Now that we have decided on the nature of this API, I'll go ahead and file a CSR for this.

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

LGTM

* requests sent through the {@code HttpClient} to fail.
*
* @implSpec The default implementation of this method throws
* {@code UnsupportedOperationException}. {@code Builder}s obtained
Copy link
Member

Choose a reason for hiding this comment

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

I think the implSpec here is correct, but will be confusing for most users. I'm not sure what value it adds. Do we really need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hello Michael,
Most users will be using the HttpClient.newBuilder() to create the builder, so this note about UnsupportedOperationException can be confusing. However, for implementations (libraries?) which provide their own implementation of the java.net.http.HttpClient.Builder, I think, this note would be relevant. This approach is similar to what we already have on java.net.http.HttpClient.newWebSocketBuilder() method.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I just think when most developers read "The default implementation of this method throws UOE" they will think they can't use it without implementing something themselves. Library developers will figure it out anyway.

Copy link
Member

@dfuch dfuch May 11, 2022

Choose a reason for hiding this comment

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

This is the standard wording that has been used throughout the JDK to document the behavior of default methods on interfaces. Unless we receive different feedback during the CSR review I'd suggest to leave it that way. The second sentence makes it clear that our concrete implementations override that default behavior.

@jaikiran
Copy link
Member Author

I've now created a CSR for this change https://bugs.openjdk.java.net/browse/JDK-8286583

… relying on hostname which

doesn't always return localhost for 127.0.0.1 on Windows
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels May 15, 2022
@jaikiran
Copy link
Member Author

The CSR for this has been approved. If the current state of the PR looks good, then I plan to integrate this later today.

@jaikiran
Copy link
Member Author

Thank you Daniel and Michael for the constant inputs and reviews on this one.

@jaikiran
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented May 16, 2022

Going to push as commit f4258a5.
Since your change was applied there have been 134 commits pushed to the master branch:

  • 65da38d: 8284585: PushPromiseContinuation test fails intermittently in timeout
  • 652044d: 8286297: G1: Simplify parallel and serial verification code paths
  • 0155e4b: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview)
  • 2d34acf: 8286638: C2: CmpU needs to do more precise over/underflow analysis
  • 46d208f: 8284950: CgroupV1 detection code should consider memory.swappiness
  • e2448ce: 8286791: CLONE - ProblemList compiler/c2/irTests/TestSkeletonPredicates.java in -Xcomp mode
  • 357f990: 8286428: AlgorithmId should understand PBES2
  • f4f1ddd: 8284194: Allow empty subject fields in keytool
  • dc94621: 8286782: Exclude vmTestbase/gc/gctests/WeakReference/weak006/weak006.java
  • 0e4bece: 8286705: GCC 12 reports use-after-free potential bugs
  • ... and 124 more: https://git.openjdk.java.net/jdk/compare/b490a58ed826de28d4c1c0abea00d51e12c4eee6...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 16, 2022
@openjdk openjdk bot closed this May 16, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 16, 2022
@openjdk
Copy link

openjdk bot commented May 16, 2022

@jaikiran Pushed as commit f4258a5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
3 participants