From f2c8f64f8bed10ae49283b77b514e733c4cef5c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20M=C3=A4rdian?= Date: Wed, 19 Jul 2023 16:18:34 +0200 Subject: [PATCH] netplan.c: Don't drop files with just global values on 'set' (LP: #2027584) The "unlink" part at the bottom of netplan.c:netplan_state_update_yaml_hierarchy seems to ignore any global values (such as "renderer") and operates on files containing netdefs only. The issue is happens due to a combination of the following PRs: https://github.com/canonical/netplan/pull/254 https://github.com/canonical/netplan/pull/299 Which got SRUed into Jammy via 0.105-0ubuntu2~22.04.2 Here's a minimal reproducer: ``` netplan set network.renderer=NetworkManager --origin-hint=00-no-netdefs-just-globals netplan set network.ethernets.eth99.dhcp4=true --origin-hint=90-some-netdefs ls -l /etc/netplan/ netplan set network.ethernets.eth99=NULL cat /etc/netplan/00-no-netdefs-just-globals.yaml ``` FR-4793 --- src/netplan.c | 22 ++++++++++-- tests/cli/test_get_set.py | 73 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/src/netplan.c b/src/netplan.c index 23a0f701d..f259e2566 100644 --- a/src/netplan.c +++ b/src/netplan.c @@ -1198,8 +1198,10 @@ netplan_state_update_yaml_hierarchy(const NetplanState* np_state, const char* de /* Dump global conf to the default path */ if (!np_state->netdefs || g_hash_table_size(np_state->netdefs) == 0) { - if ((np_state->backend != NETPLAN_BACKEND_NONE) - || has_openvswitch(&np_state->ovs_settings, NETPLAN_BACKEND_NONE, NULL)) { + if ( has_openvswitch(&np_state->ovs_settings, NETPLAN_BACKEND_NONE, NULL) + || (np_state->backend != NETPLAN_BACKEND_NONE && np_state->global_renderer && + ( g_hash_table_contains(np_state->global_renderer, default_path) // 70-netplan-set.yaml already exsits and defines a global renderer + || g_hash_table_contains(np_state->global_renderer, "")))) { // 70-netplan-set.yaml doesn't exist, but we need to create it to define a global renderer g_hash_table_insert(perfile_netdefs, default_path, NULL); } } else { @@ -1214,6 +1216,22 @@ netplan_state_update_yaml_hierarchy(const NetplanState* np_state, const char* de } } + /* Add files containing a global renderer value to "perfile_netdefs", so + * they are updated on disk. */ + if (np_state->global_renderer && g_hash_table_size(np_state->global_renderer) > 0) { + g_hash_table_iter_init(&hash_iter, np_state->global_renderer); + while (g_hash_table_iter_next (&hash_iter, &key, &value)) { + char *filename = key; + /* Anonymous globals will go to the default YAML (see above) */ + if (g_strcmp0(filename, "") == 0) + continue; + /* Ignore the update of this file if it's already going to be + * written, caused by updated netdefs. */ + if (!g_hash_table_contains(perfile_netdefs, filename)) + g_hash_table_insert(perfile_netdefs, filename, NULL); + } + } + g_hash_table_iter_init(&hash_iter, perfile_netdefs); while (g_hash_table_iter_next (&hash_iter, &key, &value)) { const char *filename = key; diff --git a/tests/cli/test_get_set.py b/tests/cli/test_get_set.py index d2c9df5a3..277dd9653 100644 --- a/tests/cli/test_get_set.py +++ b/tests/cli/test_get_set.py @@ -25,6 +25,7 @@ import yaml +from netplan.cli.commands.set import FALLBACK_FILENAME from netplan.libnetplan import NetplanException from tests.test_utils import call_cli @@ -248,6 +249,78 @@ def test_set_network_null_global(self): self.assertFalse(os.path.isfile(self.path)) self.assertFalse(os.path.isfile(some_hint)) + def test_set_no_netdefs_just_globals(self): # LP: #2027584 + keepme1 = os.path.join(self.workdir.name, 'etc', 'netplan', + '00-no-netdefs-just-renderer.yaml') + with open(keepme1, 'w') as f: + f.write('''network: {renderer: NetworkManager}''') + keepme2 = os.path.join(self.workdir.name, 'etc', 'netplan', + '00-no-netdefs-just-version.yaml') + with open(keepme2, 'w') as f: + f.write('''network: {version: 2}''') + deleteme = os.path.join(self.workdir.name, 'etc', 'netplan', + '90-some-netdefs.yaml') + with open(deleteme, 'w') as f: + f.write('''network: {ethernets: {eth99: {dhcp4: true}}}''') + + self._set(['ethernets.eth99=NULL']) + self.assertFalse(os.path.isfile(deleteme)) + self.assertTrue(os.path.isfile(keepme1)) + with open(keepme1, 'r') as f: + yml = yaml.safe_load(f) + self.assertEqual('NetworkManager', yml['network']['renderer']) + # XXX: It's probably fine to delete a file that just contains "version: 2" + # Or is it? And what about other globals, such as OVS ports? + self.assertFalse(os.path.isfile(keepme2)) + + def test_set_clear_netdefs_keep_globals(self): # LP: #2027584 + keep = os.path.join(self.workdir.name, 'etc', 'netplan', '00-keep.yaml') + with open(keep, 'w') as f: + f.write('''network: + version: 2 + renderer: NetworkManager + bridges: + br54: + addresses: [1.2.3.4/24] +''') + self._set(['network.bridges.br54=NULL']) + self.assertTrue(os.path.isfile(keep)) + with open(keep, 'r') as f: + yml = yaml.safe_load(f) + self.assertEqual(2, yml['network']['version']) + self.assertEqual('NetworkManager', yml['network']['renderer']) + self.assertNotIn('bridges', yml['network']) + default = os.path.join(self.workdir.name, 'etc', 'netplan', FALLBACK_FILENAME) + self.assertFalse(os.path.isfile(default)) + + def test_set_clear_netdefs_keep_globals_default_renderer(self): + keep = os.path.join(self.workdir.name, 'etc', 'netplan', '00-keep.yaml') + with open(keep, 'w') as f: + f.write('''network: + version: 2 + renderer: NetworkManager + bridges: + br54: + addresses: [1.2.3.4/24] +''') + default = os.path.join(self.workdir.name, 'etc', 'netplan', FALLBACK_FILENAME) + with open(default, 'w') as f: + f.write('''network: + renderer: networkd +''') + self._set(['network.bridges.br54=NULL']) + self.assertTrue(os.path.isfile(keep)) + with open(keep, 'r') as f: + yml = yaml.safe_load(f) + self.assertEqual(2, yml['network']['version']) + self.assertEqual('NetworkManager', yml['network']['renderer']) + self.assertNotIn('bridges', yml['network']) + self.assertTrue(os.path.isfile(default)) + with open(default, 'r') as f: + yml = yaml.safe_load(f) + self.assertEqual(2, yml['network']['version']) + self.assertEqual('networkd', yml['network']['renderer']) + def test_set_invalid(self): with self.assertRaises(Exception) as context: self._set(['xxx.yyy=abc'])