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.