pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard

Published 20 hours ago • verified publisher icon apache

[Bug] Version conflicts with the protobuf inside the pulsar client

Open pqab opened this issue 11 months ago • 17 comments

Search before asking

  • [X] I searched in the issues and found nothing similar.

Version

Pulsar client & client admin 3.0.2

Minimal reproduce step

We have grpc application which is using protobuf 3.25.x, however when we build the proto files, the protobuf packed inside the pulsar client which is using an old version override our dependencies, causing issues in the grpc runtime envrionment, even if we tried to exclude from the gradle, it doesn't works, because it built inside the client jar directly

What did you expect to see?

The protobuf library inside the pulsar client shouldn't override our dependencies

What did you see instead?

It took priority to load the library from the pulsar client

Anything else?

We have a workaround to use pulsar-client-original & pulsar-client-admin-original client instead

Are you willing to submit a PR?

  • [ ] I'm willing to submit a PR!

pqab avatar Mar 14 '24 02:03 pqab

Try to use pulsar-client-all

dao-jun avatar Mar 14 '24 04:03 dao-jun 

<!-- https://mvnrepository.com/artifact/org.apache.pulsar/pulsar-client-all -->
<dependency>
    <groupId>org.apache.pulsar</groupId>
    <artifactId>pulsar-client-all</artifactId>
    <version>3.0.3</version>
</dependency>

instead of

<dependency>
    <groupId>org.apache.pulsar</groupId>
    <artifactId>pulsar-client</artifactId>
    <version>3.0.3</version>
</dependency>
<!-- https://mvnrepository.com/artifact/org.apache.pulsar/pulsar-client-admin -->
<dependency>
    <groupId>org.apache.pulsar</groupId>
    <artifactId>pulsar-client-admin</artifactId>
    <version>3.0.3</version>
</dependency>

pulsar-client-all will shade these deps.

dao-jun avatar Mar 14 '24 05:03 dao-jun

image

pulsar-client-all has the same issue, the problem is the protobuf library inside the pulasr client is overriding our dependency, is it possible to decouple the protobuf from the client jar?

pqab avatar Mar 14 '24 05:03 pqab

I believe that the unshaded dependency has the artifact Id pulsar-client-original. Use should be able to override the version of protobuf when that is used.

lhotari avatar Mar 14 '24 06:03 lhotari

pulsar-client-all skipped shade protobuf-java And protobuf-java introduced by pulsar-client-origin. Maybe

    <dependency>
      <groupId>org.apache.pulsar</groupId>
      <artifactId>pulsar-client-original</artifactId>
      <version>3.0.3</version>
      <exclusions>
        <exclusion>
          <groupId>com.google.protobuf</groupId>
          <artifactId>protobuf-java</artifactId>
        </exclusion>
      </exclusions>
    </dependency>
    <dependency>
      <groupId>org.apache.pulsar</groupId>
      <artifactId>pulsar-client-admin-original</artifactId>
      <version>3.0.3</version>
      <exclusions>
        <exclusion>
          <groupId>org.apache.pulsar</groupId>
          <artifactId>pulsar-client-original</artifactId>
        </exclusion>
      </exclusions>
    </dependency>

can works

dao-jun avatar Mar 14 '24 06:03 dao-jun

yes, that's what we are using now we understand you shaded dependencies into the pulsar-client, if you move the shaded dependencies to under a shaded package or changing the namespace, then it won't affects the end user, we can use our own version of the protobuf with the pulsar-client

pqab avatar Mar 14 '24 07:03 pqab

yes, that's what we are using now we understand you shaded dependencies into the pulsar-client, if you move the shaded dependencies to under a shaded package, then it won't affects the end user, we can use our own version of the protobuf with the pulsar-client

This issue with protobuf in Pulsar client is similar to the problems with Avro (#9718, #13096, #21230) and Jackson (#6528) where you need to use pulsar-client-original dependency. I agree that this is not optimal.

Just a reminder that "we" and "you" are the same in an open source project. It's "we", the community, that can impact the development and improvements to Apache Pulsar. Contributions are welcome! Subscribing to the dev mailing list and joining the #dev channel in Slack are ways to get connected with the Apache Pulsar community.

lhotari avatar Mar 14 '24 07:03 lhotari

There's a slightly unrelated issue in Pulsar in upgrading the protobuf version that Pulsar uses internally between Pulsar and Bookkeeper. The version has to be the same in Pulsar and Bookkeeper currently. Dev mailing list thread is https://lists.apache.org/thread/s9g6w31vtwzgqf162hhlcr2nx3y68gv5 . It points to the Bookkeeper mailing list thread which contains more details.

lhotari avatar Mar 14 '24 07:03 lhotari

Regarding contributions, I think that a useful first contribution would be to update the documentation for Pulsar Java client to include the information about the workarounds for using a custom version of protobuf.

lhotari avatar Mar 14 '24 07:03 lhotari

image image

I'm wondering why do we skipped shade protobuf-java in pulsar-client, pulsar-client-admin and pulsar-client-all? Can we just shade it? @lhotari

dao-jun avatar Mar 14 '24 07:03 dao-jun

Regarding contributions, I think that a useful first contribution would be to update the documentation for Pulsar Java client to include the information about the workarounds for using a custom version of protobuf.

we will take some time to check the documentation, and might try to create some PRs later

pqab avatar Mar 14 '24 08:03 pqab

I'm wondering why do we skipped shade protobuf-java in pulsar-client, pulsar-client-admin and pulsar-client-all? Can we just shade it? @lhotari

@dao-jun You can always try. :) I don't think that shading is a solution. The reason is that it's expected to work with protoc generated classes which extend com.google.protobuf.GeneratedMessageV3. If you shade protobuf, that would break.

lhotari avatar Mar 14 '24 09:03 lhotari

slightly related to #19565 since that's making improvements in protobuf support.

lhotari avatar Mar 14 '24 11:03 lhotari

From 3.0.0, com.google.protobuf:protobuf-java is included directly in pulsar-client-admin without relocation.

In my understanding, pulsar-client-admin is a java wrapper for pulsar RESTful api. I'm curious what does protobuf-java in pulsar-client-admin used for?

~ download() {
  version=$1
  wget https://repo1.maven.org/maven2/org/apache/pulsar/pulsar-client-admin/${version}/pulsar-client-admin-${version}.jar
  unzip -tl pulsar-client-admin-${version}.jar | awk 'gsub(/\//,"/") < 3'
}

~ download 3.0.0
--2024-04-06 12:31:10--  https://repo1.maven.org/maven2/org/apache/pulsar/pulsar-client-admin/3.0.0/pulsar-client-admin-3.0.0.jar
Connecting to 127.0.0.1:6152... connected.
Proxy request sent, awaiting response... 200 OK
Length: 25830684 (25M) [application/java-archive]
Saving to: ‘pulsar-client-admin-3.0.0.jar’

pulsar-client-admin-3.0.0.jar                           100%[=============================================================================================================================>]  24.63M  7.83MB/s    in 3.3s

2024-04-06 12:31:15 (7.38 MB/s) - ‘pulsar-client-admin-3.0.0.jar’ saved [25830684/25830684]

Archive:  pulsar-client-admin-3.0.0.jar
    testing: META-INF/                OK
    testing: META-INF/MANIFEST.MF     OK
    testing: META-INF/DEPENDENCIES    OK
    testing: META-INF/LICENSE         OK
    testing: META-INF/NOTICE          OK
    testing: org/                     OK
    testing: org/apache/              OK
    testing: META-INF/maven/          OK
    testing: findbugsExclude.xml      OK
    testing: io/                      OK
    testing: io/airlift/              OK
    testing: META-INF/services/       OK
    testing: META-INF/versions/       OK
    testing: META-INF/io.netty.versions.properties   OK
    testing: META-INF/native/         OK
    testing: META-INF/native-image/   OK
    testing: META-INF/native/libnetty_resolver_dns_native_macos_aarch_64.jnilib   OK
    testing: META-INF/native/libnetty_resolver_dns_native_macos_x86_64.jnilib   OK
    testing: META-INF/LICENSE.md      OK
    testing: META-INF/NOTICE.md       OK
    testing: META-INF/NOTICE.markdown   OK
    testing: META-INF/LICENSE.txt     OK
    testing: META-INF/NOTICE.txt      OK
    testing: lib/                     OK
    testing: lib/libcpu-affinity.so   OK
    testing: META-INF/nar/            OK
    testing: META-INF/native/libnetty_transport_native_io_uring_x86_64.so   OK
    testing: META-INF/native/libnetty_transport_native_io_uring_aarch_64.so   OK
    testing: com/                     OK
    testing: com/google/              OK
    testing: google/                  OK
    testing: google/protobuf/         OK
    testing: google/protobuf/any.proto   OK
    testing: google/protobuf/api.proto   OK
    testing: google/protobuf/descriptor.proto   OK
    testing: google/protobuf/duration.proto   OK
    testing: google/protobuf/empty.proto   OK
    testing: google/protobuf/field_mask.proto   OK
    testing: google/protobuf/source_context.proto   OK
    testing: google/protobuf/struct.proto   OK
    testing: google/protobuf/timestamp.proto   OK
    testing: google/protobuf/type.proto   OK
    testing: google/protobuf/wrappers.proto   OK
    testing: META-INF/native/libnetty_transport_native_epoll_x86_64.so   OK
    testing: META-INF/license/        OK
    testing: META-INF/license/LICENSE.aix-netbsd.txt   OK
    testing: META-INF/license/LICENSE.boringssl.txt   OK
    testing: META-INF/license/LICENSE.mvn-wrapper.txt   OK
    testing: META-INF/license/LICENSE.tomcat-native.txt   OK
    testing: META-INF/native/libnetty_tcnative_linux_x86_64.so   OK
    testing: META-INF/native/libnetty_tcnative_linux_aarch_64.so   OK
    testing: META-INF/native/libnetty_tcnative_osx_x86_64.jnilib   OK
    testing: META-INF/native/libnetty_tcnative_osx_aarch_64.jnilib   OK
    testing: META-INF/native/netty_tcnative_windows_x86_64.dll   OK
    testing: lib/libcirce-checksum.so   OK
    testing: com/scurrilous/          OK
    testing: digesterRules.xml        OK
    testing: properties.dtd           OK
    testing: PropertyList-1.0.dtd     OK
    testing: META-INF/services/org.apache.pulsar.shade.com.fasterxml.jackson.databind.Module   OK
    testing: META-INF/services/org.apache.pulsar.shade.org.glassfish.jersey.internal.inject.InjectionManagerFactory   OK
    testing: META-INF/services/org.apache.pulsar.shade.com.fasterxml.jackson.core.ObjectCodec   OK
    testing: META-INF/services/org.apache.pulsar.shade.org.glassfish.hk2.extension.ServiceLocatorGenerator   OK
    testing: META-INF/services/org.apache.pulsar.shade.javax.ws.rs.ext.MessageBodyWriter   OK
    testing: META-INF/services/org.apache.pulsar.shade.org.glassfish.jersey.internal.spi.AutoDiscoverable   OK
    testing: META-INF/services/com.scurrilous.circe.HashProvider   OK
    testing: META-INF/services/org.apache.pulsar.shade.javax.ws.rs.ext.MessageBodyReader   OK
    testing: META-INF/services/org.apache.pulsar.shade.com.fasterxml.jackson.core.JsonFactory   OK
    testing: META-INF/services/reactor.blockhound.integration.BlockHoundIntegration   OK
No errors detected in compressed data of pulsar-client-admin-3.0.0.jar.

Shawyeok avatar Apr 06 '24 04:04 Shawyeok

@Shawyeok Because admin-api depends on client-api, some classes like Auth MessageId are defined in client-api.

dao-jun avatar Apr 06 '24 07:04 dao-jun

@Shawyeok Because admin-api depends on client-api, some classes like Auth MessageId are defined in client-api.

It looks like pulsar-client-admin just depends on it and not using it.

On the client side, the only feature that depends on protobuf at runtime is ProtobufSchema. However, pulsar-client-admin does not provide related capabilities. Therefore, I think pulsar-client-admin can change to declare dependencies on protobuf in the pom.xml, similar to pulsar-client, instead of directly copying protobuf-related classes into the shaded jar.

Shawyeok avatar Apr 06 '24 08:04 Shawyeok

@Shawyeok
you can try to remove client-api from admin-api and build it.

dao-jun avatar Apr 06 '24 10:04 dao-jun

@Shawyeok Because admin-api depends on client-api, some classes like Auth MessageId are defined in client-api.

It looks like pulsar-client-admin just depends on it and not using it.

On the client side, the only feature that depends on protobuf at runtime is ProtobufSchema. However, pulsar-client-admin does not provide related capabilities. Therefore, I think pulsar-client-admin can change to declare dependencies on protobuf in the pom.xml, similar to pulsar-client, instead of directly copying protobuf-related classes into the shaded jar.

Thank you @Shawyeok , that's a correct observation, there are mistakes in how protobuf-java is included in the shaded clients. I'll look into resolving this problem while upgrading protobuf-java used by Pulsar to address CVE-2024-7254. Mailing list message is https://lists.apache.org/thread/73jk2mx4nj82kxwvwgcqz5m63scqcy2s. It should be possible to use Pulsar client with a client application provided protobuf-java version, as long as it's compatible at a very granular level. Since now we'll need to upgrade protobuf-java in Pulsar to 3.25.5, some client applications might break since protoc generated stubs are coupled to the version that they were generated with. It should be possible to allow the client application to use an older version (or a newer version in the future).

lhotari avatar Sep 23 '24 16:09 lhotari