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

@@ -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)

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

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