[rhcos-4.16] tests: add fips.enable.tls#4489
[rhcos-4.16] tests: add fips.enable.tls#4489openshift-cherrypick-robot wants to merge 1 commit intocoreos:rhcos-4.16from
fips.enable.tls#4489Conversation
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 by fetching a resource over HTTPS. The changes include the test definition, a new test function, and assets for building a FIPS-compliant nginx container. My review identifies a critical dependency on a hardcoded IP address which makes the test brittle, a minor documentation inconsistency, and suggests an improvement to a build script to ensure proper cleanup of temporary resources.
| { | ||
| "path": "/var/resource/https-fips", | ||
| "contents": { | ||
| "source": "https://34.172.244.189:8443/index.html" |
There was a problem hiding this comment.
This test relies on a hardcoded IP address (34.172.244.189). This makes the test brittle and dependent on an external resource that may not be available or may change over time, leading to test flakiness and maintenance overhead. It would be more robust to have the test environment dynamically set up a local server for this purpose, or use a DNS name that can be pointed to a new server if needed. If a dynamic setup is not feasible, consider moving the IP address to a configuration variable to make it easier to update.
| @@ -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.
The README mentions the test name is fips.enable.https, but the actual test registered in mantle/kola/tests/fips/fips.go is named fips.enable.tls. To avoid confusion, please update the README to use the correct test name.
| 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 using mktemp -d but doesn't clean it up upon exit. This can lead to an accumulation of temporary files on the build machine. It's a good practice to use a trap to ensure the temporary directory is removed, even if the script fails.
| tmpdir="$(mktemp -d)" | |
| tmpdir="$(mktemp -d)" | |
| trap 'rm -rf "${tmpdir}"' EXIT |
2 participants
This is an automated cherry-pick of #4477
/assign HuijingHei