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

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

Closed
1 of 2 tasks
pqab opened this issue Mar 14, 2024 · 19 comments · Fixed by #23468
Closed
1 of 2 tasks

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

pqab opened this issue Mar 14, 2024 · 19 comments · Fixed by #23468
Assignees

Comments

@pqab
Copy link

pqab commented Mar 14, 2024

Search before asking

  • 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!
@dao-jun
Copy link
Member

dao-jun commented Mar 14, 2024

Try to use pulsar-client-all

@dao-jun
Copy link
Member

dao-jun commented Mar 14, 2024

<!-- 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.

@pqab
Copy link
Author

pqab commented Mar 14, 2024

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?

@lhotari
Copy link
Member

lhotari commented Mar 14, 2024

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.

@dao-jun
Copy link
Member

dao-jun commented Mar 14, 2024

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

@pqab
Copy link
Author

pqab commented Mar 14, 2024

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

@lhotari
Copy link
Member

lhotari commented Mar 14, 2024

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
Copy link
Member

lhotari commented Mar 14, 2024

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
Copy link
Member

lhotari commented Mar 14, 2024

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.

@dao-jun
Copy link
Member

dao-jun commented Mar 14, 2024

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

@pqab
Copy link
Author

pqab commented Mar 14, 2024

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

@lhotari
Copy link
Member

lhotari commented Mar 14, 2024

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
Copy link
Member

lhotari commented Mar 14, 2024

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

@Shawyeok
Copy link
Contributor

Shawyeok commented Apr 6, 2024

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.

@dao-jun
Copy link
Member

dao-jun commented Apr 6, 2024

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

@Shawyeok
Copy link
Contributor

Shawyeok commented Apr 6, 2024

@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.

@dao-jun
Copy link
Member

dao-jun commented Apr 6, 2024

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

@lhotari lhotari self-assigned this Sep 23, 2024
@lhotari
Copy link
Member

lhotari commented Sep 23, 2024

@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).

@Shawyeok
Copy link
Contributor

@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).

Thanks @lhotari , I noticed this issue remain unresolved in recent LTS 4.0.0 release (candidate-1), so I opened a PR #23468 to address this.

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