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

Create an abstraction layer for okhttp usage #3547

Closed
shawkins opened this issue Oct 26, 2021 · 7 comments
Closed

Create an abstraction layer for okhttp usage #3547

shawkins opened this issue Oct 26, 2021 · 7 comments
Assignees

Comments

@shawkins
Copy link
Contributor

In order to eventually do #2764 and related issues we should isolate the direct usage of okhttp. My proposal is to introduce a JDK like set of interfaces that we can further align to the built-in ones once fabric8's min version is bumped to 11.

There will be some breakage / deprecations in doing this, including:

  • client constructors that take OkHttpClient
  • OperationContext withOkHttpClient
  • PatchType getMediaType
  • ExecListener usage of Response
  • Config TlsVersions
  • you could not mix versions of client and extension jars that use Adapters to register handlers.

The intent would be to leave okhttp compatibility for some time though.

@shawkins shawkins self-assigned this Oct 26, 2021
@shawkins
Copy link
Contributor Author

The high-level tasks here are:

  1. isolate okhttp uage in kubernetes-client via Java 11 like interfaces [mostly complete]
  2. repeat as needed of the remaining modules. The interfaces should be complete enough for this to go relatively quickly.
  3. after substantial review commit this work to a common branch - that location is TBD
  4. since we are now talking about targeting these changes to fabric8 6, we will completely sever the dependency in kubernetes-client to okhttp. This will require creating 2 modules - one for the api, and one for okhttp support. Along with removing some of the proposed deprecations from 1 (for example it will no longer be an option to directly pass an okhttp client in the defaultkubernetesclient constructor)
  5. again since we are talking about fabric8 6, the direct reference even to the new client interface could be minimized. In particular the operation constructors and other places where both a config and a client are passed should be refactored. The simplest alternative will be to make the client available from the config.
  6. If the java 11 bump happens in this same timeframe more effort should be put into proving java 11 client support. This requires determining how the client should be configured (existing config properties may not be well aligned) and additional refactoring to all for the interceptor logic to be used across implementations. This also involves moving the OpenID logic off of it's direct usage of okhttp client.

@shawkins
Copy link
Contributor Author

The OpenshiftAdapterSupport logic presumes the usage of okhttp as well - so I'll need to determine how to abstract interceptors for Java 11 otherwise another module will be needed just for that.

@shawkins
Copy link
Contributor Author

1, 2, and 5 are complete

I've updated the pr for review to complete 3.

4 is no longer a priority - we are talking about making these changes for 5.11, not 6, such that the initial incarnation will still only offer the okhttp implementation.

6 is nearly ready - https://github.com/shawkins/httpclient - there's just 1 issue left to resolve around expect continue behavior.

I started looking at what is needed for the implementation of a vert.x client. At first glance that does seem to be more involved. It would use the vert.x core HttpClient for websockets and the WebClient for everything else, which complicates the factory / initialization logic - it would make sense to just introduce a base builder class. The conversion of InputStreams to ReadStreams seems to require an additional class https://stackoverflow.com/questions/66158455/how-to-create-a-vert-x-readstream-from-an-inputstream And the handling of send (instead of the body handling being part of the request builder, it's something you do on the request) and the additional indirection of futures / handlers will just take more time in general to wire up.

@joschi
Copy link

joschi commented Dec 28, 2021

@shawkins After running into a few issues with the new HTTP client abstraction in dropwizard/dropwizard-kubernetes#147, I think it would be useful to customize the OkHttpClient (not OkHttpClient.Builder) instance.

Specifically the ability to add a wrapper around the actual OkHttpClient instance would be great, for example for adding instrumentation to the OkHttp client via https://github.com/raskasa/metrics-okhttp.

@shawkins
Copy link
Contributor Author

@joschi feel free to open a new issue or pr. The goal moving forward will be to de-emphasize okhttp.

@shawkins
Copy link
Contributor Author

Related to this issue, there's a possible abstraction for creating the clients is at 2d22886 - this makes use of the client factory directly.

I have one more change needed for this to support the jdk client - Sec-WebSocket-Protocol is a reserved header for the jdk client, so there needs to be a method to set subprotocols on the websocketbuilder. I've made that change locally and updated https://github.com/shawkins/httpclient - testing with JDK 17 I see all tests and itests complete except for the pod uploads - there seems to be a mismatch in the expectations with "expect continue" between the jdk client and the mock server.

@manusa manusa mentioned this issue Feb 14, 2022
11 tasks
@shawkins
Copy link
Contributor Author

Marking as resolved. We have settled on the initial api split and how clients are created. There will be further refinements as needed to support additional clients, such as vert.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants