From 3887d898b42e833b4a544b851257b239ff50effd Mon Sep 17 00:00:00 2001 From: Adit Sarfaty Date: Wed, 28 Jun 2017 07:54:58 +0300 Subject: [PATCH] NSX|V3: Do not add SNAT rules if same address scope In case the router GW and interface belong to the same address scope there is not need to add SNAT rules for this interface, as there are no overlapping addresses. For this end, we need to add specific SNAT rule for each of the router relevant subnets, instead of just 1 SNAT all rule. Those NAT rules will be deleted when the GW is removed, changed to no-SNAT or when an interface is removed. Depends-on: Id43a2ced7d6526f538f485f345c20ba44673c7b2 Change-Id: Ie528503ec77b397a19fdcf90cfced738fdfc7a22 --- vmware_nsx/plugins/common/plugin.py | 29 +++++++ vmware_nsx/plugins/nsx_v/plugin.py | 28 ------- vmware_nsx/plugins/nsx_v3/plugin.py | 55 +++++++++++-- vmware_nsx/tests/unit/nsx_v3/test_plugin.py | 86 +++++++++++++++++++++ 4 files changed, 165 insertions(+), 33 deletions(-) diff --git a/vmware_nsx/plugins/common/plugin.py b/vmware_nsx/plugins/common/plugin.py index 23505ed5ef..8f18f0f2c8 100644 --- a/vmware_nsx/plugins/common/plugin.py +++ b/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 diff --git a/vmware_nsx/plugins/nsx_v/plugin.py b/vmware_nsx/plugins/nsx_v/plugin.py index ac1fde8fc1..e4bbbd50f8 100644 --- a/vmware_nsx/plugins/nsx_v/plugin.py +++ b/vmware_nsx/plugins/nsx_v/plugin.py @@ -3243,34 +3243,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() diff --git a/vmware_nsx/plugins/nsx_v3/plugin.py b/vmware_nsx/plugins/nsx_v3/plugin.py index b020983f30..2a40060165 100644 --- a/vmware_nsx/plugins/nsx_v3/plugin.py +++ b/vmware_nsx/plugins/nsx_v3/plugin.py @@ -2737,7 +2737,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) @@ -2752,8 +2752,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, - bypass_firewall=False) + # Add SNAT rules for all the subnets which are in different scope + # 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 @@ -2762,6 +2769,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: @@ -3070,11 +3097,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: - gw_network_id = router_db.gw_port.network_id + if not router_db.enable_snat and gw_network_id: self._validate_address_scope_for_router_interface( context, router_id, gw_network_id, subnet['id']) @@ -3106,6 +3134,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 " @@ -3176,6 +3213,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", diff --git a/vmware_nsx/tests/unit/nsx_v3/test_plugin.py b/vmware_nsx/tests/unit/nsx_v3/test_plugin.py index a2637b29cf..c641b2c13c 100644 --- a/vmware_nsx/tests/unit/nsx_v3/test_plugin.py +++ b/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):