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

Proposed by Scott Moser on 2019-10-24
Status: Merged
Approved by: Ryan Harper on 2019-10-24
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 2019-10-24 Approve on 2019-10-24
Server Team CI bot continuous-integration Approve on 2019-10-24
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.
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() ?

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.

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.

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)
e69df5d... by Scott Moser on 2019-10-24

remove 'import yaml' from cloudinit.util

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.

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)

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)
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.

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