Repository: osbuild/osbuild
Pull request #1451 (2023-11-15 15:18:48)

stages(kickstart): add `network` support to kickstart
This PR adds `network`support to the kickstart stage [0].

It is incomplete, i.e. it does not implement all options of [0]. However it is implementing all the pieces that are used in https://github.com/RedHatInsights/edge-api/blob/main/templates/templateKickstart.ks#L6 and everything that looks common.

The network options for kickstart are quite long, so I the following subset:
- bindto
- nodefroute
- nodns
- ethtool
- wepkey
- dhcpclass
- bond{opts,slaves}
- vlanid
- interfacename
- team{slaves,config}
- bridge{slave,opts}

Please let me know (especially people with deep kickstart knowledge like @bcl) if that list is sensible. It it easy enough to add more in future and also easy enough to just add them all.


[0] https://docs.fedoraproject.org/en-US/fedora/f36/install-guide/appendixes/Kickstart_Syntax_Reference/#sect-kickstart-commands-network

supakeen: I would prefer only supporting networks as a list, that there could be a single item in that list is up to the user.
supakeen: So, this is a huge explosion of new kickstart stage options; I'm generally OK with the approach taken here and it *could* be merged as is.

Personally I'd really like to have some more stringent patterns on those things that are IPv(4|6) addresses as far as that's possible (I know libc can be interesting with what it interprets as an IP).
mvo5: > So, this is a huge explosion of new kickstart stage options; I'm generally OK with the approach taken here and it _could_ be merged as is.
> 
> Personally I'd really like to have some more stringent patterns on those things that are IPv(4|6) addresses as far as that's possible (I know libc can be interesting with what it interprets as an IP).

Thank you for the initial review - I added more validations now, please let me know if we need more or if I'm overdoing it now .) I do not validate:
- ipv6 because the regex for this is 660 chars long and the resulting error if validation fails will not please our users
- netmask 



Patch:
diff --git a/stages/org.osbuild.kickstart b/stages/org.osbuild.kickstart
index 22434350..9d5f5372 100755
--- a/stages/org.osbuild.kickstart
+++ b/stages/org.osbuild.kickstart
@@ -17,6 +17,80 @@ from typing import Dict, List
 import osbuild.api
 
 SCHEMA = r"""
+"definitions": {
+  "ksboolean-from-pykickstart-ref": {
+    "enum": ["yes", "on", "true", "1", 1, "off", "no", "false", "0", 0]
+  },
+  "network-options-ref": {
+    "type": "object",
+    "additionalProperties": false,
+    "description": "Configures network information for the target system and activates network devices in the installation environment",
+    "required": ["device"],
+    "properties": {
+      "activate": {
+        "description": "Activate (or reactivate) the device. When set to false prevents the device from automatically activating on boot",
+        "type": "boolean"
+      },
+      "bootproto": {
+        "description": "Boot protocol selection",
+        "enum": ["dhcp", "bootp", "ibft", "static"]
+      },
+      "device": {
+        "description": "Specifies the device to be configured",
+        "type": "string",
+        "pattern": "^([a-zA-Z0-9]{2,16}|(([a-fA-F0-9]{2}:){5}[a-fA-F0-9A]{2}))$"
+      },
+      "onboot": {
+        "description": "Whether or not to enable the device at boot time",
+        "$ref": "#/definitions/ksboolean-from-pykickstart-ref"
+      },
+      "ip": {
+        "description": "IP address of the device",
+        "type": "string",
+        "pattern": "^(((25[0-5]|(2[0-4]|1\\d|[1-9]|)\\d)\\.?\\b){4}|auto)$"
+      },
+      "ipv6": {
+        "description": "IPv6 address of the device, in the form of address[/prefix length]",
+        "type": "string"
+      },
+      "gateway": {
+        "description": "Default gateway as a single IPv4 address",
+        "type": "string",
+        "pattern": "^((25[0-5]|(2[0-4]|1\\d|[1-9]|)\\d)\\.?\\b){4}$"
+      },
+      "ipv6gateway": {
+        "description": "Default gateway as a single IPv6 address",
+        "type": "string"
+      },
+      "nameservers": {
+        "description": "DNS name servesr, a list of IP addresses",
+        "type": "array",
+        "minSize": 1,
+        "items": {
+          "type": "string",
+          "pattern": "^((25[0-5]|(2[0-4]|1\\d|[1-9]|)\\d)\\.?\\b){4}$"
+        }
+      },
+      "netmask": {
+        "description": "Network mask for the installed system",
+        "type": "string"
+      },
+      "hostname": {
+        "description": "Host name for the installed system",
+        "type": "string",
+        "pattern": "(?=^.{1,253}$)^[a-zA-Z0-9][a-zA-Z0-9-]{0,62}((\\.[a-zA-Z0-9][a-zA-Z0-9-]{0,62})+)?$"
+      },
+      "essid": {
+        "description": "The network ID for wireless networks",
+        "type": "string"
+      },
+      "wpakey": {
+        "description": "The WPA encryption key for wireless networks",
+        "type": "string"
+      }
+    }
+  }
+},
 "additionalProperties": false,
 "required": ["path"],
 "properties": {
@@ -262,6 +336,13 @@ SCHEMA = r"""
         "type": "integer"
       }
     }
+  },
+  "network": {
+    "type": "array",
+    "minItems": 1,
+    "items": {
+      "$ref": "#/definitions/network-options-ref"
+    }
   }
 }
 """
@@ -382,6 +463,38 @@ def make_autopart(options: Dict) -> str:
     return cmd
 
 
+def make_network(options: Dict) -> List[str]:
+    networks = options.get("network")
+    if networks is None:
+        return []
+    res = []
+    for net in networks:
+        cmd = "network"
+        # simple string options
+        for key in [
+                "device", "bootproto", "onboot", "ip", "ipv6", "gateway",
+                "ipv6gateway", "netmask", "hostname", "essid", "wpakey",
+        ]:
+            if key not in net:
+                continue
+            val = net.get(key)
+            cmd += f" --{key}={val}"
+        # nameservers is a list
+        nameservers = net.get("nameservers")
+        if nameservers:
+            nss = " ".join([f"--nameserver={ns}" for ns in nameservers])
+            cmd += f" {nss}"
+        # activate can be true/false/unset
+        activate = net.get("activate")
+        if isinstance(activate, bool):
+            if activate:
+                cmd += " --activate"
+            else:
+                cmd += " --no-activate"
+        res += [cmd]
+    return res
+
+
 def main(tree, options):  # pylint: disable=too-many-branches
     path = options["path"].lstrip("/")
     ostree = options.get("ostree")
@@ -433,6 +546,7 @@ def main(tree, options):  # pylint: disable=too-many-branches
     reboot = make_reboot(options)
     if reboot:
         config += [reboot]
+    config += make_network(options)
 
     target = os.path.join(tree, path)
     base = os.path.dirname(target)


Test patch:
diff --git a/stages/test/test_kickstart.py b/stages/test/test_kickstart.py
index 6accce71..8428a266 100644
--- a/stages/test/test_kickstart.py
+++ b/stages/test/test_kickstart.py
@@ -157,14 +157,49 @@ TEST_INPUT = [
         " --backuppassphrase --cipher=twofish-cbc --luks-version=2" +
         " --pbkdf=scrypt --pbkdf-memory=256 --pbkdf-time=512"
     ),
+    # network is always a list
+    ({"network": [{"device": "foo", "activate": False}, {"device": "bar", "bootproto": "dhcp"}]},
+     "network --device=foo --no-activate\nnetwork --device=bar --bootproto=dhcp"),
+    ({"network": [{"device": "foo", "activate": True}]}, "network --device=foo --activate"),
+    ({"network": [{"device": "foo", "activate": False}]}, "network --device=foo --no-activate"),
+    ({"network": [{"device": "foo", "bootproto": "dhcp"}]}, "network --device=foo --bootproto=dhcp"),
+    ({"network": [{"device": "foo", "onboot": "on"}]}, "network --device=foo --onboot=on"),
+    ({"network": [{"device": "foo", "ip": "10.0.0.2"}]}, "network --device=foo --ip=10.0.0.2"),
+    ({"network": [{"device": "foo", "ip": "auto"}]}, "network --device=foo --ip=auto"),
+    ({"network": [{"device": "foo", "ipv6": "3ffe:ffff:0:1::1/128"}]},
+     "network --device=foo --ipv6=3ffe:ffff:0:1::1/128"),
+    ({"network": [{"device": "foo", "ipv6": "dhcp"}]}, "network --device=foo --ipv6=dhcp"),
+    ({"network": [{"device": "foo", "gateway": "10.0.0.1"}]}, "network --device=foo --gateway=10.0.0.1"),
+    ({"network": [{"device": "foo", "ipv6gateway": "FE80::1"}]}, "network --device=foo --ipv6gateway=FE80::1"),
+    ({"network": [{"device": "foo", "nameservers": ["1.1.1.1"]}]}, "network --device=foo --nameserver=1.1.1.1"),
+    ({"network": [{"device": "foo", "nameservers": ["1.1.1.1", "8.8.8.8"]}]},
+     "network --device=foo --nameserver=1.1.1.1 --nameserver=8.8.8.8"),
+    ({"network": [{"device": "foo", "netmask": "255.255.0.0"}]}, "network --device=foo --netmask=255.255.0.0"),
+    ({"network": [{"device": "foo", "hostname": "meep"}]}, "network --device=foo --hostname=meep"),
+    ({"network": [{"device": "foo", "essid": "wlan-123"}]}, "network --device=foo --essid=wlan-123"),
+    ({"network": [{"device": "foo", "wpakey": "secret"}]}, "network --device=foo --wpakey=secret"),
+    # device= can be written in multiple forms, see
+    # https://pykickstart.readthedocs.io/en/latest/kickstart-docs.html#network
+    ({"network": [{"device": "1234567890123456"}]}, "network --device=1234567890123456"),
+    ({"network": [{"device": "em1"}]}, "network --device=em1"),
+    ({"network": [{"device": "01:23:45:67:89:ab"}]}, "network --device=01:23:45:67:89:ab"),
+    ({"network": [{"device": "link"}]}, "network --device=link"),
+    ({"network": [{"device": "bootif"}]}, "network --device=bootif"),
+    # hostname alone can be 63 chars (see hostname(7))
+    ({"network": [{"device": "foo", "hostname": "123456789012345678901234567890123456789012345678901234567890123"}]},
+     "network --device=foo --hostname=123456789012345678901234567890123456789012345678901234567890123"),
+    # hostname can also be written as a QFDN
+    ({"network": [{"device": "foo", "hostname": "foo.bar.com"}]},
+     "network --device=foo --hostname=foo.bar.com"),
 ]
 
 
 def schema_validate_kickstart_stage(test_data):
     name = "org.osbuild.kickstart"
+    version = "1"
     root = os.path.join(os.path.dirname(__file__), "../..")
     mod_info = osbuild.meta.ModuleInfo.load(root, "Stage", name)
-    schema = osbuild.meta.Schema(mod_info.get_schema(), name)
+    schema = osbuild.meta.Schema(mod_info.get_schema(version=version), name)
     test_input = {
         "name": "org.osbuild.kickstart",
         "options": {
@@ -240,6 +275,49 @@ def test_kickstart_valid(tmp_path, test_input, expected):  # pylint: disable=unu
         ({"autopart": {"type": "not-valid"}}, "'not-valid' is not one of ["),
         # Only one of --pbkdf-{time,iterations} can be specified at the same time
         ({"autopart": {"pbkdf-time": 1, "pbkdf-iterations": 2}}, " should not be valid under "),
+        # network is always a list
+        ({"network": {"device": "foo"}}, " is not of type 'array'"),
+        ({"network": [{"device": "foo", "activate": "string"}]}, " is not of type 'boolean'"),
+        ({"network": [{"device": "foo", "random": "option"}]}, "Additional properties are not allowed "),
+        ({"network": [{"device": "foo", "bootproto": "invalid"}]}, " is not one of ["),
+        ({"network": [{"device": "foo", "ip": "invalid"}]}, " does not match "),
+        ({"network": [{"device": "foo", "ip": "256.1.1.1"}]}, " does not match "),
+        ({"network": [{"device": "foo", "ip": "1.256.1.1"}]}, " does not match "),
+        ({"network": [{"device": "foo", "ip": "1.1.256.1"}]}, " does not match "),
+        ({"network": [{"device": "foo", "ip": "1.1.1.256"}]}, " does not match "),
+        # kernel will accept this (and make it 127.0.0.1) but it's
+        # technically not valid. if this becomes a problem we may need to
+        # relax (or remove) the ipv4 validation regex. Also
+        # 127.256 will be accepted and overflows into "127.0.1.0".
+        ({"network": [{"device": "foo", "ip": "127.1"}]}, " does not match "),
+        ({"network": [{"device": "foo", "gateway": "invalid"}]}, " does not match "),
+        ({"network": [{"device": "foo", "nameservers": ["invalid"]}]}, " does not match "),
+        # schema says at least 2 chars (this is arbitrary)
+        ({"network": [{"device": "f"}]}, " does not match "),
+        # device name can be max 16 chars (see IFNAMSIZ in the kernel source)
+        # but otherwise are very free from, see
+        # https://elixir.bootlin.com/linux/v6.6.1/source/net/core/dev.c#L1038
+        ({"network": [{"device": "12345678901234567"}]}, " does not match "),
+        # when specificed via MAC address it 17 chars (12 chars plus 5 ":")
+        # and look like a mac address
+        ({"network": [{"device": "00:01"}]}, " does not match "),
+        ({"network": [{"device": "00:XX"}]}, " does not match "),
+        ({"network": [{"device": "foo/bar"}]}, " does not match "),
+        ({"network": [{"device": "foo?"}]}, " does not match "),
+        # see hostname(7)
+        ({"network": [{"device": "foo",
+                       "hostname": "1234567890123456789012345678901234567890123456789012345678901234"}]}, " does not match "),
+        ({"network": [{"device": "foo", "hostname": "x$"}]}, " does not match "),
+        ({"network": [{"device": "foo", "hostname": "-invalid"}]}, " does not match "),
+        ({"network": [{"device": "foo", "hostname": "foo..bar"}]}, " does not match "),
+        # not more than 253 chars (63*3 + 62 + 3 dots = 254)
+        ({"network": [{"device": "foo",
+                       "hostname":
+                       "123456789012345678901234567890123456789012345678901234567890123" +
+                       ".123456789012345678901234567890123456789012345678901234567890123" +
+                       ".123456789012345678901234567890123456789012345678901234567890123" +
+                       ".12345678901234567890123456789012345678901234567890123456789012"
+                       }]}, " does not match "),
     ],
 )
 def test_schema_validation_bad_apples(test_data, expected_err):
diff --git a/stages/test/test_ostree_post_copy.py b/stages/test/test_ostree_post_copy.py
new file mode 100644
index 00000000..2c349d02
--- /dev/null
+++ b/stages/test/test_ostree_post_copy.py
@@ -0,0 +1,61 @@
+#!/usr/bin/python3
+
+import os.path
+from unittest.mock import call, patch
+
+import pytest
+
+import osbuild.meta
+from osbuild.testutil.imports import import_module_from_path
+
+
+def schema_validate_stage_ostree_post_copy(test_data):
+    name = "org.osbuild.ostree.post-copy"
+    version = "2"
+    root = os.path.join(os.path.dirname(__file__), "../..")
+    mod_info = osbuild.meta.ModuleInfo.load(root, "Stage", name)
+    schema = osbuild.meta.Schema(mod_info.get_schema(version=version), name)
+    test_input = {
+        "type": "org.osbuild.ostree.post-copy",
+        "options": {
+            "sysroot": "/some/sysroot",
+        },
+    }
+    test_input.update(test_data)
+    return schema.validate(test_input)
+
+
+@patch("osbuild.util.ostree.cli")
+def test_ostree_post_copy_smoke(mock_ostree_cli):
+    stage_path = os.path.join(os.path.dirname(__file__), "../org.osbuild.ostree.post-copy")
+    stage = import_module_from_path("stage", stage_path)
+
+    paths = {
+        "mounts": "/run/osbuild/mounts",
+    }
+    options = {
+        "sysroot": "/some/sysroot",
+    }
+    stage.main(paths, options)
+
+    assert mock_ostree_cli.call_args_list == [
+        call("admin", "post-copy", sysroot="/run/osbuild/mounts/some/sysroot")]
+
+
+@pytest.mark.parametrize(
+    "test_data,expected_err",
+    [
+        # devices is not used directly in the stage but it's required
+        # because it's used as input for "mounts", see
+        # https://github.com/osbuild/osbuild/pull/1343/files#r1402161208
+        ({"devices": "must-be-object"}, " is not of type 'object'"),
+        ({"mounts": "must-be-array"}, " is not of type 'array'"),
+    ],
+)
+def test_schema_validation_ostree_post_copy(test_data, expected_err):
+    res = schema_validate_stage_ostree_post_copy(test_data)
+
+    assert res.valid is False
+    err_msgs = [e.as_dict()["message"] for e in res.errors]
+    assert len(res.errors) == 1, err_msgs
+    assert expected_err in err_msgs[0]