[rhcos-4.17] tests: add fips.enable.tls#4490
[rhcos-4.17] tests: add fips.enable.tls#4490openshift-cherrypick-robot wants to merge 1 commit intocoreos:rhcos-4.17from
fips.enable.tls#4490Conversation
To verify that using TLS works in FIPS mode by having Ignition fetch a remote resource over HTTPS with FIPS compatible algorithms. tests/containers: build the container based on `nginx-126:10.1`. See Timothee's comment coreos#4477 (comment) Fixes https://issues.redhat.com/browse/COS-3487
|
Hi @openshift-cherrypick-robot. Thanks for your PR. I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new test, fips.enable.tls, to verify TLS functionality in FIPS mode. The changes include the test definition in Go, and supporting scripts and a Containerfile to build an nginx container for this test. My primary concern, which I've marked as critical, is the test's reliance on a hardcoded public IP address, making it fragile and dependent on external infrastructure. I've also provided a few medium-severity suggestions to improve the robustness of the container build script and correct a minor documentation inconsistency.
| // Test that using TLS works in FIPS mode by having Ignition fetch | ||
| // a remote resource over HTTPS with FIPS compatible algorithms. | ||
| // See https://issues.redhat.com/browse/COS-3487 | ||
| // Note that 34.172.244.189, running RHCOS 9.6 (build 20260312-0) on | ||
| // Google Cloud Platform, provides HTTPS services using nginx-126:10.1. |
There was a problem hiding this comment.
The test relies on a hardcoded IP address (34.172.244.189), which makes it dependent on an external service that is not managed by the test framework. This is a fragile approach and can lead to test failures if the server at this IP is down, changed, or decommissioned, making the test suite unreliable.
A better approach would be to have the test provision the required HTTPS server as part of its setup, for example by running the fips-nginx container (which is also added in this PR) on a machine within the test cluster or on a temporary VM. This would make the test self-contained and more robust.
| @@ -0,0 +1,28 @@ | |||
| # fips-nginx Container | |||
|
|
|||
| This is used by the `fips.enable.https` test to verify that using | |||
There was a problem hiding this comment.
There's a small typo here. The test is named fips.enable.tls, not fips.enable.https. Please update the README to match the test name in mantle/kola/tests/fips/fips.go.
| This is used by the `fips.enable.https` test to verify that using | |
| This is used by the `fips.enable.tls` test to verify that using |
|
|
||
| ip="$1" | ||
|
|
||
| tmpdir="$(mktemp -d)" |
There was a problem hiding this comment.
The script creates a temporary directory but doesn't clean it up on exit. This can lead to an accumulation of temporary directories in /tmp. It's a good practice to use trap to ensure cleanup, even if the script fails. For example, you could add trap 'rm -rf "${tmpdir}"' EXIT after this line.
| ip="$1" | ||
|
|
||
| tmpdir="$(mktemp -d)" | ||
| cp Containerfile ${tmpdir} |
There was a problem hiding this comment.
This cp command assumes the script is run from the same directory as Containerfile. To make the script more robust and runnable from any location, you should reference Containerfile relative to the script's own location.
| cp Containerfile ${tmpdir} | |
| cp "$(dirname "$0")/Containerfile" "${tmpdir}" |
2 participants
This is an automated cherry-pick of #4477
/assign HuijingHei