From ff9374edc97592475fb49bb4a2cc750c06377f2f Mon Sep 17 00:00:00 2001 From: Roman Prilipskii Date: Fri, 31 Jul 2026 07:50:32 -0400 Subject: [PATCH] CLOS-4330: Inhibit upgrade when NetworkManager manages no device A keyfile under /etc/NetworkManager/conf.d setting `unmanaged-devices=*` is survivable while network-scripts is installed: the legacy network.service brings the interfaces up instead. CloudLinux 9 drops network-scripts entirely, so the same configuration leaves the upgraded host with no network at all - no SSH, no ICMP, reachable only from the hypervisor console, which reads from outside as a hung upgrade. Upstream's el8toel9 checkifcfg actor covers the equivalent NM_CONTROLLED=no setting in ifcfg files and inhibits on it, but it never looks at conf.d, so this class of configuration reaches the reboot completely unreported. CheckNetworkManagerUnmanaged (ChecksPhase) closes that gap the same way upstream handles its half: it surfaces the problem as an inhibitor with a remediation hint and lets the administrator decide, rather than editing network configuration on their behalf. Deciding whether an interface should be handed to NetworkManager is not ours to make silently, and an auto-rename that guessed wrong would cause exactly the outage it is meant to prevent. Consequences of reporting rather than mutating, both deliberate: * Provenance no longer matters. An earlier revision keyed on RPM ownership plus a machine-generation marker comment, because it had to be sure a file was machine-written before renaming it. Nothing is modified now, so any wildcard override is flagged regardless of who wrote it - a hand-written one strands the host just as effectively. That removes the heuristic entirely. * A false positive is an annoyance requiring a waiver, not a broken host. Only the wildcard form is matched. Targeted values (`=mac:...`, `=interface-name:eth1`) exclude named devices, and deciding whether the excluded one carries the host's connectivity is not something this check can do reliably. Gated on target major version >= 9. This repository is shared by the 7->8 and 8->9 upgrade paths, and on CloudLinux 8 network-scripts is still present, so inhibiting a 7->8 upgrade over this would be a false positive. Verified on a USERLAND-AUTO CL8 nopanel guest (template 6663) whose one-context-written override is the real-world case: Inhibitors: 2 -> `leapp preupgrade` exits 1 INHIBITOR: NetworkManager is configured not to manage any device severity: high Files with the problematic configuration: - /etc/NetworkManager/conf.d/50-unmanaged-devices.conf and with the file removed the report disappears, leaving only the unrelated sshd inhibitor. 17 unit tests pass on that guest under the real leapp framework. Co-Authored-By: Claude Opus 4.7 --- .../checknetworkmanagerunmanaged/actor.py | 34 ++++ .../libraries/checknetworkmanagerunmanaged.py | 107 ++++++++++++ .../unit_test_checknetworkmanagerunmanaged.py | 158 ++++++++++++++++++ 3 files changed, 299 insertions(+) create mode 100644 repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/actor.py create mode 100644 repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/libraries/checknetworkmanagerunmanaged.py create mode 100644 repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/tests/unit_test_checknetworkmanagerunmanaged.py diff --git a/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/actor.py b/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/actor.py new file mode 100644 index 0000000000..07489c760c --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/actor.py @@ -0,0 +1,34 @@ +from leapp.actors import Actor +from leapp.libraries.actor import checknetworkmanagerunmanaged +from leapp.libraries.common.cllaunch import run_on_cloudlinux +from leapp.reporting import Report +from leapp.tags import ChecksPhaseTag, IPUWorkflowTag + + +class CheckNetworkManagerUnmanaged(Actor): + """ + Inhibit the upgrade when NetworkManager is configured to manage no device. + + A keyfile under /etc/NetworkManager/conf.d setting ``unmanaged-devices=*`` + is survivable while network-scripts is installed, because the legacy + network.service brings the interfaces up instead. CloudLinux 9 drops + network-scripts, so the same configuration leaves the upgraded host with no + network at all - reachable only from the console. + + Upstream's el8toel9 checkifcfg actor covers the equivalent ``NM_CONTROLLED=no`` + setting in ifcfg files, but does not look at conf.d at all, so this class of + configuration reaches the reboot unreported. This actor closes that gap the + same way: it surfaces the problem and lets the administrator decide, rather + than editing network configuration on their behalf. + + See CLOS-4330 (one-context leaves such an override behind on OpenNebula guests). + """ + + name = 'check_network_manager_unmanaged' + consumes = () + produces = (Report,) + tags = (ChecksPhaseTag, IPUWorkflowTag) + + @run_on_cloudlinux + def process(self): + checknetworkmanagerunmanaged.process() diff --git a/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/libraries/checknetworkmanagerunmanaged.py b/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/libraries/checknetworkmanagerunmanaged.py new file mode 100644 index 0000000000..a985329456 --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/libraries/checknetworkmanagerunmanaged.py @@ -0,0 +1,107 @@ +import os +import re + +from leapp import reporting +from leapp.libraries.common.config.version import get_target_major_version +from leapp.libraries.stdlib import api + +NM_CONFD_DIR = '/etc/NetworkManager/conf.d' + +# Matches a NetworkManager keyfile entry that marks *every* device as unmanaged. +# Targeted forms (``unmanaged-devices=mac:...``, ``=interface-name:eth1``) are +# deliberately not matched: they exclude named devices, and deciding whether the +# excluded one carries the host's connectivity is not something this check can +# do reliably. The wildcard is unambiguous. +UNMANAGED_WILDCARD_RE = re.compile(r'^\s*unmanaged-devices\s*=\s*\*\s*$') + +FMT_LIST_SEPARATOR = '\n - ' + + +def _has_wildcard_override(path): + """ + Return True if any non-comment line in ``path`` sets unmanaged-devices to + the wildcard. Comments start with '#' (NetworkManager keyfile syntax). + """ + try: + with open(path, 'r') as f: + for raw in f: + line = raw.strip() + if not line or line.startswith('#'): + continue + if UNMANAGED_WILDCARD_RE.match(line): + return True + except (IOError, OSError) as exc: + api.current_logger().info( + 'Could not read NetworkManager configuration {0}: {1}'.format(path, exc) + ) + return False + + +def find_unmanaged_overrides(): + if not os.path.isdir(NM_CONFD_DIR): + return [] + found = [] + for name in sorted(os.listdir(NM_CONFD_DIR)): + if not name.endswith('.conf'): + continue + path = os.path.join(NM_CONFD_DIR, name) + if os.path.isfile(path) and _has_wildcard_override(path): + found.append(path) + return found + + +def process(): + # Only a problem when the target no longer ships network-scripts. On + # CloudLinux 8 the legacy network.service is still there to bring the + # interfaces up instead, so the same configuration is survivable and + # inhibiting a 7->8 upgrade over it would be a false positive. + if int(get_target_major_version()) < 9: + return + + overrides = find_unmanaged_overrides() + if not overrides: + return + + api.current_logger().info( + 'NetworkManager unmanaged-devices=* override(s) found: {0}'.format( + ', '.join(overrides) + ) + ) + + reporting.create_report([ + reporting.Title( + 'NetworkManager is configured not to manage any device' + ), + reporting.Summary( + 'CloudLinux {target} does not ship the network-scripts package, so ' + 'NetworkManager is the only thing that can bring network interfaces ' + 'up. The configuration below tells it to leave every device ' + 'unmanaged. Upgrading with it in place would leave this system with ' + 'no network connectivity after the reboot, reachable only from the ' + 'console.\n\n' + 'On OpenNebula guests this file is typically written at boot by ' + 'one-context (loc-10-network.d/functions), not shipped by any ' + 'package, so it is not removed when one-context is. Files with the ' + 'problematic configuration:{files}'.format( + target=get_target_major_version(), + files=''.join( + '{0}{1}'.format(FMT_LIST_SEPARATOR, path) for path in overrides + ), + ) + ), + reporting.Remediation( + hint=( + 'Remove the listed file(s), or drop the "unmanaged-devices=*" ' + 'entry from them, so NetworkManager manages the interfaces after ' + 'the upgrade. If specific devices must stay unmanaged, replace ' + 'the wildcard with the explicit device list documented in ' + 'NetworkManager.conf(5).' + ) + ), + reporting.Severity(reporting.Severity.HIGH), + reporting.Groups([reporting.Groups.NETWORK, reporting.Groups.SERVICES]), + reporting.Groups([reporting.Groups.INHIBITOR]), + reporting.RelatedResource('package', 'NetworkManager'), + ] + [ + reporting.RelatedResource('file', path) for path in overrides + ]) diff --git a/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/tests/unit_test_checknetworkmanagerunmanaged.py b/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/tests/unit_test_checknetworkmanagerunmanaged.py new file mode 100644 index 0000000000..7398aa7b9f --- /dev/null +++ b/repos/system_upgrade/cloudlinux/actors/checknetworkmanagerunmanaged/tests/unit_test_checknetworkmanagerunmanaged.py @@ -0,0 +1,158 @@ +import pytest + +from leapp import reporting +from leapp.libraries.actor import checknetworkmanagerunmanaged +from leapp.libraries.common.testutils import create_report_mocked, logger_mocked +from leapp.libraries.stdlib import api + +# Verbatim from a USERLAND-AUTO CL8 nopanel guest (template 6663, +# one-context-6.4.0-1.el8). Written at boot by one-context's +# loc-10-network.d/functions::nm_disable(), hence the marker comment and the +# fact that no RPM owns it. +RUNTIME_GENERATED_OVERRIDE = ( + '# Generated by one-context\n' + '\n' + '# NOTE: NetworkManager was dynamically disabled by OpenNebula\n' + '# contextualization scripts because interfaces are managed by\n' + '# different network service!\n' + '\n' + '[keyfile]\n' + 'unmanaged-devices=*\n' +) + + +def _seed(tmp_path, files): + conf_d = tmp_path / 'conf.d' + conf_d.mkdir() + for name, content in files.items(): + (conf_d / name).write_text(content) + return str(conf_d) + + +def _patch(monkeypatch, conf_d_path, target_version='9'): + monkeypatch.setattr(checknetworkmanagerunmanaged, 'NM_CONFD_DIR', conf_d_path) + monkeypatch.setattr(api, 'current_logger', logger_mocked()) + monkeypatch.setattr(reporting, 'create_report', create_report_mocked()) + monkeypatch.setattr( + checknetworkmanagerunmanaged, 'get_target_major_version', + lambda: target_version, + ) + + +def test_runtime_generated_override_inhibits(monkeypatch, tmp_path): + conf_d = _seed(tmp_path, {'50-unmanaged-devices.conf': RUNTIME_GENERATED_OVERRIDE}) + _patch(monkeypatch, conf_d) + + checknetworkmanagerunmanaged.process() + + assert reporting.create_report.called == 1 + fields = reporting.create_report.report_fields + assert fields['severity'] == 'high' + assert 'inhibitor' in fields['groups'] + assert conf_d + '/50-unmanaged-devices.conf' in fields['summary'] + assert 'remediations' in fields['detail'] + + +def test_admin_authored_override_also_inhibits(monkeypatch, tmp_path): + """Provenance is irrelevant now that nothing is modified: a hand-written + wildcard override strands the host just as effectively, and the operator + is the one who decides what to do about it.""" + conf_d = _seed(tmp_path, {'99-admin.conf': '[keyfile]\nunmanaged-devices=*\n'}) + _patch(monkeypatch, conf_d) + + checknetworkmanagerunmanaged.process() + + assert reporting.create_report.called == 1 + assert conf_d + '/99-admin.conf' in reporting.create_report.report_fields['summary'] + + +def test_no_report_when_target_is_el8(monkeypatch, tmp_path): + """CL8 still ships network-scripts, so network.service brings the + interfaces up and the override is survivable. Inhibiting a 7->8 upgrade + over it would be a false positive.""" + conf_d = _seed(tmp_path, {'50-unmanaged-devices.conf': RUNTIME_GENERATED_OVERRIDE}) + _patch(monkeypatch, conf_d, target_version='8') + + checknetworkmanagerunmanaged.process() + + assert reporting.create_report.called == 0 + + +def test_missing_confd_directory(monkeypatch, tmp_path): + _patch(monkeypatch, str(tmp_path / 'absent')) + checknetworkmanagerunmanaged.process() + assert reporting.create_report.called == 0 + + +@pytest.mark.parametrize('content', [ + '[keyfile]\nunmanaged-devices=mac:aa:bb:cc:dd:ee:ff\n', + '[keyfile]\nunmanaged-devices=interface-name:eth1\n', + '[keyfile]\nplugins=ifcfg-rh\n', + '[main]\ndns=none\nno-auto-default=*\n', + '# unmanaged-devices=*\n', + '', +]) +def test_non_wildcard_content_is_ignored(monkeypatch, tmp_path, content): + conf_d = _seed(tmp_path, {'10-other.conf': content}) + _patch(monkeypatch, conf_d) + + checknetworkmanagerunmanaged.process() + + assert reporting.create_report.called == 0 + + +def test_non_conf_files_are_skipped(monkeypatch, tmp_path): + conf_d = _seed(tmp_path, { + 'README.txt': '[keyfile]\nunmanaged-devices=*\n', + '50-unmanaged-devices.conf.disabled': '[keyfile]\nunmanaged-devices=*\n', + }) + _patch(monkeypatch, conf_d) + + checknetworkmanagerunmanaged.process() + + assert reporting.create_report.called == 0 + + +def test_multiple_overrides_listed_in_one_report(monkeypatch, tmp_path): + conf_d = _seed(tmp_path, { + '50-unmanaged-devices.conf': RUNTIME_GENERATED_OVERRIDE, + '99-extra.conf': '[keyfile]\nunmanaged-devices=*\n', + '10-fine.conf': '[main]\ndns=default\n', + }) + _patch(monkeypatch, conf_d) + + checknetworkmanagerunmanaged.process() + + assert reporting.create_report.called == 1 + summary = reporting.create_report.report_fields['summary'] + assert conf_d + '/50-unmanaged-devices.conf' in summary + assert conf_d + '/99-extra.conf' in summary + assert '10-fine.conf' not in summary + + +@pytest.mark.parametrize('spacing', [ + 'unmanaged-devices=*', + ' unmanaged-devices=* ', + 'unmanaged-devices = *', + 'unmanaged-devices= *', +]) +def test_wildcard_spacing_variants_match(monkeypatch, tmp_path, spacing): + conf_d = _seed(tmp_path, {'50-x.conf': '[keyfile]\n{0}\n'.format(spacing)}) + _patch(monkeypatch, conf_d) + + checknetworkmanagerunmanaged.process() + + assert reporting.create_report.called == 1 + + +def test_unreadable_file_does_not_raise(monkeypatch, tmp_path): + conf_d = _seed(tmp_path, {'50-x.conf': RUNTIME_GENERATED_OVERRIDE}) + _patch(monkeypatch, conf_d) + + def raising_open(*args, **kwargs): + raise IOError('permission denied') + monkeypatch.setattr(checknetworkmanagerunmanaged, 'open', raising_open, raising=False) + + checknetworkmanagerunmanaged.process() + + assert reporting.create_report.called == 0