From 0968ee8d81d16f19c05b87de6f095d08f282f388 Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Thu, 5 Jun 2025 16:43:46 +0200 Subject: [PATCH] stages/grub2: add compat_version for fixed behaviour For the terminal_input, terminal_output, and serial variables in the GRUB_CFG_TEMPLATE, the keys were not set when those variables were not defined in the options. This caused the template variables to show up in the final file itself, which could break the config. For example, the following line was being generated for one of our images: serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1${terminal_input}${terminal_output} Setting the keys to an empty string when they're not defined solves the issue. Since this is a stage behaviour change, the new behaviour is toggled by an option that maintains backwards compatibility when not set. This introduces a new convention of adding a `compat_version` number to the stage options to control the behaviour. The value defaults to 1, which is the old (broken) behaviour. Manifest generators should always set it to 2 to get the correct behaviour. --- stages/org.osbuild.grub2 | 17 +- stages/org.osbuild.grub2.meta.json | 9 + stages/test/test_grub2.py | 262 ++++++++++++++++++++++++++++- 3 files changed, 284 insertions(+), 4 deletions(-) diff --git a/stages/org.osbuild.grub2 b/stages/org.osbuild.grub2 index 87ba4d71..b1615882 100755 --- a/stages/org.osbuild.grub2 +++ b/stages/org.osbuild.grub2 @@ -193,7 +193,7 @@ class GrubConfig: ) return {terminal: val} - def write(self, tree): + def write(self, tree, bugfix_remove_template_vars): """Write the grub config to `tree` at `self.path`""" path = os.path.join(tree, self.path) @@ -222,6 +222,14 @@ class GrubConfig: "features": features, } + if bugfix_remove_template_vars: + # set all remaining config options to empty strings in case they are not set + config.update({ + "serial": "", + "terminal_input": "", + "terminal_output": "", + }) + if self.serial: config["serial"] = "\n" + self.serial @@ -294,6 +302,7 @@ class GrubConfig: # pylint: disable=too-many-statements,too-many-branches def main(tree, options): + compat_version = options.get("compat_version", 1) root_fs = options.get("rootfs") boot_fs = options.get("bootfs") kernel_opts = options.get("kernel_opts", "") @@ -305,6 +314,10 @@ def main(tree, options): saved_entry = options.get("saved_entry") cfg = options.get("config", {}) + # handle compat_version switching behaviour: + # Version 2+: Remove unset variables from the template. See https://issues.redhat.com/HMS-8646 for details. + bugfix_remove_template_vars = compat_version >= 2 + # backwards compatibility if not root_fs: root_fs = {"uuid": options["root_fs_uuid"]} @@ -402,7 +415,7 @@ def main(tree, options): config.path = grubcfg # Now actually write the main grub.cfg file - config.write(tree) + config.write(tree, bugfix_remove_template_vars) if legacy: copy_modules(tree, legacy) diff --git a/stages/org.osbuild.grub2.meta.json b/stages/org.osbuild.grub2.meta.json index bad25f8e..a0eedfc4 100644 --- a/stages/org.osbuild.grub2.meta.json +++ b/stages/org.osbuild.grub2.meta.json @@ -128,6 +128,15 @@ } }, "properties": { + "compat_version": { + "type": "number", + "enum": [ + 1, + 2 + ], + "description": "The compatibility behavior to use. Old versions are just there for bug compatibility. You should always use the highest version available.", + "default": 1 + }, "rootfs": { "$ref": "#/definitions/filesystem" }, diff --git a/stages/test/test_grub2.py b/stages/test/test_grub2.py index 91a8d987..a6825186 100644 --- a/stages/test/test_grub2.py +++ b/stages/test/test_grub2.py @@ -33,11 +33,38 @@ def test_grub2_copy_efi_data(tmp_path, stage_module): # Test that the /etc/default/grub file is created with the correct content -@pytest.mark.parametrize("test_data,kernel_opts,expected_conf", [ +@pytest.mark.parametrize("test_data,kernel_opts,expected_conf,expected_boot_conf", [ # default ({}, "", """GRUB_CMDLINE_LINUX="" GRUB_TIMEOUT=0 GRUB_ENABLE_BLSCFG=true +""", + """ +set timeout=0 + +# load the grubenv file +load_env + +# selection of the next boot entry via variables 'next_entry' and +# `saved_entry` present in the 'grubenv' file. Both variables are +# set by grub tools, like grub2-reboot, grub2-set-default + +if [ "${next_entry}" ] ; then + set default="${next_entry}" + set next_entry= + save_env next_entry + set boot_once=true +else + set default="${saved_entry}" +fi + +search --no-floppy --set=root --label root +set boot=${root} +function load_video { + insmod all_video +} +${serial}${terminal_input}${terminal_output} +blscfg """), # custom ({ @@ -53,6 +80,36 @@ GRUB_SERIAL_COMMAND="serial --speed=115200 --unit=0 --word=8 --parity=no --stop= GRUB_TERMINAL_INPUT="console" GRUB_TERMINAL_OUTPUT="serial" GRUB_DEFAULT=0 +""", + """ +set timeout=10 + +# load the grubenv file +load_env + +# selection of the next boot entry via variables 'next_entry' and +# `saved_entry` present in the 'grubenv' file. Both variables are +# set by grub tools, like grub2-reboot, grub2-set-default + +if [ "${next_entry}" ] ; then + set default="${next_entry}" + set next_entry= + save_env next_entry + set boot_once=true +else + set default="${saved_entry}" +fi + +search --no-floppy --set=root --label root +set boot=${root} +function load_video { + insmod all_video +} + +serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1 +terminal_input console +terminal_output serial +blscfg """), # custom (Azure) ({ @@ -78,12 +135,42 @@ GRUB_TERMINAL="serial console" GRUB_TERMINAL_OUTPUT="console" GRUB_TIMEOUT_STYLE=countdown GRUB_DEFAULT=saved +""", + """ +set timeout=10 + +# load the grubenv file +load_env + +# selection of the next boot entry via variables 'next_entry' and +# `saved_entry` present in the 'grubenv' file. Both variables are +# set by grub tools, like grub2-reboot, grub2-set-default + +if [ "${next_entry}" ] ; then + set default="${next_entry}" + set next_entry= + save_env next_entry + set boot_once=true +else + set default="${saved_entry}" +fi + +search --no-floppy --set=root --label root +set boot=${root} +function load_video { + insmod all_video +} + +serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1${terminal_input} +terminal_output console +blscfg """), ]) -def test_grub2_default_conf(tmp_path, stage_module, test_data, kernel_opts, expected_conf): +def test_grub2_default_conf(tmp_path, stage_module, test_data, kernel_opts, expected_conf, expected_boot_conf): treedir = tmp_path / "tree" confpath = treedir / "etc/default/grub" confpath.parent.mkdir(parents=True, exist_ok=True) + bootconfpath = treedir / "boot/grub2/grub.cfg" options = { "rootfs": { @@ -107,3 +194,174 @@ def test_grub2_default_conf(tmp_path, stage_module, test_data, kernel_opts, expe assert os.path.exists(confpath) assert confpath.read_text() == expected_conf + + assert os.path.exists(bootconfpath) + assert bootconfpath.read_text() == expected_boot_conf + + +# Test that the /etc/default/grub file is created with the correct content +@pytest.mark.parametrize("test_data,kernel_opts,expected_conf,expected_boot_conf", [ + # default + ({}, "", """GRUB_CMDLINE_LINUX="" +GRUB_TIMEOUT=0 +GRUB_ENABLE_BLSCFG=true +""", + """ +set timeout=0 + +# load the grubenv file +load_env + +# selection of the next boot entry via variables 'next_entry' and +# `saved_entry` present in the 'grubenv' file. Both variables are +# set by grub tools, like grub2-reboot, grub2-set-default + +if [ "${next_entry}" ] ; then + set default="${next_entry}" + set next_entry= + save_env next_entry + set boot_once=true +else + set default="${saved_entry}" +fi + +search --no-floppy --set=root --label root +set boot=${root} +function load_video { + insmod all_video +} + +blscfg +"""), + # custom + ({ + "default": "0", + "timeout": 10, + "terminal_input": ["console"], + "terminal_output": ["serial"], + "serial": "serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1", + }, "", """GRUB_CMDLINE_LINUX="" +GRUB_TIMEOUT=10 +GRUB_ENABLE_BLSCFG=true +GRUB_SERIAL_COMMAND="serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1" +GRUB_TERMINAL_INPUT="console" +GRUB_TERMINAL_OUTPUT="serial" +GRUB_DEFAULT=0 +""", + """ +set timeout=10 + +# load the grubenv file +load_env + +# selection of the next boot entry via variables 'next_entry' and +# `saved_entry` present in the 'grubenv' file. Both variables are +# set by grub tools, like grub2-reboot, grub2-set-default + +if [ "${next_entry}" ] ; then + set default="${next_entry}" + set next_entry= + save_env next_entry + set boot_once=true +else + set default="${saved_entry}" +fi + +search --no-floppy --set=root --label root +set boot=${root} +function load_video { + insmod all_video +} + +serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1 +terminal_input console +terminal_output serial +blscfg +"""), + # custom (Azure) + ({ + "default": "saved", + "disable_submenu": True, + "disable_recovery": True, + "distributor": "$(sed 's, release .*$,,g' /etc/system-release)", + "serial": "serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1", + "terminal": ["serial", "console"], + "terminal_output": ["console"], + "timeout": 10, + "timeout_style": "countdown", + }, + "loglevel=3 crashkernel=auto console=tty1 console=ttyS0 earlyprintk=ttyS0 rootdelay=300", + """GRUB_CMDLINE_LINUX="loglevel=3 crashkernel=auto console=tty1 console=ttyS0 earlyprintk=ttyS0 rootdelay=300" +GRUB_TIMEOUT=10 +GRUB_ENABLE_BLSCFG=true +GRUB_DISABLE_RECOVERY=true +GRUB_DISABLE_SUBMENU=true +GRUB_DISTRIBUTOR="$(sed 's, release .*$,,g' /etc/system-release)" +GRUB_SERIAL_COMMAND="serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1" +GRUB_TERMINAL="serial console" +GRUB_TERMINAL_OUTPUT="console" +GRUB_TIMEOUT_STYLE=countdown +GRUB_DEFAULT=saved +""", + """ +set timeout=10 + +# load the grubenv file +load_env + +# selection of the next boot entry via variables 'next_entry' and +# `saved_entry` present in the 'grubenv' file. Both variables are +# set by grub tools, like grub2-reboot, grub2-set-default + +if [ "${next_entry}" ] ; then + set default="${next_entry}" + set next_entry= + save_env next_entry + set boot_once=true +else + set default="${saved_entry}" +fi + +search --no-floppy --set=root --label root +set boot=${root} +function load_video { + insmod all_video +} + +serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1 +terminal_output console +blscfg +"""), +]) +def test_grub2_default_conf_v2(tmp_path, stage_module, test_data, kernel_opts, expected_conf, expected_boot_conf): + treedir = tmp_path / "tree" + confpath = treedir / "etc/default/grub" + confpath.parent.mkdir(parents=True, exist_ok=True) + bootconfpath = treedir / "boot/grub2/grub.cfg" + + options = { + "compat_version": 2, + "rootfs": { + "label": "root" + }, + "entries": [ + { + "id": "fff", + "kernel": "4.18", + "product": { + "name": "Fedora", + "version": "40" + } + } + ], + } + + options["config"] = test_data + options["kernel_opts"] = kernel_opts + stage_module.main(treedir, options) + + assert os.path.exists(confpath) + assert confpath.read_text() == expected_conf + + assert os.path.exists(bootconfpath) + assert bootconfpath.read_text() == expected_boot_conf