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

[Mike Pontillo]

Migrate DNS forwarders and dnssec-validation settings to MAAS database during upgrades.

[Mike Pontillo (mpontillo)]

During testing, this exception was seen, but it's resolved by executing "sudo apt-get install -f". Need to determine if a proper "apt-get install" rather than "dpkg -i" will fix this problem. If not, need to possibly change when this migration occurs, since it must occur while the database is up, running, and configured.

(pastebin link in case this is unreadable: http://paste.ubuntu.com/11808482/)

Traceback (most recent call last):
  File "/usr/bin/django-admin", line 5, in <module>
    management.execute_from_command_line()
  File "/usr/lib/python2.7/dist-packages/django/core/management/__init__.py", line 399, in execute_from_command_line
    utility.execute()
  File "/usr/lib/python2.7/dist-packages/django/core/management/__init__.py", line 392, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/lib/python2.7/dist-packages/django/core/management/base.py", line 242, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/usr/lib/python2.7/dist-packages/django/core/management/base.py", line 285, in execute
    output = self.handle(*args, **options)
  File "/usr/lib/python2.7/dist-packages/django/core/management/base.py", line 415, in handle
    return self.handle_noargs(**options)
  File "/usr/lib/python2.7/dist-packages/south/management/commands/syncdb.py", line 90, in handle_noargs
    syncdb.Command().execute(**options)
  File "/usr/lib/python2.7/dist-packages/django/core/management/base.py", line 285, in execute
    output = self.handle(*args, **options)
  File "/usr/lib/python2.7/dist-packages/django/core/management/base.py", line 415, in handle
    return self.handle_noargs(**options)
  File "/usr/lib/python2.7/dist-packages/django/core/management/commands/syncdb.py", line 57, in handle_noargs
    cursor = connection.cursor()
  File "/usr/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 159, in cursor
    cursor = util.CursorWrapper(self._cursor(), self)
  File "/usr/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 129, in _cursor
    self.ensure_connection()
  File "/usr/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 124, in ensure_connection
    self.connect()
  File "/usr/lib/python2.7/dist-packages/django/db/utils.py", line 99, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/usr/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 124, in ensure_connection
    self.connect()
  File "/usr/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 112, in connect
    self.connection = self.get_new_connection(conn_params)
  File "/usr/lib/python2.7/dist-packages/django/db/backends/postgresql_psycopg2/base.py", line 116, in get_new_connection
    return Database.connect(**conn_params)
  File "/usr/lib/python2.7/dist-packages/psycopg2/__init__.py", line 179, in connect
    connection_factory=connection_factory, async=async)
django.db.utils.OperationalError: FATAL: password authentication failed for user "maas"
FATAL: password authentication failed for user "maas"

[Christian Reis (kiko)]

Could you add some test code with a few different variations of input to
see how well the importer copes?

[Mike Pontillo (mpontillo)]

kiko, see the prerequisite branch for the parser test code:

https://code.launchpad.net/~mpontillo/maas/remove-iscpy-trunk/+merge/263549

In this MP, I'm specifically testing using the named.options.conf in the bug, among other things.

HOWEVER, as you imply, that doesn't necessarily cover negative test cases, such as if the user has invalid forwarders in their configuration. (which this code will happily migrate)

I am *a little* concerned about adding invalid forwarders, except for the fact that this could only happen if the user tries to migrate an already-broken config. For example, if I have:

forwarders {
    something-that-is-not-an-ip-address;
};

In that case, we'll happily migrate the "something that isn't an IP address" to the MAAS configuration.

But again, in this case, the users' DNS config would have already been broken anyway. GIGO.

[Mike Pontillo (mpontillo)]

Actually, I'm marking this "Work in progress", because I do need to add additional tests for the migration functionality. Thanks kiko. =)

[Raphaël Badin (rvb)]

Looks good. Marking "needs fixing" because you're missing a bunch of tests. I have a bunch of non-blocker remarks as well.

review: Needs Fixing

[Raphaël Badin (rvb)]

Looks good now, thanks for the changes. As discussed, let's just make sure we test the ordering of the IP addresses and add a test for the ISCException thingy.

review: Approve

[Andres Rodriguez (andreserl)]

Also, please let's get the backports for this.
Thanks

[Mike Pontillo (mpontillo)]

Proposed:

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

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

=== modified file 'src/maasserver/management/commands/edit_named_options.py'
--- src/maasserver/management/commands/edit_named_options.py	2015-07-01 17:06:29 +0000
+++ src/maasserver/management/commands/edit_named_options.py	2015-07-07 22:41:15 +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 "
@@ -67,13 +91,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 "
@@ -86,24 +109,65 @@
         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 remove_dnssec_validation(self, options_block):
+    def migrate_dnssec_validation(self, options_block, dry_run, stdout):
         """Remove existing dnssec-validation 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.
+
+        Migrate this value in the configuration file to the MAAS config.
         """
         if 'dnssec-validation' in options_block:
+            dnssec_validation = options_block['dnssec-validation']
+
+            if not dry_run:
+                config, created = Config.objects.get_or_create(
+                    name='dnssec_validation',
+                    defaults={'value': dnssec_validation})
+                if not created:
+                    # Update the MAAS configuration to reflect the new setting
+                    # found in the configuration file.
+                    config.value = dnssec_validation
+                    config.save()
+            else:
+                stdout.write(
+                    "// Set MAAS dnssec_validation to: %s\n"
+                    % dnssec_validation)
+
             del options_block['dnssec-validation']
 
     def back_up_existing_file(self, config_path):
@@ -115,22 +179,60 @@
             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)
-        self.remove_dnssec_validation(options_block)
+
+        if migrate_conflicting_options:
+            self.migrate_forwarders(options_block, dry_run, stdout)
+            self.migrate_dnssec_validation(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	2015-05-07 18:14:38 +0000
+++ src/maasserver/tests/test_commands_edit_named_options.py	2015-07-07 22:41:15 +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,
 )
@@ -52,7 +64,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; };
     };
@@ -99,18 +131,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_removes_existing_dnssec_validation_config(self):
         options_file = self.make_file(contents=OPTIONS_FILE_WITH_DNSSEC)
@@ -118,12 +159,23 @@
             "edit_named_options", config_path=options_file,
             stdout=self.stdout)
 
-        # Check that the file was re-written without forwarders (since
-        # that's now in the included file).
-        self.assertThat(
-            options_file,
-            Not(FileContains(
-                matcher=Contains('dnssec-validation'))))
+        # Check that the file was re-written without dnssec-validation (since
+        # that's now in the included file).
+        options = read_isc_file(options_file)
+        self.assertThat(
+            make_isc_string(options), Contains('dnssec-validation'))
+
+    def test_removes_existing_dnssec_validation_config_if_migration_set(self):
+        options_file = self.make_file(contents=OPTIONS_FILE_WITH_DNSSEC)
+        call_command(
+            "edit_named_options", config_path=options_file,
+            migrate_conflicting_options=True, stdout=self.stdout)
+
+        # Check that the file was re-written without dnssec-validation (since
+        # that's now in the included file).
+        options = read_isc_file(options_file)
+        self.assertThat(
+            make_isc_string(options), Not(Contains('dnssec-validation')))
 
     def test_normal_operation(self):
         options_file = self.make_file(contents=OPTIONS_FILE)
@@ -136,11 +188,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)
@@ -150,3 +201,106 @@
         [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())))
+
+        dnssec_validation = get_one(Config.objects.filter(
+            name="dnssec_validation"))
+        self.assertThat('no', Equals(dnssec_validation.value))
+
+    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())))
+
+        dnssec_validation = get_one(Config.objects.filter(
+            name="dnssec_validation"))
+        self.assertThat('no', Equals(dnssec_validation.value))
+
+        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)
+        dnssec_validation = get_one(Config.objects.filter(
+            name="dnssec_validation"))
+        self.assertIsNone(dnssec_validation)
+
+        # 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 17:06:29 +0000
+++ src/maasserver/utils/isc.py	2015-07-07 22:41:15 +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-03 21:19:59 +0000
+++ src/maasserver/utils/tests/test_isc.py	2015-07-07 22:41:15 +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)