OSInside / kiwi

KIWI - Appliance Builder Next Generation
https://osinside.github.io/kiwi
GNU General Public License v3.0
300 stars 152 forks source link

setup: ensure a valid keymap is passed to systemd-firstboot #2515

Closed g7 closed 5 months ago

g7 commented 6 months ago

The keytable preference might specify the file extension (like us.map.gz), while systemd-firstboot actually expects only the keymap name [0].

Until recently [1], systemd-firstboot would not validate the supplied keymap, so /etc/vconsole.conf was always updated even if the keymap is wrong.

After the recent change, kiwi would fail with this error message when using the full file name as a keytable:

[ 91s] [ INFO ]: 11:04:56 | Setting up keytable: us.map.gz [ 91s] [ DEBUG ]: 11:04:56 | EXEC: [chroot /usr/src/packages/KIWI-vmx/build/image-root systemd-firstboot --help] [ 91s] [ DEBUG ]: 11:04:56 | EXEC: [chroot /usr/src/packages/KIWI-vmx/build/image-root systemd-firstboot --keymap=us.map.gz] [ 91s] [ DEBUG ]: 11:04:56 | EXEC: Failed with stderr: Keymap us.map.gz is not installed. [ 91s] , stdout: (no output on stdout) [ 91s] [ ERROR ]: 11:04:56 | KiwiCommandError: chroot: stderr: Keymap us.map.gz is not installed. [ 91s] , stdout: (no output on stdout)

[0] https://man7.org/linux/man-pages/man1/systemd-firstboot.1.html [1] https://github.com/systemd/systemd/commit/321a8c595e3470a0fe9002014656eee6a50b9553

Fixes bsc#1221878.

As far as I know, there aren't any keymaps with a point in their name - so another approach could be to simply drop any extension from the keytable preference.

g7 commented 5 months ago

Sorry I wasn't able to further investigate due to holidays and other work stuff :(

Looking at the code I have to at least modify that to handle .map as well: https://github.com/systemd/systemd/blob/11a150bc43d08e4d1e1622aca696770c59bda03c/src/shared/kbd-util.c#L62

in SLE at least, every SLE-15 release has the --keymap option available (patch has been backported). On systemd upstream, it is supported since v236: https://github.com/systemd/systemd/commit/ed457f1380d690e26163fd3ad6aff946048e1064

I have already tried to create a SLE-15-SP0 image using <keytable>it.map.gz</keytable> and indeed in /etc/vconsole.conf it gets set as KEYMAP=it.map.gz. Unfortunately I was unable to boot to check if it works fine with it as the build fails I think because I tried to build with a too new kiwi version.

g7 commented 5 months ago

Some further tests on this:

Build host: SLE 15 SP1 virtual machine

I have tried building the following appliances:

with <keytable>it.map.gz</keytable> inside the descriptions. The descriptions I've used are here: https://github.com/g7/kiwi-descriptions-test/commit/7a299c664410d70dba5a256a8be2c375b985ef64

(to make a bootable openSUSE Leap 15.0 appliance, I had to rebuild the last kiwi 9.25 version against 15.0, that's why in the mentioned commit I had to branch the Virtualization:Appliances:Builder project)

The good news is that on every appliance both KEYMAP=it.map.gz (which is what systemd-firstboot wrote during image build) and KEYMAP=it, the keymap is changed correctly.

So it looks like this change should work on every scenario. But I need for sure to amend the replacement to take care of uncompressed .map file as well.

Thanks.

schaefi commented 5 months ago

I might misunderstand the reason for this change, so maybe you can clarify on that. So from my point of view kiwi should not be clever on the given keytable name. As you can see from the code we handle the given value in two ways:

  1. There is systemd-firstboot we use it to do the job
  2. There is the old /etc/sysconfig/keyboard we add KEYTABLE with the provided value

In none of these cases we modify the given <keytable> value and I think this behavior should not change. What I mean is if the author of an image description creates it for e.g Tumbleweed the expected value for the keytable setup is

<keytable>us</keytable>

Because this distro expects it that way according to the version of systemd-firstboot.

If the author of an image description creates it for let's say SLES 12, the correct setup is

<keytable>us.map.gz</keytable>

Because there we still have /etc/sysconfig/keyboard and this distro expects it that way.

Let's say some other distro; Fedora, RHEL, etc etc expects even another value we can handle it as long as we don't try to be clever and modify the provided value.

My point here is that I believe we are making our lives harder than it should be. An image description is a living document. If the distribution changes also the image description might be changed to follow the distro change. It should not be the case that we add some code which we have to maintain for a semantic change of some software component in the distribution.

Unless my understanding is wrong I would not add code that modifies the provided keytable value

Thoughts ?

Conan-Kudo commented 5 months ago

I agree with your assessment here. We can't really predict how this is supposed to be handled, and I'd rather not try.

g7 commented 5 months ago

Thanks for the detailed reply. So let me elaborate further, sorry for not having been more clear in the first place:

This Pull Request only changes the behaviour when calling systemd-firstboot --keymap, so /etc/sysconfig/keyboard would not be modified.

What prompted this change (and the related bugzilla ticket) is this systemd commit that added validation on the systemd-firstboot input.

(this got actually cherry-picked in SLE15SP6's systemd and that's when I noticed this issue in the first place)

Before that commit, systemd-firstboot would have accepted every kind of keymap without validation. For example, this command works fine in Leap 15.5:

rm /etc/vconsole.conf
systemd-firstboot --keymap invalid-keymap.map.gz

while this seemingly valid one would fail in SP6/Tumbleweed:

rm /etc/vconsole.conf
systemd-firstboot --keymap it.map.gz

So that's what pointed me (possibly in the wrong way!) to "I think it's a bug, which we didn't know about until now" rather than "It's a behaviour change" :)

Every systemd-firstboot --keymap usage I saw searching on the net (and in the systemd-firstboot manpage, but it's not very detailed to be honest) does not specify the file extension. yast2 itself doesn't specify the file extension in /etc/vconsole.conf when installing the system.

My point here is that I believe we are making our lives harder than it should be. An image description is a living document. If the distribution changes also the image description might be changed to follow the distro change. It should not be the case that we add some code which we have to maintain for a semantic change of some software component in the distribution.

Thanks, makes a lot of sense being put this way. I approached this thinking that this should be a bug to be fixed, but I see your point as well.

schaefi commented 5 months ago

@g7 thanks much for your detailed in-depth search on this topic.

Every systemd-firstboot --keymap usage I saw searching on the net (and in the systemd-firstboot manpage, but it's not very detailed to be honest) does not specify the file extension. yast2 itself doesn't specify the file extension in /etc/vconsole.conf when installing the system.

I agree. I think the only place where it was ever required to pass the filename of the map file was on systems with /etc/sysconfig/keyboard

@Conan-Kudo correct me when wrong but for kiwi v10 our oldest supported distro also does not use /etc/sysconfig/keyboard. So we could think of dropping that path from kiwi entirely and I suggest to update the documentation and tell the user that the expected value is the plain map name of the keyboard.

I still think kiwi should take the given name as it is and should not modify it

@g7 @Conan-Kudo How about that ?

g7 commented 5 months ago

Yes, I agree, especially after the testing in this comment https://github.com/OSInside/kiwi/pull/2515#issuecomment-2058946795 showed that systemd is perfectly fine with and without the using the explicit file name in /etc/vconsole.conf.

I'm closing this for now. If there is ever a need, it can be reopened :) Thanks for your time!

Conan-Kudo commented 5 months ago

Yes, we should drop /etc/sysconfig/keyboard too.

Conan-Kudo commented 5 months ago

@schaefi That all makes sense.