From 52c2f7733f88a7c9a8816af91fd0b33b35ce1ae9 Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Mon, 3 Feb 2025 20:11:20 -0500 Subject: [PATCH 1/3] refactor: Make SysUtil.link_info_find() more pythonic The existing implementation of `link_info_find()` had several issues that made it error-prone and harder to maintain: - The refresh parameter was unused, adding unnecessary complexity. - MAC address matching logic was convoluted, with redundant variables and conditions. - The use of locals() for fallback results was unclear and could lead to subtle bugs. - The order of checks (MAC vs ifname) didn't prioritize the most reliable matching criteria. To address these issues, the following changes were made: - Removed the unused `refresh` argument to reduce complexity. - Used `None` checks more idiomatically with `if mac` instead of `is not None`. - Eliminated redundant variables and conditions to improve readability. - Avoided using `locals()` by explicitly storing fallback results. - Made `ifname` matching take priority before checking MAC addresses. - Ensured that the function returns early when a definitive match is found. Signed-off-by: Wen Liang --- library/network_connections.py | 36 ++++++++++++++++------------------ 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/library/network_connections.py b/library/network_connections.py index fd0c56bb..9225db2a 100644 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -95,6 +95,7 @@ DEFAULT_ACTIVATION_TIMEOUT = 90 DEFAULT_TIMEOUT = 10 +NULL_MAC = "00:00:00:00:00:00" class CheckMode: @@ -222,31 +223,28 @@ def link_infos(cls, refresh=False): return linkinfos @classmethod - def link_info_find(cls, refresh=False, mac=None, ifname=None): - if mac is not None: + def link_info_find(cls, mac=None, ifname=None): + if mac: mac = Util.mac_norm(mac) - for linkinfo in cls.link_infos(refresh).values(): - perm_address = linkinfo.get("perm-address", None) - current_address = linkinfo.get("address", None) - # Match by perm-address (prioritized) - if mac is not None and perm_address not in [None, "00:00:00:00:00:00"]: - if mac == perm_address: - return linkinfo + result = None - # Fallback to match by address - if mac is not None and (perm_address in [None, "00:00:00:00:00:00"]): - if mac == current_address: - matched_by_address = linkinfo # Save for potential fallback + for linkinfo in cls.link_infos().values(): + perm_address = linkinfo.get("perm-address", NULL_MAC) + current_address = linkinfo.get("address", NULL_MAC) - if ifname is not None and ifname == linkinfo.get("ifname", None): - return linkinfo + if ifname and ifname == linkinfo["ifname"]: + result = linkinfo + break - # Return fallback match by address if no perm-address match found - if "matched_by_address" in locals(): - return matched_by_address + if mac: + if mac == perm_address: + result = linkinfo + break + elif mac == current_address: + result = linkinfo - return None + return result ############################################################################### From cf56080ff7e30ad49b52f1a8209dbd531775ec62 Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Mon, 10 Mar 2025 15:29:15 -0400 Subject: [PATCH 2/3] fix: Refine MAC validation using interface name When a user provides both an interface name and a MAC address, the current validation process retrieves sysfs link info separately using the interface name and the MAC address, then compares the results. If the information doesn't match, an error is raised. However, this approach may trigger false alarms because retrieving the link info by MAC might return data that only matches the current MAC instead of the permanent MAC. Since the interface name is unique within the kernel, a more robust validation method is to fetch the MAC address using the interface name and then compare it directly with the user-provided MAC address. Signed-off-by: Wen Liang --- library/network_connections.py | 48 +++++++++++++++---- tests/ensure_provider_tests.py | 1 + .../tests_bond_port_match_by_mac.yml | 40 ++++++++++++++++ tests/tasks/create_bond_port_match_by_mac.yml | 25 ++++++++++ tests/tests_bond_port_match_by_mac_nm.yml | 23 +++++++++ 5 files changed, 129 insertions(+), 8 deletions(-) create mode 100644 tests/playbooks/tests_bond_port_match_by_mac.yml create mode 100644 tests/tasks/create_bond_port_match_by_mac.yml create mode 100644 tests/tests_bond_port_match_by_mac_nm.yml diff --git a/library/network_connections.py b/library/network_connections.py index 9225db2a..dcaac05e 100644 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -167,6 +167,20 @@ def _link_read_address(ifname): c = SysUtil._sysctl_read("/sys/class/net/" + ifname + "/address") return Util.mac_norm(c.strip()) + @staticmethod + def _link_read_bond_port_perm_hwaddr(ifname): + filename = os.path.join( + "/sys/class/net", + ifname, + # wokeignore:rule=slave + "bonding_slave", + "perm_hwaddr", + ) + if not os.path.exists(filename): + return None + c = SysUtil._sysctl_read(filename) + return Util.mac_norm(c.strip()) + @staticmethod def _link_read_permaddress(ifname): return ethtool.get_perm_addr(ifname) @@ -187,6 +201,13 @@ def _link_infos_fetch(): "ifname": ifname, "address": SysUtil._link_read_address(ifname), "perm-address": SysUtil._link_read_permaddress(ifname), + # When an interface is added as a port of a bonding device, its MAC + # address might change, we need to retrieve and preserve the original + # MAC address to ensure the user-provided interface name and MAC match + # correctly. + "bond-port-perm-hwaddr": SysUtil._link_read_bond_port_perm_hwaddr( + ifname + ), } return links @@ -232,6 +253,7 @@ def link_info_find(cls, mac=None, ifname=None): for linkinfo in cls.link_infos().values(): perm_address = linkinfo.get("perm-address", NULL_MAC) current_address = linkinfo.get("address", NULL_MAC) + bond_port_perm_hwaddr = linkinfo.get("bond-port-perm-hwaddr", NULL_MAC) if ifname and ifname == linkinfo["ifname"]: result = linkinfo @@ -241,7 +263,7 @@ def link_info_find(cls, mac=None, ifname=None): if mac == perm_address: result = linkinfo break - elif mac == current_address: + elif mac in (current_address, bond_port_perm_hwaddr): result = linkinfo return result @@ -2180,13 +2202,23 @@ def run_prepare(self): "infiniband interface exists" % (connection["interface_name"]), ) - if li_mac and li_ifname and li_mac != li_ifname: - self.log_fatal( - idx, - "profile specifies interface_name '%s' and mac '%s' but no " - "such interface exists" - % (connection["interface_name"], connection["mac"]), - ) + elif connection["mac"]: + perm_address = li_ifname.get("perm-address", NULL_MAC) + current_address = li_ifname.get("address", NULL_MAC) + bond_port_perm_hwaddr = li_ifname.get( + "bond-port-perm-hwaddr", NULL_MAC + ) + if (perm_address not in (NULL_MAC, connection["mac"])) or ( + perm_address == NULL_MAC + and connection["mac"] + not in (current_address, bond_port_perm_hwaddr) + ): + self.log_fatal( + idx, + "profile specifies interface_name '%s' and mac '%s' " + "but no such interface exists" + % (connection["interface_name"], connection["mac"]), + ) def start_transaction(self): """Hook before making changes""" diff --git a/tests/ensure_provider_tests.py b/tests/ensure_provider_tests.py index 6e0643db..f4f83991 100755 --- a/tests/ensure_provider_tests.py +++ b/tests/ensure_provider_tests.py @@ -74,6 +74,7 @@ }, "playbooks/tests_ignore_auto_dns.yml": {}, "playbooks/tests_bond_options.yml": {}, + "playbooks/tests_bond_port_match_by_mac.yml": {}, "playbooks/tests_eth_dns_support.yml": {}, "playbooks/tests_dummy.yml": {}, # wokeignore:rule=dummy "playbooks/tests_infiniband.yml": {}, diff --git a/tests/playbooks/tests_bond_port_match_by_mac.yml b/tests/playbooks/tests_bond_port_match_by_mac.yml new file mode 100644 index 00000000..a66616b2 --- /dev/null +++ b/tests/playbooks/tests_bond_port_match_by_mac.yml @@ -0,0 +1,40 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Play for creating the connection to match the port device + based on the perm_hwaddr + hosts: all + vars: + controller_profile: bond0 + controller_device: nm-bond + port1_profile: bond0.0 + dhcp_interface1: test1 + port2_profile: bond0.1 + dhcp_interface2: test2 + profile: test2conn + interface: test2 + tasks: + - name: Test creating the connection to match the port device + based on the perm_hwaddr + tags: + - tests::bond:create + block: + - name: Include the task 'run_test.yml' + include_tasks: tasks/run_test.yml + vars: + lsr_description: Given two DHCP-enabled network interfaces, + when creating a bond profile with them, + then we can still create the connection to match the port device + based on the perm_hwaddr + lsr_setup: + - tasks/create_test_interfaces_with_dhcp.yml + - tasks/assert_dhcp_device_present.yml + lsr_test: + - tasks/create_bond_profile.yml + - tasks/create_bond_port_match_by_mac.yml + lsr_assert: + - tasks/assert_controller_device_present.yml + - tasks/assert_profile_present.yml + lsr_cleanup: + - tasks/cleanup_bond_profile+device.yml + - tasks/remove_test_interfaces_with_dhcp.yml + - tasks/check_network_dns.yml diff --git a/tests/tasks/create_bond_port_match_by_mac.yml b/tests/tasks/create_bond_port_match_by_mac.yml new file mode 100644 index 00000000..5de8133e --- /dev/null +++ b/tests/tasks/create_bond_port_match_by_mac.yml @@ -0,0 +1,25 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Retrieve perm_hwaddr using ethtool + # wokeignore:rule=slave + command: cat /sys/class/net/{{ interface }}/bonding_slave/perm_hwaddr + register: mac_address_result + changed_when: false + failed_when: mac_address_result.rc != 0 +- name: Set the MAC address variable + set_fact: + mac: "{{ mac_address_result.stdout_lines[-1].split(' ')[-1] }}" +- name: Display the retrieved MAC address + debug: + msg: "Retrieved MAC address for {{ interface }}: {{ mac }}" +- name: Test matching the port device based on the perm_hwaddr + import_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ profile }}" + state: up + type: ethernet + interface_name: "{{ interface }}" + mac: "{{ mac }}" +... diff --git a/tests/tests_bond_port_match_by_mac_nm.yml b/tests/tests_bond_port_match_by_mac_nm.yml new file mode 100644 index 00000000..399a5064 --- /dev/null +++ b/tests/tests_bond_port_match_by_mac_nm.yml @@ -0,0 +1,23 @@ +# 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_bond_port_match_by_mac.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 + + +# The test requires or should run with NetworkManager, therefore it cannot run +# on RHEL/CentOS 6 +- name: Import the playbook 'playbooks/tests_bond_port_match_by_mac.yml' + import_playbook: playbooks/tests_bond_port_match_by_mac.yml + when: + - ansible_distribution_major_version != '6' From 82ac07541854e9430c47431aaa68bc558ed9861a Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Sat, 18 Jan 2025 14:27:45 -0500 Subject: [PATCH 3/3] tests: Prefer permanent MAC for finding link info, fallback to current MAC When a network connection specifies both an interface name and MAC address, and the physical interface has identical permanent and current MACs, applying the configuration multiple times can cause an error: "no such interface exists." The issue occurs because `SysUtil.link_info_find()` may return a link info entry where the user-specified MAC matches only the current ("address") but not the permanent MAC ("perm-address"). In such cases, the returned link info may have a different interface name than the one specified in the network connection, leading to the error. We already implemented a fix (c3416831) that ensures link lookup prioritizes the permanent MAC, only falling back to the current MAC when necessary. The integration test `tests_mac_address_match.yml` verifies this fix by requiring an Ethernet interface where the permanent and current MACs match. Resolves: https://issues.redhat.com/browse/RHEL-74211 Signed-off-by: Wen Liang --- tests/ensure_provider_tests.py | 1 + tests/playbooks/tests_mac_address_match.yml | 75 +++++++++++++++++++ .../assert_network_connections_succeeded.yml | 8 ++ ...cleanup_vlan_and_parent_profile+device.yml | 26 +++++++ tests/tasks/create_mac_address_match.yml | 51 +++++++++++++ tests/tasks/find+remove_profile.yml | 15 ++++ tests/tests_mac_address_match_nm.yml | 23 ++++++ 7 files changed, 199 insertions(+) create mode 100644 tests/playbooks/tests_mac_address_match.yml create mode 100644 tests/tasks/assert_network_connections_succeeded.yml create mode 100644 tests/tasks/cleanup_vlan_and_parent_profile+device.yml create mode 100644 tests/tasks/create_mac_address_match.yml create mode 100644 tests/tasks/find+remove_profile.yml create mode 100644 tests/tests_mac_address_match_nm.yml diff --git a/tests/ensure_provider_tests.py b/tests/ensure_provider_tests.py index f4f83991..85024648 100755 --- a/tests/ensure_provider_tests.py +++ b/tests/ensure_provider_tests.py @@ -80,6 +80,7 @@ "playbooks/tests_infiniband.yml": {}, "playbooks/tests_ipv6_disabled.yml": {}, "playbooks/tests_ipv6_dns_search.yml": {}, + "playbooks/tests_mac_address_match.yml": {}, "playbooks/tests_provider.yml": { MINIMUM_VERSION: "'1.20.0'", "comment": "# NetworKmanager 1.20.0 added support for forgetting profiles", diff --git a/tests/playbooks/tests_mac_address_match.yml b/tests/playbooks/tests_mac_address_match.yml new file mode 100644 index 00000000..cc9ca224 --- /dev/null +++ b/tests/playbooks/tests_mac_address_match.yml @@ -0,0 +1,75 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Play for testing MAC address match on device + hosts: all + vars: + # This test requires an Ethernet interface whose permanent + # MAC address matches its current MAC address. Ensure that the + # specified interface meets this condition. + # + # Two VLAN profiles are defined to test deterministic behavior when fetching + # link information from sysfs. The issue being tested arises when `os.listdir(path)` + # returns interfaces in an arbitrary order, potentially listing a VLAN device + # before its parent interface. This can cause intermittent "no such interface + # exists" errors when applying configurations repeatedly. + # + # - `vlan_profile1` (e.g., `eth1.3732`) is named with the VLAN ID appended + # after the parent interface, following the standard `.` format. + # - `vlan_profile2` (e.g., `120-vlan`) has a fixed name, designed to test a scenario + # where lexicographic sorting causes the VLAN to appear before its parent interface. + interface: "{{ lookup('env', 'MAC_ADDR_MATCH_INTERFACE') | default('eth1', true) }}" + profile: "{{ interface }}" + vlan_profile1: "{{ interface }}.3732" + vlan_profile2: "120-vlan" + lsr_fail_debug: + - __network_connections_result + tags: + - "tests::match" + tasks: + - name: Show playbook name + debug: + msg: "this is: playbooks/tests_mac_address_match.yml" + tags: + - always + + - name: Install ethtool (test dependency) + package: + name: ethtool + state: present + use: "{{ (__network_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" + + - name: Retrieve MAC address using ethtool + command: ethtool -P {{ interface }} + register: mac_address_result + changed_when: false + failed_when: mac_address_result.rc != 0 + + - name: Set the MAC address variable + set_fact: + mac: "{{ mac_address_result.stdout_lines[-1].split(' ')[-1] }}" + + - name: Display the retrieved MAC address + debug: + msg: "Retrieved MAC address for {{ interface }}: {{ mac }}" + + - name: Test the MAC address match + tags: + - tests::match:match + block: + - name: Include the task 'run_test.yml' + include_tasks: tasks/run_test.yml + vars: + lsr_description: Test MAC address match on device + lsr_setup: + - tasks/find+remove_profile.yml + - tasks/assert_profile_absent.yml + lsr_test: + - tasks/create_mac_address_match.yml + - tasks/create_mac_address_match.yml + lsr_assert: + - tasks/assert_profile_present.yml + - tasks/assert_network_connections_succeeded.yml + lsr_cleanup: + - tasks/cleanup_vlan_and_parent_profile+device.yml + - tasks/check_network_dns.yml diff --git a/tests/tasks/assert_network_connections_succeeded.yml b/tests/tasks/assert_network_connections_succeeded.yml new file mode 100644 index 00000000..2bca1492 --- /dev/null +++ b/tests/tasks/assert_network_connections_succeeded.yml @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Assert that configuring network connections is succeeded + assert: + that: + - __network_connections_result.failed == false + msg: Configuring network connections is failed with the error + "{{ __network_connections_result.stderr }}" diff --git a/tests/tasks/cleanup_vlan_and_parent_profile+device.yml b/tests/tasks/cleanup_vlan_and_parent_profile+device.yml new file mode 100644 index 00000000..be5bc904 --- /dev/null +++ b/tests/tasks/cleanup_vlan_and_parent_profile+device.yml @@ -0,0 +1,26 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Clean up the test devices and the connection profiles + tags: + - "tests::cleanup" + block: + - name: Import network role + import_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ profile }}" + persistent_state: absent + state: down + - name: "{{ vlan_profile1 }}" + persistent_state: absent + state: down + - name: "{{ vlan_profile2 }}" + persistent_state: absent + state: down + failed_when: false + - name: Delete the device '{{ interface }}' + command: ip link del {{ interface }} + failed_when: false + changed_when: false +... diff --git a/tests/tasks/create_mac_address_match.yml b/tests/tasks/create_mac_address_match.yml new file mode 100644 index 00000000..412127bb --- /dev/null +++ b/tests/tasks/create_mac_address_match.yml @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Include network role + include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ interface }}" + state: up + persistent_state: present + autoconnect: true + type: ethernet + interface_name: "{{ interface }}" + mac: "{{ mac }}" + ip: + dhcp4: false + auto6: false + + - name: "{{ vlan_profile1 }}" + state: up + persistent_state: present + type: vlan + parent: "{{ interface }}" + vlan: + id: 3732 + autoconnect: true + ip: + auto_gateway: false + gateway4: 10.10.0.1 + address: 10.10.0.6/24 + dhcp4: false + auto6: false + + - name: "{{ vlan_profile2 }}" + state: up + persistent_state: present + type: vlan + parent: "{{ interface }}" + vlan: + id: 120 + autoconnect: true + ip: + auto_gateway: false + gateway4: 10.10.120.1 + address: 10.10.120.120/24 + dhcp4: false + auto6: false +- name: Show result + debug: + var: __network_connections_result +... diff --git a/tests/tasks/find+remove_profile.yml b/tests/tasks/find+remove_profile.yml new file mode 100644 index 00000000..9adbf20a --- /dev/null +++ b/tests/tasks/find+remove_profile.yml @@ -0,0 +1,15 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Get connection profile for '{{ interface }}' + command: "nmcli -g GENERAL.CONNECTION device show {{ interface }}" + register: connection_name + changed_when: false + +- name: Bring down and delete the connection profile for '{{ interface }}' + include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ connection_name.stdout }}" + persistent_state: absent + state: down diff --git a/tests/tests_mac_address_match_nm.yml b/tests/tests_mac_address_match_nm.yml new file mode 100644 index 00000000..bd7b5275 --- /dev/null +++ b/tests/tests_mac_address_match_nm.yml @@ -0,0 +1,23 @@ +# 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_mac_address_match.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 + + +# The test requires or should run with NetworkManager, therefore it cannot run +# on RHEL/CentOS 6 +- name: Import the playbook 'playbooks/tests_mac_address_match.yml' + import_playbook: playbooks/tests_mac_address_match.yml + when: + - ansible_distribution_major_version != '6'