From 37d21e0e44b96f981aa5498024a63e5778452d03 Mon Sep 17 00:00:00 2001 From: Josephine Pfeiffer Date: Fri, 25 Sep 2026 13:13:50 +0200 Subject: [PATCH 1/3] feat: describe NetworkManager changes in check mode Report individual route, address and DNS changes, scalar old and new values, and added or removed settings through the existing stderr logger. Use libnm setting diffs with the same normalization and comparison flags as connection comparison, while redacting credentials and private keys. Signed-off-by: Josephine Pfeiffer --- README.md | 15 +++ library/network_connections.py | 230 +++++++++++++++++++++++++++++++-- 2 files changed, 231 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 01ff7456..f0b26266 100644 --- a/README.md +++ b/README.md @@ -1472,6 +1472,21 @@ and `initscripts`). That means, you can use the same playbook with NetworkManage and initscripts. However, note that not every option is handled exactly the same by every provider. Do a test run first with `--check`. +With the `nm` provider, check mode describes the changes to existing connection +profiles in the role's log output. It lists individual additions and removals for +list properties such as static routes, IP addresses, and DNS servers, and shows +old and new values for properties such as MTU. Added and removed settings are +identified by name. For example: + +```text +[003] #0, state:up persistent_state:present, 'eth0': change property 802-3-ethernet.mtu from 1500 to 9000 +[004] #0, state:up persistent_state:present, 'eth0': remove DNS server 192.0.2.53 (ipv4.dns) +[005] #0, state:up persistent_state:present, 'eth0': add static route 192.168.100.0/24 via 192.168.1.129 (ipv4.routes) +``` + +Secret values, including passwords and private keys, are redacted. The details +are part of the `ansible-playbook --check` output and do not need `--diff`. + It is not supported to create a configuration for one provider, and expect another provider to handle them. For example, creating profiles with the `initscripts` provider, and later enabling NetworkManager is not guaranteed to work automatically. Possibly, diff --git a/library/network_connections.py b/library/network_connections.py index 03d994aa..349e7443 100644 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -63,6 +63,7 @@ import errno import functools +import numbers import os import re import shlex @@ -899,28 +900,221 @@ def _cmp(a, b): cons.sort(cmp=_cmp) return cons + @staticmethod + def _connection_normalized_clone(con): + con = Util.NM().SimpleConnection.new_clone(con) + try: + con.normalize() + except Exception: + pass + return con + def connection_compare( self, con_a, con_b, normalize_a=False, normalize_b=False, compare_flags=None ): - NM = Util.NM() - if normalize_a: - con_a = NM.SimpleConnection.new_clone(con_a) - try: - con_a.normalize() - except Exception: - pass + con_a = self._connection_normalized_clone(con_a) if normalize_b: - con_b = NM.SimpleConnection.new_clone(con_b) - try: - con_b.normalize() - except Exception: - pass + con_b = self._connection_normalized_clone(con_b) if compare_flags is None: - compare_flags = NM.SettingCompareFlags.IGNORE_TIMESTAMP + compare_flags = Util.NM().SettingCompareFlags.IGNORE_TIMESTAMP return con_a.compare(con_b, compare_flags) + @staticmethod + def _setting_diff_values(setting): + NM = Util.NM() + values = {} + + def collect(setting, name, value, flags): + # NM_SETTING_PARAM_SECRET is 0x400; older typelibs expose it as 4. + secret = bool(flags & 0x400) + if setting.get_name() == "802-1x": + # libnm does not mark embedded keys as secrets. A PKCS#12 + # client certificate can contain a key, and PKCS#11 URIs a PIN. + if name in ("private-key", "phase2-private-key"): + secret = True + elif name in ( + "ca-cert", + "client-cert", + "phase2-ca-cert", + "phase2-client-cert", + ): + get_scheme = getattr( + setting, "get_%s_scheme" % name.replace("-", "_") + ) + scheme = get_scheme() + secret = ( + secret + or scheme == getattr(NM.Setting8021xCKScheme, "PKCS11", None) + or ( + name in ("client-cert", "phase2-client-cert") + and scheme == NM.Setting8021xCKScheme.BLOB + ) + ) + if secret: + value = None + elif setting.get_name() in ("ipv4", "ipv6") and name == "dns-options": + value = ( + setting.get_property(name) if setting.has_dns_options() else None + ) + elif setting.get_name() in ("ipv4", "ipv6") and name == "routing-rules": + value = [ + setting.get_routing_rule(i) + for i in range(setting.get_num_routing_rules()) + ] + elif setting.find_property(name) is not None: + # Enumeration loses the element types of boxed arrays. Property + # access preserves them; dynamic ethtool options have no pspec. + value = setting.get_property(name) + values[name] = (value, secret) + + setting.enumerate_values(collect) + return values + + @classmethod + def _setting_diff_value(cls, value): + NM = Util.NM() + GLib = Util.GLib() + if isinstance(value, GLib.Variant): + return cls._setting_diff_value(value.unpack()) + if isinstance(value, GLib.Bytes): + return repr(value.get_data()) + if isinstance(value, (NM.IPRoute, NM.IPAddress)): + if isinstance(value, NM.IPRoute): + result = "%s/%s" % (value.get_dest(), value.get_prefix()) + if value.get_next_hop(): + result += " via %s" % value.get_next_hop() + if value.get_metric() != -1: + result += " metric %s" % value.get_metric() + else: + result = "%s/%s" % (value.get_address(), value.get_prefix()) + for name in sorted(value.get_attribute_names()): + result += " %s=%s" % ( + name, + cls._setting_diff_value(value.get_attribute(name)), + ) + return result + if isinstance(value, getattr(NM, "IPRoutingRule", ())): + return value.to_string(NM.IPRoutingRuleAsStringFlags.NONE, None) + if isinstance(value, getattr(NM, "BridgeVlan", ())): + return value.to_str() + if isinstance(value, getattr(NM, "TCQdisc", ())): + return NM.utils_tc_qdisc_to_str(value) + if isinstance(value, getattr(NM, "TCTfilter", ())): + return NM.utils_tc_tfilter_to_str(value) + if isinstance(value, getattr(NM, "SriovVF", ())): + return NM.utils_sriov_vf_to_str(value, False) + if isinstance(value, (list, tuple)): + return "[%s]" % ", ".join(cls._setting_diff_value(v) for v in value) + if isinstance(value, dict): + return "{%s}" % ", ".join( + "%s: %s" % (cls._setting_diff_value(k), cls._setting_diff_value(v)) + for k, v in sorted(value.items()) + ) + if isinstance(value, numbers.Integral): + return str(value) + return repr(value) + + @classmethod + def _setting_diff_property(cls, setting_name, name, old, new): + property_name = "%s.%s" % (setting_name, name) + old_value, old_secret = old + new_value, new_secret = new + if old_secret or new_secret: + yield "change property %s from to " % property_name + return + + if isinstance(old_value, (list, tuple)) or isinstance(new_value, (list, tuple)): + label = "value" + if setting_name in ("ipv4", "ipv6"): + label = { + "routes": "static route", + "addresses": "IP address", + "dns": "DNS server", + "dns-search": "DNS search domain", + "dns-options": "DNS option", + "routing-rules": "routing rule", + }.get(name, label) + + def format_item(value): + if setting_name in ("ipv4", "ipv6") and name in ( + "dns", + "dns-search", + "dns-options", + ): + return value.replace("\n", "\\n").replace("\r", "\\r") + return cls._setting_diff_value(value) + + old_items = set(format_item(v) for v in (old_value or [])) + new_items = set(format_item(v) for v in (new_value or [])) + for value in sorted(old_items - new_items): + yield "remove %s %s (%s)" % (label, value, property_name) + for value in sorted(new_items - old_items): + yield "add %s %s (%s)" % (label, value, property_name) + if old_items != new_items: + return + + # Equal membership can still differ in order (for example, DNS servers). + yield "change property %s from %s to %s" % ( + property_name, + cls._setting_diff_value(old_value), + cls._setting_diff_value(new_value), + ) + + @staticmethod + def _connection_settings(con): + if hasattr(con, "get_settings"): + return dict((s.get_name(), s) for s in (con.get_settings() or [])) + + # Before libnm 1.10, settings are only exposed through their values. + settings = {} + + def collect(setting, name, value, flags): + settings[setting.get_name()] = setting + + con.for_each_setting_value(collect) + return settings + + def connection_diff( + self, con_a, con_b, normalize_a=False, normalize_b=False, compare_flags=None + ): + """Describe changes from con_a to con_b without modifying either profile.""" + if normalize_a: + con_a = self._connection_normalized_clone(con_a) + if normalize_b: + con_b = self._connection_normalized_clone(con_b) + if compare_flags is None: + compare_flags = Util.NM().SettingCompareFlags.IGNORE_TIMESTAMP + + settings_a = self._connection_settings(con_a) + settings_b = self._connection_settings(con_b) + for name in sorted(set(settings_a) | set(settings_b)): + setting_a = settings_a.get(name) + setting_b = settings_b.get(name) + # Introspection does not allow None as the other setting in diff(). + if setting_a is None: + yield "add setting %s" % name + setting_a = type(setting_b)() + elif setting_b is None: + yield "remove setting %s" % name + setting_b = type(setting_a)() + _same, differences = setting_a.diff(setting_b, compare_flags, False, {}) + if not differences: + continue + values_a = self._setting_diff_values(setting_a) + values_b = self._setting_diff_values(setting_b) + for property_name in sorted(differences): + # Python 2 does not support yield from. + # pylint: disable=unknown-option-value,use-yield-from + for message in self._setting_diff_property( + name, + property_name, + values_a.get(property_name, (None, False)), + values_b.get(property_name, (None, False)), + ): + yield message + def connection_is_active(self, con): NM = Util.NM() for ac in self.active_connection_list(connections=[con]): @@ -2505,7 +2699,15 @@ def run_action_present(self, idx): idx, "update connection %s, %s" % (con_cur.get_id(), con_cur.get_uuid()) ) self.connections_data_set_changed(idx) - if self.check_mode == CheckMode.REAL_RUN: + if self.check_mode == CheckMode.DRY_RUN: + try: + for message in self.nmutil.connection_diff( + con_cur, con_new, normalize_a=True + ): + self.log_info(idx, message) + except Exception as e: + self.log_warn(idx, "cannot describe connection changes: %s" % (e)) + elif self.check_mode == CheckMode.REAL_RUN: try: self.nmutil.connection_update(con_cur, con_new) except MyError as e: From 97279fdfad097c8eb4fb81abf686425ecd61106f Mon Sep 17 00:00:00 2001 From: Josephine Pfeiffer Date: Fri, 25 Sep 2026 13:14:03 +0200 Subject: [PATCH 2/3] test: cover NetworkManager connection diffs Exercise property diffs with real libnm objects, including populated settings, route attributes, secret redaction and older libnm APIs. Verify check-mode logging and that dry runs do not update connections. Signed-off-by: Josephine Pfeiffer --- tests/unit/test_network_connections.py | 650 ++++++++++++++++++++++++- 1 file changed, 649 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_network_connections.py b/tests/unit/test_network_connections.py index 3f4ec489..4e24ed00 100644 --- a/tests/unit/test_network_connections.py +++ b/tests/unit/test_network_connections.py @@ -22,7 +22,15 @@ import network_lsr import network_lsr.argument_validator -from network_connections import IfcfgUtil, NMUtil, SysUtil, Util +from network_connections import ( + CheckMode, + Cmd_nm, + IfcfgUtil, + NMUtil, + RunEnvironmentAnsible, + SysUtil, + Util, +) from network_lsr.argument_validator import ValidationError try: @@ -4684,6 +4692,646 @@ def test_path_to_glib_bytes(self): self.assertEqual(result.get_data(), b"file:///my/test/path\x00") +class TestNMConnectionDiff(unittest.TestCase): + def setUp(self): + try: + self.nm = Util.NM() + except (ImportError, ValueError): + self.skipTest("no support for NM (libnm via pygobject)") + self.nmutil = NMUtil(nmclient=mock.Mock()) + + def connection(self): + con = self.nm.SimpleConnection.new() + setting = self.nm.SettingConnection.new() + setting.set_property("id", "diff-test") + setting.set_property("uuid", "deed76f5-97c5-4733-85f0-10922b0ed08b") + setting.set_property("type", "802-3-ethernet") + con.add_setting(setting) + wired = self.nm.SettingWired.new() + wired.set_property("mtu", 1500) + con.add_setting(wired) + for setting_type, family, address, dns in ( + (self.nm.SettingIP4Config, socket.AF_INET, "192.0.2.10", "192.0.2.53"), + (self.nm.SettingIP6Config, socket.AF_INET6, "2001:db8::10", "2001:db8::53"), + ): + setting = setting_type.new() + setting.set_property("method", "manual") + prefix = 24 if family == socket.AF_INET else 64 + setting.add_address(self.nm.IPAddress.new(family, address, prefix)) + setting.add_dns(dns) + con.add_setting(setting) + return con + + def route(self, metric=-1, table=None, route_type=None, src=None): + route = self.nm.IPRoute.new( + socket.AF_INET, "192.168.100.0", 24, "192.168.1.129", metric + ) + for name, variant_type, value in ( + ("table", "u", table), + ("type", "s", route_type), + ("src", "s", src), + ): + if value is not None: + route.set_attribute(name, Util.GLib().Variant(variant_type, value)) + return route + + def test_populated_settings_report_scalar_and_list_changes(self): + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_wired().set_property("mtu", 9000) + ip4 = proposed.get_setting_ip4_config() + ip4.add_route(self.route()) + ip4.remove_address(0) + ip4.add_address(self.nm.IPAddress.new(socket.AF_INET, "192.0.2.1", 24)) + ip4.remove_dns(0) + ip4.add_dns("192.0.2.54") + + lines = list(self.nmutil.connection_diff(current, proposed)) + + for line in ( + "change property 802-3-ethernet.mtu from 1500 to 9000", + "add static route 192.168.100.0/24 via 192.168.1.129 (ipv4.routes)", + "remove IP address 192.0.2.10/24 (ipv4.addresses)", + "add IP address 192.0.2.1/24 (ipv4.addresses)", + "remove DNS server 192.0.2.53 (ipv4.dns)", + "add DNS server 192.0.2.54 (ipv4.dns)", + ): + self.assertIn(line, lines) + self.assertFalse(any("setting " in line for line in lines)) + self.assertFalse(any("ipv6" in line for line in lines)) + + def test_route_metric_and_attributes_are_part_of_identity(self): + for original, replacement, old_detail, new_detail in ( + (self.route(10), self.route(20), "metric 10", "metric 20"), + (self.route(table=100), self.route(table=200), "table=100", "table=200"), + ( + self.route(route_type="unicast"), + self.route(route_type="blackhole"), + "type='unicast'", + "type='blackhole'", + ), + ( + self.route(src="192.0.2.1"), + self.route(src="192.0.2.2"), + "src='192.0.2.1'", + "src='192.0.2.2'", + ), + ): + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + current.get_setting_ip4_config().add_route(original) + proposed.get_setting_ip4_config().add_route(replacement) + + lines = list(self.nmutil.connection_diff(current, proposed)) + + self.assertIn( + "remove static route 192.168.100.0/24 " + "via 192.168.1.129 %s (ipv4.routes)" % old_detail, + lines, + ) + self.assertIn( + "add static route 192.168.100.0/24 via 192.168.1.129 %s (ipv4.routes)" + % new_detail, + lines, + ) + + def test_ipv6_and_direct_route_changes(self): + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + current.get_setting_ip6_config().add_route( + self.nm.IPRoute.new(socket.AF_INET6, "2001:db8:1::", 64, None, -1) + ) + ip6 = proposed.get_setting_ip6_config() + ip6.add_route( + self.nm.IPRoute.new(socket.AF_INET6, "2001:db8:2::", 64, "2001:db8::1", 20) + ) + ip6.remove_address(0) + ip6.remove_dns(0) + + lines = list(self.nmutil.connection_diff(current, proposed)) + + for line in ( + "remove static route 2001:db8:1::/64 (ipv6.routes)", + "add static route 2001:db8:2::/64 via 2001:db8::1 metric 20 (ipv6.routes)", + "remove IP address 2001:db8::10/64 (ipv6.addresses)", + "remove DNS server 2001:db8::53 (ipv6.dns)", + ): + self.assertIn(line, lines) + + def test_identical_boxed_values_do_not_produce_changes(self): + current = self.connection() + current.get_setting_ip4_config().add_route(self.route(10, table=100)) + proposed = self.nm.SimpleConnection.new_clone(current) + + self.assertEqual(list(self.nmutil.connection_diff(current, proposed)), []) + + def test_bridge_vlan_changes_preserve_unchanged_entries(self): + if not hasattr(self.nm, "BridgeVlan"): + self.skipTest("bridge VLANs are unavailable") + current = self.connection() + bridge = self.nm.SettingBridge.new() + bridge.add_vlan(self.nm.BridgeVlan.new(100, 100)) + vlan = self.nm.BridgeVlan.new(200, 200) + vlan.set_pvid(True) + vlan.set_untagged(True) + bridge.add_vlan(vlan) + current.add_setting(bridge) + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_bridge().remove_vlan(1) + proposed.get_setting_bridge().add_vlan(self.nm.BridgeVlan.new(300, 310)) + + self.assertEqual( + list(self.nmutil.connection_diff(current, proposed)), + [ + "remove value 200 pvid untagged (bridge.vlans)", + "add value 300-310 (bridge.vlans)", + ], + ) + + def test_tc_changes_preserve_unchanged_entries(self): + if not hasattr(self.nm, "SettingTCConfig"): + self.skipTest("traffic control settings are unavailable") + current = self.connection() + tc = self.nm.SettingTCConfig.new() + for description in ("parent 1: handle 2: sfq", "root handle 1: fq_codel"): + tc.add_qdisc(self.nm.utils_tc_qdisc_from_str(description)) + tc.add_tfilter( + self.nm.utils_tc_tfilter_from_str( + "parent ffff: matchall action simple sdata hello" + ) + ) + current.add_setting(tc) + proposed = self.nm.SimpleConnection.new_clone(current) + proposed_tc = proposed.get_setting(self.nm.SettingTCConfig) + proposed_tc.remove_qdisc(1) + proposed_tc.add_qdisc(self.nm.utils_tc_qdisc_from_str("root handle 1: fq")) + proposed_tc.remove_tfilter(0) + + self.assertEqual( + list(self.nmutil.connection_diff(current, proposed)), + [ + "remove value root handle 1: fq_codel (tc.qdiscs)", + "add value root handle 1: fq (tc.qdiscs)", + "remove value parent ffff: matchall action simple sdata hello " + "(tc.tfilters)", + ], + ) + + def test_sriov_changes_preserve_unchanged_entries(self): + if not hasattr(self.nm, "SettingSriov"): + self.skipTest("SR-IOV settings are unavailable") + current = self.connection() + sriov = self.nm.SettingSriov.new() + sriov.add_vf(self.nm.SriovVF.new(0)) + vf = self.nm.SriovVF.new(1) + vf.set_attribute("mac", Util.GLib().Variant("s", "02:00:00:00:00:01")) + sriov.add_vf(vf) + current.add_setting(sriov) + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting(self.nm.SettingSriov).remove_vf(1) + + self.assertEqual( + list(self.nmutil.connection_diff(current, proposed)), + ["remove value 1 mac=02:00:00:00:00:01 (sriov.vfs)"], + ) + + def test_empty_dns_options_differ_from_unset(self): + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_ip4_config().set_property("dns-options", []) + proposed.get_setting_ip6_config().set_property("dns-options", []) + + self.assertEqual( + list(self.nmutil.connection_diff(current, proposed)), + [ + "change property ipv4.dns-options from None to []", + "change property ipv6.dns-options from None to []", + ], + ) + self.assertEqual( + list(self.nmutil.connection_diff(proposed, current)), + [ + "change property ipv4.dns-options from [] to None", + "change property ipv6.dns-options from [] to None", + ], + ) + + def test_settings_without_get_settings_api(self): + class LegacyConnection(object): + def __init__(self, connection): + self.for_each_setting_value = connection.for_each_setting_value + + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_wired().set_property("mtu", 9000) + proposed.add_setting(self.nm.SettingBridge.new()) + + self.assertEqual( + list( + self.nmutil.connection_diff( + LegacyConnection(current), LegacyConnection(proposed) + ) + ), + [ + "change property 802-3-ethernet.mtu from 1500 to 9000", + "add setting bridge", + ], + ) + + def test_list_order_changes_are_visible(self): + current = self.connection() + current.get_setting_ip4_config().add_dns("192.0.2.54") + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_ip4_config().clear_dns() + proposed.get_setting_ip4_config().add_dns("192.0.2.54") + proposed.get_setting_ip4_config().add_dns("192.0.2.53") + + lines = list(self.nmutil.connection_diff(current, proposed)) + + self.assertTrue(any("ipv4.dns" in line for line in lines)) + self.assertFalse(any("add DNS server" in line for line in lines)) + self.assertFalse(any("remove DNS server" in line for line in lines)) + + def test_settings_added_and_removed_with_details(self): + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + current.remove_setting(self.nm.SettingIP4Config) + proposed.remove_setting(self.nm.SettingIP6Config) + + lines = list(self.nmutil.connection_diff(current, proposed)) + + for line in ( + "add setting ipv4", + "remove setting ipv6", + "add IP address 192.0.2.10/24 (ipv4.addresses)", + "remove IP address 2001:db8::10/64 (ipv6.addresses)", + "add DNS server 192.0.2.53 (ipv4.dns)", + "remove DNS server 2001:db8::53 (ipv6.dns)", + ): + self.assertIn(line, lines) + + def test_default_only_setting_is_reported(self): + setting_types = [self.nm.SettingWired] + if hasattr(self.nm, "SettingEthtool"): + setting_types.append(self.nm.SettingEthtool) + for setting_type in setting_types: + current = self.nm.SimpleConnection.new() + proposed = self.nm.SimpleConnection.new() + setting = setting_type.new() + proposed.add_setting(setting) + + self.assertIn( + "add setting %s" % setting.get_name(), + list(self.nmutil.connection_diff(current, proposed)), + ) + self.assertIn( + "remove setting %s" % setting.get_name(), + list(self.nmutil.connection_diff(proposed, current)), + ) + + def test_scalar_strings_boolean_and_unset(self): + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_connection().set_property("id", "renamed") + proposed.get_setting_connection().set_property("autoconnect", False) + proposed.get_setting_ip4_config().set_property("gateway", "192.0.2.1") + + lines = list(self.nmutil.connection_diff(current, proposed)) + + self.assertIn( + "change property connection.id from 'diff-test' to 'renamed'", lines + ) + self.assertIn( + "change property connection.autoconnect from True to False", lines + ) + self.assertTrue( + any("ipv4.gateway" in line and "'192.0.2.1'" in line for line in lines) + ) + + def test_normalization_and_timestamp_comparison_match(self): + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.normalize() + proposed.get_setting_connection().set_property("timestamp", 2**64 - 1) + + self.assertTrue( + self.nmutil.connection_compare(current, proposed, normalize_a=True) + ) + self.assertEqual( + list(self.nmutil.connection_diff(current, proposed, normalize_a=True)), [] + ) + self.assertIn( + "change property connection.timestamp from 0 to 18446744073709551615", + list( + self.nmutil.connection_diff( + current, + proposed, + normalize_a=True, + compare_flags=self.nm.SettingCompareFlags.EXACT, + ) + ), + ) + self.assertEqual(current.get_setting_connection().get_timestamp(), 0) + + def test_secret_flag_from_legacy_introspection(self): + current = self.connection() + security = self.nm.SettingWirelessSecurity.new() + security.set_property("psk", "old-test-secret") + current.add_setting(security) + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_wired().set_property("mtu", 9000) + proposed.get_setting_wireless_security().set_property("psk", "new-test-secret") + + with mock.patch.object(self.nm, "SETTING_PARAM_SECRET", 4): + lines = list(self.nmutil.connection_diff(current, proposed)) + + self.assertIn("change property 802-3-ethernet.mtu from 1500 to 9000", lines) + self.assertIn( + "change property 802-11-wireless-security.psk " + "from to ", + lines, + ) + self.assertNotIn("old-test-secret", "\n".join(lines)) + self.assertNotIn("new-test-secret", "\n".join(lines)) + + def test_secret_and_private_key_values_are_redacted(self): + current = self.connection() + security = self.nm.SettingWirelessSecurity.new() + security.set_property("psk", "old-test-secret") + current.add_setting(security) + authentication = self.nm.Setting8021x.new() + authentication.set_property("password", "old-eap-secret") + authentication.set_property( + "private-key", Util.GLib().Bytes.new(b"old-private-key-material") + ) + for property_name in ("client-cert", "phase2-client-cert"): + authentication.set_property( + property_name, Util.GLib().Bytes.new(b"old-pkcs12-private-key-material") + ) + current.add_setting(authentication) + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_wireless_security().set_property("psk", "new-test-secret") + proposed.get_setting_802_1x().set_property("password", "new-eap-secret") + proposed.get_setting_802_1x().set_property( + "private-key", Util.GLib().Bytes.new(b"new-private-key-material") + ) + for property_name in ("client-cert", "phase2-client-cert"): + proposed.get_setting_802_1x().set_property( + property_name, Util.GLib().Bytes.new(b"new-pkcs12-private-key-material") + ) + + lines = list(self.nmutil.connection_diff(current, proposed)) + output = "\n".join(lines) + + for property_name in ( + "802-11-wireless-security.psk", + "802-1x.password", + "802-1x.private-key", + "802-1x.client-cert", + "802-1x.phase2-client-cert", + ): + self.assertTrue( + any(property_name in line and "" in line for line in lines) + ) + for secret in ( + "old-test-secret", + "new-test-secret", + "old-eap-secret", + "new-eap-secret", + "old-private-key-material", + "new-private-key-material", + "old-pkcs12-private-key-material", + "new-pkcs12-private-key-material", + ): + self.assertNotIn(secret, output) + + def test_certificate_file_paths_remain_visible(self): + current = self.connection() + authentication = self.nm.Setting8021x.new() + properties = ("ca-cert", "client-cert", "phase2-ca-cert", "phase2-client-cert") + for property_name in properties: + authentication.set_property( + property_name, Util.path_to_glib_bytes("/old-client-cert.pem") + ) + current.add_setting(authentication) + proposed = self.nm.SimpleConnection.new_clone(current) + for property_name in properties: + proposed.get_setting_802_1x().set_property( + property_name, Util.path_to_glib_bytes("/new-client-cert.pem") + ) + + lines = list(self.nmutil.connection_diff(current, proposed)) + + for property_name in properties: + self.assertTrue( + any( + "802-1x.%s" % property_name in line + and "/old-client-cert.pem" in line + and "/new-client-cert.pem" in line + for line in lines + ) + ) + self.assertNotIn("", "\n".join(lines)) + + def test_pkcs11_certificate_pins_are_redacted_in_check_mode(self): + if not hasattr(self.nm.Setting8021xCKScheme, "PKCS11"): + self.skipTest("PKCS#11 certificate URIs are unavailable") + current = self.connection() + authentication = self.nm.Setting8021x.new() + authentication.set_property("eap", ["tls"]) + authentication.set_property("identity", "diff-test") + authentication.set_property( + "private-key", Util.path_to_glib_bytes("/private-key.pem") + ) + current.add_setting(authentication) + proposed = self.nm.SimpleConnection.new_clone(current) + properties = ("ca-cert", "client-cert", "phase2-ca-cert", "phase2-client-cert") + for connection, pin in ( + (current, "old-uri-secret"), + (proposed, "new-uri-secret"), + ): + for property_name in properties: + uri = ( + "pkcs11:token=diff-test;object=cert;type=cert?pin-value=%s\x00" + % pin + ) + connection.get_setting_802_1x().set_property( + property_name, Util.GLib().Bytes.new(uri.encode("utf-8")) + ) + connection.normalize() + cmd = self.command(current, proposed, CheckMode.DRY_RUN) + + cmd.run_action_present(0) + + stderr = cmd.run_env._complete_kwargs(cmd.connections, {})["stderr"] + for property_name in properties: + self.assertIn( + "change property 802-1x.%s from to " + % property_name, + stderr, + ) + self.assertNotIn("old-uri-secret", stderr) + self.assertNotIn("new-uri-secret", stderr) + self.assertNotIn("pin-value", stderr) + self.assertEqual(cmd._nmutil.connection_update.call_count, 0) + + def test_dynamic_ethtool_option(self): + if not hasattr(self.nm, "SettingEthtool"): + self.skipTest("ethtool settings require NetworkManager 1.14") + current = self.connection() + ethtool = self.nm.SettingEthtool.new() + if not hasattr(ethtool, "option_set"): + self.skipTest("generic ethtool option API is unavailable") + ethtool.option_set("feature-gro", Util.GLib().Variant("b", True)) + current.add_setting(ethtool) + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting(self.nm.SettingEthtool).option_set( + "feature-gro", Util.GLib().Variant("b", False) + ) + + self.assertIn( + "change property ethtool.feature-gro from True to False", + list(self.nmutil.connection_diff(current, proposed)), + ) + + def test_routing_rules_use_readable_values(self): + if not hasattr(self.nm, "IPRoutingRule"): + self.skipTest("routing rules are unavailable") + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + for connection, priority in ((current, 100), (proposed, 200)): + rule = self.nm.IPRoutingRule.new(socket.AF_INET) + rule.set_priority(priority) + rule.set_from("192.0.2.0", 24) + rule.set_table(100) + connection.get_setting_ip4_config().add_routing_rule(rule) + + lines = list(self.nmutil.connection_diff(current, proposed)) + + self.assertTrue( + any( + line.startswith("remove routing rule priority 100 ") + and "192.0.2.0/24" in line + and line.endswith("(ipv4.routing-rules)") + for line in lines + ) + ) + self.assertTrue( + any( + line.startswith("add routing rule priority 200 ") + and "192.0.2.0/24" in line + and line.endswith("(ipv4.routing-rules)") + for line in lines + ) + ) + self.assertNotIn("PtrArray", "\n".join(lines)) + + def test_added_secret_setting_is_redacted(self): + current = self.connection() + proposed = self.nm.SimpleConnection.new_clone(current) + security = self.nm.SettingWirelessSecurity.new() + security.set_property("psk", "new-test-secret") + proposed.add_setting(security) + + lines = list(self.nmutil.connection_diff(current, proposed)) + + self.assertIn("add setting 802-11-wireless-security", lines) + self.assertTrue(any("psk" in line and "" in line for line in lines)) + self.assertNotIn("new-test-secret", "\n".join(lines)) + + def command(self, current, proposed, check_mode): + run_env = RunEnvironmentAnsible() + run_env.module.params = {"__debug_flags": ""} + run_env._run_results_push(1) + cmd = Cmd_nm( + run_env=run_env, + connections_unvalidated=[], + connection_validator=mock.Mock(), + ) + cmd._check_mode = check_mode + cmd._connections = [ + { + "name": "diff-test", + "nm.uuid": current.get_uuid(), + "type": "ethernet", + "state": "up", + "persistent_state": "present", + "ignore_errors": None, + "ieee802_1x": None, + } + ] + cmd._nmutil = NMUtil(nmclient=mock.Mock()) + cmd._nmutil.connection_list = mock.Mock(return_value=[current]) + cmd._nmutil.connection_create = mock.Mock(return_value=proposed) + cmd._nmutil.connection_update = mock.Mock() + cmd._nm_provider = mock.Mock() + cmd._nm_provider.get_connections.return_value = [] + return cmd + + def test_unchanged_check_mode_does_not_report_diff(self): + current = self.connection() + current.normalize() + proposed = self.nm.SimpleConnection.new_clone(current) + cmd = self.command(current, proposed, CheckMode.DRY_RUN) + with mock.patch.object(cmd._nmutil, "connection_diff") as diff: + cmd.run_action_present(0) + + self.assertEqual(diff.call_count, 0) + self.assertEqual(cmd._nmutil.connection_update.call_count, 0) + self.assertFalse(cmd.is_changed_modified_system) + + def test_check_mode_diff_failure_only_warns(self): + current = self.connection() + current.normalize() + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_wired().set_property("mtu", 9000) + cmd = self.command(current, proposed, CheckMode.DRY_RUN) + + with mock.patch.object( + cmd._nmutil, "connection_diff", side_effect=RuntimeError("boom") + ): + cmd.run_action_present(0) + + stderr = cmd.run_env._complete_kwargs(cmd.connections, {})["stderr"] + self.assertIn("", stderr) + + def test_check_mode_logs_details_without_updating(self): + current = self.connection() + current.normalize() + proposed = self.nm.SimpleConnection.new_clone(current) + proposed.get_setting_wired().set_property("mtu", 9000) + for check_mode in (CheckMode.DRY_RUN, CheckMode.PRE_RUN, CheckMode.REAL_RUN): + cmd = self.command(current, proposed, check_mode) + + with mock.patch.object( + cmd._nmutil, "connection_diff", wraps=cmd._nmutil.connection_diff + ) as diff: + cmd.run_action_present(0) + + stderr = cmd.run_env._complete_kwargs(cmd.connections, {})["stderr"] + if check_mode == CheckMode.DRY_RUN: + self.assertEqual(diff.call_count, 1) + self.assertIn( + "change property 802-3-ethernet.mtu from 1500 to 9000", stderr + ) + self.assertTrue( + any( + line.startswith("[002] ") + and "#0, state:up persistent_state:present, 'diff-test':" + in line + for line in stderr.splitlines() + ) + ) + self.assertTrue(cmd.is_changed_modified_system) + else: + self.assertEqual(diff.call_count, 0) + self.assertNotIn("change property", stderr) + self.assertEqual( + cmd._nmutil.connection_update.call_count, + 1 if check_mode == CheckMode.REAL_RUN else 0, + ) + + class TestValidatorMatch(Python26CompatTestCase): def setUp(self): self.test_profile = { From a6b15bd6e77e3818079ed5584152e08ad04196c3 Mon Sep 17 00:00:00 2001 From: Josephine Pfeiffer Date: Fri, 25 Sep 2026 13:14:17 +0200 Subject: [PATCH 3/3] test: verify NetworkManager check mode end to end Run the role through Ansible to verify detailed IPv4 and IPv6 diffs, unchanged connections during check mode, and convergence after applying. Check that the test profile and interface are removed during cleanup. Signed-off-by: Josephine Pfeiffer --- tests/ensure_provider_tests.py | 3 + tests/playbooks/tests_check_mode_diff.yml | 198 ++++++++++++++++++++++ tests/tests_check_mode_diff_nm.yml | 53 ++++++ 3 files changed, 254 insertions(+) create mode 100644 tests/playbooks/tests_check_mode_diff.yml create mode 100644 tests/tests_check_mode_diff_nm.yml diff --git a/tests/ensure_provider_tests.py b/tests/ensure_provider_tests.py index 48c0e58c..48ce8358 100755 --- a/tests/ensure_provider_tests.py +++ b/tests/ensure_provider_tests.py @@ -114,6 +114,9 @@ }, "playbooks/tests_ignore_auto_dns.yml": {}, "playbooks/tests_bond_options.yml": {}, + "playbooks/tests_check_mode_diff.yml": { + MINIMUM_VERSION: "'1.8.0'", + }, "playbooks/tests_bond_port_match_by_mac.yml": {}, "playbooks/tests_eth_dns_support.yml": {}, "playbooks/tests_dummy.yml": {}, # wokeignore:rule=dummy diff --git a/tests/playbooks/tests_check_mode_diff.yml b/tests/playbooks/tests_check_mode_diff.yml new file mode 100644 index 00000000..ba5c7d07 --- /dev/null +++ b/tests/playbooks/tests_check_mode_diff.yml @@ -0,0 +1,198 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Test NetworkManager check mode diffs + hosts: all + vars: + network_provider: nm + interface: lsrchkdiff0 + profile: "{{ interface }}" + baseline_connections: + - name: "{{ interface }}" + type: dummy + state: up + autoconnect: false + mtu: 1500 + ip: + dhcp4: false + auto6: false + auto_gateway: false + address: + - 192.0.2.10/24 + - 2001:db8::10/64 + dns: + - 192.0.2.53 + - 2001:db8::53 + route: + - network: 203.0.113.0 + prefix: 24 + gateway: 192.0.2.1 + metric: 100 + proposed_connections: + - name: "{{ interface }}" + type: dummy + state: up + autoconnect: false + mtu: 9000 + ip: + dhcp4: false + auto6: false + auto_gateway: false + address: + - 192.0.2.20/24 + - 2001:db8::20/64 + dns: + - 192.0.2.54 + - 2001:db8::54 + route: + - network: 203.0.113.128 + prefix: 25 + gateway: 192.0.2.2 + metric: 200 + tasks: + - name: Initialize the network role + include_tasks: tasks/run_role_with_clear_facts.yml + vars: + network_connections: [] + + - name: Check that the test interface is absent + include_tasks: tasks/assert_device_absent.yml + + - name: List existing profiles + command: nmcli -g NAME connection show + changed_when: false + register: existing_profiles + + - name: Check that the test profile name is available + assert: + that: interface not in existing_profiles.stdout_lines + + - name: Test a managed connection in check mode + block: + - name: Create the baseline connection + include_tasks: tasks/run_role_with_clear_facts.yml + vars: + network_connections: "{{ baseline_connections }}" + + - name: Read the baseline profile + command: >- + nmcli -t -f connection,802-3-ethernet,ipv4,ipv6 + connection show {{ interface }} + changed_when: false + register: profile_before + + - name: Read the baseline addresses + command: ip -o address show dev {{ interface }} + changed_when: false + register: addresses_before + + - name: Preview the proposed connection changes + check_mode: true + block: + - name: Run the role in check mode + include_tasks: tasks/run_role_with_clear_facts.yml + vars: + network_connections: "{{ proposed_connections }}" + + - name: Verify the preview reports individual changes + assert: + that: + - __network_connections_result is changed + - item in __network_connections_result.stderr + fail_msg: "Missing change in check-mode output: {{ item }}" + loop: + - 'change property 802-3-ethernet.mtu from 1500 to 9000' + - 'remove IP address 192.0.2.10/24' + - 'add IP address 192.0.2.20/24' + - 'remove IP address 2001:db8::10/64' + - 'add IP address 2001:db8::20/64' + - 'remove DNS server 192.0.2.53' + - 'add DNS server 192.0.2.54' + - 'remove DNS server 2001:db8::53' + - 'add DNS server 2001:db8::54' + - 'remove static route 203.0.113.0/24 via 192.0.2.1 metric 100' + - 'add static route 203.0.113.128/25 via 192.0.2.2 metric 200' + + - name: Verify the preview uses numbered info log lines + assert: + that: + - >- + __network_connections_result.stderr_lines | + select('search', 'change property|add static route|remove static route|add IP address|remove IP address|add DNS server|remove DNS server') | + reject('match', '^\[[0-9]+\] \s+#0,') | list | length == 0 + + - name: Read the profile after check mode + command: >- + nmcli -t -f connection,802-3-ethernet,ipv4,ipv6 + connection show {{ interface }} + changed_when: false + register: profile_after + + - name: Read the addresses after check mode + command: ip -o address show dev {{ interface }} + changed_when: false + register: addresses_after + + - name: Verify check mode did not change the connection + vars: + before_addresses: >- + {{ addresses_before.stdout | + regex_findall('inet6? +([^ ]+)') | sort }} + after_addresses: >- + {{ addresses_after.stdout | + regex_findall('inet6? +([^ ]+)') | sort }} + assert: + that: + - profile_before.stdout == profile_after.stdout + - "'192.0.2.10/24' in before_addresses" + - "'2001:db8::10/64' in before_addresses" + - before_addresses == after_addresses + + - name: Preview the unchanged baseline connection + check_mode: true + block: + - name: Run the role with the baseline connection in check mode + include_tasks: tasks/run_role_with_clear_facts.yml + vars: + network_connections: "{{ baseline_connections }}" + + - name: Verify the baseline is unchanged + assert: + that: + - __network_connections_result is not changed + - "'change property' not in __network_connections_result.stderr" + + - name: Apply the proposed connection changes + include_tasks: tasks/run_role_with_clear_facts.yml + vars: + network_connections: "{{ proposed_connections }}" + + - name: Preview the applied connection + check_mode: true + block: + - name: Run the role with the applied connection in check mode + include_tasks: tasks/run_role_with_clear_facts.yml + vars: + network_connections: "{{ proposed_connections }}" + + - name: Verify the applied connection is unchanged + assert: + that: + - __network_connections_result is not changed + - "'change property' not in __network_connections_result.stderr" + always: + - name: Remove the test connection + tags: tests::cleanup + include_tasks: tasks/run_role_with_clear_facts.yml + vars: + network_connections: + - name: "{{ interface }}" + state: down + persistent_state: absent + + - name: Verify the test profile was removed + tags: tests::cleanup + include_tasks: tasks/assert_profile_absent.yml + + - name: Verify the test interface was removed + tags: tests::cleanup + include_tasks: tasks/assert_device_absent.yml diff --git a/tests/tests_check_mode_diff_nm.yml b/tests/tests_check_mode_diff_nm.yml new file mode 100644 index 00000000..8494e220 --- /dev/null +++ b/tests/tests_check_mode_diff_nm.yml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: BSD-3-Clause +# This file was generated by ensure_provider_tests.py +--- +# set network provider and gather facts +# yamllint disable rule:line-length +- name: Run playbook 'playbooks/tests_check_mode_diff.yml' with nm as provider + hosts: all + tasks: + - name: Include the task 'el_repo_setup.yml' + include_tasks: tasks/el_repo_setup.yml + - name: Set network provider to 'nm' + set_fact: + network_provider: nm + tags: + - always + - name: Include distro variables + include_vars: vars/rh_distros_vars.yml + - name: Set platform facts + set_fact: + __network_distro_major_version: "{{ ansible_facts['distribution_major_version'] }}" + __network_is_rhel: "{{ ansible_facts['distribution'] == 'RedHat' }}" + __network_is_fedora: "{{ ansible_facts['distribution'] == 'Fedora' }}" + __network_is_centos: "{{ ansible_facts['distribution'] == 'CentOS' }}" + __network_is_os_family_rhel: "{{ ansible_facts['os_family'] == 'RedHat' }}" + __is_rh_distro: "{{ __network_is_rh_distro }}" + + - name: Install NetworkManager and get NetworkManager version + when: + - __network_distro_major_version != '6' + tags: + - always + block: + - name: Install NetworkManager + package: + name: NetworkManager + state: present + use: "{{ (__network_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" + - name: Get package info + package_facts: + - name: Get NetworkManager version + set_fact: + networkmanager_version: "{{ + ansible_facts.packages['NetworkManager'][0]['version'] }}" + + +# The test requires or should run with NetworkManager, therefore it cannot run +# on RHEL/CentOS 6 +- name: Import the playbook 'playbooks/tests_check_mode_diff.yml' + import_playbook: playbooks/tests_check_mode_diff.yml + when: + - __network_distro_major_version != '6' + - networkmanager_version is version('1.8.0', '>=')