cloud-init

Merge ~smoser/cloud-init:fix/1849640-adjust-yaml-usage into cloud-init:master

  1. Git
  2. lp:~smoser/cloud-init
  3. fix/1849640-adjust-yaml-usage
  4. Merge into master
Proposed by Scott Moser
Status: Merged
Approved by: Ryan Harper
Approved revision: e69df5dce264f9f329dd0630aad3954b32999153
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~smoser/cloud-init:fix/1849640-adjust-yaml-usage
Merge into: cloud-init:master
Diff against target: 395 lines (+53/-46)
13 files modified
cloudinit/cmd/devel/net_convert.py (+5/-8)
cloudinit/cmd/tests/test_main.py (+4/-3)
cloudinit/config/cc_debug.py (+2/-1)
cloudinit/config/cc_salt_minion.py (+3/-3)
cloudinit/config/cc_snappy.py (+2/-1)
cloudinit/handlers/cloud_config.py (+2/-1)
cloudinit/net/netplan.py (+8/-7)
cloudinit/net/network_state.py (+3/-2)
cloudinit/safeyaml.py (+15/-0)
cloudinit/util.py (+1/-15)
tests/unittests/test_data.py (+2/-1)
tests/unittests/test_runs/test_merge_run.py (+2/-1)
tests/unittests/test_runs/test_simple_run.py (+4/-3)
Reviewer Review Type Date Requested Status
Ryan Harper Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+374679@code.launchpad.net

Commit message

Fix usages of yaml, and move yaml_dump to safeyaml.dumps.

Here we replace uses of the pyyaml module directly with functions
provided by cloudinit.safeyaml. Also, change/move
  cloudinit.util.yaml_dumps
to
  cloudinit.safeyaml.dumps

LP: #1849640

Description of the change

see commit message

To post a comment you must log in.
Revision history for this message
Ryan Harper (raharper) wrote :

Is there an advantage to moving dumps into safeyaml? I didn't think dump had any concerns. Could we just have replaced the yaml.load() calls with util.load_yaml() ?

Revision history for this message
Scott Moser (smoser) wrote :

not a real advantage other than getting closer to getting all yaml
usage into a single place.

On Thu, Oct 24, 2019 at 12:56 PM Ryan Harper <email address hidden> wrote:
>
> Is there an advantage to moving dumps into safeyaml? I didn't think dump had any concerns. Could we just have replaced the yaml.load() calls with util.load_yaml() ?
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/374679
> You are the owner of ~smoser/cloud-init:fix/1849640-adjust-yaml-usage.

Revision history for this message
Scott Moser (smoser) wrote :

just in better use of modules... putting yaml things into their own module rather than having them all in util.

I'm willing to move load_yaml also if you'd like. which would mean util would have no yaml code in it.

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

PASSED: Continuous integration, rev:e7c84bfdc42bc533a8f737ecd6b9557b466e1ebd
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1232/
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/1232//rebuild

review: Approve (continuous-integration)
~smoser/cloud-init:fix/1849640-adjust-yaml-usage updated
e69df5d... by Scott Moser

remove 'import yaml' from cloudinit.util

Revision history for this message
Ryan Harper (raharper) wrote :

OK. I just wanted to make sure that I wasn't missing something on the dump path.

For relocating; there are quite a few yaml users inside util.py so we'd still need to import
safeyaml into util; so module load wise, we're still pulling it in on any import of util. additionally, moving the load_yaml into safeyaml would mean inter-module deps; load_yaml uses decode_binary(), so we'd need to duplicate that.

I'm fine with this as it is, but would also review moving all yaml operations into safeyaml.

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

FAILED: Continuous integration, rev:e69df5dce264f9f329dd0630aad3954b32999153
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1233/
Executed test runs:
    FAILED: Checkout

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

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

PASSED: Continuous integration, rev:e69df5dce264f9f329dd0630aad3954b32999153
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1234/
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/1234//rebuild

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

I'm happy with the way it is here... we got rid of all 'import yaml' from cloudinit.util at least.
and down to a signle place that 'yaml.load' is called.

Revision history for this message
Ryan Harper (raharper) wrote :

Sounds good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/cmd/devel/net_convert.py b/cloudinit/cmd/devel/net_convert.py
2index 1ad7e0b..9b76830 100755
3--- a/cloudinit/cmd/devel/net_convert.py
4+++ b/cloudinit/cmd/devel/net_convert.py
5@@ -5,13 +5,12 @@ import argparse
6 import json
7 import os
8 import sys
9-import yaml
10
11 from cloudinit.sources.helpers import openstack
12 from cloudinit.sources import DataSourceAzure as azure
13 from cloudinit.sources import DataSourceOVF as ovf
14
15-from cloudinit import distros
16+from cloudinit import distros, safeyaml
17 from cloudinit.net import eni, netplan, network_state, sysconfig
18 from cloudinit import log
19
20@@ -78,13 +77,12 @@ def handle_args(name, args):
21 if args.kind == "eni":
22 pre_ns = eni.convert_eni_data(net_data)
23 elif args.kind == "yaml":
24- pre_ns = yaml.load(net_data)
25+ pre_ns = safeyaml.load(net_data)
26 if 'network' in pre_ns:
27 pre_ns = pre_ns.get('network')
28 if args.debug:
29 sys.stderr.write('\n'.join(
30- ["Input YAML",
31- yaml.dump(pre_ns, default_flow_style=False, indent=4), ""]))
32+ ["Input YAML", safeyaml.dumps(pre_ns), ""]))
33 elif args.kind == 'network_data.json':
34 pre_ns = openstack.convert_net_json(
35 json.loads(net_data), known_macs=known_macs)
36@@ -100,9 +98,8 @@ def handle_args(name, args):
37 "input data")
38
39 if args.debug:
40- sys.stderr.write('\n'.join([
41- "", "Internal State",
42- yaml.dump(ns, default_flow_style=False, indent=4), ""]))
43+ sys.stderr.write('\n'.join(
44+ ["", "Internal State", safeyaml.dumps(ns), ""]))
45 distro_cls = distros.fetch(args.distro)
46 distro = distro_cls(args.distro, {}, None)
47 config = {}
48diff --git a/cloudinit/cmd/tests/test_main.py b/cloudinit/cmd/tests/test_main.py
49index a1e534f..57b8fdf 100644
50--- a/cloudinit/cmd/tests/test_main.py
51+++ b/cloudinit/cmd/tests/test_main.py
52@@ -6,8 +6,9 @@ import os
53 from six import StringIO
54
55 from cloudinit.cmd import main
56+from cloudinit import safeyaml
57 from cloudinit.util import (
58- ensure_dir, load_file, write_file, yaml_dumps)
59+ ensure_dir, load_file, write_file)
60 from cloudinit.tests.helpers import (
61 FilesystemMockingTestCase, wrap_and_call)
62
63@@ -39,7 +40,7 @@ class TestMain(FilesystemMockingTestCase):
64 ],
65 'cloud_init_modules': ['write-files', 'runcmd'],
66 }
67- cloud_cfg = yaml_dumps(self.cfg)
68+ cloud_cfg = safeyaml.dumps(self.cfg)
69 ensure_dir(os.path.join(self.new_root, 'etc', 'cloud'))
70 self.cloud_cfg_file = os.path.join(
71 self.new_root, 'etc', 'cloud', 'cloud.cfg')
72@@ -113,7 +114,7 @@ class TestMain(FilesystemMockingTestCase):
73 """When local-hostname metadata is present, call cc_set_hostname."""
74 self.cfg['datasource'] = {
75 'None': {'metadata': {'local-hostname': 'md-hostname'}}}
76- cloud_cfg = yaml_dumps(self.cfg)
77+ cloud_cfg = safeyaml.dumps(self.cfg)
78 write_file(self.cloud_cfg_file, cloud_cfg)
79 cmdargs = myargs(
80 debug=False, files=None, force=False, local=False, reporter=None,
81diff --git a/cloudinit/config/cc_debug.py b/cloudinit/config/cc_debug.py
82index 0a039eb..610dbc8 100644
83--- a/cloudinit/config/cc_debug.py
84+++ b/cloudinit/config/cc_debug.py
85@@ -33,6 +33,7 @@ from six import StringIO
86
87 from cloudinit import type_utils
88 from cloudinit import util
89+from cloudinit import safeyaml
90
91 SKIP_KEYS = frozenset(['log_cfgs'])
92
93@@ -49,7 +50,7 @@ def _make_header(text):
94
95
96 def _dumps(obj):
97- text = util.yaml_dumps(obj, explicit_start=False, explicit_end=False)
98+ text = safeyaml.dumps(obj, explicit_start=False, explicit_end=False)
99 return text.rstrip()
100
101
102diff --git a/cloudinit/config/cc_salt_minion.py b/cloudinit/config/cc_salt_minion.py
103index d6a21d7..cd9cb0b 100644
104--- a/cloudinit/config/cc_salt_minion.py
105+++ b/cloudinit/config/cc_salt_minion.py
106@@ -45,7 +45,7 @@ specify them with ``pkg_name``, ``service_name`` and ``config_dir``.
107
108 import os
109
110-from cloudinit import util
111+from cloudinit import safeyaml, util
112
113 # Note: see https://docs.saltstack.com/en/latest/topics/installation/
114 # Note: see https://docs.saltstack.com/en/latest/ref/configuration/
115@@ -97,13 +97,13 @@ def handle(name, cfg, cloud, log, _args):
116 if 'conf' in s_cfg:
117 # Add all sections from the conf object to minion config file
118 minion_config = os.path.join(const.conf_dir, 'minion')
119- minion_data = util.yaml_dumps(s_cfg.get('conf'))
120+ minion_data = safeyaml.dumps(s_cfg.get('conf'))
121 util.write_file(minion_config, minion_data)
122
123 if 'grains' in s_cfg:
124 # add grains to /etc/salt/grains
125 grains_config = os.path.join(const.conf_dir, 'grains')
126- grains_data = util.yaml_dumps(s_cfg.get('grains'))
127+ grains_data = safeyaml.dumps(s_cfg.get('grains'))
128 util.write_file(grains_config, grains_data)
129
130 # ... copy the key pair if specified
131diff --git a/cloudinit/config/cc_snappy.py b/cloudinit/config/cc_snappy.py
132index 15bee2d..b94cd04 100644
133--- a/cloudinit/config/cc_snappy.py
134+++ b/cloudinit/config/cc_snappy.py
135@@ -68,6 +68,7 @@ is ``auto``. Options are:
136 from cloudinit import log as logging
137 from cloudinit.settings import PER_INSTANCE
138 from cloudinit import temp_utils
139+from cloudinit import safeyaml
140 from cloudinit import util
141
142 import glob
143@@ -188,7 +189,7 @@ def render_snap_op(op, name, path=None, cfgfile=None, config=None):
144 # Note, however, we do not touch config files on disk.
145 nested_cfg = {'config': {shortname: config}}
146 (fd, cfg_tmpf) = temp_utils.mkstemp()
147- os.write(fd, util.yaml_dumps(nested_cfg).encode())
148+ os.write(fd, safeyaml.dumps(nested_cfg).encode())
149 os.close(fd)
150 cfgfile = cfg_tmpf
151
152diff --git a/cloudinit/handlers/cloud_config.py b/cloudinit/handlers/cloud_config.py
153index 99bf0e6..2a30736 100644
154--- a/cloudinit/handlers/cloud_config.py
155+++ b/cloudinit/handlers/cloud_config.py
156@@ -14,6 +14,7 @@ from cloudinit import handlers
157 from cloudinit import log as logging
158 from cloudinit import mergers
159 from cloudinit import util
160+from cloudinit import safeyaml
161
162 from cloudinit.settings import (PER_ALWAYS)
163
164@@ -75,7 +76,7 @@ class CloudConfigPartHandler(handlers.Handler):
165 '',
166 ]
167 lines.extend(file_lines)
168- lines.append(util.yaml_dumps(self.cloud_buf))
169+ lines.append(safeyaml.dumps(self.cloud_buf))
170 else:
171 lines = []
172 util.write_file(self.cloud_fn, "\n".join(lines), 0o600)
173diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
174index e54a34e..54be122 100644
175--- a/cloudinit/net/netplan.py
176+++ b/cloudinit/net/netplan.py
177@@ -8,6 +8,7 @@ from .network_state import subnet_is_ipv6, NET_CONFIG_TO_V2
178
179 from cloudinit import log as logging
180 from cloudinit import util
181+from cloudinit import safeyaml
182 from cloudinit.net import SYS_CLASS_NET, get_devicelist
183
184 KNOWN_SNAPD_CONFIG = b"""\
185@@ -235,9 +236,9 @@ class Renderer(renderer.Renderer):
186 # if content already in netplan format, pass it back
187 if network_state.version == 2:
188 LOG.debug('V2 to V2 passthrough')
189- return util.yaml_dumps({'network': network_state.config},
190- explicit_start=False,
191- explicit_end=False)
192+ return safeyaml.dumps({'network': network_state.config},
193+ explicit_start=False,
194+ explicit_end=False)
195
196 ethernets = {}
197 wifis = {}
198@@ -359,10 +360,10 @@ class Renderer(renderer.Renderer):
199 # workaround yaml dictionary key sorting when dumping
200 def _render_section(name, section):
201 if section:
202- dump = util.yaml_dumps({name: section},
203- explicit_start=False,
204- explicit_end=False,
205- noalias=True)
206+ dump = safeyaml.dumps({name: section},
207+ explicit_start=False,
208+ explicit_end=False,
209+ noalias=True)
210 txt = util.indent(dump, ' ' * 4)
211 return [txt]
212 return []
213diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
214index c0c415d..b485f3d 100644
215--- a/cloudinit/net/network_state.py
216+++ b/cloudinit/net/network_state.py
217@@ -12,6 +12,7 @@ import struct
218
219 import six
220
221+from cloudinit import safeyaml
222 from cloudinit import util
223
224 LOG = logging.getLogger(__name__)
225@@ -253,7 +254,7 @@ class NetworkStateInterpreter(object):
226 'config': self._config,
227 'network_state': self._network_state,
228 }
229- return util.yaml_dumps(state)
230+ return safeyaml.dumps(state)
231
232 def load(self, state):
233 if 'version' not in state:
234@@ -272,7 +273,7 @@ class NetworkStateInterpreter(object):
235 setattr(self, key, state[key])
236
237 def dump_network_state(self):
238- return util.yaml_dumps(self._network_state)
239+ return safeyaml.dumps(self._network_state)
240
241 def as_dict(self):
242 return {'version': self._version, 'config': self._config}
243diff --git a/cloudinit/safeyaml.py b/cloudinit/safeyaml.py
244index 3bd5e03..d6f5f95 100644
245--- a/cloudinit/safeyaml.py
246+++ b/cloudinit/safeyaml.py
247@@ -6,6 +6,8 @@
248
249 import yaml
250
251+YAMLError = yaml.YAMLError
252+
253
254 class _CustomSafeLoader(yaml.SafeLoader):
255 def construct_python_unicode(self, node):
256@@ -27,4 +29,17 @@ class NoAliasSafeDumper(yaml.dumper.SafeDumper):
257 def load(blob):
258 return(yaml.load(blob, Loader=_CustomSafeLoader))
259
260+
261+def dumps(obj, explicit_start=True, explicit_end=True, noalias=False):
262+ """Return data in nicely formatted yaml."""
263+
264+ return yaml.dump(obj,
265+ line_break="\n",
266+ indent=4,
267+ explicit_start=explicit_start,
268+ explicit_end=explicit_end,
269+ default_flow_style=False,
270+ Dumper=(NoAliasSafeDumper
271+ if noalias else yaml.dumper.Dumper))
272+
273 # vi: ts=4 expandtab
274diff --git a/cloudinit/util.py b/cloudinit/util.py
275index 0d338ca..1f600df 100644
276--- a/cloudinit/util.py
277+++ b/cloudinit/util.py
278@@ -38,7 +38,6 @@ from base64 import b64decode, b64encode
279 from six.moves.urllib import parse as urlparse
280
281 import six
282-import yaml
283
284 from cloudinit import importer
285 from cloudinit import log as logging
286@@ -958,7 +957,7 @@ def load_yaml(blob, default=None, allowed=(dict,)):
287 " but got %s instead") %
288 (allowed, type_utils.obj_name(converted)))
289 loaded = converted
290- except (yaml.YAMLError, TypeError, ValueError) as e:
291+ except (safeyaml.YAMLError, TypeError, ValueError) as e:
292 msg = 'Failed loading yaml blob'
293 mark = None
294 if hasattr(e, 'context_mark') and getattr(e, 'context_mark'):
295@@ -1629,19 +1628,6 @@ def json_dumps(data):
296 raise
297
298
299-def yaml_dumps(obj, explicit_start=True, explicit_end=True, noalias=False):
300- """Return data in nicely formatted yaml."""
301-
302- return yaml.dump(obj,
303- line_break="\n",
304- indent=4,
305- explicit_start=explicit_start,
306- explicit_end=explicit_end,
307- default_flow_style=False,
308- Dumper=(safeyaml.NoAliasSafeDumper
309- if noalias else yaml.dumper.Dumper))
310-
311-
312 def ensure_dir(path, mode=None):
313 if not os.path.isdir(path):
314 # Make the dir and adjust the mode
315diff --git a/tests/unittests/test_data.py b/tests/unittests/test_data.py
316index 3efe7ad..22cf8f2 100644
317--- a/tests/unittests/test_data.py
318+++ b/tests/unittests/test_data.py
319@@ -27,6 +27,7 @@ from cloudinit.settings import (PER_INSTANCE)
320 from cloudinit import sources
321 from cloudinit import stages
322 from cloudinit import user_data as ud
323+from cloudinit import safeyaml
324 from cloudinit import util
325
326 from cloudinit.tests import helpers
327@@ -502,7 +503,7 @@ c: 4
328 data = [{'content': '#cloud-config\npassword: gocubs\n'},
329 {'content': '#cloud-config\nlocale: chicago\n'},
330 {'content': non_decodable}]
331- message = b'#cloud-config-archive\n' + util.yaml_dumps(data).encode()
332+ message = b'#cloud-config-archive\n' + safeyaml.dumps(data).encode()
333
334 self.reRoot()
335 ci = stages.Init()
336diff --git a/tests/unittests/test_runs/test_merge_run.py b/tests/unittests/test_runs/test_merge_run.py
337index d1ac494..ff27a28 100644
338--- a/tests/unittests/test_runs/test_merge_run.py
339+++ b/tests/unittests/test_runs/test_merge_run.py
340@@ -7,6 +7,7 @@ import tempfile
341 from cloudinit.tests import helpers
342
343 from cloudinit.settings import PER_INSTANCE
344+from cloudinit import safeyaml
345 from cloudinit import stages
346 from cloudinit import util
347
348@@ -26,7 +27,7 @@ class TestMergeRun(helpers.FilesystemMockingTestCase):
349 'system_info': {'paths': {'run_dir': new_root}}
350 }
351 ud = helpers.readResource('user_data.1.txt')
352- cloud_cfg = util.yaml_dumps(cfg)
353+ cloud_cfg = safeyaml.dumps(cfg)
354 util.ensure_dir(os.path.join(new_root, 'etc', 'cloud'))
355 util.write_file(os.path.join(new_root, 'etc',
356 'cloud', 'cloud.cfg'), cloud_cfg)
357diff --git a/tests/unittests/test_runs/test_simple_run.py b/tests/unittests/test_runs/test_simple_run.py
358index d67c422..cb3aae6 100644
359--- a/tests/unittests/test_runs/test_simple_run.py
360+++ b/tests/unittests/test_runs/test_simple_run.py
361@@ -5,6 +5,7 @@ import os
362
363
364 from cloudinit.settings import PER_INSTANCE
365+from cloudinit import safeyaml
366 from cloudinit import stages
367 from cloudinit.tests import helpers
368 from cloudinit import util
369@@ -34,7 +35,7 @@ class TestSimpleRun(helpers.FilesystemMockingTestCase):
370 ],
371 'cloud_init_modules': ['write-files', 'spacewalk', 'runcmd'],
372 }
373- cloud_cfg = util.yaml_dumps(self.cfg)
374+ cloud_cfg = safeyaml.dumps(self.cfg)
375 util.ensure_dir(os.path.join(self.new_root, 'etc', 'cloud'))
376 util.write_file(os.path.join(self.new_root, 'etc',
377 'cloud', 'cloud.cfg'), cloud_cfg)
378@@ -130,7 +131,7 @@ class TestSimpleRun(helpers.FilesystemMockingTestCase):
379 # re-write cloud.cfg with unverified_modules override
380 cfg = copy.deepcopy(self.cfg)
381 cfg['unverified_modules'] = ['spacewalk'] # Would have skipped
382- cloud_cfg = util.yaml_dumps(cfg)
383+ cloud_cfg = safeyaml.dumps(cfg)
384 util.ensure_dir(os.path.join(self.new_root, 'etc', 'cloud'))
385 util.write_file(os.path.join(self.new_root, 'etc',
386 'cloud', 'cloud.cfg'), cloud_cfg)
387@@ -159,7 +160,7 @@ class TestSimpleRun(helpers.FilesystemMockingTestCase):
388 cfg = copy.deepcopy(self.cfg)
389 # Represent empty configuration in /etc/cloud/cloud.cfg
390 cfg['cloud_init_modules'] = None
391- cloud_cfg = util.yaml_dumps(cfg)
392+ cloud_cfg = safeyaml.dumps(cfg)
393 util.ensure_dir(os.path.join(self.new_root, 'etc', 'cloud'))
394 util.write_file(os.path.join(self.new_root, 'etc',
395 'cloud', 'cloud.cfg'), cloud_cfg)

Subscribers

People subscribed via source and target branches