Add feature to check if SNAT disabled before exposing tenant networks

If SNAT on the router is enabled, then the subnet is reachable
from the outside, but all new sessions created from within the subnet
will be SNAT-ed. So those sessions will use the external IP of the router.
For example whitelisting specific tenant ips would not be possible.

With SNAT disabled, the neutron router will act as a normal gateway, and
sessions created from within a tenant vm will be sent from the real ip.

Closes-Bug: #2052292
Change-Id: Ib97065fb2fcca069195278fea804256370d21816

doc/source/contributor/drivers/nb_bgp_mode_design.rst

@@ -295,6 +295,36 @@ To accomplish the network configuration and advertisement, the driver ensures:
 .. include:: ../bgp_advertising.rst
 
 
+Traffic flow from tenant networks
++++++++++++++++++++++++++++++++++
+
+By default neutron enables SNAT on routers (because that is typically
+what you'd use the routers for). This has some side effects that might not
+be all that convenient; for one, all connections initiated from VMs in
+tenant networks will be externally identified with the IP of the cr-lrp.
+
+The VMs in the tenant networks are reachable through their own ip and
+return traffic will flow as expected as well, but it is just not really
+what one would expect.
+
+To prevent tenant networks from being exposed if SNAT is enabled, one can set
+the configuration option ``require_snat_disabled_for_tenant_networks`` to ``True``
+
+This will check if the cr-lrp has SNAT disabled for that subnet, and prevent
+announcement of those tenant networks.
+
+.. note::
+  Neutron will add IPv6 subnets are without NAT, so even though the IPv4 of
+  those tenant networks might have NAT enabled, the IPv6 subnet might still
+  be exposed, as this has no NAT enabled.
+
+To disable the SNAT on a neutron router, one could simply run this command:
+
+.. code-block:: ini
+
+  $ openstack router set --disable-snat --external-gateway <provider_network> <router>
+
+
 .. include:: ../bgp_traffic_redirection.rst
 
 

ovn_bgp_agent/config.py

@@ -45,6 +45,15 @@ agent_opts = [
                default=constants.ADVERTISEMENT_METHOD_HOST,
                choices=[constants.ADVERTISEMENT_METHOD_HOST,
                         constants.ADVERTISEMENT_METHOD_SUBNET]),
+    cfg.BoolOpt('require_snat_disabled_for_tenant_networks',
+                help='Require SNAT on the router port to be disabled before '
+                     'exposing the tenant networks. Otherwise the exposed '
+                     'tenant networks will be reachable from the outside, but'
+                     'the connections set up from within the tenant vm will '
+                     'always be SNAT-ed by the router, thus be the router ip. '
+                     'When SNAT is disabled, OVN will do pure routing without '
+                     'SNAT',
+                default=False),
     cfg.BoolOpt('expose_ipv6_gua_tenant_networks',
                 help='Expose only VM IPv6 IPs on tenant networks if they are '
                      'GUA. The expose_tenant_networks parameter takes '

ovn_bgp_agent/constants.py

@@ -23,6 +23,7 @@ OVN_PATCH_VIF_PORT_TYPE = "patch"
 OVN_ROUTER_PORT_TYPE = "router"
 OVN_CHASSISREDIRECT_VIF_PORT_TYPE = "chassisredirect"
 OVN_LOCALNET_VIF_PORT_TYPE = "localnet"
+OVN_SNAT = "snat"
 OVN_DNAT_AND_SNAT = "dnat_and_snat"
 OVN_CR_LRP_PORT_TYPE = 'crlrp'
 OVN_ROUTER_INTERFACE = 'network:router_interface'

ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import collections
+import ipaddress
 import threading
 
 from oslo_concurrency import lockutils
@@ -35,7 +36,7 @@ LOG = logging.getLogger(__name__)
 # LOG.setLevel(logging.DEBUG)
 # logging.basicConfig(level=logging.DEBUG)
 
-OVN_TABLES = ['Logical_Switch_Port', 'NAT', 'Logical_Switch',
+OVN_TABLES = ['Logical_Switch_Port', 'NAT', 'Logical_Switch', 'Logical_Router',
               'Logical_Router_Port', 'Load_Balancer']
 LOCAL_CLUSTER_OVN_TABLES = ['Logical_Switch', 'Logical_Switch_Port',
                             'Logical_Router', 'Logical_Router_Port',
@@ -686,6 +687,30 @@ class NBOVNBGPDriver(driver_api.AgentDriverBase):
                       "cr-lrp matching %s", ips, gateway_router)
             return
 
+        if CONF.require_snat_disabled_for_tenant_networks:
+            # Check if there is a SNAT entry for this LRP
+            router = self.nb_idl.get_router(gateway_router)
+
+            ips_without_snat = set(ips)
+            for nat in router.nat:
+                if nat.type == constants.OVN_SNAT:
+                    net = ipaddress.ip_network(nat.logical_ip, strict=False)
+                    for ip in list(ips_without_snat):
+                        if ipaddress.ip_address(ip.split('/')[0]) in net:
+                            ips_without_snat.discard(ip)
+
+            if len(ips_without_snat) == 0:
+                LOG.info('All ips (%s) were removed due to SNAT requirement '
+                         'when exposing subnet %s for router %s', ips,
+                         subnet_info['network'], gateway_router)
+                return
+
+            if len(set(ips)) != len(ips_without_snat):
+                LOG.info('When exposing subnet %s for router %s, these ips '
+                         'were removed for SNAT: %s', subnet_info['network'],
+                         gateway_router, set(ips) - ips_without_snat)
+                ips = list(ips_without_snat)
+
         if not self._expose_router_lsp(ips, subnet_info, cr_lrp_info):
             LOG.debug("Something happen while exposing the Subnet CIRDs %s "
                       "and they have not been properly exposed", ips)

ovn_bgp_agent/drivers/openstack/utils/ovn.py

@@ -509,6 +509,10 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
     def static_mac_binding_del(self, port, ip, if_exists=False):
         return StaticMACBindingDelCommand(self, port, ip, if_exists)
 
+    def get_router(self, router):
+        router_name = 'neutron-' + router
+        return self.lr_get(router_name).execute(check_error=True)
+
 
 class OvsdbSbOvnIdl(sb_impl_idl.OvnSbApiIdlImpl, Backend):
     def __init__(self, connection):

ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py

@@ -975,6 +975,87 @@ class TestNBOVNBGPDriver(test_base.TestCase):
         self.nb_idl.get_active_lsp.assert_not_called()
         mock_expose_remote_ip.assert_not_called()
 
+    def _test_expose_subnet_require_snat_disabled(self,
+                                                  partial_continue=False):
+        CONF.set_override('require_snat_disabled_for_tenant_networks', True)
+        self.addCleanup(CONF.clear_override,
+                        'require_snat_disabled_for_tenant_networks')
+
+        ips = ['10.0.0.1/24']
+        if partial_continue:
+            ips.append(self.ipv6 + '/64')
+
+        subnet_info = {
+            'associated_router': 'router1',
+            'network': 'network1',
+            'address_scopes': {4: None, 6: None}}
+        mock_expose_router_lsp = mock.patch.object(
+            self.nb_bgp_driver, '_expose_router_lsp').start()
+        mock_expose_remote_ip = mock.patch.object(
+            self.nb_bgp_driver, '_expose_remote_ip').start()
+
+        router = utils.create_row(
+            nat=[utils.create_row(
+                type=constants.OVN_SNAT,
+                logical_ip='10.0.0.0/24',
+            )],
+        )
+        self.nb_idl.get_router.return_value = router
+
+        self.nb_bgp_driver.expose_subnet(ips, subnet_info)
+
+        gateway_router = subnet_info['associated_router']
+        self.nb_idl.get_router.assert_called_once_with(gateway_router)
+
+        if not partial_continue:
+            self.nb_idl.get_active_lsp.assert_not_called()
+            mock_expose_remote_ip.assert_not_called()
+            mock_expose_router_lsp.assert_not_called()
+        else:
+            # partial continue scenario is when SNAT is not enabled for the
+            # router, so only the ipv6 should match
+            mock_expose_router_lsp.assert_called_once_with(
+                [self.ipv6 + '/64'], subnet_info, self.router1_info)
+
+            ips_info0 = {'mac': 'mac',
+                         'cidrs': ['192.168.0.5/24'],
+                         'type': constants.OVN_VM_VIF_PORT_TYPE,
+                         'logical_switch': 'network1'}
+            ips_info1 = {'mac': 'mac',
+                         'cidrs': ['192.168.0.6/24'],
+                         'type': constants.OVN_VIRTUAL_VIF_PORT_TYPE,
+                         'logical_switch': 'network1'}
+            expected_calls = [mock.call(['192.168.0.5'], ips_info0),
+                              mock.call(['192.168.0.6'], ips_info1)]
+            mock_expose_remote_ip.assert_has_calls(expected_calls)
+
+    def test_expose_subnet_require_snat_disabled(self):
+        self._test_expose_subnet_require_snat_disabled(
+            partial_continue=False,
+        )
+
+    def test_expose_subnet_require_snat_disabled_partial_continue(self):
+        # Setup get_active_lsp for partial_continue scenario
+        port0 = utils.create_row(
+            type=constants.OVN_VM_VIF_PORT_TYPE,
+            addresses=['mac 192.168.0.5'],
+            external_ids={
+                constants.OVN_CIDRS_EXT_ID_KEY: "192.168.0.5/24",
+                constants.OVN_LS_NAME_EXT_ID_KEY: 'network1'
+            })
+        port1 = utils.create_row(
+            type=constants.OVN_VIRTUAL_VIF_PORT_TYPE,
+            addresses=['mac 192.168.0.6'],
+            external_ids={
+                constants.OVN_CIDRS_EXT_ID_KEY: "192.168.0.6/24",
+                constants.OVN_LS_NAME_EXT_ID_KEY: 'network1'
+            })
+        self.nb_idl.get_active_lsp.return_value = [port0, port1]
+
+        self._test_expose_subnet_require_snat_disabled(
+            partial_continue=True,
+        )
+
     def test_withdraw_subnet(self):
         ips = ['10.0.0.1/24']
         subnet_info = {