[TEST] Fix elasticsearch-security-cli JUnit runner

[justincr-elastic]

Using the JUnit test runner in this project failed when launched in
an IDE. A "Jar Hell" error message was logged to console.

Root cause was BC non-FIPS and BC FIPS on the test classpath at the
same time. BC non-FIPS jars are imported by the project as api
dependencies, and BC-FIPS jars were inherited as testImplementation
dependencies from x-pack core module.

JUnit test runner failures were triggered from ESTestCase. That super
class loads BootstrapForTesting, which detects and rejects BC non-FIPS
vs BC-FIPS on the classpath. Those two sets of jars conflict because
they contain duplicate class names with different implementations.

To fix the JUnit test runner error, build.gradle has been updated to
exclude the BC-FIPS testImplementation dependencies inherited
from x-pack core module. After the fix, the JUnit test runner works
in an IDE for the elasticsearch-security-cli project.

Reason for the fix is the JUnit test runner may be more convenient,
or faster, to use in an IDE compared to the Gradle test runner.

See also: #69403

[elasticmachine]

Pinging @elastic/es-security (Team:Security)

[justincr-elastic]

@elasticmachine run elasticsearch-ci/bwc

[justincr-elastic]

@elasticmachine update branch

[jkakavas]

LGTM pending the one comment below. I was wondering if we should declare the dependency differently in

elasticsearch/x-pack/plugin/core/build.gradle

Line 53 in 35ec6f3

testCompileOnly('org.bouncycastle:bc-fips:1.0.2')

so that it doesn't leak into testArtifact. Out of curiosity, I will defer to @elastic/es-delivery for an opinion, but I'm not sure it matters that much. The suggested solutions here works fine!

[mark-vieira]

My other question is how do those tests not explode via JarHell when we run via -Dtests.fips.enabled=true since that would similarly put both jars on the classpath. I think the root issue is that when we run with FIPS we hackily smash those jars onto the classpath rather than modeling them properly. That's the whole reason we need this compileOnly dependency in the first case.

Also another great reason why we should be using test fixtures, and not these "test artifact" dependencies because we end up pulling in way more than we need to 😦

I'll defer to @breskeby on what the best coarse of action is here. Removing the compileOnly dependencies from the exported test artifact won't always work. In this case, we don't expose any of the classes via the API but we might in other cases, in which case compiling the consuming project would fail. Perhaps that's ok? Rene, what's the default behavior for compileOnly dependencies when consuming from another project? They come through, right, but are simply not also included on the runtime classpath?

[tvernum]

My other question is how do those tests not explode via JarHell

The answer will not delight you...

https://github.com/elastic/elasticsearch/blob/v7.14.1/x-pack/plugin/security/cli/build.gradle#L38-L53

[tvernum]

I'm going to drop from the review list here. If @jkakavas and Delivery are happy, you don't need my opinions as well.

[jkakavas]

My other question is how do those tests not explode via JarHell

The answer will not delight you...

https://github.com/elastic/elasticsearch/blob/v7.14.1/x-pack/plugin/security/cli/build.gradle#L38-L53

Not delighted but not surprised either :D #51733

[ywangd]

LGTM

Thanks for getting to the bottom of this! I am OK with this approach since it fixes the problem at hand. Alternatively, I wonder whether we could eliminate the bc-fips jars from core's testArtifact when the test is not inFipsJvm (something like conditional dependency and compliation in core module). It has the advantage that individual consumer does not need to worry about excluding the jars on its own. So we don't get into this problem in another place or future.

[breskeby]

My other question is how do those tests not explode via JarHell when we run via -Dtests.fips.enabled=true since that would similarly put both jars on the classpath. I think the root issue is that when we run with FIPS we hackily smash those jars onto the classpath rather than modeling them properly. That's the whole reason we need this compileOnly dependency in the first case.

Also another great reason why we should be using test fixtures, and not these "test artifact" dependencies because we end up pulling in way more than we need to 😦

I'll defer to @breskeby on what the best coarse of action is here. Removing the compileOnly dependencies from the exported test artifact won't always work. In this case, we don't expose any of the classes via the API but we might in other cases, in which case compiling the consuming project would fail. Perhaps that's ok? Rene, what's the default behavior for compileOnly dependencies when consuming from another project? They come through, right, but are simply not also included on the runtime classpath?

the gradle default is that only api dependencies are exposed to the compile classpath of a consuming project. The runtime classpath contains api and implementation but not compileOnly

[breskeby]

I think the way to go forward to a proper fix is to only expose the api dependencies of the testartifact. That allows proper encapsulation of compile and runtime dependencies and is actually what api dependencies are about. You should not depend on a compileOnly or implementation dependency at compile time.
I'll look into a proper solution next week after the long weekend as the week is ending here already.
If this PR solves a big pain point for you NOW I don't want to block you on not merging here. I'll create a follow up PR with a tweaked testArtifact handling in the upcoming days

[justincr-elastic]

@elasticmachine update branch

[mark-vieira]

The answer will not delight you...

https://github.com/elastic/elasticsearch/blob/v7.14.1/x-pack/plugin/security/cli/build.gradle#L38-L53

At least I'm not imagining things 😉

[mark-vieira]

I think the way to go forward to a proper fix is to only expose the api dependencies of the testartifact. That allows proper encapsulation of compile and runtime dependencies and is actually what api dependencies are about. You should not depend on a compileOnly or implementation dependency at compile time.

Yep, this makes sense to me. Essentially, compileOnly is an implementation dependency that's also not inherited by runtimeClasspath. 👍

[breskeby]

@justincr-elastic @jkakavas I think this PR is outdated as the original problem should have been fixed by now via #78238 can you confirm this is no longer an issue for you? I just tested it manually in my idea setup with master

[jkakavas]

@justincr-elastic can we close this ?

[justincr-elastic]

Closing because it is addressed by the other PR.