[rhcos-4.19] kola/tests: Add failing test for FIPS & LUKS #4266
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. The implementation is mostly correct, but I've identified a few areas for improvement related to code clarity and maintainability. My suggestions include refactoring a function for better readability, renaming a shadowed variable to avoid confusion, and improving comments and code style in another function. These changes should make the code easier to understand and maintain in the future.
| // 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 function's doc comment is not in the standard Go format, and the implementation contains redundant or misleading comments and can be simplified. This suggestion improves readability and adheres to Go conventions.
// fileContainsPattern reads a file and verifies if it contains a pattern.
// It ensures the file exists and can be read, then uses a regular
// expression to search for the pattern.
func fileContainsPattern(path string, searchPattern string) (bool, error) {
file, err := os.ReadFile(path)
if err != nil {
return false, err
}
match := regexp.MustCompile(searchPattern).Match(file)
return match, nil
}| go func() { | ||
| resultingError := inst.WaitAll(ctx) | ||
| if resultingError == nil { | ||
| resultingError = fmt.Errorf("ignition unexpectedly succeeded") | ||
| } else if resultingError == platform.ErrInitramfsEmergency { | ||
| // Expected initramfs failure, checking the console file to ensure | ||
| // that it failed the expected way | ||
| found, err := fileContainsPattern(builder.ConsoleFile, searchPattern) | ||
| if err != nil { | ||
| resultingError = errors.Wrapf(err, "looking for pattern '%s' in file '%s' failed", searchPattern, builder.ConsoleFile) | ||
| } else if !found { | ||
| resultingError = fmt.Errorf("pattern '%s' in file '%s' not found", searchPattern, builder.ConsoleFile) | ||
| } else { | ||
| // The expected case | ||
| resultingError = nil | ||
| } | ||
| } else { | ||
| resultingError = errors.Wrapf(resultingError, "expected initramfs emergency.target error") | ||
| } | ||
| errchan <- resultingError | ||
| }() |
There was a problem hiding this comment.
The error handling logic within the goroutine is deeply nested. It can be refactored to use early returns, which makes the code flatter and easier to read and maintain.
go func() {
resultingError := inst.WaitAll(ctx)
if resultingError == nil {
errchan <- fmt.Errorf("ignition unexpectedly succeeded")
return
}
if resultingError != platform.ErrInitramfsEmergency {
errchan <- errors.Wrapf(resultingError, "expected initramfs emergency.target error")
return
}
// Expected initramfs failure, checking the console file to ensure
// that it failed the expected way
found, err := fileContainsPattern(builder.ConsoleFile, searchPattern)
if err != nil {
errchan <- errors.Wrapf(err, "looking for pattern '%s' in file '%s' failed", searchPattern, builder.ConsoleFile)
return
}
if !found {
errchan <- fmt.Errorf("pattern '%s' in file '%s' not found", searchPattern, builder.ConsoleFile)
return
}
// The expected case
errchan <- nil
}()| 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.
The variable failConfig is shadowed on line 174. The global failConfig is a *conf.UserData, while the local failConfig is a *conf.Conf. This is confusing and can lead to bugs. It's better to use a different name for the local variable to improve code clarity.
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-")
// Instruct builder to use it
builder.ConsoleFile = consoleFile.Name()
builder.SetConfig(renderedConfig)
|
I don't think we use this branch of COSA for anything. |
Correct. The backport would go straight to 4.18 and down. |
This is an automated cherry-pick of #4181
/assign aaradhak