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 dedf8d5b6e380b6c1a8798cb075272eedf497d71..961d55791475748c55233046ea25ed931da50282 100644 (file)
--- 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: <key> <value> ..."
+        # Format of line is: "priority: <key> <value> ... [<type>]"
         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 e84e3563f72802db1197c09b4bc92f7fb9426e1b..b49a0d0ff5bf9674309ff605a6b879429a0ac40d 100644 (file)
--- 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)