From 429c77c574f471bdbb4dd91debef7a72c920b1e0 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 18 Jun 2019 16:52:10 +0000 Subject: [PATCH] Implement "ip route delete" command using Pyroute2 Change-Id: I960455d6a9bc1b633d485c42a26b3a254731558e Related-Bug: #1492714 --- neutron/agent/l3/dvr_local_router.py | 8 +- neutron/agent/linux/ip_lib.py | 68 ++++-------- neutron/privileged/agent/linux/ip_lib.py | 90 +++++++++++---- .../functional/agent/linux/test_ip_lib.py | 51 +++++++-- .../unit/agent/l3/test_dvr_local_router.py | 3 +- neutron/tests/unit/agent/linux/test_ip_lib.py | 103 ------------------ 6 files changed, 141 insertions(+), 182 deletions(-) diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index 59693025e77..1b273e6fcfb 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -17,7 +17,6 @@ import collections import netaddr from neutron_lib import constants as lib_constants -from neutron_lib import exceptions from oslo_log import log as logging from oslo_utils import excutils import six @@ -26,6 +25,7 @@ from neutron.agent.l3 import dvr_fip_ns from neutron.agent.l3 import dvr_router_base from neutron.agent.linux import ip_lib from neutron.common import utils as common_utils +from neutron.privileged.agent.linux import ip_lib as priv_ip_lib LOG = logging.getLogger(__name__) # xor-folding mask used for IPv6 rule index @@ -180,7 +180,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): device = ip_lib.IPDevice(fip_2_rtr_name, namespace=fip_ns_name) - device.route.delete_route(fip_cidr, str(rtr_2_fip.ip)) + device.route.delete_route(fip_cidr, via=str(rtr_2_fip.ip)) return device def floating_ip_moved_dist(self, fip): @@ -322,7 +322,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): snat_idx): try: ns_ip_device.route.delete_gateway(gw_ip_addr, table=snat_idx) - except exceptions.DeviceNotFoundError: + except priv_ip_lib.NetworkInterfaceNotFound: pass def _stale_ip_rule_cleanup(self, namespace, ns_ipd, ip_version): @@ -676,7 +676,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): return for subnet in router_port['subnets']: rtr_port_cidr = subnet['cidr'] - device.route.delete_route(rtr_port_cidr, str(rtr_2_fip_ip)) + device.route.delete_route(rtr_port_cidr, via=str(rtr_2_fip_ip)) def _add_interface_route_to_fip_ns(self, router_port): rtr_2_fip_ip, fip_2_rtr_name = self.get_rtr_fip_ip_and_interface_name() diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index 1ba219cea72..cf36df28f3c 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -23,7 +23,6 @@ from neutron_lib import constants from neutron_lib import exceptions from oslo_config import cfg from oslo_log import log as logging -from oslo_utils import excutils from pyroute2.netlink import exceptions as netlink_exceptions from pyroute2.netlink import rtnl from pyroute2.netlink.rtnl import ifaddrmsg @@ -586,38 +585,13 @@ class IpRouteCommand(IpDeviceCommandBase): super(IpRouteCommand, self).__init__(parent) self._table = table - def table(self, table): - """Return an instance of IpRouteCommand which works on given table""" - return IpRouteCommand(self._parent, table) - - def _table_args(self, override=None): - if override: - return ['table', override] - return ['table', self._table] if self._table else [] - - def _dev_args(self): - return ['dev', self.name] if self.name else [] - def add_gateway(self, gateway, metric=None, table=None, scope='global'): self.add_route(None, via=gateway, table=table, metric=metric, scope=scope) - def _run_as_root_detect_device_not_found(self, options, args): - try: - return self._as_root(options, tuple(args)) - except RuntimeError as rte: - with excutils.save_and_reraise_exception() as ctx: - if "Cannot find device" in str(rte): - ctx.reraise = False - raise exceptions.DeviceNotFoundError(device_name=self.name) - - def delete_gateway(self, gateway, table=None): - ip_version = common_utils.get_ip_version(gateway) - args = ['del', 'default', - 'via', gateway] - args += self._dev_args() - args += self._table_args(table) - self._run_as_root_detect_device_not_found([ip_version], args) + def delete_gateway(self, gateway, table=None, scope=None): + self.delete_route(None, device=self.name, via=gateway, table=table, + scope=scope) def list_routes(self, ip_version, scope=None, via=None, table=None, **kwargs): @@ -633,7 +607,7 @@ class IpRouteCommand(IpDeviceCommandBase): self.add_route(cidr, scope='link') def delete_onlink_route(self, cidr): - self.delete_route(cidr, scope='link') + self.delete_route(cidr, device=self.name, scope='link') def get_gateway(self, scope=None, table=None, ip_version=constants.IP_VERSION_4): @@ -643,11 +617,9 @@ class IpRouteCommand(IpDeviceCommandBase): return route def flush(self, ip_version, table=None, **kwargs): - args = ['flush'] - args += self._table_args(table) - for k, v in kwargs.items(): - args += [k, v] - self._as_root([ip_version], tuple(args)) + for route in self.list_routes(ip_version, table=table): + self.delete_route(route['cidr'], device=route['device'], + via=route['via'], table=table, **kwargs) def add_route(self, cidr, via=None, table=None, metric=None, scope=None, **kwargs): @@ -655,16 +627,11 @@ class IpRouteCommand(IpDeviceCommandBase): add_ip_route(self._parent.namespace, cidr, device=self.name, via=via, table=table, metric=metric, scope=scope, **kwargs) - def delete_route(self, cidr, via=None, table=None, **kwargs): - ip_version = common_utils.get_ip_version(cidr) - args = ['del', cidr] - if via: - args += ['via', via] - args += self._dev_args() - args += self._table_args(table) - for k, v in kwargs.items(): - args += [k, v] - self._run_as_root_detect_device_not_found([ip_version], args) + def delete_route(self, cidr, device=None, via=None, table=None, scope=None, + **kwargs): + table = table or self._table + delete_ip_route(self._parent.namespace, cidr, device=device, via=via, + table=table, scope=scope, **kwargs) class IPRoute(SubProcessBase): @@ -1531,3 +1498,14 @@ def list_ip_routes(namespace, ip_version, scope=None, via=None, table=None, ret = [route for route in ret if route['via'] == via] return ret + + +def delete_ip_route(namespace, cidr, device=None, via=None, table=None, + scope=None, **kwargs): + """Delete an IP route""" + if table: + table = IP_RULE_TABLES.get(table, table) + ip_version = common_utils.get_ip_version(cidr or via) + privileged.delete_ip_route(namespace, cidr, ip_version, + device=device, via=via, table=table, + scope=scope, **kwargs) diff --git a/neutron/privileged/agent/linux/ip_lib.py b/neutron/privileged/agent/linux/ip_lib.py index 62eb3ed7225..f0c787b0c65 100644 --- a/neutron/privileged/agent/linux/ip_lib.py +++ b/neutron/privileged/agent/linux/ip_lib.py @@ -650,29 +650,56 @@ def delete_ip_rule(namespace, **kwargs): raise +def _make_pyroute2_route_args(namespace, ip_version, cidr, device, via, table, + metric, scope, protocol): + """Returns a dictionary of arguments to be used in pyroute route commands + + :param namespace: (string) name of the namespace + :param ip_version: (int) [4, 6] + :param cidr: (string) source IP or CIDR address (IPv4, IPv6) + :param device: (string) input interface name + :param via: (string) gateway IP address + :param table: (string, int) table number or name + :param metric: (int) route metric + :param scope: (int) route scope + :param protocol: (string) protocol name (pyroute2.netlink.rtnl.rt_proto) + :return: a dictionary with the kwargs needed in pyroute rule commands + """ + args = {'family': _IP_VERSION_FAMILY_MAP[ip_version]} + if not scope: + scope = 'global' if via else 'link' + scope = _get_scope_name(scope) + if scope: + args['scope'] = scope + if cidr: + args['dst'] = cidr + if device: + args['oif'] = get_link_id(device, namespace) + if via: + args['gateway'] = via + if table: + args['table'] = int(table) + if metric: + args['priority'] = int(metric) + if protocol: + args['proto'] = protocol + return args + + @privileged.default.entrypoint +# NOTE(slaweq): Because of issue with pyroute2.NetNS objects running in threads +# we need to lock this function to workaround this issue. +# For details please check https://bugs.launchpad.net/neutron/+bug/1811515 @lockutils.synchronized("privileged-ip-lib") def add_ip_route(namespace, cidr, ip_version, device=None, via=None, table=None, metric=None, scope=None, **kwargs): """Add an IP route""" + kwargs.update(_make_pyroute2_route_args( + namespace, ip_version, cidr, device, via, table, metric, scope, + 'static')) try: with get_iproute(namespace) as ip: - family = _IP_VERSION_FAMILY_MAP[ip_version] - if not scope: - scope = 'global' if via else 'link' - scope = _get_scope_name(scope) - if cidr: - kwargs['dst'] = cidr - if via: - kwargs['gateway'] = via - if table: - kwargs['table'] = int(table) - if device: - kwargs['oif'] = get_link_id(device, namespace) - if metric: - kwargs['priority'] = int(metric) - ip.route('replace', family=family, scope=scope, proto='static', - **kwargs) + ip.route('replace', **kwargs) except OSError as e: if e.errno == errno.ENOENT: raise NetworkNamespaceNotFound(netns_name=namespace) @@ -680,17 +707,36 @@ def add_ip_route(namespace, cidr, ip_version, device=None, via=None, @privileged.default.entrypoint +# NOTE(slaweq): Because of issue with pyroute2.NetNS objects running in threads +# we need to lock this function to workaround this issue. +# For details please check https://bugs.launchpad.net/neutron/+bug/1811515 @lockutils.synchronized("privileged-ip-lib") def list_ip_routes(namespace, ip_version, device=None, table=None, **kwargs): """List IP routes""" + kwargs.update(_make_pyroute2_route_args( + namespace, ip_version, None, device, None, table, None, None, None)) try: with get_iproute(namespace) as ip: - family = _IP_VERSION_FAMILY_MAP[ip_version] - if table: - kwargs['table'] = table - if device: - kwargs['oif'] = get_link_id(device, namespace) - return make_serializable(ip.route('show', family=family, **kwargs)) + return make_serializable(ip.route('show', **kwargs)) + except OSError as e: + if e.errno == errno.ENOENT: + raise NetworkNamespaceNotFound(netns_name=namespace) + raise + + +@privileged.default.entrypoint +# NOTE(slaweq): Because of issue with pyroute2.NetNS objects running in threads +# we need to lock this function to workaround this issue. +# For details please check https://bugs.launchpad.net/neutron/+bug/1811515 +@lockutils.synchronized("privileged-ip-lib") +def delete_ip_route(namespace, cidr, ip_version, device=None, via=None, + table=None, scope=None, **kwargs): + """Delete an IP route""" + kwargs.update(_make_pyroute2_route_args( + namespace, ip_version, cidr, device, via, table, None, scope, None)) + try: + with get_iproute(namespace) as ip: + ip.route('del', **kwargs) except OSError as e: if e.errno == errno.ENOENT: raise NetworkNamespaceNotFound(netns_name=namespace) diff --git a/neutron/tests/functional/agent/linux/test_ip_lib.py b/neutron/tests/functional/agent/linux/test_ip_lib.py index e8025951390..2bf15ec4fa3 100644 --- a/neutron/tests/functional/agent/linux/test_ip_lib.py +++ b/neutron/tests/functional/agent/linux/test_ip_lib.py @@ -846,7 +846,17 @@ class IpRouteCommandTestCase(functional_base.BaseSudoTestCase): self.cidrs = ['192.168.0.0/24', '10.0.0.0/8', '2001::/64', 'faaa::/96'] def _assert_route(self, ip_version, table=None, source_prefix=None, - cidr=None, scope=None, via=None, metric=None): + cidr=None, scope=None, via=None, metric=None, + not_in=False): + if not_in: + fn = lambda: cmp not in self.device.route.list_routes(ip_version, + table=table) + msg = 'Route found: %s' + else: + fn = lambda: cmp in self.device.route.list_routes(ip_version, + table=table) + msg = 'Route not found: %s' + if cidr: ip_version = utils.get_ip_version(cidr) else: @@ -868,11 +878,9 @@ class IpRouteCommandTestCase(functional_base.BaseSudoTestCase): 'via': via, 'priority': metric} try: - utils.wait_until_true(lambda: cmp in self.device.route.list_routes( - ip_version, table=table), timeout=5) + utils.wait_until_true(fn, timeout=5) except utils.WaitTimeout: - raise self.fail('Route not found: %s' % cmp) - return True + raise self.fail(msg % cmp) def test_add_route_table(self): tables = (None, 1, 253, 254, 255) @@ -929,7 +937,7 @@ class IpRouteCommandTestCase(functional_base.BaseSudoTestCase): routes = self.device.route.list_onlink_routes(constants.IP_VERSION_4) self.assertEqual(len(cidr_ipv4), len(routes)) - def test_get_gateway(self): + def test_get_and_delete_gateway(self): gateways = (str(netaddr.IPNetwork(self.device_cidr_ipv4).ip), str(netaddr.IPNetwork(self.device_cidr_ipv6).ip + 1)) scopes = ('global', 'site', 'link') @@ -944,4 +952,33 @@ class IpRouteCommandTestCase(functional_base.BaseSudoTestCase): metric=metric, table=table) self.assertEqual(gateway, self.device.route.get_gateway( ip_version=ip_version, table=table)['via']) - self.device.route.delete_gateway(gateway, table=table) + + self.device.route.delete_gateway(gateway, table=table, scope=scope) + self.assertIsNone(self.device.route.get_gateway( + ip_version=ip_version, table=table)) + + def test_delete_route(self): + scopes = ('global', 'site', 'link') + tables = (None, 1, 254, 255) + for cidr, scope, table in itertools.product( + self.cidrs, scopes, tables): + ip_version = utils.get_ip_version(cidr) + self.device.route.add_route(cidr, table=table, scope=scope) + self._assert_route(ip_version, cidr=cidr, scope=scope, table=table) + + self.device.route.delete_route(cidr, table=table, scope=scope) + self._assert_route(ip_version, cidr=cidr, scope=scope, table=table, + not_in=True) + + def test_flush(self): + tables = (None, 1, 200) + ip_versions = (constants.IP_VERSION_4, constants.IP_VERSION_6) + for cidr, table in itertools.product(self.cidrs, tables): + self.device.route.add_route(cidr, table=table) + + for ip_version, table in itertools.product(ip_versions, tables): + routes = self.device.route.list_routes(ip_version, table=table) + self.assertGreater(len(routes), 0) + self.device.route.flush(ip_version, table=table) + routes = self.device.route.list_routes(ip_version, table=table) + self.assertEqual([], routes) diff --git a/neutron/tests/unit/agent/l3/test_dvr_local_router.py b/neutron/tests/unit/agent/l3/test_dvr_local_router.py index d9ad4138432..fe41cbba053 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -386,7 +386,8 @@ class TestDvrRouterOperations(base.BaseTestCase): ri.floating_ip_removed_dist(fip_cidr) self.mock_delete_ip_rule.assert_called_with( ri.router_namespace.name, ip=fixed_ip, table=16, priority=FIP_PRI) - mIPDevice().route.delete_route.assert_called_with(fip_cidr, str(s.ip)) + mIPDevice().route.delete_route.assert_called_with(fip_cidr, + via=str(s.ip)) ri.fip_ns.local_subnets.allocate.assert_not_called() @mock.patch.object(ip_lib, 'add_ip_rule') diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index 593dfad3a87..63b97962812 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -841,109 +841,6 @@ class TestIpAddrCommand(TestIPCmdBase): self.assertEqual(0, len(retval)) -class TestIpRouteCommand(TestIPCmdBase): - def setUp(self): - super(TestIpRouteCommand, self).setUp() - self.parent.name = 'eth0' - self.command = 'route' - self.route_cmd = ip_lib.IpRouteCommand(self.parent) - self.ip_version = constants.IP_VERSION_4 - self.table = 14 - self.metric = 100 - self.cidr = '192.168.45.100/24' - self.ip = '10.0.0.1' - self.gateway = '192.168.45.100' - self.test_cases = [{'sample': GATEWAY_SAMPLE1, - 'expected': {'gateway': '10.35.19.254', - 'metric': 100}}, - {'sample': GATEWAY_SAMPLE2, - 'expected': {'gateway': '10.35.19.254', - 'metric': 100}}, - {'sample': GATEWAY_SAMPLE3, - 'expected': None}, - {'sample': GATEWAY_SAMPLE4, - 'expected': {'gateway': '10.35.19.254'}}, - {'sample': GATEWAY_SAMPLE5, - 'expected': {'gateway': '192.168.99.1'}}, - {'sample': GATEWAY_SAMPLE6, - 'expected': {'gateway': '192.168.99.1', - 'metric': 100}}, - {'sample': GATEWAY_SAMPLE7, - 'expected': {'metric': 1}}] - - def test_del_gateway_cannot_find_device(self): - self.parent._as_root.side_effect = RuntimeError("Cannot find device") - - exc = self.assertRaises(exceptions.DeviceNotFoundError, - self.route_cmd.delete_gateway, - self.gateway, table=self.table) - self.assertIn(self.parent.name, str(exc)) - - def test_del_gateway_other_error(self): - self.parent._as_root.side_effect = RuntimeError() - - self.assertRaises(RuntimeError, self.route_cmd.delete_gateway, - self.gateway, table=self.table) - - def test_flush_route_table(self): - self.route_cmd.flush(self.ip_version, self.table) - self._assert_sudo([self.ip_version], ('flush', 'table', self.table)) - - def test_delete_route(self): - self.route_cmd.delete_route(self.cidr, self.ip, self.table) - self._assert_sudo([self.ip_version], - ('del', self.cidr, - 'via', self.ip, - 'dev', self.parent.name, - 'table', self.table)) - - def test_delete_route_no_via(self): - self.route_cmd.delete_route(self.cidr, table=self.table) - self._assert_sudo([self.ip_version], - ('del', self.cidr, - 'dev', self.parent.name, - 'table', self.table)) - - def test_delete_route_with_scope(self): - self.route_cmd.delete_route(self.cidr, scope='link') - self._assert_sudo([self.ip_version], - ('del', self.cidr, - 'dev', self.parent.name, - 'scope', 'link')) - - def test_delete_route_no_device(self): - self.parent._as_root.side_effect = RuntimeError("Cannot find device") - self.assertRaises(exceptions.DeviceNotFoundError, - self.route_cmd.delete_route, - self.cidr, self.ip, self.table) - - -class TestIPv6IpRouteCommand(TestIpRouteCommand): - def setUp(self): - super(TestIPv6IpRouteCommand, self).setUp() - self.ip_version = constants.IP_VERSION_6 - self.cidr = '2001:db8::/64' - self.ip = '2001:db8::100' - self.gateway = '2001:db8::1' - self.test_cases = [{'sample': IPv6_GATEWAY_SAMPLE1, - 'expected': - {'gateway': '2001:470:9:1224:4508:b885:5fb:740b', - 'metric': 100}}, - {'sample': IPv6_GATEWAY_SAMPLE2, - 'expected': - {'gateway': '2001:470:9:1224:4508:b885:5fb:740b', - 'metric': 100}}, - {'sample': IPv6_GATEWAY_SAMPLE3, - 'expected': None}, - {'sample': IPv6_GATEWAY_SAMPLE4, - 'expected': - {'gateway': 'fe80::dfcc:aaff:feb9:76ce'}}, - {'sample': IPv6_GATEWAY_SAMPLE5, - 'expected': - {'gateway': '2001:470:9:1224:4508:b885:5fb:740b', - 'metric': 1024}}] - - class TestIpNetnsCommand(TestIPCmdBase): def setUp(self): super(TestIpNetnsCommand, self).setUp()