From: Carl Baldwin Date: Fri, 28 Aug 2015 21:19:40 +0000 (+0000) Subject: Make ip rule comparison more robust X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=7bd30aa49c24dc65332740e4fa74da28533b92ed;p=openstack-build%2Fneutron-build.git Make ip rule comparison more robust I found that ip rules would be added multiple times in new address scopes code because the _exists method was unable to reliably determine if the rule already existed. This commit improves this by more robustly canonicalizing what it reads from the ip rule command so that like rules always compare the same. Change-Id: I6d0c208f0ed8e65cdb750789321a7ad6ca1b77c2 Partially-Implements: blueprint address-scopes --- diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index dedf8d5b6..961d55791 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -20,6 +20,7 @@ from oslo_config import cfg from oslo_log import log as logging from oslo_utils import excutils import re +import six from neutron.agent.common import utils from neutron.common import constants @@ -287,6 +288,67 @@ class IPRule(SubProcessBase): class IpRuleCommand(IpCommandBase): COMMAND = 'rule' + @staticmethod + def _make_canonical(ip_version, settings): + """Converts settings to a canonical represention to compare easily""" + def canonicalize_fwmark_string(fwmark_mask): + """Reformats fwmark/mask in to a canonical form + + Examples, these are all equivalent: + "0x1" + 0x1 + "0x1/0xfffffffff" + (0x1, 0xfffffffff) + + :param fwmark_mask: The firewall and mask (default 0xffffffff) + :type fwmark_mask: A string with / as delimiter, an iterable, or a + single value. + """ + # Turn the value we were passed in to an iterable: fwmark[, mask] + if isinstance(fwmark_mask, six.string_types): + # A / separates the optional mask in a string + iterable = fwmark_mask.split('/') + else: + try: + iterable = iter(fwmark_mask) + except TypeError: + # At this point, it must be a single integer + iterable = [fwmark_mask] + + def to_i(s): + if isinstance(s, six.string_types): + # Passing 0 as "base" arg to "int" causes it to determine + # the base automatically. + return int(s, 0) + # s isn't a string, can't specify base argument + return int(s) + + integers = [to_i(x) for x in iterable] + + # The default mask is all ones, the mask is 32 bits. + if len(integers) == 1: + integers.append(0xffffffff) + + # We now have two integers in a list. Convert to canonical string. + return '/'.join(map(hex, integers)) + + def canonicalize(item): + k, v = item + # ip rule shows these as 'any' + if k == 'from' and v == 'all': + return k, constants.IP_ANY[ip_version] + # lookup and table are interchangeable. Use table every time. + if k == 'lookup': + return 'table', v + if k == 'fwmark': + return k, canonicalize_fwmark_string(v) + return k, v + + if 'type' not in settings: + settings['type'] = 'unicast' + + return {k: str(v) for k, v in map(canonicalize, settings.items())} + def _parse_line(self, ip_version, line): # Typical rules from 'ip rule show': # 4030201: from 1.2.3.4/24 lookup 10203040 @@ -296,23 +358,21 @@ class IpRuleCommand(IpCommandBase): if not parts: return {} - # Format of line is: "priority: ..." + # Format of line is: "priority: ... []" settings = {k: v for k, v in zip(parts[1::2], parts[2::2])} settings['priority'] = parts[0][:-1] + if len(parts) % 2 == 0: + # When line has an even number of columns, last one is the type. + settings['type'] = parts[-1] - # Canonicalize some arguments - if settings.get('from') == "all": - settings['from'] = constants.IP_ANY[ip_version] - if 'lookup' in settings: - settings['table'] = settings.pop('lookup') + return self._make_canonical(ip_version, settings) - return settings + def list_rules(self, ip_version): + lines = self._as_root([ip_version], ['show']).splitlines() + return [self._parse_line(ip_version, line) for line in lines] 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) + return kwargs in self.list_rules(ip_version) def _make__flat_args_tuple(self, *args, **kwargs): for kwargs_item in sorted(kwargs.items(), key=lambda i: i[0]): @@ -323,9 +383,10 @@ class IpRuleCommand(IpCommandBase): ip_version = get_ip_version(ip) kwargs.update({'from': ip}) + canonical_kwargs = self._make_canonical(ip_version, kwargs) - if not self._exists(ip_version, **kwargs): - args_tuple = self._make__flat_args_tuple('add', **kwargs) + if not self._exists(ip_version, **canonical_kwargs): + args_tuple = self._make__flat_args_tuple('add', **canonical_kwargs) self._as_root([ip_version], args_tuple) def delete(self, ip, **kwargs): @@ -333,7 +394,9 @@ class IpRuleCommand(IpCommandBase): # TODO(Carl) ip ignored in delete, okay in general? - args_tuple = self._make__flat_args_tuple('del', **kwargs) + canonical_kwargs = self._make_canonical(ip_version, kwargs) + + args_tuple = self._make__flat_args_tuple('del', **canonical_kwargs) self._as_root([ip_version], args_tuple) diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index e84e3563f..b49a0d0ff 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -562,7 +562,9 @@ class TestIpRuleCommand(TestIPCmdBase): self.rule_cmd.add(ip, table=table, priority=priority) self._assert_sudo([ip_version], (['show'])) self._assert_sudo([ip_version], ('add', 'from', ip, - 'priority', priority, 'table', table)) + 'priority', str(priority), + 'table', str(table), + 'type', 'unicast')) def _test_add_rule_exists(self, ip, table, priority, output): self.parent._as_root.return_value = output @@ -574,8 +576,8 @@ class TestIpRuleCommand(TestIPCmdBase): ip_version = netaddr.IPNetwork(ip).version self.rule_cmd.delete(ip, table=table, priority=priority) self._assert_sudo([ip_version], - ('del', 'priority', priority, - 'table', table)) + ('del', 'priority', str(priority), + 'table', str(table), 'type', 'unicast')) def test__parse_line(self): def test(ip_version, line, expected): @@ -585,13 +587,49 @@ class TestIpRuleCommand(TestIPCmdBase): test(4, "4030201:\tfrom 1.2.3.4/24 lookup 10203040", {'from': '1.2.3.4/24', 'table': '10203040', + 'type': 'unicast', 'priority': '4030201'}) test(6, "1024: from all iif qg-c43b1928-48 lookup noscope", {'priority': '1024', 'from': '::/0', + 'type': 'unicast', 'iif': 'qg-c43b1928-48', 'table': 'noscope'}) + def test__make_canonical_all_v4(self): + actual = self.rule_cmd._make_canonical(4, {'from': 'all'}) + self.assertEqual({'from': '0.0.0.0/0', 'type': 'unicast'}, actual) + + def test__make_canonical_all_v6(self): + actual = self.rule_cmd._make_canonical(6, {'from': 'all'}) + self.assertEqual({'from': '::/0', 'type': 'unicast'}, actual) + + def test__make_canonical_lookup(self): + actual = self.rule_cmd._make_canonical(6, {'lookup': 'table'}) + self.assertEqual({'table': 'table', 'type': 'unicast'}, actual) + + def test__make_canonical_iif(self): + actual = self.rule_cmd._make_canonical(6, {'iif': 'iface_name'}) + self.assertEqual({'iif': 'iface_name', 'type': 'unicast'}, actual) + + def test__make_canonical_fwmark(self): + actual = self.rule_cmd._make_canonical(6, {'fwmark': '0x400'}) + self.assertEqual({'fwmark': '0x400/0xffffffff', + 'type': 'unicast'}, actual) + + def test__make_canonical_fwmark_with_mask(self): + actual = self.rule_cmd._make_canonical(6, {'fwmark': '0x400/0x00ff'}) + self.assertEqual({'fwmark': '0x400/0xff', 'type': 'unicast'}, actual) + + def test__make_canonical_fwmark_integer(self): + actual = self.rule_cmd._make_canonical(6, {'fwmark': 0x400}) + self.assertEqual({'fwmark': '0x400/0xffffffff', + 'type': 'unicast'}, actual) + + def test__make_canonical_fwmark_iterable(self): + actual = self.rule_cmd._make_canonical(6, {'fwmark': (0x400, 0xffff)}) + self.assertEqual({'fwmark': '0x400/0xffff', 'type': 'unicast'}, actual) + def test_add_rule_v4(self): self._test_add_rule('192.168.45.100', 2, 100)