Remove FIPS check shell code, already done in the MCO #10488
Conversation
|
cc @sallyom |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cgwalters The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This effectively reverts fb1c4b4 e2e-fips is currently failing with `/bin/bash: line 15: nodes[i]: unbound variable` Looking at this...we already have code to validate the state of FIPS in the MCO, see: https://github.com/openshift/machine-config-operator/blob/091afde36ac117ef8b782a85b38ae8783ddf4b70/pkg/daemon/update.go#L571 openshift/machine-config-operator#1252 openshift/machine-config-operator#1233 I think these types of checks should be the MCO's role, or if we choose not to do that, let's at least implement them in Go in the existing e2e suite and avoid nontrivial shell-in-YAML.
cgwalters
force-pushed
the
remove-fips-check
branch
from
786b289 to
e2e01f9
Compare
| value: e2e-aws-fips | ||
| - name: TEST_COMMAND | ||
| value: | | ||
| fips_check |
There was a problem hiding this comment.
I see no need for pull-ci-openshift-installer-master-e2e-aws-fips and similar jobs if we feel that the MCO test already covers the FIPS angle sufficiently. Can we just drop the jobs, instead of editing them to be identical to vanilla e2e? Or do we feel like we still need coverage on installer PRs of installs with FIPS enabled?
There was a problem hiding this comment.
The job injects the fips: true install config still and we're still testing that.
|
/test ci/prow/pj-rehearse |
|
@cgwalters: The specified target(s) for
Use
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/hold The point of the fips check is to verify FIPS is actually on. We need at least one check in test suite or infra that verifies that FIPS turns on. Where is that test? We can't remove this until that exists (MCO checking is not sufficient unless this code changes to check that MCO actually has fips on) |
openshift-ci-robot
added
the
do-not-merge/hold
|
It would be totally possible for the MCO fips check to break and we wouldn't notice (MCO e2es don't currently test fips). But if fips somehow failed to be enabled, the MCO would notice. I'd agree this is probably worth a "double check". How about a simple e2e test that's the equivalent of this code which checks that if |
See discussion in openshift/release#10488 and https://bugzilla.redhat.com/show_bug.cgi?id=1861095 This test replaces the bash code in the release repo with a more proper test here. While here I noticed that the topology tests had some code that reused the MCD as a handy privileged pod; extract that to the toplevel utils and use both here and there.
|
Filed |
|
See my comments in the PR - we have to do a couple of things:
|
|
Since we have to do 1, I'd like to see a version specific or double check that either the old path or some new path is checked in bash and then we can improve this with other code. |
openshift-ci-robot
added
the
needs-rebase
|
@cgwalters: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
See discussion in openshift/release#10488 and https://bugzilla.redhat.com/show_bug.cgi?id=1861095 This test replaces the bash code in the release repo with a more proper test here. While here I noticed that the topology tests had some code that reused the MCD as a handy privileged pod; extract that to the toplevel utils and use both here and there.
See discussion in openshift/release#10488 and https://bugzilla.redhat.com/show_bug.cgi?id=1861095 This test replaces the bash code in the release repo with a more proper test here. While here I noticed that the topology tests had some code that reused the MCD as a handy privileged pod; extract that to the toplevel utils and use both here and there.
See discussion in openshift/release#10488 and https://bugzilla.redhat.com/show_bug.cgi?id=1861095 This test replaces the bash code in the release repo with a more proper test here. While here I noticed that the topology tests had some code that reused the MCD as a handy privileged pod; extract that to the toplevel utils and use both here and there.
See discussion in openshift/release#10488 and https://bugzilla.redhat.com/show_bug.cgi?id=1861095 This test replaces the bash code in the release repo with a more proper test here. While here I noticed that the topology tests had some code that reused the MCD as a handy privileged pod; extract that to the toplevel utils and use both here and there.
See discussion in openshift/release#10488 and https://bugzilla.redhat.com/show_bug.cgi?id=1861095 This test replaces the bash code in the release repo with a more proper test here. While here I noticed that the topology tests had some code that reused the MCD as a handy privileged pod; extract that to the toplevel utils and use both here and there.
|
@cgwalters: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@cgwalters: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@cgwalters: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
6 participants
This effectively reverts
fb1c4b4
e2e-fips is currently failing with
/bin/bash: line 15: nodes[i]: unbound variableSee example: https://prow.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_installer/3918/pull-ci-openshift-installer-master-e2e-aws-fips/1287800541086224384
Looking at this...we already have code to validate
the state of FIPS in the MCO, see:
https://github.com/openshift/machine-config-operator/blob/091afde36ac117ef8b782a85b38ae8783ddf4b70/pkg/daemon/update.go#L571
openshift/machine-config-operator#1252
openshift/machine-config-operator#1233
I think these types of checks should be the MCO's role,
or if we choose not to do that, let's at least implement
them in Go in the existing e2e suite and avoid
nontrivial shell-in-YAML.