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

Mute bwc tests for old versions in FIPS JVMs #32839

Closed
wants to merge 2 commits into from

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Aug 14, 2018

Elasticsearch versions earlier than 6.4.0 cannot properly run in a
FIPS 140 JVM. This commit ensures that we do not try to run bwc
tests that entail spinning up < 6.4.0 nodes when CI is run in a FIPS
140 JVM.
It also reverts e497173 and e64bb48 as the workarounds inserted there
are no longer required.

Resolves #32737

Elasticsearch versions earlier than 6.4.0 cannot properly run in a
FIPS 140 JVM. This commit ensures that we do not try to run bwc
tests that entail spinning up < 6.4.0 nodes when CI is run in a FIPS
140 JVM.
It also reverts e497173 and e64bb48 as the workarounds inserted there
are no longer required.

Resolves elastic#32727
@jkakavas jkakavas added >test Issues or PRs that are addressing/adding tests v7.0.0 :Security/Security Security issues without another label v6.4.0 v6.5.0 labels Aug 14, 2018
@jkakavas jkakavas requested a review from alpar-t August 14, 2018 10:29
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

List<Version> indexCompatibleVersions = bwcVersions.indexCompatible
// Versions before 6.4.0 cannot be run in a FIPS 140 JVM, so exclude them
if (inFipsJvm){
indexCompatibleVersions.removeAll { it.before("6.4.0")}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this not to be repeated in the build logic and encapsulated in VersionCollection, perhaps by passing inFipsJvm to the constructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

When calling the constructor in build.gradle, the inFipsJvm property hasn't been evaluated, as this happens in BuildPlugin#globalBuildInfo().

Another approach would be to add a inFipsJvm optional param to getSnapshotsIndexCompatible() , getIndexCompatible() etc, defaulting to false. WDYT?

@colings86
Copy link
Contributor

Just a quick note: I think the "Resolves" reference int eh description might be wrong as it links to an issue that seems unrelated?

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I don't think this makes sense as something VersionCollection figures out. Whether a particular version will run on a fips JVM has to do with that version, not version compatibility, which is what VersionCollection is all about. Instead of doing this on each compatible list method, I think you can do it as an attribute on the Version object itself? Then the code in eg full cluster restart tests can be something like:

for (Version version : bwcVersions.wireCompatible) {
    if (inFipsJvm && version.isFipsCompatible() == false) {
        continue
    }
    ....
}

@jkakavas
Copy link
Member Author

Thanks for the feedback @rjernst. I'm closing this PR in favor of #32901 ,that re-enables the tests and addresses the root cause instead.

@jkakavas jkakavas closed this Aug 16, 2018
@jkakavas jkakavas deleted the mute-bwc-qa-fips branch September 14, 2018 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/Security Security issues without another label >test Issues or PRs that are addressing/adding tests v6.4.0 v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants