Repository navigation
FIPS tests failing on v5.x #5465
Description
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
Activity
rvagg commentedon Feb 27, 2016
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 commentedon Feb 27, 2016
/cc @stefanmb @mhdawson
stefanmb commentedon Feb 27, 2016
If we're seeing "bad option" it means we're passing "--enable-fips" to a non-FIPS build.
stefanmb commentedon Feb 27, 2016
@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 commentedon Feb 27, 2016
@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 commentedon Feb 28, 2016
it was against the latest v5.x
rvagg commentedon Feb 28, 2016
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 commentedon Feb 28, 2016
@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 commentedon Feb 28, 2016
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 commentedon Feb 28, 2016
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 commentedon Feb 28, 2016
@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 commentedon Feb 29, 2016
I was just thinking of master when we made the update to the job, sorry about that.
mhdawson commentedon Feb 29, 2016
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 commentedon Feb 29, 2016
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 commentedon Feb 29, 2016
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 commentedon Feb 29, 2016
Ok all 3 ran ok. Will wait for feedback from @rvagg on approach before closing.
rvagg commentedon Mar 1, 2016
Actually I think @joaocgreis has had the strongest jenkins-fu of late, would you mind reviewing and approving the change?
joaocgreis commentedon Mar 1, 2016
@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 commentedon Mar 1, 2016
@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 commentedon Mar 2, 2016
@mhdawson I see. Jenkins changes LGTM then.
mhdawson commentedon Mar 7, 2016
ok closing this issue based on last comment.