Merge ~smoser/cloud-init:fix/1798117-allow-toplevel-network-in-network-config into cloud-init:master

Proposed by Scott Moser
Status: Merged
Approved by: Scott Moser
Approved revision: a1337d0c644e80d40c38d153338082b53de44c63
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~smoser/cloud-init:fix/1798117-allow-toplevel-network-in-network-config
Merge into: cloud-init:master
Diff against target: 262 lines (+97/-35)
2 files modified
cloudinit/sources/DataSourceNoCloud.py (+31/-1)
tests/unittests/test_datasource/test_nocloud.py (+66/-34)
Reviewer Review Type Date Requested Status
Scott Moser Approve
Server Team CI bot continuous-integration Approve
Chad Smith Approve
Review via email: mp+356850@code.launchpad.net

Commit message

NoCloud: Allow top level 'network' key in network-config.

NoCloud's 'network-config' file was originally expected to contain
network configuration without the top level 'network' key. This was
because the file was named 'network-config' so specifying 'network'
seemed redundant.

However, JuJu is currently providing a top level 'network' config when
it tries to disable networking ({"network": {"config": "disabled"}).
Other users have also been surprised/confused by the fact that
a network config in /etc/cloud/cloud.cfg.d/network.cfg differed from
what was expected in 'network-config'.

LP: #1798117

Description of the change

see commit message

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:02d7c7bfdefceb71db86cec0ca09dea49576fc34
https://jenkins.ubuntu.com/server/job/cloud-init-ci/394/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/394/rebuild

review: Approve (continuous-integration)
Revision history for this message
Chad Smith (chad.smith) :
Revision history for this message
Chad Smith (chad.smith) :
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:08621f1188566b888240a408dbe019fec03439ec
https://jenkins.ubuntu.com/server/job/cloud-init-ci/419/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/419/rebuild

review: Approve (continuous-integration)
Revision history for this message
Chad Smith (chad.smith) wrote :

Thanks. LGTM!

review: Approve
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/cloud-init-autoland-test/101/
Executed test runs:
    FAILED: Checkout

review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:a1337d0c644e80d40c38d153338082b53de44c63
https://jenkins.ubuntu.com/server/job/cloud-init-ci/476/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/476/rebuild

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

marking approved based on c-i now approving.
the fail'd auto-land was a infrastructure failure.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/sources/DataSourceNoCloud.py b/cloudinit/sources/DataSourceNoCloud.py
2index 9010f06..6860f0c 100644
3--- a/cloudinit/sources/DataSourceNoCloud.py
4+++ b/cloudinit/sources/DataSourceNoCloud.py
5@@ -311,6 +311,35 @@ def parse_cmdline_data(ds_id, fill, cmdline=None):
6 return True
7
8
9+def _maybe_remove_top_network(cfg):
10+ """If network-config contains top level 'network' key, then remove it.
11+
12+ Some providers of network configuration may provide a top level
13+ 'network' key (LP: #1798117) even though it is not necessary.
14+
15+ Be friendly and remove it if it really seems so.
16+
17+ Return the original value if no change or the updated value if changed."""
18+ nullval = object()
19+ network_val = cfg.get('network', nullval)
20+ if network_val is nullval:
21+ return cfg
22+ bmsg = 'Top level network key in network-config %s: %s'
23+ if not isinstance(network_val, dict):
24+ LOG.debug(bmsg, "was not a dict", cfg)
25+ return cfg
26+ if len(list(cfg.keys())) != 1:
27+ LOG.debug(bmsg, "had multiple top level keys", cfg)
28+ return cfg
29+ if network_val.get('config') == "disabled":
30+ LOG.debug(bmsg, "was config/disabled", cfg)
31+ elif not all(('config' in network_val, 'version' in network_val)):
32+ LOG.debug(bmsg, "but missing 'config' or 'version'", cfg)
33+ return cfg
34+ LOG.debug(bmsg, "fixed by removing shifting network.", cfg)
35+ return network_val
36+
37+
38 def _merge_new_seed(cur, seeded):
39 ret = cur.copy()
40
41@@ -320,7 +349,8 @@ def _merge_new_seed(cur, seeded):
42 ret['meta-data'] = util.mergemanydict([cur['meta-data'], newmd])
43
44 if seeded.get('network-config'):
45- ret['network-config'] = util.load_yaml(seeded['network-config'])
46+ ret['network-config'] = _maybe_remove_top_network(
47+ util.load_yaml(seeded.get('network-config')))
48
49 if 'user-data' in seeded:
50 ret['user-data'] = seeded['user-data']
51diff --git a/tests/unittests/test_datasource/test_nocloud.py b/tests/unittests/test_datasource/test_nocloud.py
52index b6468b6..3429272 100644
53--- a/tests/unittests/test_datasource/test_nocloud.py
54+++ b/tests/unittests/test_datasource/test_nocloud.py
55@@ -1,7 +1,10 @@
56 # This file is part of cloud-init. See LICENSE file for license information.
57
58 from cloudinit import helpers
59-from cloudinit.sources import DataSourceNoCloud
60+from cloudinit.sources.DataSourceNoCloud import (
61+ DataSourceNoCloud as dsNoCloud,
62+ _maybe_remove_top_network,
63+ parse_cmdline_data)
64 from cloudinit import util
65 from cloudinit.tests.helpers import CiTestCase, populate_dir, mock, ExitStack
66
67@@ -40,9 +43,7 @@ class TestNoCloudDataSource(CiTestCase):
68 'datasource': {'NoCloud': {'fs_label': None}}
69 }
70
71- ds = DataSourceNoCloud.DataSourceNoCloud
72-
73- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
74+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
75 ret = dsrc.get_data()
76 self.assertEqual(dsrc.userdata_raw, ud)
77 self.assertEqual(dsrc.metadata, md)
78@@ -63,9 +64,7 @@ class TestNoCloudDataSource(CiTestCase):
79 'datasource': {'NoCloud': {'fs_label': None}}
80 }
81
82- ds = DataSourceNoCloud.DataSourceNoCloud
83-
84- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
85+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
86 self.assertTrue(dsrc.get_data())
87 self.assertEqual(dsrc.platform_type, 'nocloud')
88 self.assertEqual(
89@@ -73,8 +72,6 @@ class TestNoCloudDataSource(CiTestCase):
90
91 def test_fs_label(self, m_is_lxd):
92 # find_devs_with should not be called ff fs_label is None
93- ds = DataSourceNoCloud.DataSourceNoCloud
94-
95 class PsuedoException(Exception):
96 pass
97
98@@ -84,12 +81,12 @@ class TestNoCloudDataSource(CiTestCase):
99
100 # by default, NoCloud should search for filesystems by label
101 sys_cfg = {'datasource': {'NoCloud': {}}}
102- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
103+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
104 self.assertRaises(PsuedoException, dsrc.get_data)
105
106 # but disabling searching should just end up with None found
107 sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
108- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
109+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
110 ret = dsrc.get_data()
111 self.assertFalse(ret)
112
113@@ -97,13 +94,10 @@ class TestNoCloudDataSource(CiTestCase):
114 # no source should be found if no cmdline, config, and fs_label=None
115 sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
116
117- ds = DataSourceNoCloud.DataSourceNoCloud
118- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
119+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
120 self.assertFalse(dsrc.get_data())
121
122 def test_seed_in_config(self, m_is_lxd):
123- ds = DataSourceNoCloud.DataSourceNoCloud
124-
125 data = {
126 'fs_label': None,
127 'meta-data': yaml.safe_dump({'instance-id': 'IID'}),
128@@ -111,7 +105,7 @@ class TestNoCloudDataSource(CiTestCase):
129 }
130
131 sys_cfg = {'datasource': {'NoCloud': data}}
132- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
133+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
134 ret = dsrc.get_data()
135 self.assertEqual(dsrc.userdata_raw, b"USER_DATA_RAW")
136 self.assertEqual(dsrc.metadata.get('instance-id'), 'IID')
137@@ -130,9 +124,7 @@ class TestNoCloudDataSource(CiTestCase):
138 'datasource': {'NoCloud': {'fs_label': None}}
139 }
140
141- ds = DataSourceNoCloud.DataSourceNoCloud
142-
143- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
144+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
145 ret = dsrc.get_data()
146 self.assertEqual(dsrc.userdata_raw, ud)
147 self.assertEqual(dsrc.metadata, md)
148@@ -145,9 +137,7 @@ class TestNoCloudDataSource(CiTestCase):
149
150 sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
151
152- ds = DataSourceNoCloud.DataSourceNoCloud
153-
154- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
155+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
156 ret = dsrc.get_data()
157 self.assertEqual(dsrc.userdata_raw, b"ud")
158 self.assertFalse(dsrc.vendordata)
159@@ -174,9 +164,7 @@ class TestNoCloudDataSource(CiTestCase):
160
161 sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
162
163- ds = DataSourceNoCloud.DataSourceNoCloud
164-
165- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
166+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
167 ret = dsrc.get_data()
168 self.assertTrue(ret)
169 # very simple check just for the strings above
170@@ -195,9 +183,23 @@ class TestNoCloudDataSource(CiTestCase):
171
172 sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
173
174- ds = DataSourceNoCloud.DataSourceNoCloud
175+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
176+ ret = dsrc.get_data()
177+ self.assertTrue(ret)
178+ self.assertEqual(netconf, dsrc.network_config)
179+
180+ def test_metadata_network_config_with_toplevel_network(self, m_is_lxd):
181+ """network-config may have 'network' top level key."""
182+ netconf = {'config': 'disabled'}
183+ populate_dir(
184+ os.path.join(self.paths.seed_dir, "nocloud"),
185+ {'user-data': b"ud",
186+ 'meta-data': "instance-id: IID\n",
187+ 'network-config': yaml.dump({'network': netconf}) + "\n"})
188+
189+ sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
190
191- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
192+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
193 ret = dsrc.get_data()
194 self.assertTrue(ret)
195 self.assertEqual(netconf, dsrc.network_config)
196@@ -228,9 +230,7 @@ class TestNoCloudDataSource(CiTestCase):
197
198 sys_cfg = {'datasource': {'NoCloud': {'fs_label': None}}}
199
200- ds = DataSourceNoCloud.DataSourceNoCloud
201-
202- dsrc = ds(sys_cfg=sys_cfg, distro=None, paths=self.paths)
203+ dsrc = dsNoCloud(sys_cfg=sys_cfg, distro=None, paths=self.paths)
204 ret = dsrc.get_data()
205 self.assertTrue(ret)
206 self.assertEqual(netconf, dsrc.network_config)
207@@ -258,8 +258,7 @@ class TestParseCommandLineData(CiTestCase):
208 for (fmt, expected) in pairs:
209 fill = {}
210 cmdline = fmt % {'ds_id': ds_id}
211- ret = DataSourceNoCloud.parse_cmdline_data(ds_id=ds_id, fill=fill,
212- cmdline=cmdline)
213+ ret = parse_cmdline_data(ds_id=ds_id, fill=fill, cmdline=cmdline)
214 self.assertEqual(expected, fill)
215 self.assertTrue(ret)
216
217@@ -276,10 +275,43 @@ class TestParseCommandLineData(CiTestCase):
218
219 for cmdline in cmdlines:
220 fill = {}
221- ret = DataSourceNoCloud.parse_cmdline_data(ds_id=ds_id, fill=fill,
222- cmdline=cmdline)
223+ ret = parse_cmdline_data(ds_id=ds_id, fill=fill, cmdline=cmdline)
224 self.assertEqual(fill, {})
225 self.assertFalse(ret)
226
227
228+class TestMaybeRemoveToplevelNetwork(CiTestCase):
229+ """test _maybe_remove_top_network function."""
230+ basecfg = [{'type': 'physical', 'name': 'interface0',
231+ 'subnets': [{'type': 'dhcp'}]}]
232+
233+ def test_should_remove_safely(self):
234+ mcfg = {'config': self.basecfg, 'version': 1}
235+ self.assertEqual(mcfg, _maybe_remove_top_network({'network': mcfg}))
236+
237+ def test_no_remove_if_other_keys(self):
238+ """should not shift if other keys at top level."""
239+ mcfg = {'network': {'config': self.basecfg, 'version': 1},
240+ 'unknown_keyname': 'keyval'}
241+ self.assertEqual(mcfg, _maybe_remove_top_network(mcfg))
242+
243+ def test_no_remove_if_non_dict(self):
244+ """should not shift if not a dict."""
245+ mcfg = {'network': '"content here'}
246+ self.assertEqual(mcfg, _maybe_remove_top_network(mcfg))
247+
248+ def test_no_remove_if_missing_config_or_version(self):
249+ """should not shift unless network entry has config and version."""
250+ mcfg = {'network': {'config': self.basecfg}}
251+ self.assertEqual(mcfg, _maybe_remove_top_network(mcfg))
252+
253+ mcfg = {'network': {'version': 1}}
254+ self.assertEqual(mcfg, _maybe_remove_top_network(mcfg))
255+
256+ def test_remove_with_config_disabled(self):
257+ """network/config=disabled should be shifted."""
258+ mcfg = {'config': 'disabled'}
259+ self.assertEqual(mcfg, _maybe_remove_top_network({'network': mcfg}))
260+
261+
262 # vi: ts=4 expandtab

Subscribers

People subscribed via source and target branches