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

Proposed by Mike Pontillo on 2015-07-02
Status: Merged
Approved by: Mike Pontillo on 2015-07-07
Approved revision: 4072
Merged at revision: 4077
Proposed branch: lp:~mpontillo/maas/migrate-dns-settings-trunk
Merge into: lp:maas/trunk
Prerequisite: lp:~mpontillo/maas/remove-iscpy-trunk
Diff against target: 689 lines (+409/-39)
4 files modified
src/maasserver/management/commands/edit_named_options.py (+116/-14)
src/maasserver/tests/test_commands_edit_named_options.py (+169/-15)
src/maasserver/utils/isc.py (+25/-4)
src/maasserver/utils/tests/test_isc.py (+99/-6)
To merge this branch: bzr merge lp:~mpontillo/maas/migrate-dns-settings-trunk
Reviewer Review Type Date Requested Status
Raphaël Badin (community) 2015-07-02 Approve on 2015-07-07
Review via email: mp+263610@code.launchpad.net

Commit message

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

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

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) wrote :

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

Mike Pontillo (mpontillo) wrote :

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) wrote :

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) wrote :

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
4063. By Mike Pontillo on 2015-07-07

Merge trunk

4064. By Mike Pontillo on 2015-07-07

Make ISC parsing code order-preserving; address some review comments.

4065. By Mike Pontillo on 2015-07-07

Fix formatting

4066. By Mike Pontillo on 2015-07-07

Preserve ordering in forwarders; add test for order-preserving behavior

4067. By Mike Pontillo on 2015-07-07

Fix formatting

Raphaël Badin (rvb) wrote :

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
4068. By Mike Pontillo on 2015-07-07

Remove ue of set() in edit_named_options, fix tests

4069. By Mike Pontillo on 2015-07-07

Add test case to verify that unmatched brackets throw ISCParseException

4070. By Mike Pontillo on 2015-07-07

Add check for additional parse error corner case

4071. By Mike Pontillo on 2015-07-07

Fix test name, and copy/paste 'typos'

4072. By Mike Pontillo on 2015-07-07

Fix broken tests

Andres Rodriguez (andreserl) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/management/commands/edit_named_options.py'
2--- src/maasserver/management/commands/edit_named_options.py 2015-07-01 17:06:29 +0000
3+++ src/maasserver/management/commands/edit_named_options.py 2015-07-07 22:41:15 +0000
4@@ -11,6 +11,8 @@
5 print_function,
6 unicode_literals,
7 )
8+from collections import OrderedDict
9+
10
11 str = None
12
13@@ -19,16 +21,20 @@
14 'Command',
15 ]
16
17+from copy import deepcopy
18 from datetime import datetime
19 from optparse import make_option
20 import os
21 import shutil
22+import sys
23
24 from django.core.management.base import (
25 BaseCommand,
26 CommandError,
27 )
28+from maasserver.models import Config
29 from maasserver.utils.isc import (
30+ ISCParseException,
31 make_isc_string,
32 parse_isc_string,
33 )
34@@ -42,6 +48,24 @@
35 '--config-path', dest='config_path',
36 default="/etc/bind/named.conf.options",
37 help="Specify the configuration file to edit."),
38+ make_option(
39+ '--dry-run', dest='dry_run',
40+ default=False, action='store_true',
41+ help="Do not edit any configuration; instead, print to stdout the "
42+ "actions that would be performed, and/or the new "
43+ "configuration that would be written."),
44+ make_option(
45+ '--force', dest='force',
46+ default=False, action='store_true',
47+ help="Force the BIND configuration to be written, even if it "
48+ "appears as though nothing has changed."),
49+ make_option(
50+ '--migrate-conflicting-options', default=False,
51+ dest='migrate_conflicting_options', action='store_true',
52+ help="Causes any options that conflict with MAAS-managed options "
53+ "to be deleted from the BIND configuration and moved to the "
54+ "MAAS-managed configuration. Requires the MAAS database to "
55+ "be configured and running."),
56 )
57 help = (
58 "Edit the named.conf.options file so that it includes the "
59@@ -67,13 +91,12 @@
60 """
61 try:
62 config_dict = parse_isc_string(options_file)
63- except Exception as e:
64- # Yes, it throws bare exceptions :(
65+ except ISCParseException as e:
66 raise CommandError("Failed to parse %s: %s" % (
67 config_path, e.message))
68 options_block = config_dict.get("options", None)
69 if options_block is None:
70- # Something is horribly wrong with the file, bail out rather
71+ # Something is horribly wrong with the file; bail out rather
72 # than doing anything drastic.
73 raise CommandError(
74 "Can't find options {} block in %s, bailing out without "
75@@ -86,24 +109,65 @@
76 options_block['include'] = '"%s%s%s"' % (
77 dir, os.path.sep, MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
78
79- def remove_forwarders(self, options_block):
80+ def migrate_forwarders(self, options_block, dry_run, stdout):
81 """Remove existing forwarders from the options block.
82
83 It's a syntax error to have more than one in the combined
84- configuration for named so we just remove whatever was there.
85- There is no data loss due to the backup file made later.
86+ configuration for named, so we just remove whatever was there.
87+
88+ Migrate any forwarders in the configuration file to the MAAS config.
89 """
90 if 'forwarders' in options_block:
91+ bind_forwarders = options_block['forwarders']
92+
93+ if not dry_run:
94+ config, created = Config.objects.get_or_create(
95+ name='upstream_dns',
96+ defaults={'value': ' '.join(bind_forwarders)})
97+ if not created:
98+ # A configuration value already exists, so add the
99+ # additional values we found in the configuration file to
100+ # MAAS.
101+ if config.value is None:
102+ config.value = ''
103+ maas_forwarders = OrderedDict.fromkeys(
104+ config.value.split())
105+ maas_forwarders.update(bind_forwarders)
106+ config.value = ' '.join(maas_forwarders)
107+ config.save()
108+ else:
109+ stdout.write(
110+ "// Append to MAAS forwarders: %s\n"
111+ % ' '.join(bind_forwarders))
112+
113 del options_block['forwarders']
114
115- def remove_dnssec_validation(self, options_block):
116+ def migrate_dnssec_validation(self, options_block, dry_run, stdout):
117 """Remove existing dnssec-validation from the options block.
118
119 It's a syntax error to have more than one in the combined
120 configuration for named so we just remove whatever was there.
121 There is no data loss due to the backup file made later.
122+
123+ Migrate this value in the configuration file to the MAAS config.
124 """
125 if 'dnssec-validation' in options_block:
126+ dnssec_validation = options_block['dnssec-validation']
127+
128+ if not dry_run:
129+ config, created = Config.objects.get_or_create(
130+ name='dnssec_validation',
131+ defaults={'value': dnssec_validation})
132+ if not created:
133+ # Update the MAAS configuration to reflect the new setting
134+ # found in the configuration file.
135+ config.value = dnssec_validation
136+ config.save()
137+ else:
138+ stdout.write(
139+ "// Set MAAS dnssec_validation to: %s\n"
140+ % dnssec_validation)
141+
142 del options_block['dnssec-validation']
143
144 def back_up_existing_file(self, config_path):
145@@ -115,22 +179,60 @@
146 raise CommandError(
147 "Failed to make a backup of %s, exiting: %s" % (
148 config_path, e.message))
149+ return backup_destination
150+
151+ def write_new_named_conf_options(self, fd, backup_filename, new_content):
152+ fd.write("""\
153+//
154+// This file is managed by MAAS. Although MAAS attempts to preserve changes
155+// made here, it is possible to create conflicts that MAAS can not resolve.
156+//
157+// DNS settings available in MAAS (for example, forwarders and
158+// dnssec-validation) should be managed only in MAAS.
159+//
160+// The previous configuration file was backed up at:
161+// %s
162+//
163+""" % backup_filename)
164+ fd.write(new_content)
165+ fd.write("\n")
166
167 def handle(self, *args, **options):
168 """Entry point for BaseCommand."""
169 # Read stuff in, validate.
170 config_path = options.get('config_path')
171+ dry_run = options.get('dry_run')
172+ force = options.get('force')
173+ stdout = options.get('stdout')
174+ if stdout is None:
175+ stdout = sys.stdout
176+ migrate_conflicting_options = options.get(
177+ 'migrate_conflicting_options')
178+
179 options_file = self.read_file(config_path)
180 config_dict = self.parse_file(config_path, options_file)
181+ original_config = deepcopy(config_dict)
182+
183 options_block = config_dict['options']
184
185- # Modify the config.
186+ # Modify the configuration (if necessary).
187 self.set_up_include_statement(options_block, config_path)
188- self.remove_forwarders(options_block)
189- self.remove_dnssec_validation(options_block)
190+
191+ if migrate_conflicting_options:
192+ self.migrate_forwarders(options_block, dry_run, stdout)
193+ self.migrate_dnssec_validation(options_block, dry_run, stdout)
194+
195+ # Re-parse the new configuration, so we can detect any changes.
196 new_content = make_isc_string(config_dict)
197+ new_config = parse_isc_string(new_content)
198
199- # Back up and write new file.
200- self.back_up_existing_file(config_path)
201- with open(config_path, "wb") as fd:
202- fd.write(new_content)
203+ if original_config != new_config or force:
204+ # The configuration has changed. Back up and write new file.
205+ if dry_run:
206+ self.write_new_named_conf_options(
207+ stdout, config_path, new_content)
208+ else:
209+ backup_filename = self.back_up_existing_file(config_path)
210+ with open(config_path, "wb") as fd:
211+ self.write_new_named_conf_options(
212+ fd, backup_filename, new_content)
213
214=== modified file 'src/maasserver/tests/test_commands_edit_named_options.py'
215--- src/maasserver/tests/test_commands_edit_named_options.py 2015-05-07 18:14:38 +0000
216+++ src/maasserver/tests/test_commands_edit_named_options.py 2015-07-07 22:41:15 +0000
217@@ -15,6 +15,7 @@
218 __all__ = []
219
220 from codecs import getwriter
221+from collections import OrderedDict
222 from io import BytesIO
223 import os
224 import shutil
225@@ -22,11 +23,22 @@
226
227 from django.core.management import call_command
228 from django.core.management.base import CommandError
229+from maasserver.management.commands.edit_named_options import (
230+ Command as command_module,
231+)
232+from maasserver.models import Config
233 from maasserver.testing.factory import factory
234 from maasserver.testing.testcase import MAASServerTestCase
235+from maasserver.utils import get_one
236+from maasserver.utils.isc import (
237+ make_isc_string,
238+ parse_isc_string,
239+ read_isc_file,
240+)
241 from provisioningserver.dns.config import MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME
242 from testtools.matchers import (
243 Contains,
244+ Equals,
245 FileContains,
246 Not,
247 )
248@@ -52,7 +64,27 @@
249 OPTIONS_FILE_WITH_FORWARDERS = textwrap.dedent("""\
250 options {
251 directory "/var/cache/bind";
252- forwarders { 192.168.1.1; };
253+ forwarders { 192.168.1.1; 192.168.1.2; };
254+ auth-nxdomain no; # conform to RFC1035
255+ listen-on-v6 { any; };
256+ };
257+""")
258+
259+OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC = textwrap.dedent("""\
260+ options {
261+ directory "/var/cache/bind";
262+ forwarders { 192.168.1.1; 192.168.1.2; };
263+ dnssec-validation no;
264+ auth-nxdomain no; # conform to RFC1035
265+ listen-on-v6 { any; };
266+ };
267+""")
268+
269+OPTIONS_FILE_WITH_EXTRA_AND_DUP_FORWARDER = textwrap.dedent("""\
270+ options {
271+ directory "/var/cache/bind";
272+ forwarders { 192.168.1.2; 192.168.1.3; };
273+ dnssec-validation no;
274 auth-nxdomain no; # conform to RFC1035
275 listen-on-v6 { any; };
276 };
277@@ -99,18 +131,27 @@
278 self.assertContentFailsWithMessage(
279 OPTIONS_FILE, "Failed to make a backup")
280
281- def test_removes_existing_forwarders_config(self):
282+ def test_does_not_remove_existing_forwarders_config(self):
283 options_file = self.make_file(contents=OPTIONS_FILE_WITH_FORWARDERS)
284 call_command(
285 "edit_named_options", config_path=options_file,
286 stdout=self.stdout)
287
288+ options = read_isc_file(options_file)
289+ self.assertThat(make_isc_string(options), Contains('forwarders'))
290+
291+ def test_removes_existing_forwarders_config_if_migrate_set(self):
292+ options_file = self.make_file(contents=OPTIONS_FILE_WITH_FORWARDERS)
293+ call_command(
294+ "edit_named_options", config_path=options_file,
295+ migrate_conflicting_options=True, stdout=self.stdout)
296+
297 # Check that the file was re-written without forwarders (since
298 # that's now in the included file).
299+ options = read_isc_file(options_file)
300 self.assertThat(
301- options_file,
302- Not(FileContains(
303- matcher=Contains('forwarders'))))
304+ make_isc_string(options),
305+ Not(Contains('forwarders')))
306
307 def test_removes_existing_dnssec_validation_config(self):
308 options_file = self.make_file(contents=OPTIONS_FILE_WITH_DNSSEC)
309@@ -118,12 +159,23 @@
310 "edit_named_options", config_path=options_file,
311 stdout=self.stdout)
312
313- # Check that the file was re-written without forwarders (since
314- # that's now in the included file).
315- self.assertThat(
316- options_file,
317- Not(FileContains(
318- matcher=Contains('dnssec-validation'))))
319+ # Check that the file was re-written without dnssec-validation (since
320+ # that's now in the included file).
321+ options = read_isc_file(options_file)
322+ self.assertThat(
323+ make_isc_string(options), Contains('dnssec-validation'))
324+
325+ def test_removes_existing_dnssec_validation_config_if_migration_set(self):
326+ options_file = self.make_file(contents=OPTIONS_FILE_WITH_DNSSEC)
327+ call_command(
328+ "edit_named_options", config_path=options_file,
329+ migrate_conflicting_options=True, stdout=self.stdout)
330+
331+ # Check that the file was re-written without dnssec-validation (since
332+ # that's now in the included file).
333+ options = read_isc_file(options_file)
334+ self.assertThat(
335+ make_isc_string(options), Not(Contains('dnssec-validation')))
336
337 def test_normal_operation(self):
338 options_file = self.make_file(contents=OPTIONS_FILE)
339@@ -136,11 +188,10 @@
340 MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
341
342 # Check that the file was re-written with the include statement.
343+ options = read_isc_file(options_file)
344 self.assertThat(
345- options_file,
346- FileContains(
347- matcher=Contains(
348- 'include "%s";' % expected_path)))
349+ make_isc_string(options),
350+ Contains('include "%s";' % expected_path))
351
352 # Check that the backup was made.
353 options_file_base = os.path.dirname(options_file)
354@@ -150,3 +201,106 @@
355 [backup_file] = files
356 backup_file = os.path.join(options_file_base, backup_file)
357 self.assertThat(backup_file, FileContains(OPTIONS_FILE))
358+
359+ def test_migrates_bind_config_to_database(self):
360+ options_file = self.make_file(
361+ contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
362+ call_command(
363+ "edit_named_options", config_path=options_file,
364+ migrate_conflicting_options=True, stdout=self.stdout)
365+
366+ upstream_dns = get_one(Config.objects.filter(name="upstream_dns"))
367+ self.assertThat({'192.168.1.1', '192.168.1.2'},
368+ Equals(set(upstream_dns.value.split())))
369+
370+ dnssec_validation = get_one(Config.objects.filter(
371+ name="dnssec_validation"))
372+ self.assertThat('no', Equals(dnssec_validation.value))
373+
374+ def test_migrate_combines_with_existing_forwarders(self):
375+ options_file = self.make_file(
376+ contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
377+ call_command(
378+ "edit_named_options", config_path=options_file,
379+ migrate_conflicting_options=True, stdout=self.stdout)
380+
381+ upstream_dns = get_one(Config.objects.filter(name="upstream_dns"))
382+ self.assertThat(OrderedDict.fromkeys(['192.168.1.1', '192.168.1.2']),
383+ Equals(OrderedDict.fromkeys(
384+ upstream_dns.value.split())))
385+
386+ dnssec_validation = get_one(Config.objects.filter(
387+ name="dnssec_validation"))
388+ self.assertThat('no', Equals(dnssec_validation.value))
389+
390+ options_file = self.make_file(
391+ contents=OPTIONS_FILE_WITH_EXTRA_AND_DUP_FORWARDER)
392+
393+ call_command(
394+ "edit_named_options", config_path=options_file,
395+ migrate_conflicting_options=True, stdout=self.stdout)
396+
397+ upstream_dns = get_one(Config.objects.filter(name="upstream_dns"))
398+ self.assertThat(
399+ OrderedDict.fromkeys(
400+ ['192.168.1.1', '192.168.1.2', '192.168.1.3']),
401+ Equals(OrderedDict.fromkeys(upstream_dns.value.split())))
402+
403+ def test_dry_run_migrates_nothing_and_prints_config(self):
404+ options_file = self.make_file(
405+ contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
406+ call_command(
407+ "edit_named_options", config_path=options_file,
408+ migrate_conflicting_options=True, dry_run=True, stdout=self.stdout)
409+
410+ upstream_dns = get_one(Config.objects.filter(name="upstream_dns"))
411+ self.assertIsNone(upstream_dns)
412+ dnssec_validation = get_one(Config.objects.filter(
413+ name="dnssec_validation"))
414+ self.assertIsNone(dnssec_validation)
415+
416+ # Check that a proper configuration was written to stdout.
417+ config = parse_isc_string(self.stdout.getvalue())
418+ self.assertIsNotNone(config)
419+
420+ def test_repeat_migrations_migrate_nothing(self):
421+ options_file = self.make_file(
422+ contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
423+ backup_mock = self.patch(command_module, "back_up_existing_file")
424+
425+ call_command(
426+ "edit_named_options", config_path=options_file,
427+ migrate_conflicting_options=True, stdout=self.stdout)
428+
429+ self.assertTrue(backup_mock.called)
430+ backup_mock.reset_mock()
431+
432+ write_mock = self.patch(command_module, "write_new_named_conf_options")
433+
434+ call_command(
435+ "edit_named_options", config_path=options_file,
436+ migrate_conflicting_options=True, stdout=self.stdout)
437+
438+ self.assertFalse(backup_mock.called)
439+ self.assertFalse(write_mock.called)
440+
441+ def test_repeat_forced_migrations_write_file_anyway(self):
442+ options_file = self.make_file(
443+ contents=OPTIONS_FILE_WITH_FORWARDERS_AND_DNSSEC)
444+ backup_mock = self.patch(command_module, "back_up_existing_file")
445+
446+ call_command(
447+ "edit_named_options", config_path=options_file,
448+ migrate_conflicting_options=True, stdout=self.stdout)
449+
450+ self.assertTrue(backup_mock.called)
451+ backup_mock.reset_mock()
452+
453+ write_mock = self.patch(command_module, "write_new_named_conf_options")
454+
455+ call_command(
456+ "edit_named_options", config_path=options_file,
457+ migrate_conflicting_options=True, force=True, stdout=self.stdout)
458+
459+ self.assertTrue(backup_mock.called)
460+ self.assertTrue(write_mock.called)
461
462=== modified file 'src/maasserver/utils/isc.py'
463--- src/maasserver/utils/isc.py 2015-07-01 17:06:29 +0000
464+++ src/maasserver/utils/isc.py 2015-07-07 22:41:15 +0000
465@@ -33,18 +33,26 @@
466 print_function,
467 unicode_literals,
468 )
469+from collections import OrderedDict
470+
471
472 str = None
473
474 __metaclass__ = type
475 __all__ = [
476+ 'ISCParseException',
477 'make_isc_string',
478 'parse_isc_string',
479+ 'read_isc_file',
480 ]
481
482 import copy
483
484
485+class ISCParseException(Exception):
486+ """Thrown when an ISC string cannot be parsed."""
487+
488+
489 def _clip(char_list):
490 """Clips char_list to individual stanza.
491
492@@ -64,7 +72,7 @@
493 return index, char_list[:index]
494 elif item == '}':
495 skip -= 1
496- raise Exception("Invalid brackets.")
497+ raise ISCParseException("Invalid brackets.")
498
499
500 def _parse_tokens(char_list):
501@@ -86,11 +94,11 @@
502 {'10.1.0/32': True, '10.1.1/32': True}}}
503 """
504 index = 0
505- dictionary_fragment = {}
506+ dictionary_fragment = OrderedDict()
507 new_char_list = copy.deepcopy(char_list)
508 if type(new_char_list) == str:
509 return new_char_list
510- if type(new_char_list) == dict:
511+ if type(new_char_list) == OrderedDict:
512 return new_char_list
513 last_open = None
514 continuous_line = False
515@@ -146,6 +154,8 @@
516 # fine)
517 elif new_char_list[index] not in ['{', ';', '}']:
518 key = new_char_list[index]
519+ if type(dictionary_fragment) == list:
520+ raise ISCParseException("Dictionary expected; got a list")
521 dictionary_fragment[key] = ''
522 index += 1
523 index += 1
524@@ -277,7 +287,18 @@
525 new_list[-1] = '%s%s' % (new_list[-1], terminator)
526 isc_list.append(
527 '%s { %s }%s' % (option, ' '.join(new_list), terminator))
528- elif type(isc_dict[option]) == dict:
529+ elif (type(isc_dict[option]) == OrderedDict or
530+ type(isc_dict[option]) == dict):
531 isc_list.append('%s { %s }%s' % (
532 option, make_isc_string(isc_dict[option]), terminator))
533 return '\n'.join(isc_list)
534+
535+
536+def read_isc_file(isc_file):
537+ """Given the specified filename, parses it to create a dictionary.
538+
539+ :param:isc_file: the filename to read
540+ :return:dict: dictionary of ISC file representation
541+ """
542+ with open(isc_file, "r") as f:
543+ return parse_isc_string(f.read())
544
545=== modified file 'src/maasserver/utils/tests/test_isc.py'
546--- src/maasserver/utils/tests/test_isc.py 2015-07-03 21:19:59 +0000
547+++ src/maasserver/utils/tests/test_isc.py 2015-07-07 22:41:15 +0000
548@@ -9,19 +9,23 @@
549 unicode_literals,
550 )
551
552+
553 str = None
554
555 __metaclass__ = type
556 __all__ = []
557
558-
559+from collections import OrderedDict
560 from textwrap import dedent
561
562 from maasserver.utils.isc import (
563+ ISCParseException,
564 make_isc_string,
565 parse_isc_string,
566+ read_isc_file,
567 )
568 from maastesting.testcase import MAASTestCase
569+from testtools import ExpectedException
570
571
572 class TestParseISCString(MAASTestCase):
573@@ -39,10 +43,11 @@
574 """)
575 options = parse_isc_string(testdata)
576 self.assertEqual(
577- {u'options': {u'auth-nxdomain': u'no',
578- u'directory': u'"/var/cache/bind"',
579- u'dnssec-validation': u'auto',
580- u'listen-on-v6': {u'any': True}}}, options)
581+ OrderedDict({u'options': OrderedDict({u'auth-nxdomain': u'no',
582+ u'directory': u'"/var/cache/bind"',
583+ u'dnssec-validation': u'auto',
584+ u'listen-on-v6': OrderedDict({u'any': True})})}),
585+ options)
586
587 def test_parses_bind_acl(self):
588 testdata = dedent("""\
589@@ -159,6 +164,94 @@
590 config_string = make_isc_string(config)
591 config = parse_isc_string(config_string)
592 self.assertEqual(
593+ OrderedDict(
594+ [(u'acl canonical-int-ns',
595+ OrderedDict(
596+ [(u'91.189.90.151', True), (u'91.189.89.192', True)])),
597+ (u'options', OrderedDict(
598+ [(u'directory', u'"/var/cache/bind"'),
599+ (u'forwarders', OrderedDict(
600+ [(u'91.189.94.2', True)])),
601+ (u'dnssec-validation', u'auto'),
602+ (u'auth-nxdomain', u'no'),
603+ (u'listen-on-v6', OrderedDict([(u'any', True)])),
604+ (u'allow-query', OrderedDict([(u'any', True)])),
605+ (u'allow-transfer', OrderedDict(
606+ [(u'10.222.64.1', True),
607+ (u'canonical-int-ns', True)])),
608+ (u'notify', u'explicit'),
609+ (u'also-notify', OrderedDict(
610+ [(u'91.189.90.151', True),
611+ (u'91.189.89.192', True)])),
612+ (u'allow-query-cache', OrderedDict(
613+ [(u'10.222.64.0/18', True)])),
614+ (u'recursion', u'yes')])),
615+ (u'zone "."', OrderedDict(
616+ [(u'type', u'master'),
617+ (u'file', u'"/etc/bind/db.special"')]))]),
618+ config)
619+
620+ def test_parser_preserves_order(self):
621+ testdata = dedent("""\
622+ forwarders {
623+ 9.9.9.9;
624+ 8.8.8.8;
625+ 7.7.7.7;
626+ 6.6.6.6;
627+ 5.5.5.5;
628+ 4.4.4.4;
629+ 3.3.3.3;
630+ 2.2.2.2;
631+ 1.1.1.1;
632+ };
633+ """)
634+ forwarders = parse_isc_string(testdata)
635+ self.assertEqual(OrderedDict([(u'forwarders', OrderedDict(
636+ [(u'9.9.9.9', True), (u'8.8.8.8', True), (u'7.7.7.7', True),
637+ (u'6.6.6.6', True), (u'5.5.5.5', True), (u'4.4.4.4', True),
638+ (u'3.3.3.3', True), (u'2.2.2.2', True), (u'1.1.1.1', True)]))]),
639+ forwarders)
640+
641+ def test_parse_unmatched_brackets_throws_iscparseexception(self):
642+ with ExpectedException(ISCParseException):
643+ parse_isc_string("forwarders {")
644+
645+ def test_parse_malformed_list_throws_iscparseexception(self):
646+ with ExpectedException(ISCParseException):
647+ parse_isc_string("forwarders {{}a;;b}")
648+
649+ def test_read_isc_file(self):
650+ testdata = dedent("""\
651+ acl canonical-int-ns { 91.189.90.151; 91.189.89.192; };
652+
653+ options {
654+ directory "/var/cache/bind";
655+
656+ forwarders {
657+ 91.189.94.2;
658+ 91.189.94.2;
659+ };
660+
661+ dnssec-validation auto;
662+
663+ auth-nxdomain no; # conform to RFC1035
664+ listen-on-v6 { any; };
665+
666+ allow-query { any; };
667+ allow-transfer { 10.222.64.1; canonical-int-ns; };
668+
669+ notify explicit;
670+ also-notify { 91.189.90.151; 91.189.89.192; };
671+
672+ allow-query-cache { 10.222.64.0/18; };
673+ recursion yes;
674+ };
675+
676+ zone "." { type master; file "/etc/bind/db.special"; };
677+ """)
678+ testfile = self.make_file(contents=testdata)
679+ parsed = read_isc_file(testfile)
680+ self.assertEqual(
681 {u'acl canonical-int-ns':
682 {u'91.189.89.192': True, u'91.189.90.151': True},
683 u'options': {u'allow-query': {u'any': True},
684@@ -176,4 +269,4 @@
685 u'recursion': u'yes'},
686 u'zone "."':
687 {u'file': u'"/etc/bind/db.special"', u'type': u'master'}},
688- config)
689+ parsed)