Skip to content

Commit

Permalink
cli: drop python-netifaces
Browse files Browse the repository at this point in the history
netifaces doesn't return the permanent MAC address of physical
interfaces. This is causing issues when we try to find interfaces that
are matched by MAC address but their MACs changed (because they were
added to a bond for example).

We only use it to retrieve the list of interfaces and their MACs so
let's drop it and implement this functionalities in the CLI.

get_interfaces() will simply call "ip link" and return a list with the
interface names.

get_interface_macaddress() will try to get the permanent MAC address and
fallback to the transient address if it can't be found.
  • Loading branch information
daniloegea committed Aug 14, 2024
1 parent 900b5a7 commit 0f4626d
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 44 deletions.
11 changes: 5 additions & 6 deletions netplan_cli/cli/commands/apply.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import shutil
import tempfile
import filecmp
import netifaces
import time

from .. import utils
Expand Down Expand Up @@ -117,7 +116,7 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
old_ovs_glob.remove(ovs_cleanup_service)
old_files_ovs = bool(old_ovs_glob)
old_nm_glob = glob.glob('/run/NetworkManager/system-connections/netplan-*')
nm_ifaces = utils.nm_interfaces(old_nm_glob, netifaces.interfaces())
nm_ifaces = utils.nm_interfaces(old_nm_glob, utils.get_interfaces())
old_files_nm = bool(old_nm_glob)

restart_networkd = False
Expand Down Expand Up @@ -151,7 +150,7 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
if comp.left_only or comp.right_only or comp.diff_files:
restart_networkd = True

devices = netifaces.interfaces()
devices = utils.get_interfaces()

restart_ovs_glob = glob.glob('/run/systemd/system/netplan-ovs-*')
# Ignore netplan-ovs-cleanup.service, as it can always be there
Expand Down Expand Up @@ -203,7 +202,7 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
logging.debug('no netplan generated NM configuration exists')

# Refresh devices now; restarting a backend might have made something appear.
devices = netifaces.interfaces()
devices = utils.get_interfaces()

# evaluate config for extra steps we need to take (like renaming)
# for now, only applies to non-virtual (real) devices.
Expand All @@ -223,7 +222,7 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
# the interface name, if it was already renamed once (e.g. during boot),
# because of the NamePolicy=keep default:
# https://www.freedesktop.org/software/systemd/man/systemd.net-naming-scheme.html
devices = netifaces.interfaces()
devices = utils.get_interfaces()
for device in devices:
logging.debug('netplan triggering .link rules for %s', device)
try:
Expand All @@ -239,7 +238,7 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
except subprocess.CalledProcessError:
logging.debug('Ignoring device without syspath: %s', device)

devices_after_udev = netifaces.interfaces()
devices_after_udev = utils.get_interfaces()
# apply some more changes manually
for iface, settings in changes.items():
# rename non-critical network interfaces
Expand Down
8 changes: 3 additions & 5 deletions netplan_cli/cli/sriov.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
from ..configmanager import ConfigurationError
import netplan

import netifaces


# PCIDevice class originates from mlnx_switchdev_mode/sriovify.py
# Copyright 2019 Canonical Ltd, Apache License, Version 2.0
Expand Down Expand Up @@ -223,7 +221,7 @@ def _get_interface_name_for_netdef(netdef: netplan.NetDefinition) -> Optional[st
Try to match a netdef with the real system network interface.
Throws ConfigurationError if there is more than one match.
"""
interfaces: List[str] = netifaces.interfaces()
interfaces: List[str] = utils.get_interfaces()
if netdef._has_match:
# now here it's a bit tricky
set_name: str = netdef.set_name
Expand Down Expand Up @@ -455,7 +453,7 @@ def apply_sriov_config(config_manager, rootdir='/'):
them and perform all other necessary setup.
"""
config_manager.parse()
interfaces = netifaces.interfaces()
interfaces = utils.get_interfaces()
np_state = config_manager.np_state

# for sr-iov devices, we identify VFs by them having a link: field
Expand Down Expand Up @@ -487,7 +485,7 @@ def apply_sriov_config(config_manager, rootdir='/'):

# also, since the VF number changed, the interfaces list also
# changed, so we need to refresh it
interfaces = netifaces.interfaces()
interfaces = utils.get_interfaces()

# now in theory we should have all the new VFs set up and existing;
# this is needed because we will have to now match the defined VF
Expand Down
43 changes: 38 additions & 5 deletions netplan_cli/cli/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
import logging
import argparse
import subprocess
import netifaces
import fnmatch
import re
import json

from ..configmanager import ConfigurationError
from netplan import NetDefinition, NetplanException
Expand Down Expand Up @@ -196,10 +196,43 @@ def get_interface_driver_name(interface, only_down=False): # pragma: nocover (c
return driver_name


def get_interface_macaddress(interface):
# return an empty list (and string) if no LL data can be found
link = netifaces.ifaddresses(interface).get(netifaces.AF_LINK, [{}])[0]
return link.get('addr', '')
def _get_permanent_macaddress(interface: str) -> str:
mac = ""
try:
out = subprocess.check_output(['ethtool', '-P', interface]).decode('utf-8')
split = out.split(': ')
if len(split) == 2 and split[1].strip() != 'not set':
mac = split[1].strip()
except Exception:
return mac

return mac


def _get_macaddress(interface: str) -> str:
try:
with open(f'/sys/class/net/{interface}/address') as f:
return f.read().strip()
except Exception:
return ""


def get_interfaces() -> list[str]:
try:
out = subprocess.check_output(['ip', '--json', 'link']).decode('utf-8')
out_json = json.loads(out)
return [iface['ifname'] for iface in out_json]
except Exception:
return []


def get_interface_macaddress(interface: str) -> str:
mac = _get_permanent_macaddress(interface)

if not mac:
mac = _get_macaddress(interface)

return mac


def find_matching_iface(interfaces: list, netdef):
Expand Down
42 changes: 21 additions & 21 deletions tests/test_sriov.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ def _prepare_sysfs_dir_structure(self, pf=('enp2', '0000:00:1f.0'),
for i in range(len(vfs)):
os.symlink(os.path.join('../../..', vfs[i][1]), os.path.join(pf_dev_path, 'virtfn'+str(i)))

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_get_vf_count_vfs_and_pfs(self, gim, gidn, ifaces):
Expand Down Expand Up @@ -207,7 +207,7 @@ def test_get_vf_count_vfs_and_pfs(self, gim, gidn, ifaces):
{'enp1': 'enp1', 'enp2': 'enp2', 'enp3': 'enp3',
'enpx': 'enp5', 'enp8': 'enp8', 'enp10': 'enp10'})

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_get_physical_functions(self, gim, gidn, ifaces):
Expand Down Expand Up @@ -268,7 +268,7 @@ def test_get_physical_functions(self, gim, gidn, ifaces):
{'enp1': 'enp1', 'enp2': 'enp2', 'enp3': 'enp3',
'enpx': 'enp5', 'enp8': 'enp8', 'enp10': 'enp10'})

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_get_vf_number_per_pf(self, gim, gidn, ifaces):
Expand Down Expand Up @@ -327,7 +327,7 @@ def test_get_vf_number_per_pf(self, gim, gidn, ifaces):
vf_counts,
{'enp1': 2, 'enp2': 2, 'enp3': 1, 'enp5': 1, 'enp8': 7})

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_get_virtual_functions(self, gim, gidn, ifaces):
Expand Down Expand Up @@ -386,7 +386,7 @@ def test_get_virtual_functions(self, gim, gidn, ifaces):
{'enp1s16f1', 'enp1s16f2', 'enp2s16f1',
'enp2s16f2', 'enp3s16f1', 'enpxs16f1'})

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_get_vf_count_vfs_and_pfs_set_name(self, gim, gidn, ifaces):
Expand Down Expand Up @@ -433,7 +433,7 @@ def test_get_vf_count_vfs_and_pfs_set_name(self, gim, gidn, ifaces):
pfs,
{'enp1': 'pf1', 'enp8': 'enp8'})

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_get_vf_count_vfs_and_pfs_many_match(self, gim, gidn, ifaces):
Expand Down Expand Up @@ -464,7 +464,7 @@ def test_get_vf_count_vfs_and_pfs_many_match(self, gim, gidn, ifaces):
self.assertIn('matched more than one interface for a PF device: enpx',
str(e.exception))

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_get_vf_count_vfs_and_pfs_not_enough_explicit(self, gim, gidn, ifaces):
Expand Down Expand Up @@ -646,7 +646,6 @@ def test_apply_vlan_filter_for_vf_failed_ip_link_set(self, check_call):
self.assertIn('failed setting SR-IOV VLAN filter for vlan vlan10',
str(e.exception))

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.sriov._get_vf_number_per_pf')
@patch('netplan_cli.cli.sriov._get_virtual_functions')
@patch('netplan_cli.cli.sriov._get_physical_functions')
Expand All @@ -655,8 +654,9 @@ def test_apply_vlan_filter_for_vf_failed_ip_link_set(self, check_call):
@patch('netplan_cli.cli.sriov.apply_vlan_filter_for_vf')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_apply_sriov_config(self, gim, gidn, apply_vlan, quirks,
set_numvfs, get_phys, get_virt, get_num, netifs):
@patch('netplan_cli.cli.utils.get_interfaces')
def test_apply_sriov_config(self, netifs, gim, gidn, apply_vlan, quirks,
set_numvfs, get_phys, get_virt, get_num):
# set up the environment
with open(os.path.join(self.workdir.name, "etc/netplan/test.yaml"), 'w') as fd:
print('''network:
Expand Down Expand Up @@ -711,7 +711,6 @@ def test_apply_sriov_config(self, gim, gidn, apply_vlan, quirks,
# only one had a hardware vlan
apply_vlan.assert_called_once_with('enp2', 'enp2s16f1', 'vf1.15', 15)

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.sriov._get_vf_number_per_pf')
@patch('netplan_cli.cli.sriov._get_virtual_functions')
@patch('netplan_cli.cli.sriov._get_physical_functions')
Expand All @@ -720,8 +719,9 @@ def test_apply_sriov_config(self, gim, gidn, apply_vlan, quirks,
@patch('netplan_cli.cli.sriov.apply_vlan_filter_for_vf')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_apply_sriov_config_invalid_vlan(self, gim, gidn, apply_vlan, quirks,
set_numvfs, get_phys, get_virt, get_num, netifs):
@patch('netplan_cli.cli.utils.get_interfaces')
def test_apply_sriov_config_invalid_vlan(self, netifs, gim, gidn, apply_vlan, quirks,
set_numvfs, get_phys, get_virt, get_num):
# set up the environment
with open(os.path.join(self.workdir.name, "etc/netplan/test.yaml"), 'w') as fd:
print('''network:
Expand Down Expand Up @@ -783,7 +783,6 @@ def test_apply_sriov_invalid_link_no_vf(self):
'either not a VF or has no matches',
logs.output[0])

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.sriov._get_vf_number_per_pf')
@patch('netplan_cli.cli.sriov._get_virtual_functions')
@patch('netplan_cli.cli.sriov._get_physical_functions')
Expand All @@ -792,8 +791,9 @@ def test_apply_sriov_invalid_link_no_vf(self):
@patch('netplan_cli.cli.sriov.apply_vlan_filter_for_vf')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_apply_sriov_config_too_many_vlans(self, gim, gidn, apply_vlan, quirks,
set_numvfs, get_phys, get_virt, get_num, netifs):
@patch('netplan_cli.cli.utils.get_interfaces')
def test_apply_sriov_config_too_many_vlans(self, netifs, gim, gidn, apply_vlan, quirks,
set_numvfs, get_phys, get_virt, get_num):
# set up the environment
with open(os.path.join(self.workdir.name, "etc/netplan/test.yaml"), 'w') as fd:
print('''network:
Expand Down Expand Up @@ -842,7 +842,6 @@ def test_apply_sriov_config_too_many_vlans(self, gim, gidn, apply_vlan, quirks,
str(e.exception))
self.assertEqual(apply_vlan.call_count, 1)

@patch('netifaces.interfaces')
@patch('netplan_cli.cli.sriov._get_vf_number_per_pf')
@patch('netplan_cli.cli.sriov._get_virtual_functions')
@patch('netplan_cli.cli.sriov._get_physical_functions')
Expand All @@ -851,8 +850,9 @@ def test_apply_sriov_config_too_many_vlans(self, gim, gidn, apply_vlan, quirks,
@patch('netplan_cli.cli.sriov.apply_vlan_filter_for_vf')
@patch('netplan_cli.cli.utils.get_interface_driver_name')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
def test_apply_sriov_config_many_match(self, gim, gidn, apply_vlan, quirks,
set_numvfs, get_phys, get_virt, get_num, netifs):
@patch('netplan_cli.cli.utils.get_interfaces')
def test_apply_sriov_config_many_match(self, netifs, gim, gidn, apply_vlan, quirks,
set_numvfs, get_phys, get_virt, get_num):
# set up the environment
with open(os.path.join(self.workdir.name, "etc/netplan/test.yaml"), 'w') as fd:
print('''network:
Expand Down Expand Up @@ -919,7 +919,7 @@ def test_unit_class_PCIDevice(self):
self.assertTrue(pcidev.is_vf)

@patch('netplan_cli.cli.utils.get_interface_macaddress')
@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.sriov._get_vf_number_per_pf')
@patch('netplan_cli.cli.sriov._get_virtual_functions')
@patch('netplan_cli.cli.sriov._get_physical_functions')
Expand Down Expand Up @@ -1022,7 +1022,7 @@ def driver_mock_open(*args, **kwargs):

@patch('netplan_cli.cli.sriov.unbind_vfs')
@patch('netplan_cli.cli.utils.get_interface_macaddress')
@patch('netifaces.interfaces')
@patch('netplan_cli.cli.utils.get_interfaces')
@patch('netplan_cli.cli.sriov._get_vf_number_per_pf')
@patch('netplan_cli.cli.sriov._get_virtual_functions')
@patch('netplan_cli.cli.sriov._get_physical_functions')
Expand Down
35 changes: 28 additions & 7 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import unittest
import tempfile
import glob
import netifaces
import netplan

from contextlib import redirect_stdout
Expand Down Expand Up @@ -235,16 +234,38 @@ def test_find_matching_iface_name_and_drivers(self, gim, gidn):
iface = utils.find_matching_iface(DEVICES, state['netplan-id'])
self.assertEqual(iface, 'ens4')

@patch('netifaces.ifaddresses')
def test_interface_macaddress(self, ifaddr):
ifaddr.side_effect = lambda _: {netifaces.AF_LINK: [{'addr': '00:01:02:03:04:05'}]}
@patch('netplan_cli.cli.utils._get_macaddress')
@patch('netplan_cli.cli.utils._get_permanent_macaddress')
def test_interface_macaddress(self, getpm, getm):
getpm.return_value = ''
getm.return_value = '00:01:02:03:04:05'
self.assertEqual(utils.get_interface_macaddress('eth42'), '00:01:02:03:04:05')

@patch('netifaces.ifaddresses')
def test_interface_macaddress_empty(self, ifaddr):
ifaddr.side_effect = lambda _: {}
@patch('builtins.open')
@patch('subprocess.check_output')
def test_interface_macaddress_empty(self, subp, o):
subp.side_effect = Exception
o.side_effect = Exception
self.assertEqual(utils.get_interface_macaddress('eth42'), '')

@patch('subprocess.check_output')
def test_interface_permanent_macaddress(self, subp):
subp.return_value = b'Permanent address: 00:01:02:03:04:05'
self.assertEqual(utils.get_interface_macaddress('eth42'), '00:01:02:03:04:05')

@patch('builtins.open')
@patch('subprocess.check_output')
def test_interface_nonpermanent_macaddress(self, subp, o):
file = io.StringIO('00:01:02:03:04:05')
subp.side_effect = Exception
o.return_value = file
self.assertEqual(utils.get_interface_macaddress('eth42'), '00:01:02:03:04:05')

@patch('subprocess.check_output')
def test_get_interfaces_empty(self, subp):
subp.side_effect = Exception
self.assertListEqual(utils.get_interfaces(), [])

def test_systemctl(self):
self.mock_systemctl = MockCmd('systemctl')
path_env = os.environ['PATH']
Expand Down

0 comments on commit 0f4626d

Please sign in to comment.