[rhcos-4.18] kola/tests: Add failing test for FIPS & LUKS #4265
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 test case to verify that cryptsetup fails with non-FIPS-compliant algorithms when FIPS mode is enabled. The overall approach is sound. I've provided a few suggestions to improve code clarity, maintainability, and resource management in the new test file.
| // 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.
The comments in this function are either redundant or incorrect (e.g., line 111). The function body can also be simplified for better readability and maintainability.
// fileContainsPattern reads a file and verifies if it contains a pattern.
func fileContainsPattern(path string, searchPattern string) (bool, error) {
file, err := os.ReadFile(path)
if err != nil {
return false, err
}
return regexp.Match(searchPattern, file)
}| failConfig, err := failConfig.Render(conf.FailWarnings) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "creating invalid FIPS config") | ||
| } | ||
|
|
||
| // Create a temporary log file | ||
| consoleFile := c.H.TempFile("console-") | ||
|
|
||
| // Instruct builder to use it | ||
| builder.ConsoleFile = consoleFile.Name() | ||
| builder.SetConfig(failConfig) |
There was a problem hiding this comment.
There are a couple of improvements that can be made here:
- The variable
failConfigis shadowed on line 174. The globalfailConfigis of typeconf.UserData, while the new local variable is*conf.Conf. This can be confusing. Consider renaming the local variable to something likerenderedConfigto improve clarity. - The temporary file created by
c.H.TempFileon line 180 is not being closed, which can lead to a resource leak. You should defer its closing right after creation.
renderedConfig, err := failConfig.Render(conf.FailWarnings)
if err != nil {
return errors.Wrapf(err, "creating invalid FIPS config")
}
// Create a temporary log file
consoleFile := c.H.TempFile("console-")
defer consoleFile.Close()
// Instruct builder to use it
builder.ConsoleFile = consoleFile.Name()
builder.SetConfig(renderedConfig)
|
/ok-to-test |
This is an automated cherry-pick of #4181
/assign aaradhak