Log warning about port forwardings that won't work properly

slawqo · slawqo · commit bd6595ddb866 · 2024-02-14T09:46:28.000Z
This is follow up patch to [1] in which was added warning about incompatible configuration of the vlan/flat networks allowed as tenant networks, distributed routing and port forwardings. In this new patch similar warning is logged every time when port forwarding is created using router which have actually connected vlan or flat networks as "internal networks" (external gateway network is fine) and when distributed routing is enabled in the Neutron config. This patch additionally adds "neutron:is_ext_gw" flag to the Logical_Router_Port's external_ids. With that it's easier to check if network is used as gateway network (no checks needed) or not (checks are perfomed and warning may be logged). [1] https://review.opendev.org/c/openstack/neutron/+/892542 Conflicts: neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py neutron/tests/unit/services/ovn_l3/test_plugin.py Related-Bug: #2028846 Change-Id: I101128bdb421ec83df5cdcb0d486cbafbbca2ce5 (cherry picked from commit a355d2a) (cherry picked from commit 3eaa326)
diff --git a/neutron/common/ovn/utils.py b/neutron/common/ovn/utils.py
@@ -679,6 +679,15 @@ def is_gateway_chassis_invalid(chassis_name, gw_chassis,
 
 
 def is_provider_network(network):
+    """Check if given network is provider network
+    :param network: (str, dict) it can be given as network object or as string
+                    with network ID only. In the latter case, network object
+                    will be loaded from the database
+    """
+    if isinstance(network, str):
+        ctx = n_context.get_admin_context()
+        plugin = directory.get_plugin()
+        network = plugin.get_network(ctx, network)
     return network.get(provider_net.PHYSICAL_NETWORK, False)
 
 
diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py
@@ -971,6 +971,41 @@ def create_router_extra_attributes_registers(self):
 
         raise periodics.NeverAgain()
 
+    # TODO(slaweq): Remove this in the E cycle (C+2 as it will be next SLURP)
+    @periodics.periodic(spacing=600, run_immediately=True)
+    def add_gw_port_info_to_logical_router_port(self):
+        """Add info if LRP is connecting internal subnet or ext gateway."""
+        cmds = []
+        context = n_context.get_admin_context()
+        for router in self._ovn_client._l3_plugin.get_routers(context):
+            ext_gw_networks = [
+                ext_gw['network_id'] for ext_gw in router['external_gateways']]
+            rtr_name = 'neutron-{}'.format(router['id'])
+            ovn_lr = self._nb_idl.get_lrouter(rtr_name)
+            for lrp in ovn_lr.ports:
+                if ovn_const.OVN_ROUTER_IS_EXT_GW in lrp.external_ids:
+                    continue
+                ovn_network_name = lrp.external_ids.get(
+                    ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY)
+                if not ovn_network_name:
+                    continue
+                network_id = ovn_network_name.replace('neutron-', '')
+                if not network_id:
+                    continue
+                is_ext_gw = str(network_id in ext_gw_networks)
+                external_ids = lrp.external_ids
+                external_ids[ovn_const.OVN_ROUTER_IS_EXT_GW] = is_ext_gw
+                cmds.append(
+                    self._nb_idl.update_lrouter_port(
+                        name=lrp.name,
+                        external_ids=external_ids))
+
+        if cmds:
+            with self._nb_idl.transaction(check_error=True) as txn:
+                for cmd in cmds:
+                    txn.add(cmd)
+        raise periodics.NeverAgain()
+
     @periodics.periodic(spacing=600, run_immediately=True)
     def check_router_default_route_empty_dst_ip(self):
         """Check routers with default route with empty dst-ip (LP: #2002993).
diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
@@ -1537,7 +1537,10 @@ def _gen_router_port_ext_ids(self, port):
             ovn_const.OVN_SUBNET_EXT_IDS_KEY:
                 ' '.join(utils.get_port_subnet_ids(port)),
             ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
-                utils.ovn_name(port['network_id'])}
+                utils.ovn_name(port['network_id']),
+            ovn_const.OVN_ROUTER_IS_EXT_GW:
+                str(const.DEVICE_OWNER_ROUTER_GW == port.get('device_owner')),
+        }
 
         router_id = port.get('device_id')
         if router_id:
diff --git a/neutron/services/portforwarding/drivers/ovn/driver.py b/neutron/services/portforwarding/drivers/ovn/driver.py
@@ -17,12 +17,14 @@
 from neutron_lib.plugins import constants as plugin_constants
 from neutron_lib.plugins import directory
 from oslo_log import log
+from oslo_utils import strutils
 from ovsdbapp.backend.ovs_idl import idlutils
 from ovsdbapp import constants as ovsdbapp_const
 
 from neutron.common.ovn import constants as ovn_const
 from neutron.common.ovn import exceptions as ovn_exc
 from neutron.common.ovn import utils as ovn_utils
+from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
 from neutron.db import ovn_revision_numbers_db as db_rev
 from neutron import manager
 from neutron.objects import port_forwarding as port_forwarding_obj
@@ -129,7 +131,41 @@ def _port_forwarding_created(self, ovn_txn, nb_ovn, pf_obj,
                                 "Switch %s failed as it is not found",
                                 lb_name, ls_name)
 
+    def _validate_router_networks(self, nb_ovn, router_id):
+        if not ovn_conf.is_ovn_distributed_floating_ip():
+            return
+        rtr_name = 'neutron-{}'.format(router_id)
+        ovn_lr = nb_ovn.get_lrouter(rtr_name)
+        if not ovn_lr:
+            return
+        for lrouter_port in ovn_lr.ports:
+            is_ext_gw = strutils.bool_from_string(
+                lrouter_port.external_ids.get(ovn_const.OVN_ROUTER_IS_EXT_GW))
+            if is_ext_gw:
+                # NOTE(slaweq): This is external gateway port of the router and
+                # this not needs to be checked
+                continue
+            ovn_network_name = lrouter_port.external_ids.get(
+                ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY)
+            if not ovn_network_name:
+                continue
+            network_id = ovn_network_name.replace('neutron-', '')
+            if not network_id:
+                continue
+            if ovn_utils.is_provider_network(network_id):
+                LOG.warning("Port forwarding configured in the router "
+                            "%(router_id)s will not work properly as "
+                            "distributed floating IPs are enabled "
+                            "and at least one provider network "
+                            "(%(network_id)s) is connected to that router. "
+                            "See bug https://launchpad.net/bugs/2028846 for "
+                            "more details.", {
+                                'router_id': router_id,
+                                'network_id': network_id})
+                return
+
     def port_forwarding_created(self, ovn_txn, nb_ovn, pf_obj):
+        self._validate_router_networks(nb_ovn, pf_obj.router_id)
         pf_objs = pf_obj.unroll_port_ranges()
         is_range = len(pf_objs) > 1
         for pf_obj in pf_objs:
diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py
@@ -937,6 +937,70 @@ def test_update_port_virtual_type(self, *args):
                                     ('type', constants.LSP_TYPE_VIRTUAL))]
         nb_idl.db_set.assert_has_calls(expected_calls)
 
+    def test_add_gw_port_info_to_logical_router_port(self):
+        nb_idl = self.fake_ovn_client._nb_idl
+        ext_net_id = uuidutils.generate_uuid()
+        internal_net_id = uuidutils.generate_uuid()
+        routers_db = [{
+            'id': uuidutils.generate_uuid(),
+            'external_gateways': [{'network_id': ext_net_id}]}]
+        ext_gw_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'external_ids': {
+                constants.OVN_NETWORK_NAME_EXT_ID_KEY:
+                    'neutron-{}'.format(ext_net_id)}})
+        internal_net_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'external_ids': {
+                constants.OVN_NETWORK_NAME_EXT_ID_KEY:
+                    'neutron-{}'.format(internal_net_id)}})
+        fake_router = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={
+                'ports': [ext_gw_lrp, internal_net_lrp]})
+
+        expected_new_ext_gw_lrp_ids = ext_gw_lrp.external_ids
+        expected_new_ext_gw_lrp_ids[constants.OVN_ROUTER_IS_EXT_GW] = 'True'
+        expected_new_internal_lrp_ids = internal_net_lrp.external_ids
+        expected_new_internal_lrp_ids[constants.OVN_ROUTER_IS_EXT_GW] = 'False'
+
+        self.fake_ovn_client._l3_plugin.get_routers.return_value = routers_db
+        nb_idl.get_lrouter.return_value = fake_router
+        self.assertRaises(
+            periodics.NeverAgain,
+            self.periodic.add_gw_port_info_to_logical_router_port)
+        nb_idl.update_lrouter_port.assert_has_calls([
+            mock.call(name=ext_gw_lrp.name,
+                      external_ids=expected_new_ext_gw_lrp_ids),
+            mock.call(name=internal_net_lrp.name,
+                      external_ids=expected_new_internal_lrp_ids)],
+            any_order=True)
+
+    def test_add_gw_port_info_to_logical_router_port_no_action_needed(self):
+        nb_idl = self.fake_ovn_client._nb_idl
+        ext_net_id = uuidutils.generate_uuid()
+        internal_net_id = uuidutils.generate_uuid()
+        routers_db = [{
+            'id': uuidutils.generate_uuid(),
+            'external_gateways': [{'network_id': ext_net_id}]}]
+        ext_gw_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'external_ids': {
+                constants.OVN_NETWORK_NAME_EXT_ID_KEY:
+                    'neutron-{}'.format(ext_net_id),
+                constants.OVN_ROUTER_IS_EXT_GW: 'True'}})
+        internal_net_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'external_ids': {
+                constants.OVN_NETWORK_NAME_EXT_ID_KEY:
+                    'neutron-{}'.format(internal_net_id),
+                constants.OVN_ROUTER_IS_EXT_GW: 'False'}})
+        fake_router = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={
+                'ports': [ext_gw_lrp, internal_net_lrp]})
+
+        self.fake_ovn_client._l3_plugin.get_routers.return_value = routers_db
+        nb_idl.get_lrouter.return_value = fake_router
+        self.assertRaises(
+            periodics.NeverAgain,
+            self.periodic.add_gw_port_info_to_logical_router_port)
+        nb_idl.update_lrouter_port.assert_not_called()
+
     def test_check_router_default_route_empty_dst_ip(self):
         nb_idl = self.fake_ovn_client._nb_idl
         route0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
diff --git a/neutron/tests/unit/services/ovn_l3/test_plugin.py b/neutron/tests/unit/services/ovn_l3/test_plugin.py
@@ -93,7 +93,8 @@ def setUp(self):
                 ovn_const.OVN_SUBNET_EXT_IDS_KEY: 'subnet-id',
                 ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
                 ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
-                utils.ovn_name(self.fake_network['id'])}}
+                utils.ovn_name(self.fake_network['id']),
+                ovn_const.OVN_ROUTER_IS_EXT_GW: 'False'}}
         self.fake_router_ports = [self.fake_router_port]
         self.fake_subnet = {'id': 'subnet-id',
                             'ip_version': 4,
@@ -149,7 +150,8 @@ def setUp(self):
                 ovn_const.OVN_SUBNET_EXT_IDS_KEY: 'ext-subnet-id',
                 ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
                 ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
-                utils.ovn_name(self.fake_network['id'])},
+                utils.ovn_name(self.fake_network['id']),
+                ovn_const.OVN_ROUTER_IS_EXT_GW: 'True'},
             'gateway_chassis': ['hv1'],
             'options': {}}
         self.fake_floating_ip_attrs = {'floating_ip_address': '192.168.0.10',
@@ -404,7 +406,8 @@ def test_remove_router_interface_update_lrouter_port(self):
                 ovn_const.OVN_SUBNET_EXT_IDS_KEY: 'subnet-id',
                 ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
                 ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY:
-                utils.ovn_name(self.fake_network['id'])})
+                utils.ovn_name(self.fake_network['id']),
+                ovn_const.OVN_ROUTER_IS_EXT_GW: 'False'})
 
     def test_remove_router_interface_router_not_found(self):
         router_id = 'router-id'
@@ -1631,6 +1634,8 @@ def test_add_router_interface_need_to_frag_enabled(
         fake_router_port_assert['options'] = {
             ovn_const.OVN_ROUTER_PORT_GW_MTU_OPTION:
             str(prov_net['mtu'])}
+        fake_router_port_assert['external_ids'][
+            ovn_const.OVN_ROUTER_IS_EXT_GW] = 'True'
 
         self.l3_inst._nb_ovn.add_lrouter_port.assert_called_once_with(
             **fake_router_port_assert)
@@ -1678,6 +1683,8 @@ def test_add_router_interface_need_to_frag_enabled_then_remove(
         fake_router_port_assert = self.fake_router_port_assert
         fake_router_port_assert['gateway_chassis'] = mock.ANY
         fake_router_port_assert['options'] = {}
+        fake_router_port_assert['external_ids'][
+            ovn_const.OVN_ROUTER_IS_EXT_GW] = 'True'
 
         self.l3_inst._nb_ovn.add_lrouter_port.assert_called_once_with(
             **fake_router_port_assert)
diff --git a/neutron/tests/unit/services/portforwarding/drivers/ovn/test_driver.py b/neutron/tests/unit/services/portforwarding/drivers/ovn/test_driver.py
@@ -450,6 +450,48 @@ def test_port_forwarding_deleted(self, m_info):
         self.l3_plugin._nb_ovn.lb_del.assert_called_once_with(
             exp_lb_name, exp_vip, if_exists=mock.ANY)
 
+    @mock.patch.object(port_forwarding.LOG, 'warning')
+    def test__validate_router_networks_provider_networks(self, mock_warning):
+        lr_ports = [
+            mock.MagicMock(external_ids={
+                ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-ext-net-id',
+                ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'True'}),
+            mock.MagicMock(external_ids={
+                ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-net-id',
+                ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'False'})]
+        lr_mock = mock.MagicMock(ports=lr_ports)
+        ovn_nb_mock = mock.Mock()
+        ovn_nb_mock.get_lrouter.return_value = lr_mock
+        with mock.patch.object(
+                ovn_conf, 'is_ovn_distributed_floating_ip',
+                return_value=True), mock.patch.object(
+                    ovn_utils, 'is_provider_network',
+                    return_value=True):
+            self.handler._validate_router_networks(
+                ovn_nb_mock, 'router-id')
+        self.assertEqual(1, mock_warning.call_count)
+
+    @mock.patch.object(port_forwarding.LOG, 'warning')
+    def test__validate_router_networks_tunnel_networks(self, mock_warning):
+        lr_ports = [
+            mock.MagicMock(external_ids={
+                ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-ext-net-id',
+                ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'True'}),
+            mock.MagicMock(external_ids={
+                ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-net-id',
+                ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'False'})]
+        lr_mock = mock.MagicMock(ports=lr_ports)
+        ovn_nb_mock = mock.Mock()
+        ovn_nb_mock.get_lrouter.return_value = lr_mock
+        with mock.patch.object(
+                ovn_conf, 'is_ovn_distributed_floating_ip',
+                return_value=True), mock.patch.object(
+                    ovn_utils, 'is_provider_network',
+                    return_value=False):
+            self.handler._validate_router_networks(
+                ovn_nb_mock, 'router-id')
+        mock_warning.assert_not_called()
+
 
 class TestOVNPortForwarding(TestOVNPortForwardingBase):
     def setUp(self):
