feat: Jetty HttpClient implementation #4180

manusa · 2022-05-30T17:04:04Z

Description

feat: Jetty HttpClient implementation

HttpClient Tests
- Added HttpClient behavioral abstract tests that should be implemented for each of the HttpClient implementation modules (currently added implementations for OkHttp and Jetty).
Jetty HttpClient implementation
- Java 11 optional module
- Should provide full-support for all HttpClient features

Follow up tasks

Enable HTTP/2 when MockWebServer provides adequate support
MockWebServer HTTP/2 full support #4193
Refactor sendAsync for InputStream or Reader types so they don't read the full response in memory (easy refactor, but I prefer to do this separately ---> Not urgent)
[refactor] Change JettyHttpClient#sendAsync method to not read full response in memory #4201
~~OkHttp & Jdk HttpClient failed response interceptors don't seem to work with consumeLines or consumeBytes~~
- ~~afterHttpFailureReplacesResponseInConsumeLines();~~
- ~~afterHttpFailureReplacesResponseInConsumeBytes();~~
- ~~See OkHttpInterceptorTest & JdkHttpClientInterceptorTest~~
~~AsyncBody.consume has a confusing and inconsistent behavior~~
- ~~feat: Jetty HttpClient implementation #4180 (comment)~~
- ~~JdkHttpClientAsyncBodyTest completely disabled due to non-compliance with prepared test cases~~

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

shawkins

Looks very good Marc, just a couple comments.

httpclient-jdk/src/main/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpRequestImpl.java

shawkins · 2022-06-03T01:34:19Z

...lient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyAsyncResponseListener.java

+  @Override
+  public void onContent(Response response, ByteBuffer content) {
+    try {
+      consumeLock.await();


What is the threading model for Jetty? Is it like okhttp?

More info is here: https://www.eclipse.org/jetty/documentation/jetty-11/programming-guide/index.html#pg-client-io-arch-network

We can fine-tune pools and so on in our ClientConnector instance.

For me it's unclear what the consume lock does. I expect some timeouts to happen depending on the HttpClient implementation if the lock is kept for too long.

A timeout can be something easy to be added here based on the inherent client timeouts. Also better cancellation support.

shawkins · 2022-06-03T01:38:10Z

httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyHttpClientFactory.java

+
+import static io.fabric8.kubernetes.client.utils.HttpClientUtils.applyCommonConfiguration;
+
+public class JettyHttpClientFactory implements HttpClient.Factory {


The other factories provide several hooks. Do you prefer something simpler? We could minimize the methods on those factories as well.

I forgot about this one. Yup, need to add some hook too. Also maybe create an interface to keep things consistent.

httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyHttpClient.java

manusa · 2022-06-03T12:08:01Z

I'm facing issues with the Exec streamIDs, for some reason they are not OK, and the ExecListener behaves erratically.

...es-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractWebSocketSendTest.java

shawkins · 2022-06-07T15:47:55Z

httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyHttpClient.java

+      //       Find a way to stream the response body without completing the future
+      //       We need two signals, one when the response is received, and one when the body is completely
+      //       read.
+      //       Should this method be completely replaced by consumeXxx()?


Yes we'll just need to capture an issue for that. We can provide something like a utility to convert the consumeBytes into a ReadableByteChannel.

There's two thing here:

First one is addressing the in-memory load of the response, which I'll fix in a subsequent PR.

Second is about "merging" the sendAsync and consumeLines/consumeBytes since they seem redundant. I guess aligned with your thoughts. An issue for this sounds good.

shawkins · 2022-06-07T15:49:24Z

httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyHttpClientBuilder.java

+  private Origin.Address proxyAddress;
+  private String proxyAuthorization;
+  private TlsVersion[] tlsVersions;
+  // TODO: HTTP2 disabled, MockWebServer support is limited and requires changes


I don't recall if there's anything captured on the expect continue behavior of the mock server - it requires more logic to respond with continue and at least the jdk client won't progress if it doesn't see that response.

The handshake is not correctly implemented in the MockServer for non https responses (and probably for https too).
It works for OkHttp, but I'm not sure why (I guess it's more lenient).

I'll create a new issue for this once we merge. I already have some changes on the MockServer, but didn't want to spend more time on it, hence the TODO.

...rnetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractAsyncBodyTest.java

...lient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientAsyncBodyTest.java

kubernetes-itests/src/test/java/io/fabric8/kubernetes/PodIT.java

kubernetes-itests/pom.xml

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/BaseClient.java

Signed-off-by: Marc Nuri <[email protected]>

also making jetty consume apply to each message, and removing the okhttp requirement for a newline

sonarqubecloud · 2022-06-10T05:59:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

88.5% Coverage
0.0% Duplication

manusa added the wip label May 30, 2022

manusa self-assigned this May 30, 2022

manusa requested a review from shawkins May 30, 2022 17:04

manusa force-pushed the feat/httpclient-jetty branch 6 times, most recently from e394b18 to 5b82ae8 Compare May 31, 2022 11:18

manusa mentioned this pull request May 31, 2022

Jetty 11 WebSocket Client: SSL/TLS authentication using client certificate jetty/jetty.project#8085

Closed

manusa force-pushed the feat/httpclient-jetty branch 7 times, most recently from a8c50e1 to 14c5f06 Compare June 2, 2022 17:00

manusa removed the wip label Jun 2, 2022

manusa marked this pull request as ready for review June 2, 2022 17:02

manusa added this to the 6.0.0 milestone Jun 2, 2022

manusa force-pushed the feat/httpclient-jetty branch from 14c5f06 to 7ef6d3a Compare June 2, 2022 17:06

manusa requested a review from rohanKanojia June 2, 2022 17:19

shawkins reviewed Jun 3, 2022

View reviewed changes

manusa force-pushed the feat/httpclient-jetty branch from 7ef6d3a to 9e8ef20 Compare June 3, 2022 11:43

manusa force-pushed the feat/httpclient-jetty branch 5 times, most recently from 9b2863d to 99fe926 Compare June 6, 2022 07:57

manusa force-pushed the feat/httpclient-jetty branch 6 times, most recently from 3cb5494 to 9f2ff3c Compare June 7, 2022 11:08

shawkins reviewed Jun 7, 2022

View reviewed changes

manusa force-pushed the feat/httpclient-jetty branch 2 times, most recently from 9754ce3 to 65b7900 Compare June 9, 2022 16:19

manusa commented Jun 9, 2022

View reviewed changes

kubernetes-itests/src/test/java/io/fabric8/kubernetes/PodIT.java Outdated Show resolved Hide resolved

manusa commented Jun 9, 2022

View reviewed changes

kubernetes-itests/pom.xml Outdated Show resolved Hide resolved

manusa commented Jun 9, 2022

View reviewed changes

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/BaseClient.java Show resolved Hide resolved

manusa force-pushed the feat/httpclient-jetty branch from 65b7900 to ed7de60 Compare June 9, 2022 16:55

feat: Jetty HttpClient implementation

9e5aa4c

Signed-off-by: Marc Nuri <[email protected]>

manusa force-pushed the feat/httpclient-jetty branch from ed7de60 to 8632d4b Compare June 10, 2022 05:20

correcting jdk logic wrt interceptors and subscriptions

052ca50

also making jetty consume apply to each message, and removing the okhttp requirement for a newline

manusa force-pushed the feat/httpclient-jetty branch from 8632d4b to 052ca50 Compare June 10, 2022 05:29

manusa merged commit 1ce8e20 into fabric8io:master Jun 10, 2022

manusa deleted the feat/httpclient-jetty branch June 10, 2022 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Jetty HttpClient implementation #4180

feat: Jetty HttpClient implementation #4180

manusa commented May 30, 2022 •

edited

Loading

shawkins left a comment

shawkins Jun 3, 2022

manusa Jun 3, 2022

shawkins Jun 3, 2022

manusa Jun 3, 2022

manusa commented Jun 3, 2022

shawkins Jun 7, 2022

manusa Jun 8, 2022

manusa Jun 10, 2022

shawkins Jun 7, 2022

manusa Jun 8, 2022

manusa Jun 8, 2022

sonarqubecloud bot commented Jun 10, 2022


		import static io.fabric8.kubernetes.client.utils.HttpClientUtils.applyCommonConfiguration;

		public class JettyHttpClientFactory implements HttpClient.Factory {

feat: Jetty HttpClient implementation #4180

feat: Jetty HttpClient implementation #4180

Conversation

manusa commented May 30, 2022 • edited Loading

Description

Follow up tasks

Type of change

Checklist

shawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manusa commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 10, 2022

manusa commented May 30, 2022 •

edited

Loading