strimzi / kafka-kubernetes-config-provider

Kubernetes Configuration Provider for Apache Kafka®
Apache License 2.0
25 stars 12 forks source link

Override okhttp 3.x pulled in by Fabric8 Kubernetes Client with okhttp 4.x #16

Closed mstruk closed 1 year ago

mstruk commented 1 year ago

Fabric8 Kubernetes Client 6.2.0 pulls in a dependency on okhttp-3.x which during bundling into kafka/libs folder conflicts with okhttp-4.x brought in by opentelemetry.

Only one can be present in kafka/libs.

OkHttp 4.x is a drop-in replacement for 3.x so we can replace it in this project to catch any potential issues (none expected) as soon as possible.

Instructions are available at: https://github.com/fabric8io/kubernetes-client/blob/v6.2.0/doc/KubernetesClientWithIPv6Clusters.md

Signed-off-by: Marko Strukelj marko.strukelj@gmail.com

mstruk commented 1 year ago

The only alternative I can see is to override okhttp-4.x brought in by OpenTelemetry to version 3.12.12 (tested here as you pointed out).

[INFO] ------------------< io.strimzi:kafka-thirdparty-libs >------------------
[INFO] Building kafka-thirdparty-libs 1.0.0-SNAPSHOT
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] io.strimzi:kafka-thirdparty-libs:pom:1.0.0-SNAPSHOT
[INFO] +- io.opentelemetry:opentelemetry-exporter-jaeger:jar:1.18.0:compile
[INFO] |  +- com.squareup.okhttp3:okhttp:jar:4.10.0:compile

I would say that it's generally safer to upgrade a version of the dependency, than to downgrade the version of the dependency. Especially when the higher version claims to be a drop-in replacement for the lower version.

mstruk commented 1 year ago

Also, the Fabric8 Kubernetes Client project themself provides instruction for how to perform such an upgrade.

scholzj commented 1 year ago

@mstruk Sorry, but I do not understand what problem are you trying to solve. This is a library which is used in many different places and situations. If someone somewhere mixes it with other libraries using other versions of OkHttp, it should be dealt with there and not here because there might be other users who maybe want to use OkHttp 3 and this override will screw them up. So I think we should stick with the default used by Fabric8 here.

As for the Kafka third-party-libs, I do not think they have any issues with this. Maven seems to deal with it on its own and actually picks OkHttp 4 -> but that does not mean anything for the dependencies used here.

mstruk commented 1 year ago

Ok, it was my understanding that the primary use of this library is to be bundled into Operator's Kafka images. I wasn't aware of any other uses. By bringing in OpenTelemetry with its dependencies into Kafka it makes sense to me to align on the versions, and make sure each component works fine with the dependency version shared by all in the end.

As long as Kafka bundling is the only destiny of this component, then the only runtime dependency of this component will in the end be 4.x as automatically picked by Maven dependency resolution. Makes no sense to me to have 3.x here at build time, and 4.x at runtime.

If this component has other uses / users then yeah the first point of testing for the operator in the Operator testsuite makes sense.

scholzj commented 1 year ago

You can for example use this in clients which is also one of the use-cases described in the README. So no, this is not there only for the Strimzi operator Kafka images

mstruk commented 1 year ago

Very well then. Closing this.