From d10feb642f2adacb90c83e383fd387e8f4e87295 Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Thu, 26 Jan 2023 17:04:55 +0100 Subject: [PATCH] Remove LB from LS belonging to provider networks In core OVN, LBs on switches with localnet ports (i.e., neutron provider networks) don't work if traffic comes from localnet [1] In order to force NAT to happen at the virtual router instead of the LS level, when the VIP of the LoadBalancer is associated to a provider network we should avoid adding the LB to the LS associated to the provider network [1] https://bugzilla.redhat.com/show_bug.cgi?id=2164652 Closes-Bug: #2003997 Change-Id: I009ddd2604d208bbf793e2d19d4195b77726f7b2 --- ovn_octavia_provider/helper.py | 17 +++++++--- ovn_octavia_provider/tests/functional/base.py | 7 ++++ .../tests/unit/test_helper.py | 32 +++++++++++++------ 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/ovn_octavia_provider/helper.py b/ovn_octavia_provider/helper.py index 28062d40..8c3f4391 100644 --- a/ovn_octavia_provider/helper.py +++ b/ovn_octavia_provider/helper.py @@ -18,6 +18,7 @@ import re import threading import netaddr +from neutron_lib.api.definitions import provider_net from neutron_lib import constants as n_const from neutronclient.common import exceptions as n_exc from octavia_lib.api.drivers import data_models as o_datamodels @@ -989,11 +990,17 @@ class OvnProviderHelper(): loadbalancer[constants.ID], protocol=protocol) ovn_lb = ovn_lb if protocol else ovn_lb[0] - # NOTE(froyo): This is the association of the lb to the VIP ls - # so this is executed right away - self._update_lb_to_ls_association( - ovn_lb, network_id=port['network_id'], - associate=True, update_ls_ref=True) + + # NOTE(ltomasbo): If the VIP is on a provider network, it does + # not need to be associated to its LS + network = neutron_client.show_network( + port['network_id'])['network'] + if not network.get(provider_net.PHYSICAL_NETWORK, False): + # NOTE(froyo): This is the association of the lb to the VIP ls + # so this is executed right away + self._update_lb_to_ls_association( + ovn_lb, network_id=port['network_id'], + associate=True, update_ls_ref=True) ls_name = utils.ovn_name(port['network_id']) ovn_ls = self.ovn_nbdb_api.ls_get(ls_name).execute( check_error=True) diff --git a/ovn_octavia_provider/tests/functional/base.py b/ovn_octavia_provider/tests/functional/base.py index 8886d4fb..f5b73a69 100644 --- a/ovn_octavia_provider/tests/functional/base.py +++ b/ovn_octavia_provider/tests/functional/base.py @@ -57,6 +57,7 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, self.fake_neutron_client = mock.MagicMock() clients.get_neutron_client = mock.MagicMock() clients.get_neutron_client.return_value = self.fake_neutron_client + self.fake_neutron_client.show_network = self._mock_show_network self.fake_neutron_client.show_subnet = self._mock_show_subnet self.fake_neutron_client.list_ports = self._mock_list_ports self.fake_neutron_client.show_port = self._mock_show_port @@ -66,6 +67,12 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, self._local_port_cache = {'ports': []} self.core_plugin = directory.get_plugin() + def _mock_show_network(self, network_id): + network = {} + network['id'] = network_id + network['provider:physical_network'] = None + return {'network': network} + def _mock_show_subnet(self, subnet_id): subnet = {} subnet['network_id'] = self._local_net_cache[subnet_id] diff --git a/ovn_octavia_provider/tests/unit/test_helper.py b/ovn_octavia_provider/tests/unit/test_helper.py index a3fd068a..2734a34b 100644 --- a/ovn_octavia_provider/tests/unit/test_helper.py +++ b/ovn_octavia_provider/tests/unit/test_helper.py @@ -14,6 +14,7 @@ import copy from unittest import mock +from neutron_lib.api.definitions import provider_net from neutron_lib import constants as n_const from neutronclient.common import exceptions as n_exc from octavia_lib.api.drivers import data_models @@ -643,7 +644,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): selection_fields=['ip_src', 'ip_dst', 'tp_src', 'tp_dst']) @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client') - def _test_lb_create_on_multi_protocol(self, protocol, net_cli): + def _test_lb_create_on_multi_protocol(self, protocol, provider, net_cli): """This test situation when new protocol is added to the same loadbalancer and we need to add @@ -654,6 +655,11 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): self.lb[ovn_const.LB_EXT_IDS_LR_REF_KEY] = 'foo' self.lb[ovn_const.LB_EXT_IDS_LS_REFS_KEY] = '{\"neutron-foo\": 1}' net_cli.return_value.list_ports.return_value = self.ports + fake_network = {'id': self.lb['vip_network_id'], + provider_net.PHYSICAL_NETWORK: provider} + net_cli.return_value.show_network.return_value = { + 'network': fake_network} + status = self.helper.lb_create(self.lb, protocol=protocol) self.assertEqual(status['loadbalancers'][0]['provisioning_status'], constants.ACTIVE) @@ -668,18 +674,26 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase): name=mock.ANY, protocol=protocol.lower(), selection_fields=['ip_src', 'ip_dst', 'tp_src', 'tp_dst']) - self.helper._update_lb_to_ls_association.assert_has_calls([ - mock.call(self.ovn_lb, associate=True, - network_id=self.lb['vip_network_id'], - update_ls_ref=True), - mock.call(self.ovn_lb, associate=True, network_id='foo', - update_ls_ref=True)]) + if provider: + self.helper._update_lb_to_ls_association.assert_not_called() + else: + self.helper._update_lb_to_ls_association.assert_has_calls([ + mock.call(self.ovn_lb, associate=True, + network_id=self.lb['vip_network_id'], + update_ls_ref=True), + mock.call(self.ovn_lb, associate=True, network_id='foo', + update_ls_ref=True)]) def test_lb_create_on_multi_protocol_UDP(self): - self._test_lb_create_on_multi_protocol('UDP') + self._test_lb_create_on_multi_protocol('UDP', None) def test_lb_create_on_multi_protocol_SCTP(self): - self._test_lb_create_on_multi_protocol('SCTP') + self._test_lb_create_on_multi_protocol('SCTP', None) + + def _test_lb_create_on_provider_network(self): + # Test case for LB created on provider network. + # Ensure LB is not associated to the LS in that case + self._test_lb_create_on_multi_protocol('TCP', "provider") @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client') def test_lb_create_neutron_client_exception(self, net_cli):