Merge "NSX|V3: Do not add SNAT rules if same address scope"

vmware_nsx/plugins/common/plugin.py

@@ -20,6 +20,7 @@ from neutron.db import address_scope_db
 from neutron.db import api as db_api
 from neutron.db import db_base_plugin_v2
 from neutron.db import l3_db
+from neutron.db import models_v2
 from neutron.extensions import address_scope as ext_address_scope
 from neutron_lib.api.definitions import network as net_def
 from neutron_lib.api.definitions import port as port_def
@@ -126,3 +127,31 @@ class NsxPluginBase(db_base_plugin_v2.NeutronDbPluginV2,
         port_filters = {'device_id': [router_id],
                         'device_owner': [l3_db.DEVICE_OWNER_ROUTER_INTF]}
         return self.get_ports(context, filters=port_filters)
+
+    def _find_router_subnets_cidrs(self, context, router_id):
+        """Retrieve cidrs of subnets attached to the specified router."""
+        subnets = self._find_router_subnets_and_cidrs(context, router_id)
+        return [subnet['cidr'] for subnet in subnets]
+
+    def _get_port_by_device_id(self, context, device_id, device_owner):
+        """Retrieve ports associated with a specific device id.
+
+        Used for retrieving all neutron ports attached to a given router.
+        """
+        port_qry = context.session.query(models_v2.Port)
+        return port_qry.filter_by(
+            device_id=device_id,
+            device_owner=device_owner,).all()
+
+    def _find_router_subnets_and_cidrs(self, context, router_id):
+        """Retrieve subnets attached to the specified router."""
+        ports = self._get_port_by_device_id(context, router_id,
+                                            l3_db.DEVICE_OWNER_ROUTER_INTF)
+        # No need to check for overlapping CIDRs
+        subnets = []
+        for port in ports:
+            for ip in port.get('fixed_ips', []):
+                subnet_qry = context.session.query(models_v2.Subnet)
+                subnet = subnet_qry.filter_by(id=ip.subnet_id).one()
+                subnets.append({'id': subnet.id, 'cidr': subnet.cidr})
+        return subnets

vmware_nsx/plugins/nsx_v/plugin.py

@@ -3248,34 +3248,6 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin,
             address_groups.append(address_group)
         return address_groups
 
-    def _get_port_by_device_id(self, context, device_id, device_owner):
-        """Retrieve ports associated with a specific device id.
-
-        Used for retrieving all neutron ports attached to a given router.
-        """
-        port_qry = context.session.query(models_v2.Port)
-        return port_qry.filter_by(
-            device_id=device_id,
-            device_owner=device_owner,).all()
-
-    def _find_router_subnets_cidrs(self, context, router_id):
-        """Retrieve cidrs of subnets attached to the specified router."""
-        subnets = self._find_router_subnets_and_cidrs(context, router_id)
-        return [subnet['cidr'] for subnet in subnets]
-
-    def _find_router_subnets_and_cidrs(self, context, router_id):
-        """Retrieve subnets attached to the specified router."""
-        ports = self._get_port_by_device_id(context, router_id,
-                                            l3_db.DEVICE_OWNER_ROUTER_INTF)
-        # No need to check for overlapping CIDRs
-        subnets = []
-        for port in ports:
-            for ip in port.get('fixed_ips', []):
-                subnet_qry = context.session.query(models_v2.Subnet)
-                subnet = subnet_qry.filter_by(id=ip.subnet_id).one()
-                subnets.append({'id': subnet.id, 'cidr': subnet.cidr})
-        return subnets
-
     def _get_nat_rules(self, context, router):
         fip_qry = context.session.query(l3_db_models.FloatingIP)
         fip_db = fip_qry.filter_by(router_id=router['id']).all()

vmware_nsx/plugins/nsx_v3/plugin.py

@@ -2686,7 +2686,7 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
             # TODO(berlin): revocate bgp announce on org tier0 router
             pass
         if remove_snat_rules:
-            self._routerlib.delete_gw_snat_rule(nsx_router_id, orgaddr)
+            self._routerlib.delete_gw_snat_rules(nsx_router_id, orgaddr)
         if remove_router_link_port:
             self._routerlib.remove_router_link_port(
                 nsx_router_id, org_tier0_uuid)
@@ -2701,8 +2701,15 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
             self._routerlib.add_router_link_port(nsx_router_id, new_tier0_uuid,
                                                  tags=tags)
         if add_snat_rules:
-            self._routerlib.add_gw_snat_rule(nsx_router_id, newaddr,
+            # Add SNAT rules for all the subnets which are in different scope
-                                             bypass_firewall=False)
+            # than the gw
+            gw_address_scope = self._get_network_address_scope(
+                context, router.gw_port.network_id)
+            subnets = self._find_router_subnets_and_cidrs(context.elevated(),
+                                                          router_id)
+            for subnet in subnets:
+                self._add_subnet_snat_rule(context, router_id, nsx_router_id,
+                                           subnet, gw_address_scope, newaddr)
         if bgp_announce:
             # TODO(berlin): bgp announce on new tier0 router
             pass
@@ -2711,6 +2718,26 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
                                              advertise_route_nat_flag,
                                              advertise_route_connected_flag)
 
+    def _add_subnet_snat_rule(self, context, router_id, nsx_router_id, subnet,
+                              gw_address_scope, gw_ip):
+        # if the subnets address scope is the same as the gateways:
+        # no need for SNAT
+        if gw_address_scope:
+            subnet_address_scope = self._get_subnet_address_scope(
+                context, subnet['id'])
+            if (gw_address_scope == subnet_address_scope):
+                LOG.info("No need for SNAT rule for router %(router)s "
+                         "and subnet %(subnet)s because they use the "
+                         "same address scope %(addr_scope)s.",
+                         {'router': router_id,
+                          'subnet': subnet['id'],
+                          'addr_scope': gw_address_scope})
+                return
+
+        self._routerlib.add_gw_snat_rule(nsx_router_id, gw_ip,
+                                         source_net=subnet['cidr'],
+                                         bypass_firewall=False)
+
     def _process_extra_attr_router_create(self, context, router_db, r):
         for extra_attr in l3_attrs_db.get_attr_info().keys():
             if extra_attr in r:
@@ -3019,11 +3046,12 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
             nsx_net_id, nsx_port_id = nsx_db.get_nsx_switch_and_port_id(
                 context.session, port['id'])
             router_db = self._get_router(context, router_id)
+            gw_network_id = (router_db.gw_port.network_id if router_db.gw_port
+                             else None)
 
             # If it is a no-snat router, interface address scope must be the
             # same as the gateways
-            if not router_db.enable_snat:
+            if not router_db.enable_snat and gw_network_id:
-                gw_network_id = router_db.gw_port.network_id
                 self._validate_address_scope_for_router_interface(
                     context.elevated(), router_id, gw_network_id, subnet['id'])
 
@@ -3055,6 +3083,15 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
                 # its DHCP port), by creating it if needed.
                 nsx_rpc.handle_router_metadata_access(self, context, router_id,
                                                       interface=info)
+
+            # add the SNAT rule for this interface
+            if (router_db.enable_snat and gw_network_id and
+                router_db.gw_port.get('fixed_ips')):
+                gw_ip = router_db.gw_port['fixed_ips'][0]['ip_address']
+                gw_address_scope = self._get_network_address_scope(
+                    context, gw_network_id)
+                self._add_subnet_snat_rule(context, router_id, nsx_router_id,
+                                           subnet, gw_address_scope, gw_ip)
         except Exception:
             with excutils.save_and_reraise_exception():
                 LOG.error("Neutron failed to add_router_interface on "
@@ -3125,6 +3162,14 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
             else:
                 self.nsxlib.logical_router_port.delete_by_lswitch_id(
                     nsx_net_id)
+            # try to delete the SNAT rule of this subnet
+            if (router_db.gw_port and router_db.enable_snat and
+                router_db.gw_port.get('fixed_ips')):
+                gw_ip = router_db.gw_port['fixed_ips'][0]['ip_address']
+                self._routerlib.delete_gw_snat_rule_by_source(
+                    nsx_router_id, gw_ip, subnet['cidr'],
+                    skip_not_found=True)
+
         except nsx_lib_exc.ResourceNotFound:
             LOG.error("router port on router %(router_id)s for net "
                       "%(net_id)s not found at the backend",

vmware_nsx/tests/unit/nsx_v3/test_plugin.py

@@ -891,6 +891,92 @@ class TestL3NatTestCase(L3NatTest,
                                                   int_subnet['subnet']['id'],
                                                   None)
 
+    def test_router_address_scope_snat_rules(self):
+        """Test that if the router interface had the same address scope
+        as the gateway - snat rule is not added.
+        """
+        # create an external network on one address scope
+        with self.address_scope(name='as1') as addr_scope, \
+            self.network() as ext_net:
+            self._set_net_external(ext_net['network']['id'])
+            as_id = addr_scope['address_scope']['id']
+            subnet = netaddr.IPNetwork('10.10.10.0/21')
+            subnetpool = self._test_create_subnetpool(
+                [subnet.cidr], name='sp1',
+                min_prefixlen='24', address_scope_id=as_id)
+            subnetpool_id = subnetpool['subnetpool']['id']
+            data = {'subnet': {
+                    'network_id': ext_net['network']['id'],
+                    'subnetpool_id': subnetpool_id,
+                    'ip_version': 4,
+                    'enable_dhcp': False,
+                    'tenant_id': ext_net['network']['tenant_id']}}
+            req = self.new_create_request('subnets', data)
+            ext_subnet = self.deserialize(self.fmt, req.get_response(self.api))
+
+            # create a regular network on the same address scope
+            with self.network() as net:
+                data = {'subnet': {
+                        'network_id': net['network']['id'],
+                        'subnetpool_id': subnetpool_id,
+                        'ip_version': 4,
+                        'tenant_id': net['network']['tenant_id']}}
+                req = self.new_create_request('subnets', data)
+                int_subnet = self.deserialize(
+                    self.fmt, req.get_response(self.api))
+
+                # create a router with this gateway
+                with self.router() as r:
+                    self._add_external_gateway_to_router(
+                        r['router']['id'],
+                        ext_subnet['subnet']['network_id'])
+
+                    with mock.patch("vmware_nsxlib.v3.router.RouterLib."
+                                    "add_gw_snat_rule") as add_nat:
+                        # Add the interface
+                        self._router_interface_action(
+                            'add',
+                            r['router']['id'],
+                            int_subnet['subnet']['id'],
+                            None)
+                        # make sure snat rules are not added
+                        add_nat.assert_not_called()
+
+            # create a regular network on a different address scope
+            with self.address_scope(name='as2') as addr_scope2, \
+                self.network() as net:
+                as_id2 = addr_scope2['address_scope']['id']
+                subnet2 = netaddr.IPNetwork('20.10.10.0/24')
+                subnetpool2 = self._test_create_subnetpool(
+                    [subnet2.cidr], name='sp2',
+                    min_prefixlen='24', address_scope_id=as_id2)
+                subnetpool_id2 = subnetpool2['subnetpool']['id']
+                data = {'subnet': {
+                        'network_id': net['network']['id'],
+                        'subnetpool_id': subnetpool_id2,
+                        'ip_version': 4,
+                        'tenant_id': net['network']['tenant_id']}}
+                req = self.new_create_request('subnets', data)
+                int_subnet = self.deserialize(
+                    self.fmt, req.get_response(self.api))
+
+                # create a router with this gateway
+                with self.router() as r:
+                    self._add_external_gateway_to_router(
+                        r['router']['id'],
+                        ext_subnet['subnet']['network_id'])
+
+                    with mock.patch("vmware_nsxlib.v3.router.RouterLib."
+                                    "add_gw_snat_rule") as add_nat:
+                        # Add the interface
+                        self._router_interface_action(
+                            'add',
+                            r['router']['id'],
+                            int_subnet['subnet']['id'],
+                            None)
+                        # make sure snat rules are added
+                        add_nat.assert_called_once()
+
 
 class ExtGwModeTestCase(test_ext_gw_mode.ExtGwModeIntTestCase,
                         L3NatTest):