Merge pull request #1195 from hensur/current-ipv6-local-route

policy: T4151: bugfix multiple commits and smoketest

2 files changed, 82 insertions, 51 deletions

diff --git a/smoketest/scripts/cli/test_policy.py b/smoketest/scripts/cli/test_policy.py
index d055762f4..73d93c986 100755
--- a/smoketest/scripts/cli/test_policy.py
+++ b/smoketest/scripts/cli/test_policy.py
@@ -1135,9 +1135,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         50:	from 203.0.113.1 lookup 23
         50:	from 203.0.113.2 lookup 23
@@ -1159,9 +1156,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         101:    from all fwmark 0x18 lookup 154
         """
@@ -1182,9 +1176,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         102:    from all to 203.0.113.1 lookup 154
         """
@@ -1207,9 +1198,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         100:	from 203.0.113.11 fwmark 0x17 lookup 150
         100:	from 203.0.113.12 fwmark 0x17 lookup 150
@@ -1236,9 +1224,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         103:	from 203.0.113.11 to 203.0.113.13 fwmark 0x17 lookup 150
         103:	from 203.0.113.11 to 203.0.113.15 fwmark 0x17 lookup 150
@@ -1262,9 +1247,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         50:	from 2001:db8:123::/48 lookup 23
         50:	from 2001:db8:126::/48 lookup 23
@@ -1286,9 +1268,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         100:    from all fwmark 0x18 lookup 154
         """
@@ -1309,9 +1288,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         101:    from all to 2001:db8:1337::/126 lookup 154
         """
@@ -1334,9 +1310,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         102:	from 2001:db8:1338::/126 fwmark 0x17 lookup 150
         102:	from 2001:db8:1339::/126 fwmark 0x17 lookup 150
@@ -1363,9 +1336,6 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
         103:	from 2001:db8:1338::/126 to 2001:db8:13::/48 fwmark 0x17 lookup 150
         103:	from 2001:db8:1338::/126 to 2001:db8:16::/48 fwmark 0x17 lookup 150
@@ -1404,20 +1374,17 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
 
         self.cli_commit()
 
-        # Check generated configuration
-
-        # Expected values
         original = """
-        103:	from 203.0.113.1/24 to 203.0.112.1/24 fwmark 0x17 lookup 150
-        103:	from 203.0.113.1/24 to 203.0.116.5 fwmark 0x17 lookup 150
-        103:	from 203.0.114.5 to 203.0.112.1/24 fwmark 0x17 lookup 150
+        103:	from 203.0.113.0/24 to 203.0.116.5 fwmark 0x17 lookup 150
+        103:	from 203.0.114.5 to 203.0.112.0/24 fwmark 0x17 lookup 150
         103:	from 203.0.114.5 to 203.0.116.5 fwmark 0x17 lookup 150
+        103:	from 203.0.113.0/24 to 203.0.112.0/24 fwmark 0x17 lookup 150
         """
         original_v6 = """
-        103:	from 20016 to 2001:db8:13::/48 fwmark 0x17 lookup 150
         103:	from 2001:db8:1338::/126 to 2001:db8:16::/48 fwmark 0x17 lookup 150
         103:	from 2001:db8:1339::/56 to 2001:db8:13::/48 fwmark 0x17 lookup 150
         103:	from 2001:db8:1339::/56 to 2001:db8:16::/48 fwmark 0x17 lookup 150
+        103:    from 2001:db8:1338::/126 to 2001:db8:13::/48 fwmark 0x17 lookup 150
         """
         tmp = cmd('ip rule show prio 103')
         tmp_v6 = cmd('ip -6 rule show prio 103')
@@ -1432,14 +1399,49 @@ class TestPolicy(VyOSUnitTestSHIM.TestCase):
         tmp = cmd('ip rule show prio 103')
         tmp_v6 = cmd('ip -6 rule show prio 103')
 
-        original = None
-        original_v6 = None
+        self.assertEqual(sort_ip(tmp), [])
+        self.assertEqual(sort_ip(tmp_v6), [])
+
+    # Test multiple commits ipv4
+    def test_multiple_commit_ipv4_table_id(self):
+        path = base_path + ['local-route']
+
+        sources = ['192.0.2.1', '192.0.2.2']
+        destination = '203.0.113.25'
+        rule = '105'
+        table = '151'
+        self.cli_set(path + ['rule', rule, 'set', 'table', table])
+        for src in sources:
+            self.cli_set(path + ['rule', rule, 'source', src])
+
+        self.cli_commit()
+
+        original_first = """
+        105:	from 192.0.2.1 lookup 151
+        105:	from 192.0.2.2 lookup 151
+        """
+        tmp = cmd('ip rule show prio 105')
+
+        self.assertEqual(sort_ip(tmp), sort_ip(original_first))
+
+        # Create second commit with added destination
+        self.cli_set(path + ['rule', rule, 'destination', destination])
+        self.cli_commit()
+
+        original_second = """
+        105:	from 192.0.2.1 to 203.0.113.25 lookup 151
+        105:	from 192.0.2.2 to 203.0.113.25 lookup 151
+        """
+        tmp = cmd('ip rule show prio 105')
+
+        self.assertEqual(sort_ip(tmp), sort_ip(original_second))
 
-        self.assertEqual(sort_ip(tmp), original)
-        self.assertEqual(sort_ip(tmp_v6), original_v6)
 
 def sort_ip(output):
-    return output.splitlines().sort()
+    o = '\n'.join([' '.join(line.strip().split()) for line in output.strip().splitlines()])
+    o = o.splitlines()
+    o.sort()
+    return o
 
 if __name__ == '__main__':
     unittest.main(verbosity=2)
diff --git a/src/conf_mode/policy-local-route.py b/src/conf_mode/policy-local-route.py
index 6dabb37ae..71183c6ba 100755
--- a/src/conf_mode/policy-local-route.py
+++ b/src/conf_mode/policy-local-route.py
@@ -69,20 +69,47 @@ def get_config(config=None):
         # delete policy local-route rule x fwmark x
         # delete policy local-route rule x destination x.x.x.x
         if 'rule' in pbr[route]:
-            for rule in pbr[route]['rule']:
+            for rule, rule_config in pbr[route]['rule'].items():
                 src = leaf_node_changed(conf, base_rule + [rule, 'source'])
                 fwmk = leaf_node_changed(conf, base_rule + [rule, 'fwmark'])
                 dst = leaf_node_changed(conf, base_rule + [rule, 'destination'])
+                # keep track of changes in configuration
+                # otherwise we might remove an existing node although nothing else has changed
+                changed = False
 
                 rule_def = {}
-                if src:
-                    rule_def = dict_merge({'source' : src}, rule_def)
-                if fwmk:
-                    rule_def = dict_merge({'fwmark' : fwmk}, rule_def)
-                if dst:
-                    rule_def = dict_merge({'destination' : dst}, rule_def)
-                dict = dict_merge({dict_id : {rule : rule_def}}, dict)
-                pbr.update(dict)
+                # src is None if there are no changes to src
+                if src is None:
+                    # if src hasn't changed, include it in the removal selector
+                    # if a new selector is added, we have to remove all previous rules without this selector
+                    # to make sure we remove all previous rules with this source(s), it will be included
+                    if 'source' in rule_config:
+                        rule_def = dict_merge({'source': rule_config['source']}, rule_def)
+                else:
+                    # if src is not None, it's previous content will be returned
+                    # this can be an empty array if it's just being set, or the previous value
+                    # either way, something has to be changed and we only want to remove previous values
+                    changed = True
+                    # set the old value for removal if it's not empty
+                    if len(src) > 0:
+                        rule_def = dict_merge({'source' : src}, rule_def)
+                if fwmk is None:
+                    if 'fwmark' in rule_config:
+                        rule_def = dict_merge({'fwmark': rule_config['fwmark']}, rule_def)
+                else:
+                    changed = True
+                    if len(fwmk) > 0:
+                        rule_def = dict_merge({'fwmark' : fwmk}, rule_def)
+                if dst is None:
+                    if 'destination' in rule_config:
+                        rule_def = dict_merge({'destination': rule_config['destination']}, rule_def)
+                else:
+                    changed = True
+                    if len(dst) > 0:
+                        rule_def = dict_merge({'destination' : dst}, rule_def)
+                if changed:
+                    dict = dict_merge({dict_id : {rule : rule_def}}, dict)
+                    pbr.update(dict)
 
     return pbr
 
@@ -116,6 +143,8 @@ def apply(pbr):
     if not pbr:
         return None
 
+    print(pbr)
+
     # Delete old rule if needed
     for rule_rm in ['rule_remove', 'rule6_remove']:
         if rule_rm in pbr: