[rhcos-4.16] kola/tests: Add failing test for FIPS & LUKS#4261
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 lukscreate fails when FIPS is enabled with an incompatible crypto algorithm. The implementation looks good, but I have identified a couple of areas for improvement: a potential goroutine leak due to an unbuffered channel, and a variable shadowing issue that could affect code readability and maintainability. Please see my detailed comments.
|
|
||
| defer cancel() | ||
|
|
||
| errchan := make(chan error) |
There was a problem hiding this comment.
The errchan is unbuffered. If the context times out (<-ctx.Done() case is selected), the goroutine launched on line 133 will block forever when it tries to send on errchan, as there will be no receiver. This will cause a goroutine leak. Using a buffered channel of size 1 will prevent this.
| errchan := make(chan error) | |
| errchan := make(chan error, 1) |
| 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 declared on line 174 shadows the global variable of the same name. This can be confusing. It's better to use a different name for the rendered config, for example renderedConfig.
| 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) | |
| 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) |
|
/ok-to-test |
5 participants
This is an automated cherry-pick of #4181
/assign aaradhak