[fix][build] Remove commons-collections:commons-collections references

[ZachChuba]

Replacing with org.apache.commons:commons-collections4. This ensures the outdated dependency is not used by pulsar code, but does not strip commons-collections from transitive dependencies. Initially aimed at addressing a CVE bundled with commons-collections, but does not clear commons-collections from the classpath.

Main Issue: #24817

Motivation

Removing references of commons-collections:commons-collections from all code. All newer versions of this dependency have moved to another group id. This will make it seemless to eventually remove the transitive dependency reference to commons-collections:3.2.2. Root motivation is a security issue flagged by jar scanning software, but this PR does not remediate it. Instead, it makes it seemless to upgrade when the upgrade becomes available.

Modifications

Build files and test imports

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

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

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: My Fork

[lhotari]

Thanks for the PR.

It seems that the correct solution would be to simply remove the inclusion of commons-collections since it's not required by any of the transitive dependencies of pulsar-client-admin-shaded, pulsar-client-all or pulsar-client-shaded.

org.apache.bookkeeper:bookkeeper-server:jar:tests:4.17.2:test pull it in, but only for test dependencies.

You can check this with dependency:tree

mvn -pl pulsar-client-all dependency:tree |less
mvn -pl pulsar-client-admin-shaded dependency:tree |less
mvn -pl pulsar-client-shaded dependency:tree |less

[ZachChuba]

Note, on my workstation I cannot locally build pulsar. So I'm using CI to guide my build results. Don't review until I request re-review. Primarily using CI on my fork to see build results and run some dependency graphs

[lhotari]

Note, on my workstation I cannot locally build pulsar. So I'm using CI to guide my build results. Don't review until I request re-review. Primarily using CI on my fork to see build results and run some dependency graphs

It doesn't sound like a great way to develop Pulsar using Pulsar CI. If you are experimenting, I'd recommend pushing to experimental branches and keeping them in your own repository. In your own repository you can open PRs with these experimental branches. There are details about "Personal CI" in the contribution guide.

Just curious to know what's the reason that you cannot build locally. If you have Windows, you should be able to build in WSL2 Ubuntu. In Mac and Linux, there shouldn't be issues in building. With VirtualBox and Vagrant, you can easily create VMs on Windows, Mac and Linux (there are also other alternatives).

[lhotari]

LGTM, thanks for the contribution!

[ZachChuba]

It appears that the test cases pull in some utils from common, which are expecting bookkeeper to be shaded in. Ran into same issue even when explicitly including the dependency with test scope. Unsure if we can actually remove bookkeeper from the client shading. Will experiment.

As for why I can't test locally - machine restrictions. I must pull dependencies via my company's proxy and some dependencies are not available on that proxy (funnily enough, partially due to the very vulnerabilities im trying to remove); even tried manually populating my cache and it still failed. My personal machine is a beater. I have been testing through CI on the PR I made in my fork

[lhotari]

It appears that the test cases pull in some utils from common, which are expecting bookkeeper to be shaded in. Ran into same issue even when explicitly including the dependency with test scope. Unsure if we can actually remove bookkeeper from the client shading. Will experiment.

Oh yes, there are a few dependencies that need to be shaded. The dependency graph shows them. Without having CLI access, your feedback loop is just going to be very long.

As for why I can't test locally - machine restrictions. I must pull dependencies via my company's proxy and some dependencies are not available on that proxy (funnily enough, partially due to the very vulnerabilities im trying to remove); even tried manually populating my cache and it still failed. My personal machine is a beater. I have been testing through CI on the PR I made in my fork

There might be free VMs available. For example Google Cloud has free credits which will last long if you stop and start a VM when needed. There's also GitHub Codespaces which is fully automated pay-per-use with 60 hours free per month, that has terminal access too. Are you already using it besides GitHub Actions?

[lhotari]

should be replaced with

<include>org.apache.bookkeeper:bookkeeper-common-allocator</include>
<include>org.apache.bookkeeper:cpu-affinity</include>
<include>org.apache.bookkeeper:circe-checksum</include>

[lhotari]

Changing this line might not even be needed since I'd assume that it would work correctly since test dependencies won't get included in the shaded jar.