Repository: osbuild/osbuild
Pull request #2104 (2025-06-05 14:49:34)

stages/grub2: set template vars to empty strings when missing (HMS-8646)
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.


achilleas-k: This is technically a stage behaviour change without an option change.  Which is bad.  We've let it slide for build-breaking bugs and this is technically a bug fix, but since it doesn't break the build or the image entirely, we should probably make the change visible, right?
achilleas-k: It breaks the caching and reproducibility guarantees.
That's all I got.
supakeen: I'll put a comment in this PR; when I previously toyed with versioning of stages I did it by adding a `vX` (so `v0`, `v1`, etc) to the name of the stage as it allows for large variations in schemas between those different versions.

With a `compat_version` field those might be a bit more difficult (but not impossible!) to implement while leading to much less duplication so I'm not against having this inside the input either.

Patch:
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"
       },


Test patch:
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