Catch invalid root user kickstart options early [HMS-9378]#1879
Conversation
| } | ||
| } | ||
|
|
||
| // This check repeats the same checks that are made in the kickstart stage |
There was a problem hiding this comment.
Quick drive-by: can move the checks into a shared helper somehow maybe? It worries me that we have two places now that (almost) do the same thing, it will most likely cause drift if/when we add more checks to one place.
Or we could look at the root of the problem and make serialize() return an error (probably a much bigger PR though)
There was a problem hiding this comment.
Quick drive-by: can move the checks into a shared helper somehow maybe? It worries me that we have two places now that (almost) do the same thing, it will most likely cause drift if/when we add more checks to one place.
Yeah I mentioned as much in the PR description, but I couldn't think of an obvious way to do it nicely since they're different types.
Or we could look at the root of the problem and make serialize() return an error (probably a much bigger PR though)
Yes, we should get on this.
Make the Path option in the kickstart stage constructors required and validate it. The path was always required in the underlying stage schema, but we never checked for it here. This catches problems earlier in the manifest generation.
achilleas-k
force-pushed
the
fix/catch-invalid-root-user-kickstart-early
branch
from
b3f8a6a to
247b6c6
Compare
Add two tests to each AnacondaISOTree variant test: - One with a valid user customization in the kickstart. - One with an invalid root user customization in the kickstart.
The previous message was accidentally set to the same as the container case.
Validate root user options when creating internal kickstart customizations from a blueprint. These validations replicate the same checks made in the kickstart stage option creation, but by having them here we can catch invalid customizations earlier and provide better error reporting.
achilleas-k
force-pushed
the
fix/catch-invalid-root-user-kickstart-early
branch
from
247b6c6 to
4f76acf
Compare
|
Rebased and fixed the tests. Also fixed an error message from the other PR. |
thozza
left a comment
thozza
left a comment
There was a problem hiding this comment.
LGTM. I think that the validation code duplication is acceptable in this case.
4 participants
In #1806 I changed the behaviour of the kickstart stage so that it returns errors when unsupported root user options are specified. I knew this might cause issues with old working blueprints becoming invalid, but I decided (and I think we all agreed) that errors are preferable to options being silently ignored.
This is now causing
panic()s during serialization of stages that write kickstart files, because we currently don't return errors from theserialize()functions in pipelines (see below).https://github.com/osbuild/images/blob/0a3a48f4bc8ec382d7e31f69535488cbb1e27fc2/pkg/manifest/anaconda_installer_iso_tree.go#L553-L565
This PR adds the same validation earlier in the pipeline generation, in the
kickstart.New()function that is called by the manifest instantiation functions. This makes it possible to return an error early instead of causing a panic during serialization.I think it's generally valid to have the same or similar checks in two places, since the osbuild package can work on its own as an interface for stages. It would be nice if we could unify the checks somehow, but that's an exercise for later, if it would be possible.
I considered putting these checks in the
CheckOptions()function(s) as well, but that requires knowing which image type uses kickstarts, and that information isn't available right now (BootISOis true for the ostree-simplified-installers, which don't use kickstarts).Another solution here would be if we moved user creation out of kickstarts and into the root tree itself for the image installer, but that's a much bigger change that would require some thought. One issue with that change, off the top of my head, is that it could create problems if the same user that's been created in the image already is also set during installation.