[Merge] ~slyon/network-manager:netplan-integration into network-manager:ubuntu/master

[Olivier Gayot]

Thanks! A few comments inline.

Diff comments:

> diff --git a/debian/patches/netplan/keyfile-drop-netplan-YAML-on-delete.patch b/debian/patches/netplan/keyfile-drop-netplan-YAML-on-delete.patch
> new file mode 100644
> index 0000000..545a8c7
> --- /dev/null
> +++ b/debian/patches/netplan/keyfile-drop-netplan-YAML-on-delete.patch
> @@ -0,0 +1,42 @@
> +From: =?utf-8?q?Lukas_M=C3=A4rdian?= <slyon at ubuntu.com>
> +Date: Tue, 31 May 2022 16:12:54 +0200
> +Subject: keyfile: drop netplan YAML on delete
> +
> +---
> + src/core/settings/plugins/keyfile/nms-keyfile-plugin.c | 16 ++++++++++++++++
> + 1 file changed, 16 insertions(+)
> +
> +diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-plugin.c b/src/core/settings/plugins/keyfile/nms-keyfile-plugin.c
> +index efb9bfc..716d89e 100644
> +--- a/src/core/settings/plugins/keyfile/nms-keyfile-plugin.c
> ++++ b/src/core/settings/plugins/keyfile/nms-keyfile-plugin.c
> +@@ -12,6 +12,7 @@
> + #include <unistd.h>
> + #include <sys/types.h>
> + #include <sys/time.h>
> ++#include <netplan/util.h>
> + 
> + #include "libnm-std-aux/c-list-util.h"
> + #include "libnm-glib-aux/nm-c-list.h"
> +@@ -1010,6 +1011,21 @@ delete_connection(NMSettingsPlugin *plugin, NMSettingsStorage *storage_x, GError
> +     previous_filename = nms_keyfile_storage_get_filename(storage);
> +     uuid              = nms_keyfile_storage_get_uuid(storage);
> + 
> ++    /* Clear from YAML if this connection was generated by netplan */
> ++    if (previous_filename && strstr(previous_filename, "run/NetworkManager/system-connections/netplan-")) {

I think a leading slash before "run/NetworkManager" would be safer, wouldn't it?

> ++        nm_auto_unref_keyfile GKeyFile *key_file = g_key_file_new();
> ++        if (!g_key_file_load_from_file (key_file, previous_filename, G_KEY_FILE_NONE, error))
> ++            _LOGW ("Clearing netplan YAML: cannot load keyfile connection profile");

shouldn't we goto here to avoid calling g_key_file_get_string() with an "invalid" key_file?

Assuming that g_key_file_get_string(NULL, ...) returns NULL (I don't know if this happens in practice), we would end up with ssid=NULL. Wouldn't netplan_get_if_from_nm_filename misbehave?

> ++
> ++        g_autofree gchar* ssid = g_key_file_get_string(key_file, "wifi", "ssid", NULL);
> ++        g_autofree gchar* netplan_id = netplan_get_id_from_nm_filename(previous_filename, ssid);
> ++        if (netplan_id) {
> ++            _LOGI ("Deleting netplan YAML: %s", netplan_id);
> ++            if (!netplan_delete_connection(netplan_id, NULL))
> ++                _LOGW ("Deleting netplan YAML (%s) failed.", netplan_id);
> ++        }
> ++    }
> ++
> +     if (!NM_IN_SET(storage->storage_type,
> +                    NMS_KEYFILE_STORAGE_TYPE_ETC,
> +                    NMS_KEYFILE_STORAGE_TYPE_RUN)) {
> diff --git a/debian/tests/netplan-integration b/debian/tests/netplan-integration
> new file mode 100755
> index 0000000..c2bf224
> --- /dev/null
> +++ b/debian/tests/netplan-integration
> @@ -0,0 +1,57 @@
> +#!/bin/sh
> +set -e
> +
> +# keep management network on sd-networkd
> +mgmt=$(netplan get network.ethernets | head -1 | rev | cut -c 2- | rev)
> +netplan set "network.ethernets.$mgmt.renderer=networkd"
> +
> +# setup some netplan connections, in addition to the NM default renderer
> +cat <<EOT >> /etc/netplan/01-network-manager-all.yaml

Just in case this test is run twice, I would rather use > instead of >>. Although I don't know how netplan would behave if the content is duplicate.

In the meantime, this file /could/ exist on the filesystem before the tests are run, right? Maybe adding -autopkgtest as in 01-autopkgtest-network-manager-all.yaml would make sense? What are your thoughts?

> +network:
> +  version: 2
> +  renderer: NetworkManager
> +EOT
> +
> +cat <<EOT >> /etc/netplan/02-extra.yaml
> +network:
> +  ethernets:
> +    eth42:
> +      dhcp4: true
> +      dhcp6: false
> +    eth43:
> +      dhcp4: false
> +      dhcp6: true
> +  version: 2
> +EOT
> +
> +echo "=== netplan configuration setup"
> +netplan get
> +
> +echo "=== make sure both netplan connections got picked up by NM"
> +netplan generate
> +systemctl restart NetworkManager.service
> +nmcli con show
> +nmcli con show | grep -q netplan-eth42
> +nmcli con show | grep -q netplan-eth43
> +echo "OK"
> +
> +echo "=== delete one NM connection, make sure it got dropped from netplan YAML"
> +nmcli con del netplan-eth42
> +nmcli con show
> +netplan get
> +! nmcli con show | grep netplan-eth42
> +! netplan get | grep eth42
> +netplan get | grep -q eth43 #but the other should stick around
> +echo "OK"
> +
> +echo "=== edit one NM connection, make sure it got dropped from netplan YAML"
> +# control is taken over by NM
> +nmcli con mod netplan-eth43 ipv4.method auto
> +nmcli con show
> +netplan get
> +nmcli con show | grep -q netplan-eth43
> +! netplan get | grep eth42
> +! netplan get | grep eth43
> +echo "OK"
> +
> +exit 0


-- 
https://code.launchpad.net/~slyon/network-manager/+git/network-manager/+merge/423735
Your team Network-manager is requested to review the proposed merge of ~slyon/network-manager:netplan-integration into network-manager:ubuntu/master.