Merge lp:~jtv/maas/bug-1373261 into lp:maas/trunk

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: 3364
Merged at revision: 3352
Proposed branch: lp:~jtv/maas/bug-1373261
Merge into: lp:maas/trunk
Diff against target: 503 lines (+476/-0)
3 files modified
src/provisioningserver/__main__.py (+2/-0)
src/provisioningserver/configure_maas_url.py (+123/-0)
src/provisioningserver/tests/test_configure_maas_url.py (+351/-0)
To merge this branch: bzr merge lp:~jtv/maas/bug-1373261
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+241123@code.launchpad.net

Commit message

Python replacement for maas-cluster-controller postinst code to rewrite maas_cluster.conf and pserv.yaml. To be invoked as a maas-provision sub-command.

This is lots and lots more work than the old shell code, but it has two advantages: it's covered by the test suite, and it supports IPv6 addresses in URLs. These have a special syntax, and caused our shell code to corrupt pserv.yaml.

Description of the change

The problem with the shell code was that it mistook any colon (":") in the generator URL for the beginning of a port specifier. Thus, once you had an IPv6-based URL configured, reconfiguring for a different host would produce garbage in pserv.yaml.

While I was at it, I opted to handle both config files that this shell code rewrites. The one that needed fixing was the hard one anyway, so replacing a bit more shell code was a cheap bonus.

An accompanying packaging change will replace the postinst shell code with an invocation of the new maas-provision sub-command.

Jeroen

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Nice :)

review: Approve
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Thanks!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/__main__.py'
2--- src/provisioningserver/__main__.py 2014-10-09 19:48:37 +0000
3+++ src/provisioningserver/__main__.py 2014-11-07 18:13:30 +0000
4@@ -17,6 +17,7 @@
5 from provisioningserver import security
6 import provisioningserver.boot.install_bootloader
7 import provisioningserver.boot.install_grub
8+import provisioningserver.configure_maas_url
9 import provisioningserver.customize_config
10 import provisioningserver.dhcp.writer
11 import provisioningserver.upgrade_cluster
12@@ -29,6 +30,7 @@
13 script_commands = {
14 'atomic-write': AtomicWriteScript,
15 'check-for-shared-secret': security.CheckForSharedSecretScript,
16+ 'configure-maas-url': provisioningserver.configure_maas_url,
17 'customize-config': provisioningserver.customize_config,
18 'generate-dhcp-config': provisioningserver.dhcp.writer,
19 'install-shared-secret': security.InstallSharedSecretScript,
20
21=== added file 'src/provisioningserver/configure_maas_url.py'
22--- src/provisioningserver/configure_maas_url.py 1970-01-01 00:00:00 +0000
23+++ src/provisioningserver/configure_maas_url.py 2014-11-07 18:13:30 +0000
24@@ -0,0 +1,123 @@
25+# Copyright 2014 Canonical Ltd. This software is licensed under the
26+# GNU Affero General Public License version 3 (see the file LICENSE).
27+
28+"""Management command: update `MAAS_URL`.
29+
30+The MAAS cluster controller packaging calls this in order to set a new
31+"MAAS URL" (the URL where nodes and cluster controllers can reach the
32+region controller) in the cluster controller's configuration files.
33+"""
34+
35+from __future__ import (
36+ absolute_import,
37+ print_function,
38+ unicode_literals,
39+ )
40+
41+str = None
42+
43+__metaclass__ = type
44+__all__ = [
45+ 'add_arguments',
46+ 'run',
47+ ]
48+
49+from functools import partial
50+import re
51+from urlparse import urlparse
52+
53+from provisioningserver.utils.fs import (
54+ atomic_write,
55+ read_text_file,
56+ )
57+from provisioningserver.utils.url import compose_URL
58+
59+
60+MAAS_CLUSTER_CONF = '/etc/maas/maas_cluster.conf'
61+
62+PSERV_YAML = '/etc/maas/pserv.yaml'
63+
64+
65+def rewrite_config_file(path, line_filter, mode=0600):
66+ """Rewrite config file at `path` on a line-by-line basis.
67+
68+ Reads the file at `path`, runs its lines through `line_filter`, and
69+ writes the result back to `path`.
70+
71+ Newlines may not be exactly as they were. A trailing newline is ensured.
72+
73+ :param path: Path to the config file to be rewritten.
74+ :param line_filter: A callable which accepts a line of input text (without
75+ trailing newline), and returns the corresponding line of output text
76+ (also without trailing newline).
77+ :param mode: File access permissions for the newly written file.
78+ """
79+ input_lines = read_text_file(path).splitlines()
80+ output_lines = [line_filter(line) for line in input_lines]
81+ result = '%s\n' % '\n'.join(output_lines)
82+ atomic_write(result, path, mode=mode)
83+
84+
85+def update_maas_cluster_conf(url):
86+ """Update `MAAS_URL` in `/etc/maas/maas_cluster.conf`.
87+
88+ This file contains a shell-style assignment of the `MAAS_URL`
89+ variable. Its assigned value will be changed to `url`.
90+ """
91+ substitute_line = lambda line: (
92+ 'MAAS_URL="%s"' % url
93+ if re.match('\s*MAAS_URL=', line)
94+ else line)
95+ rewrite_config_file(MAAS_CLUSTER_CONF, substitute_line, mode=0640)
96+
97+
98+def extract_host(url):
99+ """Return just the host part of `url`."""
100+ return urlparse(url).hostname
101+
102+
103+def substitute_pserv_yaml_line(new_host, line):
104+ match = re.match('(\s*generator:)\s+(\S*)(.*)$', line)
105+ if match is None:
106+ # Not the generator line. Keep as-is.
107+ return line
108+ [head, input_url, tail] = match.groups()
109+ return "%s %s%s" % (head, compose_URL(input_url, new_host), tail)
110+
111+
112+def update_pserv_yaml(host):
113+ """Update `generator` in `/etc/maas/pserv.yaml`.
114+
115+ This file contains a YAML line defining a `generator` URL. The line must
116+ look something like::
117+
118+ generator: http://10.9.8.7/MAAS/api/1.0/pxeconfig/
119+
120+ The host part of the URL (in this example, `10.9.8.7`) will be replaced
121+ with the new `host`. If `host` is an IPv6 address, this function will
122+ ensure that it is surrounded by square brackets.
123+ """
124+ substitute_line = partial(substitute_pserv_yaml_line, host)
125+ rewrite_config_file(PSERV_YAML, substitute_line, mode=0644)
126+
127+
128+def add_arguments(parser):
129+ """Add this command's options to the `ArgumentParser`.
130+
131+ Specified by the `ActionScript` interface.
132+ """
133+ parser.add_argument(
134+ 'maas_url', metavar='URL',
135+ help=(
136+ "URL where nodes and cluster controllers can reach the MAAS "
137+ "region controller."))
138+
139+
140+def run(args):
141+ """Update MAAS_URL setting in configuration files.
142+
143+ For use by the MAAS packaging scripts. Updates configuration files
144+ to reflect a new MAAS_URL setting.
145+ """
146+ update_maas_cluster_conf(args.maas_url)
147+ update_pserv_yaml(extract_host(args.maas_url))
148
149=== added file 'src/provisioningserver/tests/test_configure_maas_url.py'
150--- src/provisioningserver/tests/test_configure_maas_url.py 1970-01-01 00:00:00 +0000
151+++ src/provisioningserver/tests/test_configure_maas_url.py 2014-11-07 18:13:30 +0000
152@@ -0,0 +1,351 @@
153+# Copyright 2014 Canonical Ltd. This software is licensed under the
154+# GNU Affero General Public License version 3 (see the file LICENSE).
155+
156+"""Tests for `MAAS_URL` configuration update code."""
157+
158+from __future__ import (
159+ absolute_import,
160+ print_function,
161+ unicode_literals,
162+ )
163+
164+str = None
165+
166+__metaclass__ = type
167+__all__ = []
168+
169+from argparse import ArgumentParser
170+from random import randint
171+from textwrap import dedent
172+
173+from maastesting.factory import factory
174+from maastesting.matchers import (
175+ MockAnyCall,
176+ MockCalledOnceWith,
177+ )
178+from maastesting.testcase import MAASTestCase
179+from mock import (
180+ ANY,
181+ Mock,
182+ )
183+from provisioningserver import configure_maas_url
184+from provisioningserver.configure_maas_url import substitute_pserv_yaml_line
185+from testtools.matchers import (
186+ FileContains,
187+ StartsWith,
188+ )
189+
190+
191+class TestRewriteConfigFile(MAASTestCase):
192+
193+ def test__rewrites_file(self):
194+ path = self.make_file(contents='foo\n')
195+ configure_maas_url.rewrite_config_file(path, lambda line: 'bar')
196+ self.assertThat(path, FileContains('bar\n'))
197+
198+ def test__sets_access_permissions(self):
199+ writer = self.patch(configure_maas_url, 'atomic_write')
200+ mode = 0215
201+ path = self.make_file()
202+ configure_maas_url.rewrite_config_file(
203+ path, lambda line: line, mode=mode)
204+ self.assertThat(writer, MockCalledOnceWith(ANY, path, mode=mode))
205+
206+ def test__preserves_trailing_newline(self):
207+ path = self.make_file(contents='x\n')
208+ configure_maas_url.rewrite_config_file(path, lambda line: line)
209+ self.assertThat(path, FileContains('x\n'))
210+
211+ def test__ensures_trailing_newline(self):
212+ path = self.make_file(contents='x')
213+ configure_maas_url.rewrite_config_file(path, lambda line: line)
214+ self.assertThat(path, FileContains('x\n'))
215+
216+
217+class TestUpdateMAASClusterConf(MAASTestCase):
218+
219+ def patch_file(self, content):
220+ """Inject a fake `/etc/maas/maas_cluster.conf`."""
221+ path = self.make_file(name='maas_cluster.conf', contents=content)
222+ self.patch(configure_maas_url, 'MAAS_CLUSTER_CONF', path)
223+ return path
224+
225+ def test__updates_realistic_file(self):
226+ config_file = self.patch_file(dedent("""\
227+ # Leading comments.
228+ MAAS_URL="http://10.9.8.7/MAAS"
229+ CLUSTER_UUID="5d02950e-6318-8195-ac3e-e6ccb12673c5"
230+ """))
231+ configure_maas_url.update_maas_cluster_conf('http://1.2.3.4/MAAS')
232+ self.assertThat(
233+ config_file,
234+ FileContains(dedent("""\
235+ # Leading comments.
236+ MAAS_URL="http://1.2.3.4/MAAS"
237+ CLUSTER_UUID="5d02950e-6318-8195-ac3e-e6ccb12673c5"
238+ """)))
239+
240+ def test__updates_quoted_value(self):
241+ old_url = factory.make_url()
242+ new_url = factory.make_url()
243+ config_file = self.patch_file('MAAS_URL="%s"\n' % old_url)
244+ configure_maas_url.update_maas_cluster_conf(new_url)
245+ self.assertThat(
246+ config_file,
247+ FileContains('MAAS_URL="%s"\n' % new_url))
248+
249+ def test__updates_unquoted_value(self):
250+ old_url = factory.make_url()
251+ new_url = factory.make_url()
252+ config_file = self.patch_file('MAAS_URL=%s\n' % old_url)
253+ configure_maas_url.update_maas_cluster_conf(new_url)
254+ self.assertThat(
255+ config_file,
256+ FileContains('MAAS_URL="%s"\n' % new_url))
257+
258+ def test__leaves_other_lines_unchanged(self):
259+ old_content = '#MAAS_URL="%s"\n' % factory.make_url()
260+ config_file = self.patch_file(old_content)
261+ configure_maas_url.update_maas_cluster_conf(factory.make_url())
262+ self.assertThat(config_file, FileContains(old_content))
263+
264+
265+class TestExtractHost(MAASTestCase):
266+
267+ def test__extracts_hostname(self):
268+ host = factory.make_name('host').lower()
269+ port = factory.pick_port()
270+ self.assertEqual(
271+ host,
272+ configure_maas_url.extract_host('http://%s/path' % host))
273+ self.assertEqual(
274+ host,
275+ configure_maas_url.extract_host('http://%s:%d' % (host, port)))
276+
277+ def test__extracts_IPv4_address(self):
278+ host = factory.make_ipv4_address()
279+ port = factory.pick_port()
280+ self.assertEqual(
281+ host,
282+ configure_maas_url.extract_host('http://%s' % host))
283+ self.assertEqual(
284+ host,
285+ configure_maas_url.extract_host('http://%s:%d' % (host, port)))
286+
287+ def test__extracts_IPv6_address(self):
288+ host = factory.make_ipv6_address()
289+ port = factory.pick_port()
290+ self.assertEqual(
291+ host,
292+ configure_maas_url.extract_host('http://[%s]' % host))
293+ self.assertEqual(
294+ host,
295+ configure_maas_url.extract_host('http://[%s]:%d' % (host, port)))
296+
297+ def test__extracts_IPv6_address_with_zone_index(self):
298+ host = (
299+ factory.make_ipv6_address() +
300+ '%25' +
301+ factory.make_name('zone').lower())
302+ port = factory.pick_port()
303+ self.assertEqual(
304+ host,
305+ configure_maas_url.extract_host('http://[%s]' % host))
306+ self.assertEqual(
307+ host,
308+ configure_maas_url.extract_host('http://[%s]:%d' % (host, port)))
309+
310+
311+class TestSubstitutePservYamlLine(MAASTestCase):
312+
313+ def make_generator_line(self, url):
314+ return " generator: %s" % url
315+
316+ def test__replaces_hostname_generator_URL(self):
317+ old_host = factory.make_name('old-host')
318+ new_host = factory.make_name('new-host')
319+ input_line = self.make_generator_line('http://%s' % old_host)
320+ self.assertEqual(
321+ self.make_generator_line('http://%s' % new_host),
322+ substitute_pserv_yaml_line(new_host, input_line))
323+
324+ def test__replaces_IPv4_generator_URL(self):
325+ old_host = factory.make_ipv4_address()
326+ new_host = factory.make_name('new-host')
327+ input_line = self.make_generator_line('http://%s' % old_host)
328+ self.assertEqual(
329+ self.make_generator_line('http://%s' % new_host),
330+ substitute_pserv_yaml_line(new_host, input_line))
331+
332+ def test__replaces_IPv6_generator_URL(self):
333+ old_host = factory.make_ipv6_address()
334+ new_host = factory.make_name('new-host')
335+ input_line = self.make_generator_line('http://[%s]' % old_host)
336+ self.assertEqual(
337+ self.make_generator_line('http://%s' % new_host),
338+ substitute_pserv_yaml_line(new_host, input_line))
339+
340+ def test__replaces_IPv6_generator_URL_with_zone_index(self):
341+ old_host = (
342+ factory.make_ipv6_address() +
343+ '%25' +
344+ factory.make_name('zone')
345+ )
346+ new_host = factory.make_name('new-host')
347+ input_line = self.make_generator_line('http://[%s]' % old_host)
348+ self.assertEqual(
349+ self.make_generator_line('http://%s' % new_host),
350+ substitute_pserv_yaml_line(new_host, input_line))
351+
352+ def test__inserts_IPv6_with_brackets(self):
353+ old_host = factory.make_name('old-host')
354+ new_host = '[%s]' % factory.make_ipv6_address()
355+ input_line = self.make_generator_line('http://%s' % old_host)
356+ self.assertEqual(
357+ self.make_generator_line('http://%s' % new_host),
358+ substitute_pserv_yaml_line(new_host, input_line))
359+
360+ def test__inserts_IPv6_without_brackets(self):
361+ old_host = factory.make_name('old-host')
362+ new_host = factory.make_ipv6_address()
363+ input_line = self.make_generator_line('http://%s' % old_host)
364+ self.assertEqual(
365+ self.make_generator_line('http://[%s]' % new_host),
366+ substitute_pserv_yaml_line(new_host, input_line))
367+
368+ def test__preserves_port_after_simple_host(self):
369+ port = factory.pick_port()
370+ old_host = factory.make_name('old-host')
371+ new_host = factory.make_name('new-host')
372+ input_line = self.make_generator_line(
373+ 'http://%s:%d' % (old_host, port))
374+ self.assertEqual(
375+ self.make_generator_line('http://%s:%d' % (new_host, port)),
376+ substitute_pserv_yaml_line(new_host, input_line))
377+
378+ def test__preserves_port_with_IPv6(self):
379+ port = factory.pick_port()
380+ old_host = factory.make_ipv6_address()
381+ new_host = factory.make_name('new-host')
382+ input_line = self.make_generator_line(
383+ 'http://[%s]:%d' % (old_host, port))
384+ self.assertEqual(
385+ self.make_generator_line('http://%s:%d' % (new_host, port)),
386+ substitute_pserv_yaml_line(new_host, input_line))
387+
388+ def test__preserves_port_with_IPv6_and_zone_index(self):
389+ port = factory.pick_port()
390+ old_host = (
391+ factory.make_ipv6_address() +
392+ '%25' +
393+ factory.make_name('zone')
394+ )
395+ new_host = factory.make_name('new-host')
396+ input_line = self.make_generator_line(
397+ 'http://[%s]:%d' % (old_host, port))
398+ self.assertEqual(
399+ self.make_generator_line('http://%s:%d' % (new_host, port)),
400+ substitute_pserv_yaml_line(new_host, input_line))
401+
402+ def test__preserves_other_line(self):
403+ line = '#' + self.make_generator_line(factory.make_url())
404+ self.assertEqual(
405+ line,
406+ substitute_pserv_yaml_line(factory.make_name('host'), line))
407+
408+ def test__preserves_indentation(self):
409+ spaces = ' ' * randint(0, 10)
410+ input_line = spaces + 'generator: %s' % factory.make_url()
411+ output_line = substitute_pserv_yaml_line(
412+ factory.make_name('host'), input_line)
413+ self.assertThat(output_line, StartsWith(spaces + 'generator:'))
414+
415+ def test__preserves_trailing_comments(self):
416+ comment = " # Trailing comment."
417+ old_host = factory.make_name('old-host')
418+ new_host = factory.make_name('new-host')
419+ input_line = self.make_generator_line('http://%s' % old_host) + comment
420+ self.assertEqual(
421+ self.make_generator_line('http://%s' % new_host) + comment,
422+ substitute_pserv_yaml_line(new_host, input_line))
423+
424+
425+class TestUpdatePservYaml(MAASTestCase):
426+
427+ def patch_file(self, content):
428+ """Inject a fake `/etc/maas/pserv.yaml`."""
429+ path = self.make_file(name='pserv.yaml', contents=content)
430+ self.patch(configure_maas_url, 'PSERV_YAML', path)
431+ return path
432+
433+ def test__updates_realistic_file(self):
434+ old_host = factory.make_name('old-host')
435+ new_host = factory.make_name('new-host')
436+ config_file = self.patch_file(dedent("""\
437+ ## TFTP configuration.
438+ tftp:
439+ ## The URL to be contacted to generate PXE configurations.
440+ generator: http://%s/MAAS/api/1.0/pxeconfig/
441+ """) % old_host)
442+ configure_maas_url.update_pserv_yaml(new_host)
443+ self.assertThat(
444+ config_file,
445+ FileContains(dedent("""\
446+ ## TFTP configuration.
447+ tftp:
448+ ## The URL to be contacted to generate PXE configurations.
449+ generator: http://%s/MAAS/api/1.0/pxeconfig/
450+ """) % new_host))
451+
452+
453+class TestAddArguments(MAASTestCase):
454+
455+ def test__accepts_maas_url(self):
456+ url = factory.make_url()
457+ parser = ArgumentParser()
458+ configure_maas_url.add_arguments(parser)
459+ args = parser.parse_args([url])
460+ self.assertEqual(url, args.maas_url)
461+
462+
463+class TestRun(MAASTestCase):
464+
465+ def make_args(self, maas_url):
466+ args = Mock()
467+ args.maas_url = maas_url
468+ return args
469+
470+ def patch_read_file(self):
471+ return self.patch(configure_maas_url, 'read_text_file')
472+
473+ def patch_write_file(self):
474+ return self.patch(configure_maas_url, 'atomic_write')
475+
476+ def test__updates_maas_cluster_conf(self):
477+ reader = self.patch_read_file()
478+ writer = self.patch_write_file()
479+ url = factory.make_url()
480+ configure_maas_url.run(self.make_args(url))
481+ self.assertThat(reader, MockAnyCall('/etc/maas/maas_cluster.conf'))
482+ self.assertThat(
483+ writer,
484+ MockAnyCall(ANY, '/etc/maas/maas_cluster.conf', mode=0640))
485+
486+ def test__updates_pserv_yaml(self):
487+ reader = self.patch_read_file()
488+ writer = self.patch_write_file()
489+ url = factory.make_url()
490+ configure_maas_url.run(self.make_args(url))
491+ self.assertThat(reader, MockAnyCall('/etc/maas/pserv.yaml'))
492+ self.assertThat(
493+ writer,
494+ MockAnyCall(ANY, '/etc/maas/pserv.yaml', mode=0644))
495+
496+ def test__passes_host_to_update_pserv_yaml(self):
497+ self.patch_read_file()
498+ self.patch_write_file()
499+ update_pserv_yaml = self.patch(configure_maas_url, 'update_pserv_yaml')
500+ host = factory.make_name('host').lower()
501+ url = factory.make_url(netloc=host)
502+ configure_maas_url.run(self.make_args(url))
503+ self.assertThat(update_pserv_yaml, MockCalledOnceWith(host))