FIPS tests failing on v5.x

[rvagg]

See https://ci.nodejs.org/job/node-test-commit-linux-fips/1064/nodes=ubuntu1404-64/console & https://ci.nodejs.org/job/node-test-commit-linux-fips/1063/nodes=ubuntu1404-64/console

A bunch of tests failing with:

# out/Release/node: bad option: --enable-fips

I don't see anything obvious in v5.x since v5.7.0 that would explain this (see CHANGELOG.md in #5464 for full list of commits so far).

/cc @nodejs/crypto

[rvagg]

I think #5376 is to blame + TEST_CI_ARGS="--node-args --enable-fips" $MAKE test-ci -j $(getconf _NPROCESSORS_ONLN) in node-test-commit-linux-fips on Jenkins. @nodejs/build can someone explain the sequence of events that have got us to this point so we can back it out? We have to get all our branches working and it seems that this Jenkins config is probably going to cause breakage across release lines.

[Trott]

/cc @stefanmb @mhdawson

[stefanmb]

If we're seeing "bad option" it means we're passing "--enable-fips" to a non-FIPS build.

[stefanmb]

@rvagg @Trott @mhdawson @ofrobots The problem here is that this build failed testing the PR for Ali's change here: #5462

His branch "https://github.com/ofrobots/node/tree/newexternal" is behind 180 commits and is missing support for "--enable-fips", so it won't work with the newly added FIPS tests. The solution is to update the branch in the PR to sync up with master and the errors will go away.

[stefanmb]

@rvagg I tried checking your branch but it seems you deleted it ("osx-installer-new-logo at rvagg/io.js"). My best guess is that it was also behind the master.

@mhdawson added the test configuration for --enable-fips, but that option requires my commits in order to work, therefore older forks won't pass the new test job because they'll be missing the support for parsing the new option and they'll fail with the error you saw ("bad option").

[rvagg]

it was against the latest v5.x

[rvagg]

Can you please check v5.x and see what is missing that should be there to support this, also are we expecting v4.x to support this too because I don't believe there's any differentiation in Jenkins.

[stefanmb]

@rvagg This is the current situation:

Master (https://github.com/nodejs/node/tree/master)

All needed commits are present:
c98d159
23a584d
7c48cb5

v4.x (https://github.com/nodejs/node/tree/v4.x)

None of the above commits, the FIPS jobs with "--enable-fips" should not run on this branch.

v5.x (https://github.com/nodejs/node/tree/v5.x)
22bb7c9
479a43c
The test runner commits are there, but it's missing the actual support for controlling fips at runtime (7c48cb5), therefore the FIPS jobs with "--enable-fips" should not run on this branch.

Note that 7c48cb5 is a semver-major change so I don't think it can be backported.

The Jenkins jobs will have to be updated to take these differences into account, unfortunately I do not have permissions to do so in Jenkins. :(

[stefanmb]

Come to think of it, I think I may have misunderstood how the commit test jobs work, if they are merging the commits under test into master then they should pass. For reasons explained above the --enable-fips jobs won't work against v5.x and v4.x and should not be run. Note that for those versions the regular FIPS tests should be run (i.e. just running the test suite against the FIPS build, without passing any arguments).

[rvagg]

ok, so I'm going to comment out that line in Jenkins, whoever put it in there can go back and revisit it to make sure it's not run for anything other than master

[stefanmb]

@rvagg Sounds good, thanks, I'll speak with @mhdawson about running those jobs only against master. Thanks for reporting this. Sorry, for the trouble. :(

[mhdawson]

I was just thinking of master when we made the update to the job, sorry about that.

[mhdawson]

Testing fix under this job: https://ci.nodejs.org/job/node-test-commit-linux-fips-mdawson/.

I'm using a runtime check on the node version to decide whether to run it or not. @rvagg is this a reasonable way to do it or do we have other existing jobs that do it in a better way ?

[mhdawson]

Ok, validated what I have did the right thing on 4x, 5x and master. Will push to the real job and can change later if there is a better way to do it.

[mhdawson]

Build with master to validate updated job: https://ci.nodejs.org/job/node-test-commit-linux-fips/1086/
Build with 5.X to validate updated job: https://ci.nodejs.org/job/node-test-commit-linux-fips/1087/
Build with 4.X to validate updated job: https://ci.nodejs.org/job/node-test-commit-linux-fips/1088/

[mhdawson]

Ok all 3 ran ok. Will wait for feedback from @rvagg on approach before closing.

[rvagg]

Actually I think @joaocgreis has had the strongest jenkins-fu of late, would you mind reviewing and approving the change?

[joaocgreis]

@mhdawson for master (v6) won't this run the tests twice, with the first exactly equal to node-test-commit-linux (because FIPS defaults to false)?

[mhdawson]

@joaocgreis, the difference is that the code is compiled differently, so although the tests will be the same, the binary will be different. In this one the binary was compiled with FIPs capable support enabled and FIPs can be enabled even if the default is off, but in the regular run the binary is compiled so that FIPs cannot be enabled at all. So it is worth testing both cases.

[joaocgreis]

@mhdawson I see. Jenkins changes LGTM then.

[mhdawson]

ok closing this issue based on last comment.