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] JarHell enforcement during integration tests is blocking test development for the security plugin #3905

Closed
peternied opened this issue Jul 14, 2022 · 5 comments
Labels
Build Libraries & Interfaces enhancement Enhancement or improvement to existing feature or request

Comments

@peternied
Copy link
Member

peternied commented Jul 14, 2022

Describe the bug
Kafka's clients production org.apache.kafka:kafka-clients:3.0.1 and test org.apache.kafka:kafka-clients:3.0.1:test jars contain overlapping classes in org.apache.kafka.common.message. When attempting to build or run tests JarHell is detecting this an erroring out. This is preventing the Security team from authoring test cases.

To Reproduce
Steps to reproduce the behavior:

  1. git clone git@github.com:peternied/security.git
  2. git checkout force-jar-hell-issue
  3. ./gradlew jarHell
...
| class: org.apache.kafka.common.message.TxnOffsetCommitRequestDataJsonConverter$TxnOffsetCommitRequestTopicJsonConverter
| jar1: /local/home/petern/.gradle/caches/modules-2/files-2.1/org.apache.kafka/kafka-clients/3.0.1/8f931e45e96e952728d540829e5bde9d79fab172/kafka-clients-3.0.1.jar
| jar2: /local/home/petern/.gradle/caches/modules-2/files-2.1/org.apache.kafka/kafka-clients/3.0.1/da500d6dfd3447f56df88dad8ab7ba801150fa4c/kafka-clients-3.0.1-test.jar
|       at org.opensearch.bootstrap.JarHell.checkClass(JarHell.java:314)
|       at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:213)
|       at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:100)
|       at org.opensearch.bootstrap.JarHell.main(JarHell.java:84)

Alternative repro available in opensearch-project/security#1938 that exercises the test runtime

Expected behavior
There should be a way to disable this error from blocking build or test runtime.

Additional context
Note; test are failing due to this check being run during bootstrap

// check for jar hell
try {
final Logger logger = LogManager.getLogger(JarHell.class);
JarHell.checkJarHell(logger::debug);
} catch (Exception e) {
throw new RuntimeException("found jar hell in test classpath", e);
}

@peternied peternied added bug Something isn't working untriaged v2.2.0 labels Jul 14, 2022
@peternied peternied changed the title [BUG] Cannot use Kafka [BUG] JarHell enforcement during integration tests is blocking test development for the security plugin Jul 14, 2022
@peternied
Copy link
Member Author

I have created a pull request, apache/kafka#12407 with the source of the issue, but I am uncertain of the release timeline or how quickly we can use the updated in the middle of the security dependency tree.

This leaves 3 potential options to work around in order of effort:

  1. Security plugin overrides the JarHell.java functionality and rolls this back this patch when the dependency has been updated. This is the most encapsulated change, and thus the easiest for OpenSearch to support (aka no-effort)
  2. There is a way to disable running jarhell during integration tests that can be configured on and off. This setting can be used to disable the tests inline with the existing build.gradle functionality that allows for jarhell.enabled = false to disabling this check.
  3. There is a way to bypass certain files/classes during inspection. This was already done with system resources, ref.

Let me know if there are other options to help keep the team unblocked. Without any other input I think we will proceed with #1

@dblock
Copy link
Member

dblock commented Jul 14, 2022

I think #1 or #3 are fine since they are temporary anyway.

@minalsha
Copy link
Contributor

Extensions team vote for #3 which is the easiest path forward. Let us know if any help is needed from core team. Thanks

@kartg
Copy link
Member

kartg commented Aug 3, 2022

Currently, the security plugin is following option #1 above. I'll drop the 2.2 label from this and keep it in our backlog so we can look at #2 and/or #3

@kartg kartg removed the v2.2.0 label Aug 3, 2022
@minalsha minalsha added enhancement Enhancement or improvement to existing feature or request and removed bug Something isn't working labels Aug 30, 2022
@peternied
Copy link
Member Author

We've been using option #1 to locally disable jarhell in the security project. As that is the case no change is needed from the OpenSearch codebase - closing out this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Libraries & Interfaces enhancement Enhancement or improvement to existing feature or request
Projects
None yet
Development

No branches or pull requests

5 participants