[rhcos-4.15] tests: add fips.enable.tls#4488
[rhcos-4.15] tests: add fips.enable.tls#4488openshift-cherrypick-robot wants to merge 1 commit intocoreos:rhcos-4.15from
fips.enable.tls#4488Conversation
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. This involves adding the test logic in Go, along with a container and build scripts to provide a FIPS-compliant HTTPS server for the test. My review focuses on improving the test's robustness and the maintainability of the build scripts. The main concern is the use of a hardcoded IP address in the test, which makes it fragile. I've also suggested improvements to the shell script for building the test container to make it more robust and cleaner.
| { | ||
| "path": "/var/resource/https-fips", | ||
| "contents": { | ||
| "source": "https://34.172.244.189:8443/index.html" |
There was a problem hiding this comment.
Using a hardcoded IP address makes the test fragile and dependent on an external server that might not be available or could change. This can lead to flaky tests. Consider making the IP address configurable, for example, via an environment variable, or setting up the test server dynamically as part of the test suite execution. The accompanying build.sh script for the test container already takes an IP, which suggests this was considered.
| @@ -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 name is fips.enable.tls, not fips.enable.https.
| 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 with mktemp -d but never cleans it up. This can leave garbage in /tmp. It's good practice to use trap to ensure cleanup on exit, even if the script fails.
| tmpdir="$(mktemp -d)" | |
| tmpdir="$(mktemp -d)" | |
| trap 'rm -rf "${tmpdir}"' EXIT |
| ip="$1" | ||
|
|
||
| tmpdir="$(mktemp -d)" | ||
| cp Containerfile ${tmpdir} |
There was a problem hiding this comment.
This cp command assumes the script is run from its own directory. It would be more robust to qualify the path to Containerfile so the script can be called from anywhere.
| cp Containerfile ${tmpdir} | |
| cp "$(dirname "$0")/Containerfile" "${tmpdir}" |
2 participants
This is an automated cherry-pick of #4477
/assign HuijingHei