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.

[shawkins]

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

@iocanel @manusa I think we're in a much better starting point now as we've also elevated the interceptor concerns. This leaves a much more bare bones implementation that we're shooting for.

@maxandersen the branch has been updated - fabric8io/kubernetes-client@master...vertx-client who would be a good contact to work through the remaining TODOs and unimplemented exceptions?

[maxandersen]

@vietj and @iocanel said they would help on this.

[cescoffier]

@vietj told me this morning that everything should be fine.

[geoand]

There is a PR bumping the client to a new version that includes Vert.x

[jtama]

@geoand the okhttp is still enforced in the platform : https://github.com/quarkusio/quarkus/blob/3.0.0.Alpha4/bom/application/pom.xml#L177

Is this expected ?

[metacosm]

@jtama as far as I'm aware there's still a dependency on okhttp (at least) for test support of the kubernetes client.

[jtama]

Ok I thought this issue was about decoupling and stop enforcing the okhttp version...
I maintain the minio extension which is stucked with an old version of the sdk because of that dependency.

Is it ok to override this in an extension ?

[manusa]

Ok I thought this issue was about decoupling and stop enforcing the okhttp version... I maintain the minio extension which is stucked with an old version of the sdk because of that dependency.

Is it ok to override this in an extension ?

OkHttp should only be used for testing purposes as a test dependency.

OkHttp shouldn't be required at all for runtime. If you're using the Kubernetes Mock Server, you will need the dependency, but overriding it with the latest OkHttp should be fine (haven't tried it though).

[jtama]

Actually okhttp is still en forces in the bom.

That means an extension CAN use okhttp 4, but any applications using it will have to redeclare okhttp version.

[sberyozkin]

@manusa Hi,

OkHttp shouldn't be required at all for runtime. If you're using the Kubernetes Mock Server, you will need the dependency, but overriding it with the latest OkHttp should be fine (haven't tried it though).

FYI, bumping OkHttp version to 4.10.0 in the 3.2 branch, in bom/application/pom.xml, causes:

023-09-30T22:04:46.8018369Z [INFO] Running io.quarkus.it.openshift.client.OpenShiftClientTest 2023-09-30T22:04:55.1786013Z [ERROR] Tests run: 2, Failures: 0, Errors: 1, Skipped: 1, Time elapsed: 8.378 s <<< FAILURE! - in io.quarkus.it.openshift.client.OpenShiftClientTest 2023-09-30T22:04:55.1787513Z [ERROR] io.quarkus.it.openshift.client.OpenShiftClientTest.createRoute Time elapsed: 0.02 s <<< ERROR! 

2023-09-30T22:04:55.1788474Z java.lang.RuntimeException: java.lang.NoSuchMethodError: 'void okhttp3.internal.Internal.initializeInstanceForTests()' 2023-09-30T22:04:55.1789328Z at 

io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:640) 2023-09-30T22:04:55.1790206Z at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:711)

[jtama]

Actually okhttp is still en forces in the bom.

That means an extension CAN use okhttp 4, but any applications using it will have to redeclare okhttp version.

As stated here, although the issue is closed, quarkus hasn't finalized it's decoupling and OkHttp is still in the bom.

[manusa]

Hi @sberyozkin

FYI, bumping OkHttp version to 4.10.0 in the 3.2 branch, in bom/application/pom.xml, causes:

That's weird, OkHttp folks assure 4.x is binary compatible with 3.x.

Is this also happening with prior versions of OkHttp 4.x?

It would be good to open an issue in the Fabric8 repo to better track the problem and its solution.

[manusa]

As stated here, although the issue is closed, quarkus hasn't finalized it's decoupling and OkHttp is still in the bom.

AFAIR there was a different issue tracking the progress of the OkHttp removal. There are other extensions that declare a dependency to OkHttp too.

[jtama]

I didn't found a way to use OKHttp 4 without forcing my consumer to also enforce v4, but if there is a way, I'd be very happy to use it ! Do you know which extensions ?

[geoand]

The only thing I remember is Infinispan needing OkHttp, is that correct @karesti ?

[sberyozkin]

@manusa Hi, sorry, missed your response.

That's weird, OkHttp folks assure 4.x is binary compatible with 3.x.

Perhaps at the level of the code not related to the testing support ? I guess it would be unusual to have the okhttp3.internal.* package found in the 4.x stream.

Is this also happening with prior versions of OkHttp 4.x?

I haven't done testing across all 4.x versions, AFAIK we have 4.10.0 productized in some form, which is why chose that version.

[gsmet]

That's not the whole story : https://groups.google.com/g/quarkus-dev/c/C2b45e5l6l0/m/d7d-VrHpGQAJ . I plan on working on moving SmallRye OpenTracing and Jaeger to the Quarkiverse so it should be fine on this front. And then there are the test dependencies for Infinispan and Kubernetes client (not sure if it's still a problem there, my investigation is quite old), which might or might be a problem. If they are part of an external BOM, maybe Okhttp will be resolved in the platform if it's out of our BOM but coming from other dependencies. Alexey would probably know more about it. I'm working on a few other things before that but I plan to move the extensions this month or early November.

…

On Tue, Oct 3, 2023 at 4:58 PM Georgios Andrianakis < ***@***.***> wrote: The only thing I remember is Infinispan needing OkHttp, is that correct @karesti <https://github.com/karesti> ? — Reply to this email directly, view it on GitHub <#29520 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJYOBJ7UAKMDZHW2V2US7LX5QR2BAVCNFSM6AAAAAASNDJKECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBVGE2TMNJVGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>