Skip to content

Commit cac2bbb

Browse files
liangwen12yearliangwen12year
committed
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 <[email protected]>
1 parent 6a5ca93 commit cac2bbb

8 files changed

+208
-35
lines changed

‎library/network_connections.py‎

Lines changed: 9 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -222,31 +222,15 @@ def link_infos(cls, refresh=False):
222222
return linkinfos
223223

224224
@classmethod
225-
def link_info_find(cls, refresh=False, mac=None, ifname=None):
226-
if mac is not None:
227-
mac = Util.mac_norm(mac)
228-
for linkinfo in cls.link_infos(refresh).values():
229-
perm_address = linkinfo.get("perm-address", None)
230-
current_address = linkinfo.get("address", None)
231-
232-
# Match by perm-address (prioritized)
233-
if mac is not None and perm_address not in [None, "00:00:00:00:00:00"]:
234-
if mac == perm_address:
235-
return linkinfo
236-
237-
# Fallback to match by address
238-
if mac is not None and (perm_address in [None, "00:00:00:00:00:00"]):
239-
if mac == current_address:
240-
matched_by_address = linkinfo # Save for potential fallback
241-
242-
if ifname is not None and ifname == linkinfo.get("ifname", None):
243-
return linkinfo
244-
245-
# Return fallback match by address if no perm-address match found
246-
if "matched_by_address" in locals():
247-
return matched_by_address
225+
def link_info_find(cls, ifname):
226+
result = None
248227

249-
return None
228+
for linkinfo in cls.link_infos().values():
229+
if ifname == linkinfo["ifname"]:
230+
result = linkinfo
231+
break
232+
233+
return result
250234

251235

252236
###############################################################################
@@ -2155,18 +2139,8 @@ def run_prepare(self):
21552139
# permanent MAC address.
21562140
li_mac = None
21572141
li_ifname = None
2158-
if connection["mac"]:
2159-
li_mac = SysUtil.link_info_find(mac=connection["mac"])
2160-
if not li_mac:
2161-
self.log_fatal(
2162-
idx,
2163-
"profile specifies mac '%s' but no such interface exists"
2164-
% (connection["mac"]),
2165-
)
21662142
if connection["interface_name"]:
2167-
li_ifname = SysUtil.link_info_find(
2168-
ifname=connection["interface_name"]
2169-
)
2143+
li_ifname = SysUtil.link_info_find(connection["interface_name"])
21702144
if not li_ifname:
21712145
if connection["type"] == "ethernet":
21722146
self.log_fatal(

‎tests/ensure_provider_tests.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@
7979
"playbooks/tests_infiniband.yml": {},
8080
"playbooks/tests_ipv6_disabled.yml": {},
8181
"playbooks/tests_ipv6_dns_search.yml": {},
82+
"playbooks/tests_mac_address_match.yml": {},
8283
"playbooks/tests_provider.yml": {
8384
MINIMUM_VERSION: "'1.20.0'",
8485
"comment": "# NetworKmanager 1.20.0 added support for forgetting profiles",

‎tests/playbooks/tests_mac_address_match.yml‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
# SPDX-License-Identifier: BSD-3-Clause
2+
---
3+
- name: Play for testing MAC address match on device
4+
hosts: all
5+
vars:
6+
# This test requires an Ethernet interface whose permanent
7+
# MAC address matches its current MAC address. Ensure that the
8+
# specified interface meets this condition.
9+
#
10+
# Two VLAN profiles are defined to test deterministic behavior when fetching
11+
# link information from sysfs. The issue being tested arises when `os.listdir(path)`
12+
# returns interfaces in an arbitrary order, potentially listing a VLAN device
13+
# before its parent interface. This can cause intermittent "no such interface
14+
# exists" errors when applying configurations repeatedly.
15+
#
16+
# - `vlan_profile1` (e.g., `eth1.3732`) is named with the VLAN ID appended
17+
# after the parent interface, following the standard `<parent>.<vlan_id>` format.
18+
# - `vlan_profile2` (e.g., `120-vlan`) has a fixed name, designed to test a scenario
19+
# where lexicographic sorting causes the VLAN to appear before its parent interface.
20+
interface: "{{ lookup('env', 'MAC_ADDR_MATCH_INTERFACE') | default('eth1', true) }}"
21+
profile: "{{ interface }}"
22+
vlan_profile1: "{{ interface }}.3732"
23+
vlan_profile2: "120-vlan"
24+
lsr_fail_debug:
25+
- __network_connections_result
26+
tags:
27+
- "tests::match"
28+
tasks:
29+
- name: Show playbook name
30+
debug:
31+
msg: "this is: playbooks/tests_mac_address_match.yml"
32+
tags:
33+
- always
34+
35+
- name: Install ethtool (test dependency)
36+
package:
37+
name: ethtool
38+
state: present
39+
use: "{{ (__network_is_ostree | d(false)) |
40+
ternary('ansible.posix.rhel_rpm_ostree', omit) }}"
41+
42+
- name: Retrieve MAC address using ethtool
43+
command: ethtool -P {{ interface }}
44+
register: mac_address_result
45+
changed_when: false
46+
failed_when: mac_address_result.rc != 0
47+
48+
- name: Set the MAC address variable
49+
set_fact:
50+
mac: "{{ mac_address_result.stdout_lines[-1].split(' ')[-1] }}"
51+
52+
- name: Display the retrieved MAC address
53+
debug:
54+
msg: "Retrieved MAC address for {{ interface }}: {{ mac }}"
55+
56+
- name: Test the MAC address match
57+
tags:
58+
- tests::match:match
59+
block:
60+
- name: Include the task 'run_test.yml'
61+
include_tasks: tasks/run_test.yml
62+
vars:
63+
lsr_description: Test MAC address match on device
64+
lsr_setup:
65+
- tasks/find+remove_profile.yml
66+
- tasks/assert_profile_absent.yml
67+
lsr_test:
68+
- tasks/create_mac_address_match.yml
69+
- tasks/create_mac_address_match.yml
70+
lsr_assert:
71+
- tasks/assert_profile_present.yml
72+
- tasks/assert_network_connections_succeeded.yml
73+
lsr_cleanup:
74+
- tasks/cleanup_vlan_and_parent_profile+device.yml
75+
- tasks/check_network_dns.yml

‎tests/tasks/assert_network_connections_succeeded.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
# SPDX-License-Identifier: BSD-3-Clause
2+
---
3+
- name: Assert that configuring network connections is succeeded
4+
assert:
5+
that:
6+
- __network_connections_result.failed == false
7+
msg: Configuring network connections is failed with the error
8+
"{{ __network_connections_result.stderr }}"

‎tests/tasks/cleanup_vlan_and_parent_profile+device.yml‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
# SPDX-License-Identifier: BSD-3-Clause
2+
---
3+
- name: Clean up the test devices and the connection profiles
4+
tags:
5+
- "tests::cleanup"
6+
block:
7+
- name: Import network role
8+
import_role:
9+
name: linux-system-roles.network
10+
vars:
11+
network_connections:
12+
- name: "{{ profile }}"
13+
persistent_state: absent
14+
state: down
15+
- name: "{{ vlan_profile1 }}"
16+
persistent_state: absent
17+
state: down
18+
- name: "{{ vlan_profile2 }}"
19+
persistent_state: absent
20+
state: down
21+
failed_when: false
22+
- name: Delete the device '{{ interface }}'
23+
command: ip link del {{ interface }}
24+
failed_when: false
25+
changed_when: false
26+
...

‎tests/tasks/create_mac_address_match.yml‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# SPDX-License-Identifier: BSD-3-Clause
2+
---
3+
- name: Include network role
4+
include_role:
5+
name: linux-system-roles.network
6+
vars:
7+
network_connections:
8+
- name: "{{ interface }}"
9+
state: up
10+
persistent_state: present
11+
autoconnect: true
12+
type: ethernet
13+
interface_name: "{{ interface }}"
14+
mac: "{{ mac }}"
15+
ip:
16+
dhcp4: false
17+
auto6: false
18+
19+
- name: "{{ vlan_profile1 }}"
20+
state: up
21+
persistent_state: present
22+
type: vlan
23+
parent: "{{ interface }}"
24+
vlan:
25+
id: 3732
26+
autoconnect: true
27+
ip:
28+
auto_gateway: false
29+
gateway4: 10.10.0.1
30+
address: 10.10.0.6/24
31+
dhcp4: false
32+
auto6: false
33+
34+
- name: "{{ vlan_profile2 }}"
35+
state: up
36+
persistent_state: present
37+
type: vlan
38+
parent: "{{ interface }}"
39+
vlan:
40+
id: 120
41+
autoconnect: true
42+
ip:
43+
auto_gateway: false
44+
gateway4: 10.10.120.1
45+
address: 10.10.120.120/24
46+
dhcp4: false
47+
auto6: false
48+
- name: Show result
49+
debug:
50+
var: __network_connections_result
51+
...

‎tests/tasks/find+remove_profile.yml‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# SPDX-License-Identifier: BSD-3-Clause
2+
---
3+
- name: Get connection profile for '{{ interface }}'
4+
command: "nmcli -g GENERAL.CONNECTION device show {{ interface }}"
5+
register: connection_name
6+
changed_when: false
7+
8+
- name: Bring down and delete the connection profile for '{{ interface }}'
9+
include_role:
10+
name: linux-system-roles.network
11+
vars:
12+
network_connections:
13+
- name: "{{ connection_name.stdout }}"
14+
persistent_state: absent
15+
state: down

‎tests/tests_mac_address_match_nm.yml‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# SPDX-License-Identifier: BSD-3-Clause
2+
# This file was generated by ensure_provider_tests.py
3+
---
4+
# set network provider and gather facts
5+
# yamllint disable rule:line-length
6+
- name: Run playbook 'playbooks/tests_mac_address_match.yml' with nm as provider
7+
hosts: all
8+
tasks:
9+
- name: Include the task 'el_repo_setup.yml'
10+
include_tasks: tasks/el_repo_setup.yml
11+
- name: Set network provider to 'nm'
12+
set_fact:
13+
network_provider: nm
14+
tags:
15+
- always
16+
17+
18+
# The test requires or should run with NetworkManager, therefore it cannot run
19+
# on RHEL/CentOS 6
20+
- name: Import the playbook 'playbooks/tests_mac_address_match.yml'
21+
import_playbook: playbooks/tests_mac_address_match.yml
22+
when:
23+
- ansible_distribution_major_version != '6'

0 commit comments

Comments
 (0)