decouple from okhttp(3)

[maxandersen]

Description

Currently quarkus ends up having a dependency to okhttp3 via the kubernetes client. This issue tries to gather the issues/options as fixing this potentially involves collaboration across Quarkus, fabric8 io kubernetes client, operator SDK and the OpenJDK team.

This is a root cause of a list of issues:

1. CVE issue list for okhttp3 keeps growing (albeit at this time of writing not aware of issues that gets exposed via Quarkus usage of okhttp3 but it is problematic if anyone do need to use okhttp for other means)
2. We prefer to use one HTTP client (vert.x) to reduce exposure of CVE's and in vert.x case to be able to reuse the same asyn event loop wherever possible
3. Upgrading to okhttp4 would help on both Switch to the Maven distributed copy of the SubstrateVM annotations #1 and Make it possible to use docker to build native images #2 but then we drag in a reliance on kotlin stdlib which has all kinds of potential downsides when it comes to dependency alignment and support in native.

Thus the best option will be to find ways to not introduce more CVE exposure, not add more libraries and still have good async and native support.

On Kubernetes client side they now have support for different HTTP clients. We should make that work for us!

There are 3 client options:

Jetty

This client is somewhat complete but has some issues around blocking behaviours. Adding Jetty into Quarkus managed dependency set does not sound like a good option.

Users can though update the dependency to use it if they so wish.

JDK http client

This is working but only recommended for Java 16 and upwards due to JDK 11 bug affecting query parameters in websocket based calls.

Also would be improved when cleaner async support merged

The JDK bug we can probably get backported to JDK 11. Will take a few months before broadly available though (January earliest).

vert.x

Current progress is in this branch.
It has issues and is awaiting fabric8io/kubernetes-client#4430 to get merged. This PR fixes issues
around how the client deals with bytebuffers and streams. With this in a vert.x client should be much more manageable to get in.

Implementation ideas

My (Max) preferable option is to get vert.x client enabled as that avoids all the issues know for okhttp.
Secondary option is to get jdk issue fixed allowing users to at least just fall back to the JDK.

Jetty I'm not a fan of as it adds yet another dependency chain to maintain.

[quarkus-bot]

/cc @Sgitario, @evanchooly, @geoand, @iocanel

[geoand]

cc @manusa @metacosm

[maxandersen]

/cc @metacosm @manusa @shawkins @vietj - fyi

/cc @cescoffier - the issue I mentioned last week.

[maxandersen]

@manusa anything blocking fabric8io/kubernetes-client#4430 from getting merged?

@vietj @shawkins : if above is merged; when could we have a vert.x client available? how much effort is left?

[manusa]

@manusa anything blocking fabric8io/kubernetes-client#4430 from getting merged?

Working on this ATM

[shawkins]

@maxandersen @manusa 4430 has been committed. The next refinement will be fabric8io/kubernetes-client#4619 which will remove the need to implement the consumeLines method. This pr has already been included in the vertx branch https://github.com/fabric8io/kubernetes-client/tree/vertx-client - so just look at the last commit for the initial vertx implementation.

You can run VertxHttpClientTest as an initial fabric8 level validation of the logic - it will fail most tests. In particular ssl is not configured correctly to work with the fabric8 mock server.

As for what still needs to be done:

All of the client factory TODOs and UnsupportedOperationExceptions (several are commented out), need to be cross checked or implemented: https://github.com/fabric8io/kubernetes-client/blob/vertx-client/httpclient-vertx/src/main/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientFactory.java#L50 - that will then get us aligned with how the client is configured in fabric8.

The next pain point is the application of interceptors. This needs to happen for both regular request (consumeBytes) and websocket requests.

There is code stubbed in there now for regular request interceptors https://github.com/fabric8io/kubernetes-client/blob/vertx-client/httpclient-vertx/src/main/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientFactory.java#L234 however that breaks as creating a wrapper with the same class type will throw an exception.

For websocket requests before interceptors are applied here https://github.com/fabric8io/kubernetes-client/blob/vertx-client/httpclient-vertx/src/main/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientFactory.java#L181 - but the interceptor after failure case needs handled as well.

We are of course open to elevating interceptor logic out of what each client must implement if that will help.

Other than the inteceptor logic the websocket stuff should be working as it's pretty straight-forward.

I've roughed in an implementation of consumeBytes https://github.com/fabric8io/kubernetes-client/blob/vertx-client/httpclient-vertx/src/main/java/io/fabric8/kubernetes/client/vertx/VertxHttpRequest.java#L54 but did not immediately see how one would cancel the response.

consumeBytes and a websocket call will work when using the VertxUnitRunner instead of our mock server - see the HttpClientTest that @vietj added. However the presence of that test means that we cannot use that project to run additional tests that @manusa created called AbstractXXXTest, which require creating a test class e.g. JettyHttpPostTest. So we need to do one of the following:

• remove the VertxUnitRunner test
• create tests like these in httpclient-tests
• rethink how we're doing this to be more like the integration test runs, such that we have single test suite that can rerun for the various implementations via just a config / build change.

[iocanel]

AFAIR, interceptors were introduced to support some openshift specific use cases regarding authentication (e.g. perfrom oc login if needed). At the time okhttp provided interceptors so we just used them. In theory we could have implement this logic at a higher level. I am wondering if we should do that now, in order to break the dependency with non standard http client features.

Is this something we can do, or we have more use cases nowdays relying on interceptors?

[manusa]

Testing

One of my pending goals is to remove the httpclient-tests module and consolidate every test in an Abstract test that should/is then be implemented for each module. The abstract tests contain a single abstract method to provide the HttpClient implementation to be tested and shouldn't override or extend anything else (tests that don't pass can be overridden to highlight that some of the features are not yet covered by the implementation).

The main purpose of these tests is to ensure a consistent behavior across the different HttpClient implementations verifying that every feature behaves according to specification. Tests must be black-boxed and shouldn't rely or know about any of the implementation details.

Extending each of the abstract tests in the appropriate implementation module makes it easier for Sonar to calculate coverage, and for IDE (at least on IntelliJ) users to test specific features.

These tests should ease the development and maintenance of any of the client implementations and should not imply any additional burden (if the effect is the opposite, then we should discuss why). Currently, the HTTP server used is the one provided by OkHttp, but replacing this if needed shouldn't be a problem.

Regarding the new httpclient-vertx module, I not only see the problem with the VertxUnitRunner, but also with the dependency with JUnit4. We should only use JUnit5. I assume that if the mentioned test needs to be preserved, the runner could be replaced with the VertxExtension (?). I'm guessing that this would allow to preserve all tests (the common abstract test implementations and any of the extra ones you need to provide).

Interceptors

Interceptors were already abstracted away and provided as an agnostic feature to intercept requests (, request failures, and websocket failures) from HttpClient regardless of the underlying implementation.

We have now many use-cases for interceptors baked-into the client (TokenRefresh, OpenShift OAuth, Backwards compatibility, etc.), and probably some users have already built their own.

What Steven is suggesting (correct me if I'm wrong), is elevating any interceptor-related logic into the abstraction (not sure how, probably easier now that we've elevated other stuff too) in case providing the full implementation is difficult or unachievable.

[shawkins]

Regarding the new httpclient-vertx module, I not only see the problem with the VertxUnitRunner, but also with the dependency with JUnit4.

Yes that is what I mean. I don't think it necessarily needs to be preserved - that's currently the only test class in the module, and it only has two tests (1 for a regular request, 1 for a websocket). Given our other tests supersede these, I'd be fine with removing it.

What Steven is suggesting (correct me if I'm wrong), is elevating any interceptor-related logic into the abstraction (not sure how, probably easier now that we've elevated other stuff too) in case providing the full implementation is difficult or unachievable.

Yes, I'm proposing: fabric8io/kubernetes-client#4628 - it's not the greatest abstraction, but it minimizes the concerns of the httpclient implementation.