Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: test root user setup #363

Merged
merged 2 commits into from
May 7, 2024
Merged

Conversation

mvo5
Copy link
Collaborator

@mvo5 mvo5 commented Apr 18, 2024

A continuation of #357 - the root user is used to do a bootc status inside the booted container to ensure the setup is correct. This is nicer then the previous sudo approach and it has the added benefit of testing the root user creation/setup during the build too.

@mvo5 mvo5 force-pushed the test-root-customizations branch 2 times, most recently from a52b2c8 to 7f55f24 Compare April 29, 2024 09:52
@mvo5 mvo5 marked this pull request as ready for review April 30, 2024 07:20
@mvo5 mvo5 requested a review from achilleas-k April 30, 2024 07:20
@mvo5 mvo5 force-pushed the test-root-customizations branch from 7325f31 to 40912b6 Compare May 3, 2024 09:14
achilleas-k
achilleas-k previously approved these changes May 3, 2024
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splendid. Minor comment but no biggie.

bib/go.mod Outdated Show resolved Hide resolved
@mvo5 mvo5 force-pushed the test-root-customizations branch 2 times, most recently from d3a379c to 00d7193 Compare May 6, 2024 06:41
@achilleas-k achilleas-k enabled auto-merge May 6, 2024 10:51
@achilleas-k achilleas-k disabled auto-merge May 6, 2024 10:51
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost merged. Missed this fixup commit.

@mvo5 mvo5 force-pushed the test-root-customizations branch from 18b544d to 3425576 Compare May 6, 2024 13:37
mvo5 added a commit to mvo5/osbuild that referenced this pull request May 6, 2024
There was a bug found in
osbuild/bootc-image-builder#363

where a stray newlnine in the ssh key broke the kickstart file.
mvo5 added 2 commits May 6, 2024 16:08
This allows us to test our customization with something actually
really useful as it will give us easier debug when test runs
fail.
With ssh a root login is only possible via a sshkey. So let's
support this so that we can run `bootc status` which requires
root privs.
With the switch to bootc we need to adjust the testing. We inject
a root ssh key now and just use that for login.
@mvo5 mvo5 force-pushed the test-root-customizations branch from 3425576 to 65faf11 Compare May 6, 2024 14:08
@mvo5
Copy link
Collaborator Author

mvo5 commented May 6, 2024

Almost merged. Missed this fixup commit.

I removed the fixup commit now and fixed the bug in the PR that prevented the tests from working - it turns out we wrote invalid kickstart files because the keyfile read from disk contained a (not stripped) newline at the end and with the kickstart file looked somehting like:

sshkey --username foo "ssh-key
"

I opened osbuild/osbuild#1772 with one idea

@mvo5 mvo5 enabled auto-merge May 7, 2024 16:20
@mvo5 mvo5 added this pull request to the merge queue May 7, 2024
Merged via the queue into osbuild:main with commit e332166 May 7, 2024
9 checks passed
mvo5 added a commit to mvo5/osbuild that referenced this pull request May 16, 2024
There was a bug in osbuild/bootc-image-builder#363
where a stray newlnine in the ssh key broke the kickstart file.

The isse is that when auto-generating the config.json for an
ssh keyfile it's easy to accidentally include the final `\n` in
the ssh key json string. This will break in very non-obvious ways,
i.e. the image will be generated but the kickstart file is broken
because now it looks like:
```
sshkey --username foo "ssh-key
"
```
which make the kickstart invalid but that is only discovered
much later when the image booted (or even later after the installer
put the kickstart into the target system).

We probably want to disallow `\n` in any kickstart option via
the schema as I don't see how this could possibly not break.
mvo5 added a commit to mvo5/osbuild that referenced this pull request May 16, 2024
There was a bug in osbuild/bootc-image-builder#363 where a stray
newlnine in the ssh key broke the kickstart file.

The isse is that when auto-generating the config.json for an ssh keyfile
it's easy to accidentally include the final `\n` in the ssh key json
string. This will break in very non-obvious ways, i.e. the image will
be generated but the kickstart file is broken because now it looks like:
```
sshkey --username foo "ssh-key
"
```
which make the kickstart invalid but that is only discovered much later
when the image booted (or even later after the installer put the
kickstart into the target system).

This commit runs `ksvalidator` (if available) to detect this kind
of issue early. It requires `pykickstart` in the buildroot but
that seems to be not too bad as we already have `python3-kickstart`
in our buildroots AFAICT (unless I read the manifest-db incorrectly).

So it does require an extra tweak to `images` before it's fully
useful.
mvo5 added a commit to mvo5/osbuild that referenced this pull request May 16, 2024
There was a bug in osbuild/bootc-image-builder#363 where a stray
newlnine in the ssh key broke the kickstart file.

The isse is that when auto-generating the config.json for an ssh keyfile
it's easy to accidentally include the final `\n` in the ssh key json
string. This will break in very non-obvious ways, i.e. the image will
be generated but the kickstart file is broken because now it looks like:
```
sshkey --username foo "ssh-key
"
```
which make the kickstart invalid but that is only discovered much later
when the image booted (or even later after the installer put the
kickstart into the target system).

This commit runs `ksvalidator` (if available) to detect this kind
of issue early. It requires `pykickstart` in the buildroot but
that seems to be not too bad as we already have `python3-kickstart`
in our buildroots AFAICT (unless I read the manifest-db incorrectly).

So it does require an extra tweak to `images` before it's fully
useful.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants