From c4dd87f9b119ccaef2d01ae35c9fa0ffbe5f23b2 Mon Sep 17 00:00:00 2001 From: Gregory Thiemonge Date: Thu, 27 Apr 2023 06:00:52 -0400 Subject: [PATCH] Avoid interface name collisions in the amphora After unpluging a network, the octavia-agent may try to plug a new network to a new interface while another interface with the same name already exists. Closes-Bug: #2017894 Change-Id: I7f150c2c1c3da706055aba5e02b5ad7c032806f1 --- .../backends/agent/api_server/plug.py | 22 +++++++--- .../backend/agent/api_server/test_server.py | 19 ++++---- .../backends/agent/api_server/test_plug.py | 43 +++++++++++++++++-- ...-interface-collision-939fd32587ea3344.yaml | 8 ++++ 4 files changed, 72 insertions(+), 20 deletions(-) create mode 100644 releasenotes/notes/fix-network-interface-collision-939fd32587ea3344.yaml diff --git a/octavia/amphorae/backends/agent/api_server/plug.py b/octavia/amphorae/backends/agent/api_server/plug.py index bfdd687c1d..414b4f1853 100644 --- a/octavia/amphorae/backends/agent/api_server/plug.py +++ b/octavia/amphorae/backends/agent/api_server/plug.py @@ -14,6 +14,7 @@ # under the License. import ipaddress +import itertools import os import socket import stat @@ -212,13 +213,7 @@ class Plug(object): # We need to determine the interface name when inside the namespace # to avoid name conflicts - with pyroute2.NetNS(consts.AMPHORA_NAMESPACE, - flags=os.O_CREAT) as netns: - - # 1 means just loopback, but we should already have a VIP. This - # works for the add/delete/add case as we don't delete interfaces - # Note, eth0 is skipped because that is the VIP interface - netns_interface = 'eth{0}'.format(len(netns.get_links())) + netns_interface = self._netns_get_next_interface() LOG.info('Plugged interface %s will become %s in the namespace %s', default_netns_interface, netns_interface, @@ -295,3 +290,16 @@ class Plug(object): def _netns_interface_exists(self, mac_address): return self._netns_interface_by_mac(mac_address) is not None + + def _netns_get_next_interface(self): + with pyroute2.NetNS(consts.AMPHORA_NAMESPACE, + flags=os.O_CREAT) as netns: + existing_ifaces = [ + dict(link['attrs']).get(consts.IFLA_IFNAME) + for link in netns.get_links()] + # find the first unused ethXXX + for idx in itertools.count(start=2): + iface_name = f"eth{idx}" + if iface_name not in existing_ifaces: + break + return iface_name diff --git a/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py b/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py index 8445566fda..95c78f3bb7 100644 --- a/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py +++ b/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py @@ -1008,7 +1008,9 @@ class TestServerTestCase(base.TestCase): mock_int_exists.return_value = False netns_handle = mock_netns.return_value.__enter__.return_value - netns_handle.get_links.return_value = [0] * test_int_num + netns_handle.get_links.return_value = [ + {'attrs': [['IFLA_IFNAME', f'eth{idx}']]} + for idx in range(test_int_num)] mock_isfile.return_value = True mock_check_output.return_value = b"1\n2\n3\n" @@ -1396,7 +1398,7 @@ class TestServerTestCase(base.TestCase): netns_handle = mock_netns.return_value.__enter__.return_value netns_handle.get_links.return_value = [{ - 'attrs': [['IFLA_IFNAME', consts.NETNS_PRIMARY_INTERFACE]]}] + 'attrs': [['IFLA_IFNAME', 'eth2']]}] port_info = {'mac_address': MAC, 'mtu': 1450, 'fixed_ips': [ {'ip_address': IP, 'subnet_cidr': SUBNET_CIDR, @@ -1405,8 +1407,7 @@ class TestServerTestCase(base.TestCase): flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC mode = stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH - file_name = '/etc/octavia/interfaces/{}.json'.format( - consts.NETNS_PRIMARY_INTERFACE) + file_name = '/etc/octavia/interfaces/eth3.json' m = self.useFixture(test_utils.OpenFixture(file_name)).mock_open with mock.patch('os.open') as mock_open, mock.patch.object( @@ -1436,7 +1437,7 @@ class TestServerTestCase(base.TestCase): mock_fdopen.assert_any_call(123, 'r+') expected_dict = { - consts.NAME: consts.NETNS_PRIMARY_INTERFACE, + consts.NAME: 'eth3', consts.MTU: 1450, consts.ADDRESSES: [ { @@ -1458,13 +1459,12 @@ class TestServerTestCase(base.TestCase): consts.SCRIPTS: { consts.IFACE_UP: [{ consts.COMMAND: ( - "/usr/local/bin/lvs-masquerade.sh add ipv4 " - "{}".format(consts.NETNS_PRIMARY_INTERFACE)) + "/usr/local/bin/lvs-masquerade.sh add ipv4 eth3") }], consts.IFACE_DOWN: [{ consts.COMMAND: ( "/usr/local/bin/lvs-masquerade.sh delete ipv4 " - "{}".format(consts.NETNS_PRIMARY_INTERFACE)) + "eth3") }] } } @@ -1476,8 +1476,7 @@ class TestServerTestCase(base.TestCase): mock_check_output.assert_called_with( ['ip', 'netns', 'exec', consts.AMPHORA_NAMESPACE, - 'amphora-interface', 'up', - consts.NETNS_PRIMARY_INTERFACE], stderr=-2) + 'amphora-interface', 'up', 'eth3'], stderr=-2) def test_ubuntu_plug_VIP4(self): self._test_plug_VIP4(consts.UBUNTU) diff --git a/octavia/tests/unit/amphorae/backends/agent/api_server/test_plug.py b/octavia/tests/unit/amphorae/backends/agent/api_server/test_plug.py index 7bd2cb51c6..4090b04c3d 100644 --- a/octavia/tests/unit/amphorae/backends/agent/api_server/test_plug.py +++ b/octavia/tests/unit/amphorae/backends/agent/api_server/test_plug.py @@ -285,13 +285,13 @@ class TestPlug(base.TestCase): self.test_plug.plug_network(FAKE_MAC_ADDRESS, fixed_ips, 1400) mock_write_port_interface.assert_called_once_with( - interface='eth0', fixed_ips=fixed_ips, mtu=mtu) - mock_if_up.assert_called_once_with('eth0', 'network') + interface='eth2', fixed_ips=fixed_ips, mtu=mtu) + mock_if_up.assert_called_once_with('eth2', 'network') mock_send_member_adv.assert_called_once_with(fixed_ips) mock_webob.Response.assert_any_call( json={'message': 'OK', - 'details': 'Plugged on interface eth0'}, + 'details': 'Plugged on interface eth2'}, status=202) @mock.patch.object(plug, "webob") @@ -411,3 +411,40 @@ class TestPlug(base.TestCase): 'details': 'Updated existing interface {}'.format( FAKE_INTERFACE)}, status=202) + + @mock.patch('pyroute2.NetNS', create=True) + def test__netns_get_next_interface(self, mock_netns): + netns_handle = mock_netns.return_value.__enter__.return_value + + netns_handle.get_links.return_value = [ + {'attrs': [['IFLA_IFNAME', 'lo']]}, + ] + + ifname = self.test_plug._netns_get_next_interface() + self.assertEqual('eth2', ifname) + + netns_handle.get_links.return_value = [ + {'attrs': [['IFLA_IFNAME', 'lo']]}, + {'attrs': [['IFLA_IFNAME', 'eth1']]}, + {'attrs': [['IFLA_IFNAME', 'eth2']]}, + {'attrs': [['IFLA_IFNAME', 'eth3']]}, + ] + + ifname = self.test_plug._netns_get_next_interface() + self.assertEqual('eth4', ifname) + + netns_handle.get_links.return_value = [ + {'attrs': [['IFLA_IFNAME', 'lo']]}, + {'attrs': [['IFLA_IFNAME', 'eth1']]}, + {'attrs': [['IFLA_IFNAME', 'eth3']]}, + ] + + ifname = self.test_plug._netns_get_next_interface() + self.assertEqual('eth2', ifname) + + netns_handle.get_links.return_value = [ + {'attrs': [['IFLA_IFNAME', f'eth{idx}']]} + for idx in range(2, 1000)] + + ifname = self.test_plug._netns_get_next_interface() + self.assertEqual('eth1000', ifname) diff --git a/releasenotes/notes/fix-network-interface-collision-939fd32587ea3344.yaml b/releasenotes/notes/fix-network-interface-collision-939fd32587ea3344.yaml new file mode 100644 index 0000000000..7dd8776dc4 --- /dev/null +++ b/releasenotes/notes/fix-network-interface-collision-939fd32587ea3344.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixed a potential error when plugging a member from a new network after + deleting another member and unplugging its network. Octavia may have tried + to plug the new network to a new interface but with an already existing + name. + This fix requires to update the Amphora image.