Merge ~danilogondolfo/netplan:noble_lp1809994 into ~ubuntu-core-dev/netplan/+git/ubuntu:ubuntu-noble

[Danilo Egea Gondolfo]

[Danilo Egea Gondolfo (danilogondolfo)]

PPA: https://launchpad.net/~danilogondolfo/+archive/ubuntu/netplan

Autopkgtests:

https://autopkgtest.ubuntu.com/results/autopkgtest-noble-danilogondolfo-netplan/noble/amd64/n/netplan.io/20231114_180016_fa593@/log.gz

https://autopkgtest.ubuntu.com/results/autopkgtest-noble-danilogondolfo-netplan/noble/arm64/n/netplan.io/20231114_195805_893d9@/log.gz

https://autopkgtest.ubuntu.com/results/autopkgtest-noble-danilogondolfo-netplan/noble/armhf/n/netplan.io/20231114_181654_b4f16@/log.gz

https://autopkgtest.ubuntu.com/results/autopkgtest-noble-danilogondolfo-netplan/noble/ppc64el/n/netplan.io/20231114_180447_b4f16@/log.gz

https://autopkgtest.ubuntu.com/results/autopkgtest-noble-danilogondolfo-netplan/noble/s390x/n/netplan.io/20231114_191920_3fa76@/log.gz

[Lukas Märdian (slyon)]

Thanks for providing build-time (PPA) and autopkgtests. The patches match what we've already reviewed in the upstream PR. LGTM.

Sponsored into Noble.

review: Approve

diff --git a/debian/changelog b/debian/changelog
index 2e1f737..07f3dd1 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,15 @@
+netplan.io (0.107-5ubuntu2) noble; urgency=medium
+
+  * d/p/lp1809994/0010-parse-improve-the-parsing-of-access-points-LP-180999.patch.
+    Improve access-points parsing. Properly handle access-points if a second
+    pass if required in the parser and not report them as duplicates. (LP: #1809994)
+  * d/p/lp1809994/0011-wifi-replace-the-previously-defined-AP-with-the-new-.patch.
+    Overwrite the access-point object if another access-point with the same
+    name is found instead of ignoring the new one. This behavior is closer
+    to the merging process Netplan does.
+
+ -- Danilo Egea Gondolfo <[e-mail]>  Tue, 14 Nov 2023 14:13:31 +0000
+
 netplan.io (0.107-5ubuntu1) noble; urgency=medium
 
   * d/p/lp2039821/0008-wireguard-ignore-empty-endpoints.patch (LP: #2039821)
diff --git a/debian/patches/lp1809994/0010-parse-improve-the-parsing-of-access-points-LP-180999.patch b/debian/patches/lp1809994/0010-parse-improve-the-parsing-of-access-points-LP-180999.patch
new file mode 100644
index 0000000..85392c7
--- /dev/null
+++ b/debian/patches/lp1809994/0010-parse-improve-the-parsing-of-access-points-LP-180999.patch
@@ -0,0 +1,197 @@
+From: Danilo Egea Gondolfo <[e-mail]>
+Date: Fri, 6 Oct 2023 11:42:36 +0100
+Subject: parse: improve the parsing of access-points (LP: #1809994)
+
+When the parser requires a second pass, it will report access-points it
+already parsed as duplicates and error out.
+
+With this change, access-points will be stored in a local hash table,
+which will be used to find duplicates in the same netdef, and moved to
+the netdef->access_points hash table in the end. Access-points already
+present in the netdef hash table will be considered already parsed and
+skipped.
+
+Add a test to check the problem is fixed and another one to test if
+merging the access-points when the same interface is present in multiple
+files is working properly.
+
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/netplan.io/+bug/1809994
+Origin: https://github.com/canonical/netplan/pull/413
+---
+ src/parse.c                    | 45 +++++++++++++++++++---------
+ src/types-internal.h           |  3 ++
+ src/types.c                    |  2 +-
+ tests/generator/test_common.py | 54 ++++++++++++++++++++++++++++++++++
+ 4 files changed, 89 insertions(+), 15 deletions(-)
+
+--- a/src/parse.c
++++ b/src/parse.c
+@@ -59,6 +59,9 @@
+ 
+ NetplanParser global_parser = {0};
+ 
++static gboolean
++insert_kv_into_hash(void *key, void *value, void *hash);
++
+ /**
+  * Load YAML file into a yaml_document_t.
+  *
+@@ -1404,11 +1407,13 @@
+ static gboolean
+ handle_wifi_access_points(NetplanParser* npp, yaml_node_t* node, const char* key_prefix, __unused const void* data, GError** error)
+ {
++    GHashTable* access_points = g_hash_table_new(g_str_hash, g_str_equal);
++
+     for (yaml_node_pair_t* entry = node->data.mapping.pairs.start; entry < node->data.mapping.pairs.top; entry++) {
+         NetplanWifiAccessPoint *access_point = NULL;
+         g_autofree char* full_key = NULL;
+         yaml_node_t* key, *value;
+-        gboolean ret = TRUE;
++        const gchar* ssid;
+ 
+         key = yaml_document_get_node(&npp->doc, entry->key);
+         assert_type(npp, key, YAML_SCALAR_NODE);
+@@ -1421,31 +1426,43 @@
+                 continue;
+         }
+ 
++        ssid = scalar(key);
++
++        /* Skip access-points that already exist in the netdef */
++        if (npp->current.netdef->access_points && g_hash_table_contains(npp->current.netdef->access_points, ssid))
++            continue;
++
++        /* Check if there's already an SSID with that name in the list of APs we are parsing */
++        if (g_hash_table_contains(access_points, ssid)) {
++            g_hash_table_foreach(access_points, free_access_point, NULL);
++            g_hash_table_destroy(access_points);
++            return yaml_error(npp, key, error, "%s: Duplicate access point SSID '%s'", npp->current.netdef->id, ssid);
++        }
++
+         g_assert(access_point == NULL);
+         access_point = g_new0(NetplanWifiAccessPoint, 1);
+-        access_point->ssid = g_strdup(scalar(key));
++        access_point->ssid = g_strdup(ssid);
+         g_debug("%s: adding wifi AP '%s'", npp->current.netdef->id, access_point->ssid);
+ 
+-        /* Check if there's already an SSID with that name */
+-        // FIXME: This check fails on multi-pass parsing, e.g. when defined in
+-        //        the same YAML file with a set of virtual-ethernets peers.
+-        if (npp->current.netdef->access_points &&
+-                g_hash_table_lookup(npp->current.netdef->access_points, access_point->ssid)) {
+-            ret = yaml_error(npp, key, error, "%s: Duplicate access point SSID '%s'", npp->current.netdef->id, access_point->ssid);
+-        }
+-
+         npp->current.access_point = access_point;
+-        if (!ret || !process_mapping(npp, value, full_key, wifi_access_point_handlers, NULL, error)) {
++        if (!process_mapping(npp, value, full_key, wifi_access_point_handlers, NULL, error)) {
+             access_point_clear(&npp->current.access_point, npp->current.backend);
++            g_hash_table_foreach(access_points, free_access_point, NULL);
++            g_hash_table_destroy(access_points);
+             return FALSE;
+         }
+ 
++        g_hash_table_insert(access_points, access_point->ssid, access_point);
++        npp->current.access_point = NULL;
++    }
++
++    if (g_hash_table_size(access_points) > 0) {
+         if (!npp->current.netdef->access_points)
+             npp->current.netdef->access_points = g_hash_table_new(g_str_hash, g_str_equal);
+-        g_hash_table_insert(npp->current.netdef->access_points, access_point->ssid, access_point);
+-        npp->current.access_point = NULL;
++        g_hash_table_foreach_steal(access_points, insert_kv_into_hash, npp->current.netdef->access_points);
++        mark_data_as_dirty(npp, &npp->current.netdef->access_points);
+     }
+-    mark_data_as_dirty(npp, &npp->current.netdef->access_points);
++    g_hash_table_destroy(access_points);
+     return TRUE;
+ }
+ 
+--- a/src/types-internal.h
++++ b/src/types-internal.h
+@@ -313,3 +313,6 @@
+ 
+ void
+ free_address_options(void* ptr);
++
++void
++free_access_point(void* key, void* value, void* data);
+--- a/src/types.c
++++ b/src/types.c
+@@ -200,7 +200,7 @@
+  * @data: pointer to a NetplanBackend value representing the renderer context in which
+  *        to interpret the processed object, especially regarding the backend settings
+  */
+-static void
++void
+ free_access_point(__unused void* key, void* value, __unused void* data)
+ {
+     NetplanWifiAccessPoint* ap = value;
+--- a/tests/generator/test_common.py
++++ b/tests/generator/test_common.py
+@@ -1557,6 +1557,28 @@
+ ''',
+                               'eth0.network': ND_EMPTY % ('eth0', 'no') + 'Bond=aggi\n'})
+ 
++    def test_check_parser_second_pass_will_not_lead_to_duplicate_access_point(self):
++        '''
++        When the parser needs more than one pass we shouldn't
++        try to load the same access-point again from wifi devices.
++        Testcase for LP: #1809994'''
++
++        self.generate('''network:
++  bridges:
++    br0:
++      interfaces:
++        - eth0
++  ethernets:
++    eth0:
++      dhcp4: false
++  wifis:
++    wlan0:
++      dhcp4: true
++      access-points:
++        "mywifi":
++          password: "aaaaaaaa"
++''')
++
+ 
+ class TestMerging(TestBase):
+     '''multiple *.yaml merging'''
+@@ -1740,3 +1762,35 @@
+ ''',
+                               'enyellow.network': ND_DHCP4 % 'enyellow',
+                               'enblue.network': ND_DHCP4 % 'enblue'})
++
++    def test_wifi_access_points_merging(self):
++        self.generate('''network:
++  version: 2
++  wifis:
++    wlan0:
++      dhcp4: true
++      access-points:
++        "mywifi":
++          password: "aaaaaaaa"''',
++                      confs={'newwifi': '''network:
++  version: 2
++  wifis:
++    wlan0:
++      dhcp4: true
++      access-points:
++        "mynewwifi":
++          password: "aaaaaaaa"'''})
++
++        self.assert_wpa_supplicant("wlan0", """ctrl_interface=/run/wpa_supplicant
++
++network={
++  ssid="mynewwifi"
++  key_mgmt=WPA-PSK
++  psk="aaaaaaaa"
++}
++network={
++  ssid="mywifi"
++  key_mgmt=WPA-PSK
++  psk="aaaaaaaa"
++}
++""")
diff --git a/debian/patches/lp1809994/0011-wifi-replace-the-previously-defined-AP-with-the-new-.patch b/debian/patches/lp1809994/0011-wifi-replace-the-previously-defined-AP-with-the-new-.patch
new file mode 100644
index 0000000..c03521e
--- /dev/null
+++ b/debian/patches/lp1809994/0011-wifi-replace-the-previously-defined-AP-with-the-new-.patch
@@ -0,0 +1,79 @@
+From: Danilo Egea Gondolfo <[e-mail]>
+Date: Thu, 12 Oct 2023 18:07:31 +0100
+Subject: wifi: replace the previously defined AP with the new one
+
+If we find an AP with the same name in the same interface, we drop the
+first one and use the new one. This is done to maintain the documented
+behavior of respecting the order in which the files are parsed. We still
+need to implement support for merging AP settings.
+
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/netplan.io/+bug/1809994
+Origin: https://github.com/canonical/netplan/pull/413
+---
+ src/parse.c                    | 17 +++++++++++++----
+ tests/generator/test_common.py | 30 ++++++++++++++++++++++++++++++
+ 2 files changed, 43 insertions(+), 4 deletions(-)
+
+--- a/src/parse.c
++++ b/src/parse.c
+@@ -1428,11 +1428,20 @@
+ 
+         ssid = scalar(key);
+ 
+-        /* Skip access-points that already exist in the netdef */
+-        if (npp->current.netdef->access_points && g_hash_table_contains(npp->current.netdef->access_points, ssid))
+-            continue;
++        /*
++         * Delete the access-point if it already exists in the netdef and let the new
++         * one be added. It has the side effect of reprocessing APs if the parser requires a
++         * second pass.
++         *
++         * TODO: implement support for merging AP settings if they were previously defined
++         */
++        if (npp->current.netdef->access_points && g_hash_table_contains(npp->current.netdef->access_points, ssid)) {
++            NetplanWifiAccessPoint *ap = g_hash_table_lookup(npp->current.netdef->access_points, ssid);
++            g_hash_table_remove(npp->current.netdef->access_points, ssid);
++            free_access_point(NULL, ap, NULL);
++        }
+ 
+-        /* Check if there's already an SSID with that name in the list of APs we are parsing */
++        /* Check if the SSID was already defined in the same netdef in this YAML file we are parsing */
+         if (g_hash_table_contains(access_points, ssid)) {
+             g_hash_table_foreach(access_points, free_access_point, NULL);
+             g_hash_table_destroy(access_points);
+--- a/tests/generator/test_common.py
++++ b/tests/generator/test_common.py
+@@ -1794,3 +1794,33 @@
+   psk="aaaaaaaa"
+ }
+ """)
++
++    def test_wifi_access_points_overwriting(self):
++        ''' If we find an AP that is already defined we drop the first one.
++            XXX: this test must be removed once we implement support for AP merging
++        '''
++        self.generate('''network:
++  version: 2
++  wifis:
++    wlan0:
++      dhcp4: true
++      access-points:
++        "mywifi":
++          password: "aaaaaaaa"''',
++                      confs={'newwifi': '''network:
++  version: 2
++  wifis:
++    wlan0:
++      dhcp4: true
++      access-points:
++        "mywifi":
++          password: "bbbbbbbb"'''})
++
++        self.assert_wpa_supplicant("wlan0", """ctrl_interface=/run/wpa_supplicant
++
++network={
++  ssid="mywifi"
++  key_mgmt=WPA-PSK
++  psk="bbbbbbbb"
++}
++""")
diff --git a/debian/patches/series b/debian/patches/series
index 837291f..4d9d078 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -7,3 +7,5 @@ lp2034595/0006-apply-bring-lo-back-up-if-it-s-managed-by-NM.patch
 lp2034595/0007-apply-don-t-assume-the-NM-loopback-connection-is-cal.patch
 lp2039821/0008-wireguard-ignore-empty-endpoints.patch
 lp2039825/0009-auth-add-support-for-LEAP-and-EAP-PWD.patch
+lp1809994/0010-parse-improve-the-parsing-of-access-points-LP-180999.patch
+lp1809994/0011-wifi-replace-the-previously-defined-AP-with-the-new-.patch