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

[Jeroen T. Vermeulen]

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.

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

[Gavin Panella (allenap)]

Nice :)

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks!

=== modified file 'src/provisioningserver/__main__.py'
--- src/provisioningserver/__main__.py	2014-10-09 19:48:37 +0000
+++ src/provisioningserver/__main__.py	2014-11-07 18:13:30 +0000
@@ -17,6 +17,7 @@
 from provisioningserver import security
 import provisioningserver.boot.install_bootloader
 import provisioningserver.boot.install_grub
+import provisioningserver.configure_maas_url
 import provisioningserver.customize_config
 import provisioningserver.dhcp.writer
 import provisioningserver.upgrade_cluster
@@ -29,6 +30,7 @@
 script_commands = {
     'atomic-write': AtomicWriteScript,
     'check-for-shared-secret': security.CheckForSharedSecretScript,
+    'configure-maas-url': provisioningserver.configure_maas_url,
     'customize-config': provisioningserver.customize_config,
     'generate-dhcp-config': provisioningserver.dhcp.writer,
     'install-shared-secret': security.InstallSharedSecretScript,

=== added file 'src/provisioningserver/configure_maas_url.py'
--- src/provisioningserver/configure_maas_url.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/configure_maas_url.py	2014-11-07 18:13:30 +0000
@@ -0,0 +1,123 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Management command: update `MAAS_URL`.
+
+The MAAS cluster controller packaging calls this in order to set a new
+"MAAS URL" (the URL where nodes and cluster controllers can reach the
+region controller) in the cluster controller's configuration files.
+"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'add_arguments',
+    'run',
+    ]
+
+from functools import partial
+import re
+from urlparse import urlparse
+
+from provisioningserver.utils.fs import (
+    atomic_write,
+    read_text_file,
+    )
+from provisioningserver.utils.url import compose_URL
+
+
+MAAS_CLUSTER_CONF = '/etc/maas/maas_cluster.conf'
+
+PSERV_YAML = '/etc/maas/pserv.yaml'
+
+
+def rewrite_config_file(path, line_filter, mode=0600):
+    """Rewrite config file at `path` on a line-by-line basis.
+
+    Reads the file at `path`, runs its lines through `line_filter`, and
+    writes the result back to `path`.
+
+    Newlines may not be exactly as they were.  A trailing newline is ensured.
+
+    :param path: Path to the config file to be rewritten.
+    :param line_filter: A callable which accepts a line of input text (without
+        trailing newline), and returns the corresponding line of output text
+        (also without trailing newline).
+    :param mode: File access permissions for the newly written file.
+    """
+    input_lines = read_text_file(path).splitlines()
+    output_lines = [line_filter(line) for line in input_lines]
+    result = '%s\n' % '\n'.join(output_lines)
+    atomic_write(result, path, mode=mode)
+
+
+def update_maas_cluster_conf(url):
+    """Update `MAAS_URL` in `/etc/maas/maas_cluster.conf`.
+
+    This file contains a shell-style assignment of the `MAAS_URL`
+    variable.  Its assigned value will be changed to `url`.
+    """
+    substitute_line = lambda line: (
+        'MAAS_URL="%s"' % url
+        if re.match('\s*MAAS_URL=', line)
+        else line)
+    rewrite_config_file(MAAS_CLUSTER_CONF, substitute_line, mode=0640)
+
+
+def extract_host(url):
+    """Return just the host part of `url`."""
+    return urlparse(url).hostname
+
+
+def substitute_pserv_yaml_line(new_host, line):
+    match = re.match('(\s*generator:)\s+(\S*)(.*)$', line)
+    if match is None:
+        # Not the generator line.  Keep as-is.
+        return line
+    [head, input_url, tail] = match.groups()
+    return "%s %s%s" % (head, compose_URL(input_url, new_host), tail)
+
+
+def update_pserv_yaml(host):
+    """Update `generator` in `/etc/maas/pserv.yaml`.
+
+    This file contains a YAML line defining a `generator` URL.  The line must
+    look something like::
+
+        generator: http://10.9.8.7/MAAS/api/1.0/pxeconfig/
+
+    The host part of the URL (in this example, `10.9.8.7`) will be replaced
+    with the new `host`.  If `host` is an IPv6 address, this function will
+    ensure that it is surrounded by square brackets.
+    """
+    substitute_line = partial(substitute_pserv_yaml_line, host)
+    rewrite_config_file(PSERV_YAML, substitute_line, mode=0644)
+
+
+def add_arguments(parser):
+    """Add this command's options to the `ArgumentParser`.
+
+    Specified by the `ActionScript` interface.
+    """
+    parser.add_argument(
+        'maas_url', metavar='URL',
+        help=(
+            "URL where nodes and cluster controllers can reach the MAAS "
+            "region controller."))
+
+
+def run(args):
+    """Update MAAS_URL setting in configuration files.
+
+    For use by the MAAS packaging scripts.  Updates configuration files
+    to reflect a new MAAS_URL setting.
+    """
+    update_maas_cluster_conf(args.maas_url)
+    update_pserv_yaml(extract_host(args.maas_url))

=== added file 'src/provisioningserver/tests/test_configure_maas_url.py'
--- src/provisioningserver/tests/test_configure_maas_url.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/tests/test_configure_maas_url.py	2014-11-07 18:13:30 +0000
@@ -0,0 +1,351 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `MAAS_URL` configuration update code."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+from argparse import ArgumentParser
+from random import randint
+from textwrap import dedent
+
+from maastesting.factory import factory
+from maastesting.matchers import (
+    MockAnyCall,
+    MockCalledOnceWith,
+    )
+from maastesting.testcase import MAASTestCase
+from mock import (
+    ANY,
+    Mock,
+    )
+from provisioningserver import configure_maas_url
+from provisioningserver.configure_maas_url import substitute_pserv_yaml_line
+from testtools.matchers import (
+    FileContains,
+    StartsWith,
+    )
+
+
+class TestRewriteConfigFile(MAASTestCase):
+
+    def test__rewrites_file(self):
+        path = self.make_file(contents='foo\n')
+        configure_maas_url.rewrite_config_file(path, lambda line: 'bar')
+        self.assertThat(path, FileContains('bar\n'))
+
+    def test__sets_access_permissions(self):
+        writer = self.patch(configure_maas_url, 'atomic_write')
+        mode = 0215
+        path = self.make_file()
+        configure_maas_url.rewrite_config_file(
+            path, lambda line: line, mode=mode)
+        self.assertThat(writer, MockCalledOnceWith(ANY, path, mode=mode))
+
+    def test__preserves_trailing_newline(self):
+        path = self.make_file(contents='x\n')
+        configure_maas_url.rewrite_config_file(path, lambda line: line)
+        self.assertThat(path, FileContains('x\n'))
+
+    def test__ensures_trailing_newline(self):
+        path = self.make_file(contents='x')
+        configure_maas_url.rewrite_config_file(path, lambda line: line)
+        self.assertThat(path, FileContains('x\n'))
+
+
+class TestUpdateMAASClusterConf(MAASTestCase):
+
+    def patch_file(self, content):
+        """Inject a fake `/etc/maas/maas_cluster.conf`."""
+        path = self.make_file(name='maas_cluster.conf', contents=content)
+        self.patch(configure_maas_url, 'MAAS_CLUSTER_CONF', path)
+        return path
+
+    def test__updates_realistic_file(self):
+        config_file = self.patch_file(dedent("""\
+            # Leading comments.
+            MAAS_URL="http://10.9.8.7/MAAS"
+            CLUSTER_UUID="5d02950e-6318-8195-ac3e-e6ccb12673c5"
+            """))
+        configure_maas_url.update_maas_cluster_conf('http://1.2.3.4/MAAS')
+        self.assertThat(
+            config_file,
+            FileContains(dedent("""\
+                # Leading comments.
+                MAAS_URL="http://1.2.3.4/MAAS"
+                CLUSTER_UUID="5d02950e-6318-8195-ac3e-e6ccb12673c5"
+                """)))
+
+    def test__updates_quoted_value(self):
+        old_url = factory.make_url()
+        new_url = factory.make_url()
+        config_file = self.patch_file('MAAS_URL="%s"\n' % old_url)
+        configure_maas_url.update_maas_cluster_conf(new_url)
+        self.assertThat(
+            config_file,
+            FileContains('MAAS_URL="%s"\n' % new_url))
+
+    def test__updates_unquoted_value(self):
+        old_url = factory.make_url()
+        new_url = factory.make_url()
+        config_file = self.patch_file('MAAS_URL=%s\n' % old_url)
+        configure_maas_url.update_maas_cluster_conf(new_url)
+        self.assertThat(
+            config_file,
+            FileContains('MAAS_URL="%s"\n' % new_url))
+
+    def test__leaves_other_lines_unchanged(self):
+        old_content = '#MAAS_URL="%s"\n' % factory.make_url()
+        config_file = self.patch_file(old_content)
+        configure_maas_url.update_maas_cluster_conf(factory.make_url())
+        self.assertThat(config_file, FileContains(old_content))
+
+
+class TestExtractHost(MAASTestCase):
+
+    def test__extracts_hostname(self):
+        host = factory.make_name('host').lower()
+        port = factory.pick_port()
+        self.assertEqual(
+            host,
+            configure_maas_url.extract_host('http://%s/path' % host))
+        self.assertEqual(
+            host,
+            configure_maas_url.extract_host('http://%s:%d' % (host, port)))
+
+    def test__extracts_IPv4_address(self):
+        host = factory.make_ipv4_address()
+        port = factory.pick_port()
+        self.assertEqual(
+            host,
+            configure_maas_url.extract_host('http://%s' % host))
+        self.assertEqual(
+            host,
+            configure_maas_url.extract_host('http://%s:%d' % (host, port)))
+
+    def test__extracts_IPv6_address(self):
+        host = factory.make_ipv6_address()
+        port = factory.pick_port()
+        self.assertEqual(
+            host,
+            configure_maas_url.extract_host('http://[%s]' % host))
+        self.assertEqual(
+            host,
+            configure_maas_url.extract_host('http://[%s]:%d' % (host, port)))
+
+    def test__extracts_IPv6_address_with_zone_index(self):
+        host = (
+            factory.make_ipv6_address() +
+            '%25' +
+            factory.make_name('zone').lower())
+        port = factory.pick_port()
+        self.assertEqual(
+            host,
+            configure_maas_url.extract_host('http://[%s]' % host))
+        self.assertEqual(
+            host,
+            configure_maas_url.extract_host('http://[%s]:%d' % (host, port)))
+
+
+class TestSubstitutePservYamlLine(MAASTestCase):
+
+    def make_generator_line(self, url):
+        return "  generator: %s" % url
+
+    def test__replaces_hostname_generator_URL(self):
+        old_host = factory.make_name('old-host')
+        new_host = factory.make_name('new-host')
+        input_line = self.make_generator_line('http://%s' % old_host)
+        self.assertEqual(
+            self.make_generator_line('http://%s' % new_host),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__replaces_IPv4_generator_URL(self):
+        old_host = factory.make_ipv4_address()
+        new_host = factory.make_name('new-host')
+        input_line = self.make_generator_line('http://%s' % old_host)
+        self.assertEqual(
+            self.make_generator_line('http://%s' % new_host),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__replaces_IPv6_generator_URL(self):
+        old_host = factory.make_ipv6_address()
+        new_host = factory.make_name('new-host')
+        input_line = self.make_generator_line('http://[%s]' % old_host)
+        self.assertEqual(
+            self.make_generator_line('http://%s' % new_host),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__replaces_IPv6_generator_URL_with_zone_index(self):
+        old_host = (
+            factory.make_ipv6_address() +
+            '%25' +
+            factory.make_name('zone')
+            )
+        new_host = factory.make_name('new-host')
+        input_line = self.make_generator_line('http://[%s]' % old_host)
+        self.assertEqual(
+            self.make_generator_line('http://%s' % new_host),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__inserts_IPv6_with_brackets(self):
+        old_host = factory.make_name('old-host')
+        new_host = '[%s]' % factory.make_ipv6_address()
+        input_line = self.make_generator_line('http://%s' % old_host)
+        self.assertEqual(
+            self.make_generator_line('http://%s' % new_host),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__inserts_IPv6_without_brackets(self):
+        old_host = factory.make_name('old-host')
+        new_host = factory.make_ipv6_address()
+        input_line = self.make_generator_line('http://%s' % old_host)
+        self.assertEqual(
+            self.make_generator_line('http://[%s]' % new_host),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__preserves_port_after_simple_host(self):
+        port = factory.pick_port()
+        old_host = factory.make_name('old-host')
+        new_host = factory.make_name('new-host')
+        input_line = self.make_generator_line(
+            'http://%s:%d' % (old_host, port))
+        self.assertEqual(
+            self.make_generator_line('http://%s:%d' % (new_host, port)),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__preserves_port_with_IPv6(self):
+        port = factory.pick_port()
+        old_host = factory.make_ipv6_address()
+        new_host = factory.make_name('new-host')
+        input_line = self.make_generator_line(
+            'http://[%s]:%d' % (old_host, port))
+        self.assertEqual(
+            self.make_generator_line('http://%s:%d' % (new_host, port)),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__preserves_port_with_IPv6_and_zone_index(self):
+        port = factory.pick_port()
+        old_host = (
+            factory.make_ipv6_address() +
+            '%25' +
+            factory.make_name('zone')
+            )
+        new_host = factory.make_name('new-host')
+        input_line = self.make_generator_line(
+            'http://[%s]:%d' % (old_host, port))
+        self.assertEqual(
+            self.make_generator_line('http://%s:%d' % (new_host, port)),
+            substitute_pserv_yaml_line(new_host, input_line))
+
+    def test__preserves_other_line(self):
+        line = '#' + self.make_generator_line(factory.make_url())
+        self.assertEqual(
+            line,
+            substitute_pserv_yaml_line(factory.make_name('host'), line))
+
+    def test__preserves_indentation(self):
+        spaces = ' ' * randint(0, 10)
+        input_line = spaces + 'generator: %s' % factory.make_url()
+        output_line = substitute_pserv_yaml_line(
+            factory.make_name('host'), input_line)
+        self.assertThat(output_line, StartsWith(spaces + 'generator:'))
+
+    def test__preserves_trailing_comments(self):
+        comment = " # Trailing comment."
+        old_host = factory.make_name('old-host')
+        new_host = factory.make_name('new-host')
+        input_line = self.make_generator_line('http://%s' % old_host) + comment
+        self.assertEqual(
+            self.make_generator_line('http://%s' % new_host) + comment,
+            substitute_pserv_yaml_line(new_host, input_line))
+
+
+class TestUpdatePservYaml(MAASTestCase):
+
+    def patch_file(self, content):
+        """Inject a fake `/etc/maas/pserv.yaml`."""
+        path = self.make_file(name='pserv.yaml', contents=content)
+        self.patch(configure_maas_url, 'PSERV_YAML', path)
+        return path
+
+    def test__updates_realistic_file(self):
+        old_host = factory.make_name('old-host')
+        new_host = factory.make_name('new-host')
+        config_file = self.patch_file(dedent("""\
+            ## TFTP configuration.
+            tftp:
+              ## The URL to be contacted to generate PXE configurations.
+              generator: http://%s/MAAS/api/1.0/pxeconfig/
+            """) % old_host)
+        configure_maas_url.update_pserv_yaml(new_host)
+        self.assertThat(
+            config_file,
+            FileContains(dedent("""\
+                ## TFTP configuration.
+                tftp:
+                  ## The URL to be contacted to generate PXE configurations.
+                  generator: http://%s/MAAS/api/1.0/pxeconfig/
+                """) % new_host))
+
+
+class TestAddArguments(MAASTestCase):
+
+    def test__accepts_maas_url(self):
+        url = factory.make_url()
+        parser = ArgumentParser()
+        configure_maas_url.add_arguments(parser)
+        args = parser.parse_args([url])
+        self.assertEqual(url, args.maas_url)
+
+
+class TestRun(MAASTestCase):
+
+    def make_args(self, maas_url):
+        args = Mock()
+        args.maas_url = maas_url
+        return args
+
+    def patch_read_file(self):
+        return self.patch(configure_maas_url, 'read_text_file')
+
+    def patch_write_file(self):
+        return self.patch(configure_maas_url, 'atomic_write')
+
+    def test__updates_maas_cluster_conf(self):
+        reader = self.patch_read_file()
+        writer = self.patch_write_file()
+        url = factory.make_url()
+        configure_maas_url.run(self.make_args(url))
+        self.assertThat(reader, MockAnyCall('/etc/maas/maas_cluster.conf'))
+        self.assertThat(
+            writer,
+            MockAnyCall(ANY, '/etc/maas/maas_cluster.conf', mode=0640))
+
+    def test__updates_pserv_yaml(self):
+        reader = self.patch_read_file()
+        writer = self.patch_write_file()
+        url = factory.make_url()
+        configure_maas_url.run(self.make_args(url))
+        self.assertThat(reader, MockAnyCall('/etc/maas/pserv.yaml'))
+        self.assertThat(
+            writer,
+            MockAnyCall(ANY, '/etc/maas/pserv.yaml', mode=0644))
+
+    def test__passes_host_to_update_pserv_yaml(self):
+        self.patch_read_file()
+        self.patch_write_file()
+        update_pserv_yaml = self.patch(configure_maas_url, 'update_pserv_yaml')
+        host = factory.make_name('host').lower()
+        url = factory.make_url(netloc=host)
+        configure_maas_url.run(self.make_args(url))
+        self.assertThat(update_pserv_yaml, MockCalledOnceWith(host))