From 4a84cc65bc0d8dda583765c94c23c91cc3848d8a Mon Sep 17 00:00:00 2001 From: Adit Sarfaty Date: Sun, 16 Jul 2017 11:14:03 +0300 Subject: [PATCH] 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 | 16 ++++ vmware_nsx/plugins/nsx_v/plugin.py | 19 +++-- vmware_nsx/tests/unit/nsx_v/test_md_proxy.py | 3 + vmware_nsx/tests/unit/nsx_v/test_plugin.py | 86 ++++++++++++++++++++ 4 files changed, 116 insertions(+), 8 deletions(-) diff --git a/vmware_nsx/plugins/common/plugin.py b/vmware_nsx/plugins/common/plugin.py index 8f18f0f2c8..c15256adcf 100644 --- a/vmware_nsx/plugins/common/plugin.py +++ b/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. diff --git a/vmware_nsx/plugins/nsx_v/plugin.py b/vmware_nsx/plugins/nsx_v/plugin.py index 05f5ac9344..9db4690a70 100644 --- a/vmware_nsx/plugins/nsx_v/plugin.py +++ b/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]) - #TODO(asarfaty): need a separate rule per address scope - if subnet_cidrs: - subnet_fw_rule = { + routes_dest = [route['destination'] for route in routes] + for subnet_cidrs in subnet_cidrs_per_ads: + # create a rule to allow east-west traffic between subnets on this + # 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, - 'destination_ip_address': subnet_cidrs} - fw_rules.append(subnet_fw_rule) + 'source_ip_address': ips, + 'destination_ip_address': ips}) return fw_rules def _update_nat_rules(self, context, router, router_id=None): diff --git a/vmware_nsx/tests/unit/nsx_v/test_md_proxy.py b/vmware_nsx/tests/unit/nsx_v/test_md_proxy.py index 977c8ec857..591884808e 100644 --- a/vmware_nsx/tests/unit/nsx_v/test_md_proxy.py +++ b/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): diff --git a/vmware_nsx/tests/unit/nsx_v/test_plugin.py b/vmware_nsx/tests/unit/nsx_v/test_plugin.py index c5321d4034..5a9c5eca2a 100644 --- a/vmware_nsx/tests/unit/nsx_v/test_plugin.py +++ b/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):