Repository: osbuild/osbuild
Pull request #1781 (2024-05-13 07:34:40)

test: tweak test_osbuild_mount_failure_msg() for tmt/fc40
The `test_osbuild_mount_failure_msg` currently fails on fc40 when run in tmt, see:
https://artifacts.dev.testing-farm.io/c6588a82-a2cb-46df-8ca8-85dd809465f2/

This is because the failure output is slightly different between a container and a VM/real-machine. The test ensures that we capture the output of mount and present to the user (for easier debugging). So this commit updates this test once more for the error string (that part of the error comes directly from the kernels fsconfig).

If we need another update of the string we should reconsider this test and e.g. just use `testutil.mock_command()` for this. But for now it's easier to just add this one more failure string.

mvo5: Sorry, had to re-push to fix a trivial linter failure (line too long)

Patch:
diff --git a/osbuild.spec b/osbuild.spec
index f6425449..eba33105 100644
--- a/osbuild.spec
+++ b/osbuild.spec
@@ -1,7 +1,7 @@
 %global         forgeurl https://github.com/osbuild/osbuild
 %global         selinuxtype targeted
 
-Version:        119
+Version:        120
 
 %forgemeta
 
diff --git a/osbuild/__init__.py b/osbuild/__init__.py
index e62ed348..ad34a664 100644
--- a/osbuild/__init__.py
+++ b/osbuild/__init__.py
@@ -10,7 +10,7 @@ independent of osbuild but used across the osbuild codebase.
 
 from .pipeline import Manifest, Pipeline, Stage
 
-__version__ = "119"
+__version__ = "120"
 
 __all__ = [
     "Manifest",
diff --git a/osbuild/util/runners.py b/osbuild/util/runners.py
index ec2a3fec..99f5de91 100644
--- a/osbuild/util/runners.py
+++ b/osbuild/util/runners.py
@@ -62,10 +62,6 @@ def tmpfiles():
     subprocess.run(["systemd-tmpfiles", "--create"], check=False)
 
 
-def remove_tmpfiles():
-    subprocess.run(["systemd-tmpfiles", "--clean", "--remove"], check=False)
-
-
 def nsswitch():
     # the default behavior is fine, but using nss-resolve does not
     # necessarily work in a non-booted container, so make sure that
@@ -94,8 +90,7 @@ def sequoia():
     # images).
     os.makedirs("/etc/crypto-policies", exist_ok=True)
     shutil.copytree(
-        "/usr/share/crypto-policies/back-ends/DEFAULT",
-        "/etc/crypto-policies/back-ends"
+        "/usr/share/crypto-policies/back-ends/DEFAULT", "/etc/crypto-policies/back-ends"
     )
 
 
diff --git a/runners/org.osbuild.centos9 b/runners/org.osbuild.centos9
index b9a6c397..3e0f60f0 100755
--- a/runners/org.osbuild.centos9
+++ b/runners/org.osbuild.centos9
@@ -14,6 +14,5 @@ if __name__ == "__main__":
             runners.tmpfiles()
             runners.nsswitch()
             r = subprocess.run(sys.argv[1:], check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/runners/org.osbuild.fedora30 b/runners/org.osbuild.fedora30
index b9a6c397..3e0f60f0 100755
--- a/runners/org.osbuild.fedora30
+++ b/runners/org.osbuild.fedora30
@@ -14,6 +14,5 @@ if __name__ == "__main__":
             runners.tmpfiles()
             runners.nsswitch()
             r = subprocess.run(sys.argv[1:], check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/runners/org.osbuild.fedora38 b/runners/org.osbuild.fedora38
index 45a7c245..2d0b8198 100755
--- a/runners/org.osbuild.fedora38
+++ b/runners/org.osbuild.fedora38
@@ -15,6 +15,5 @@ if __name__ == "__main__":
             runners.nsswitch()
             runners.sequoia()
             r = subprocess.run(sys.argv[1:], check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/runners/org.osbuild.rhel81 b/runners/org.osbuild.rhel81
index 9a911dea..db7fe78c 100755
--- a/runners/org.osbuild.rhel81
+++ b/runners/org.osbuild.rhel81
@@ -41,6 +41,5 @@ if __name__ == "__main__":
             os_release()
             runners.python_alternatives()
             r = subprocess.run(sys.argv[1:], check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/runners/org.osbuild.rhel82 b/runners/org.osbuild.rhel82
index 1c14e9da..101c5771 100755
--- a/runners/org.osbuild.rhel82
+++ b/runners/org.osbuild.rhel82
@@ -16,6 +16,5 @@ if __name__ == "__main__":
             runners.python_alternatives()
             env = runners.quirks()
             r = subprocess.run(sys.argv[1:], env=env, check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/setup.py b/setup.py
index 3323362a..07813336 100644
--- a/setup.py
+++ b/setup.py
@@ -2,7 +2,7 @@ import setuptools
 
 setuptools.setup(
     name="osbuild",
-    version="119",
+    version="120",
     description="A build system for OS images",
     packages=["osbuild", "osbuild.formats", "osbuild.util"],
     license='Apache-2.0',
diff --git a/stages/org.osbuild.systemd.unit.create b/stages/org.osbuild.systemd.unit.create
index f0b61016..2c367f3f 100755
--- a/stages/org.osbuild.systemd.unit.create
+++ b/stages/org.osbuild.systemd.unit.create
@@ -5,13 +5,24 @@ import sys
 import osbuild.api
 
 
-def main(tree, options):
-    filename = options["filename"]
+def validate(filename, cfg):
     # ensure the service name does not exceed maximum filename length
     if len(filename) > 255:
         raise ValueError(f"Error: the {filename} service exceeds the maximum filename length.")
 
+    # Filename extension must match the config:
+    #   .service requires a Service section
+    #   .mount requires a Mount section
+    if filename.endswith(".service") and "Service" not in cfg:
+        raise ValueError(f"Error: {filename} unit requires Service section")
+    if filename.endswith(".mount") and "Mount" not in cfg:
+        raise ValueError(f"Error: {filename} unit requires Mount section")
+
+
+def main(tree, options):
+    filename = options["filename"]
     cfg = options["config"]
+    validate(filename, cfg)
 
     # We trick configparser into letting us write multiple instances of the same option by writing them as keys with no
     # value, so we enable allow_no_value
diff --git a/stages/org.osbuild.systemd.unit.create.meta.json b/stages/org.osbuild.systemd.unit.create.meta.json
index 05431429..2446a8c0 100644
--- a/stages/org.osbuild.systemd.unit.create.meta.json
+++ b/stages/org.osbuild.systemd.unit.create.meta.json
@@ -1,13 +1,13 @@
 {
-  "summary": "Create Systemd services unit file",
+  "summary": "Create a Systemd unit file",
   "description": [
-    "This stage allows to create Systemd unit files in",
-    "`/usr/lib/systemd/system/`. The `filename` property specifies the",
-    "'.service' file to be added. These names are validated using the",
-    "same rules as specified by systemd.unit(5) and they must contain the",
-    "'.service' suffix (other types of unit files are not supported).",
-    "'unit-type' determines if the unit file is created at global or",
-    "system level,'unit-path' will determine the systemd unit load path.",
+    "This stage allows to create Systemd unit files. The `filename` property",
+    "specifies the, '.service' or '.mount' file to be added. These names are",
+    "validated using the, same rules as specified by systemd.unit(5) and they",
+    "must contain the, '.service' or '.mount' suffix (other types of unit files",
+    "are not supported). 'unit-type' determines if the unit file is created at",
+    "'global' (user) or 'system' paths,'unit-path' will determine the systemd",
+    "unit load path.",
     "",
     "The Unit configuration can currently specify the following subset",
     "of options:",
@@ -28,6 +28,11 @@
     "    - 'ExecStart' - [string]",
     "    - 'Environment' - [object]",
     "    - 'EnvironmentFile' - [string]",
+    "  - 'Mount' section",
+    "    - 'What' - string",
+    "    - 'Where' - string",
+    "    - 'Type' - string",
+    "    - 'Options' - string",
     "  - 'Install' section",
     "    - 'WantedBy' - [string]",
     "    - 'RequiredBy' - [string]"
@@ -41,7 +46,7 @@
     "properties": {
       "filename": {
         "type": "string",
-        "pattern": "^[\\w:.\\\\-]+[@]{0,1}[\\w:.\\\\-]*\\.service$"
+        "pattern": "^[\\w:.\\\\-]+[@]{0,1}[\\w:.\\\\-]*\\.(service|mount)$"
       },
       "unit-type": {
         "type": "string",
@@ -64,10 +69,29 @@
       "config": {
         "additionalProperties": false,
         "type": "object",
-        "required": [
-          "Unit",
-          "Service",
-          "Install"
+        "oneOf": [
+          {
+            "required": [
+              "Unit",
+              "Service",
+              "Install"
+            ],
+            "not": {
+              "required": [
+                "Mount"
+              ]
+            }
+          },
+          {
+            "required": [
+              "Mount"
+            ],
+            "not": {
+              "required": [
+                "Service"
+              ]
+            }
+          }
         ],
         "description": "Configuration for a '.service' unit.",
         "properties": {
@@ -186,6 +210,32 @@
               }
             }
           },
+          "Mount": {
+            "additionalProperties": false,
+            "type": "object",
+            "description": "'Mount' configuration section of a unit file.",
+            "required": [
+              "What"
+            ],
+            "properties": {
+              "What": {
+                "description": "Absolute path to device node",
+                "type": "string"
+              },
+              "Where": {
+                "description": "Absolute path for a file or directory for the mountpoint",
+                "type": "string"
+              },
+              "Type": {
+                "descriptions": "File system type",
+                "type": "string"
+              },
+              "Options": {
+                "descriptions": "Mount options to use when mounting",
+                "type": "string"
+              }
+            }
+          },
           "Install": {
             "additionalProperties": false,
             "type": "object",
diff --git a/stages/org.osbuild.users b/stages/org.osbuild.users
index 67722a5d..08fbf287 100755
--- a/stages/org.osbuild.users
+++ b/stages/org.osbuild.users
@@ -110,6 +110,7 @@ def main(tree, options):
         shell = user_options.get("shell")
         password = user_options.get("password")
         expiredate = user_options.get("expiredate")
+        force_password_reset = user_options.get("force_password_reset", False)
 
         passwd = getpwnam(tree, name)
         if passwd is not None:
@@ -125,6 +126,9 @@ def main(tree, options):
         else:
             useradd(tree, name, uid, gid, groups, description, home, shell, password, expiredate)
 
+        if force_password_reset:
+            subprocess.run(["chroot", tree, "passwd", "--expire", name], check=True)
+
         # following maintains backwards compatibility for handling a single ssh key
         key = user_options.get("key")   # Public SSH key
         keys = user_options.get("keys", [])  # Additional public SSH keys
diff --git a/stages/org.osbuild.users.meta.json b/stages/org.osbuild.users.meta.json
index 6c8668a6..22df5dee 100644
--- a/stages/org.osbuild.users.meta.json
+++ b/stages/org.osbuild.users.meta.json
@@ -66,6 +66,10 @@
                 "expiredate": {
                   "description": "The date on which the user account will be disabled. This date is represented as a number of days since January 1st, 1970.",
                   "type": "integer"
+                },
+                "force_password_reset": {
+                  "description": "Force this user to change their password on first login",
+                  "type": "boolean"
                 }
               }
             }


Test patch:
diff --git a/stages/test/test_systemd_unit_create.py b/stages/test/test_systemd_unit_create.py
index 35e27a79..a748e2f0 100644
--- a/stages/test/test_systemd_unit_create.py
+++ b/stages/test/test_systemd_unit_create.py
@@ -21,16 +21,42 @@ STAGE_NAME = "org.osbuild.systemd.unit.create"
                 "Service": {},
                 "Install": {},
             },
-        }, ""),
+        },
+        "",
+    ),
+    (
+        {
+            "filename": "foo.mount",
+            "config": {
+                "Unit": {},
+                "Mount": {"What": ""},
+                "Install": {},
+            },
+        },
+        "",
+    ),
+    (
+        {
+            "filename": "foo.mount",
+            "config": {
+                "Mount": {"What": ""},
+            },
+        },
+        "",
+    ),
     # bad
     ({"config": {"Unit": {}, "Service": {}, "Install": {}}}, "'filename' is a required property"),
     ({"filename": "foo.service"}, "'config' is a required property"),
     ({"filename": "foo.service", "config": {"Service": {}, "Install": {}}},
-     "'Unit' is a required property"),
+     "{'Service': {}, 'Install': {}} is not valid under any of the given schemas"),
     ({"filename": "foo.service", "config": {"Unit": {}, "Install": {}}},
-     "'Service' is a required property"),
+     "{'Unit': {}, 'Install': {}} is not valid under any of the given schemas"),
     ({"filename": "foo.service", "config": {"Unit": {}, "Service": {}}},
-     "'Install' is a required property"),
+     "{'Unit': {}, 'Service': {}} is not valid under any of the given schemas"),
+    ({"filename": "foo.service", "config": {"Service": {}, "Mount": {"What": ""}}},
+     "{'Service': {}, 'Mount': {'What': ''}} is not valid under any of the given schemas"),
+    ({"filename": "foo.service", "config": {"Unit": {}, "Service": {}, "Mount": {"What": ""}, "Install": {}}},
+     "{'Unit': {}, 'Service': {}, 'Mount': {'What': ''}, 'Install': {}} is not valid under any of the given schemas"),
 ])
 @pytest.mark.parametrize("stage_schema", ["1"], indirect=True)
 def test_schema_validation(stage_schema, test_data, expected_err):
@@ -46,6 +72,54 @@ def test_schema_validation(stage_schema, test_data, expected_err):
         testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
+@pytest.mark.parametrize("test_data,expected_err", [
+    # good
+    (
+        {
+            "filename": "foo.service",
+            "config": {
+                "Unit": {},
+                "Service": {},
+                "Install": {},
+            },
+        },
+        "",
+    ),
+    (
+        {
+            "filename": "foo.mount",
+            "config": {
+                "Unit": {},
+                "Mount": {"What": ""},
+                "Install": {},
+            },
+        },
+        "",
+    ),
+    (
+        {
+            "filename": "foo.mount",
+            "config": {
+                "Mount": {"What": ""},
+            },
+        },
+        "",
+    ),
+    # bad
+    ({"filename": "something.service", "config": {"Unit": {}, "Mount": {}, "Install": {}}},
+     "Error: something.service unit requires Service section"),
+    ({"filename": "data-gifs-cats.mount", "config": {"Unit": {}, "Service": {}, "Install": {}}},
+     "Error: data-gifs-cats.mount unit requires Mount section"),
+])
+def test_name_config_match(tmp_path, stage_module, test_data, expected_err):
+    expected_unit_path = tmp_path / "usr/lib/systemd/system"
+    expected_unit_path.mkdir(parents=True, exist_ok=True)
+    try:
+        stage_module.main(tmp_path, test_data)
+    except ValueError as ve:
+        assert expected_err == str(ve)
+
+
 @pytest.mark.parametrize("unit_type,unit_path,expected_prefix", [
     ("system", "usr", "usr/lib/systemd/system"),
     ("system", "etc", "etc/systemd/system"),
@@ -174,3 +248,80 @@ def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expec
     RequiredBy=multi-user.target
 
     """)
+
+
+@pytest.mark.parametrize("unit_type,unit_path,expected_prefix", [
+    ("system", "usr", "usr/lib/systemd/system"),
+    ("system", "etc", "etc/systemd/system"),
+    ("global", "usr", "usr/lib/systemd/user"),
+    ("global", "etc", "etc/systemd/user"),
+])
+def test_systemd_unit_create_mount(tmp_path, stage_module, unit_type, unit_path, expected_prefix):
+    options = {
+        "filename": "data-test.mount",
+        "unit-type": unit_type,
+        "unit-path": unit_path,
+        "config": {
+            "Unit": {
+                "DefaultDependencies": True,
+                "ConditionPathExists": [
+                    "/data"
+                ],
+                "ConditionPathIsDirectory": [
+                    "/data"
+                ],
+                "Before": [
+                    "local-fs.target",
+                ],
+            },
+            "Mount": {
+                "What": "/dev/sda2",
+                "Where": "/data/test",
+                "Type": "xfs",
+                "Options": "rw,noatime"
+            }
+        }
+    }
+
+    # create units in the tree for systemd-analyze verify
+    systemd_dir = "usr/lib/systemd/system/"
+    # if a unit file is empty, it gets detected as "masked" and fails verification
+    testutil.make_fake_tree(tmp_path, {
+        systemd_dir + "basic.target": "[Unit]\n",
+        systemd_dir + "sysinit.target": "[Unit]\n",
+        systemd_dir + "paths.target": "[Unit]\n",
+        systemd_dir + "sockets.target": "[Unit]\n",
+        "usr/bin/mkdir": "fake-mkdir",
+        "usr/bin/touch": "fake-touch",
+    })
+    for bin_name in ["mkdir", "touch"]:
+        os.chmod(tmp_path / "usr/bin" / bin_name, mode=0o755)
+
+    expected_unit_path = tmp_path / expected_prefix / "data-test.mount"
+    expected_unit_path.parent.mkdir(parents=True, exist_ok=True)
+
+    stage_module.main(tmp_path, options)
+    assert os.path.exists(expected_unit_path)
+    if unit_type == "system":
+        # When verifying user units, systemd expects runtime directories and more generally a booted system. Setting
+        # something up like that to verify it is more trouble than it's worth, especially since the system units are
+        # identical.
+        subprocess.run(["systemd-analyze", "verify",
+                        "--man=no",  # disable manpage checks
+                        f"--root={tmp_path}",  # verify commands and units in the root tree
+                        expected_unit_path],
+                       check=True)
+    assert expected_unit_path.read_text(encoding="utf-8") == textwrap.dedent("""\
+    [Unit]
+    DefaultDependencies=True
+    ConditionPathExists=/data
+    ConditionPathIsDirectory=/data
+    Before=local-fs.target
+
+    [Mount]
+    What=/dev/sda2
+    Where=/data/test
+    Type=xfs
+    Options=rw,noatime
+
+    """)
diff --git a/stages/test/test_users.py b/stages/test/test_users.py
index c74cbe55..58b681e5 100644
--- a/stages/test/test_users.py
+++ b/stages/test/test_users.py
@@ -82,3 +82,70 @@ def test_users_mock_bin(tmp_path, stage_module, user_opts, expected_args):
         stage_module.main(tmp_path, options)
         assert len(mocked_chroot.call_args_list) == 1
         assert mocked_chroot.call_args_list[0][2:] == expected_args + ["foo"]
+
+
+# separate test right now as it results in two binaries being called
+# (adduser,passwd) which our parameter tests cannot do yet
+
+
+def test_users_with_password_reset_none(tmp_path, stage_module):
+    with mock_command("chroot", "") as mocked_chroot:
+        make_fake_tree(tmp_path, {
+            "/etc/passwd": "",
+        })
+
+        options = {
+            "users": {
+                "foo": {
+                },
+            }
+        }
+
+        stage_module.main(tmp_path, options)
+        assert len(mocked_chroot.call_args_list) == 1
+        assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"]
+
+# separate test right now as it results in two binaries being called
+# (adduser,passwd) which our parameter tests cannot do yet
+
+
+def test_users_with_password_reset_false(tmp_path, stage_module):
+    with mock_command("chroot", "") as mocked_chroot:
+        make_fake_tree(tmp_path, {
+            "/etc/passwd": "",
+        })
+
+        options = {
+            "users": {
+                "foo": {
+                    "force_password_reset": False,
+                },
+            }
+        }
+
+        stage_module.main(tmp_path, options)
+        assert len(mocked_chroot.call_args_list) == 1
+        assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"]
+
+# separate test right now as it results in two binaries being called
+# (adduser,passwd) which our parameter tests cannot do yet
+
+
+def test_users_with_password_reset_true(tmp_path, stage_module):
+    with mock_command("chroot", "") as mocked_chroot:
+        make_fake_tree(tmp_path, {
+            "/etc/passwd": "",
+        })
+
+        options = {
+            "users": {
+                "foo": {
+                    "force_password_reset": True,
+                },
+            }
+        }
+
+        stage_module.main(tmp_path, options)
+        assert len(mocked_chroot.call_args_list) == 2
+        assert mocked_chroot.call_args_list[0][1:] == ["useradd", "foo"]
+        assert mocked_chroot.call_args_list[1][1:] == ["passwd", "--expire", "foo"]
diff --git a/test/mod/test_util_mnt.py b/test/mod/test_util_mnt.py
index 259ac026..cfe7095f 100644
--- a/test/mod/test_util_mnt.py
+++ b/test/mod/test_util_mnt.py
@@ -57,4 +57,5 @@ def test_osbuild_mount_failure_msg(tmp_path):
             "options": [],
         }
         mnt_service.mount(args)
-    assert re.search(r"special device /dev/invalid-src does not exist|Can't open blockdev.", str(e.value))
+    assert re.search(
+        r"special device /dev/invalid-src does not exist|Can't open blockdev.|Can't lookup blockdev", str(e.value))