[rhcos-4.14] kola/tests: Add failing test for FIPS & LUKS#4262
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. 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 case to verify that cryptsetup fails with non-FIPS-compliant algorithms when FIPS mode is enabled. The overall test logic is sound and covers the intended scenario. I have provided a few suggestions to improve code clarity and maintainability by addressing variable shadowing and simplifying error handling logic.
| 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 in this goroutine is a bit complex due to multiple reassignments of resultingError. Using a switch statement on the error from inst.WaitAll(ctx) would make the logic clearer and easier to maintain.
| 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 | |
| err := inst.WaitAll(ctx) | |
| switch err { | |
| case nil: | |
| errchan <- fmt.Errorf("ignition unexpectedly succeeded") | |
| case platform.ErrInitramfsEmergency: | |
| // Expected initramfs failure, checking the console file to ensure | |
| // that it failed the expected way | |
| found, patternErr := fileContainsPattern(builder.ConsoleFile, searchPattern) | |
| if patternErr != nil { | |
| errchan <- errors.Wrapf(patternErr, "looking for pattern '%s' in file '%s' failed", searchPattern, builder.ConsoleFile) | |
| } else if !found { | |
| errchan <- fmt.Errorf("pattern '%s' in file '%s' not found", searchPattern, builder.ConsoleFile) | |
| } else { | |
| // The expected case | |
| errchan <- nil | |
| } | |
| default: | |
| errchan <- errors.Wrap(err, "expected initramfs emergency.target error") | |
| } |
| defer builder.Close() | ||
|
|
||
| // Prepare Ingnition config | ||
| failConfig, err := failConfig.Render(conf.FailWarnings) |
There was a problem hiding this comment.
The local variable failConfig shadows the global package-level variable of the same name. This can be confusing and lead to bugs. It's better to use a different name for the local variable, for example renderedConfig.
| failConfig, err := failConfig.Render(conf.FailWarnings) | |
| renderedConfig, err := failConfig.Render(conf.FailWarnings) |
|
|
||
| // Instruct builder to use it | ||
| builder.ConsoleFile = consoleFile.Name() | ||
| builder.SetConfig(failConfig) |
There was a problem hiding this comment.
To fix the variable shadowing issue from line 174, this should use the newly named renderedConfig variable.
| builder.SetConfig(failConfig) | |
| builder.SetConfig(renderedConfig) |
|
/retest |
|
@openshift-cherrypick-robot: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
The lint failures here look like they would be legitimate build failures. |
4 participants
This is an automated cherry-pick of #4181
/assign aaradhak