Merge "NSXv3: Handle address scope change on subnetpool"

vmware_nsx/plugins/common/plugin.py

@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+
 from oslo_log import log as logging
 
 from neutron.db import _resource_extend as resource_extend
@@ -108,6 +109,12 @@ class NsxPluginBase(db_base_plugin_v2.NeutronDbPluginV2,
         subnetpool = self.get_subnetpool(context, subnet['subnetpool_id'])
         return subnetpool.get('address_scope_id', '')
 
+    def _get_subnetpool_address_scope(self, context, subnetpool_id):
+        if not subnetpool_id:
+            return
+        subnetpool = self.get_subnetpool(context, subnetpool_id)
+        return subnetpool.get('address_scope_id', '')
+
     # TODO(asarfaty): the NSX-V3 needs a very similar code too
     def _validate_address_scope_for_router_interface(self, context, router_id,
                                                      gw_network_id, subnet_id):
@@ -130,7 +137,7 @@ class NsxPluginBase(db_base_plugin_v2.NeutronDbPluginV2,
 
     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)
+        subnets = self._find_router_subnets(context, router_id)
         return [subnet['cidr'] for subnet in subnets]
 
     def _find_router_subnets_cidrs_per_addr_scope(self, context, router_id):
@@ -140,10 +147,11 @@ class NsxPluginBase(db_base_plugin_v2.NeutronDbPluginV2,
         return a list of lists of subnets cidrs belonging to same
         address pool.
         """
-        subnets = self._find_router_subnets_and_cidrs(context, router_id)
+        subnets = self._find_router_subnets(context, router_id)
         cidrs_map = {}
         for subnet in subnets:
-            ads = self._get_subnet_address_scope(context, subnet['id']) or ''
+            ads = self._get_subnetpool_address_scope(
+                context, subnet['subnetpool_id']) or ''
             if ads not in cidrs_map:
                 cidrs_map[ads] = []
             cidrs_map[ads].append(subnet['cidr'])
@@ -159,7 +167,7 @@ class NsxPluginBase(db_base_plugin_v2.NeutronDbPluginV2,
             device_id=device_id,
             device_owner=device_owner,).all()
 
-    def _find_router_subnets_and_cidrs(self, context, router_id):
+    def _find_router_subnets(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)
@@ -169,5 +177,80 @@ class NsxPluginBase(db_base_plugin_v2.NeutronDbPluginV2,
             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})
+                subnets.append({'id': subnet.id, 'cidr': subnet.cidr,
+                                'subnetpool_id': subnet.subnetpool_id})
         return subnets
+
+    def _find_router_gw_subnets(self, context, router):
+        """Retrieve external subnets attached to router GW"""
+        if not router['external_gateway_info']:
+            return []
+
+        subnets = []
+        for fip in router['external_gateway_info']['external_fixed_ips']:
+            subnet = self.get_subnet(context, fip['subnet_id'])
+            subnets.append(subnet)
+
+        return subnets
+
+    def recalculate_snat_rules_for_router(self, context, router, subnets):
+        """Method to recalculate router snat rules for specific subnets.
+        Invoked when subnetpool address scope changes.
+        Implemented in child plugin classes
+        """
+        pass
+
+    def _filter_subnets_by_subnetpool(self, subnets, subnetpool_id):
+        return [subnet for subnet in subnets
+                if subnet['subnetpool_id'] == subnetpool_id]
+
+    def on_subnetpool_address_scope_updated(self, resource, event,
+                                            trigger, **kwargs):
+        context = kwargs['context']
+
+        routers = self.get_routers(context)
+        subnetpool_id = kwargs['subnetpool_id']
+        elevated_context = context.elevated()
+        LOG.info("Inspecting routers for potential configuration changes "
+                 "due to address scope change on subnetpool %s", subnetpool_id)
+        for rtr in routers:
+            subnets = self._find_router_subnets(elevated_context,
+                                                rtr['id'])
+            gw_subnets = self._find_router_gw_subnets(elevated_context,
+                                                      rtr)
+
+            affected_subnets = self._filter_subnets_by_subnetpool(
+                subnets, subnetpool_id)
+            affected_gw_subnets = self._filter_subnets_by_subnetpool(
+                gw_subnets, subnetpool_id)
+
+            if not affected_subnets and not affected_gw_subnets:
+                # No subnets were affected by address scope change
+                continue
+
+            if (affected_subnets == subnets and
+                affected_gw_subnets == gw_subnets):
+                # All subnets remain under the same address scope
+                # (all router subnets were allocated from subnetpool_id)
+                continue
+
+            # TODO(annak): handle east-west FW rules
+            if not rtr['external_gateway_info']:
+                continue
+
+            if not rtr['external_gateway_info']['enable_snat']:
+                LOG.warning("Due to address scope change on subnetpool "
+                            "%(subnetpool)s, uniqueness on interface "
+                            "addresses on no-snat router %(router) is no "
+                            "longer guaranteed, which may result in faulty "
+                            "operation.", {'subnetpool': subnetpool_id,
+                                           'router': rtr['id']})
+                continue
+
+            if affected_gw_subnets:
+                # GW address scope have changed - we need to revisit snat
+                # rules for all router interfaces
+                affected_subnets = subnets
+
+            self.recalculate_snat_rules_for_router(context, rtr,
+                                                   affected_subnets)

vmware_nsx/plugins/nsx_v/plugin.py

@@ -3327,7 +3327,7 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin,
         gw_port = router.gw_port
         if gw_port and gw_port.get('fixed_ips') and router.enable_snat:
             snat_ip = gw_port['fixed_ips'][0]['ip_address']
-            subnets = self._find_router_subnets_and_cidrs(context.elevated(),
+            subnets = self._find_router_subnets(context.elevated(),
                                                 router['id'])
             for subnet in subnets:
 
@@ -3335,8 +3335,8 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin,
                 # no need for SNAT
                 gw_address_scope = self._get_network_address_scope(
                     context.elevated(), gw_port['network_id'])
-                subnet_address_scope = self._get_subnet_address_scope(
+                subnet_address_scope = self._get_subnetpool_address_scope(
-                    context.elevated(), subnet['id'])
+                    context.elevated(), subnet['subnetpool_id'])
                 if (gw_address_scope and
                     gw_address_scope == subnet_address_scope):
                     LOG.info("No need for SNAT rule for router %(router)s "
@@ -3386,14 +3386,14 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin,
         if gw_address_scope is None:
             return
 
-        subnets = self._find_router_subnets_and_cidrs(context.elevated(),
+        subnets = self._find_router_subnets(context.elevated(),
                                             router['id'])
         no_nat_cidrs = []
         for subnet in subnets:
             # if the subnets address scope is the same as the gateways:
             # we should add it to the rule
-            subnet_address_scope = self._get_subnet_address_scope(
+            subnet_address_scope = self._get_subnetpool_address_scope(
-                context.elevated(), subnet['id'])
+                context.elevated(), subnet['subnetpool_id'])
             if (gw_address_scope == subnet_address_scope):
                 no_nat_cidrs.append(subnet['cidr'])
 

vmware_nsx/plugins/nsx_v3/plugin.py

@@ -195,6 +195,10 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
             self.nsxlib.reinitialize_cluster,
             resources.PROCESS, events.AFTER_INIT)
 
+        registry.subscribe(
+            self.on_subnetpool_address_scope_updated,
+            resources.SUBNETPOOL_ADDRESS_SCOPE, events.AFTER_UPDATE)
+
         self._nsx_version = self.nsxlib.get_version()
         LOG.info("NSX Version: %s", self._nsx_version)
 
@@ -2796,7 +2800,7 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
             # 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(),
+            subnets = self._find_router_subnets(context.elevated(),
                                                 router_id)
             for subnet in subnets:
                 self._add_subnet_snat_rule(context, router_id, nsx_router_id,
@@ -2814,8 +2818,8 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
         # 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(
+            subnet_address_scope = self._get_subnetpool_address_scope(
-                context, subnet['id'])
+                context, subnet['subnetpool_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 "
@@ -3702,3 +3706,45 @@ class NsxV3Plugin(agentschedulers_db.AZDhcpAgentSchedulerDbMixin,
             else:
                 az_name = nsx_az.DEFAULT_NAME
             net_res[az_ext.AVAILABILITY_ZONES] = [az_name]
+
+    def recalculate_snat_rules_for_router(self, context, router, subnets):
+        """Rrecalculate router snat rules for specific subnets.
+        Invoked when subnetpool address scope changes.
+        """
+        nsx_router_id = nsx_db.get_nsx_router_id(context.session,
+                                                 router['id'])
+
+        if not router['external_gateway_info']:
+            return
+
+        LOG.info("Recalculating snat rules for router %s", router['id'])
+        fip = router['external_gateway_info']['external_fixed_ips'][0]
+        ext_addr = fip['ip_address']
+        gw_address_scope = self._get_network_address_scope(
+            context, router['external_gateway_info']['network_id'])
+
+        # TODO(annak): improve amount of backend calls by rebuilding all
+        # snat rules when API is available
+        for subnet in subnets:
+            if gw_address_scope:
+                subnet_address_scope = self._get_subnetpool_address_scope(
+                    context, subnet['subnetpool_id'])
+                LOG.info("Deleting SNAT rule for %(router)s "
+                         "and subnet %(subnet)s",
+                         {'router': router['id'],
+                          'subnet': subnet['id']})
+
+                # Delete rule for this router/subnet pair if it exists
+                self._routerlib.delete_gw_snat_rule_by_source(
+                    nsx_router_id, ext_addr, subnet['cidr'],
+                    skip_not_found=True)
+
+                if (gw_address_scope != subnet_address_scope):
+                    # subnet is no longer under same address scope with GW
+                    LOG.info("Adding SNAT rule for %(router)s "
+                             "and subnet %(subnet)s",
+                             {'router': router['id'],
+                              'subnet': subnet['id']})
+                    self._routerlib.add_gw_snat_rule(nsx_router_id, ext_addr,
+                                                     source_net=subnet['cidr'],
+                                                     bypass_firewall=False)

vmware_nsx/tests/unit/nsx_v3/test_plugin.py

@@ -892,15 +892,20 @@ class TestL3NatTestCase(L3NatTest,
                                                   int_subnet['subnet']['id'],
                                                   None)
 
-    def test_router_address_scope_snat_rules(self):
+    def _mock_add_snat_rule(self):
-        """Test that if the router interface had the same address scope
+        return mock.patch("vmware_nsxlib.v3.router.RouterLib."
-        as the gateway - snat rule is not added.
+                          "add_gw_snat_rule")
-        """
+
-        # create an external network on one address scope
+    def _mock_del_snat_rule(self):
-        with self.address_scope(name='as1') as addr_scope, \
+        return mock.patch("vmware_nsxlib.v3.router.RouterLib."
-            self.network() as ext_net:
+                          "delete_gw_snat_rule_by_source")
+
+    def _prepare_external_subnet_on_address_scope(self,
+                                                  ext_net,
+                                                  address_scope):
+
         self._set_net_external(ext_net['network']['id'])
-            as_id = addr_scope['address_scope']['id']
+        as_id = address_scope['address_scope']['id']
         subnet = netaddr.IPNetwork('10.10.10.0/21')
         subnetpool = self._test_create_subnetpool(
             [subnet.cidr], name='sp1',
@@ -914,7 +919,12 @@ class TestL3NatTestCase(L3NatTest,
                 '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))
+        return ext_subnet['subnet']
 
+    def _create_subnet_and_assert_snat_rules(self, subnetpool_id,
+                                             router_id,
+                                             assert_snat_deleted=False,
+                                             assert_snat_added=False):
         # create a regular network on the same address scope
         with self.network() as net:
             data = {'subnet': {
@@ -926,58 +936,250 @@ class TestL3NatTestCase(L3NatTest,
             int_subnet = self.deserialize(
                 self.fmt, req.get_response(self.api))
 
+            with self._mock_add_snat_rule() as add_nat,\
+                self._mock_del_snat_rule() as delete_nat:
+                # Add the interface
+                self._router_interface_action(
+                    'add',
+                    router_id,
+                    int_subnet['subnet']['id'],
+                    None)
+
+                if assert_snat_deleted:
+                    delete_nat.assert_called()
+                else:
+                    delete_nat.assert_not_called()
+
+                if assert_snat_added:
+                    add_nat.assert_called()
+                else:
+                    add_nat.assert_not_called()
+
+    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:
+            ext_subnet = self._prepare_external_subnet_on_address_scope(
+                ext_net, addr_scope)
+
             # 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'])
+                    ext_subnet['network_id'])
 
-                    with mock.patch("vmware_nsxlib.v3.router.RouterLib."
+            # create a regular network on same address scope
-                                    "add_gw_snat_rule") as add_nat:
+            # and verify no snat change
-                        # Add the interface
+            as_id = addr_scope['address_scope']['id']
-                        self._router_interface_action(
+            subnet = netaddr.IPNetwork('30.10.10.0/24')
-                            'add',
+            subnetpool = self._test_create_subnetpool(
-                            r['router']['id'],
+                [subnet.cidr], name='sp2',
-                            int_subnet['subnet']['id'],
+                min_prefixlen='24', address_scope_id=as_id)
-                            None)
+            as_id = addr_scope['address_scope']['id']
-                        # make sure snat rules are not added
+            subnetpool_id = subnetpool['subnetpool']['id']
-                        add_nat.assert_not_called()
+            self._create_subnet_and_assert_snat_rules(
+                subnetpool_id, r['router']['id'])
 
             # create a regular network on a different address scope
-            with self.address_scope(name='as2') as addr_scope2, \
+            # and verify snat rules are added
-                self.network() as net:
+            with self.address_scope(name='as2') as addr_scope2:
-                as_id2 = addr_scope2['address_scope']['id']
+                as2_id = 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)
+                    min_prefixlen='24', address_scope_id=as2_id)
-                subnetpool_id2 = subnetpool2['subnetpool']['id']
+                subnetpool2_id = subnetpool2['subnetpool']['id']
-                data = {'subnet': {
+
-                        'network_id': net['network']['id'],
+                self._create_subnet_and_assert_snat_rules(
-                        'subnetpool_id': subnetpool_id2,
+                    subnetpool2_id, r['router']['id'],
-                        'ip_version': 4,
+                    assert_snat_added=True)
-                        'tenant_id': net['network']['tenant_id']}}
+
-                req = self.new_create_request('subnets', data)
+    def _test_router_address_scope_change(self, change_gw=False):
-                int_subnet = self.deserialize(
+        """When subnetpool address scope changes, and router that was
-                    self.fmt, req.get_response(self.api))
+        originally under same address scope, results having different
+        address scopes, relevant snat rules are added.
+        """
+        # create an external network on one address scope
+        with self.address_scope(name='as1') as addr_scope, \
+            self.network() as ext_net:
+            ext_subnet = self._prepare_external_subnet_on_address_scope(
+                ext_net, addr_scope)
 
             # 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'])
+                    ext_subnet['network_id'])
+
+            # create a regular network on same address scope
+            # and verify no snat change
+            as_id = addr_scope['address_scope']['id']
+            subnet2 = netaddr.IPNetwork('40.10.10.0/24')
+            subnetpool2 = self._test_create_subnetpool(
+                [subnet2.cidr], name='sp2',
+                min_prefixlen='24', address_scope_id=as_id)
+            subnetpool2_id = subnetpool2['subnetpool']['id']
+
+            self._create_subnet_and_assert_snat_rules(
+                subnetpool2_id, r['router']['id'])
+
+            # change address scope of the first subnetpool
+            with self.address_scope(name='as2') as addr_scope2,\
+                self._mock_add_snat_rule() as add_nat:
+
+                as2_id = addr_scope2['address_scope']['id']
+                data = {'subnetpool': {
+                        'address_scope_id': as2_id}}
+
+                if change_gw:
+                    subnetpool_to_update = ext_subnet['subnetpool_id']
+                else:
+                    subnetpool_to_update = subnetpool2_id
+
+                req = self.new_update_request('subnetpools', data,
+                                              subnetpool_to_update)
+                req.get_response(self.api)
 
-                    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()
 
+    def test_router_address_scope_change(self):
+        self._test_router_address_scope_change()
+
+    def test_router_address_scope_gw_change(self):
+        self._test_router_address_scope_change(change_gw=True)
+
+    def _test_3leg_router_address_scope_change(self, change_gw=False,
+                                               change_2gw=False):
+        """Test address scope change scenarios with router that covers
+        3 address scopes
+        """
+        # create an external network on one address scope
+        with self.address_scope(name='as1') as as1, \
+            self.address_scope(name='as2') as as2, \
+            self.address_scope(name='as3') as as3, \
+            self.network() as ext_net:
+            ext_subnet = self._prepare_external_subnet_on_address_scope(
+                ext_net, as1)
+            as1_id = as1['address_scope']['id']
+
+            # create a router with this gateway
+            with self.router() as r:
+                self._add_external_gateway_to_router(
+                    r['router']['id'],
+                    ext_subnet['network_id'])
+
+            # create a regular network on address scope 2
+            # and verify snat change
+            as2_id = as2['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=as2_id)
+            subnetpool2_id = subnetpool2['subnetpool']['id']
+            self._create_subnet_and_assert_snat_rules(
+                subnetpool2_id, r['router']['id'], assert_snat_added=True)
+
+            # create a regular network on address scope 3
+            # verify no snat change
+            as3_id = as3['address_scope']['id']
+            subnet3 = netaddr.IPNetwork('30.10.10.0/24')
+            subnetpool3 = self._test_create_subnetpool(
+                [subnet3.cidr], name='sp2',
+                min_prefixlen='24', address_scope_id=as3_id)
+            subnetpool3_id = subnetpool3['subnetpool']['id']
+            self._create_subnet_and_assert_snat_rules(
+                subnetpool3_id, r['router']['id'], assert_snat_added=True)
+
+            with self._mock_add_snat_rule() as add_nat, \
+                self._mock_del_snat_rule() as del_nat:
+
+                if change_gw:
+                    # change address scope of GW subnet
+                    subnetpool_to_update = ext_subnet['subnetpool_id']
+                else:
+                    subnetpool_to_update = subnetpool2_id
+
+                if change_2gw:
+                    # change subnet2 to be in GW address scope
+                    target_as = as1_id
+                else:
+                    target_as = as3_id
+
+                data = {'subnetpool': {
+                        'address_scope_id': target_as}}
+
+                req = self.new_update_request('subnetpools', data,
+                                              subnetpool_to_update)
+                req.get_response(self.api)
+
+                if change_gw:
+                    # The test changed address scope of gw subnet.
+                    # Both previous rules should be deleted,
+                    # and one new rule for subnet2 should be added
+                    del_nat.assert_called()
+                    self.assertEqual(2, del_nat.call_count)
+                    add_nat.assert_called_once()
+                else:
+                    if change_2gw:
+                        # The test changed address scope of subnet2 to be
+                        # same as GW address scope.
+                        # Snat rule for as2 will be deleted. No effect on as3
+                        # rule.
+                        del_nat.assert_called_once()
+                    else:
+                        # The test changed address scope of subnet2 to
+                        # as3. Affected snat rule should be re-created.
+                        del_nat.assert_called_once()
+                        add_nat.assert_called_once()
+
+    def test_3leg_router_address_scope_change(self):
+        self._test_3leg_router_address_scope_change()
+
+    def test_3leg_router_address_scope_change_to_gw(self, change_2gw=True):
+        self._test_3leg_router_address_scope_change()
+
+    def test_3leg_router_gw_address_scope_change(self):
+        self._test_3leg_router_address_scope_change(change_gw=True)
+
+    def test_subnetpool_router_address_scope_change_no_effect(self):
+        """When all router interfaces are allocated from same subnetpool,
+        changing address scope on this subnetpool should not affect snat rules.
+        """
+        # create an external network on one address scope
+        with self.address_scope(name='as1') as addr_scope, \
+            self.network() as ext_net:
+            ext_subnet = self._prepare_external_subnet_on_address_scope(
+                ext_net, addr_scope)
+
+            # create a router with this gateway
+            with self.router() as r:
+                self._add_external_gateway_to_router(
+                    r['router']['id'],
+                    ext_subnet['network_id'])
+
+            # create a regular network on same address scope
+            # and verify no snat change
+            self._create_subnet_and_assert_snat_rules(
+                ext_subnet['subnetpool_id'], r['router']['id'])
+
+            with self.address_scope(name='as2') as addr_scope2,\
+                self._mock_add_snat_rule() as add_nat,\
+                self._mock_del_snat_rule() as delete_nat:
+
+                as2_id = addr_scope2['address_scope']['id']
+                # change address scope of the subnetpool
+                data = {'subnetpool': {
+                        'address_scope_id': as2_id}}
+                req = self.new_update_request('subnetpools', data,
+                                              ext_subnet['subnetpool_id'])
+                req.get_response(self.api)
+
+                add_nat.assert_not_called()
+                delete_nat.assert_not_called()
+
 
 class ExtGwModeTestCase(test_ext_gw_mode.ExtGwModeIntTestCase,
                         L3NatTest):