Merge lp:~mpontillo/maas/migrate-dns-settings-1.7 into lp:maas/1.7

[Mike Pontillo]

Backport DNS migration fixes (Merge revision 4077 from trunk)

Original proposal:

https://code.launchpad.net/~mpontillo/maas/migrate-dns-settings-trunk/+merge/263610

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

=== modified file 'docs/changelog.rst'
--- docs/changelog.rst	2015-05-26 16:36:50 +0000
+++ docs/changelog.rst	2015-07-08 00:32:06 +0000
@@ -2,6 +2,17 @@
 Changelog
 =========
 
+1.7.6
+=====
+
+Bug Fix Update
+--------------
+
+#1470585    Accept list of forwarders for upstream_dns rather than just one.
+
+#1413388    Fix upgrade issue where it would remove custom DNS config,
+            potentially breaking DNS.
+
 1.7.5
 =====
 

=== modified file 'src/maasserver/management/commands/edit_named_options.py'
--- src/maasserver/management/commands/edit_named_options.py	2015-07-01 01:38:49 +0000
+++ src/maasserver/management/commands/edit_named_options.py	2015-07-08 00:32:06 +0000
@@ -11,6 +11,8 @@
     print_function,
     unicode_literals,
     )
+from collections import OrderedDict
+
 
 str = None
 
@@ -19,16 +21,20 @@
     'Command',
     ]
 
+from copy import deepcopy
 from datetime import datetime
 from optparse import make_option
 import os
 import shutil
+import sys
 
 from django.core.management.base import (
     BaseCommand,
     CommandError,
     )
+from maasserver.models import Config
 from maasserver.utils.isc import (
+    ISCParseException,
     make_isc_string,
     parse_isc_string,
     )
@@ -42,6 +48,24 @@
             '--config-path', dest='config_path',
             default="/etc/bind/named.conf.options",
             help="Specify the configuration file to edit."),
+        make_option(
+            '--dry-run', dest='dry_run',
+            default=False, action='store_true',
+            help="Do not edit any configuration; instead, print to stdout the "
+                 "actions that would be performed, and/or the new "
+                 "configuration that would be written."),
+        make_option(
+            '--force', dest='force',
+            default=False, action='store_true',
+            help="Force the BIND configuration to be written, even if it "
+                 "appears as though nothing has changed."),
+        make_option(
+            '--migrate-conflicting-options', default=False,
+            dest='migrate_conflicting_options', action='store_true',
+            help="Causes any options that conflict with MAAS-managed options "
+                 "to be deleted from the BIND configuration and moved to the "
+                 "MAAS-managed configuration. Requires the MAAS database to "
+                 "be configured and running."),
     )
     help = (
         "Edit the named.conf.options file so that it includes the "
@@ -66,13 +90,12 @@
         """
         try:
             config_dict = parse_isc_string(options_file)
-        except Exception as e:
-            # Yes, it throws bare exceptions :(
+        except ISCParseException as e:
             raise CommandError("Failed to parse %s: %s" % (
                 config_path, e.message))
         options_block = config_dict.get("options", None)
         if options_block is None:
-            # Something is horribly wrong with the file, bail out rather
+            # Something is horribly wrong with the file; bail out rather
             # than doing anything drastic.
             raise CommandError(
                 "Can't find options {} block in %s, bailing out without "
@@ -85,14 +108,37 @@
         options_block['include'] = '"%s%s%s"' % (
             dir, os.path.sep, MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
 
-    def remove_forwarders(self, options_block):
+    def migrate_forwarders(self, options_block, dry_run, stdout):
         """Remove existing forwarders from the options block.
 
         It's a syntax error to have more than one in the combined
-        configuration for named so we just remove whatever was there.
-        There is no data loss due to the backup file made later.
+        configuration for named, so we just remove whatever was there.
+
+        Migrate any forwarders in the configuration file to the MAAS config.
         """
         if 'forwarders' in options_block:
+            bind_forwarders = options_block['forwarders']
+
+            if not dry_run:
+                config, created = Config.objects.get_or_create(
+                    name='upstream_dns',
+                    defaults={'value': ' '.join(bind_forwarders)})
+                if not created:
+                    # A configuration value already exists, so add the
+                    # additional values we found in the configuration file to
+                    # MAAS.
+                    if config.value is None:
+                        config.value = ''
+                    maas_forwarders = OrderedDict.fromkeys(
+                        config.value.split())
+                    maas_forwarders.update(bind_forwarders)
+                    config.value = ' '.join(maas_forwarders)
+                    config.save()
+            else:
+                stdout.write(
+                    "// Append to MAAS forwarders: %s\n"
+                    % ' '.join(bind_forwarders))
+
             del options_block['forwarders']
 
     def back_up_existing_file(self, config_path):
@@ -104,21 +150,59 @@
             raise CommandError(
                 "Failed to make a backup of %s, exiting: %s" % (
                     config_path, e.message))
+        return backup_destination
+
+    def write_new_named_conf_options(self, fd, backup_filename, new_content):
+        fd.write("""\
+//
+// This file is managed by MAAS. Although MAAS attempts to preserve changes
+// made here, it is possible to create conflicts that MAAS can not resolve.
+//
+// DNS settings available in MAAS (for example, forwarders and
+// dnssec-validation) should be managed only in MAAS.
+//
+// The previous configuration file was backed up at:
+//     %s
+//
+""" % backup_filename)
+        fd.write(new_content)
+        fd.write("\n")
 
     def handle(self, *args, **options):
         """Entry point for BaseCommand."""
         # Read stuff in, validate.
         config_path = options.get('config_path')
+        dry_run = options.get('dry_run')
+        force = options.get('force')
+        stdout = options.get('stdout')
+        if stdout is None:
+            stdout = sys.stdout
+        migrate_conflicting_options = options.get(
+            'migrate_conflicting_options')
+
         options_file = self.read_file(config_path)
         config_dict = self.parse_file(config_path, options_file)
+        original_config = deepcopy(config_dict)
+
         options_block = config_dict['options']
 
-        # Modify the config.
+        # Modify the configuration (if necessary).
         self.set_up_include_statement(options_block, config_path)
-        self.remove_forwarders(options_block)
+
+        if migrate_conflicting_options:
+            self.migrate_forwarders(options_block, dry_run, stdout)
+
+        # Re-parse the new configuration, so we can detect any changes.
         new_content = make_isc_string(config_dict)
+        new_config = parse_isc_string(new_content)
 
-        # Back up and write new file.
-        self.back_up_existing_file(config_path)
-        with open(config_path, "wb") as fd:
-            fd.write(new_content)
+        if original_config != new_config or force:
+            # The configuration has changed. Back up and write new file.
+            if dry_run:
+                self.write_new_named_conf_options(
+                    stdout, config_path, new_content)
+            else:
+                backup_filename = self.back_up_existing_file(config_path)
+                with open(config_path, "wb") as fd:
+                    self.write_new_named_conf_options(
+                        fd, backup_filename, new_content)

=== modified file 'src/maasserver/tests/test_commands_edit_named_options.py'
--- src/maasserver/tests/test_commands_edit_named_options.py	2014-07-18 17:05:57 +0000
+++ src/maasserver/tests/test_commands_edit_named_options.py	2015-07-08 00:32:06 +0000
@@ -15,6 +15,7 @@
 __all__ = []
 
 from codecs import getwriter
+from collections import OrderedDict
 from io import BytesIO
 import os
 import shutil
@@ -22,11 +23,22 @@
 
 from django.core.management import call_command
 from django.core.management.base import CommandError
+from maasserver.management.commands.edit_named_options import (
+    Command as command_module,
+    )
+from maasserver.models import Config
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils import get_one
+from maasserver.utils.isc import (
+    make_isc_string,
+    parse_isc_string,
+    read_isc_file,
+    )
 from provisioningserver.dns.config import MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME
 from testtools.matchers import (
     Contains,
+    Equals,
     FileContains,
     Not,
     )
@@ -43,7 +55,27 @@
 OPTIONS_FILE_WITH_FORWARDERS = textwrap.dedent("""\
     options {
         directory "/var/cache/bind";
-        forwarders { 192.168.1.1; };
+        forwarders { 192.168.1.1; 192.168.1.2; };
+        auth-nxdomain no;    # conform to RFC1035
+        listen-on-v6 { any; };
+    };
+""")
+
+OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC = textwrap.dedent("""\
+    options {
+        directory "/var/cache/bind";
+        forwarders { 192.168.1.1; 192.168.1.2; };
+        dnssec-validation no;
+        auth-nxdomain no;    # conform to RFC1035
+        listen-on-v6 { any; };
+    };
+""")
+
+OPTIONS_FILE_WITH_EXTRA_AND_DUP_FORWARDER = textwrap.dedent("""\
+    options {
+        directory "/var/cache/bind";
+        forwarders { 192.168.1.2; 192.168.1.3; };
+        dnssec-validation no;
         auth-nxdomain no;    # conform to RFC1035
         listen-on-v6 { any; };
     };
@@ -90,18 +122,27 @@
         self.assertContentFailsWithMessage(
             OPTIONS_FILE, "Failed to make a backup")
 
-    def test_removes_existing_forwarders_config(self):
+    def test_does_not_remove_existing_forwarders_config(self):
         options_file = self.make_file(contents=OPTIONS_FILE_WITH_FORWARDERS)
         call_command(
             "edit_named_options", config_path=options_file,
             stdout=self.stdout)
 
+        options = read_isc_file(options_file)
+        self.assertThat(make_isc_string(options), Contains('forwarders'))
+
+    def test_removes_existing_forwarders_config_if_migrate_set(self):
+        options_file = self.make_file(contents=OPTIONS_FILE_WITH_FORWARDERS)
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, stdout=self.stdout)
+
         # Check that the file was re-written without forwarders (since
         # that's now in the included file).
+        options = read_isc_file(options_file)
         self.assertThat(
-            options_file,
-            Not(FileContains(
-                matcher=Contains('forwarders'))))
+            make_isc_string(options),
+            Not(Contains('forwarders')))
 
     def test_normal_operation(self):
         options_file = self.make_file(contents=OPTIONS_FILE)
@@ -114,11 +155,10 @@
             MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
 
         # Check that the file was re-written with the include statement.
+        options = read_isc_file(options_file)
         self.assertThat(
-            options_file,
-            FileContains(
-                matcher=Contains(
-                    'include "%s";' % expected_path)))
+            make_isc_string(options),
+            Contains('include "%s";' % expected_path))
 
         # Check that the backup was made.
         options_file_base = os.path.dirname(options_file)
@@ -128,3 +168,95 @@
         [backup_file] = files
         backup_file = os.path.join(options_file_base, backup_file)
         self.assertThat(backup_file, FileContains(OPTIONS_FILE))
+
+    def test_migrates_bind_config_to_database(self):
+        options_file = self.make_file(
+            contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, stdout=self.stdout)
+
+        upstream_dns = get_one(Config.objects.filter(name="upstream_dns"))
+        self.assertThat({'192.168.1.1', '192.168.1.2'},
+                        Equals(set(upstream_dns.value.split())))
+
+    def test_migrate_combines_with_existing_forwarders(self):
+        options_file = self.make_file(
+            contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, stdout=self.stdout)
+
+        upstream_dns = get_one(Config.objects.filter(name="upstream_dns"))
+        self.assertThat(OrderedDict.fromkeys(['192.168.1.1', '192.168.1.2']),
+                        Equals(OrderedDict.fromkeys(
+                            upstream_dns.value.split())))
+
+        options_file = self.make_file(
+            contents=OPTIONS_FILE_WITH_EXTRA_AND_DUP_FORWARDER)
+
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, stdout=self.stdout)
+
+        upstream_dns = get_one(Config.objects.filter(name="upstream_dns"))
+        self.assertThat(
+            OrderedDict.fromkeys(
+                ['192.168.1.1', '192.168.1.2', '192.168.1.3']),
+            Equals(OrderedDict.fromkeys(upstream_dns.value.split())))
+
+    def test_dry_run_migrates_nothing_and_prints_config(self):
+        options_file = self.make_file(
+            contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, dry_run=True, stdout=self.stdout)
+
+        upstream_dns = get_one(Config.objects.filter(name="upstream_dns"))
+        self.assertIsNone(upstream_dns)
+
+        # Check that a proper configuration was written to stdout.
+        config = parse_isc_string(self.stdout.getvalue())
+        self.assertIsNotNone(config)
+
+    def test_repeat_migrations_migrate_nothing(self):
+        options_file = self.make_file(
+            contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
+        backup_mock = self.patch(command_module, "back_up_existing_file")
+
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, stdout=self.stdout)
+
+        self.assertTrue(backup_mock.called)
+        backup_mock.reset_mock()
+
+        write_mock = self.patch(command_module, "write_new_named_conf_options")
+
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, stdout=self.stdout)
+
+        self.assertFalse(backup_mock.called)
+        self.assertFalse(write_mock.called)
+
+    def test_repeat_forced_migrations_write_file_anyway(self):
+        options_file = self.make_file(
+            contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
+        backup_mock = self.patch(command_module, "back_up_existing_file")
+
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, stdout=self.stdout)
+
+        self.assertTrue(backup_mock.called)
+        backup_mock.reset_mock()
+
+        write_mock = self.patch(command_module, "write_new_named_conf_options")
+
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, force=True, stdout=self.stdout)
+
+        self.assertTrue(backup_mock.called)
+        self.assertTrue(write_mock.called)

=== modified file 'src/maasserver/utils/isc.py'
--- src/maasserver/utils/isc.py	2015-07-01 01:42:07 +0000
+++ src/maasserver/utils/isc.py	2015-07-08 00:32:06 +0000
@@ -33,18 +33,26 @@
     print_function,
     unicode_literals,
 )
+from collections import OrderedDict
+
 
 str = None
 
 __metaclass__ = type
 __all__ = [
+    'ISCParseException',
     'make_isc_string',
     'parse_isc_string',
+    'read_isc_file',
 ]
 
 import copy
 
 
+class ISCParseException(Exception):
+    """Thrown when an ISC string cannot be parsed."""
+
+
 def _clip(char_list):
     """Clips char_list to individual stanza.
 
@@ -64,7 +72,7 @@
             return index, char_list[:index]
         elif item == '}':
             skip -= 1
-    raise Exception("Invalid brackets.")
+    raise ISCParseException("Invalid brackets.")
 
 
 def _parse_tokens(char_list):
@@ -86,11 +94,11 @@
           {'10.1.0/32': True, '10.1.1/32': True}}}
     """
     index = 0
-    dictionary_fragment = {}
+    dictionary_fragment = OrderedDict()
     new_char_list = copy.deepcopy(char_list)
     if type(new_char_list) == str:
         return new_char_list
-    if type(new_char_list) == dict:
+    if type(new_char_list) == OrderedDict:
         return new_char_list
     last_open = None
     continuous_line = False
@@ -146,6 +154,8 @@
             # fine)
             elif new_char_list[index] not in ['{', ';', '}']:
                 key = new_char_list[index]
+                if type(dictionary_fragment) == list:
+                    raise ISCParseException("Dictionary expected; got a list")
                 dictionary_fragment[key] = ''
                 index += 1
             index += 1
@@ -277,7 +287,18 @@
             new_list[-1] = '%s%s' % (new_list[-1], terminator)
             isc_list.append(
                 '%s { %s }%s' % (option, ' '.join(new_list), terminator))
-        elif type(isc_dict[option]) == dict:
+        elif (type(isc_dict[option]) == OrderedDict or
+              type(isc_dict[option]) == dict):
             isc_list.append('%s { %s }%s' % (
                 option, make_isc_string(isc_dict[option]), terminator))
     return '\n'.join(isc_list)
+
+
+def read_isc_file(isc_file):
+    """Given the specified filename, parses it to create a dictionary.
+
+    :param:isc_file: the filename to read
+    :return:dict: dictionary of ISC file representation
+    """
+    with open(isc_file, "r") as f:
+        return parse_isc_string(f.read())

=== modified file 'src/maasserver/utils/tests/test_isc.py'
--- src/maasserver/utils/tests/test_isc.py	2015-07-01 01:43:35 +0000
+++ src/maasserver/utils/tests/test_isc.py	2015-07-08 00:32:06 +0000
@@ -9,19 +9,23 @@
     unicode_literals,
 )
 
+
 str = None
 
 __metaclass__ = type
 __all__ = []
 
-
+from collections import OrderedDict
 from textwrap import dedent
 
 from maasserver.utils.isc import (
+    ISCParseException,
     make_isc_string,
     parse_isc_string,
+    read_isc_file,
     )
 from maastesting.testcase import MAASTestCase
+from testtools import ExpectedException
 
 
 class TestParseISCString(MAASTestCase):
@@ -39,10 +43,11 @@
             """)
         options = parse_isc_string(testdata)
         self.assertEqual(
-            {u'options': {u'auth-nxdomain': u'no',
-                          u'directory': u'"/var/cache/bind"',
-                          u'dnssec-validation': u'auto',
-                          u'listen-on-v6': {u'any': True}}}, options)
+            OrderedDict({u'options': OrderedDict({u'auth-nxdomain': u'no',
+                        u'directory': u'"/var/cache/bind"',
+                        u'dnssec-validation': u'auto',
+                        u'listen-on-v6': OrderedDict({u'any': True})})}),
+            options)
 
     def test_parses_bind_acl(self):
         testdata = dedent("""\
@@ -159,6 +164,94 @@
         config_string = make_isc_string(config)
         config = parse_isc_string(config_string)
         self.assertEqual(
+            OrderedDict(
+                [(u'acl canonical-int-ns',
+                    OrderedDict(
+                        [(u'91.189.90.151', True), (u'91.189.89.192', True)])),
+                 (u'options', OrderedDict(
+                     [(u'directory', u'"/var/cache/bind"'),
+                      (u'forwarders', OrderedDict(
+                          [(u'91.189.94.2', True)])),
+                      (u'dnssec-validation', u'auto'),
+                      (u'auth-nxdomain', u'no'),
+                      (u'listen-on-v6', OrderedDict([(u'any', True)])),
+                      (u'allow-query', OrderedDict([(u'any', True)])),
+                      (u'allow-transfer', OrderedDict(
+                          [(u'10.222.64.1', True),
+                           (u'canonical-int-ns', True)])),
+                      (u'notify', u'explicit'),
+                      (u'also-notify', OrderedDict(
+                          [(u'91.189.90.151', True),
+                           (u'91.189.89.192', True)])),
+                      (u'allow-query-cache', OrderedDict(
+                          [(u'10.222.64.0/18', True)])),
+                      (u'recursion', u'yes')])),
+                 (u'zone "."', OrderedDict(
+                     [(u'type', u'master'),
+                      (u'file', u'"/etc/bind/db.special"')]))]),
+            config)
+
+    def test_parser_preserves_order(self):
+        testdata = dedent("""\
+            forwarders {
+                9.9.9.9;
+                8.8.8.8;
+                7.7.7.7;
+                6.6.6.6;
+                5.5.5.5;
+                4.4.4.4;
+                3.3.3.3;
+                2.2.2.2;
+                1.1.1.1;
+            };
+            """)
+        forwarders = parse_isc_string(testdata)
+        self.assertEqual(OrderedDict([(u'forwarders', OrderedDict(
+            [(u'9.9.9.9', True), (u'8.8.8.8', True), (u'7.7.7.7', True),
+             (u'6.6.6.6', True), (u'5.5.5.5', True), (u'4.4.4.4', True),
+             (u'3.3.3.3', True), (u'2.2.2.2', True), (u'1.1.1.1', True)]))]),
+            forwarders)
+
+    def test_parse_unmatched_brackets_throws_iscparseexception(self):
+        with ExpectedException(ISCParseException):
+            parse_isc_string("forwarders {")
+
+    def test_parse_malformed_list_throws_iscparseexception(self):
+        with ExpectedException(ISCParseException):
+            parse_isc_string("forwarders {{}a;;b}")
+
+    def test_read_isc_file(self):
+        testdata = dedent("""\
+            acl canonical-int-ns { 91.189.90.151; 91.189.89.192;  };
+
+            options {
+                directory "/var/cache/bind";
+
+                forwarders {
+                    91.189.94.2;
+                    91.189.94.2;
+                };
+
+                dnssec-validation auto;
+
+                auth-nxdomain no;    # conform to RFC1035
+                listen-on-v6 { any; };
+
+                allow-query { any; };
+                allow-transfer { 10.222.64.1; canonical-int-ns; };
+
+                notify explicit;
+                also-notify { 91.189.90.151; 91.189.89.192;  };
+
+                allow-query-cache { 10.222.64.0/18; };
+                recursion yes;
+            };
+
+            zone "."  { type master; file "/etc/bind/db.special"; };
+            """)
+        testfile = self.make_file(contents=testdata)
+        parsed = read_isc_file(testfile)
+        self.assertEqual(
             {u'acl canonical-int-ns':
              {u'91.189.89.192': True, u'91.189.90.151': True},
              u'options': {u'allow-query': {u'any': True},
@@ -176,4 +269,4 @@
                           u'recursion': u'yes'},
              u'zone "."':
              {u'file': u'"/etc/bind/db.special"', u'type': u'master'}},
-            config)
+            parsed)