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