[rhcos-4.17] kola/tests: Add failing test for FIPS & LUKS #4264
Conversation
Ensure that setting up a LUKS device with FIPS incompatible algorithms will fail when FIPS mode is enabled. Only run this on QEMU as it should behave the same way on all platforms.
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-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 failing test for FIPS and LUKS, which is a valuable addition to verify failure modes. The overall logic is sound. My review includes a fix for a potential resource leak, and suggestions to improve error handling and code maintainability for better long-term quality.
| } | ||
|
|
||
| // Create a temporary log file | ||
| consoleFile := c.H.TempFile("console-") |
There was a problem hiding this comment.
The file handle returned by c.H.TempFile is not being closed, which can lead to a resource leak. You should defer its closure immediately after creation.
consoleFile := c.H.TempFile("console-")
defer consoleFile.Close()| if err := ignitionFailure(c); err != nil { | ||
| c.Fatal(err.Error()) | ||
| } |
There was a problem hiding this comment.
Using err.Error() with c.Fatal can cause loss of valuable stack trace information when using libraries like github.com/pkg/errors. Passing the error object directly to c.Fatal will preserve the stack trace and improve debuggability.
| if err := ignitionFailure(c); err != nil { | |
| c.Fatal(err.Error()) | |
| } | |
| if err := ignitionFailure(c); err != nil { | |
| c.Fatal(err) | |
| } |
| // Read file and verify if it contains a pattern | ||
| // 1. Read file, make sure it exists | ||
| // 2. regex for pattern | ||
| func fileContainsPattern(path string, searchPattern string) (bool, error) { | ||
| file, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| // File has content, but the pattern is not present | ||
| match := regexp.MustCompile(searchPattern).Match(file) | ||
| if match { | ||
| // Pattern found | ||
| return true, nil | ||
| } | ||
| // Pattern not found | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
This function is more verbose than necessary and contains redundant comments. It can be simplified for better readability and maintainability.
// fileContainsPattern reads a file and reports whether it contains a match for a regex pattern.
func fileContainsPattern(path string, searchPattern string) (bool, error) {
file, err := os.ReadFile(path)
if err != nil {
return false, err
}
// The pattern is a static string, so MustCompile is safe.
return regexp.MustCompile(searchPattern).Match(file), nil
}| defer builder.Close() | ||
|
|
||
| // Prepare Ingnition config | ||
| failConfig, err := failConfig.Render(conf.FailWarnings) |
There was a problem hiding this comment.
The variable failConfig returned by failConfig.Render shadows the package-level variable of the same name. This can be confusing and hurt maintainability. It's better to use a different name for the rendered config, like renderedConfig, to avoid ambiguity. You will also need to update its usage on line 184.
renderedConfig, err := failConfig.Render(conf.FailWarnings)
|
/retest |
This is an automated cherry-pick of #4181
/assign aaradhak