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

Fixes #8979 - Jetty 12 - HttpClientTransport network "modes". #11368

Merged
merged 16 commits into from
Feb 26, 2024

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Feb 2, 2024

  • Introduced oej.io.TransportProtocol as the abstraction for the low-level transport of high-level protocols. Now protocols such as HTTP/1.1 or HTTP/2 can be transported over QUIC, Unix-Domain, memory, and possibly over other low-level custom protocols too.
  • Introduced oej.client.Request.transportProtocol(TransportProtocol) to specify TransportProtocol for each request.
  • Introduced TransportProtocol to [HTTP2Client|HTTP3Client].connect(...) methods.
  • Introduced [Client|Server]QuicConfiguration so that it can be used in other Connectors such as MemoryConnector.
  • Introduced oej.server.MemoryConnector and EndPoint.Pipe for memory communication between peers, along with a MemoryTransportProtocol.
  • Introduced QuicTransportProtocol as a wrapper for other TransportProtocols, so that QUIC can now also be transported over memory.

* Introduced oej.io.TransportProtocol as the abstraction for the low-level transport of high-level protocols.
Now protocols such as HTTP/1.1 or HTTP/2 can be transported over QUIC, Unix-Domain, memory, and possibly over other low-level custom protocols too.
* Introduced oej.client.Request.transportProtocol(TransportProtocol) to specify TransportProtocol for each request.
* Introduced TransportProtocol to [HTTP2Client|HTTP3Client].connect(...) methods.
* Introduced [Client|Server]QuicConfiguration so that it can be used in other Connectors such as MemoryConnector.
* Introduced oej.server.MemoryConnector and EndPoint.Pipe for memory communication between peers, along with a MemoryTransportProtocol.
* Introduced QuicTransportProtocol as a wrapper for other TransportProtocols, so that QUIC can now also be transported over memory.

Signed-off-by: Simone Bordet <[email protected]>
* Removed usage of ClientConnector.forUnixDomain() from FastCGIProxyServlet (ee10 and ee9).
* Replaced usage of HTTP3ServerConnector with QuicServerConnector in jetty-http3.xml.

Signed-off-by: Simone Bordet <[email protected]>
* Updated client documentation to consider TransportProtocol.

Signed-off-by: Simone Bordet <[email protected]>
* @return the peer address that sent the data, or {@link #EOF}
* @throws IOException if the receive fails
*/
default SocketAddress receive(ByteBuffer buffer) throws IOException
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this blocking?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm few of the existing methods have javadoc making it clear their blocking behaviour. We should fix this .

Signed-off-by: Simone Bordet <[email protected]>
@marchof
Copy link

marchof commented Feb 6, 2024

@sbordet Are there any instructions how to build this locally for testing? Local Maven build fails with the same errors than the CI.

@sbordet
Copy link
Contributor Author

sbordet commented Feb 6, 2024

@marchof build locally with mvn clean install -DskipTests.

The CI failure is due to javadocs, I'm fixing it right now.

Signed-off-by: Simone Bordet <[email protected]>
@marchof
Copy link

marchof commented Feb 6, 2024

@sbordet Thanks for this effort! Finally I got a super simple prototype running with a naive TransportProtocoland EndPoint implementation. I have to admit that my brain can hardly handle the abstractions used in these interfaces. But in the end I only needed to implement these methods (probably missing several corner cases):

  • TransportProtocol.connect()
  • EndPoint.fillInterested()
  • EndPoint.fill()
  • Endpoint.write()

@sbordet
Copy link
Contributor Author

sbordet commented Feb 6, 2024

@marchof you should not be needing to implement a new TransportProtocol and EndPoint.

If you explain your use case in more details, perhaps we can help you save some unnecessary work.

@marchof
Copy link

marchof commented Feb 6, 2024

@sbordet Let me try to explain my use case: A proxy server which proxies incoming HTTPS requests over SSH tunnels to HTTP servers. The SSH server managing the tunnels itself is implemented in Java with Apache Mina.

So if implemented in Jetty I see the following flow:

  • A Jetty server accepts HTTPS requests and handles it with a ProxyServlet.
  • The ProxyServlet uses a HttpClient with a custom transport
  • The custom transport delegates to the Apache Mina API for ssh tunnels, which is basically writing/receiving byte[] chunks

@sbordet
Copy link
Contributor Author

sbordet commented Feb 7, 2024

@marchof let me try to write a test case similar to your use case. I'll ping you when done.

@sbordet
Copy link
Contributor Author

sbordet commented Feb 7, 2024

@marchof I need more information.

Are you using SSH port forwarding?

If so, then this is what I think it happens:

  • Mina sets up port forwarding; now you have localhost:1234 on your proxy server that forwards to the remote server.
  • A request arrives to the Servlet
  • You use HttpClient to send a request to localhost:1234 with the Servlet request information and you're done.

Does Mina allow you to bypass the connection to localhost:1234?
Can you give Mina just a ByteBuffer, as if you have written it to localhost:1234 using a socket, therefore bypassing the whole network stack?

@marchof
Copy link

marchof commented Feb 7, 2024

@sbordet Yes, I'm using SSH port forwarding but without using sockets on the server side. This avoids the network stack and allows me to separate the forwards of different users. The forwarding between http hosts and ssh sockets is pure virtual and happens in memory. Therefore I cannot use a HTTP client which connects to a (local) TCP port.

Apache Mina supports this, you can basically implement your own forwarding endpoints. The use something similar to ByteBuffer called org.apache.sshd.common.util.buffer.Buffer, but in the end it's just passing around byte[]instances.

@sbordet
Copy link
Contributor Author

sbordet commented Feb 7, 2024

@marchof thanks, we'll write a test case for this scenario.

Now first clear the list, then notify to avoid that when re-entering the same instruction is notified multiple times.

Signed-off-by: Simone Bordet <[email protected]>
Introduced ContentSourceRequestContent, and updated ProxyHandler to use it.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Copy link
Contributor Author

sbordet commented Feb 8, 2024

@marchof see CustomTransportProtocolTest in this PR. Should be almost identical to your use case.

Now unix-domain files are temp files, so their name is unique.

Signed-off-by: Simone Bordet <[email protected]>
HTTP3Client http3Client = new HTTP3Client(quicConfiguration, connector);
ClientConnectionFactoryOverHTTP3.HTTP3 http3 = new ClientConnectionFactoryOverHTTP3.HTTP3(http3Client);

// The order of the protocols indicates the client's preference.
Copy link
Contributor

@cowwoc cowwoc Feb 14, 2024

Choose a reason for hiding this comment

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

Can you please clarify (in the documentation and test code) whether the order is from the most preferred to least preferred, or the other way around? In the following line, does the client prefer HTTP/1 first, HTTP/2 second and HTTP/3 third?

Is it technically possible to flip that around and have it try HTTP/3, followed by HTTP/2, followed by HTTP/1? Or is this not possible because the connection type needs to be upgraded through the earlier versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cowwoc clarified the preference.

It is possible to reverse the order.

However, since HTTP/3 is not compatible with HTTP/2 and HTTP/1, there is no way to "downgrade" the attempt (and switch from UDP to TCP), so if the server does not support HTTP/3 the request will fail.
Applications may handle the failure and attempt to specify explicitly the HTTP version via request.version(...).
This is not done automatically by HttpClient.

The reverse order may be useful if you want your default to be HTTP/3, but you want to explicitly specify HTTP/2 or HTTP/1 for those servers you know don't support HTTP/3.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbordet if the order of preference is HTTP/1, HTTP/2, HTTP/3 does that mean that the client will settle on HTTP/1 if the server supports all three protocols? Or will it try to upgrade the connection to HTTP/3?

I suspect that the client will settle on HTTP/1. If this is the case, how could one configure the client to try HTTP/3 and if that fails try HTTP/1 followed by an upgrade to HTTP/2? And if the upgrade to HTTP/2 fails then stay on HTTP/1.

I may be wrong, but this sounds like the happy path to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTTP/2 does not usually work with upgrades, it's negotiated via TLS + ALPN.

A client configured with http/1.1, h2 will send that to the server, and the server will likely pick h2 because it's the server that decides.

Trying HTTP/3 with a server that does not support it will result in a failure after the connect timeout (differently from TCP, where the failure is almost immediate).
After that timeout, the application may try explicitly HTTP/2.

That's why it's probably most common to do the opposite: if you know the server supports HTTP/3, you explicitly try that; otherwise you are not explicit and therefore HTTP/1.1+HTTP/2 will be negotiated.

Likely, as of now, the best configuration for the client is therefore: HTTP/2, HTTP/1.1, HTTP/3.

Copy link
Contributor

Choose a reason for hiding this comment

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

As usual, nothing is simple in life :) Check out this interesting discussion related to HTTP/3 negotiation: https://stackoverflow.com/a/77408325/14731 and https://stackoverflow.com/a/67470164/14731

I assume that if support is added for these approaches then users can move from the order HTTP/2, HTTP/1.1, HTTP/3 to HTTP/3, HTTP/2, HTTP/1.1.

I'm not sure what Jetty can do with the Alt-Svc approach though. In the case of browsers, they say that future connections will then try HTTP/3 first but in the case of Jetty can the HttpClient instance do the same? Meaning, the first connection would try HTTP/2, HTTP/1.1, HTTP/3 but if HTTP/3 support is found then subsequent connections to the same host would try HTTP/3 first.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbordet I guess the only thing I'm wondering is whether:

  1. I can start out with an HTTP/2, HTTP/1.1, HTTP/3 preference.
  2. Upon making the first call to a hostname, take note whether the server response contains a Alt-Svc: h3="<port number> header.
  3. Configure Jetty to use a different order of preference for that host in future calls.

If Jetty doesn't provide a way to set preferences on a per-host basis, one thing I could do is create one HttpClient per preference order (one normal, one that prefers HTTP/3) and use application-level logic to issue requests with one or the other depending on whether the host is known to support HTTP/3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bullet 3 should be: Explicitly set the HTTP version on the request via request.version(HttpVersion.HTTP_3).

That's it.

The option of configuring different HttpClient instances is not one I would suggest.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbordet Filed #11407. Thank you.

Copy link
Contributor

@cowwoc cowwoc Feb 17, 2024

Choose a reason for hiding this comment

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

I got HTTPS/3 working on the server side but I can't get past the dreaded "PKIX path building failed" exception on the client side. Even hitting "google.com" fails with this error. I'll let you know once I figure this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbordet It looks like this PR is fine. See #11417 for a follow-up discussion.

lorban
lorban previously approved these changes Feb 20, 2024
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

Besides EndPoint.Pipe which I think isn't needed as I do not think we're going to add other pipe connectors in the future, LGTM.

/**
* <p>A communication conduit between two peers.</p>
*/
interface Pipe
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need this interface in the IO API.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet requested review from gregw and lorban February 23, 2024 15:48
@joakime joakime added Enhancement Http Client For issues encountered in Browser and Http Client implementations labels Feb 23, 2024
gregw
gregw previously approved these changes Feb 26, 2024
…meResolution().

Marked HttpDestination constructor as deprecated for removal.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet merged commit 24c1140 into jetty-12.0.x Feb 26, 2024
2 of 4 checks passed
@sbordet sbordet deleted the fix/jetty-12/8979/httpclient-network-modes branch February 26, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Http Client For issues encountered in Browser and Http Client implementations
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Jetty 12 - HttpClientTransport network "modes"
6 participants