[ML2/OVN] Add gateway_port support for FIP

The OVN changed support for NAT rules including a new column and auto discovery logic (which may not work in some cases) [1][2].
If the OVN backend supports this column in the Northbound DB Schema, set gateway port uuid to any floating IP to prevent North/South traffic issues for floating IPs.

This patch updates the method for creating FIP NAT rules in OVN backend and updates previously created FIP rules to include the gateway_port reference. This NAT rule update task runs only once during the maintenance task, and if all entries are already configured no action is performed.

[1] ovn-org/ovn@15348b7
[2] ovn-org/ovn@2d942be

Conflicts:
	neutron/common/ovn/utils.py
	neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py
	neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

(cherry picked from commit 78b5fe2)

Closes-Bug: 2035281
Change-Id: I802b6bd8c281cb6dacdee2e9c15285f069d4e04c

Author: rbartzen

neutron/common/ovn/utils.py

@@ -851,3 +851,7 @@ def get_requested_chassis(requested_chassis):
     if isinstance(requested_chassis, str):
         return requested_chassis.split(',')
     return []
+
+
+def is_nat_gateway_port_supported(idl):
+    return idl.is_col_present('NAT', 'gateway_port')

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py

@@ -360,6 +360,10 @@ def get_all_logical_routers_with_rports(self):
                         columns['external_mac'] = nat.external_mac[0]
                     if nat.logical_port:
                         columns['logical_port'] = nat.logical_port[0]
+                    columns['external_ids'] = nat.external_ids
+                    columns['uuid'] = nat.uuid
+                    if utils.is_nat_gateway_port_supported(self):
+                        columns['gateway_port'] = nat.gateway_port
                     dnat_and_snats.append(columns)
                 elif nat.type == 'snat':
                     snat.append(columns)

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py

@@ -1062,6 +1062,52 @@ def cleanup_old_hash_ring_nodes(self):
         context = n_context.get_admin_context()
         hash_ring_db.cleanup_old_nodes(context, days=5)
 
+    @periodics.periodic(spacing=600, run_immediately=True)
+    def update_nat_floating_ip_with_gateway_port_reference(self):
+        """Set NAT rule gateway_port column to any floating IP without
+        router gateway port uuid reference - LP#2035281.
+        """
+
+        if not utils.is_nat_gateway_port_supported(self._nb_idl):
+            raise periodics.NeverAgain()
+
+        context = n_context.get_admin_context()
+        fip_update = []
+        lrouters = self._nb_idl.get_all_logical_routers_with_rports()
+        for router in lrouters:
+            ovn_fips = router['dnat_and_snats']
+            for ovn_fip in ovn_fips:
+                # Skip FIPs that are already configured with gateway_port
+                if ovn_fip['gateway_port']:
+                    continue
+                fip_id = ovn_fip['external_ids'].get(
+                            ovn_const.OVN_FIP_EXT_ID_KEY)
+                if fip_id:
+                    fip_update.append({'uuid': ovn_fip['uuid'],
+                                       'router_id': router['name']})
+
+        # Simple caching mechanism to avoid unnecessary DB calls
+        gw_port_id_cache = {}
+        lrp_cache = {}
+        cmds = []
+        for fip in fip_update:
+            lrouter = utils.ovn_name(fip['router_id'])
+            if lrouter not in gw_port_id_cache.keys():
+                router_db = self._ovn_client._l3_plugin.get_router(context,
+                    fip['router_id'], fields=['gw_port_id'])
+                gw_port_id_cache[lrouter] = router_db.get('gw_port_id')
+                lrp_cache[lrouter] = self._nb_idl.get_lrouter_port(
+                    gw_port_id_cache[lrouter])
+            columns = {'gateway_port': lrp_cache[lrouter].uuid}
+            cmds.append(self._nb_idl.set_nat_rule_in_lrouter(lrouter,
+                fip['uuid'], **columns))
+
+        if cmds:
+            with self._nb_idl.transaction(check_error=True) as txn:
+                for cmd in cmds:
+                    txn.add(cmd)
+        raise periodics.NeverAgain()
+
 
 class HashRingHealthCheckPeriodics(object):
 

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

@@ -967,6 +967,14 @@ def _create_or_update_floatingip(self, floatingip, txn=None):
                    'logical_port': floatingip['port_id'],
                    'external_ids': ext_ids}
 
+        # If OVN supports gateway_port column for NAT rules set gateway port
+        # uuid to any floating IP without gw port reference - LP#2035281.
+        if utils.is_nat_gateway_port_supported(self._nb_idl):
+            router_db = self._l3_plugin.get_router(admin_context, router_id)
+            gw_port_id = router_db.get('gw_port_id')
+            lrp = self._nb_idl.get_lrouter_port(gw_port_id)
+            columns['gateway_port'] = lrp.uuid
+
         if ovn_conf.is_ovn_distributed_floating_ip():
             if self._nb_idl.lsp_get_up(floatingip['port_id']).execute():
                 columns['external_mac'] = port_db['mac_address']

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

@@ -1014,6 +1014,65 @@ def test_remove_duplicated_chassis_registers_no_ch_private_register(self):
         # "Chassis_Private" register was missing.
         self.assertEqual(2, len(chassis_result))
 
+    def test_floating_ip_with_gateway_port(self):
+        ext_net = self._create_network('ext_networktest', external=True)
+        ext_subnet = self._create_subnet(
+            'ext_subnettest',
+            ext_net['id'],
+            **{'cidr': '100.0.0.0/24',
+               'gateway_ip': '100.0.0.254',
+               'allocation_pools': [
+                   {'start': '100.0.0.2', 'end': '100.0.0.253'}],
+               'enable_dhcp': False})
+        net1 = self._create_network('network1test', external=False)
+        subnet1 = self._create_subnet('subnet1test', net1['id'])
+        external_gateway_info = {
+            'enable_snat': True,
+            'network_id': ext_net['id'],
+            'external_fixed_ips': [
+                {'ip_address': '100.0.0.2', 'subnet_id': ext_subnet['id']}]}
+        router = self._create_router(
+            'routertest', external_gateway_info=external_gateway_info)
+        self._add_router_interface(router['id'], subnet1['id'])
+
+        p1 = self._create_port('testp1', net1['id'])
+        logical_ip = p1['fixed_ips'][0]['ip_address']
+        fip_info = {'floatingip': {
+            'tenant_id': self._tenant_id,
+            'description': 'test_fip',
+            'floating_network_id': ext_net['id'],
+            'port_id': p1['id'],
+            'fixed_ip_address': logical_ip}}
+
+        # Create floating IP without gateway_port
+        with mock.patch.object(utils,
+                'is_nat_gateway_port_supported', return_value=False):
+            fip = self.l3_plugin.create_floatingip(self.context, fip_info)
+
+        self.assertEqual(router['id'], fip['router_id'])
+        self.assertEqual('testp1', fip['port_details']['name'])
+        self.assertIsNotNone(self.nb_api.get_lswitch_port(fip['port_id']))
+
+        rules = self.nb_api.get_all_logical_routers_with_rports()[0]
+        fip_rule = rules['dnat_and_snats'][0]
+        if utils.is_nat_gateway_port_supported(self.nb_api):
+            self.assertEqual([], fip_rule['gateway_port'])
+        else:
+            self.assertNotIn('gateway_port', fip_rule)
+
+        # Call the maintenance task and check that the value has been
+        # updated in the NAT rule
+        self.assertRaises(periodics.NeverAgain,
+            self.maint.update_nat_floating_ip_with_gateway_port_reference)
+
+        rules = self.nb_api.get_all_logical_routers_with_rports()[0]
+        fip_rule = rules['dnat_and_snats'][0]
+
+        if utils.is_nat_gateway_port_supported(self.nb_api):
+            self.assertNotEqual([], fip_rule['gateway_port'])
+        else:
+            self.assertNotIn('gateway_port', fip_rule)
+
 
 class TestLogMaintenance(_TestMaintenanceHelper,
                          test_log_driver.LogApiTestCaseBase):

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py

@@ -1248,3 +1248,103 @@ def test_agent_delete(self):
         self.plugin.delete_agent(self.context, metadata_id)
         self.assertRaises(agent_exc.AgentNotFound, self.plugin.get_agent,
                           self.context, metadata_id)
+
+
+class TestNATRuleGatewayPort(base.TestOVNFunctionalBase):
+
+    def setUp(self):
+        super().setUp()
+        self._ovn_client = self.mech_driver._ovn_client
+
+    def deserialize(self, content_type, response):
+        ctype = 'application/%s' % content_type
+        data = self._deserializers[ctype].deserialize(response.body)['body']
+        return data
+
+    def _create_router(self, name, external_gateway_info=None):
+        data = {'router': {'name': name, 'tenant_id': self._tenant_id,
+                           'external_gateway_info': external_gateway_info}}
+        req = self.new_create_request('routers', data, self.fmt)
+        res = req.get_response(self.api)
+        return self.deserialize(self.fmt, res)['router']
+
+    def _process_router_interface(self, action, router_id, subnet_id):
+        req = self.new_action_request(
+            'routers', {'subnet_id': subnet_id}, router_id,
+            '%s_router_interface' % action)
+        res = req.get_response(self.api)
+        return self.deserialize(self.fmt, res)
+
+    def _add_router_interface(self, router_id, subnet_id):
+        return self._process_router_interface('add', router_id, subnet_id)
+
+    def _create_port(self, name, net_id, security_groups=None,
+                     device_owner=None):
+        data = {'port': {'name': name,
+                         'tenant_id': self._tenant_id,
+                         'network_id': net_id}}
+
+        if security_groups is not None:
+            data['port']['security_groups'] = security_groups
+
+        if device_owner is not None:
+            data['port']['device_owner'] = device_owner
+
+        req = self.new_create_request('ports', data, self.fmt)
+        res = req.get_response(self.api)
+        return self.deserialize(self.fmt, res)['port']
+
+    def test_create_floatingip(self):
+        ext_net = self._make_network(
+            self.fmt, 'ext_networktest', True,
+            arg_list=('router:external',
+                      'provider:network_type',
+                      'provider:physical_network'),
+            **{'router:external': True,
+               'provider:network_type': 'flat',
+               'provider:physical_network': 'public'})['network']
+        res = self._create_subnet(self.fmt, ext_net['id'],
+            '100.0.0.0/24', gateway_ip='100.0.0.254',
+            allocation_pools=[{'start': '100.0.0.2',
+                               'end': '100.0.0.253'}],
+            enable_dhcp=False)
+        ext_subnet = self.deserialize(self.fmt, res)['subnet']
+        net1 = self._make_network(
+            self.fmt, 'network1test', True)['network']
+        res = self._create_subnet(self.fmt, net1['id'],
+            '192.168.0.0/24', gateway_ip='192.168.0.1',
+            allocation_pools=[{'start': '192.168.0.2',
+                               'end': '192.168.0.253'}],
+            enable_dhcp=False)
+        subnet1 = self.deserialize(self.fmt, res)['subnet']
+        external_gateway_info = {
+            'enable_snat': True,
+            'network_id': ext_net['id'],
+            'external_fixed_ips': [
+                {'ip_address': '100.0.0.2', 'subnet_id': ext_subnet['id']}]}
+        router = self._create_router(
+            'routertest', external_gateway_info=external_gateway_info)
+        self._add_router_interface(router['id'], subnet1['id'])
+
+        p1 = self._create_port('testp1', net1['id'])
+        logical_ip = p1['fixed_ips'][0]['ip_address']
+        fip_info = {'floatingip': {
+            'tenant_id': self._tenant_id,
+            'description': 'test_fip',
+            'floating_network_id': ext_net['id'],
+            'port_id': p1['id'],
+            'fixed_ip_address': logical_ip}}
+
+        fip = self.l3_plugin.create_floatingip(self.context, fip_info)
+
+        self.assertEqual(router['id'], fip['router_id'])
+        self.assertEqual('testp1', fip['port_details']['name'])
+        self.assertIsNotNone(self.nb_api.get_lswitch_port(fip['port_id']))
+
+        rules = self.nb_api.get_all_logical_routers_with_rports()[0]
+        fip_rule = rules['dnat_and_snats'][0]
+
+        if utils.is_nat_gateway_port_supported(self.nb_api):
+            self.assertNotEqual([], fip_rule['gateway_port'])
+        else:
+            self.assertNotIn('gateway_port', fip_rule)

neutron/tests/unit/fake_resources.py

@@ -164,6 +164,7 @@ def __init__(self, **kwargs):
         self.ha_chassis_group_del_chassis = mock.Mock()
         self.lrp_get = mock.Mock()
         self.get_schema_version = mock.Mock(return_value='3.6.0')
+        self.get_lrouter_port = mock.Mock()
 
 
 class FakeOvsdbSbOvnIdl(object):

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py

@@ -15,6 +15,7 @@
 import copy
 from unittest import mock
 
+from oslo_utils import uuidutils
 from ovsdbapp.backend import ovs_idl
 
 from neutron.common.ovn import constants as ovn_const
@@ -193,11 +194,15 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
                   'type': 'snat'},
                  {'external_ip': '20.0.2.4', 'logical_ip': '10.0.0.4',
                   'type': 'dnat_and_snat', 'external_mac': [],
-                  'logical_port': []},
+                  'logical_port': [],
+                  'external_ids': {ovn_const.OVN_FIP_EXT_ID_KEY: 'fip_id_a'},
+                  'gateway_port': uuidutils.generate_uuid()},
                  {'external_ip': '20.0.2.5', 'logical_ip': '10.0.0.5',
                   'type': 'dnat_and_snat',
                   'external_mac': ['00:01:02:03:04:05'],
-                  'logical_port': ['lsp-id-001']}],
+                  'logical_port': ['lsp-id-001'],
+                  'external_ids': {ovn_const.OVN_FIP_EXT_ID_KEY: 'fip_id_b'},
+                  'gateway_port': []}],
         'acls': [
             {'unit_test_id': 1,
              'action': 'allow-related', 'direction': 'from-lport',
@@ -446,13 +451,39 @@ def test_get_all_logical_switches_with_ports(self):
                      'provnet_ports': []}]
         self.assertCountEqual(mapping, expected)
 
-    def test_get_all_logical_routers_with_rports(self):
+    def _test_get_all_logical_routers_with_rports(self, is_gw_port):
         # Test empty
         mapping = self.nb_ovn_idl.get_all_logical_switches_with_ports()
         self.assertCountEqual(mapping, {})
         # Test loaded values
         self._load_nb_db()
+
+        # Test with gateway_port_support enabled
+        utils.is_nat_gateway_port_supported = mock.Mock()
+        utils.is_nat_gateway_port_supported.return_value = is_gw_port
         mapping = self.nb_ovn_idl.get_all_logical_routers_with_rports()
+        lra_nat = self._find_ovsdb_fake_row(self.nat_table,
+                                            'external_ip', '20.0.2.4')
+        lrb_nat = self._find_ovsdb_fake_row(self.nat_table,
+                                            'external_ip', '20.0.2.5')
+
+        lra_fip = {'external_ip': '20.0.2.4',
+                   'logical_ip': '10.0.0.4',
+                   'type': 'dnat_and_snat',
+                   'external_ids': {ovn_const.OVN_FIP_EXT_ID_KEY: 'fip_id_a'},
+                   'uuid': lra_nat.uuid}
+        lrb_fip = {'external_ip': '20.0.2.5',
+                   'logical_ip': '10.0.0.5',
+                   'type': 'dnat_and_snat',
+                   'external_mac': '00:01:02:03:04:05',
+                   'logical_port': 'lsp-id-001',
+                   'external_ids': {ovn_const.OVN_FIP_EXT_ID_KEY: 'fip_id_b'},
+                   'uuid': lrb_nat.uuid}
+
+        if is_gw_port:
+            lra_fip['gateway_port'] = lra_nat.gateway_port
+            lrb_fip['gateway_port'] = lrb_nat.gateway_port
+
         expected = [{'name': 'lr-id-a',
                      'ports': {'orp-id-a1': ['10.0.1.0/24'],
                                'orp-id-a2': ['10.0.2.0/24'],
@@ -471,14 +502,7 @@ def test_get_all_logical_routers_with_rports(self):
                      'snats': [{'external_ip': '20.0.2.1',
                                 'logical_ip': '10.0.0.0/24',
                                 'type': 'snat'}],
-                     'dnat_and_snats': [{'external_ip': '20.0.2.4',
-                                         'logical_ip': '10.0.0.4',
-                                         'type': 'dnat_and_snat'},
-                                        {'external_ip': '20.0.2.5',
-                                         'logical_ip': '10.0.0.5',
-                                         'type': 'dnat_and_snat',
-                                         'external_mac': '00:01:02:03:04:05',
-                                         'logical_port': 'lsp-id-001'}]},
+                     'dnat_and_snats': [lra_fip, lrb_fip]},
                     {'name': 'lr-id-c', 'ports': {}, 'static_routes': [],
                      'snats': [], 'dnat_and_snats': []},
                     {'name': 'lr-id-d', 'ports': {}, 'static_routes': [],
@@ -487,6 +511,12 @@ def test_get_all_logical_routers_with_rports(self):
                      'snats': [], 'dnat_and_snats': []}]
         self.assertCountEqual(mapping, expected)
 
+    def test_get_all_logical_routers_with_rports(self):
+        self._test_get_all_logical_routers_with_rports(True)
+
+    def test_get_all_logical_routers_with_rports_without_nat_gw_port(self):
+        self._test_get_all_logical_routers_with_rports(False)
+
     def test_get_acls_for_lswitches(self):
         self._load_nb_db()
         # Test neutron switches