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

[Discovery Service] Remove module and all its references #12119

Merged

Conversation

michaeljmarshall
Copy link
Member

Motivation

As discussed on the dev and users mailing lists, the Pulsar Discovery Service no longer appears necessary.

As further justification, when inspecting maven central, the discovery service jar is only a dependency for other Pulsar modules, which only use the discovery service for tests. https://mvnrepository.com/artifact/org.apache.pulsar/pulsar-discovery-service/usages

This PR removes the Pulsar Discovery Service. It leaves old references to the discovery service in the versioned docs, as the discovery service is still available for those versions of Pulsar.

Modifications

  • Remove pulsar-discovery-service module
  • Remove discovery references from bin/pulsar
  • Remove discovery references from bin/pulsar-daemon
  • Remove conf/discovery.conf file
  • Remove all references to pulsar-discovery-service module from pom.xml files
  • Remove discovery service tests from the pulsar-broker module (only removed tests for the discovery service module)
  • Update docs

Verifying this change

I built the project and will ensure tests pass. The one concern might be that I've deleted too many tests. I only deleted tests with explicit references to the removed module. It'd be worth a second check from someone else, too.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: yes

I've described the changes above. We're removing a module, so it will break certain parts of our API. The module appears unused, and we've taken steps to discuss this on the mailing list to ensure there is proper discussion and notice.

Documentation

I've updated the docs by removing references to the discovery service. It'd be a good idea to make sure this gets mentioned in the change log when we release the major version that includes this change.

@michaeljmarshall
Copy link
Member Author

Looks like bouncy castle dependencies/licenses are making the "misc" tests fail. I think part of it is that the discovery module was the only module that required org.apache.pulsar:bouncy-castle-bc:jar:pkg:2.9.0-SNAPSHOT:complie. When running mvn dependency:tree on both master and on my branch.

The error message for the test is:

$ src/check-binary-license ./distribution/server/target/apache-pulsar-*-bin.tar.gz
org.bouncycastle-bcpkix-jdk15on-1.61.jar unaccounted for in LICENSE
org.bouncycastle-bcprov-jdk15on-1.61.jar unaccounted for in LICENSE
org.bouncycastle-bcpkix-jdk15on-1.69.jar mentioned in LICENSE, but not bundled
org.bouncycastle-bcprov-jdk15on-1.69.jar mentioned in LICENSE, but not bundled
org.bouncycastle-bcutil-jdk15on-1.69.jar mentioned in LICENSE, but not bundled

When I inspect the server distribution after building from this branch, I see the following:

$ tar -tf apache-pulsar-2.9.0-SNAPSHOT-bin.tar.gz  | grep -i bounc
apache-pulsar-2.9.0-SNAPSHOT/licenses/LICENSE-bouncycastle.txt
apache-pulsar-2.9.0-SNAPSHOT/lib/presto/plugin/pulsar-presto-connector/bouncy-castle-bc-2.9.0-SNAPSHOT-pkg.jar
apache-pulsar-2.9.0-SNAPSHOT/lib/org.apache.pulsar-bouncy-castle-bc-2.9.0-SNAPSHOT-pkg.jar
apache-pulsar-2.9.0-SNAPSHOT/lib/org.bouncycastle-bcprov-ext-jdk15on-1.69.jar
apache-pulsar-2.9.0-SNAPSHOT/lib/org.bouncycastle-bcpkix-jdk15on-1.61.jar
apache-pulsar-2.9.0-SNAPSHOT/lib/org.bouncycastle-bcprov-jdk15on-1.61.jar

It looks like the 1.61 jars are coming from a grpc dependency. After running mvn dependency:tree, I can see the following several times throughout our dependency tree:

[INFO] |  |  |  \- io.grpc:grpc-xds:jar:1.33.0:test
[INFO] |  |  |     +- org.bouncycastle:bcpkix-jdk15on:jar:1.61:test
[INFO] |  |  |     |  \- org.bouncycastle:bcprov-jdk15on:jar:1.61:test
[INFO] |  |  |     \- io.grpc:grpc-netty-shaded:jar:1.33.0:test (version selected from constraint [1.33.0,1.33.0])

I'm not familiar enough with Maven or with our build to know the right way to solve this. On one hand, grpc-xds is bringing in an older version of bouncy castle, which is known to have security issues (#10867), so I think we'll want to force the version to 1.69. Note that the latest version of grpc-xds is 1.40.1 and is only using bouncy castle jars 1.67. On the other hand, is it a good idea to be using a later version of bouncy castle than the grpc-xds jar requires?

@lhotari - can you help me figure out the right next step here?

@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/note-required labels Sep 22, 2021
@michaeljmarshall
Copy link
Member Author

@jiazhai - I notice that you wrote the documentation on Bouncy Castle dependencies. Would you be able to take a look at this PR to verify that I've followed the right paradigm for using the right version of Bouncy Castle? Because the dependency on bouncy-castle-bc was removed with the pulsar-discovery-service module, the pulsar server did not have any explicit dependency bringing in bouncy-castle-bc. I've added it explicitly in my most recent commit. Without this declared dependency, the bouncy castle deps are brought in as transitive dependencies.

@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I left one minor comment

@@ -160,6 +154,16 @@
<artifactId>simpleclient_log4j2</artifactId>
</dependency>

<!-- This dependency was previously brought in transitively through the pulsar-discovery-service module. When the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not need this comment in the source code

@eolivelli
Copy link
Contributor

if this is useless it is better to drop it in a major release like 2.9.
We can always revert the deletion if someone cares about this module (it looks like no one responded to the ML yet)

@eolivelli eolivelli added this to the 2.9.0 milestone Sep 23, 2021
@michaeljmarshall
Copy link
Member Author

@eolivelli - makes sense to me. I am fine merging this before we cut branch-2.9.

Regarding the comment in the pom.xml, which I removed in my most recent commit, I want to make sure that adding the bouncy castle dependency to the server pom.xml is the correct solution. The discovery service was the dependency forcing our version of bouncy castle to 1.69. Without the discovery service module as a dependency, the grpc-xds dependency determined out bouncy castle version, which would have been 1.61. Since we need the grpc-xds dependency, and we want version 1.69 for security reasons, I added the dependency to the server's pom.xml.

@merlimat
Copy link
Contributor

@eolivelli - makes sense to me. I am fine merging this before we cut branch-2.9.

Regarding the comment in the pom.xml, which I removed in my most recent commit, I want to make sure that adding the bouncy castle dependency to the server pom.xml is the correct solution. The discovery service was the dependency forcing our version of bouncy castle to 1.69. Without the discovery service module as a dependency, the grpc-xds dependency determined out bouncy castle version, which would have been 1.61. Since we need the grpc-xds dependency, and we want version 1.69 for security reasons, I added the dependency to the server's pom.xml.

Yes, I think that was the correct fix. Merging this.

@merlimat merlimat merged commit 8c4c630 into apache:master Sep 23, 2021
@michaeljmarshall michaeljmarshall deleted the remove-pulsar-discovery-service branch September 24, 2021 00:05
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
* [Discovery Service] Remove module and all its references

* Explicitly declare dependency on bouncy-castle-bc in server pom.xml

* Remove unnecessary comment
@thinker0
Copy link
Member

Please #15225 rollback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/2.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants