Support nested SNAT for ml2/ovn

When ovn_router_indirect_snat = True, ml2/ovn will set a catch-all snat
rule for each external ip, instead of a snat rule per attached subnet.

NB: This option is global to cluster and cannot be controlled per
project or per router.

NB2: this patch assumes that 0.0.0.0/0 snat rules are properly handled
by OVN. Some (e.g. 22.03 and 24.03) OVN versions may have this scenario
broken. See: https://issues.redhat.com/browse/FDP-744 for details.

--

A long time ago, nested SNAT behavior was unconditionally enabled for
ml2/ovs, see: https://bugs.launchpad.net/neutron/+bug/1386041

Since this behavior has potential security implications, and since it
may not be desired in all environments, a new flag is introduced.

Since OVN was deployed without nested SNAT enabled in multiple
environments, the flag is set to False by default (meaning: no nested
SNAT).

In theory, instead of a config option, neutron could introduce a new API
to allow users to control the behavior per router. This would require
more work though. This granular API is left out of the patch. Interested
parties are welcome to start a discussion about adding the new API as a
new neutron extension to routers.

--

Before this patch, there was an alternative implementation proposed that
was not relying on 0.0.0.0/0 snat behavior implemented properly in OVN.
The implementation was abandoned because it introduced non-negligible
complexity in the neutron code and the OVN NB database.

See: https://review.opendev.org/c/openstack/neutron/+/907504

--

Conflicts:
    neutron/conf/plugins/ml2/drivers/ovn/ovn_conf.py
    neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

Closes-Bug: #2051935
Co-Authored-By: Brian Haley <[email protected]>
Change-Id: I28fae44edc122fae389916e25b3321550de001fd
(cherry picked from commit dbf53b7)

Author: 2 people

neutron/common/ovn/constants.py

@@ -463,3 +463,6 @@
                             portbindings.VNIC_BAREMETAL,
                             portbindings.VNIC_VIRTIO_FORWARDER,
                             ]
+
+# OVN default SNAT CIDR
+OVN_DEFAULT_SNAT_CIDR = '0.0.0.0/0'

neutron/conf/plugins/ml2/drivers/ovn/ovn_conf.py

@@ -221,6 +221,12 @@
                default=0,
                help=_('The number of seconds to keep MAC_Binding entries in '
                       'the OVN DB. 0 to disable aging.')),
+    cfg.BoolOpt('ovn_router_indirect_snat',
+                default=False,
+                help=_('Whether to configure SNAT for all nested subnets '
+                       'connected to the router through any other routers, '
+                       'similar to the default ML2/OVS behavior. Defaults to '
+                       '"False".')),
 ]
 
 nb_global_opts = [
@@ -376,3 +382,7 @@ def get_fdb_removal_limit():
 def get_ovn_mac_binding_age_threshold():
     # This value is always stored as a string in the OVN DB
     return str(cfg.CONF.ovn.mac_binding_age_threshold)
+
+
+def is_ovn_router_indirect_snat_enabled():
+    return cfg.CONF.ovn.ovn_router_indirect_snat

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

@@ -16,6 +16,7 @@
 import collections
 import copy
 import datetime
+import functools
 
 import netaddr
 from neutron_lib.api.definitions import l3
@@ -51,6 +52,8 @@
 from neutron.common import utils as common_utils
 from neutron.conf.agent import ovs_conf
 from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
+from neutron.conf.plugins.ml2.drivers.ovn.ovn_conf \
+    import is_ovn_router_indirect_snat_enabled as is_nested_snat
 from neutron.db import ovn_revision_numbers_db as db_rev
 from neutron.db import segments_db
 from neutron.objects import router
@@ -64,6 +67,10 @@
 LOG = log.getLogger(__name__)
 
 
+def _has_separate_snat_per_subnet(router):
+    return utils.is_snat_enabled(router) and not is_nested_snat()
+
+
 OvnPortInfo = collections.namedtuple(
     "OvnPortInfo",
     [
@@ -1219,23 +1226,23 @@ def _get_gw_info(self, context, port_dict):
                 else const.IPv6_ANY))
         return gateways_info
 
-    def _delete_router_ext_gw(self, router, networks, txn):
+    def _delete_router_ext_gw(self, router_id, txn):
         context = n_context.get_admin_context()
-        if not networks:
-            networks = []
-        router_id = router['id']
+        cidrs = self._get_snat_cidrs_for_external_router(context, router_id)
         gw_lrouter_name = utils.ovn_name(router_id)
         deleted_ports = []
         for gw_port in self._get_router_gw_ports(context, router_id):
             for gw_info in self._get_gw_info(context, gw_port):
-                if gw_info.ip_version == const.IP_VERSION_4:
-                    for network in networks:
-                        txn.add(self._nb_idl.delete_nat_rule_in_lrouter(
-                            gw_lrouter_name, type='snat', logical_ip=network,
-                            external_ip=gw_info.router_ip))
                 txn.add(self._nb_idl.delete_static_route(
                     gw_lrouter_name, ip_prefix=gw_info.ip_prefix,
                     nexthop=gw_info.gateway_ip))
+                if gw_info.ip_version != const.IP_VERSION_4:
+                    continue
+                for cidr in cidrs:
+                    txn.add(self._nb_idl.delete_nat_rule_in_lrouter(
+                        gw_lrouter_name, type='snat',
+                        external_ip=gw_info.router_ip,
+                        logical_ip=cidr))
             txn.add(self._nb_idl.delete_lrouter_port(
                 utils.ovn_lrouter_port_name(gw_port['id']),
                 gw_lrouter_name))
@@ -1281,7 +1288,7 @@ def _get_nets_and_ipv6_ra_confs_for_router_port(self, context, port):
 
         return list(networks), ipv6_ra_configs
 
-    def _add_router_ext_gw(self, context, router, networks, txn):
+    def _add_router_ext_gw(self, context, router, txn):
         lrouter_name = utils.ovn_name(router['id'])
         router_default_route_ecmp_enabled = router.get(
             'enable_default_route_ecmp', False)
@@ -1319,9 +1326,9 @@ def _add_router_ext_gw(self, context, router, networks, txn):
                     maintain_bfd=router_default_route_bfd_enabled,
                     **columns))
 
-        # 3. Add snat rules for tenant networks in lrouter if snat is enabled
-        if utils.is_snat_enabled(router) and networks:
-            self.update_nat_rules(router, networks, enable_snat=True, txn=txn)
+        # 3. Add necessary snat rule(s) in lrouter if snat is enabled
+        if utils.is_snat_enabled(router):
+            self.update_nat_rules(router['id'], enable_snat=True, txn=txn)
         return added_ports
 
     def _check_external_ips_changed(self, ovn_snats,
@@ -1423,17 +1430,20 @@ def _get_v4_network_for_router_port(self, context, port):
             cidr = subnet['cidr']
         return cidr
 
-    def _get_v4_network_of_all_router_ports(self, context, router_id,
-                                            ports=None):
+    def _get_v4_network_of_all_router_ports(self, context, router_id):
         networks = []
-        ports = ports or self._get_router_ports(context, router_id)
-        for port in ports:
+        for port in self._get_router_ports(context, router_id):
             network = self._get_v4_network_for_router_port(context, port)
             if network:
                 networks.append(network)
-
         return networks
 
+    def _get_snat_cidrs_for_external_router(self, context, router_id):
+        if is_nested_snat():
+            return [ovn_const.OVN_DEFAULT_SNAT_CIDR]
+        # nat rule per attached subnet per external ip
+        return self._get_v4_network_of_all_router_ports(context, router_id)
+
     def _gen_router_ext_ids(self, router):
         return {
             ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
@@ -1462,12 +1472,9 @@ def create_router(self, context, router, add_external_gateway=True):
             # by the ovn_db_sync.py script, remove it after the database
             # synchronization work
             if add_external_gateway:
-                networks = self._get_v4_network_of_all_router_ports(
-                    context, router['id'])
-                if (router.get(l3_ext_gw_multihoming.EXTERNAL_GATEWAYS) and
-                        networks is not None):
+                if router.get(l3_ext_gw_multihoming.EXTERNAL_GATEWAYS):
                     added_gw_ports = self._add_router_ext_gw(
-                        context, router, networks, txn)
+                        context, router, txn)
 
             self._qos_driver.create_router(txn, router)
 
@@ -1495,7 +1502,6 @@ def update_router(self, context, new_router, router_object=None):
                 l3_ext_gw_multihoming.EXTERNAL_GATEWAYS)
 
         ovn_snats = utils.get_lrouter_snats(ovn_router)
-        networks = self._get_v4_network_of_all_router_ports(context, router_id)
         try:
             check_rev_cmd = self._nb_idl.check_revision_number(
                 router_name, new_router, ovn_const.TYPE_ROUTERS)
@@ -1504,13 +1510,13 @@ def update_router(self, context, new_router, router_object=None):
                 if gateway_new and not gateway_old:
                     # Route gateway is set
                     added_gw_ports = self._add_router_ext_gw(
-                        context, new_router, networks, txn)
+                        context, new_router, txn)
                 elif gateway_old and not gateway_new:
                     # router gateway is removed
                     txn.add(self._nb_idl.delete_lrouter_ext_gw(router_name))
                     if router_object:
                         deleted_gw_port_ids = self._delete_router_ext_gw(
-                            router_object, networks, txn)
+                            router_object['id'], txn)
                 elif gateway_new and gateway_old:
                     # Check if external gateway has changed, if yes, delete
                     # the old gateway and add the new gateway
@@ -1528,16 +1534,16 @@ def update_router(self, context, new_router, router_object=None):
                             router_name))
                         if router_object:
                             deleted_gw_port_ids = self._delete_router_ext_gw(
-                                router_object, networks, txn)
+                                router_object['id'], txn)
                         added_gw_ports = self._add_router_ext_gw(
-                            context, new_router, networks, txn)
+                            context, new_router, txn)
                     else:
                         # Check if snat has been enabled/disabled and update
                         new_snat_state = utils.is_snat_enabled(new_router)
-                        if bool(ovn_snats) != new_snat_state and networks:
+                        if bool(ovn_snats) != new_snat_state:
                             self.update_nat_rules(
-                                new_router, networks,
-                                enable_snat=new_snat_state, txn=txn)
+                                new_router['id'], enable_snat=new_snat_state,
+                                txn=txn)
 
                 update = {'external_ids': self._gen_router_ext_ids(new_router)}
                 update['enabled'] = new_router.get('admin_state_up') or False
@@ -1785,26 +1791,26 @@ def create_router_port(self, context, router_id, router_interface):
 
             gw_ports = self._get_router_gw_ports(context, router_id)
             if gw_ports:
-                cidr = None
-                for fixed_ip in port['fixed_ips']:
-                    subnet = self._plugin.get_subnet(context,
-                                                     fixed_ip['subnet_id'])
-                    if multi_prefix:
-                        if 'subnet_id' in router_interface:
-                            if subnet['id'] != router_interface['subnet_id']:
-                                continue
-                    if subnet['ip_version'] == const.IP_VERSION_4:
-                        cidr = subnet['cidr']
-
                 if ovn_conf.is_ovn_emit_need_to_frag_enabled():
                     for gw_port in gw_ports:
                         provider_net = self._plugin.get_network(
                             context, gw_port['network_id'])
                         self.set_gateway_mtu(context, provider_net)
 
-                if utils.is_snat_enabled(router) and cidr:
-                    self.update_nat_rules(router, networks=[cidr],
-                                          enable_snat=True, txn=txn)
+                if _has_separate_snat_per_subnet(router):
+                    for fixed_ip in port['fixed_ips']:
+                        subnet = self._plugin.get_subnet(
+                            context, fixed_ip['subnet_id'])
+                        if (multi_prefix and
+                                'subnet_id' in router_interface and
+                                subnet['id'] != router_interface['subnet_id']):
+                            continue
+                        if subnet['ip_version'] == const.IP_VERSION_4:
+                            self.update_nat_rules(
+                                router['id'], cidrs=[subnet['cidr']],
+                                enable_snat=True, txn=txn)
+                            break  # TODO(ihar): handle multiple ipv4 ips?
+
                 if ovn_conf.is_ovn_distributed_floating_ip():
                     router_gw_ports = self._get_router_gw_ports(context,
                                                                 router_id)
@@ -1937,19 +1943,17 @@ def delete_router_port(self, context, port_id, subnet_ids=None):
                         context, gw_port['network_id'])
                     self.set_gateway_mtu(context, provider_net, txn=txn)
 
-            cidr = None
-            for sid in subnet_ids:
-                try:
-                    subnet = self._plugin.get_subnet(context, sid)
-                except n_exc.SubnetNotFound:
-                    continue
-                if subnet['ip_version'] == const.IP_VERSION_4:
-                    cidr = subnet['cidr']
-                    break
-
-            if utils.is_snat_enabled(router) and cidr:
-                self.update_nat_rules(
-                    router, networks=[cidr], enable_snat=False, txn=txn)
+            if _has_separate_snat_per_subnet(router):
+                for sid in subnet_ids:
+                    try:
+                        subnet = self._plugin.get_subnet(context, sid)
+                    except n_exc.SubnetNotFound:
+                        continue
+                    if subnet['ip_version'] == const.IP_VERSION_4:
+                        self.update_nat_rules(
+                            router['id'], cidrs=[subnet['cidr']],
+                            enable_snat=False, txn=txn)
+                        break  # TODO(ihar): handle multiple ipv4 ips?
 
             if ovn_conf.is_ovn_distributed_floating_ip():
                 router_gw_ports = self._get_router_gw_ports(context, router_id)
@@ -1968,20 +1972,35 @@ def delete_router_port(self, context, port_id, subnet_ids=None):
                 db_rev.bump_revision(
                     context, port, ovn_const.TYPE_ROUTER_PORTS)
 
-    def update_nat_rules(self, router, networks, enable_snat, txn=None):
-        """Update the NAT rules in a logical router."""
+    def _iter_ipv4_gw_addrs(self, context, router_id):
+        yield from (
+            gw_info.router_ip
+            for gw_port in self._get_router_gw_ports(context, router_id)
+            for gw_info in self._get_gw_info(context, gw_port)
+            if gw_info.ip_version != const.IP_VERSION_6
+        )
+
+    def update_nat_rules(self, router_id, enable_snat, cidrs=None, txn=None):
+        if enable_snat:
+            idl_func = self._nb_idl.add_nat_rule_in_lrouter
+        else:
+            idl_func = self._nb_idl.delete_nat_rule_in_lrouter
+        func = functools.partial(
+            idl_func, utils.ovn_name(router_id), type='snat')
+
         context = n_context.get_admin_context()
-        func = (self._nb_idl.add_nat_rule_in_lrouter if enable_snat else
-                self._nb_idl.delete_nat_rule_in_lrouter)
-        gw_lrouter_name = utils.ovn_name(router['id'])
-        # Update NAT rules only for IPv4 subnets
-        commands = [func(gw_lrouter_name, type='snat', logical_ip=network,
-                         external_ip=gw_info.router_ip)
-                    for gw_port in self._get_router_gw_ports(context,
-                                                             router['id'])
-                    for gw_info in self._get_gw_info(context, gw_port)
-                    if gw_info.ip_version != const.IP_VERSION_6
-                    for network in networks]
+        cidrs = (
+            cidrs or
+            self._get_snat_cidrs_for_external_router(context, router_id)
+        )
+        commands = [
+            func(logical_ip=cidr, external_ip=router_ip)
+            for router_ip in self._iter_ipv4_gw_addrs(context, router_id)
+            for cidr in cidrs
+        ]
+        if not commands:
+            return
+
         self._transaction(commands, txn=txn)
 
     def create_provnet_port(self, network_id, segment, txn=None):

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

@@ -540,12 +540,12 @@ def sync_routers_and_rports(self, ctx):
                     if gw_info.ip_version == constants.IP_VERSION_6:
                         continue
                     if gw_info.router_ip and utils.is_snat_enabled(router):
-                        networks = self._ovn_client.\
-                            _get_v4_network_of_all_router_ports(
-                                ctx, router['id'])
-                        for network in networks:
+                        cidrs = self._ovn_client.\
+                            _get_snat_cidrs_for_external_router(ctx,
+                                                                router['id'])
+                        for cidr in cidrs:
                             db_extends[router['id']]['snats'].append({
-                                'logical_ip': network,
+                                'logical_ip': cidr,
                                 'external_ip': gw_info.router_ip,
                                 'type': 'snat'})