From 552ade702fc033f68cda998055bd5b74f195c7ac Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:56:33 +0200 Subject: Revert: T3871: make boot-time interface naming deterministic and hw-id aware This reverts commit 22cfee116c9935f4a33da4923eff189f2f646371. --- src/init/vyos-router | 27 --------------------------- 1 file changed, 27 deletions(-) (limited to 'src/init') diff --git a/src/init/vyos-router b/src/init/vyos-router index f109b95da..c0d12e2a7 100755 --- a/src/init/vyos-router +++ b/src/init/vyos-router @@ -362,26 +362,6 @@ update_interface_config () fi } -# warn (but do not fail boot) about configured hw-id interfaces whose -# hardware was not found by vyos-net-name-resolve.service -warn_missing_interface_hardware () -{ - local status_file="/run/vyos-net-name-resolve.json" - [ -f "${status_file}" ] || return 0 - - jq -r '.missing // {} | to_entries[] | "\(.value) \(.key)"' "${status_file}" \ - 2>/dev/null | while read -r intf hwid; do - [ -n "${intf}" ] || continue - log_failure_msg "hw-id ${hwid} configured as '${intf}' was not found during boot - check cabling, hardware and driver/firmware" - done - - jq -r '.pending_unresolved // [] | .[]' "${status_file}" 2>/dev/null | \ - while read -r intf; do - [ -n "${intf}" ] || continue - log_failure_msg "interface '${intf}' still has no hw-id configured after this boot's naming pass - bind it manually, e.g. 'set interfaces ethernet ${intf} hw-id ', or remove the node if it is no longer needed" - done -} - cleanup_post_commit_hooks () { # Remove links from the post-commit hooks directory. # note that this approach only supports hooks that are "configured", @@ -626,12 +606,6 @@ start () disabled migrate || migrate_bootfile || overall_status=1 - # config.boot is only guaranteed to exist/be current from this point on - # (tmpfs mount + encrypted config unlock + syntax migration have all - # already happened above) - hw-id based interface naming must be - # resolved against it before anything below applies interface config. - systemctl start vyos-net-name-resolve.service || overall_status=1 - restore_if_missing_bootup_script pre run_preconfig_script @@ -639,7 +613,6 @@ start () run_postupgrade_script update_interface_config || overall_status=1 - warn_missing_interface_hardware disabled system_activate || system_activate || overall_status=1 -- cgit v1.2.3 From c73faac7f95ec837da6f964d15cc05ff9ffd2de6 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 7 Oct 2026 19:56:37 +0200 Subject: interfaces: T3871: name interfaces from a slot-anchored mapping store Names used to be derived from 'hw-id' in config.boot. A node carrying an address but no hw-id had to be matched to some NIC, and nothing available at boot could say which one - every heuristic could bind a configured address to the wrong physical wire. Naming authority moves to a mapping store binding a name to the slot the hardware occupies. The slot is the identity, so a replacement card in that slot is that interface again. The store renders systemd .link files, leaving udev to apply names as devices appear. --- debian/control | 1 - python/vyos/defaults.py | 1 - python/vyos/ifconfig/ifname_store.py | 373 ++++++++++++++ .../rules.d/62-temporary-interface-rename.rules | 1 - src/etc/udev/rules.d/63-hyperv-vf-net.rules | 16 + src/etc/udev/rules.d/64-vyos-vmware-net.rules | 14 - src/etc/udev/rules.d/65-vyos-net.rules | 41 -- src/helpers/vyos-interface-rescan.py | 155 ++---- src/init/vyos-router | 37 +- src/system/vyos-net-name-resolve.py | 221 ++++++++ src/systemd/vyos-net-name-resolve.service | 15 + src/tests/helper.py | 11 + src/tests/test_ifname_store.py | 572 +++++++++++++++++++++ src/tests/test_net_name_resolve.py | 227 ++++++++ src/udev/vyos_net_name | 276 ---------- 15 files changed, 1518 insertions(+), 443 deletions(-) create mode 100644 python/vyos/ifconfig/ifname_store.py delete mode 100644 src/etc/udev/rules.d/62-temporary-interface-rename.rules delete mode 100644 src/etc/udev/rules.d/64-vyos-vmware-net.rules delete mode 100644 src/etc/udev/rules.d/65-vyos-net.rules create mode 100755 src/system/vyos-net-name-resolve.py create mode 100644 src/systemd/vyos-net-name-resolve.service create mode 100644 src/tests/test_ifname_store.py create mode 100644 src/tests/test_net_name_resolve.py delete mode 100755 src/udev/vyos_net_name (limited to 'src/init') diff --git a/debian/control b/debian/control index 0c7a6f505..43ebe43e8 100644 --- a/debian/control +++ b/debian/control @@ -68,7 +68,6 @@ Depends: at, rsync, vyatta-bash, - vyatta-biosdevname, vyatta-cfg, vyos-http-api-tools, ## End of Fundamentals diff --git a/python/vyos/defaults.py b/python/vyos/defaults.py index 84244d5c1..20d13df7b 100644 --- a/python/vyos/defaults.py +++ b/python/vyos/defaults.py @@ -32,7 +32,6 @@ directories = { 'api_schema': f'{base_dir}/services/api/graphql/graphql/schema/', 'api_client_op': f'{base_dir}/services/api/graphql/graphql/client_op/', 'api_templates': f'{base_dir}/services/api/graphql/session/templates/', - 'vyos_udev_dir' : '/run/udev/vyos', 'isc_dhclient_dir' : '/run/dhclient', 'dhcp6_client_dir' : '/run/dhcp6c', 'vyos_configdir' : '/opt/vyatta/config', diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py new file mode 100644 index 000000000..0d10fd1ef --- /dev/null +++ b/python/vyos/ifconfig/ifname_store.py @@ -0,0 +1,373 @@ +# Copyright VyOS maintainers and contributors +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library. If not, see . +# +# Persistent interface name store. +# +# The kernel command line carries net.ifnames=0, so VyOS owns the +# 'ethN'/'wlanN' namespace and has to make its choice survive a reboot. A name +# is mapped to the slot the hardware sits in rather than to a MAC in +# config.boot: the slot is the identity, so a replacement card in that slot is +# simply that interface again. +# +# The key is a single udev property picked from a fallback chain, most stable +# first. udev still computes these under net.ifnames=0 - only the renaming +# policy is disabled. + +import json +import os +import re +import tempfile +from pathlib import Path + +from vyos.defaults import directories +from vyos.utils.process import rc_cmd +from vyos.utils.permission import get_cfg_group_id + +STORE_VERSION = 1 + +# The real path, not '/config': naming runs before the bind mount is up. +store_path = Path(directories['config']) / 'interface-mapping.json' + +# Most stable first: a firmware slot index survives PCI renumbering, a path +# does not. The MAC-derived entry is a last resort for paravirtual buses. +KEY_PROPERTIES = ( + 'ID_NET_NAME_ONBOARD', + 'ID_NET_NAME_SLOT', + 'ID_NET_NAME_PATH', + 'ID_PATH', + 'ID_NET_NAME_MAC', +) + +_BUS_RANK = {'pci': 0, 'vmbus': 1, 'xen': 2, 'usb': 3} +_BUS_RANK_UNKNOWN = 90 + +_PCI_BDF = re.compile(r'([0-9a-f]{4}):([0-9a-f]{2}):([0-9a-f]{2})\.([0-9a-f]+)', re.I) +_MAC_RE = re.compile(r'([0-9a-fA-F]{2}(:[0-9a-fA-F]{2}){5})') + + +def ambiguous_keys(devices: list) -> set: + """Property values carried by more than one device.""" + seen = {} + for properties in devices: + for prop in KEY_PROPERTIES: + value = properties.get(prop, '') + if value: + tmp = f'{prop}={value}' + seen[tmp] = seen.get(tmp, 0) + 1 + return {key for key, count in seen.items() if count > 1} + + +def hardware_key(properties: dict, ambiguous: set = frozenset()) -> str: + """The most stable identifier this device offers, as 'PROPERTY=value'. + + A value more than one device carries identifies neither of them, so it is + passed over for the next one. + """ + for prop in KEY_PROPERTIES: + value = properties.get(prop, '') + if value and f'{prop}={value}' not in ambiguous: + return f'{prop}={value}' + return '' + + +def device_matches(properties: dict, key: str) -> bool: + """Does this device carry the property a stored key names?""" + if not key or '=' not in key: + return False + prop, _, value = key.partition('=') + return properties.get(prop, '') == value + + +def canonical_sort_key(device: dict) -> tuple: + """Total order by bus class, physical position then MAC, so it never + depends on probe order. Only used for hardware not yet in the store. + """ + properties = device.get('properties') or {} + path = properties.get('ID_PATH', '') + rank = _BUS_RANK.get(properties.get('ID_BUS', ''), _BUS_RANK_UNKNOWN) + + # numeric, so 0000:09:00.0 sorts before 0000:10:00.0 + segments = [tuple(int(part, 16) for part in m.groups()) + for m in _PCI_BDF.finditer(path)] + + return (rank, segments, path, device.get('mac') or '') + + +def permanent_mac(ifname: str, sys_class_net: str = '/sys/class/net') -> str: + """Permanent hardware address, falling back to the current one when the + driver cannot report it separately. + """ + code, out = rc_cmd(f'ethtool -P {ifname}') + if code == 0: + m = _MAC_RE.search(out) + if m and m.group(1) != '00:00:00:00:00:00': + return m.group(1).lower() + + try: + return Path(f'{sys_class_net}/{ifname}/address').read_text().strip().lower() + except OSError: + return '' + + +def is_wireless(ifname: str, sys_class_net: str = '/sys/class/net') -> bool: + """Checked via sysfs, not via the - possibly still probe-order - name.""" + return (Path(sys_class_net) / ifname / 'phy80211').exists() + + +def read_properties(ifname: str) -> dict: + """udev properties for an interface.""" + try: + import pyudev + + context = pyudev.Context() + device = pyudev.Devices.from_path(context, f'/sys/class/net/{ifname}') + return dict(device.properties) + except Exception: + return {} + + +def discover_devices(sys_class_net: str = '/sys/class/net') -> list: + """Every interface backed by independently-addressable hardware. + + Skips virtual interfaces, enslaved ones, SR-IOV virtual functions and + hypervisor VF datapaths - the latter share another interface's MAC. + """ + devices = [] + net_dir = Path(sys_class_net) + if not net_dir.is_dir(): + return devices + + for entry in sorted(net_dir.iterdir()): + if not (entry / 'device').exists(): + continue + if (entry / 'master').exists(): + continue + if (entry / 'device' / 'physfn').exists(): + continue + if entry.name.startswith('vf_'): + continue + + mac = permanent_mac(entry.name, sys_class_net) + if not mac: + continue + + devices.append({ + 'name': entry.name, + 'mac': mac, + 'wireless': is_wireless(entry.name, sys_class_net), + 'properties': read_properties(entry.name), + }) + + ambiguous = ambiguous_keys([device['properties'] for device in devices]) + for device in devices: + device['key'] = hardware_key(device['properties'], ambiguous) + + return devices + + +def empty_store() -> dict: + return {'version': STORE_VERSION, 'interfaces': {}, 'hardware': {}} + + +def load_store(path: Path = None) -> dict: + """A missing, unreadable or outdated store yields an empty one, so + everything is named from scratch. + """ + path = path or store_path + try: + data = json.loads(path.read_text()) + except (FileNotFoundError, OSError, ValueError): + return empty_store() + + if not isinstance(data, dict) or data.get('version') != STORE_VERSION: + return empty_store() + if not isinstance(data.get('interfaces'), dict): + return empty_store() + + interfaces = {name: key for name, key in data['interfaces'].items() + if isinstance(key, str) and key} + + # the card each name last sat on. Absent from a store written before this + # was recorded, which just means the first pass has nothing to compare + # against and reports no replacement - not that nothing changed. + seen = data.get('hardware') + hardware = {name: mac for name, mac in seen.items() + if isinstance(mac, str) and mac and name in interfaces + } if isinstance(seen, dict) else {} + + return {'version': STORE_VERSION, 'interfaces': interfaces, + 'hardware': hardware} + + +def config_dir_is_mounted(path: Path = None) -> bool: + """Is the config directory the real one, or a bare rootfs overlay? + + A failed TPM/LUKS unlock does not stop the boot. Writing to the overlay + underneath would be masked once the real volume mounts, leaving two + stores which silently diverge. + """ + path = path or store_path + return (path.parent / '.vyatta_config').exists() + + +def save_store(store: dict, path: Path = None) -> bool: + """Write atomically, so an interrupted boot leaves no half-written file. + + Returns False without writing on a degraded boot: names are still + resolved for this boot, they just must not be persisted. + """ + path = path or store_path + if not config_dir_is_mounted(path): + return False + + path.parent.mkdir(parents=True, exist_ok=True) + + fd, tmp = tempfile.mkstemp(dir=path.parent, prefix=f'.{path.name}.') + try: + with os.fdopen(fd, 'w') as f: + json.dump(store, f, indent=2, sort_keys=True) + f.write('\n') + os.chmod(tmp, 0o664) + try: + os.chown(tmp, -1, get_cfg_group_id()) + except (OSError, KeyError): + # not fatal, the store is read by root at boot + pass + os.replace(tmp, path) + except Exception: + Path(tmp).unlink(missing_ok=True) + raise + + return True + + +def _prefix_for(device: dict) -> str: + return 'wlan' if device.get('wireless') else 'eth' + + +def _next_free(taken: set, prefix: str) -> str: + index = 0 + while f'{prefix}{index}' in taken: + index += 1 + return f'{prefix}{index}' + + +def resolve(devices: list, store: dict) -> tuple: + """Name every physical interface present this boot. + + Returns (plan, new_store, report); plan is {current: new} for the + interfaces which have to be renamed. The slot decides the name - nothing + here looks at the configuration, so there is nothing to guess. + """ + entries = store.get('interfaces', {}) + seen = store.get('hardware', {}) + + assigned = {} # final name -> device + report = {'matched': {}, 'bootstrapped': {}} + + # A recorded name stays off-limits even while its slot is empty, so new + # hardware cannot inherit an absent interface's name and configuration. + reserved = set(entries) + remaining = list(devices) + + def claim(device, name, bucket): + assigned[name] = device + report[bucket][name] = device['mac'] + remaining.remove(device) + + # whatever card occupies the recorded slot is this interface + for name, key in entries.items(): + for device in remaining: + if device_matches(device['properties'], key): + claim(device, name, 'matched') + break + + # unknown hardware: lowest free name of its type, in hardware order + for device in sorted(remaining, key=canonical_sort_key): + name = _next_free(reserved | set(assigned), _prefix_for(device)) + claim(device, name, 'bootstrapped') + + # keep entries for empty slots - an unplugged card keeps its name + new_interfaces = dict(entries) + for name, device in assigned.items(): + if device['key']: + new_interfaces[name] = device['key'] + + # remember which card each name sat on, keeping the last record for an + # absent one so its eventual replacement is still noticed + new_hardware = dict(seen) + for name, device in assigned.items(): + if device['mac']: + new_hardware[name] = device['mac'] + + # a slot which kept its name but changed card hands that interface's + # addresses to different hardware - never silently + report['replaced'] = {name: mac for name, mac in report['matched'].items() + if seen.get(name) and seen[name] != mac} + + plan = {device['name']: name for name, device in assigned.items() + if device['name'] != name} + + return plan, {'version': STORE_VERSION, 'interfaces': new_interfaces, + 'hardware': new_hardware}, report + + +LINK_DIR = Path('/etc/systemd/network') +LINK_PREFIX = '10-vyos-' + + +def render_link(name: str, key: str) -> str: + """Render a systemd .link file body, matching on the stored property. + + The device type has to match as well, and a radio is 'wlan' - a .link + naming a type the device is not simply never applies, leaving the + interface on its probe-order name. resolve() already names by type (see + _prefix_for()), so the recorded name is what says which. + """ + return ( + '# Generated by VyOS - do not edit.\n' + "# Interface names are managed via 'interface-mapping.json'.\n" + '[Match]\n' + f"Type={'wlan' if name.startswith('wlan') else 'ether'}\n" + f'Property={key}\n' + '\n' + '[Link]\n' + f'Name={name}\n' + ) + + +def sync_link_files(store: dict, link_dir: Path = None) -> list: + """One .link file per recorded name, so udev applies it as the device is + added - before anything can observe a probe-order name. + """ + link_dir = link_dir or LINK_DIR + link_dir.mkdir(parents=True, exist_ok=True) + + written = {} + for name, key in store.get('interfaces', {}).items(): + if key: + written[link_dir / f'{LINK_PREFIX}{name}.link'] = render_link(name, key) + + for path, body in written.items(): + if path.exists() and path.read_text() == body: + continue + path.write_text(body) + + # drop stale files, but only our own generated ones + for existing in link_dir.glob(f'{LINK_PREFIX}*.link'): + if existing not in written: + existing.unlink(missing_ok=True) + + return sorted(written) diff --git a/src/etc/udev/rules.d/62-temporary-interface-rename.rules b/src/etc/udev/rules.d/62-temporary-interface-rename.rules deleted file mode 100644 index 4a579dcab..000000000 --- a/src/etc/udev/rules.d/62-temporary-interface-rename.rules +++ /dev/null @@ -1 +0,0 @@ -SUBSYSTEM=="net", ACTION=="add", KERNEL=="eth*", DRIVERS=="?*", NAME="e$env{IFINDEX}" diff --git a/src/etc/udev/rules.d/63-hyperv-vf-net.rules b/src/etc/udev/rules.d/63-hyperv-vf-net.rules index e5ecb4ede..224af12ab 100644 --- a/src/etc/udev/rules.d/63-hyperv-vf-net.rules +++ b/src/etc/udev/rules.d/63-hyperv-vf-net.rules @@ -1,3 +1,12 @@ +# Azure/Hyper-V VF interfaces must be kept out of the ethN namespace that +# VyOS manages. A VF is the acceleration datapath of an existing synthetic +# NIC, not an interface in its own right, and it shares that NIC's MAC - so +# left alone it would be taken for a second, independent interface. +# +# Renaming it to vf_ethN here, at device-add, is what keeps it out of the +# way; vyos-net-name-resolve.py then skips anything named vf_* (as well as +# anything with an SR-IOV physfn link, which is the general case). + ATTR{[dmi/id]sys_vendor}!="Microsoft Corporation", GOTO="end_hyperv_nic" # Primary rename path for Azure VF. @@ -6,4 +15,11 @@ ATTR{[dmi/id]sys_vendor}!="Microsoft Corporation", GOTO="end_hyperv_nic" # the interfaces already renamed to vf_ethN ACTION=="add", SUBSYSTEM=="net", DRIVERS=="hv_pci", KERNEL=="eth*", PROGRAM="vyos_vf_name %k", NAME="%c" +# Fallback path: a VF that still appears as a plain ethN, which happens +# when it is seen in an intermediate state before the hv_pci match above +# applies. Scoped to Azure on purpose - bare-metal Mellanox NICs are +# ordinary interfaces and must not be caught here. +ACTION=="add", SUBSYSTEM=="net", DRIVERS=="mlx*_core", KERNEL=="eth*", PROGRAM="vyos_vf_name %k", NAME="%c" +ACTION=="add", SUBSYSTEM=="net", DRIVERS=="mana", KERNEL=="eth*", PROGRAM="vyos_vf_name %k", NAME="%c" + LABEL="end_hyperv_nic" diff --git a/src/etc/udev/rules.d/64-vyos-vmware-net.rules b/src/etc/udev/rules.d/64-vyos-vmware-net.rules deleted file mode 100644 index 66a4a069b..000000000 --- a/src/etc/udev/rules.d/64-vyos-vmware-net.rules +++ /dev/null @@ -1,14 +0,0 @@ -ATTR{[dmi/id]sys_vendor}!="VMware, Inc.", GOTO="end_vmware_nic" - -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet0", ENV{VYOS_IFNAME}="eth0" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet1", ENV{VYOS_IFNAME}="eth1" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet2", ENV{VYOS_IFNAME}="eth2" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet3", ENV{VYOS_IFNAME}="eth3" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet4", ENV{VYOS_IFNAME}="eth4" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet5", ENV{VYOS_IFNAME}="eth5" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet6", ENV{VYOS_IFNAME}="eth6" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet7", ENV{VYOS_IFNAME}="eth7" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet8", ENV{VYOS_IFNAME}="eth8" -ACTION=="add", SUBSYSTEM=="net", ATTRS{label}=="Ethernet9", ENV{VYOS_IFNAME}="eth9" - -LABEL="end_vmware_nic" diff --git a/src/etc/udev/rules.d/65-vyos-net.rules b/src/etc/udev/rules.d/65-vyos-net.rules deleted file mode 100644 index 0e7567cf8..000000000 --- a/src/etc/udev/rules.d/65-vyos-net.rules +++ /dev/null @@ -1,41 +0,0 @@ -# These rules use vyos_net_name to persistently name network interfaces -# per "hwid" association in the VyOS configuration file. - -ACTION!="add", GOTO="vyos_net_end" -SUBSYSTEM!="net", GOTO="vyos_net_end" - -# Fallback path for Azure VF interfaces: if a VF still appears as plain ethN, -# rename it to vf_ethX before persistent synthetic interface naming runs to -# prevent it from incorrectly being considered as a synthetic interface. -ATTR{[dmi/id]sys_vendor}=="Microsoft Corporation", DRIVERS=="mlx*_core", KERNEL=="eth*", PROGRAM="vyos_vf_name %k", NAME="%c", GOTO="vyos_net_end" -ATTR{[dmi/id]sys_vendor}=="Microsoft Corporation", DRIVERS=="mana", KERNEL=="eth*", PROGRAM="vyos_vf_name %k", NAME="%c", GOTO="vyos_net_end" - -# VF interfaces are not handled by vyos_net_name, so skip them. -# KERNEL=="vf_*" catches already-renamed Mellanox and MANA VFs. -# DRIVERS=="mlx*_core" and DRIVERS=="mana" on Azure catch VF interfaces -# in an unexpected intermediate state (still named ethN/eN) that must not -# be remapped by vyos_net_name. -# This guard is intentionally scoped to Microsoft Azure to -# avoid excluding bare-metal Mellanox NICs on physical hardware, which do -# need persistent naming via vyos_net_name. -KERNEL=="vf_*", GOTO="vyos_net_end" -ATTR{[dmi/id]sys_vendor}=="Microsoft Corporation", DRIVERS=="mlx*_core", GOTO="vyos_net_end" -ATTR{[dmi/id]sys_vendor}=="Microsoft Corporation", DRIVERS=="mana", GOTO="vyos_net_end" - -# Do name change for ethernet and wireless devices only -KERNEL!="eth*|wlan*|e*", GOTO="vyos_net_end" - -# ignore "secondary" monitor interfaces of mac80211 drivers -KERNEL=="wlan*", ATTRS{type}=="803", GOTO="vyos_net_end" - -# If using VyOS predefined names -ENV{VYOS_IFNAME}!="eth*", GOTO="end_vyos_predef_names" - -DRIVERS=="?*", PROGRAM="vyos_net_name %k $attr{address} $env{VYOS_IFNAME}", NAME="%c", GOTO="vyos_net_end" - -LABEL="end_vyos_predef_names" - -# ignore interfaces without a driver link like bridges and VLANs -DRIVERS=="?*", PROGRAM="vyos_net_name %k $attr{address}", NAME="%c" - -LABEL="vyos_net_end" diff --git a/src/helpers/vyos-interface-rescan.py b/src/helpers/vyos-interface-rescan.py index fea9bca1c..fd2bf907e 100755 --- a/src/helpers/vyos-interface-rescan.py +++ b/src/helpers/vyos-interface-rescan.py @@ -14,21 +14,25 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # +# Make discovered interfaces visible in the configuration, so a newly +# installed NIC shows up without the operator adding it by hand. # +# T3871: the node is created bare. Writing a 'hw-id' back into config.boot is +# what used to make a bad guess unrecoverable - naming belongs to the store. -import os -import stat import argparse import logging -import netaddr +import os +import stat +from pathlib import Path from vyos.configtree import ConfigTree from vyos.defaults import directories +from vyos.ifconfig.ifname_store import load_store from vyos.utils.permission import get_cfg_group_id debug = False -vyos_udev_dir = directories['vyos_udev_dir'] vyos_log_dir = directories['log'] log_file = os.path.splitext(os.path.basename(__file__))[0] vyos_log_file = os.path.join(vyos_log_dir, log_file) @@ -39,139 +43,73 @@ formatter = logging.Formatter('%(levelname)s: %(message)s') handler.setFormatter(formatter) logger.addHandler(handler) -passlist = { - '02:07:01' : 'Interlan', - '02:60:60' : '3Com', - '02:60:8c' : '3Com', - '02:a0:c9' : 'Intel', - '02:aa:3c' : 'Olivetti', - '02:cf:1f' : 'CMC', - '02:e0:3b' : 'Prominet', - '02:e6:d3' : 'BTI', - '52:54:00' : 'Realtek', - '52:54:4c' : 'Novell 2000', - '52:54:ab' : 'Realtec', - 'e2:0c:0f' : 'Kingston Technologies' -} - -def is_multicast(addr: netaddr.eui.EUI) -> bool: - return bool(addr.words[0] & 0b1) - -def is_locally_administered(addr: netaddr.eui.EUI) -> bool: - return bool(addr.words[0] & 0b10) - -def is_on_passlist(hwid: str) -> bool: - top = hwid.rsplit(':', 3)[0] - if top in list(passlist): - return True - return False - -def is_persistent(hwid: str) -> bool: - addr = netaddr.EUI(hwid) - if is_multicast(addr): - return False - if is_locally_administered(addr) and not is_on_passlist(hwid): - return False - return True + +def get_interface_type(intf: str) -> str: + if intf.startswith('eth'): + return 'ethernet' + if intf.startswith('wlan'): + return 'wireless' + logger.critical(f"Unrecognized interface type for '{intf}'") + return '' + def get_wireless_physical_device(intf: str) -> str: - if 'wlan' not in intf: - return '' + """The phy a wireless interface belongs to.""" try: - tmp = os.readlink(f'/sys/class/net/{intf}/phy80211') + return os.path.basename( + os.readlink(f'/sys/class/net/{intf}/phy80211')) except OSError: - logger.critical(f"Failed to read '/sys/class/net/{intf}/phy80211'") return '' - phy = os.path.basename(tmp) - logger.info(f"wireless phy is {phy}") - return phy - -def get_interface_type(intf: str) -> str: - if 'eth' in intf: - intf_type = 'ethernet' - elif 'wlan' in intf: - intf_type = 'wireless' - else: - logger.critical('Unrecognized interface type!') - intf_type = '' - return intf_type - -def get_new_interfaces() -> dict: - """ Read any new interface data left in /run/udev/vyos by vyos_net_name - """ - interfaces = {} - - for intf in os.listdir(vyos_udev_dir): - path = os.path.join(vyos_udev_dir, intf) - try: - with open(path) as f: - hwid = f.read().rstrip() - except OSError as e: - logger.error(f"OSError {e}") - continue - interfaces[intf] = hwid - # reverse sort to simplify insertion in config - interfaces = {key: value for key, value in sorted(interfaces.items(), - reverse=True)} - return interfaces - -def filter_interfaces(intfs: dict) -> dict: - """ Ignore no longer existing interfaces or non-persistent mac addresses - """ - filtered = {} - - for intf, hwid in intfs.items(): - if not os.path.isdir(os.path.join('/sys/class/net', intf)): - continue - if not is_persistent(hwid): - continue - filtered[intf] = hwid - - return filtered def interface_rescan(config_path: str): - """ Read new data and update config file - """ - interfaces = get_new_interfaces() - - logger.debug(f"interfaces from udev: {interfaces}") + """Add a node for every named interface which does not have one yet.""" + store = load_store() - interfaces = filter_interfaces(interfaces) + # a reserved name whose hardware is absent has nothing to create a node for + names = [name for name in sorted(store.get('interfaces', {})) + if Path(f'/sys/class/net/{name}').is_dir()] - logger.debug(f"filtered interfaces: {interfaces}") + logger.debug(f'named interfaces present: {names}') try: - with open(config_path) as f: - config_file = f.read() + config_file = Path(config_path).read_text() except OSError as e: - logger.critical(f"OSError {e}") + logger.critical(f'OSError {e}') exit(1) config = ConfigTree(config_file) + changed = False - for intf, hwid in interfaces.items(): - logger.info(f"Writing '{intf}' '{hwid}' to config file") + for intf in names: intf_type = get_interface_type(intf) if not intf_type: continue + if config.exists(['interfaces', intf_type, intf]): + continue + + logger.info(f"Adding '{intf}' to the configuration") if not config.exists(['interfaces', intf_type]): config.set(['interfaces', intf_type]) config.set_tag(['interfaces', intf_type]) - config.set(['interfaces', intf_type, intf, 'hw-id'], value=hwid) + config.set(['interfaces', intf_type, intf]) + changed = True if intf_type == 'wireless': phy = get_wireless_physical_device(intf) - if not phy: - continue - config.set(['interfaces', intf_type, intf, 'physical-device'], - value=phy) + if phy: + config.set(['interfaces', intf_type, intf, 'physical-device'], + value=phy) + + if not changed: + return try: - with open(config_path, 'w') as f: - f.write(config.to_string()) + Path(config_path).write_text(config.to_string()) except OSError as e: - logger.critical(f"OSError {e}") + logger.critical(f'OSError {e}') + exit(1) + def main(): global debug @@ -202,5 +140,6 @@ def main(): interface_rescan(configfile) + if __name__ == '__main__': main() diff --git a/src/init/vyos-router b/src/init/vyos-router index c0d12e2a7..ff1de0584 100755 --- a/src/init/vyos-router +++ b/src/init/vyos-router @@ -357,11 +357,39 @@ clear_or_override_config_files () update_interface_config () { - if [ -d /run/udev/vyos ]; then + # add a configuration node for any interface the name store knows about + # that does not have one yet + if [ -f /opt/vyatta/etc/config/interface-mapping.json ]; then $vyos_libexec_dir/vyos-interface-rescan.py $BOOTFILE fi } +# warn (but do not fail boot) about configured hw-id interfaces whose +# hardware was not found by vyos-net-name-resolve.service +warn_missing_interface_hardware () +{ + local status_file="/run/vyos-net-name-resolve.json" + [ -f "${status_file}" ] || return 0 + + jq -r '.missing // {} | to_entries[] | "\(.key) \(.value)"' "${status_file}" \ + 2>/dev/null | while read -r intf hwid; do + [ -n "${intf}" ] || continue + log_failure_msg "interface '${intf}' (${hwid}) did not appear during boot - check cabling, hardware and driver/firmware. Its name stays reserved." + done + + # A replacement NIC inheriting a slot's name also inherits that + # interface's addresses, so say so rather than doing it silently. + jq -r '.matched_by_path // {} | to_entries[] | "\(.key) \(.value)"' "${status_file}" \ + 2>/dev/null | while read -r intf hwid; do + [ -n "${intf}" ] || continue + log_warning_msg "interface '${intf}' was matched to new hardware ${hwid} by slot - verify it is the port you expect before relying on its configuration" + done + + if [ "$(jq -r '.persisted // true' "${status_file}" 2>/dev/null)" = "false" ]; then + log_failure_msg "interface names could not be persisted - the configuration volume is not mounted" + fi +} + cleanup_post_commit_hooks () { # Remove links from the post-commit hooks directory. # note that this approach only supports hooks that are "configured", @@ -606,6 +634,12 @@ start () disabled migrate || migrate_bootfile || overall_status=1 + # config.boot is only guaranteed to exist/be current from this point on + # (tmpfs mount + encrypted config unlock + syntax migration have all + # already happened above) - hw-id based interface naming must be + # resolved against it before anything below applies interface config. + systemctl start vyos-net-name-resolve.service || overall_status=1 + restore_if_missing_bootup_script pre run_preconfig_script @@ -613,6 +647,7 @@ start () run_postupgrade_script update_interface_config || overall_status=1 + warn_missing_interface_hardware disabled system_activate || system_activate || overall_status=1 diff --git a/src/system/vyos-net-name-resolve.py b/src/system/vyos-net-name-resolve.py new file mode 100755 index 000000000..b7fe052e6 --- /dev/null +++ b/src/system/vyos-net-name-resolve.py @@ -0,0 +1,221 @@ +#!/usr/bin/env python3 +# +# Copyright VyOS maintainers and contributors +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 2 or later as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +# Boot-time maintenance of the interface mapping store (T3871). +# +# The store is rendered into systemd .link files, so udev does the actual +# naming as each device is added. What is left here is what udev cannot do: +# name hardware the store has never seen, record it, and re-render. +# +# Nothing here reads the configuration - a name is a property of the +# hardware, not of what happens to be configured on it. + +import json +import logging +import logging.handlers +import time +from pathlib import Path + +from vyos.ifconfig.ifname_store import device_matches +from vyos.ifconfig.ifname_store import discover_devices +from vyos.ifconfig.ifname_store import load_store +from vyos.ifconfig.ifname_store import resolve +from vyos.ifconfig.ifname_store import save_store +from vyos.ifconfig.ifname_store import sync_link_files +from vyos.system.image import is_running_as_container +from vyos.utils.process import run + +# bounded wait for known hardware to appear +HARDWARE_WAIT_TIMEOUT = 10 +HARDWARE_WAIT_POLL = 0.25 + +# unknown hardware has no specific device to wait for, so N identical +# snapshots stand in for "probing has finished" +HARDWARE_SETTLE_TIMEOUT = 10 +HARDWARE_SETTLE_STABLE_POLLS = 3 + +status_file = Path('/run/vyos-net-name-resolve.json') + +logger = logging.getLogger() + + +def _snapshot(devices: list) -> set: + return {(d['name'], d['mac']) for d in devices} + + +def _unmatched(keys: set, devices: list) -> set: + """Recorded slots that no device present this boot occupies.""" + return {key for key in keys + if not any(device_matches(d['properties'], key) for d in devices)} + + +def wait_for_hardware(known_keys: set, timeout: float = HARDWARE_WAIT_TIMEOUT, + poll: float = HARDWARE_WAIT_POLL) -> tuple: + """Poll until every slot the store knows is filled, or until timeout.""" + deadline = time.monotonic() + timeout + devices = discover_devices() + while True: + missing = _unmatched(known_keys, devices) + if not missing or time.monotonic() >= deadline: + return devices, missing + time.sleep(poll) + devices = discover_devices() + + +def wait_for_settle(initial: list, timeout: float = HARDWARE_SETTLE_TIMEOUT, + poll: float = HARDWARE_WAIT_POLL, + stable_polls: int = HARDWARE_SETTLE_STABLE_POLLS) -> list: + """Poll until the set of interfaces stops changing, or until timeout. + Used before naming hardware the store has never seen, where there is no + specific MAC to wait for. + """ + deadline = time.monotonic() + timeout + devices = initial + stable = 1 if devices else 0 + while stable < stable_polls and time.monotonic() < deadline: + time.sleep(poll) + seen = discover_devices() + stable = stable + 1 if _snapshot(seen) == _snapshot(devices) else 1 + devices = seen + return devices + + +def get_ifindex(name: str) -> str: + try: + return Path(f'/sys/class/net/{name}/ifindex').read_text().strip() + except OSError: + return name + + +def rename_interface(old: str, new: str) -> bool: + run(f'ip link set dev {old} down') + code = run(f'ip link set dev {old} name {new}') + if code != 0: + logger.error(f"failed to rename '{old}' -> '{new}' (exit {code})") + return False + logger.info(f"renamed '{old}' -> '{new}'") + return True + + +def safe_bulk_rename(plan: dict) -> dict: + """Stage every interface under a scratch name derived from its ifindex + before assigning the final ones, so the batch cannot collide on a + permutation or a cycle. Only new hardware reaches this - udev has already + named everything the store knows. + """ + if not plan: + return {} + + applied = {} + scratch = {} + for old, target in plan.items(): + tmp = f'vyeth{get_ifindex(old)}' + if rename_interface(old, tmp): + scratch[tmp] = (old, target) + applied[old] = target + + for tmp, (old, target) in scratch.items(): + if rename_interface(tmp, target): + run(f'ip link set dev {target} up') + else: + # still under the scratch name - do not report it as renamed + del applied[old] + run(f'ip link set dev {tmp} up') + + return applied + + +def write_status(store: dict, devices: list, applied: dict, report: dict, + persisted: bool) -> None: + """Publish what this pass decided, to warn about and to inspect later.""" + absent = _unmatched(set(store['interfaces'].values()), devices) + status = { + 'interfaces': dict(sorted(store['interfaces'].items())), + 'missing': { + name: key for name, key in sorted(store['interfaces'].items()) + if key in absent + }, + 'renamed': applied, + 'matched': report.get('matched', {}), + 'matched_by_path': report.get('replaced', {}), + 'new_hardware': report.get('bootstrapped', {}), + 'persisted': persisted, + } + try: + status_file.write_text(json.dumps(status, indent=2)) + except OSError as e: + logger.error(f'could not write status file: {e}') + + +def main(): + if is_running_as_container(): + # a container owns no NIC - its veth pairs have no backing bus device + logger.info('running inside a container - skipping interface naming') + return + + store = load_store() + known = set(store['interfaces'].values()) + + devices, _missing = wait_for_hardware(known) + + if not store['interfaces'] or any( + not any(device_matches(d['properties'], k) for k in known) + for d in devices): + # unknown hardware present - let slower drivers finish registering + # so a merely late NIC is not treated as new + devices = wait_for_settle(devices) + + absent = _unmatched(known, devices) + for name, key in sorted(store['interfaces'].items()): + if key in absent: + logger.warning( + f"'{name}' ({key}) did not appear within " + f'{HARDWARE_WAIT_TIMEOUT}s - hardware may be missing, still ' + 'initializing, or its driver failed to load. Its name stays ' + 'reserved.' + ) + + plan, store, report = resolve(devices, store) + + for name, mac in sorted(report.get('bootstrapped', {}).items()): + logger.info(f"new hardware {mac} named '{name}'") + applied = safe_bulk_rename(plan) + + persisted = save_store(store) + if persisted: + # render so udev applies the names on the next boot + try: + sync_link_files(store) + except OSError as e: + logger.error(f'could not write .link files: {e}') + else: + logger.warning( + 'the configuration volume is not mounted - interface names were ' + 'resolved for this boot but could not be persisted' + ) + + write_status(store, devices, applied, report, persisted) + + +if __name__ == '__main__': + syslog_handler = logging.handlers.SysLogHandler(address='/dev/log') + formatter = logging.Formatter(f'{Path(__file__).name}: %(message)s') + syslog_handler.setFormatter(formatter) + + logger.addHandler(syslog_handler) + logger.setLevel(logging.DEBUG) + + main() diff --git a/src/systemd/vyos-net-name-resolve.service b/src/systemd/vyos-net-name-resolve.service new file mode 100644 index 000000000..c3563dd84 --- /dev/null +++ b/src/systemd/vyos-net-name-resolve.service @@ -0,0 +1,15 @@ +[Unit] +Description=VyOS persistent interface naming +Documentation=man:vyos-net-name-resolve.py(8) + +# Started by vyos-router once the config directory is mounted - for an +# encrypted volume that is only after it has been unlocked - so this can not +# be ordered independently. +DefaultDependencies=no + +[Service] +Type=oneshot +RemainAfterExit=yes +ExecStart=/usr/libexec/vyos/system/vyos-net-name-resolve.py +TimeoutStartSec=90 +StandardOutput=journal+console diff --git a/src/tests/helper.py b/src/tests/helper.py index 2c4421d0a..5a74525b7 100644 --- a/src/tests/helper.py +++ b/src/tests/helper.py @@ -14,9 +14,20 @@ import sys import importlib.util +from importlib.machinery import SourceFileLoader def prepare_module(file_path='', module_name=''): + """Load file_path as module_name, returning the loaded module. + + file_path may be an extensionless script (e.g. udev helpers under + src/udev/) - spec_from_file_location() cannot infer a loader for those + from the suffix alone, so fall back to an explicit SourceFileLoader. + """ spec = importlib.util.spec_from_file_location(module_name, file_path) + if spec is None: + loader = SourceFileLoader(module_name, file_path) + spec = importlib.util.spec_from_loader(module_name, loader) module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) sys.modules[module_name] = module + return module diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py new file mode 100644 index 000000000..3f12594d4 --- /dev/null +++ b/src/tests/test_ifname_store.py @@ -0,0 +1,572 @@ +# Copyright VyOS maintainers and contributors +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 2 or later as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +# Coverage for T3871: slot-anchored persistent interface naming. +# +# The regression classes at the bottom replay, verbatim, the field +# reproductions from the review of vyos-1x PR #5417 - the cases where the +# config-tree-driven resolver bound a configured address to the wrong +# physical wire. Under the store-based design none of them have a code path +# to travel, and these tests exist to keep it that way. + +import os +import shutil +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +from vyos.ifconfig.ifname_store import STORE_VERSION +from vyos.ifconfig.ifname_store import canonical_sort_key +from vyos.ifconfig.ifname_store import device_matches +from vyos.ifconfig.ifname_store import ambiguous_keys +from vyos.ifconfig.ifname_store import hardware_key +from vyos.ifconfig.ifname_store import discover_devices +from vyos.ifconfig.ifname_store import empty_store +from vyos.ifconfig.ifname_store import load_store +from vyos.ifconfig.ifname_store import permanent_mac +from vyos.ifconfig.ifname_store import render_link +from vyos.ifconfig.ifname_store import resolve +from vyos.ifconfig.ifname_store import sync_link_files +from vyos.ifconfig.ifname_store import save_store + + +def dev(name, path, mac, wireless=False, bus='pci'): + """A device sitting at 'path'. Its key is what udev would report for it - + here the raw ID_PATH, which is what a plain virtio or e1000e NIC with no + firmware slot index ends up keyed on.""" + properties = {'ID_PATH': path, 'ID_BUS': bus} if path else {} + return {'name': name, 'mac': mac, 'wireless': wireless, + 'properties': properties, + 'key': hardware_key(properties)} + + +def store_of(mapping, hardware=None): + """mapping: {name: path} - the slot each name is recorded against""" + return { + 'version': STORE_VERSION, + 'interfaces': {n: f'ID_PATH={p}' for n, p in mapping.items()}, + 'hardware': dict(hardware or {}), + } + + +class TestCanonicalOrder(unittest.TestCase): + def test_pci_sorted_numerically_not_lexically(self): + # 0000:09:00.0 must precede 0000:10:00.0 - string order gets this wrong + a = dev('x', 'pci-0000:00:09.0', 'aa:00:00:00:00:01') + b = dev('y', 'pci-0000:00:10.0', 'aa:00:00:00:00:02') + self.assertLess(canonical_sort_key(a), canonical_sort_key(b)) + + def test_pci_before_usb(self): + pci = dev('x', 'pci-0000:00:1f.6', 'aa:00:00:00:00:01') + usb = dev('y', 'usb-0:1:1.0', 'aa:00:00:00:00:00', bus='usb') + self.assertLess(canonical_sort_key(pci), canonical_sort_key(usb)) + + def test_order_is_independent_of_mac(self): + # the whole point: a numerically higher MAC in a lower slot still wins + low_slot = dev('x', 'pci-0000:00:02.0', 'ff:ff:ff:ff:ff:ff') + high_slot = dev('y', 'pci-0000:00:05.0', '00:00:00:00:00:01') + self.assertLess(canonical_sort_key(low_slot), canonical_sort_key(high_slot)) + + +class TestReplacedHardware(unittest.TestCase): + """A slot keeps its name when its card is swapped - which hands that + interface's addresses to different hardware. resolve() reports the change + so vyos-router can warn instead of letting it happen silently. + """ + + PATH = 'pci-0000:00:12.0' + + def _device(self, mac): + return dev('eth0', self.PATH, mac) + + def test_same_card_reports_nothing(self): + store = store_of({'eth0': self.PATH}, {'eth0': 'aa:aa:aa:aa:aa:01'}) + _, _, report = resolve([self._device('aa:aa:aa:aa:aa:01')], store) + self.assertEqual(report['replaced'], {}) + + def test_swapped_card_is_reported_with_its_new_mac(self): + store = store_of({'eth0': self.PATH}, {'eth0': 'aa:aa:aa:aa:aa:01'}) + _, new_store, report = resolve([self._device('bb:bb:bb:bb:bb:02')], store) + self.assertEqual(report['replaced'], {'eth0': 'bb:bb:bb:bb:bb:02'}) + # the name is still the slot's, and the new card is now what is on record + self.assertEqual(new_store['interfaces']['eth0'], f'ID_PATH={self.PATH}') + self.assertEqual(new_store['hardware']['eth0'], 'bb:bb:bb:bb:bb:02') + + def test_reported_once_not_on_every_later_boot(self): + store = store_of({'eth0': self.PATH}, {'eth0': 'aa:aa:aa:aa:aa:01'}) + _, store, _ = resolve([self._device('bb:bb:bb:bb:bb:02')], store) + _, _, report = resolve([self._device('bb:bb:bb:bb:bb:02')], store) + self.assertEqual(report['replaced'], {}) + + def test_absent_card_keeps_its_record_so_a_later_swap_is_caught(self): + store = store_of({'eth0': self.PATH}, {'eth0': 'aa:aa:aa:aa:aa:01'}) + _, store, _ = resolve([], store) + self.assertEqual(store['hardware']['eth0'], 'aa:aa:aa:aa:aa:01') + _, _, report = resolve([self._device('bb:bb:bb:bb:bb:02')], store) + self.assertEqual(report['replaced'], {'eth0': 'bb:bb:bb:bb:bb:02'}) + + def test_new_hardware_is_not_a_replacement(self): + _, _, report = resolve([self._device('aa:aa:aa:aa:aa:01')], empty_store()) + self.assertEqual(report['replaced'], {}) + self.assertIn('eth0', report['bootstrapped']) + + def test_store_written_before_this_field_reports_nothing(self): + # nothing to compare against yet - and no interface may be renamed + legacy = {'version': STORE_VERSION, + 'interfaces': {'eth0': f'ID_PATH={self.PATH}'}} + plan, new_store, report = resolve([self._device('aa:aa:aa:aa:aa:01')], + legacy) + self.assertEqual(report['replaced'], {}) + self.assertEqual(plan, {}) + self.assertEqual(new_store['interfaces']['eth0'], f'ID_PATH={self.PATH}') + + +class TestStoreIO(unittest.TestCase): + @staticmethod + def _config_dir(d): + """A directory that looks like a properly mounted config volume.""" + (Path(d) / '.vyatta_config').touch() + return Path(d) / 'interface-mapping.json' + + def test_roundtrip(self): + with tempfile.TemporaryDirectory() as d: + p = self._config_dir(d) + s = store_of({'eth0': 'pci-0000:00:12.0'}) + self.assertTrue(save_store(s, p)) + self.assertEqual(load_store(p), s) + + def test_refuses_to_write_when_config_volume_is_not_mounted(self): + # TPM/LUKS unlock failed: vyos-router continues, but persisting here + # would write a shadow store that the real volume masks later + with tempfile.TemporaryDirectory() as d: + p = Path(d) / 'interface-mapping.json' # no .vyatta_config marker + self.assertFalse(save_store(store_of({'eth0': 'x'}), p)) + self.assertFalse(p.exists()) + + def test_missing_file_is_empty_store(self): + with tempfile.TemporaryDirectory() as d: + self.assertEqual(load_store(Path(d) / 'nope.json'), empty_store()) + + def test_corrupt_file_is_empty_store(self): + with tempfile.TemporaryDirectory() as d: + p = self._config_dir(d) + p.write_text('{ this is not json') + self.assertEqual(load_store(p), empty_store()) + + def test_write_is_atomic_no_leftovers(self): + with tempfile.TemporaryDirectory() as d: + p = self._config_dir(d) + save_store(store_of({'eth0': 'pci-0000:00:12.0'}), p) + self.assertEqual(sorted(f.name for f in Path(d).iterdir()), + ['.vyatta_config', 'interface-mapping.json']) + + +def final_names(report): + """Which name each MAC ended up with, read back off the resolve report.""" + out = {} + for bucket in report.values(): + for name, mac in bucket.items(): + out[mac] = name + return out + + +class TestDeviceMatches(unittest.TestCase): + """A stored key names the property it was taken from, so matching a + device is comparing that one property - never a guess about which + identifier was meant.""" + + def test_matches_the_named_property(self): + self.assertTrue(device_matches({'ID_NET_NAME_SLOT': 'ens18'}, + 'ID_NET_NAME_SLOT=ens18')) + + def test_same_value_under_a_different_property_does_not_match(self): + self.assertFalse(device_matches({'ID_NET_NAME_PATH': 'ens18'}, + 'ID_NET_NAME_SLOT=ens18')) + + def test_device_without_the_property_does_not_match(self): + self.assertFalse(device_matches({'ID_PATH': 'pci-0000:00:12.0'}, + 'ID_NET_NAME_SLOT=ens18')) + + def test_empty_or_malformed_key_never_matches(self): + self.assertFalse(device_matches({'ID_NET_NAME_SLOT': 'ens18'}, '')) + self.assertFalse(device_matches({'ID_NET_NAME_SLOT': 'ens18'}, 'ens18')) + + +class TestResolveBasics(unittest.TestCase): + def test_first_boot_bootstraps_in_slot_order(self): + devices = [ + dev('eth0', 'pci-0000:00:15.0', '00:00:5e:00:53:03'), + dev('eth1', 'pci-0000:00:12.0', '00:00:5e:00:53:00'), + dev('eth2', 'pci-0000:00:13.0', '00:00:5e:00:53:01'), + ] + _, new_store, _ = resolve(devices, empty_store()) + self.assertEqual(new_store['interfaces']['eth0'], 'ID_PATH=pci-0000:00:12.0') + self.assertEqual(new_store['interfaces']['eth1'], 'ID_PATH=pci-0000:00:13.0') + self.assertEqual(new_store['interfaces']['eth2'], 'ID_PATH=pci-0000:00:15.0') + + def test_established_names_are_stable_regardless_of_probe_order(self): + store = store_of({'eth0': 'pci-0000:00:12.0', 'eth1': 'pci-0000:00:13.0'}) + # drivers finished in the opposite order this boot + devices = [ + dev('eth0', 'pci-0000:00:13.0', '00:00:5e:00:53:01'), + dev('eth1', 'pci-0000:00:12.0', '00:00:5e:00:53:00'), + ] + plan, _, _ = resolve(devices, store) + self.assertEqual(plan, {'eth0': 'eth1', 'eth1': 'eth0'}) + + def test_replacement_nic_in_same_slot_inherits_name(self): + # the slot is the identity, so this needs no special rule: whatever + # card is in eth1's slot is eth1, along with its configuration + store = store_of({'eth1': 'pci-0000:00:13.0'}) + devices = [dev('eth0', 'pci-0000:00:13.0', 'bb:bb:bb:bb:bb:bb')] + plan, new_store, report = resolve(devices, store) + self.assertEqual(plan, {'eth0': 'eth1'}) + self.assertEqual(report['matched'], {'eth1': 'bb:bb:bb:bb:bb:bb'}) + self.assertEqual(new_store['interfaces']['eth1'], 'ID_PATH=pci-0000:00:13.0') + + def test_a_card_moved_to_another_slot_takes_that_slot_s_name(self): + # the consequence of slot identity, stated plainly: move a card and it + # becomes whatever the new slot is called, not what it used to be + store = store_of({'eth1': 'pci-0000:00:13.0', 'eth2': 'pci-0000:00:14.0'}) + devices = [dev('eth9', 'pci-0000:00:14.0', 'aa:aa:aa:aa:aa:aa')] + plan, _, report = resolve(devices, store) + self.assertEqual(plan, {'eth9': 'eth2'}) + self.assertEqual(final_names(report)['aa:aa:aa:aa:aa:aa'], 'eth2') + + def test_absent_hardware_keeps_its_name_reserved(self): + store = store_of({'eth0': 'pci-0000:00:12.0', 'eth1': 'pci-0000:00:13.0'}) + # eth0's slot is empty; a brand new card appeared elsewhere + devices = [ + dev('eth0', 'pci-0000:00:13.0', '00:00:5e:00:53:01'), + dev('eth1', 'pci-0000:00:1c.0', 'cc:cc:cc:cc:cc:cc'), + ] + _, new_store, report = resolve(devices, store) + # the new card must not inherit eth0's name, and with it eth0's config + self.assertNotEqual(final_names(report)['cc:cc:cc:cc:cc:cc'], 'eth0') + self.assertEqual(new_store['interfaces']['eth0'], 'ID_PATH=pci-0000:00:12.0') + + def test_hardware_with_no_usable_path_falls_back_to_its_mac(self): + # some paravirtual buses give udev nothing to build a path from; the + # MAC-derived name is the last step of the key chain so the interface + # is still stable instead of being renamed every boot + properties = {'ID_NET_NAME_MAC': 'enxaabbccddeeff'} + device = {'name': 'eth0', 'mac': 'aa:bb:cc:dd:ee:ff', 'wireless': False, + 'properties': properties, 'key': hardware_key(properties)} + _, new_store, _ = resolve([device], empty_store()) + self.assertEqual(new_store['interfaces']['eth0'], + 'ID_NET_NAME_MAC=enxaabbccddeeff') + + def test_a_path_two_devices_share_is_not_used_to_identify_either(self): + # a modem can put several interfaces on one parent device, and they + # then report the same path. Keying both on it would give them one + # name between them, so fall through to something which tells them + # apart. + first = {'ID_PATH': 'pci-0000:00:12.0', + 'ID_NET_NAME_MAC': 'enxaabbccddee01'} + second = {'ID_PATH': 'pci-0000:00:12.0', + 'ID_NET_NAME_MAC': 'enxaabbccddee02'} + ambiguous = ambiguous_keys([first, second]) + self.assertEqual(hardware_key(first, ambiguous), + 'ID_NET_NAME_MAC=enxaabbccddee01') + self.assertEqual(hardware_key(second, ambiguous), + 'ID_NET_NAME_MAC=enxaabbccddee02') + + def test_a_path_only_one_device_has_is_still_used(self): + first = {'ID_PATH': 'pci-0000:00:12.0'} + second = {'ID_PATH': 'pci-0000:00:13.0'} + ambiguous = ambiguous_keys([first, second]) + self.assertEqual(hardware_key(first, ambiguous), + 'ID_PATH=pci-0000:00:12.0') + + def test_firmware_slot_is_preferred_over_a_bare_pci_path(self): + # a firmware slot index survives PCI renumbering, a bus address does not + properties = {'ID_NET_NAME_SLOT': 'ens18', + 'ID_NET_NAME_PATH': 'enp0s18', + 'ID_PATH': 'pci-0000:00:12.0'} + self.assertEqual(hardware_key(properties), 'ID_NET_NAME_SLOT=ens18') + + def test_wireless_uses_its_own_namespace(self): + devices = [dev('eth0', 'pci-0000:00:12.0', 'aa:aa:aa:aa:aa:aa'), + dev('wlan5', 'pci-0000:00:14.0', 'cc:cc:cc:cc:cc:cc', + wireless=True)] + _, new_store, _ = resolve(devices, empty_store()) + self.assertIn('eth0', new_store['interfaces']) + self.assertIn('wlan0', new_store['interfaces']) + + +class TestPR5417Regressions(unittest.TestCase): + """Each test replays a reproduction from the review of PR #5417, where the + config-tree-driven resolver put a configured address on the wrong wire. + The assertions are about physical correctness - which MAC ends up behind + which name - not about log output. + """ + + def test_multi_nic_cloud_init_first_boot(self): + """2026-08-25 #1: three NICs, cloud-init synthesized a hw-id for eth0 + only. eth1/eth2 were pushed to eth3/eth4 and both data addresses were + unreachable, persisting across reboot. + """ + devices = [ + dev('eth0', 'pci-0000:00:03.0', '52:54:00:00:00:10'), + dev('eth1', 'pci-0000:00:04.0', '52:54:00:ff:00:21'), + dev('eth2', 'pci-0000:00:05.0', '52:54:00:00:00:22'), + ] + _, new_store, report = resolve(devices, empty_store()) + names = final_names(report) + self.assertEqual(names['52:54:00:00:00:10'], 'eth0') + self.assertEqual(names['52:54:00:ff:00:21'], 'eth1') + self.assertEqual(names['52:54:00:00:00:22'], 'eth2') + self.assertEqual(set(new_store['interfaces']), {'eth0', 'eth1', 'eth2'}) + + def test_names_follow_slots_not_mac_magnitude(self): + """2026-08-25 #2: the resolver swapped eth1/eth2 by MAC order, so each + address landed on the other's physical network, and the swap was + persisted with no warning. eth1 sits in the lower slot but has the + numerically HIGHER MAC. + """ + devices = [ + dev('eth1', 'pci-0000:00:04.0', '52:54:00:ff:00:21'), + dev('eth2', 'pci-0000:00:05.0', '52:54:00:00:00:22'), + ] + _, _, report = resolve(devices, empty_store()) + names = final_names(report) + self.assertEqual(names['52:54:00:ff:00:21'], 'eth0') + self.assertEqual(names['52:54:00:00:00:22'], 'eth1') + + def test_box_named_out_of_slot_order_is_not_reordered(self): + """2026-09-02 / 2026-09-09: a box first named by an earlier release in + MAC order - eth1 on the slot-5 NIC, eth4 on the slot-2 NIC. Deleting + hw-id entries made the resolver re-derive names in PCI slot order and + put eth1 and eth4 on each other's cable, silently and permanently. + + The store records the slot each name was actually on, so nothing is + ever re-derived and the box keeps the names it had. + """ + store = store_of({ + 'eth0': 'pci-0000:00:01.0', + 'eth1': 'pci-0000:00:05.0', + 'eth2': 'pci-0000:00:03.0', + 'eth3': 'pci-0000:00:04.0', + 'eth4': 'pci-0000:00:02.0', + }) + devices = [ + dev('eth0', 'pci-0000:00:01.0', '00:00:5e:00:54:01'), + dev('eth1', 'pci-0000:00:05.0', '00:00:5e:00:54:12'), + dev('eth2', 'pci-0000:00:03.0', '00:00:5e:00:54:13'), + dev('eth3', 'pci-0000:00:04.0', '00:00:5e:00:54:14'), + dev('eth4', 'pci-0000:00:02.0', '00:00:5e:00:54:19'), + ] + plan, _, report = resolve(devices, store) + self.assertEqual(plan, {}, 'nothing may be renamed') + names = final_names(report) + self.assertEqual(names['00:00:5e:00:54:12'], 'eth1') + self.assertEqual(names['00:00:5e:00:54:19'], 'eth4') + + def test_spare_nic_does_not_displace_configured_interfaces(self): + """2026-09-02 #2: a fourth, unconfigured NIC was enough to make the + three configured interfaces fail to get their hardware - eth1/eth2 + stayed bare while eth3/eth4/eth5 appeared holding the NICs. + """ + devices = [ + dev('eth0', 'pci-0000:00:03.0', '00:00:5e:00:54:31'), + dev('eth1', 'pci-0000:00:04.0', '00:00:5e:00:54:39'), + dev('eth2', 'pci-0000:00:05.0', '00:00:5e:00:54:32'), + dev('eth3', 'pci-0000:00:06.0', '00:00:5e:00:54:3c'), + ] + _, new_store, report = resolve(devices, empty_store()) + self.assertEqual(set(new_store['interfaces']), + {'eth0', 'eth1', 'eth2', 'eth3'}) + names = final_names(report) + self.assertEqual(names['00:00:5e:00:54:31'], 'eth0') + self.assertEqual(names['00:00:5e:00:54:39'], 'eth1') + self.assertEqual(names['00:00:5e:00:54:32'], 'eth2') + + def test_config_node_without_hardware_does_not_block_the_others(self): + """2026-09-09: eth0/eth1/eth2 configured with no hw-id on a two-NIC + VM. Every interface was left pending, both NICs parked on eth3/eth4, + and the box was unreachable from the network. + """ + devices = [ + dev('eth0', 'pci-0000:00:03.0', '00:00:5e:00:54:a1'), + dev('eth1', 'pci-0000:00:04.0', '00:00:5e:00:54:a9'), + ] + plan, new_store, report = resolve(devices, empty_store()) + # the two NICs present take eth0 and eth1; a configured eth2 node + # simply has no hardware, which is a reporting matter not a naming one + self.assertEqual(plan, {}) + names = final_names(report) + self.assertEqual(names['00:00:5e:00:54:a1'], 'eth0') + self.assertEqual(names['00:00:5e:00:54:a9'], 'eth1') + self.assertNotIn('eth3', new_store['interfaces']) + + +class TestLinkFiles(unittest.TestCase): + """The .link layer is what removes the rename window: udev applies the + name as the device is added, so no probe-order name is ever observable. + """ + + def test_matches_the_same_property_the_store_keyed_on(self): + # udev must apply exactly the mapping recorded in the store - if the + # .link matched on anything else the two could disagree about what + # identifies an interface + body = render_link('eth1', 'ID_NET_NAME_SLOT=ens18') + self.assertIn('Property=ID_NET_NAME_SLOT=ens18', body) + self.assertIn('Name=eth1', body) + self.assertIn('Type=ether', body) + + def test_link_for_a_radio_matches_the_wireless_type(self): + # a .link naming a type the device is not never applies, so a radio + # rendered as 'ether' would silently keep its probe-order name + body = render_link('wlan0', 'ID_PATH=pci-0000:00:14.0') + self.assertIn('Type=wlan', body) + self.assertNotIn('Type=ether', body) + self.assertIn('Name=wlan0', body) + + def test_radio_named_by_resolve_gets_a_wireless_link(self): + # end to end: resolve() names by type, and the file written for that + # name has to agree with it + devices = [dev('wlan5', 'pci-0000:00:14.0', 'aa:aa:aa:aa:aa:01', + wireless=True), + dev('eth9', 'pci-0000:00:12.0', 'bb:bb:bb:bb:bb:02')] + _, store, _ = resolve(devices, empty_store()) + self.assertIn('wlan0', store['interfaces']) + with tempfile.TemporaryDirectory() as d: + written = sync_link_files(store, Path(d)) + bodies = {p.name: p.read_text() for p in written} + self.assertIn('Type=wlan', bodies['10-vyos-wlan0.link']) + self.assertIn('Type=ether', bodies['10-vyos-eth0.link']) + + def test_one_file_per_interface(self): + with tempfile.TemporaryDirectory() as d: + written = sync_link_files( + store_of({'eth1': 'pci-0000:00:13.0'}), Path(d)) + self.assertEqual([p.name for p in written], ['10-vyos-eth1.link']) + + def test_stale_files_are_removed(self): + with tempfile.TemporaryDirectory() as d: + link_dir = Path(d) + sync_link_files(store_of({'eth1': 'p1', + 'eth2': 'p2'}), link_dir) + sync_link_files(store_of({'eth1': 'p1'}), link_dir) + remaining = sorted(p.name for p in link_dir.glob('*.link')) + self.assertEqual(remaining, ['10-vyos-eth1.link']) + + def test_operator_files_are_left_alone(self): + with tempfile.TemporaryDirectory() as d: + link_dir = Path(d) + custom = link_dir / '99-my-tuning.link' + custom.write_text('[Match]\nType=ether\n\n[Link]\nMTUBytes=9000\n') + sync_link_files(store_of({'eth1': 'p1'}), link_dir) + self.assertTrue(custom.exists()) + + +class TestPermanentMac(unittest.TestCase): + """Reading the hardware's own address matters wherever a custom MAC may + be in effect: the current address is then the custom one. This used to + come from the hw-id node, which could also simply be stale. + """ + + def test_ethtool_permanent_address(self): + with mock.patch('vyos.ifconfig.ifname_store.rc_cmd', + return_value=(0, 'Permanent address: aa:bb:cc:dd:ee:ff\n')): + self.assertEqual(permanent_mac('eth0'), 'aa:bb:cc:dd:ee:ff') + + def test_zero_address_falls_back_to_sysfs(self): + # some drivers report an all-zero "unsupported" permanent address + # instead of failing outright + with mock.patch('vyos.ifconfig.ifname_store.rc_cmd', + return_value=(0, 'Permanent address: 00:00:00:00:00:00\n')), \ + mock.patch('pathlib.Path.read_text', return_value='11:22:33:44:55:66\n'): + self.assertEqual(permanent_mac('eth0'), '11:22:33:44:55:66') + + def test_ethtool_unsupported_falls_back_to_sysfs(self): + with mock.patch('vyos.ifconfig.ifname_store.rc_cmd', + return_value=(1, 'Operation not supported')), \ + mock.patch('pathlib.Path.read_text', return_value='11:22:33:44:55:66\n'): + self.assertEqual(permanent_mac('eth0'), '11:22:33:44:55:66') + + def test_no_mac_available_returns_empty(self): + with mock.patch('vyos.ifconfig.ifname_store.rc_cmd', return_value=(1, '')), \ + mock.patch('pathlib.Path.read_text', side_effect=OSError): + self.assertEqual(permanent_mac('eth0'), '') + + +class TestDiscoverDevices(unittest.TestCase): + """Only real, independently-addressable hardware is a naming candidate. + Virtual interfaces have no bus device; enslaved interfaces, SR-IOV VFs + and hypervisor VF datapaths are acceleration children that share another + interface's MAC, so naming them would invent a second interface for one + physical port. + """ + + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) + + for patch in (mock.patch('vyos.ifconfig.ifname_store.read_properties', + return_value={}), + mock.patch('vyos.ifconfig.ifname_store.rc_cmd', + return_value=(1, ''))): + patch.start() + self.addCleanup(patch.stop) + + def make(name, mac=None, has_device=True, enslaved=False, is_vf=False): + path = os.path.join(self.tmp, name) + os.mkdir(path) + if has_device: + os.mkdir(os.path.join(path, 'device')) + if is_vf: + pf = os.path.join(self.tmp, f'{name}_pf') + os.mkdir(pf) + os.symlink(pf, os.path.join(path, 'device', 'physfn')) + if enslaved: + master = os.path.join(self.tmp, f'{name}_master') + os.mkdir(master) + os.symlink(master, os.path.join(path, 'master')) + if mac: + with open(os.path.join(path, 'address'), 'w') as f: + f.write(mac + '\n') + + make('eth0', mac='aa:aa:aa:aa:aa:00') + make('eth1', mac='aa:aa:aa:aa:aa:01') + make('br0', mac='aa:aa:aa:aa:aa:99', has_device=False) + make('lo', has_device=False) + make('eth8', mac='aa:aa:aa:aa:aa:08', enslaved=True) + make('eth9', mac='aa:aa:aa:aa:aa:09', is_vf=True) + make('vf_eth0', mac='aa:aa:aa:aa:aa:00') + + def _names(self): + return {d['name'] for d in discover_devices(self.tmp)} + + def test_only_physical_independent_interfaces(self): + self.assertEqual(self._names(), {'eth0', 'eth1'}) + + def test_virtual_interfaces_excluded(self): + self.assertNotIn('br0', self._names()) + self.assertNotIn('lo', self._names()) + + def test_enslaved_interface_excluded(self): + self.assertNotIn('eth8', self._names()) + + def test_sriov_virtual_function_excluded(self): + # the general marker, independent of hypervisor - the previous + # exclusion relied on udev rules scoped to a Microsoft DMI vendor + # string and would miss a bare-metal Mellanox VF + self.assertNotIn('eth9', self._names()) + + def test_hypervisor_vf_datapath_excluded(self): + # parked as vf_* by 63-hyperv-vf-net.rules; shares its synthetic + # parent's MAC and must not be taken for a second interface + self.assertNotIn('vf_eth0', self._names()) diff --git a/src/tests/test_net_name_resolve.py b/src/tests/test_net_name_resolve.py new file mode 100644 index 000000000..e4473f49b --- /dev/null +++ b/src/tests/test_net_name_resolve.py @@ -0,0 +1,227 @@ +# Copyright VyOS maintainers and contributors +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 2 or later as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +# Coverage for T3871: the boot-time half of persistent interface naming - +# settling, renaming and boot ordering. +# +# The naming decision and hardware discovery both live in +# vyos.ifconfig.ifname_store and are covered by test_ifname_store.py. + +import os +import shutil +import tempfile +import unittest +from unittest import mock + +from helper import prepare_module + +_here = os.path.dirname(__file__) + +resolver = prepare_module( + os.path.join(_here, '../system/vyos-net-name-resolve.py'), + 'vyos_net_name_resolve') + +_container_patcher = None + + +def setUpModule(): + # main() deliberately short-circuits inside a container, and these tests + # are routinely run from one (the VyOS build container). Pin the detection + # off for the whole module so the naming pass under test actually runs - + # TestMainInContainer patches it back on locally where that is the point. + global _container_patcher + _container_patcher = mock.patch.object( + resolver, 'is_running_as_container', return_value=False) + _container_patcher.start() + + +def tearDownModule(): + _container_patcher.stop() + + +def device(name, mac, path='', wireless=False): + return {'name': name, 'mac': mac, 'path': path, 'wireless': wireless, + 'properties': {}} + + +class TestWaitForSettle(unittest.TestCase): + """There is no specific MAC to wait for when naming hardware the store + has never seen, so settling is detected as N consecutive identical + discover_physical_interfaces() snapshots instead. + """ + + def test_returns_after_n_consecutive_stable_polls(self): + one = [device('eth0', 'm0')] + two = [device('eth0', 'm0'), device('eth1', 'm1')] + snapshots = [one, two, two, two] + with mock.patch.object(resolver, 'discover_devices', + side_effect=snapshots): + result = resolver.wait_for_settle([], timeout=5, poll=0, + stable_polls=3) + self.assertEqual(resolver._snapshot(result), + {('eth0', 'm0'), ('eth1', 'm1')}) + + def test_bounded_by_timeout_when_never_stable(self): + counter = iter(range(1000)) + + def ever_changing(*_a, **_kw): + return [device('eth0', f'm{next(counter)}')] + + clock = iter([0, 0.1, 20, 20]) + with mock.patch.object(resolver, 'discover_devices', + side_effect=ever_changing), \ + mock.patch('time.monotonic', side_effect=lambda: next(clock)): + result = resolver.wait_for_settle([], timeout=10, poll=0, + stable_polls=3) + # must return SOMETHING (the last snapshot seen), not hang or crash + self.assertEqual(len(result), 1) + + def test_stable_polls_of_one_returns_on_initial_snapshot(self): + initial = [device('eth0', 'm0')] + with mock.patch.object(resolver, 'discover_devices') as m: + result = resolver.wait_for_settle(initial, timeout=5, poll=0, + stable_polls=1) + m.assert_not_called() + self.assertEqual(result, initial) + + +class TestSafeBulkRename(unittest.TestCase): + """Only hardware the store has never seen should reach this on a settled + system, but when it does the two-phase rename must report accurately: a + phase-2 (scratch -> final target) failure must not be reported as a + successful rename, and must not leave the interface down indefinitely + under a name nothing else knows about. + """ + + def setUp(self): + patcher = mock.patch.object(resolver, 'get_ifindex', + side_effect=lambda name: name) + patcher.start() + self.addCleanup(patcher.stop) + + def test_all_renames_succeed(self): + with mock.patch.object(resolver, 'run', return_value=0): + applied = resolver.safe_bulk_rename({'eth5': 'eth0'}) + self.assertEqual(applied, {'eth5': 'eth0'}) + + def test_phase_two_failure_is_not_reported_as_applied(self): + def fake_run(command, *_a, **_kw): + return 1 if command == 'ip link set dev vyetheth5 name eth0' else 0 + + with mock.patch.object(resolver, 'run', side_effect=fake_run): + applied = resolver.safe_bulk_rename({'eth5': 'eth0'}) + + self.assertNotIn('eth5', applied) + + def test_phase_two_failure_brings_scratch_name_back_up(self): + calls = [] + + def fake_run(command, *_a, **_kw): + calls.append(command) + return 1 if command == 'ip link set dev vyetheth5 name eth0' else 0 + + with mock.patch.object(resolver, 'run', side_effect=fake_run): + resolver.safe_bulk_rename({'eth5': 'eth0'}) + + self.assertIn('ip link set dev vyetheth5 up', calls) + self.assertNotIn('ip link set dev eth0 up', calls) + + def test_one_failure_does_not_affect_other_renames_in_the_batch(self): + def fake_run(command, *_a, **_kw): + return 1 if command == 'ip link set dev vyetheth5 name eth0' else 0 + + with mock.patch.object(resolver, 'run', side_effect=fake_run): + applied = resolver.safe_bulk_rename({'eth5': 'eth0', + 'eth9': 'eth1'}) + + self.assertNotIn('eth5', applied) + self.assertEqual(applied.get('eth9'), 'eth1') + + +class TestMainInContainer(unittest.TestCase): + """A container owns no NIC: its interfaces are runtime-created veth pairs + with no backing bus device in sysfs and a host-assigned MAC that changes + on every start. There is nothing to name and nothing worth persisting. + """ + + def setUp(self): + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + self._orig_status_file = resolver.status_file + resolver.status_file = resolver.Path(status_dir) / 'status.json' + self.addCleanup(setattr, resolver, 'status_file', + self._orig_status_file) + + def test_naming_pass_is_skipped_entirely(self): + with mock.patch.object(resolver, 'is_running_as_container', + return_value=True), \ + mock.patch.object(resolver, 'load_store') as load, \ + mock.patch.object(resolver, 'discover_devices') as discover, \ + mock.patch.object(resolver, 'save_store') as save, \ + mock.patch.object(resolver, 'run') as run: + resolver.main() + + load.assert_not_called() + discover.assert_not_called() + save.assert_not_called() + run.assert_not_called() + self.assertFalse(resolver.status_file.exists()) + + +class TestBootOrdering(unittest.TestCase): + """The store lives in the config directory, which does not exist until + vyos-router mounts (and, for an encrypted config volume, decrypts) it. So + the pass must be started explicitly from inside vyos-router after that + point, and must not be reintroduced as an independently-scheduled unit + ordered before it. + """ + + def setUp(self): + path = os.path.join(_here, '../init/vyos-router') + with open(path) as f: + self.lines = f.readlines() + + def _first_index(self, needle): + for i, line in enumerate(self.lines): + if needle in line: + return i + self.fail(f"'{needle}' not found in src/init/vyos-router") + + def test_resolver_runs_after_migrate_and_before_config_apply(self): + # search for call SITES, not the function definitions further up + migrate = self._first_index('migrate_bootfile || overall_status=1') + resolve = self._first_index( + 'systemctl start vyos-net-name-resolve.service') + update_iface = self._first_index( + 'update_interface_config || overall_status=1') + load_boot = self._first_index('disabled configure || load_bootfile') + + self.assertLess(migrate, resolve, + 'resolver must run after config.boot is current') + self.assertLess(resolve, update_iface, + 'resolver must write the store before the rescan reads it') + self.assertLess(resolve, load_boot, + 'resolver must run before the CLI config is applied') + + def test_service_unit_not_independently_ordered_before_vyos_router(self): + path = os.path.join(_here, '../systemd/vyos-net-name-resolve.service') + with open(path) as f: + unit = f.read() + self.assertNotIn('Before=vyos-router.service', unit) + self.assertNotIn('Before=network-pre.target', unit) + self.assertNotIn('WantedBy=', unit) + + +if __name__ == '__main__': + unittest.main() diff --git a/src/udev/vyos_net_name b/src/udev/vyos_net_name deleted file mode 100755 index f5c3b9f1b..000000000 --- a/src/udev/vyos_net_name +++ /dev/null @@ -1,276 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright VyOS maintainers and contributors -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License version 2 or later as -# published by the Free Software Foundation. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program. If not, see . - -import os -import re -import time -import logging -import logging.handlers -import tempfile -from pathlib import Path -from sys import argv - -from vyos.configtree import ConfigTree -from vyos.defaults import directories -from vyos.utils.process import cmdl -from vyos.utils.boot import boot_configuration_complete -from vyos.utils.locking import Lock -from vyos.migrate import ConfigMigrate - -# Define variables -vyos_udev_dir = directories['vyos_udev_dir'] -config_path = '/opt/vyatta/etc/config/config.boot' - - -def is_available(intfs: dict, intf_name: str) -> bool: - """Check if interface name is already assigned""" - if intf_name in list(intfs.values()): - return False - return True - - -def find_available(intfs: dict, prefix: str) -> str: - """Find lowest indexed interface name that is not assigned""" - index_list = [ - int(x.replace(prefix, '')) for x in list(intfs.values()) if prefix in x - ] - index_list.sort() - # find 'holes' in list, if any - missing = sorted(set(range(index_list[0], index_list[-1])) - set(index_list)) - if missing: - return f'{prefix}{missing[0]}' - - return f'{prefix}{len(index_list)}' - - -def mod_ifname(ifname: str) -> str: - """Check interface with names eX and return ifname on the next format eth{ifindex} - 2""" - if re.match('^e[0-9]+$', ifname): - intf = ifname.split('e') - if intf[1]: - if int(intf[1]) >= 2: - return 'eth' + str(int(intf[1]) - 2) - else: - return 'eth' + str(intf[1]) - - return ifname - - -def get_biosdevname(ifname: str) -> str: - """Use legacy vyatta-biosdevname to query for name - - This is carried over for compatibility only, and will likely be dropped - going forward. - XXX: This throws an error, and likely has for a long time, unnoticed - since vyatta_net_name redirected stderr to /dev/null. - """ - intf = mod_ifname(ifname) - - if 'eth' not in intf: - return intf - if os.path.isdir('/proc/xen'): - return intf - - time.sleep(1) - - try: - biosname = cmdl(['/sbin/biosdevname', '--policy', 'all_ethN', '-i', ifname]) - except Exception as e: - logger.error(f'biosdevname error: {e}') - biosname = '' - - return intf if biosname == '' else biosname - - -def leave_rescan_hint(intf_name: str, hwid: str): - """Write interface information reported by udev - - This script is called while the root mount is still read-only. Leave - information in /run/udev: file name, the interface; contents, the - hardware id. - """ - try: - os.mkdir(vyos_udev_dir) - except FileExistsError: - pass - except Exception as e: - logger.critical(f'Error creating rescan hint directory: {e}') - exit(1) - - try: - with open(os.path.join(vyos_udev_dir, intf_name), 'w') as f: - f.write(hwid) - except OSError as e: - logger.critical(f'OSError {e}') - - -def get_configfile_interfaces() -> dict: - """Read existing interfaces from config file""" - interfaces: dict = {} - - if not os.path.isfile(config_path): - # If the case, then we are running off of livecd; return empty - return interfaces - - try: - with open(config_path) as f: - config_file = f.read() - except OSError as e: - logger.critical(f'OSError {e}') - exit(1) - - try: - config = ConfigTree(config_file) - except Exception: - try: - logger.debug('updating component version string syntax') - # this will update the component version string syntax, - # required for updates 1.2 --> 1.3/1.4 - with tempfile.NamedTemporaryFile() as fp: - with open(fp.name, 'w') as fd: - fd.write(config_file) - config_migrate = ConfigMigrate(fp.name) - if config_migrate.syntax_update_needed(): - config_migrate.update_syntax() - config_migrate.write_config() - with open(fp.name) as fd: - config_file = fd.read() - - config = ConfigTree(config_file) - - except Exception as e: - logger.critical(f'ConfigTree error: {e}') - exit(1) - - base = ['interfaces', 'ethernet'] - if config.exists(base): - eth_intfs = config.list_nodes(base) - for intf in eth_intfs: - path = base + [intf, 'hw-id'] - if not config.exists(path): - logger.warning(f"no 'hw-id' entry for {intf}") - continue - hwid = config.return_value(path) - if hwid in list(interfaces): - logger.warning( - f'multiple entries for {hwid}: {interfaces[hwid]}, {intf}' - ) - continue - interfaces[hwid] = intf - - base = ['interfaces', 'wireless'] - if config.exists(base): - wlan_intfs = config.list_nodes(base) - for intf in wlan_intfs: - path = base + [intf, 'hw-id'] - if not config.exists(path): - logger.warning(f"no 'hw-id' entry for {intf}") - continue - hwid = config.return_value(path) - if hwid in list(interfaces): - logger.warning( - f'multiple entries for {hwid}: {interfaces[hwid]}, {intf}' - ) - continue - interfaces[hwid] = intf - - logger.debug(f'config file entries: {interfaces}') - - return interfaces - - -def add_assigned_interfaces(intfs: dict): - """Add interfaces found by previous invocation of udev rule""" - if not os.path.isdir(vyos_udev_dir): - return - - for intf in os.listdir(vyos_udev_dir): - path = os.path.join(vyos_udev_dir, intf) - try: - with open(path) as f: - hwid = f.read().rstrip() - except OSError as e: - logger.error(f'OSError {e}') - continue - intfs[hwid] = intf - - -def on_boot_event(intf_name: str, hwid: str, predefined: str = '') -> str: - """Called on boot by vyos-router: 'coldplug' in vyatta_net_name""" - logger.info(f'lookup {intf_name}, {hwid}') - interfaces = get_configfile_interfaces() - logger.debug(f'config file interfaces are {interfaces}') - - if hwid in list(interfaces): - logger.info(f"use mapping from config file: '{hwid}' -> '{interfaces[hwid]}'") - return interfaces[hwid] - - add_assigned_interfaces(interfaces) - logger.debug(f'adding assigned interfaces: {interfaces}') - - if predefined: - newname = predefined - logger.info(f"predefined interface name for '{intf_name}' is '{newname}'") - else: - newname = get_biosdevname(intf_name) - logger.info(f"biosdevname returned '{newname}' for '{intf_name}'") - - if not is_available(interfaces, newname): - prefix = re.sub(r'\d+$', '', newname) - newname = find_available(interfaces, prefix) - - logger.info(f"new name for '{intf_name}' is '{newname}'") - - leave_rescan_hint(newname, hwid) - - return newname - - -def hotplug_event(): - # Not yet implemented, since interface-rescan will only be run on boot. - pass - - -if __name__ == '__main__': - # Set up logging to syslog - syslog_handler = logging.handlers.SysLogHandler(address='/dev/log') - formatter = logging.Formatter(f'{Path(__file__).name}: %(message)s') - syslog_handler.setFormatter(formatter) - - logger = logging.getLogger() - logger.addHandler(syslog_handler) - logger.setLevel(logging.DEBUG) - - logger.debug(f'Started with arguments: {argv}') - - if len(argv) > 3: - predef_name = argv[3] - else: - predef_name = '' - - lock = Lock('vyos_net_name') - # Wait 60 seconds for other running scripts to finish - lock.acquire(60) - - if not boot_configuration_complete(): - res = on_boot_event(argv[1], argv[2], predefined=predef_name) - logger.debug(f'on boot, returned name is {res}') - print(res) - else: - logger.debug('boot configuration complete') - - lock.release() - logger.debug('Finished') -- cgit v1.2.3 From 153dd688b327c922ec4b371db1fdd11b8ee8cf9e Mon Sep 17 00:00:00 2001 From: Alex Kudentsov <43482574+alexk37@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:46:08 +0700 Subject: interfaces: T3871: give a re-created Hyper-V NIC the name of the NIC it replaced A Hyper-V NIC is known by its MAC alone. One deleted and created again on the same VM - an Azure NIC re-created, or a Hyper-V adapter removed and added - comes back with a new MAC, so nothing on it matched the store: its name stayed reserved, the new NIC got a new name, and its configuration was left on a name with no NIC. cloud-init does not run again on the same instance, so nothing else repairs it. At boot, a vacant name now goes to a new NIC only when the pairing is not in doubt: - exactly one recorded name has no NIC, and exactly one new Hyper-V NIC appeared. With more of either, the order the NICs come up in does not say which replaced which, so nothing is handed over and the names stay reserved, as before; - the name went missing at this pass. The store records which names were missing at the last pass, so a NIC removed for good does not hand its name to a NIC added at a later boot; - the name's own NIC was a Hyper-V one. The store records which names sat on a Hyper-V NIC; a MAC-keyed name can belong to other hardware, and a store written before this was recorded hands nothing over (the boot pass logs that when it holds a name back); - only at boot. On a running system the configuration under a name is not applied to the NIC that takes it, so the hotplug unit passes --hotplug, which names the new NIC like any other and records the vacant name as missing, so the next boot does not hand it over either. An adoption is logged as a warning, reported under 'adopted' in the status file (kept there across hotplug passes while it holds), and repeated by vyos-router, as a slot match is: verify the port before relying on its configuration. --- python/vyos/ifconfig/ifname_store.py | 89 +++++++++- src/init/vyos-router | 6 + src/system/vyos-net-name-resolve.py | 56 ++++++- src/systemd/vyos-net-name-resolve-hotplug.service | 2 +- src/systemd/vyos-net-name-resolve.service | 4 +- src/tests/test_ifname_store.py | 191 +++++++++++++++++++++- src/tests/test_net_name_resolve.py | 157 ++++++++++++++++++ 7 files changed, 487 insertions(+), 18 deletions(-) (limited to 'src/init') diff --git a/python/vyos/ifconfig/ifname_store.py b/python/vyos/ifconfig/ifname_store.py index 20731aed1..961a81d15 100644 --- a/python/vyos/ifconfig/ifname_store.py +++ b/python/vyos/ifconfig/ifname_store.py @@ -302,8 +302,25 @@ def load_store(path: Path = None) -> dict: if isinstance(mac, str) and mac and name in interfaces } if isinstance(seen, dict) else {} - return {'version': STORE_VERSION, 'interfaces': interfaces, - 'hardware': hardware} + store = {'version': STORE_VERSION, 'interfaces': interfaces, 'hardware': hardware} + + # names whose hardware was missing at the last pass - kept only while + # there are any, so a store without them reads as it always did + absent = data.get('absent') + if isinstance(absent, list): + absent = sorted({n for n in absent if n in interfaces}) + if absent: + store['absent'] = absent + + # names whose NIC was a Hyper-V one when last seen - the only names a new + # Hyper-V NIC may take over. A store without them (written before they + # were recorded) has none. + hyperv = data.get('hyperv') + if isinstance(hyperv, list): + hyperv = sorted({n for n in hyperv if n in interfaces}) + if hyperv: + store['hyperv'] = hyperv + return store def encrypted_config_volume() -> bool: @@ -411,12 +428,15 @@ def _next_free(taken: set, prefix: str) -> str: return f'{prefix}{index}' -def resolve(devices: list, store: dict) -> tuple: +def resolve(devices: list, store: dict, adopt: bool = True) -> tuple: """Name every physical interface present this boot. Returns (plan, new_store, report); plan is {current: new} for the interfaces which have to be renamed. The slot decides the name - nothing here looks at the configuration, so there is nothing to guess. + + adopt=False keeps a vacant Hyper-V name from going to a new NIC, for a + pass on a running system (see the adoption block below). """ entries = store.get('interfaces', {}) seen = store.get('hardware', {}) @@ -436,13 +456,23 @@ def resolve(devices: list, store: dict) -> tuple: seen = {n: m for n, m in seen.items() if n not in misfiled} assigned = {} # final name -> device - report = {'matched': {}, 'reassigned': {}, 'moved': {}, - 'replaced': {}, 'bootstrapped': {}} + report = { + 'matched': {}, + 'reassigned': {}, + 'moved': {}, + 'replaced': {}, + 'adopted': {}, + 'bootstrapped': {}, + } # A recorded name stays off-limits even while its slot is empty, so new # hardware cannot inherit an absent interface's name and configuration. reserved = set(entries) remaining = list(devices) + # what the last pass recorded: names already missing then, and names + # whose NIC was a Hyper-V one + missing_before = set(store.get('absent', [])) + hyperv_before = set(store.get('hyperv', [])) def claim(device, name, bucket): assigned[name] = device @@ -500,6 +530,36 @@ def resolve(devices: list, store: dict) -> tuple: claim(device, name, 'replaced') break + # a Hyper-V NIC is known by its MAC alone, and one deleted and created + # again comes back with a new MAC, so nothing on it could match. When one + # recorded NIC is gone and one new NIC took its place the pairing is not in + # doubt, and the name goes to it. With more of either it is: the order the + # NICs come up in here does not say which replaced which, so nothing is + # handed over and the names stay reserved. + # Only on the pass the NIC first went missing: a replacement is one change + # to the VM. A name already missing at the last pass belongs to a NIC that + # was removed, and a NIC added later is not its replacement. + # Only at boot: on a running system the configuration under the name is + # not applied to the NIC that takes it, so the name would point at a port + # that is down and unaddressed until the next boot. The new NIC gets a new + # name there, and the vacant one is recorded as missing, so the next boot + # does not hand it over either. + # Only a name whose own NIC was a Hyper-V one: a MAC-keyed name can + # belong to other hardware too, and a store that does not say (written + # before this was recorded) hands nothing over. + vacant = [ + name + for name, key in entries.items() + if name not in assigned + and name not in missing_before + and name in hyperv_before + and key.startswith('ID_NET_NAME_MAC=') + ] + adoptable = [device for device in remaining if is_hyperv_nic(device)] + if (adopt and len(vacant) == 1 and len(adoptable) == 1 + and _name_fits(vacant[0], adoptable[0])): + claim(adoptable[0], vacant[0], 'adopted') + # unknown hardware: lowest free name of its type, in hardware order for device in sorted(remaining, key=canonical_sort_key): name = _next_free(reserved | set(assigned), _prefix_for(device)) @@ -521,8 +581,23 @@ def resolve(devices: list, store: dict) -> tuple: plan = {device['name']: name for name, device in assigned.items() if device['name'] != name} - return plan, {'version': STORE_VERSION, 'interfaces': new_interfaces, - 'hardware': new_hardware}, report + new_store = { + 'version': STORE_VERSION, + 'interfaces': new_interfaces, + 'hardware': new_hardware, + } + absent = sorted(name for name in new_interfaces if name not in assigned) + if absent: + new_store['absent'] = absent + # what each name's NIC was, kept for an absent one like its address + hyperv = sorted( + {name for name, device in assigned.items() if is_hyperv_nic(device)} + | (hyperv_before & set(absent)) + ) + if hyperv: + new_store['hyperv'] = hyperv + + return plan, new_store, report LINK_DIR = Path('/etc/systemd/network') diff --git a/src/init/vyos-router b/src/init/vyos-router index ff1de0584..8978d4157 100755 --- a/src/init/vyos-router +++ b/src/init/vyos-router @@ -385,6 +385,12 @@ warn_missing_interface_hardware () log_warning_msg "interface '${intf}' was matched to new hardware ${hwid} by slot - verify it is the port you expect before relying on its configuration" done + jq -r '.adopted // {} | to_entries[] | "\(.key) \(.value)"' "${status_file}" \ + 2>/dev/null | while read -r intf hwid; do + [ -n "${intf}" ] || continue + log_warning_msg "interface '${intf}' had no NIC this boot and was given to the new Hyper-V NIC ${hwid} - verify it is the port you expect before relying on its configuration" + done + if [ "$(jq -r '.persisted // true' "${status_file}" 2>/dev/null)" = "false" ]; then log_failure_msg "interface names could not be persisted - the configuration volume is not mounted" fi diff --git a/src/system/vyos-net-name-resolve.py b/src/system/vyos-net-name-resolve.py index d74663707..f3e4e62fb 100755 --- a/src/system/vyos-net-name-resolve.py +++ b/src/system/vyos-net-name-resolve.py @@ -26,6 +26,7 @@ import json import logging import logging.handlers +import sys import time from pathlib import Path @@ -182,16 +183,21 @@ def write_status(store: dict, devices: list, applied: dict, report: dict, # in use, so left alone until the next boot 'rename_refused': refused or {}, # everything recognised from the store, however it was recognised - 'matched': {**report.get('matched', {}), - **report.get('reassigned', {}), - **report.get('moved', {}), - **report.get('replaced', {})}, + 'matched': { + **report.get('matched', {}), + **report.get('reassigned', {}), + **report.get('moved', {}), + **report.get('replaced', {}), + **report.get('adopted', {}), + }, # the name changed hands between cards the store already knows 'reassigned': report.get('reassigned', {}), # the card was found in a different slot - certain, it is the same card 'moved': report.get('moved', {}), # a different card took the slot - the one case that needs verifying 'matched_by_path': report.get('replaced', {}), + # a recorded Hyper-V NIC was gone and its one replacement took its name + 'adopted': report.get('adopted', {}), 'new_hardware': report.get('bootstrapped', {}), 'persisted': persisted, } @@ -201,7 +207,25 @@ def write_status(store: dict, devices: list, applied: dict, report: dict, logger.error(f'could not write status file: {e}') -def main(): +def boot_adoptions(store: dict) -> dict: + """The adoptions the boot pass reported that still hold. A pass for + hot-plugged hardware adopts nothing, and must not wipe the record of + what this boot did.""" + try: + adopted = json.loads(status_file.read_text()).get('adopted') or {} + except (OSError, ValueError, AttributeError): + return {} + hardware = store.get('hardware', {}) + return {name: mac for name, mac in adopted.items() + if hardware.get(name) == mac} + + +def hotplug_requested(argv: list) -> bool: + """--hotplug: started for hardware added to a running system""" + return '--hotplug' in argv + + +def main(hotplug: bool = False): if is_running_as_container(): # a container owns no NIC - its veth pairs have no backing bus device logger.info('running inside a container - skipping interface naming') @@ -229,7 +253,16 @@ def main(): 'reserved.' ) - plan, store, report = resolve(devices, store) + if not hotplug and 'hyperv' not in store and any( + key.startswith('ID_NET_NAME_MAC=') and key in absent + for key in store['interfaces'].values()): + # a name is vacant, and the map was written by code that did not + # record which NICs were Hyper-V ones: the name is not handed over. + # Each pass records the Hyper-V NICs it finds, but never a vacant name + logger.info('the interface map does not say which interfaces were ' + 'Hyper-V NICs - no vacant name is given to a new NIC ' + 'this boot') + plan, store, report = resolve(devices, store, adopt=not hotplug) for name, mac in sorted(report.get('bootstrapped', {}).items()): logger.info(f"new hardware {mac} named '{name}'") @@ -242,6 +275,12 @@ def main(): f"'{name}' was matched to new hardware {mac} by slot - verify it " 'is the port you expect before relying on its configuration' ) + for name, mac in sorted(report.get('adopted', {}).items()): + logger.warning( + f"'{name}' had no NIC this boot and was given to the new Hyper-V " + f'NIC {mac}, which took its place - verify it is the port you ' + 'expect before relying on its configuration' + ) plan, refused = refuse_renames_in_use(plan) for old, target in sorted(refused.items()): logger.warning( @@ -263,6 +302,9 @@ def main(): 'resolved for this boot but could not be persisted' ) + if hotplug: + # reported once, by the boot pass; kept on record until the next boot + report = {**report, 'adopted': boot_adoptions(store)} write_status(store, devices, applied, report, persisted, refused) @@ -274,4 +316,4 @@ if __name__ == '__main__': logger.addHandler(syslog_handler) logger.setLevel(logging.DEBUG) - main() + main(hotplug=hotplug_requested(sys.argv[1:])) diff --git a/src/systemd/vyos-net-name-resolve-hotplug.service b/src/systemd/vyos-net-name-resolve-hotplug.service index c798feb99..d52fc6061 100644 --- a/src/systemd/vyos-net-name-resolve-hotplug.service +++ b/src/systemd/vyos-net-name-resolve-hotplug.service @@ -8,6 +8,6 @@ ConditionPathExists=/tmp/vyos-config-status [Service] Type=oneshot -ExecStart=/usr/libexec/vyos/system/vyos-net-name-resolve.py +ExecStart=/usr/libexec/vyos/system/vyos-net-name-resolve.py --hotplug TimeoutStartSec=90 StandardOutput=journal diff --git a/src/systemd/vyos-net-name-resolve.service b/src/systemd/vyos-net-name-resolve.service index b02d22cf7..8ad1c3418 100644 --- a/src/systemd/vyos-net-name-resolve.service +++ b/src/systemd/vyos-net-name-resolve.service @@ -4,8 +4,8 @@ Documentation=man:vyos-net-name-resolve.py(8) # Started by vyos-router once the config directory is mounted - for an # encrypted volume that is only after it has been unlocked - so this can not -# be ordered independently. Started again for hot-plugged hardware, hence no -# RemainAfterExit, which would make every later start a no-op. +# be ordered independently. This is the boot pass; hot-plugged hardware is +# named by vyos-net-name-resolve-hotplug.service, which passes --hotplug. DefaultDependencies=no [Service] diff --git a/src/tests/test_ifname_store.py b/src/tests/test_ifname_store.py index ff0f916ec..635033fff 100644 --- a/src/tests/test_ifname_store.py +++ b/src/tests/test_ifname_store.py @@ -199,7 +199,9 @@ def keyed(devices): class TestHyperV(unittest.TestCase): """Azure, four NICs, measured on a fresh deployment of this series: every Hyper-V NIC shares one ID_PATH and has no slot, so its MAC is its key. - The order Azure lists the NICs in is cloud-init's to record. + The order Azure lists the NICs in is cloud-init's to record; what is left + here is a NIC deleted and created again on the same VM, where cloud-init + does not run again. """ AZURE = [ @@ -231,6 +233,162 @@ class TestHyperV(unittest.TestCase): ), ) + def test_a_recreated_nic_takes_the_name_of_the_one_it_replaced(self): + # LAN3's NIC deleted and created again: new MAC. It came up as eth4, + # eth3 stayed reserved and the boot commit failed on it every boot. + store = self.stored() + devices = self.nics() + devices[3] = keyed([hv('eth3', '00:0d:3a:47:aa:01')] + devices[:3])[0] + plan, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {'eth3': '00:0d:3a:47:aa:01'}) + self.assertEqual(report['bootstrapped'], {}) + self.assertEqual(plan, {}) + self.assertEqual(store['interfaces']['eth3'], 'ID_NET_NAME_MAC=enx000d3a47aa01') + + def test_two_recreated_nics_are_not_paired_by_guess(self): + # nothing here says which new NIC replaced which, and pairing them by + # the order they came up in can swap two subnets' configuration + store = self.stored() + devices = self.nics()[:2] + keyed( + [hv('eth3', '00:0d:3a:99:00:11'), hv('eth2', '00:0d:3a:99:00:33')] + ) + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(set(report['bootstrapped']), {'eth4', 'eth5'}) + self.assertIn('eth2', store['interfaces']) + self.assertIn('eth3', store['interfaces']) + + def test_two_vacancies_and_one_new_nic_are_not_paired(self): + # which of two gone NICs the one new NIC replaced is not known + store = self.stored() + devices = self.nics()[:2] + keyed( + [hv('eth3', '00:0d:3a:99:00:11')] + self.nics()[:2])[:1] + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:99:00:11'}) + self.assertEqual(store.get('absent'), ['eth2', 'eth3']) + + def test_a_new_nic_with_no_vacancy_takes_a_new_name(self): + store = self.stored() + devices = self.nics() + keyed([hv('eth4', '00:0d:3a:48:00:09')]) + _, _, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:48:00:09'}) + + def test_one_vacancy_and_two_new_nics_are_not_paired(self): + store = self.stored() + devices = self.nics()[:3] + keyed( + [hv('eth3', '00:0d:3a:48:00:01'), hv('eth4', '00:0d:3a:48:00:02')] + ) + _, _, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + + def test_a_disk_on_all_new_nics_is_not_handed_over_here(self): + # every recorded NIC gone at once: the platform's order is cloud-init's + # to supply (it runs again on a new instance); guessing it here could + # put the WAN's configuration on a LAN + store = self.stored() + new = keyed([hv(f'eth{i}', f'00:0d:3a:48:00:0{i}') for i in range(4)]) + _, _, report = resolve(new, store) + self.assertEqual(report.get('adopted', {}), {}) + + def test_a_slot_keyed_name_is_never_adopted(self): + # a PCI card has a slot to come back to; its name stays reserved + devices = [dev('eth0', 'pci-0000:00:03.0', 'aa:00')] + _, store, _ = resolve(devices, empty_store()) + other = keyed([hv('eth1', '00:0d:3a:48:00:09')]) + _, store, report = resolve(other, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth1': '00:0d:3a:48:00:09'}) + + def test_a_nic_added_after_one_was_removed_is_not_its_replacement(self): + # P8, measured on Azure: LAN3's NIC deleted for good (one boot), a NIC + # on another subnet added later (the next boot). Taking the vacant + # name put LAN3's configuration on the other subnet. + store = self.stored() + devices = self.nics()[:3] + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + later = devices + keyed([hv('eth4', '38:33:c5:cc:a3:3e')]) + _, store, report = resolve(later, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '38:33:c5:cc:a3:3e'}) + self.assertEqual(store.get('absent'), ['eth3']) + + def test_a_nic_replaced_on_a_running_system_is_not_adopted(self): + # measured on Hyper-V: a NIC hot-added after another was hot-removed + # took its name at once, while the configuration under that name + # stayed unapplied until the next boot + store = self.stored() + devices = self.nics() + devices[3] = keyed([hv('eth3', '00:0d:3a:47:aa:01')] + devices[:3])[0] + _, store, report = resolve(devices, store, adopt=False) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:47:aa:01'}) + self.assertEqual(store.get('absent'), ['eth3']) + # nor at the next boot: the name was already missing at the last pass + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['matched'].get('eth4'), '00:0d:3a:47:aa:01') + + def test_a_name_whose_nic_was_not_hyperv_is_not_handed_over(self): + # a MAC-keyed name can belong to other hardware sharing the VMBus + # path; only a Hyper-V NIC's name goes to a Hyper-V replacement + other = hv('eth1', '00:0d:3a:47:3b:29') + other['properties']['ID_NET_DRIVER'] = 'mlx5_core' + devices = keyed([hv('eth0', '00:0d:3a:47:38:1d'), other]) + _, store, _ = resolve(devices, empty_store()) + self.assertEqual(store.get('hyperv'), ['eth0']) + later = keyed([hv('eth0', '00:0d:3a:47:38:1d'), + hv('eth1', '00:0d:3a:48:00:09')]) + _, _, report = resolve(later, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth2': '00:0d:3a:48:00:09'}) + + def test_only_a_hyperv_nic_takes_a_vacant_name(self): + store = self.stored() + new = hv('eth3', '00:0d:3a:48:00:09') + new['properties']['ID_NET_DRIVER'] = 'virtio_net' + devices = self.nics()[:3] + keyed([new] + self.nics()[:3])[:1] + _, _, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:48:00:09'}) + + def test_a_new_nic_takes_no_name_of_another_namespace(self): + store = self.stored() + new = keyed([hv('wlan0', '00:0d:3a:48:00:09')] + self.nics()[:3])[0] + new['wireless'] = True + _, _, report = resolve(self.nics()[:3] + [new], store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'wlan0': '00:0d:3a:48:00:09'}) + + def test_a_store_that_does_not_say_which_nics_were_hyperv_hands_nothing_over(self): + # written by code that did not record it: whether the vacant name + # was a Hyper-V NIC's, or how long it has been vacant, is unknown + store = self.stored() + store.pop('hyperv') + devices = self.nics() + devices[3] = keyed([hv('eth3', '00:0d:3a:47:aa:01')] + devices[:3])[0] + _, store, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {}) + self.assertEqual(report['bootstrapped'], {'eth4': '00:0d:3a:47:aa:01'}) + self.assertEqual(store.get('absent'), ['eth3']) + + def test_the_hyperv_record_survives_a_load_and_follows_the_nics(self): + store = self.stored() + self.assertEqual(store['hyperv'], ['eth0', 'eth1', 'eth2', 'eth3']) + with tempfile.TemporaryDirectory() as d: + path = Path(d) / 'map.json' + with mock.patch('vyos.ifconfig.ifname_store.config_dir_is_mounted', + return_value=True), \ + mock.patch('vyos.ifconfig.ifname_store.get_cfg_group_id', + return_value=0): + save_store(store, path) + self.assertEqual(load_store(path)['hyperv'], store['hyperv']) + # an absent NIC keeps its record, so its replacement is recognised + _, store, _ = resolve(self.nics()[:3], store) + self.assertEqual(store['hyperv'], ['eth0', 'eth1', 'eth2', 'eth3']) + def test_a_lone_hyperv_nic_keyed_on_its_path_still_pins_its_driver(self): # one NIC has the VMBus path to itself, so it is keyed on it; its VF # must not match its file any more than with a MAC key @@ -242,6 +400,37 @@ class TestHyperV(unittest.TestCase): body = (Path(d) / '10-vyos-eth0.link').read_text() self.assertIn('Driver=hv_netvsc\n', body) + def test_a_store_without_the_record_still_adopts(self): + # written before names missing at a pass were recorded: nothing was + # missing then, as far as it can say, so a one-step replacement holds + store = self.stored() + store.pop('absent', None) + devices = self.nics() + devices[3] = keyed([hv('eth3', '00:0d:3a:47:aa:01')] + devices[:3])[0] + _, _, report = resolve(devices, store) + self.assertEqual(report.get('adopted', {}), {'eth3': '00:0d:3a:47:aa:01'}) + + def test_the_record_clears_when_the_nic_is_back(self): + store = self.stored() + _, store, _ = resolve(self.nics()[:3], store) + self.assertEqual(store.get('absent'), ['eth3']) + _, store, report = resolve(self.nics(), store) + self.assertNotIn('absent', store) + self.assertEqual(report['matched']['eth3'], self.AZURE[3][1]) + + def test_the_record_survives_a_load(self): + tmp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, tmp) + path = tmp / 'interface-mapping.json' + store = self.stored() + _, store, _ = resolve(self.nics()[:3], store) + path.write_text(json.dumps(store)) + self.assertEqual(load_store(path).get('absent'), ['eth3']) + # a name the map no longer has is dropped from it + store['absent'] = ['eth3', 'eth9'] + path.write_text(json.dumps(store)) + self.assertEqual(load_store(path).get('absent'), ['eth3']) + def test_link_file_pins_the_driver_for_a_mac_key(self): # the accelerated-networking VF carries its NIC's MAC body = render_link('eth0', 'ID_NET_NAME_MAC=enx000d3a47381d', 'hv_netvsc') diff --git a/src/tests/test_net_name_resolve.py b/src/tests/test_net_name_resolve.py index 62236219a..b529c7164 100644 --- a/src/tests/test_net_name_resolve.py +++ b/src/tests/test_net_name_resolve.py @@ -18,10 +18,12 @@ # The naming decision and hardware discovery both live in # vyos.ifconfig.ifname_store and are covered by test_ifname_store.py. +import json import os import shutil import tempfile import unittest +from pathlib import Path from unittest import mock from helper import prepare_module @@ -200,6 +202,33 @@ class TestRefuseRenamesInUse(unittest.TestCase): self.assertEqual(refused, {'eth1': 'eth23'}) +class TestStatusAdopted(unittest.TestCase): + """A Hyper-V NIC re-created with a new MAC takes the vacant name. That is + neither a card found in its recorded slot nor anything the slot says, so + it is reported under its own key - vyos-router warns about it from there. + """ + + def test_an_adoption_is_reported_apart_from_slot_matches(self): + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp) + status = os.path.join(tmp, 'status.json') + store = { + 'version': 1, + 'hardware': {}, + 'interfaces': {'eth3': 'ID_NET_NAME_MAC=enx000d3a47aa01'}, + } + report = {'adopted': {'eth3': '00:0d:3a:47:aa:01'}} + devices = [device('eth3', '00:0d:3a:47:aa:01')] + devices[0]['properties'] = {'ID_NET_NAME_MAC': 'enx000d3a47aa01'} + with mock.patch.object(resolver, 'status_file', Path(status)): + resolver.write_status(store, devices, {}, report, True) + with open(status) as f: + data = json.load(f) + self.assertEqual(data['adopted'], {'eth3': '00:0d:3a:47:aa:01'}) + self.assertEqual(data['matched_by_path'], {}) + self.assertIn('eth3', data['matched']) + + class TestMainInContainer(unittest.TestCase): """A container owns no NIC: its interfaces are runtime-created veth pairs with no backing bus device in sysfs and a host-assigned MAC that changes @@ -230,6 +259,134 @@ class TestMainInContainer(unittest.TestCase): self.assertFalse(resolver.status_file.exists()) +class TestHotplugPass(unittest.TestCase): + """A pass for hardware added to a running system hands no vacant name + over: the configuration under it would not be applied until the next boot. + """ + + def _unit(self, name): + with open(os.path.join(_here, '../systemd', name)) as f: + return f.read() + + def test_only_the_hotplug_unit_says_so(self): + self.assertIn('vyos-net-name-resolve.py --hotplug\n', + self._unit('vyos-net-name-resolve-hotplug.service')) + exec_start = [line for line in + self._unit('vyos-net-name-resolve.service').splitlines() + if line.startswith('ExecStart=')] + self.assertEqual(len(exec_start), 1) + self.assertNotIn('--hotplug', exec_start[0]) + + def _main(self, hotplug): + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + store = {'interfaces': {}, 'hardware': {}} + with mock.patch.object(resolver, 'is_running_as_container', + return_value=False), \ + mock.patch.object(resolver, 'status_file', + resolver.Path(status_dir) / 'status.json'), \ + mock.patch.object(resolver, 'load_store', return_value=store), \ + mock.patch.object(resolver, 'wait_for_hardware', + return_value=([], set())), \ + mock.patch.object(resolver, 'wait_for_settle', return_value=[]), \ + mock.patch.object(resolver, 'resolve', + return_value=({}, store, {})) as resolve, \ + mock.patch.object(resolver, 'save_store', return_value=False): + resolver.main(hotplug=hotplug) + return resolve.call_args + + def test_the_flag_is_read_from_the_command_line(self): + self.assertTrue(resolver.hotplug_requested(['--hotplug'])) + self.assertFalse(resolver.hotplug_requested([])) + + def test_a_hotplug_pass_keeps_the_boot_adoption_on_record(self): + # the boot pass gave eth2 to a new NIC; a later VF re-add starts a + # hotplug pass, which must neither forget nor repeat that + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + status = resolver.Path(status_dir) / 'status.json' + status.write_text(json.dumps({'adopted': {'eth2': '60:45:bd:00:00:01', + 'eth3': '60:45:bd:00:00:02'}})) + store = {'interfaces': {'eth2': 'ID_NET_NAME_MAC=enx6045bd000001', + 'eth3': 'ID_NET_NAME_MAC=enx6045bd000002'}, + 'hardware': {'eth2': '60:45:bd:00:00:01', + 'eth3': '60:45:bd:00:00:09'}} + with mock.patch.object(resolver, 'is_running_as_container', + return_value=False), \ + mock.patch.object(resolver, 'status_file', status), \ + mock.patch.object(resolver, 'load_store', return_value=store), \ + mock.patch.object(resolver, 'wait_for_hardware', + return_value=([], set())), \ + mock.patch.object(resolver, 'wait_for_settle', return_value=[]), \ + mock.patch.object(resolver, 'resolve', + return_value=({}, store, {})), \ + mock.patch.object(resolver, 'save_store', return_value=False), \ + mock.patch.object(resolver.logger, 'warning') as warning: + resolver.main(hotplug=True) + # eth3's NIC changed since: that record no longer holds + self.assertEqual(json.loads(status.read_text())['adopted'], + {'eth2': '60:45:bd:00:00:01'}) + self.assertFalse(any('had no NIC this boot' in str(c) + for c in warning.call_args_list)) + + def test_the_boot_pass_reports_only_its_own_adoptions(self): + # a status left from an earlier pass is not carried by a boot pass + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + status = resolver.Path(status_dir) / 'status.json' + status.write_text(json.dumps({'adopted': {'eth2': '60:45:bd:00:00:01'}})) + store = {'interfaces': {'eth2': 'ID_NET_NAME_MAC=enx6045bd000001'}, + 'hardware': {'eth2': '60:45:bd:00:00:01'}, 'hyperv': ['eth2']} + with mock.patch.object(resolver, 'is_running_as_container', + return_value=False), \ + mock.patch.object(resolver, 'status_file', status), \ + mock.patch.object(resolver, 'load_store', return_value=store), \ + mock.patch.object(resolver, 'wait_for_hardware', + return_value=([], set())), \ + mock.patch.object(resolver, 'wait_for_settle', return_value=[]), \ + mock.patch.object(resolver, 'resolve', + return_value=({}, store, {})), \ + mock.patch.object(resolver, 'save_store', return_value=False): + resolver.main(hotplug=False) + self.assertEqual(json.loads(status.read_text())['adopted'], {}) + + def _unrecorded_map_logs(self, present): + store = {'interfaces': {'eth0': 'ID_NET_NAME_MAC=enx000d3a000001'}, + 'hardware': {'eth0': '00:0d:3a:00:00:01'}} + devices = [{'properties': {'ID_NET_NAME_MAC': 'enx000d3a000001'}, + 'mac': '00:0d:3a:00:00:01', 'name': 'eth0'}] if present else [] + status_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, status_dir, ignore_errors=True) + with mock.patch.object(resolver, 'is_running_as_container', + return_value=False), \ + mock.patch.object(resolver, 'status_file', + resolver.Path(status_dir) / 'status.json'), \ + mock.patch.object(resolver, 'load_store', return_value=store), \ + mock.patch.object(resolver, 'wait_for_hardware', + return_value=(devices, set())), \ + mock.patch.object(resolver, 'wait_for_settle', + return_value=devices), \ + mock.patch.object(resolver, 'resolve', + return_value=({}, store, {})), \ + mock.patch.object(resolver, 'save_store', return_value=False), \ + mock.patch.object(resolver.logger, 'info') as info: + resolver.main(hotplug=False) + return any('does not say which interfaces' in str(c) + for c in info.call_args_list) + + def test_an_unrecorded_map_is_mentioned_only_when_a_name_is_vacant(self): + # a fresh Azure deployment: cloud-init wrote the map, every NIC is + # there - nothing is held back, so nothing is said (5479.0.9 G1) + self.assertFalse(self._unrecorded_map_logs(present=True)) + self.assertTrue(self._unrecorded_map_logs(present=False)) + + def test_the_boot_pass_may_adopt(self): + self.assertTrue(self._main(False).kwargs['adopt']) + + def test_the_hotplug_pass_does_not(self): + self.assertFalse(self._main(True).kwargs['adopt']) + + class TestBootOrdering(unittest.TestCase): """The store lives in the config directory, which does not exist until vyos-router mounts (and, for an encrypted config volume, decrypts) it. So -- cgit v1.2.3