From 5fb5653ffe700a8f9fb0949d16ecbf0978b61fb6 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 5 May 2021 17:26:23 +0000 Subject: [PATCH] Check router routes connectivity when GW port is updated When the router GW interface is updated or deleted, the routes should be checked in other to prove all of them have connectivity to any of the router interface subnets. In a router, all routes nexthops must have connectivity with one interface subnet. If not, this route cannot send the packets to this route gateway; therefore this route becomes invalid. Closes-Bug: #1925368 Change-Id: I7ce93b863b0dc0d4a2376fcfd602d2facb6fb2d0 --- neutron/db/extraroute_db.py | 35 ++++--- neutron/db/l3_db.py | 50 +++++++--- neutron/tests/unit/db/test_l3_db.py | 97 ++++++++++++++++++- ...router-gw-and-routes-af45c89c52612028.yaml | 6 ++ 4 files changed, 163 insertions(+), 25 deletions(-) create mode 100644 releasenotes/notes/validate-router-gw-and-routes-af45c89c52612028.yaml diff --git a/neutron/db/extraroute_db.py b/neutron/db/extraroute_db.py index d6339fe6231..74870008336 100644 --- a/neutron/db/extraroute_db.py +++ b/neutron/db/extraroute_db.py @@ -85,8 +85,18 @@ class ExtraRoute_dbonly_mixin(l3_db.L3_NAT_dbonly_mixin): routes=routes, reason=_('the nexthop is used by router')) - def _validate_routes(self, context, - router_id, routes): + def _validate_routes(self, context, router_id, routes, cidrs=None, + ip_addresses=None): + """Validate a router routes with its interface subnets CIDRs and IPs + + If any route cannot reach any subnet CIDR from any interface or the + route nethop match any interface IP address, this route is invalid. + :param context: Neutron request context + :param router_id: router ID + :param routes: router routes (list of dictionaries) + :param cidrs: (optional) list of CIDRs (strings) + :param ip_addresses: (optional) list of IP addresses (strings) + """ if len(routes) > cfg.CONF.max_routes: raise xroute_exc.RoutesExhausted( router_id=router_id, @@ -94,17 +104,20 @@ class ExtraRoute_dbonly_mixin(l3_db.L3_NAT_dbonly_mixin): context = context.elevated() filters = {'device_id': [router_id]} - ports = self._core_plugin.get_ports(context, filters) - cidrs = [] - ips = [] - for port in ports: - for ip in port['fixed_ips']: - cidrs.append(self._core_plugin.get_subnet( - context, ip['subnet_id'])['cidr']) - ips.append(ip['ip_address']) + + cidrs = cidrs or [] + ip_addresses = ip_addresses or [] + if not (cidrs or ip_addresses): + ports = self._core_plugin.get_ports(context, filters) + for port in ports: + for ip in port['fixed_ips']: + cidrs.append(self._core_plugin.get_subnet( + context, ip['subnet_id'])['cidr']) + ip_addresses.append(ip['ip_address']) + for route in routes: self._validate_routes_nexthop( - cidrs, ips, routes, route['nexthop']) + cidrs, ip_addresses, routes, route['nexthop']) def _update_extra_routes(self, context, router, routes): self._validate_routes(context, router['id'], routes) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index eb8e6795395..88695aba1e1 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -341,24 +341,48 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, ) router_port.create() - def _validate_gw_info(self, context, gw_port, info, ext_ips): + def _validate_gw_info(self, context, info, ext_ips, router): network_id = info['network_id'] if info else None + gw_subnets = [] if network_id: network_db = self._core_plugin._get_network(context, network_id) if not network_db.external: msg = _("Network %s is not an external network") % network_id raise n_exc.BadRequest(resource='router', msg=msg) - if ext_ips: - subnets = self._core_plugin.get_subnets_by_network(context, - network_id) - for s in subnets: - if not s['gateway_ip']: + + gw_subnets = network_db.subnets + for ext_ip in ext_ips: + for subnet in network_db.subnets: + if not subnet.gateway_ip: continue - for ext_ip in ext_ips: - if ext_ip.get('ip_address') == s['gateway_ip']: - msg = _("External IP %s is the same as the " - "gateway IP") % ext_ip.get('ip_address') - raise n_exc.BadRequest(resource='router', msg=msg) + if ext_ip.get('ip_address') == subnet.gateway_ip: + msg = _("External IP %s is the same as the " + "gateway IP") % ext_ip.get('ip_address') + raise n_exc.BadRequest(resource='router', msg=msg) + + method_validate_routes = getattr(self, '_validate_routes', None) + if not method_validate_routes or not router.route_list: + return network_id + + cidrs = [] + ip_addresses = [] + # All attached ports but the GW port, if exists, that will change + # because the GW information is being updated. + attached_ports = [rp.port for rp in router.attached_ports if + rp.port.id != router.gw_port_id] + for port in attached_ports: + for ip in port.fixed_ips: + cidrs.append(self._core_plugin.get_subnet( + context.elevated(), ip.subnet_id)['cidr']) + ip_addresses.append(ip.ip_address) + + # NOTE(ralonsoh): the GW port is not updated yet and the its IP address + # won't be added to "ip_addresses", only the subnet CIDRs. + for gw_subnet in gw_subnets: + cidrs.append(gw_subnet.cidr) + + method_validate_routes(context, router.id, router.route_list, + cidrs=cidrs, ip_addresses=ip_addresses) return network_id # NOTE(yamamoto): This method is an override point for plugins @@ -470,10 +494,10 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, def _update_router_gw_info(self, context, router_id, info, router=None): router = router or self._get_router(context, router_id) gw_port = router.gw_port - ext_ips = info.get('external_fixed_ips') if info else [] + ext_ips = info.get('external_fixed_ips', []) if info else [] ext_ip_change = self._check_for_external_ip_change( context, gw_port, ext_ips) - network_id = self._validate_gw_info(context, gw_port, info, ext_ips) + network_id = self._validate_gw_info(context, info, ext_ips, router) if gw_port and ext_ip_change and gw_port['network_id'] == network_id: self._update_current_gw_port(context, router_id, router, ext_ips) diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index a0c1ef07111..9f583427e35 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -24,14 +24,17 @@ from neutron_lib import constants as n_const from neutron_lib import context from neutron_lib.db import api as db_api from neutron_lib import exceptions as n_exc +from neutron_lib.exceptions import extraroute as xroute_exc from neutron_lib.plugins import constants as plugin_constants from neutron_lib.plugins import directory from neutron_lib.plugins import utils as plugin_utils from oslo_utils import uuidutils import testtools +from neutron.db import extraroute_db from neutron.db import l3_db from neutron.db.models import l3 as l3_models +from neutron.db import models_v2 from neutron.extensions import segment as segment_ext from neutron.objects import base as base_obj from neutron.objects import network as network_obj @@ -48,7 +51,12 @@ class TestL3_NAT_dbonly_mixin( def setUp(self, *args, **kwargs): super(TestL3_NAT_dbonly_mixin, self).setUp(*args, **kwargs) - self.db = l3_db.L3_NAT_dbonly_mixin() + # "extraroute_db.ExtraRoute_dbonly_mixin" inherits from + # "l3_db.L3_NAT_dbonly_mixin()", the class under test. This is used + # instead to test the validation of router routes and GW change because + # implements "_validate_routes". + self.db = extraroute_db.ExtraRoute_dbonly_mixin() + self.ctx = mock.Mock() def test__each_port_having_fixed_ips_none(self): """Be sure the method returns an empty list when None is passed""" @@ -416,6 +424,93 @@ class TestL3_NAT_dbonly_mixin( cb_payload.metadata.get('network_id')) self.assertEqual(router_id, cb_payload.resource_id) + def _create_router(self, gw_port=True, num_ports=2, create_routes=True): + # GW CIDR: 10.0.0.0/24 + # Interface CIDRS: 10.0.1.0/24, 10.0.2.0/24, etc. + router_id = uuidutils.generate_uuid() + port_gw_cidr = netaddr.IPNetwork('10.0.0.0/24') + rports = [] + if gw_port: + port_gw = models_v2.Port( + id=uuidutils.generate_uuid(), + fixed_ips=[models_v2.IPAllocation( + ip_address=str(port_gw_cidr.ip + 1))]) + rports.append(l3_models.RouterPort(router_id=router_id, + port=port_gw)) + else: + port_gw = None + + port_cidrs = [] + port_subnets = [] + for idx in range(num_ports): + cidr = port_gw_cidr.cidr.next(idx + 1) + port = models_v2.Port( + id=uuidutils.generate_uuid(), + fixed_ips=[models_v2.IPAllocation( + ip_address=str(cidr.ip + 1))]) + port_cidrs.append(cidr) + rports.append(l3_models.RouterPort(router_id=router_id, port=port)) + port_subnets.append({'cidr': str(cidr)}) + + routes = [] + if create_routes: + for cidr in [*port_cidrs, port_gw_cidr]: + routes.append(l3_models.RouterRoute( + destination=str(cidr.next(100)), + nexthop=str(cidr.ip + 10))) + return (l3_models.Router( + id=router_id, attached_ports=rports, route_list=routes, + gw_port_id=port_gw.id if port_gw else None), port_subnets) + + def test__validate_gw_info(self): + gw_network = mock.Mock(subnets=[mock.Mock(cidr='10.0.0.0/24')], + external=True) + router, port_subnets = self._create_router(gw_port=False) + info = {'network_id': 'net_id'} + with mock.patch.object(self.db._core_plugin, '_get_network', + return_value=gw_network), \ + mock.patch.object(self.db._core_plugin, 'get_subnet', + side_effect=port_subnets): + self.assertEqual( + 'net_id', + self.db._validate_gw_info(self.ctx, info, [], router)) + + def test__validate_gw_info_no_route_connectivity(self): + gw_network = mock.Mock(subnets=[mock.Mock(cidr='10.50.0.0/24')], + external=True) + router, port_subnets = self._create_router(gw_port=False) + info = {'network_id': 'net_id'} + with mock.patch.object(self.db._core_plugin, '_get_network', + return_value=gw_network), \ + mock.patch.object(self.db._core_plugin, 'get_subnet', + side_effect=port_subnets): + self.assertRaises( + xroute_exc.InvalidRoutes, self.db._validate_gw_info, self.ctx, + info, [], router) + + def test__validate_gw_info_delete_gateway(self): + router, port_subnets = self._create_router() + info = {'network_id': None} + with mock.patch.object(self.db._core_plugin, '_get_network', + return_value=None), \ + mock.patch.object(self.db._core_plugin, 'get_subnet', + side_effect=port_subnets): + self.assertRaises( + xroute_exc.InvalidRoutes, self.db._validate_gw_info, self.ctx, + info, [], router) + + def test__validate_gw_info_delete_gateway_no_route(self): + gw_network = mock.Mock(subnets=[mock.Mock(cidr='10.50.0.0/24')], + external=True) + router, port_subnets = self._create_router(create_routes=False) + info = {'network_id': None} + with mock.patch.object(self.db._core_plugin, '_get_network', + return_value=gw_network), \ + mock.patch.object(self.db._core_plugin, 'get_subnet', + side_effect=port_subnets): + self.assertIsNone( + self.db._validate_gw_info(mock.ANY, info, [], router)) + class L3_NAT_db_mixin(base.BaseTestCase): def setUp(self): diff --git a/releasenotes/notes/validate-router-gw-and-routes-af45c89c52612028.yaml b/releasenotes/notes/validate-router-gw-and-routes-af45c89c52612028.yaml new file mode 100644 index 00000000000..a710fba609e --- /dev/null +++ b/releasenotes/notes/validate-router-gw-and-routes-af45c89c52612028.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + Reject any router route or gateway update if not all route nexthops have + connectivity with any gateway subnets CIDRs; in other words, all route + nexthops IP addresses should belong to one gateway subnet CIDR.