parser: check for route duplicates (LP: #2003061)

src/parse.c

@@ -1927,14 +1927,6 @@ handle_routes(NetplanParser* npp, yaml_node_t* node, const void* _, GError** err
     if (!npp->current.netdef->routes)
         npp->current.netdef->routes = g_array_new(FALSE, TRUE, sizeof(NetplanIPRoute*));
 
-    /* Avoid adding the same routes in a 2nd parsing pass by comparing
-     * the array size to the YAML sequence size. Skip if they are equal. */
-    guint item_count = node->data.sequence.items.top - node->data.sequence.items.start;
-    if (npp->current.netdef->routes->len == item_count) {
-        g_debug("%s: all routes have already been added", npp->current.netdef->id);
-        return TRUE;
-    }
-
     for (yaml_node_item_t *i = node->data.sequence.items.start; i < node->data.sequence.items.top; i++) {
         yaml_node_t *entry = yaml_document_get_node(&npp->doc, *i);
         NetplanIPRoute* route;
@@ -1985,6 +1977,18 @@ handle_routes(NetplanParser* npp, yaml_node_t* node, const void* _, GError** err
             goto err;
         }
 
+        if (is_route_present(npp->current.netdef, route)) {
+            g_debug("%s: route (to: %s, via: %s, table: %d, metric: %d) has already been added",
+                    npp->current.netdef->id,
+                    route->to,
+                    route->via,
+                    route->table,
+                    route->metric);
+            route_clear(&npp->current.route);
+            npp->current.route = NULL;
+            continue;
+        }
+
         g_array_append_val(npp->current.netdef->routes, route);
         npp->current.route = NULL;
     }

src/util-internal.h

@@ -124,3 +124,6 @@ _netplan_netdef_get_vlan_id(const NetplanNetDefinition* netdef);
 
 NETPLAN_INTERNAL gboolean
 _netplan_netdef_is_trivial_compound_itf(const NetplanNetDefinition* netdef);
+
+NETPLAN_INTERNAL gboolean
+is_route_present(const NetplanNetDefinition* netdef, const NetplanIPRoute* route);

src/util.c

@@ -871,3 +871,46 @@ netplan_state_iterator_has_next(const NetplanStateIterator* iter)
         return FALSE;
     return _iter->next != NULL;
 }
+
+static const char*
+normalize_ip_address(const char* addr, const guint family)
+{
+    if (!g_strcmp0(addr, "default")) {
+        if (family == AF_INET)
+            return "0.0.0.0/0";
+        else
+            return "::/0";
+    }
+
+    return addr;
+}
+/*
+ * Returns true if a route already exists in the netdef routes list.
+ *
+ * We consider a route a duplicate if it is in the same table, has the same metric,
+ * src, to, via and family values.
+ *
+ * XXX: in the future we could add a route "key" to a hash set so this verification could
+ * be done faster.
+ */
+gboolean
+is_route_present(const NetplanNetDefinition* netdef, const NetplanIPRoute* route)
+{
+    const GArray* routes = netdef->routes;
+
+    for (int i = 0; i < routes->len; i++) {
+        const NetplanIPRoute* entry = g_array_index(routes, NetplanIPRoute*, i);
+        if (
+                entry->family == route->family &&
+                entry->table == route->table &&
+                entry->metric == route->metric &&
+                g_strcmp0(entry->from, route->from) == 0 &&
+                g_strcmp0(normalize_ip_address(entry->to, entry->family),
+                    normalize_ip_address(route->to, route->family)) == 0 &&
+                g_strcmp0(entry->via, route->via) == 0
+           )
+            return TRUE;
+    }
+
+    return FALSE;
+}

tests/ctests/test_netplan_misc.c

@@ -178,6 +178,114 @@ test_netplan_netdef_get_output_filename_invalid_backend(void** state)
     assert_int_equal(ret, 0);
 }
 
+void
+test_util_is_route_present(void** state)
+{
+    const char* yaml =
+        "network:\n"
+        "  version: 2\n"
+        "  ethernets:\n"
+        "    eth0:\n"
+        "      routing-policy:\n"
+        "        - from: 10.0.0.1\n"
+        "          table: 1001\n"
+        "        - from: 10.0.0.2\n"
+        "          table: 1002\n"
+        "      routes:\n"
+        "        - to: 0.0.0.0/0\n"
+        "          via: 10.0.0.200\n"
+        "          table: 1002\n"
+        "        - to: 0.0.0.0/0\n"
+        "          via: 10.0.0.200\n"
+        "          table: 1001\n"
+        "        - to: 192.168.0.0/24\n"
+        "          via: 10.20.30.40\n"
+        "        - to: 192.168.0.0/24\n"
+        "          scope: link\n"
+        "        - to: default\n"
+        "          via: abcd::1\n";
+
+    NetplanState* np_state = load_string_to_netplan_state(yaml);
+    NetplanStateIterator iter;
+    NetplanNetDefinition* netdef = NULL;
+    netplan_state_iterator_init(np_state, &iter);
+
+    netdef = netplan_state_iterator_next(&iter);
+
+    NetplanIPRoute* route = g_new0(NetplanIPRoute, 1);
+    route->family = AF_INET;
+    route->metric = NETPLAN_METRIC_UNSPEC;
+    route->table = 1001;
+    route->to = "0.0.0.0/0";
+    route->via = "10.0.0.200";
+    route->from = NULL;
+
+    assert_true(is_route_present(netdef, route));
+
+    route->table = 1002;
+    route->to = "0.0.0.0/0";
+    route->via = "10.0.0.200";
+    route->from = NULL;
+
+    assert_true(is_route_present(netdef, route));
+
+    route->table = NETPLAN_ROUTE_TABLE_UNSPEC;
+    route->to = "192.168.0.0/24";
+    route->via = "10.20.30.40";
+    route->from = NULL;
+
+    assert_true(is_route_present(netdef, route));
+
+    route->table = 1002;
+    route->to = "0.0.0.0/0";
+    route->via = "10.0.0.100";
+    route->from = NULL;
+
+    assert_false(is_route_present(netdef, route));
+
+    route->table = 1003;
+    route->to = "0.0.0.0/0";
+    route->via = "10.0.0.200";
+    route->from = NULL;
+
+    assert_false(is_route_present(netdef, route));
+
+    route->table = 1001;
+    route->to = "default";
+    route->via = "10.0.0.200";
+    route->from = NULL;
+
+    assert_true(is_route_present(netdef, route));
+
+    route->table = NETPLAN_ROUTE_TABLE_UNSPEC;
+    route->family = AF_INET6;
+    route->to = "::/0";
+    route->via = "abcd::1";
+    route->from = NULL;
+
+    assert_true(is_route_present(netdef, route));
+
+    route->table = NETPLAN_ROUTE_TABLE_UNSPEC;
+    route->family = AF_INET;
+    route->to = "192.168.0.0/24";
+    route->via = NULL;
+    route->from = NULL;
+    route->scope = "link";
+
+    assert_true(is_route_present(netdef, route));
+
+    g_free(route);
+    netplan_state_clear(&np_state);
+}
+
+void
+test_normalize_ip_address(void** state)
+{
+    assert_string_equal(normalize_ip_address("default", AF_INET), "0.0.0.0/0");
+    assert_string_equal(normalize_ip_address("default", AF_INET6), "::/0");
+    assert_string_equal(normalize_ip_address("0.0.0.0/0", AF_INET), "0.0.0.0/0");
+}
+
 int
 setup(void** state)
 {
@@ -206,6 +314,8 @@ main()
            cmocka_unit_test(test_netplan_netdef_get_output_filename_networkd),
            cmocka_unit_test(test_netplan_netdef_get_output_filename_buffer_is_too_small),
            cmocka_unit_test(test_netplan_netdef_get_output_filename_invalid_backend),
+           cmocka_unit_test(test_util_is_route_present),
+           cmocka_unit_test(test_normalize_ip_address),
        };
 
        return cmocka_run_group_tests(tests, setup, tear_down);

tests/generator/test_routing.py

@@ -16,7 +16,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from .base import TestBase
+from .base import TestBase, ND_VLAN, ND_DHCP4, ND_EMPTY
 
 
 class TestNetworkd(TestBase):
@@ -1423,4 +1423,118 @@ def test_default_metric_v6(self):
 method=auto
 ip6-privacy=0
 route-metric=5050
+'''})
+
+    def test_add_routes_to_different_tables_from_multiple_files(self):
+        """Test case for bug LP#2003061"""
+
+        self.generate('''network:
+  version: 2''',
+                      confs={'01-netcfg': '''network:
+  version: 2
+  ethernets:
+    eth0:
+      dhcp4: true
+  vlans:
+    vlan100:
+      id: 100
+      link: eth0''',
+                             '10-table1': '''network:
+  version: 2
+  vlans:
+    vlan100:
+      routing-policy:
+        - from: 10.0.0.1
+          table: 1001
+      routes:
+        - to: 0.0.0.0/0
+          via: 10.0.0.100
+          table: 1001''',
+                             '10-table2': '''network:
+  version: 2
+  vlans:
+    vlan100:
+      routing-policy:
+        - from: 10.0.0.2
+          table: 1002
+      routes:
+        - to: 0.0.0.0/0
+          via: 10.0.0.200
+          table: 1002'''})
+
+        self.assert_networkd({'eth0.network': (ND_DHCP4 % 'eth0').replace('\n[DHCP]', 'VLAN=vlan100\n\n[DHCP]'),
+                              'vlan100.netdev': ND_VLAN % ('vlan100', 100),
+                              'vlan100.network': ND_EMPTY % ('vlan100', 'ipv6') + '''
+[Route]
+Destination=0.0.0.0/0
+Gateway=10.0.0.100
+Table=1001
+
+[Route]
+Destination=0.0.0.0/0
+Gateway=10.0.0.200
+Table=1002
+
+[RoutingPolicyRule]
+From=10.0.0.1
+Table=1001
+
+[RoutingPolicyRule]
+From=10.0.0.2
+Table=1002
+'''})
+
+    def test_add_duplicate_routes_from_multiple_files(self):
+        """ Duplicate route should produce a single entry in the
+        backend configuration"""
+
+        self.generate('''network:
+  version: 2''',
+                      confs={'01-netcfg': '''network:
+  version: 2
+  ethernets:
+    eth0:
+      dhcp4: true
+  vlans:
+    vlan100:
+      id: 100
+      link: eth0''',
+                             '10-table1': '''network:
+  version: 2
+  vlans:
+    vlan100:
+      routing-policy:
+        - from: 10.0.0.1
+          table: 1001
+      routes:
+        - to: 0.0.0.0/0
+          via: 10.0.0.100
+          table: 1001''',
+                             '10-table2': '''network:
+  version: 2
+  vlans:
+    vlan100:
+      routing-policy:
+        - from: 10.0.0.2
+          table: 1002
+      routes:
+        - to: 0.0.0.0/0
+          via: 10.0.0.100
+          table: 1001'''})
+
+        self.assert_networkd({'eth0.network': (ND_DHCP4 % 'eth0').replace('\n[DHCP]', 'VLAN=vlan100\n\n[DHCP]'),
+                              'vlan100.netdev': ND_VLAN % ('vlan100', 100),
+                              'vlan100.network': ND_EMPTY % ('vlan100', 'ipv6') + '''
+[Route]
+Destination=0.0.0.0/0
+Gateway=10.0.0.100
+Table=1001
+
+[RoutingPolicyRule]
+From=10.0.0.1
+Table=1001
+
+[RoutingPolicyRule]
+From=10.0.0.2
+Table=1002
 '''})