From cac2bbb43e04678cb5126836a01358cb74e40e2d Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Thu, 13 Mar 2025 18:10:47 -0400 Subject: [PATCH] fix: Remove MAC address matching from SysUtil.link_info_find() The link_info_find() function previously allowed searching for links by MAC address, but this introduced ambiguity and could cause false alarms in certain cases (e.g. retrieving the link info by MAC might return the link info that only matches the current MAC instead of the permanent MAC). To ensure reliable behavior, this function should accept and match the link info only by interface name. To address the issues, the following changes were made: - Removed MAC address matching logic to eliminate ambiguity. - Simplified the function to only check ifname, making it more predictable. - Updated all callers to adapt to this change, ensuring correctness. - When a profile is tied to an interface via mac only, the validation of the existence of interface will now be delegated to NetworkManager instead. Resolves: https://issues.redhat.com/browse/RHEL-84197 Signed-off-by: Wen Liang --- library/network_connections.py | 42 ++--------- 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 ++++++ 8 files changed, 207 insertions(+), 34 deletions(-) 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/library/network_connections.py b/library/network_connections.py index fd0c56b..588013b 100644 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -222,31 +222,15 @@ class SysUtil: return linkinfos @classmethod - def link_info_find(cls, refresh=False, mac=None, ifname=None): - if mac is not None: - 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) + def link_info_find(cls, ifname): + result = 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 + for linkinfo in cls.link_infos().values(): + if ifname == linkinfo["ifname"]: + result = linkinfo + break - # 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 - - if ifname is not None and ifname == linkinfo.get("ifname", None): - return linkinfo - - # Return fallback match by address if no perm-address match found - if "matched_by_address" in locals(): - return matched_by_address - - return None + return result ############################################################################### @@ -2155,18 +2139,8 @@ class Cmd(object): # permanent MAC address. li_mac = None li_ifname = None - if connection["mac"]: - li_mac = SysUtil.link_info_find(mac=connection["mac"]) - if not li_mac: - self.log_fatal( - idx, - "profile specifies mac '%s' but no such interface exists" - % (connection["mac"]), - ) if connection["interface_name"]: - li_ifname = SysUtil.link_info_find( - ifname=connection["interface_name"] - ) + li_ifname = SysUtil.link_info_find(connection["interface_name"]) if not li_ifname: if connection["type"] == "ethernet": self.log_fatal( diff --git a/tests/ensure_provider_tests.py b/tests/ensure_provider_tests.py index 6e0643d..d1d1dc4 100755 --- a/tests/ensure_provider_tests.py +++ b/tests/ensure_provider_tests.py @@ -79,6 +79,7 @@ ibution_major_version | int < 9", "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 0000000..cc9ca22 --- /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 0000000..2bca149 --- /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 0000000..be5bc90 --- /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 0000000..412127b --- /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 0000000..9adbf20 --- /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 0000000..bd7b527 --- /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'