Merge ~netplan-developers/netplan/+git/netplan-lp:cyphermox/forward-definition into ~netplan-developers/netplan/+git/netplan-lp:master

[Mathieu Trudel-Lapierre]

Add forward declaration support -- this means that the netplan parser needs to run multiple passes over a yaml file to first catch any identifiers that were "missing" (used before they were defined), then go through the yaml file again to piece everything together and take the steps that were skipped while the missing identifiers couldn't be matched to anything.

[Tiago Stürmer Daitx (tdaitx)]

review: Approve (code review)

diff --git a/src/parse.c b/src/parse.c
index 345c8ad..cac70f9 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -50,6 +50,13 @@ GHashTable* netdefs;
  * existing definition */
 GHashTable* ids_in_file;
 
+/* List of "seen" ids not found in netdefs yet by the parser.
+ * These are removed when it exists in this list and we reach the point of
+ * creating a netdef for that id; so by the time we're done parsing the yaml
+ * document it should be empty. */
+GHashTable *missing_id;
+int missing_ids_found;
+
 /****************************************************
  * Loading and error handling
  ****************************************************/
@@ -137,6 +144,22 @@ scalar(const yaml_node_t* node)
     return (const char*) node->data.scalar.value;
 }
 
+static void
+add_missing_node(const yaml_node_t* node)
+{
+    missing_node* missing;
+
+    /* Let's capture the current netdef we were playing with along with the
+     * actual yaml_node_t that errors (that is an identifier not previously
+     * seen by the compiler). We can use it later to write an sensible error
+     * message and point the user in the right direction. */
+    missing = g_new0(missing_node, 1);
+    missing->netdef_id = cur_netdef->id;
+    missing->node = node;
+
+    g_debug("recording missing yaml_node_t %s", scalar(node));
+    g_hash_table_insert(missing_id, (gpointer)scalar(node), missing);
+}
 
 /**
  * Check that node contains a valid ID/interface name. Raise GError if not.
@@ -276,11 +299,11 @@ handle_netdef_id_ref(yaml_document_t* doc, yaml_node_t* node, const void* data, 
     net_definition* ref = NULL;
 
     ref = g_hash_table_lookup(netdefs, scalar(node));
-    if (!ref)
-        return yaml_error(node, error, "%s: interface %s is not defined",
-                          cur_netdef->id, scalar(node));
-
-    *((net_definition**) ((void*) cur_netdef + offset)) = ref;
+    if (!ref) {
+        add_missing_node(node);
+    } else {
+        *((net_definition**) ((void*) cur_netdef + offset)) = ref;
+    }
     return TRUE;
 }
 
@@ -563,14 +586,15 @@ handle_interfaces(yaml_document_t* doc, yaml_node_t* node, const void* data, GEr
 
         assert_type(entry, YAML_SCALAR_NODE);
         component = g_hash_table_lookup(netdefs, scalar(entry));
-        if (!component)
-            return yaml_error(node, error, "%s: interface %s is not defined",
-                              cur_netdef->id, scalar(entry));
-        component_ref_ptr = ((char**) ((void*) component + GPOINTER_TO_UINT(data)));
-        if (*component_ref_ptr)
-            return yaml_error(node, error, "%s: interface %s is already assigned to %s",
-                              cur_netdef->id, scalar(entry), *component_ref_ptr);
-        *component_ref_ptr = cur_netdef->id;
+        if (!component) {
+            add_missing_node(entry);
+        } else {
+            component_ref_ptr = ((char**) ((void*) component + GPOINTER_TO_UINT(data)));
+            if (*component_ref_ptr && *component_ref_ptr != cur_netdef->id)
+                return yaml_error(node, error, "%s: interface %s is already assigned to %s",
+                                  cur_netdef->id, scalar(entry), *component_ref_ptr);
+            *component_ref_ptr = cur_netdef->id;
+        }
     }
 
     return TRUE;
@@ -720,22 +744,22 @@ handle_bridge_path_cost(yaml_document_t* doc, yaml_node_t* node, const void* dat
         assert_type(value, YAML_SCALAR_NODE);
 
         component = g_hash_table_lookup(netdefs, scalar(key));
-        if (!component)
-            return yaml_error(node, error, "%s: interface %s is not defined",
-                              cur_netdef->id, scalar(key));
-
-        ref_ptr = ((guint*) ((void*) component + GPOINTER_TO_UINT(data)));
-        if (*ref_ptr)
-            return yaml_error(node, error, "%s: interface %s already has a path cost of %u",
-                              cur_netdef->id, scalar(key), *ref_ptr);
+        if (!component) {
+            add_missing_node(key);
+        } else {
+            ref_ptr = ((guint*) ((void*) component + GPOINTER_TO_UINT(data)));
+            if (*ref_ptr)
+                return yaml_error(node, error, "%s: interface %s already has a path cost of %u",
+                                  cur_netdef->id, scalar(key), *ref_ptr);
 
-        v = g_ascii_strtoull(scalar(value), &endptr, 10);
-        if (*endptr != '\0' || v > G_MAXUINT)
-            return yaml_error(node, error, "invalid unsigned int value %s", scalar(value));
+            v = g_ascii_strtoull(scalar(value), &endptr, 10);
+            if (*endptr != '\0' || v > G_MAXUINT)
+                return yaml_error(node, error, "invalid unsigned int value %s", scalar(value));
 
-        g_debug("%s: adding path '%s' of cost: %d", cur_netdef->id, scalar(key), v);
+            g_debug("%s: adding path '%s' of cost: %d", cur_netdef->id, scalar(key), v);
 
-        *ref_ptr = v;
+            *ref_ptr = v;
+        }
     }
     return TRUE;
 }
@@ -959,8 +983,15 @@ handle_network_renderer(yaml_document_t* doc, yaml_node_t* node, const void* _, 
 static gboolean
 validate_netdef(net_definition* nd, yaml_node_t* node, GError** error)
 {
+    int missing_id_count = g_hash_table_size(missing_id);
     g_assert(nd->type != ND_NONE);
 
+    /* Skip all validation if we're missing some definition IDs (devices).
+     * The ones we have yet to see may be necessary for validation to succeed,
+     * we can complete it on the next parser pass. */
+    if (missing_id_count > 0)
+        return TRUE;
+
     /* set-name: requires match: */
     if (nd->set_name && !nd->has_match)
         return yaml_error(node, error, "%s: set-name: requires match: properties", nd->id);
@@ -1010,6 +1041,12 @@ handle_network_type(yaml_document_t* doc, yaml_node_t* node, const void* data, G
 
         assert_type(value, YAML_MAPPING_NODE);
 
+        /* At this point we've seen a new starting definition, if it has been
+         * already mentioned in another netdef, removing it from our "missing"
+         * list. */
+        if(g_hash_table_remove(missing_id, scalar(key)))
+            missing_ids_found++;
+
         cur_netdef = g_hash_table_lookup(netdefs, scalar(key));
         if (cur_netdef) {
             /* already exists, overriding/amending previous definition */
@@ -1026,8 +1063,9 @@ handle_network_type(yaml_document_t* doc, yaml_node_t* node, const void* data, G
             g_hash_table_insert(netdefs, cur_netdef->id, cur_netdef);
         }
 
-        if (!g_hash_table_add(ids_in_file, cur_netdef->id))
-            return yaml_error(key, error, "Duplicate net definition ID '%s'", cur_netdef->id);
+        // XXX: breaks multi-pass parsing.
+        //if (!g_hash_table_add(ids_in_file, cur_netdef->id))
+        //    return yaml_error(key, error, "Duplicate net definition ID '%s'", cur_netdef->id);
 
         /* and fill it with definitions */
         switch (cur_netdef->type) {
@@ -1074,7 +1112,55 @@ const mapping_entry_handler root_handlers[] = {
     {NULL}
 };
 
+/**
+ * Handle multiple-pass parsing of the yaml document.
+ */
+static gboolean
+process_document(yaml_document_t* doc, GError** error)
+{
+    gboolean ret;
+    int previously_found;
+    int still_missing;
+
+    g_assert(missing_id == NULL);
+    missing_id = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, g_free);
+
+    do {
+        g_debug("starting new processing pass");
+
+        previously_found = missing_ids_found;
+        missing_ids_found = 0;
+
+        ret = process_mapping(doc, yaml_document_get_root_node(doc), root_handlers, error);
+
+        still_missing = g_hash_table_size(missing_id);
+
+        if (still_missing > 0 && missing_ids_found == previously_found)
+            break;
+    } while (still_missing > 0 || missing_ids_found > 0);
 
+    if (g_hash_table_size(missing_id) > 0) {
+        GHashTableIter iter;
+        gpointer key, value;
+        missing_node *missing;
+
+        g_clear_error(error);
+
+        /* Get the first missing identifier we can get from our list, to
+         * approximate early failure and give the user a meaningful error. */
+        g_hash_table_iter_init (&iter, missing_id);
+        g_hash_table_iter_next (&iter, &key, &value);
+        missing = (missing_node*) value;
+
+        return yaml_error(missing->node, error, "%s: interface %s is not defined",
+                          missing->netdef_id,
+                          key);
+    }
+
+    g_hash_table_destroy(missing_id);
+    missing_id = NULL;
+    return ret;
+}
 
 /**
  * Parse given YAML file and create/update global "netdefs" list.
@@ -1098,7 +1184,8 @@ parse_yaml(const char* filename, GError** error)
     g_assert(ids_in_file == NULL);
     ids_in_file = g_hash_table_new(g_str_hash, NULL);
 
-    ret = process_mapping(&doc, yaml_document_get_root_node(&doc), root_handlers, error);
+    ret = process_document(&doc, error);
+
     cur_netdef = NULL;
     yaml_document_delete(&doc);
     g_hash_table_destroy(ids_in_file);
diff --git a/src/parse.h b/src/parse.h
index 75d6531..f2e9c81 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -18,6 +18,7 @@
 #pragma once
 
 #include <uuid.h>
+#include <yaml.h>
 
 /****************************************************
  * Parsed definitions
@@ -41,6 +42,11 @@ typedef enum {
     BACKEND_NM,
 } netdef_backend;
 
+typedef struct missing_node {
+    char* netdef_id;
+    yaml_node_t* node;
+} missing_node;
+
 /**
  * Represent a configuration stanza
  */
diff --git a/tests/generate.py b/tests/generate.py
index e168283..179b4de 100755
--- a/tests/generate.py
+++ b/tests/generate.py
@@ -777,6 +777,27 @@ Address=1.2.3.4/12
 unmanaged-devices+=interface-name:br0,''')
         self.assert_udev(None)
 
+    def test_bridge_forward_declaration(self):
+        self.generate('''network:
+  version: 2
+  bridges:
+    br0:
+      interfaces: [eno1, switchports]
+      dhcp4: true
+  ethernets:
+    eno1: {}
+    switchports:
+      match:
+        driver: yayroute
+''')
+
+        self.assert_networkd({'br0.netdev': '[NetDev]\nName=br0\nKind=bridge\n',
+                              'br0.network': ND_DHCP4 % 'br0',
+                              'eno1.network': '[Match]\nName=eno1\n\n'
+                                              '[Network]\nBridge=br0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n',
+                              'switchports.network': '[Match]\nDriver=yayroute\n\n'
+                                                     '[Network]\nBridge=br0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n'})
+
     def test_bridge_components(self):
         self.generate('''network:
   version: 2
@@ -1836,6 +1857,58 @@ address1=1.2.3.4/12
         self.assert_networkd({})
         self.assert_udev(None)
 
+    def test_bridge_forward_declaration(self):
+        self.generate('''network:
+  version: 2
+  renderer: NetworkManager
+  bridges:
+    br0:
+      interfaces: [eno1, switchport]
+      dhcp4: true
+  ethernets:
+    eno1: {}
+    switchport:
+      match:
+        name: enp2s1
+''')
+
+        self.assert_nm({'eno1': '''[connection]
+id=netplan-eno1
+type=ethernet
+interface-name=eno1
+slave-type=bridge
+master=br0
+
+[ethernet]
+wake-on-lan=0
+
+[ipv4]
+method=link-local
+''',
+                        'switchport': '''[connection]
+id=netplan-switchport
+type=ethernet
+interface-name=enp2s1
+slave-type=bridge
+master=br0
+
+[ethernet]
+wake-on-lan=0
+
+[ipv4]
+method=link-local
+''',
+                        'br0': '''[connection]
+id=netplan-br0
+type=bridge
+interface-name=br0
+
+[ipv4]
+method=auto
+'''})
+        self.assert_networkd({})
+        self.assert_udev(None)
+
     def test_bridge_components(self):
         self.generate('''network:
   version: 2
@@ -2384,17 +2457,6 @@ class TestConfigErrors(TestBase):
         err = self.generate('network:\n  version: 1', expect_fail=True)
         self.assertIn('/a.yaml line 1 column 11: Only version 2 is supported', err)
 
-    def test_duplicate_id(self):
-        err = self.generate('''network:
-  version: 2
-  ethernets:
-    id0:
-      wakeonlan: true
-    id0:
-      wakeonlan: true
-''', expect_fail=True)
-        self.assertIn("Duplicate net definition ID 'id0'", err)
-
     def test_id_redef_type_mismatch(self):
         err = self.generate('''network:
   version: 2
@@ -2448,7 +2510,7 @@ class TestConfigErrors(TestBase):
   bridges:
     br0:
       interfaces: ['foo']''', expect_fail=True)
-        self.assertIn('/a.yaml line 4 column 18: br0: interface foo is not defined\n', err)
+        self.assertIn('/a.yaml line 4 column 19: br0: interface foo is not defined\n', err)
 
     def test_bridge_multiple_assignments(self):
         err = self.generate('''network:
@@ -2717,7 +2779,7 @@ class TestConfigErrors(TestBase):
   version: 2
   vlans:
     ena: {id: 1, link: en1}''', expect_fail=True)
-        self.assertIn('interface en1 is not defined\n', err)
+        self.assertIn('ena: interface en1 is not defined\n', err)
 
     def test_device_bad_route_to(self):
         self.generate('''network:
@@ -2870,6 +2932,105 @@ class TestConfigErrors(TestBase):
       dhcp4: true''', expect_fail=True)
 
 
+class TestForwardDeclaration(TestBase):
+
+    def test_fwdecl_bridge_on_bond(self):
+        self.generate('''network:
+  version: 2
+  bridges:
+    br0:
+      interfaces: ['bond0']
+      dhcp4: true
+  bonds:
+    bond0:
+      interfaces: ['eth0', 'eth1']
+  ethernets:
+    eth0:
+      match:
+        macaddress: 00:01:02:03:04:05
+      set-name: eth0
+    eth1:
+      match:
+        macaddress: 02:01:02:03:04:05
+      set-name: eth1
+''')
+
+        self.assert_networkd({'br0.netdev': '[NetDev]\nName=br0\nKind=bridge\n',
+                              'br0.network': ND_DHCP4 % 'br0',
+                              'bond0.netdev': '[NetDev]\nName=bond0\nKind=bond\n',
+                              'bond0.network': '[Match]\nName=bond0\n\n'
+                                               '[Network]\nBridge=br0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n',
+                              'eth0.link': '[Match]\nMACAddress=00:01:02:03:04:05\n\n'
+                                           '[Link]\nName=eth0\nWakeOnLan=off\n',
+                              'eth0.network': '[Match]\nMACAddress=00:01:02:03:04:05\nName=eth0\n\n'
+                                              '[Network]\nBond=bond0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n',
+                              'eth1.link': '[Match]\nMACAddress=02:01:02:03:04:05\n\n'
+                                           '[Link]\nName=eth1\nWakeOnLan=off\n',
+                              'eth1.network': '[Match]\nMACAddress=02:01:02:03:04:05\nName=eth1\n\n'
+                                              '[Network]\nBond=bond0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n'})
+
+    def test_fwdecl_feature_blend(self):
+        self.generate('''network:
+  version: 2
+  vlans:
+    vlan1:
+      link: 'br0'
+      id: 1
+      dhcp4: true
+  bridges:
+    br0:
+      interfaces: ['bond0', 'eth2']
+      parameters:
+        path-cost:
+          eth2: 1000
+          bond0: 8888
+  bonds:
+    bond0:
+      interfaces: ['eth0', 'br1']
+  ethernets:
+    eth0:
+      match:
+        macaddress: 00:01:02:03:04:05
+      set-name: eth0
+  bridges:
+    br1:
+      interfaces: ['eth1']
+  ethernets:
+    eth1:
+      match:
+        macaddress: 02:01:02:03:04:05
+      set-name: eth1
+    eth2:
+      match:
+        name: eth2
+''')
+
+        self.assert_networkd({'vlan1.netdev': '[NetDev]\nName=vlan1\nKind=vlan\n\n'
+                                              '[VLAN]\nId=1\n',
+                              'vlan1.network': ND_DHCP4 % 'vlan1',
+                              'br0.netdev': '[NetDev]\nName=br0\nKind=bridge\n',
+                              'br0.network': '[Match]\nName=br0\n\n'
+                                             '[Network]\nVLAN=vlan1\n',
+                              'bond0.netdev': '[NetDev]\nName=bond0\nKind=bond\n',
+                              'bond0.network': '[Match]\nName=bond0\n\n'
+                                               '[Network]\nBridge=br0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n\n'
+                                               '[Bridge]\nCost=8888\n',
+                              'eth2.network': '[Match]\nName=eth2\n\n'
+                                              '[Network]\nBridge=br0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n\n'
+                                              '[Bridge]\nCost=1000\n',
+                              'br1.netdev': '[NetDev]\nName=br1\nKind=bridge\n',
+                              'br1.network': '[Match]\nName=br1\n\n'
+                                             '[Network]\nBond=bond0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n',
+                              'eth0.link': '[Match]\nMACAddress=00:01:02:03:04:05\n\n'
+                                           '[Link]\nName=eth0\nWakeOnLan=off\n',
+                              'eth0.network': '[Match]\nMACAddress=00:01:02:03:04:05\nName=eth0\n\n'
+                                              '[Network]\nBond=bond0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n',
+                              'eth1.link': '[Match]\nMACAddress=02:01:02:03:04:05\n\n'
+                                           '[Link]\nName=eth1\nWakeOnLan=off\n',
+                              'eth1.network': '[Match]\nMACAddress=02:01:02:03:04:05\nName=eth1\n\n'
+                                              '[Network]\nBridge=br1\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n'})
+
+
 class TestMerging(TestBase):
     '''multiple *.yaml merging'''
 
diff --git a/tests/integration.py b/tests/integration.py
index b1bfbcc..6fe97f6 100755
--- a/tests/integration.py
+++ b/tests/integration.py
@@ -1147,6 +1147,99 @@ wpa_passphrase=12345678
                                           universal_newlines=True)
             self.assertRegex(out, 'DNS.*192.168.5.1')
 
+    def test_mix_bridge_on_bond(self):
+        self.setup_eth(None)
+        self.addCleanup(subprocess.call, ['ip', 'link', 'delete', 'bond0'], stderr=subprocess.DEVNULL)
+        self.addCleanup(subprocess.call, ['ip', 'link', 'delete', 'br0'], stderr=subprocess.DEVNULL)
+        self.addCleanup(subprocess.call, ['ip', 'link', 'delete', 'br1'], stderr=subprocess.DEVNULL)
+        with open(self.config, 'w') as f:
+            f.write('''network:
+  renderer: %(r)s
+  bridges:
+    br0:
+      interfaces: [bond0]
+      dhcp4: true
+  bonds:
+    bond0:
+      interfaces: [ethbn, ethb2]
+      parameters:
+        mii-monitor-interval: 100000
+  ethernets:
+    ethbn:
+      match: {name: %(ec)s}
+    ethb2:
+      match: {name: %(e2c)s}
+''' % {'r': self.backend, 'ec': self.dev_e_client, 'e2c': self.dev_e2_client})
+        self.generate_and_settle()
+        self.assert_iface_up(self.dev_e_client,
+                             ['master bond0'],
+                             ['inet '])
+        self.assert_iface_up(self.dev_e2_client,
+                             ['master bond0'],
+                             ['inet '])
+        self.assert_iface_up('bond0',
+                             ['master br0'])
+        self.assert_iface_up('br0',
+                             ['inet 192.168'])
+        with open('/sys/class/net/bond0/bonding/slaves') as f:
+            result = f.read().strip()
+            self.assertIn(self.dev_e_client, result)
+            self.assertIn(self.dev_e2_client, result)
+
+    def test_mix_vlan_on_bridge_on_bond(self):
+        self.setup_eth(None)
+        self.addCleanup(subprocess.call, ['ip', 'link', 'delete', 'bond0'], stderr=subprocess.DEVNULL)
+        self.addCleanup(subprocess.call, ['ip', 'link', 'delete', 'br0'], stderr=subprocess.DEVNULL)
+        self.addCleanup(subprocess.call, ['ip', 'link', 'delete', 'br1'], stderr=subprocess.DEVNULL)
+        with open(self.config, 'w') as f:
+            f.write('''network:
+  renderer: %(r)s
+  version: 2
+  vlans:
+    vlan1:
+      link: 'br0'
+      id: 1
+      addresses: [ '10.10.10.1/24' ]
+  bridges:
+    br0:
+      interfaces: ['bond0', 'vlan2']
+      parameters:
+        path-cost:
+          bond0: 1000
+          vlan2: 2000
+  bonds:
+    bond0:
+      interfaces: ['br1']
+      parameters:
+        mii-monitor-interval: 100000
+  bridges:
+    br1:
+      interfaces: ['ethb2']
+  vlans:
+    vlan2:
+      link: ethbn
+      id: 2
+  ethernets:
+    ethbn:
+      match: {name: %(ec)s}
+    ethb2:
+      match: {name: %(e2c)s}
+''' % {'r': self.backend, 'ec': self.dev_e_client, 'e2c': self.dev_e2_client})
+        self.generate_and_settle()
+        self.assert_iface_up('vlan1', ['vlan1@br0'])
+        self.assert_iface_up('vlan2',
+                             ['vlan2@' + self.dev_e_client, 'master br0'])
+        self.assert_iface_up(self.dev_e2_client,
+                             ['master br1'],
+                             ['inet '])
+        self.assert_iface_up('br1',
+                             ['master bond0'])
+        self.assert_iface_up('bond0',
+                             ['master br0'])
+        with open('/sys/class/net/bond0/bonding/slaves') as f:
+            result = f.read().strip()
+            self.assertIn('br1', result)
+
 
 class TestNetworkd(NetworkTestBase, _CommonTests):
     backend = 'networkd'