From 7a73c2d0f87bb269d0cced1847edce4d1e76179e Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Tue, 30 Jun 2015 20:23:39 +0000 Subject: [PATCH] Refactor IpRuleCommand to take more arguments The iproute2 rule command takes more arguments than the ones supported by this wrapper. Particularly, for address scopes, we're interested in iif and fwmark. Instead of adding these piecemeal, this change makes the wrapper flexible to pass any of them using kwargs. Callers of add / delete are updated to pass keyword arguments for table and priority since they are no longer required positional arguments. This looks better anyway. Change-Id: Ia93b086b787c34bd560961cb84e4a003cf359e7e Partially-Implements: blueprint address-scopes --- neutron/agent/l3/dvr_local_router.py | 17 ++++-- neutron/agent/linux/ip_lib.py | 61 +++++++++++++------ .../unit/agent/l3/test_dvr_local_router.py | 6 +- neutron/tests/unit/agent/linux/test_ip_lib.py | 27 ++++++-- 4 files changed, 81 insertions(+), 30 deletions(-) diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index ead84e4e7..e306c7c91 100755 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -84,7 +84,9 @@ class DvrLocalRouter(router.RouterInfo): self.floating_ips_dict[floating_ip] = rule_pr fip_2_rtr_name = self.fip_ns.get_int_device_name(self.router_id) ip_rule = ip_lib.IPRule(namespace=self.ns_name) - ip_rule.rule.add(fixed_ip, dvr_fip_ns.FIP_RT_TBL, rule_pr) + ip_rule.rule.add(ip=fixed_ip, + table=dvr_fip_ns.FIP_RT_TBL, + priority=rule_pr) #Add routing rule in fip namespace fip_ns_name = self.fip_ns.get_name() rtr_2_fip, _ = self.rtr_fip_subnet.get_pair() @@ -114,7 +116,9 @@ class DvrLocalRouter(router.RouterInfo): if floating_ip in self.floating_ips_dict: rule_pr = self.floating_ips_dict[floating_ip] ip_rule = ip_lib.IPRule(namespace=self.ns_name) - ip_rule.rule.delete(floating_ip, dvr_fip_ns.FIP_RT_TBL, rule_pr) + ip_rule.rule.delete(ip=floating_ip, + table=dvr_fip_ns.FIP_RT_TBL, + priority=rule_pr) self.fip_ns.deallocate_rule_priority(rule_pr) #TODO(rajeev): Handle else case - exception/log? @@ -258,7 +262,9 @@ class DvrLocalRouter(router.RouterInfo): if is_add: ns_ipd.route.add_gateway(gw_ip_addr, table=snat_idx) - ns_ipr.rule.add(sn_port_cidr, snat_idx, snat_idx) + ns_ipr.rule.add(ip=sn_port_cidr, + table=snat_idx, + priority=snat_idx) ns_ipwrapr.netns.execute( ['sysctl', '-w', 'net.ipv4.conf.%s.send_redirects=0' % sn_int]) @@ -266,8 +272,9 @@ class DvrLocalRouter(router.RouterInfo): self._delete_gateway_device_if_exists(ns_ipd, gw_ip_addr, snat_idx) - ns_ipr.rule.delete(sn_port_cidr, snat_idx, - snat_idx) + ns_ipr.rule.delete(ip=sn_port_cidr, + table=snat_idx, + priority=snat_idx) break except Exception: if is_add: diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index 890444a01..edb35d5a7 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -22,6 +22,7 @@ from oslo_utils import excutils import re from neutron.agent.common import utils +from neutron.common import constants from neutron.common import exceptions from neutron.i18n import _LE @@ -281,30 +282,56 @@ class IPRule(SubProcessBase): class IpRuleCommand(IpCommandBase): COMMAND = 'rule' - def _exists(self, ip, ip_version, table, rule_pr): - # Typical rule from 'ip rule show': + ALL = {4: constants.IPv4_ANY, 6: constants.IPv6_ANY} + + def _parse_line(self, ip_version, line): + # Typical rules from 'ip rule show': # 4030201: from 1.2.3.4/24 lookup 10203040 + # 1024: from all iif qg-c43b1928-48 lookup noscope - rule_pr = str(rule_pr) + ":" - for line in self._as_root([ip_version], ['show']).splitlines(): - parts = line.split() - if parts and (parts[0] == rule_pr and - parts[2] == str(ip) and - parts[-1] == str(table)): - return True + parts = line.split() + if not parts: + return {} - return False + # Format of line is: "priority: ..." + settings = {k: v for k, v in zip(parts[1::2], parts[2::2])} + settings['priority'] = parts[0][:-1] + + # Canonicalize some arguments + if settings.get('from') == "all": + settings['from'] = self.ALL[ip_version] + if 'lookup' in settings: + settings['table'] = settings.pop('lookup') + + return settings + + def _exists(self, ip_version, **kwargs): + kwargs_strings = {k: str(v) for k, v in kwargs.items()} + lines = self._as_root([ip_version], ['show']).splitlines() + return kwargs_strings in (self._parse_line(ip_version, line) + for line in lines) - def add(self, ip, table, rule_pr): + def _make__flat_args_tuple(self, *args, **kwargs): + for kwargs_item in sorted(kwargs.items(), key=lambda i: i[0]): + args += kwargs_item + return tuple(args) + + def add(self, ip, **kwargs): ip_version = get_ip_version(ip) - if not self._exists(ip, ip_version, table, rule_pr): - args = ['add', 'from', ip, 'table', table, 'priority', rule_pr] - self._as_root([ip_version], tuple(args)) - def delete(self, ip, table, rule_pr): + kwargs.update({'from': ip}) + + if not self._exists(ip_version, **kwargs): + args_tuple = self._make__flat_args_tuple('add', **kwargs) + self._as_root([ip_version], args_tuple) + + def delete(self, ip, **kwargs): ip_version = get_ip_version(ip) - args = ['del', 'table', table, 'priority', rule_pr] - self._as_root([ip_version], tuple(args)) + + # TODO(Carl) ip ignored in delete, okay in general? + + args_tuple = self._make__flat_args_tuple('del', **kwargs) + self._as_root([ip_version], args_tuple) class IpDeviceCommandBase(IpCommandBase): 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 51e89802f..bec9168af 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -199,7 +199,9 @@ class TestDvrRouterOperations(base.BaseTestCase): ri.dist_fip_count = 0 ip_cidr = common_utils.ip_to_cidr(fip['floating_ip_address']) ri.floating_ip_added_dist(fip, ip_cidr) - mIPRule().rule.add.assert_called_with('192.168.0.1', 16, FIP_PRI) + mIPRule().rule.add.assert_called_with(ip='192.168.0.1', + table=16, + priority=FIP_PRI) self.assertEqual(1, ri.dist_fip_count) # TODO(mrsmith): add more asserts @@ -233,7 +235,7 @@ class TestDvrRouterOperations(base.BaseTestCase): ri.rtr_fip_subnet = s ri.floating_ip_removed_dist(fip_cidr) mIPRule().rule.delete.assert_called_with( - str(netaddr.IPNetwork(fip_cidr).ip), 16, FIP_PRI) + ip=str(netaddr.IPNetwork(fip_cidr).ip), table=16, priority=FIP_PRI) mIPDevice().route.delete_route.assert_called_with(fip_cidr, str(s.ip)) self.assertFalse(ri.fip_ns.unsubscribe.called) diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index 42c3befa3..f28232cdf 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -551,23 +551,38 @@ class TestIpRuleCommand(TestIPCmdBase): def _test_add_rule(self, ip, table, priority): ip_version = netaddr.IPNetwork(ip).version - self.rule_cmd.add(ip, table, priority) + self.rule_cmd.add(ip, table=table, priority=priority) self._assert_sudo([ip_version], (['show'])) self._assert_sudo([ip_version], ('add', 'from', ip, - 'table', table, 'priority', priority)) + 'priority', priority, 'table', table)) def _test_add_rule_exists(self, ip, table, priority, output): self.parent._as_root.return_value = output ip_version = netaddr.IPNetwork(ip).version - self.rule_cmd.add(ip, table, priority) + self.rule_cmd.add(ip, table=table, priority=priority) self._assert_sudo([ip_version], (['show'])) def _test_delete_rule(self, ip, table, priority): ip_version = netaddr.IPNetwork(ip).version - self.rule_cmd.delete(ip, table, priority) + self.rule_cmd.delete(ip, table=table, priority=priority) self._assert_sudo([ip_version], - ('del', 'table', table, - 'priority', priority)) + ('del', 'priority', priority, + 'table', table)) + + def test__parse_line(self): + def test(ip_version, line, expected): + actual = self.rule_cmd._parse_line(ip_version, line) + self.assertEqual(expected, actual) + + test(4, "4030201:\tfrom 1.2.3.4/24 lookup 10203040", + {'from': '1.2.3.4/24', + 'table': '10203040', + 'priority': '4030201'}) + test(6, "1024: from all iif qg-c43b1928-48 lookup noscope", + {'priority': '1024', + 'from': '::/0', + 'iif': 'qg-c43b1928-48', + 'table': 'noscope'}) def test_add_rule_v4(self): self._test_add_rule('192.168.45.100', 2, 100) -- 2.45.2