Adding an api layer to isolate Okhttp

[shawkins]

Description

This is to address #3547

Lower level implementation details can be discussed here. This will be somewhat long running as there is quite a bit of okhttp usage.

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

[centos-ci]

Can one of the admins verify this patch?

[shawkins]

For simplicity the proposed changes to BuildConfigOperationsImpl produce a more verbose error message on all exceptions, not just IOExceptions related specifically to the writing the inputstream.

[shawkins]

This should be mostly* working up to the kubernetes-tests. I haven't yet pulled some of the deprecated methods off of DefaultKubernetesClient and related. Other than that the only remaining direct references to okhttp in non-test code are in the io.fabric8.kubernetes.client.internal.okhttp package.

The HttpClientUtils will need broken apart. The are some utility methods that need to be free of okhttp, like the creation of basic credentials. There is also the init logic for okhttp which will need to be encapsulated through a service/factory

OpenIDConnectionUtils - needs to be rewritten in terms of the new api, or if support gets bumped to java 11 could just be directly written for the java 11 client. *Related to this, the TokenRefreshInterceptorTest has been disabled for now.

I've been writing a java 11 implementation on the side. There are a couple of pain points that may require a few additional tweaks to the api layer.

[shawkins]

Everything other than the TokenRefreshInterceptorTest has been prepped for the separation of the okhttp dependency.

There are still other test dependencies to okhttp - but that is allowable, or at the least can be removed later.

@manusa @rohanKanojia @iocanel can you see if what has been done so far makes sense? The new api is an amalgam of the okhttp client usage and the java 11 apis - it obviously still needs documented more thoroughly.

I do have an implementation using java 11 - see https://github.com/shawkins/httpclient The obvious pain point is adding the interceptor pattern - which needs to also cover websocket requests. The java api completely hides that from the user, so the Interceptor interface in the new api is stripped down as much as possible. One minor issue is that it's not fully non-blocking as the calls to refresh the tokens will be blocking - but I think that can be addressed if needed. I have not yet added a similar createHttpClient method - not all config properties will be applicable, and some of the logic will end up moving to a common spot back in HttpClientUtils..

TODOs have been added anywhere that needs additional thought. Another thing to note is that the adapt logic for the openshift client has been made to piggy-back more on the main configuration.

I have not yet made the client available from the config, I would like to tackle that after the initial work is done.

[maxandersen]

I raised this pr in quarkus chat - details at https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/kubernetes.20client.20okhttp.20isolation - but summary is:

With a Java 11 enabled http client we can always use https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpClient.Builder.html#executor(java.util.concurrent.Executor) to wire in a vert.x/quarkus based executor so the same thread/execution model is used. Simplifies configuration and avoids unnecessary thread creation.

It would be nice to have a vert.x http client natively implemented so even in native compiled apps we can strip out even more classes; but that is secondary. Big win not having to deal with okhttp at least :)

So from Quarkus perspective we should be fine here and we can evolve from this point.

[metacosm]

LGTM, hopefully we will be able to wire in a vert.x client in a not-so-distant future! 😉

[shawkins]

LGTM, hopefully we will be able to wire in a vert.x client in a not-so-distant future!

Hopefully - I did start down that path, but quickly got bogged down - #3547 (comment) Maybe there is someone on their team who could pick that up if they want to go beyond the jdk impl.

[shawkins]

On the 4 sonarcloud bugs https://sonarcloud.io/project/issues?id=fabric8io_kubernetes-client&open=AXziolikSmmHNwo2Qbf6&pullRequest=3549&resolved=false&types=BUG - 3 are with the usage of the WritableByteChannel. Since we close the underlying streams, there's no need for an additional close. I'm just using that logic to write out the ByteBuffer. We also need a reference to the underlying streams to perform a flush - that is not done through a WritableByteChannel. It would probably be a good idea to add flushes to the error calls - I can do it with this pr or separately.

On the other one the isPresent call is there. It probably just doesn't like multiple calls to previousResponse

[shawkins]

@manusa it probably goes without saying, but feel free to squash this as the individual commits at this point don't convey much. alternatively I can rebase it if you want.

[rohanKanojia]

Great effort, Thanks a lot! 🏋️‍♂️

[metacosm]

Great effort, Thanks a lot! 🏋️‍♂️

Herculean effort, indeed! 💪🏼

[manusa]

@manusa it probably goes without saying, but feel free to squash this as the individual commits at this point don't convey much. alternatively I can rebase it if you want.

The commit history for the PR is not adding much value at this point. I do think it's better if we squash.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

4 Bugs
5 Vulnerabilities
1 Security Hotspot
22 Code Smells

28.6% Coverage
2.4% Duplication

[manusa]

LGTM
Awesome job, thank you very much!

[manusa]

A basic check with JKube worked OK, everything appears to work as intended 🚀