[rhcos-4.14] kola/tests: Add failing test for FIPS & LUKS#4262
Conversation
|
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 |
|
The lint failures here look like they would be legitimate build failures. |
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.
HuijingHei
force-pushed
the
cherry-pick-4181-to-rhcos-4.14
branch
from
29292fa to
2f029b9
Compare
|
Rebase to rhcos-4.14 branch and wait for the results. |
It's generally a good idea for variable names include their units. (cherry picked from commit 55658b4)
Now that coreos/fedora-coreos-pipeline#959 is merged, we have these images built by the coreOS CI and hosted on the quay.io/coreos-assembler org. Point tests that requires containers to use images from the coreos-assembler org. See coreos#3727 Fixes coreos/fedora-coreos-tracker#1639 (cherry picked from commit 6cbe502)
|
Glad to see the test is passed. |
|
/retest-required |
|
/test rhcos |
1 similar comment
|
/test rhcos |
7 participants
This is an automated cherry-pick of #4181
/assign aaradhak