Discard east-west traffic between different address scopes

If subnets from different address scopes are attached to the same router,
traffic between them should not be allowed.

Change-Id: Ib7b4204bab1ab6a01c54ea1647eeeef2144fd469

vmware_nsx/plugins/common/plugin.py

@@ -133,6 +133,22 @@ class NsxPluginBase(db_base_plugin_v2.NeutronDbPluginV2,
         subnets = self._find_router_subnets_and_cidrs(context, router_id)
         return [subnet['cidr'] for subnet in subnets]
 
+    def _find_router_subnets_cidrs_per_addr_scope(self, context, router_id):
+        """Generate a list of cidrs per address pool.
+
+        Go over all the router interface subnets.
+        return a list of lists of subnets cidrs belonging to same
+        address pool.
+        """
+        subnets = self._find_router_subnets_and_cidrs(context, router_id)
+        cidrs_map = {}
+        for subnet in subnets:
+            ads = self._get_subnet_address_scope(context, subnet['id']) or ''
+            if ads not in cidrs_map:
+                cidrs_map[ads] = []
+            cidrs_map[ads].append(subnet['cidr'])
+        return list(cidrs_map.values())
+
     def _get_port_by_device_id(self, context, device_id, device_owner):
         """Retrieve ports associated with a specific device id.
 

vmware_nsx/plugins/nsx_v/plugin.py

@@ -3413,18 +3413,21 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin,
         # Get FW rule/s to open subnets firewall flows and static routes
         # relative flows
         fw_rules = []
-        subnet_cidrs = self._find_router_subnets_cidrs(context, router['id'])
+        subnet_cidrs_per_ads = self._find_router_subnets_cidrs_per_addr_scope(
+            context.elevated(), router['id'])
         routes = self._get_extra_routes_by_router_id(context, router['id'])
-        subnet_cidrs.extend([route['destination'] for route in routes])
+        routes_dest = [route['destination'] for route in routes]
-        #TODO(asarfaty): need a separate rule per address scope
+        for subnet_cidrs in subnet_cidrs_per_ads:
-        if subnet_cidrs:
+            # create a rule to allow east-west traffic between subnets on this
-            subnet_fw_rule = {
+            # address scope
+            # Also add the static routes to each address scope
+            ips = subnet_cidrs + routes_dest
+            fw_rules.append({
                 'name': SUBNET_RULE_NAME,
                 'action': 'allow',
                 'enabled': True,
-                'source_ip_address': subnet_cidrs,
+                'source_ip_address': ips,
-                'destination_ip_address': subnet_cidrs}
+                'destination_ip_address': ips})
-            fw_rules.append(subnet_fw_rule)
         return fw_rules
 
     def _update_nat_rules(self, context, router, router_id=None):

vmware_nsx/tests/unit/nsx_v/test_md_proxy.py

@@ -187,6 +187,9 @@ class TestExclusiveRouterWithMdTestCase(
     def test_router_address_scope_snat_rules(self):
         self.skipTest("The test is not suitable for the metadata test case")
 
+    def test_router_address_scope_fw_rules(self):
+        self.skipTest("The test is not suitable for the metadata test case")
+
 
 class TestVdrWithMdTestCase(test_plugin.TestVdrTestCase,
                             NsxVPluginWithMdV2TestCase):

vmware_nsx/tests/unit/nsx_v/test_plugin.py

@@ -3695,6 +3695,92 @@ class TestExclusiveRouterTestCase(L3NatTest, L3NatTestCaseBase,
                         self.assertEqual('external',
                                          fw_rules[1]['source_vnic_groups'][0])
 
+    def test_router_address_scope_fw_rules(self):
+        """Test that if the router interfaces has different address scope
+        there are separate fw rules
+        """
+        # create a router, networks, and address scopes
+        with self.address_scope(name='as1') as addr_scope1, \
+            self.address_scope(name='as2') as addr_scope2,\
+            self.network() as net1, self.network() as net2,\
+            self.network() as net3, self.router() as r:
+
+            as1_id = addr_scope1['address_scope']['id']
+            as2_id = addr_scope2['address_scope']['id']
+            pool1 = netaddr.IPNetwork('10.10.10.0/21')
+            subnetpool1 = self._test_create_subnetpool(
+                [pool1.cidr], name='sp1',
+                min_prefixlen='24', address_scope_id=as1_id)
+            pool2 = netaddr.IPNetwork('20.20.20.0/21')
+            subnetpool2 = self._test_create_subnetpool(
+                [pool2.cidr], name='sp2',
+                min_prefixlen='24', address_scope_id=as2_id)
+            subnetpool_id1 = subnetpool1['subnetpool']['id']
+            subnetpool_id2 = subnetpool2['subnetpool']['id']
+
+            # create subnets on the 2 subnet pools
+            data = {'subnet': {
+                    'network_id': net1['network']['id'],
+                    'subnetpool_id': subnetpool_id1,
+                    'ip_version': 4,
+                    'tenant_id': net1['network']['tenant_id']}}
+            req = self.new_create_request('subnets', data)
+            subnet1 = self.deserialize(
+                self.fmt, req.get_response(self.api))
+
+            data = {'subnet': {
+                    'network_id': net2['network']['id'],
+                    'subnetpool_id': subnetpool_id2,
+                    'ip_version': 4,
+                    'tenant_id': net2['network']['tenant_id']}}
+            req = self.new_create_request('subnets', data)
+            subnet2 = self.deserialize(
+                self.fmt, req.get_response(self.api))
+
+            data = {'subnet': {
+                    'network_id': net3['network']['id'],
+                    'subnetpool_id': subnetpool_id2,
+                    'ip_version': 4,
+                    'tenant_id': net3['network']['tenant_id']}}
+            req = self.new_create_request('subnets', data)
+            subnet3 = self.deserialize(
+                self.fmt, req.get_response(self.api))
+
+            expected_rules = [
+                {'enabled': True,
+                 'destination_ip_address': [subnet1['subnet']['cidr']],
+                 'action': 'allow',
+                 'name': 'Subnet Rule',
+                 'source_ip_address': [subnet1['subnet']['cidr']]},
+                {'enabled': True,
+                 'destination_ip_address': [subnet2['subnet']['cidr'],
+                                            subnet3['subnet']['cidr']],
+                 'action': 'allow',
+                 'name': 'Subnet Rule',
+                 'source_ip_address': [subnet2['subnet']['cidr'],
+                                       subnet3['subnet']['cidr']]}]
+
+            # Add the interfaces to the router
+            with mock.patch.object(
+                edge_utils, 'update_nat_rules'),\
+                mock.patch.object(edge_utils, 'update_firewall') as update_fw:
+                self._router_interface_action(
+                    'add', r['router']['id'],
+                    subnet1['subnet']['id'], None)
+                self._router_interface_action(
+                    'add', r['router']['id'],
+                    subnet2['subnet']['id'], None)
+                self._router_interface_action(
+                    'add', r['router']['id'],
+                    subnet3['subnet']['id'], None)
+
+                # check the final fw rules
+                fw_rules = update_fw.call_args[0][3][
+                    'firewall_rule_list']
+                self.assertEqual(2, len(fw_rules))
+                self.assertEqual(self._recursive_sort_list(expected_rules),
+                                 self._recursive_sort_list(fw_rules))
+
 
 class ExtGwModeTestCase(NsxVPluginV2TestCase,
                         test_ext_gw_mode.ExtGwModeIntTestCase):