From b1ba2a6ba07563d6f56355d0a9f497bd6c13d3bf Mon Sep 17 00:00:00 2001 From: Jorge Merlino Date: Wed, 26 Oct 2022 12:51:15 -0300 Subject: [PATCH 1/5] Check if the name to set to an interface is already an altname This prevents an exception caused by trying to set a name that already exists in an interface --- netplan/cli/commands/apply.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/netplan/cli/commands/apply.py b/netplan/cli/commands/apply.py index ae2813057..e38cf3348 100644 --- a/netplan/cli/commands/apply.py +++ b/netplan/cli/commands/apply.py @@ -60,6 +60,12 @@ def run(self): # pragma: nocover (covered in autopkgtest) self.parse_args() self.run_command() + @staticmethod + def get_alt_names(iface): + ret = subprocess.run(['ip', 'link', 'show', iface], capture_output=True) + data = ret.stdout.decode('utf-8').split() + return [data[i + 1] for i, x in enumerate(data) if x == "altname"] + def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state_dir=None): # pragma: nocover config_manager = ConfigManager() if state_dir: @@ -237,6 +243,9 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state if iface in devices and new_name in devices_after_udev: logging.debug('Interface rename {} -> {} already happened.'.format(iface, new_name)) continue # re-name already happened via 'udevadm test' + if new_name in self.get_alt_names(iface): + logging.debug('Interface name {} already present as altname on {}.'.format(new_name, iface)) + continue # name already present in interface # bring down the interface, using its current (matched) interface name subprocess.check_call(['ip', 'link', 'set', 'dev', iface, 'down'], stdout=subprocess.DEVNULL, From 3f0d6d1d236b2cbc6e3bb6371abf84986d0f0395 Mon Sep 17 00:00:00 2001 From: Jorge Merlino Date: Thu, 27 Oct 2022 10:54:45 -0300 Subject: [PATCH 2/5] Add unit test for new method Also call static method using class name instead of self instance --- netplan/cli/commands/apply.py | 2 +- tests/test_cli_units.py | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/netplan/cli/commands/apply.py b/netplan/cli/commands/apply.py index e38cf3348..0b1b6fe4d 100644 --- a/netplan/cli/commands/apply.py +++ b/netplan/cli/commands/apply.py @@ -243,7 +243,7 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state if iface in devices and new_name in devices_after_udev: logging.debug('Interface rename {} -> {} already happened.'.format(iface, new_name)) continue # re-name already happened via 'udevadm test' - if new_name in self.get_alt_names(iface): + if new_name in NetplanApply.get_alt_names(iface): logging.debug('Interface name {} already present as altname on {}.'.format(new_name, iface)) continue # name already present in interface # bring down the interface, using its current (matched) interface name diff --git a/tests/test_cli_units.py b/tests/test_cli_units.py index f860965cf..e8f5505a0 100644 --- a/tests/test_cli_units.py +++ b/tests/test_cli_units.py @@ -55,6 +55,18 @@ def test_is_composite_member_with_renderer(self): res = NetplanApply.is_composite_member([{'renderer': 'networkd', 'br0': {'interfaces': ['eth0']}}], 'eth0') self.assertTrue(res) + @patch('subprocess.run') + def test_get_alt_names(self, mock): + stdout_mock = mock.Mock() + stdout_mock.stdout = b"""3: ens4: mtu 8958 qdisc fq_codel state UP mode DEFAULT group default qlen 1000 + link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff + altname enp0s4 + altname enp0s41""" + mock.return_value = stdout_mock + res = NetplanApply.get_alt_names('ens4') + mock.assert_called_with(['ip', 'link', 'show', 'ens4'], capture_output=True) + self.assertEquals(res, ['enp0s4', 'enp0s41']) + @patch('subprocess.check_call') def test_clear_virtual_links(self, mock): # simulate as if 'tun3' would have already been delete another way, From 37e9fb4453c67595afdf8a58ca3b516cbc6b66fb Mon Sep 17 00:00:00 2001 From: Jorge Merlino Date: Thu, 27 Oct 2022 11:40:41 -0300 Subject: [PATCH 3/5] Replace multi line string for string concatenation To comply with codestyle regulations --- tests/test_cli_units.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/test_cli_units.py b/tests/test_cli_units.py index e8f5505a0..e9c4d132d 100644 --- a/tests/test_cli_units.py +++ b/tests/test_cli_units.py @@ -58,10 +58,11 @@ def test_is_composite_member_with_renderer(self): @patch('subprocess.run') def test_get_alt_names(self, mock): stdout_mock = mock.Mock() - stdout_mock.stdout = b"""3: ens4: mtu 8958 qdisc fq_codel state UP mode DEFAULT group default qlen 1000 - link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff - altname enp0s4 - altname enp0s41""" + stdout_mock.stdout = "3: ens4: mtu 8958 qdisc fq_codel state "\ + "UP mode DEFAULT group default qlen 1000\n"\ + " link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff\n"\ + " altname enp0s4\n"\ + " altname enp0s41".encode('utf-8') mock.return_value = stdout_mock res = NetplanApply.get_alt_names('ens4') mock.assert_called_with(['ip', 'link', 'show', 'ens4'], capture_output=True) From 85639283fa855f7f202712c179fba54bbfeb3a0d Mon Sep 17 00:00:00 2001 From: Jorge Merlino Date: Thu, 27 Oct 2022 14:45:35 -0300 Subject: [PATCH 4/5] Change implementation of get_alt_names to use json output Fix unit test to adapt to new implementation --- netplan/cli/commands/apply.py | 9 ++++++--- tests/test_cli_units.py | 16 ++++++++++------ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/netplan/cli/commands/apply.py b/netplan/cli/commands/apply.py index 0b1b6fe4d..7af5d6e93 100644 --- a/netplan/cli/commands/apply.py +++ b/netplan/cli/commands/apply.py @@ -27,6 +27,7 @@ import shutil import netifaces import time +import yaml import netplan.cli.utils as utils from netplan.configmanager import ConfigManager, ConfigurationError @@ -62,9 +63,11 @@ def run(self): # pragma: nocover (covered in autopkgtest) @staticmethod def get_alt_names(iface): - ret = subprocess.run(['ip', 'link', 'show', iface], capture_output=True) - data = ret.stdout.decode('utf-8').split() - return [data[i + 1] for i, x in enumerate(data) if x == "altname"] + ret = subprocess.run(['ip', '-j', 'link', 'show', iface], capture_output=True) + data = yaml.safe_load(ret.stdout.decode('utf-8')) + if data and len(data) == 1: + return data[0].get('altnames', []) + return [] def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state_dir=None): # pragma: nocover config_manager = ConfigManager() diff --git a/tests/test_cli_units.py b/tests/test_cli_units.py index e9c4d132d..d0a66a770 100644 --- a/tests/test_cli_units.py +++ b/tests/test_cli_units.py @@ -58,16 +58,20 @@ def test_is_composite_member_with_renderer(self): @patch('subprocess.run') def test_get_alt_names(self, mock): stdout_mock = mock.Mock() - stdout_mock.stdout = "3: ens4: mtu 8958 qdisc fq_codel state "\ - "UP mode DEFAULT group default qlen 1000\n"\ - " link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff\n"\ - " altname enp0s4\n"\ - " altname enp0s41".encode('utf-8') + stdout_mock.stdout = '[{"ifindex":3,"ifname":"ens4","flags":["BROADCAST","MULTICAST","UP","LOWER_UP"],'\ + '"mtu":8958,"qdisc":"fq_codel","operstate":"UP","linkmode":"DEFAULT","group":"default",'\ + '"txqlen":1000,"link_type":"ether","address":"xx:xx:xx:xx:xx:xx",'\ + '"broadcast":"ff:ff:ff:ff:ff:ff","altnames":["enp0s4","enp0s41"]}]'.encode('utf-8') mock.return_value = stdout_mock res = NetplanApply.get_alt_names('ens4') - mock.assert_called_with(['ip', 'link', 'show', 'ens4'], capture_output=True) + mock.assert_called_with(['ip', '-j', 'link', 'show', 'ens4'], capture_output=True) self.assertEquals(res, ['enp0s4', 'enp0s41']) + mock.reset_mock() + stdout_mock.stdout = ''.encode('utf-8') + res = NetplanApply.get_alt_names('ens4') + mock.assert_called_with(['ip', '-j', 'link', 'show', 'ens4'], capture_output=True) + self.assertEquals(res, []) @patch('subprocess.check_call') def test_clear_virtual_links(self, mock): # simulate as if 'tun3' would have already been delete another way, From 76a85682fa399f423e00cacbeff82a4b3b71385e Mon Sep 17 00:00:00 2001 From: Jorge Merlino Date: Thu, 27 Oct 2022 15:22:42 -0300 Subject: [PATCH 5/5] Add missing blank line --- tests/test_cli_units.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_cli_units.py b/tests/test_cli_units.py index d0a66a770..bc9c53b3e 100644 --- a/tests/test_cli_units.py +++ b/tests/test_cli_units.py @@ -72,6 +72,7 @@ def test_get_alt_names(self, mock): res = NetplanApply.get_alt_names('ens4') mock.assert_called_with(['ip', '-j', 'link', 'show', 'ens4'], capture_output=True) self.assertEquals(res, []) + @patch('subprocess.check_call') def test_clear_virtual_links(self, mock): # simulate as if 'tun3' would have already been delete another way,