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

Add support for concatenated plugin caches #2887

Closed
wants to merge 2 commits into from

Conversation

ppkarwasz
Copy link
Contributor

Log4j2Plugins.dat descriptors cause problems while shading an application that uses Log4j Core, since they need a specialized resource transformer to be merged.

We maintain such a resource transformer for the Maven Shade Plugin and Gradle Up Shadow also contains a similar tool. Both of them have open issues.
The remaining shading build tools, such as SBT Assembly, however, don't have any resource transformer capable of dealing with the Log4j2Plugins.dat format.

This PR extends the Log4j2Plugins.dat format to allow it to contain \n-separated lists of serialized PluginCache objects.
This will allow users to merge plugin caches using more standard tools.

`Log4j2Plugins.dat` descriptors cause problems while shading
an application that uses Log4j Core, since they need a
specialized resource transformer to be merged.

We maintain such a resource transformer for the Maven Shade Plugin
and Gradle Up Shadow also contains a similar tool.
Both of them have open issues.
The remaining shading build tools, such as SBT Assembly,
however, don't have any resource transformer capable of dealing
with the `Log4j2Plugins.dat` format.

This PR extends the `Log4j2Plugins.dat` format to allow
it to contain `\n`-separated lists of serialized `PluginCache`
objects.
This will allow users to merge plugin caches using more standard
tools.
@ppkarwasz ppkarwasz requested a review from jvz August 23, 2024 13:03
@ppkarwasz ppkarwasz self-assigned this Aug 23, 2024
@vy
Copy link
Member

vy commented Aug 26, 2024

Together with this PR, we will have 3 plugin descriptor formats:

  1. The Log4j 2 Log4j2Plugins.dat format
  2. The newline-separated Log4j 2 Log4j2Plugins.dat format (introduced by this PR)
  3. Log4j 3 ServiceLoaders

Can we instead backport ServiceLoaders to 2.x and be done with just two?

I understand the temptation for this easy fix and I am aware that backporting ServiceLoaders from 3.x to 2.x is non-trivial. That said, when it comes to the motivation for this new plugin descriptor file format:

  • I don't see a pressing need (users are doing okay with the present tooling)
  • Shading is a bad practice anyway

Last minute edit: Though backporting ServiceLoaders to 2.x will not help with already compiled JARs containing Log4j 2 Log4j2Plugins.dat files, right?

@vy vy added enhancement Additions or updates to features plugins Affects the plugin system labels Aug 26, 2024
@ppkarwasz
Copy link
Contributor Author

That said, when it comes to the motivation for this new plugin descriptor file format:

  • I don't see a pressing need (users are doing okay with the present tooling)
  • Shading is a bad practice anyway

There is no pressing need for this PR, I only created it as a response to this log4j-user@logging thread, when I realized that not all build tools have a resource handler for Log4j2Plugins.dat.

I find shading a bad practice too:

  • libraries should not be shaded, since this provides a software supply chain risk. The current SBOM tools do not support shading (see support for maven-shade-plugin CycloneDX/cyclonedx-maven-plugin#472), so user might be unaware that they have a vulnerable copy of a library on the classpath. MvnRepository doesn't recognize shaded libraries either (see pax-logging-log4j2, which contains a copy of log4j-core) and does not show CVE warnings.
  • applications should not be shaded, since there are better alternatives to create executable JARs, like the Spring Boot plugins.

@jvz
Copy link
Member

jvz commented Aug 26, 2024

While I like the ServiceLoader approach best (and don't think it's that hard to back port; the main challenges there revolve around updating the annotation processor to generate the service classes as the main version has diverged considerably from 2.x. However, that only solves the problem for plugins compiled against the version of Log4j that introduces support for it, and 3.0 seemed like a solid point for that.

I agree with Volkan here that having three formats is not ideal, and even two formats is annoying enough (I had hoped to eliminate support for the .dat format in 3.0, but that's the only thing that allows for 2.x plugins to still be loadable due to annotation changes).

Could we have some sort of compromise where the .dat parser can work in a lax mode or something where it tries to continue parsing a concatenated .dat file?

@ppkarwasz
Copy link
Contributor Author

Could we have some sort of compromise where the .dat parser can work in a lax mode or something where it tries to continue parsing a concatenated .dat file?

That is exactly what this PR does: the .dat parser continues reading the file until there are no bytes left.

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

I think this is a reasonable approach even though it reminds me of how we bundled everything in Core due to developers' ignorance of dependency management.

Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

Even though I find it helpful, I am not in favor of this hack relaxed-mode for following reasons:

  1. PR is developed in response to a user trying to implement a JTL pretty-printer in a shaded application built using SBT. This is wrong in two major ways:
    1. She is doing wrong by trying to pretty-print JTL (already explained this to her)
    2. She is doing wrong by using shading
  2. PR further complicates the officially accepted Log4j plugin descriptor file format
  3. We haven't received a single legitimate complaint from SBT users. I don't see a pressing need. This is important and a sensitive point, in particular, given Log4j is known for its bloat. We already massively revamped the docs. Let's first collect some user feedback before settling down on a solution.

These said, PR is technically sound. If there is a consensus from other maintainers to get this in, I won't stop in the way.

== Merging plugin descriptors

The plugin descriptor is located in a JAR file at the following fixed location:
----
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
----
[source]
----

You can use the
https://github.com/GradleUp/shadow/blob/main/src/main/groovy/com/github/jengelman/gradle/plugins/shadow/transformers/Log4j2PluginsCacheFileTransformer.groovy[`Log4j2PluginsCacheFileTransformer`].

* Since version `2.24.0` you can any resource transformer capable of concatenating files using `\n` as separator.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Since version `2.24.0` you can any resource transformer capable of concatenating files using `\n` as separator.
* Since version `2.24.0`, you can employ any resource transformer capable of concatenating files using `\n` as separator.


assertThat(actual)
.as("Deserialized plugin cache")
.isEqualTo(createSampleCache().getAllCategories());
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd prefer createSampleCache() accept categories and entries, instead of assuming hardcoded values will be identical in multiple places.

@jvz
Copy link
Member

jvz commented Aug 26, 2024

I agree with Volkan still. No strong feelings from me on whether we should support this.

@ppkarwasz
Copy link
Contributor Author

I agree with Volkan still. No strong feelings from me on whether we should support this.

Although I am the author of the PR, I also agree with Volkan. So we have consensus!

@ppkarwasz ppkarwasz closed this Aug 26, 2024
@ppkarwasz ppkarwasz deleted the feature/2.x/plugin-cache-additivity branch August 26, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions or updates to features plugins Affects the plugin system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants