[rhcos-4.18] tests: add fips.enable.tls#4491
[rhcos-4.18] tests: add fips.enable.tls#4491openshift-cherrypick-robot wants to merge 2 commits intocoreos:rhcos-4.18from
fips.enable.tls#4491Conversation
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 introduces a new test to verify TLS functionality in FIPS mode, which is a valuable addition. The implementation includes the test logic in Go, along with a containerized Nginx server for the test endpoint. My review focuses on improving the robustness and maintainability of the new test and its supporting scripts. The most critical issue is the use of a hardcoded IP address, which can make the test fragile. I've also provided suggestions to improve the build script and fix a minor documentation inconsistency.
| "contents": { | ||
| "source": "https://34.172.244.189:8443/index.html" | ||
| } |
There was a problem hiding this comment.
The test uses a hardcoded IP address 34.172.244.189 to fetch a resource. This makes the test fragile and dependent on an external service that might not be available or could change, leading to flaky tests and maintenance overhead.
It would be better to either:
- Dynamically provision the nginx server as part of the test setup and retrieve its IP address.
- If the server must be pre-existing, its address should be passed as a configuration parameter to the test suite, not hardcoded in the test itself.
| @@ -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 test name appears to be fips.enable.tls, not fips.enable.https. Please update the documentation for consistency.
| 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 leave garbage files on the system. It's a good practice to use trap to ensure cleanup, 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.
The script assumes it is run from the directory where Containerfile is located. This can be brittle. To make the script more robust and runnable from any location, you should reference files relative to the script's own path.
| cp Containerfile ${tmpdir} | |
| cp "$(dirname "$0")/Containerfile" "${tmpdir}" |
This should help us catch cases like [1] where we regressed on FIPS downstream. [1] https://issues.redhat.com/browse/OCPBUGS-65684 (cherry picked from commit d4327a4)
|
Pick up d4327a4 to see if this works. |
3 participants
This is an automated cherry-pick of #4477
/assign HuijingHei