Merge lp:~mpontillo/maas/remove-iscpy-1.7 into lp:maas/1.7

Proposed by Mike Pontillo on 2015-07-01
Status: Merged
Approved by: Mike Pontillo on 2015-07-03
Approved revision: 3373
Merged at revision: 3373
Proposed branch: lp:~mpontillo/maas/remove-iscpy-1.7
Merge into: lp:maas/1.7
Diff against target: 552 lines (+471/-11)
5 files modified
buildout.cfg (+0/-1)
src/maasserver/management/commands/edit_named_options.py (+9/-9)
src/maasserver/utils/isc.py (+283/-0)
src/maasserver/utils/tests/test_isc.py (+179/-0)
versions.cfg (+0/-1)
To merge this branch: bzr merge lp:~mpontillo/maas/remove-iscpy-1.7
Reviewer Review Type Date Requested Status
Andres Rodriguez Needs Information on 2015-07-01
Raphaël Badin (community) 2015-07-01 Approve on 2015-07-01
Review via email: mp+263450@code.launchpad.net

Commit message

Remove dependency on iscpy. Add MAAS utility (based on iscpy) for parsing named.options. Add tests for iscpy-derived code.

Description of the change

This is a portion of the fix for https://bugs.launchpad.net/maas/+bug/1413388.

Truthfully, it turned out to not be necessary to replace the parser. But the ISC configuration parser we were using hasn't been maintained for years, and with a MAAS-owned parser complete with unit tests, we can be more confident in the code.

To post a comment you must log in.
lp:~mpontillo/maas/remove-iscpy-1.7 updated on 2015-07-01
3372. By Mike Pontillo on 2015-07-01

Add missing files

3373. By Mike Pontillo on 2015-07-01

Clean up header in test file

Mike Pontillo (mpontillo) wrote :

Note: the ported iscpy code has not been functionally changed. The code and function names have been reformatted to meet PEP8 guidelines, and the code now passes flake8.

Only the portions of the code that MAAS was using were ported over.

Raphaël Badin (rvb) wrote :

Looks good.
- This will have to be forward ported to 1.8 and trunk. I'd advise *always* doing work on trunk and *then* backport it to older versions. The main reason for this is that you can do the QA on trunk first and then backport it to older versions and QA the change there.
- This should go hand in hand with the packaging branch: https://code.launchpad.net/~rvb/maas/packaging.utopic-1413388/+merge/263477

review: Approve
Andres Rodriguez (andreserl) wrote :

I'd normally go for enhancing the parser library rather than "copying" code. The only concerns I have though, are licensing.

review: Needs Information
Mike Pontillo (mpontillo) wrote :

Understood; I have the same reservations. Upstream has not touched this code since 2012, and I could find an active community (such as a GitHub page) to contribute unit tests, etc. In addition, since dealing with ISC-style DHCP and DNS configuration files are part of MAAS core functionality, I think this code should be brought fully under our control.

The license is BSD, so (as I understand it as a non-lawyer) there should be no issue with including it in our code.

For more information, see:

https://pypi.python.org/pypi/iscpy

Raphaël Badin (rvb) wrote :

I agree with Mike on the licensing issue: this is BSD so as long as we preserve the copyright statement it should be okay.

Raphaël Badin (rvb) wrote :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'buildout.cfg'
2--- buildout.cfg 2014-10-01 15:22:21 +0000
3+++ buildout.cfg 2015-07-01 01:43:38 +0000
4@@ -72,7 +72,6 @@
5 djorm-ext-pgarray
6 docutils
7 crochet
8- iscpy
9 entry-points =
10 maas-region-admin=django.core.management:execute_from_command_line
11 initialization =
12
13=== modified file 'src/maasserver/management/commands/edit_named_options.py'
14--- src/maasserver/management/commands/edit_named_options.py 2014-03-19 02:13:47 +0000
15+++ src/maasserver/management/commands/edit_named_options.py 2015-07-01 01:43:38 +0000
16@@ -1,4 +1,4 @@
17-# Copyright 2014 Canonical Ltd. This software is licensed under the
18+# Copyright 2014-2015 Canonical Ltd. This software is licensed under the
19 # GNU Affero General Public License version 3 (see the file LICENSE).
20
21 """Django command: Edit the named.conf.options file so that it includes
22@@ -28,9 +28,9 @@
23 BaseCommand,
24 CommandError,
25 )
26-from iscpy import (
27- MakeISC,
28- ParseISCString,
29+from maasserver.utils.isc import (
30+ make_isc_string,
31+ parse_isc_string,
32 )
33 from provisioningserver.dns.config import MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME
34
35@@ -60,12 +60,12 @@
36 return options_file
37
38 def parse_file(self, config_path, options_file):
39- """Read the named.conf.options file and parse it with iscpy.
40+ """Read the named.conf.options file and parse it.
41
42- We also use iscpy to insert the include statement that we need.
43+ Then insert the include statement that we need.
44 """
45 try:
46- config_dict = ParseISCString(options_file)
47+ config_dict = parse_isc_string(options_file)
48 except Exception as e:
49 # Yes, it throws bare exceptions :(
50 raise CommandError("Failed to parse %s: %s" % (
51@@ -80,7 +80,7 @@
52 return config_dict
53
54 def set_up_include_statement(self, options_block, config_path):
55- """Insert the 'include' directive into the iscpy-parsed options."""
56+ """Insert the 'include' directive into the parsed options."""
57 dir = os.path.join(os.path.dirname(config_path), "maas")
58 options_block['include'] = '"%s%s%s"' % (
59 dir, os.path.sep, MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
60@@ -116,7 +116,7 @@
61 # Modify the config.
62 self.set_up_include_statement(options_block, config_path)
63 self.remove_forwarders(options_block)
64- new_content = MakeISC(config_dict)
65+ new_content = make_isc_string(config_dict)
66
67 # Back up and write new file.
68 self.back_up_existing_file(config_path)
69
70=== added file 'src/maasserver/utils/isc.py'
71--- src/maasserver/utils/isc.py 1970-01-01 00:00:00 +0000
72+++ src/maasserver/utils/isc.py 2015-07-01 01:43:38 +0000
73@@ -0,0 +1,283 @@
74+# Copyright (c) 2009, Purdue University
75+# Copyright (c) 2015, Canonical Ltd.
76+# All rights reserved.
77+#
78+# Redistribution and use in source and binary forms, with or without
79+# modification, are permitted provided that the following conditions are met:
80+#
81+# Redistributions of source code must retain the above copyright notice, this
82+# list of conditions and the following disclaimer.
83+#
84+# Redistributions in binary form must reproduce the above copyright notice,
85+# this list of conditions and the following disclaimer in the documentation
86+# and/or other materials provided with the distribution.
87+#
88+# Neither the name of the Purdue University nor the names of its contributors
89+# may be used to endorse or promote products derived from this software without
90+# specific prior written permission.
91+#
92+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
93+# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
94+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
95+# ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
96+# LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
97+# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
98+# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
99+# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
100+# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
101+# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
102+# POSSIBILITY OF SUCH DAMAGE.
103+
104+from __future__ import (
105+ absolute_import,
106+ print_function,
107+ unicode_literals,
108+)
109+
110+str = None
111+
112+__metaclass__ = type
113+__all__ = [
114+ 'make_isc_string',
115+ 'parse_isc_string',
116+]
117+
118+import copy
119+
120+
121+def _clip(char_list):
122+ """Clips char_list to individual stanza.
123+
124+ Inputs:
125+ char_list: partial of char_list from _parse_tokens
126+
127+ Outputs:
128+ tuple: (int: skip to char list index, list: shortened char_list)
129+ """
130+ assert char_list[0] == '{'
131+ char_list.pop(0)
132+ skip = 0
133+ for index, item in enumerate(char_list):
134+ if item == '{':
135+ skip += 1
136+ elif item == '}' and skip == 0:
137+ return index, char_list[:index]
138+ elif item == '}':
139+ skip -= 1
140+ raise Exception("Invalid brackets.")
141+
142+
143+def _parse_tokens(char_list):
144+ """Parses exploded isc named.conf portions.
145+
146+ Inputs:
147+ char_list: List of isc file parts
148+
149+ Outputs:
150+ dict: fragment or full isc file dict
151+ Recursive dictionary of isc file, dict values can be of 3 types,
152+ dict, string and bool. Boolean values are always true. Booleans are false
153+ if key is absent. Booleans represent situations in isc files such as:
154+ acl "registered" { 10.1.0/32; 10.1.1:/32;}}
155+
156+ Example:
157+
158+ {'stanza1 "new"': 'test_info', 'stanza1 "embedded"': {'acl "registered"':
159+ {'10.1.0/32': True, '10.1.1/32': True}}}
160+ """
161+ index = 0
162+ dictionary_fragment = {}
163+ new_char_list = copy.deepcopy(char_list)
164+ if type(new_char_list) == str:
165+ return new_char_list
166+ if type(new_char_list) == dict:
167+ return new_char_list
168+ last_open = None
169+ continuous_line = False
170+ temp_list = []
171+
172+ # Prevent "may be referenced before assignment" error
173+ key = None
174+
175+ while index < len(new_char_list):
176+ if new_char_list[index] == '{':
177+ last_open = index
178+ if new_char_list[index] == ';' and continuous_line:
179+ dictionary_fragment = temp_list
180+ temp_list = []
181+ continuous_line = False
182+ if new_char_list[index] == ';':
183+ continuous_line = False
184+ if (len(new_char_list) > index + 1 and
185+ new_char_list[index] == '}' and
186+ new_char_list[index + 1] != ';'):
187+ skip, value = _clip(new_char_list[last_open:])
188+ temp_list.append({key: copy.deepcopy(_parse_tokens(value))})
189+ continuous_line = True
190+ if len(new_char_list) > index + 1 and new_char_list[index + 1] == '{':
191+ # assert key is not None
192+ key = new_char_list.pop(index)
193+ skip, dict_value = _clip(new_char_list[index:])
194+ if continuous_line:
195+ temp_list.append(
196+ {key: copy.deepcopy(_parse_tokens(dict_value))})
197+ else:
198+ dictionary_fragment[key] = copy.deepcopy(
199+ _parse_tokens(dict_value))
200+ index += skip
201+ else:
202+ if len(new_char_list[
203+ index].split()) == 1 and '{' not in new_char_list:
204+ for item in new_char_list:
205+ if item in [';']:
206+ continue
207+ dictionary_fragment[item] = True
208+
209+ # If there are more than 1 'keywords' at new_char_list[index]
210+ # ex - "recursion no;"
211+ elif len(new_char_list[index].split()) >= 2:
212+ dictionary_fragment[
213+ new_char_list[index].split()[0]] = (
214+ ' '.join(new_char_list[index].split()[1:]))
215+ index += 1
216+
217+ # If there is just 1 'keyword' at new_char_list[index]
218+ # ex "recursion;" (not a valid option, but for example's sake it's
219+ # fine)
220+ elif new_char_list[index] not in ['{', ';', '}']:
221+ key = new_char_list[index]
222+ dictionary_fragment[key] = ''
223+ index += 1
224+ index += 1
225+
226+ return dictionary_fragment
227+
228+
229+def _scrub_comments(isc_string):
230+ """Clears comments from an isc file
231+
232+ Inputs:
233+ isc_string: string of isc file
234+ Outputs:
235+ string: string of scrubbed isc file
236+ """
237+ isc_list = []
238+ if isc_string is None:
239+ return ''
240+ expanded_comment = False
241+ for line in isc_string.split('\n'):
242+ no_comment_line = ""
243+ # Vet out any inline comments
244+ if '/*' in line.strip():
245+ try:
246+ striped_line = line.strip()
247+ chars = enumerate(striped_line)
248+ while True:
249+ i, c = chars.next()
250+ try:
251+ if c == '/' and striped_line[i + 1] == '*':
252+ expanded_comment = True
253+ chars.next() # Skip '*'
254+ continue
255+ elif c == '*' and striped_line[i + 1] == '/':
256+ expanded_comment = False
257+ chars.next() # Skip '/'
258+ continue
259+ except IndexError:
260+ continue # We are at the end of the line
261+ if expanded_comment:
262+ continue
263+ else:
264+ no_comment_line += c
265+ except StopIteration:
266+ if no_comment_line:
267+ isc_list.append(no_comment_line)
268+ continue
269+
270+ if expanded_comment:
271+ if '*/' in line.strip():
272+ expanded_comment = False
273+ isc_list.append(line.split('*/')[-1])
274+ continue
275+ else:
276+ continue
277+ if line.strip().startswith(('#', '//')):
278+ continue
279+ else:
280+ isc_list.append(line.split('#')[0].split('//')[0].strip())
281+ return '\n'.join(isc_list)
282+
283+
284+def _explode(isc_string):
285+ """Explodes isc file into relevant tokens.
286+
287+ Inputs:
288+ isc_string: String of isc file
289+
290+ Outputs:
291+ list: list of isc file tokens delimited by brackets and semicolons
292+ ['stanza1 "new"', '{', 'test_info', ';', '}']
293+ """
294+ str_array = []
295+ temp_string = []
296+ for char in isc_string:
297+ if char in ['\n']:
298+ continue
299+ if char in ['{', '}', ';']:
300+ if ''.join(temp_string).strip() == '':
301+ str_array.append(char)
302+ else:
303+ str_array.append(''.join(temp_string).strip())
304+ str_array.append(char)
305+ temp_string = []
306+ else:
307+ temp_string.append(char)
308+ return str_array
309+
310+
311+def parse_isc_string(isc_string):
312+ """Makes a dictionary from an ISC file string
313+
314+ Inputs:
315+ isc_string: string of isc file
316+
317+ Outputs:
318+ dict: dictionary of ISC file representation
319+ """
320+ return _parse_tokens(_explode(_scrub_comments(isc_string)))
321+
322+
323+def make_isc_string(isc_dict, terminate=True):
324+ """Outputs an isc formatted file string from a dict
325+
326+ Inputs:
327+ isc_dict: a recursive dictionary to be turned into an isc file
328+ (from ParseTokens)
329+
330+ Outputs:
331+ str: string of isc file without indentation
332+ """
333+ if terminate:
334+ terminator = ';'
335+ else:
336+ terminator = ''
337+ if type(isc_dict) == str:
338+ return isc_dict
339+ isc_list = []
340+ for option in isc_dict:
341+ if type(isc_dict[option]) == bool:
342+ isc_list.append('%s%s' % (option, terminator))
343+ elif (type(isc_dict[option]) == str or
344+ type(isc_dict[option]) == unicode):
345+ isc_list.append('%s %s%s' % (option, isc_dict[option], terminator))
346+ elif type(isc_dict[option]) == list:
347+ new_list = []
348+ for item in isc_dict[option]:
349+ new_list.append(make_isc_string(item, terminate=False))
350+ new_list[-1] = '%s%s' % (new_list[-1], terminator)
351+ isc_list.append(
352+ '%s { %s }%s' % (option, ' '.join(new_list), terminator))
353+ elif type(isc_dict[option]) == dict:
354+ isc_list.append('%s { %s }%s' % (
355+ option, make_isc_string(isc_dict[option]), terminator))
356+ return '\n'.join(isc_list)
357
358=== added file 'src/maasserver/utils/tests/test_isc.py'
359--- src/maasserver/utils/tests/test_isc.py 1970-01-01 00:00:00 +0000
360+++ src/maasserver/utils/tests/test_isc.py 2015-07-01 01:43:38 +0000
361@@ -0,0 +1,179 @@
362+# Copyright 2015 Canonical Ltd. This software is licensed under the
363+# GNU Affero General Public License version 3 (see the file LICENSE).
364+
365+"""Test ISC configuration file parser/generator."""
366+
367+from __future__ import (
368+ absolute_import,
369+ print_function,
370+ unicode_literals,
371+)
372+
373+str = None
374+
375+__metaclass__ = type
376+__all__ = []
377+
378+
379+from textwrap import dedent
380+
381+from maasserver.utils.isc import (
382+ make_isc_string,
383+ parse_isc_string,
384+ )
385+from maastesting.testcase import MAASTestCase
386+
387+
388+class TestParseISCString(MAASTestCase):
389+
390+ def test_parses_simple_bind_options(self):
391+ testdata = dedent("""\
392+ options {
393+ directory "/var/cache/bind";
394+
395+ dnssec-validation auto;
396+
397+ auth-nxdomain no; # conform to RFC1035
398+ listen-on-v6 { any; };
399+ };
400+ """)
401+ options = parse_isc_string(testdata)
402+ self.assertEqual(
403+ {u'options': {u'auth-nxdomain': u'no',
404+ u'directory': u'"/var/cache/bind"',
405+ u'dnssec-validation': u'auto',
406+ u'listen-on-v6': {u'any': True}}}, options)
407+
408+ def test_parses_bind_acl(self):
409+ testdata = dedent("""\
410+ acl goodclients {
411+ 192.0.2.0/24;
412+ localhost;
413+ localnets;
414+ };
415+ """)
416+ acl = parse_isc_string(testdata)
417+ self.assertEqual(
418+ {u'acl goodclients': {u'192.0.2.0/24': True,
419+ u'localhost': True,
420+ u'localnets': True}}, acl)
421+
422+ def test_parses_multiple_forwarders(self):
423+ testdata = dedent("""\
424+ forwarders {
425+ 91.189.94.2;
426+ 91.189.94.3;
427+ 91.189.94.4;
428+ 91.189.94.5;
429+ 91.189.94.6;
430+ };
431+ """)
432+ forwarders = parse_isc_string(testdata)
433+ self.assertEqual(
434+ {u'forwarders': {u'91.189.94.2': True,
435+ u'91.189.94.3': True,
436+ u'91.189.94.4': True,
437+ u'91.189.94.5': True,
438+ u'91.189.94.6': True}}, forwarders)
439+
440+ def test_parses_bug_1413388_config(self):
441+ testdata = dedent("""\
442+ acl canonical-int-ns { 91.189.90.151; 91.189.89.192; };
443+
444+ options {
445+ directory "/var/cache/bind";
446+
447+ forwarders {
448+ 91.189.94.2;
449+ 91.189.94.2;
450+ };
451+
452+ dnssec-validation auto;
453+
454+ auth-nxdomain no; # conform to RFC1035
455+ listen-on-v6 { any; };
456+
457+ allow-query { any; };
458+ allow-transfer { 10.222.64.1; canonical-int-ns; };
459+
460+ notify explicit;
461+ also-notify { 91.189.90.151; 91.189.89.192; };
462+
463+ allow-query-cache { 10.222.64.0/18; };
464+ recursion yes;
465+ };
466+
467+ zone "." { type master; file "/etc/bind/db.special"; };
468+ """)
469+ config = parse_isc_string(testdata)
470+ self.assertEqual(
471+ {u'acl canonical-int-ns':
472+ {u'91.189.89.192': True, u'91.189.90.151': True},
473+ u'options': {u'allow-query': {u'any': True},
474+ u'allow-query-cache': {u'10.222.64.0/18': True},
475+ u'allow-transfer': {u'10.222.64.1': True,
476+ u'canonical-int-ns': True},
477+ u'also-notify': {u'91.189.89.192': True,
478+ u'91.189.90.151': True},
479+ u'auth-nxdomain': u'no',
480+ u'directory': u'"/var/cache/bind"',
481+ u'dnssec-validation': u'auto',
482+ u'forwarders': {u'91.189.94.2': True},
483+ u'listen-on-v6': {u'any': True},
484+ u'notify': u'explicit',
485+ u'recursion': u'yes'},
486+ u'zone "."':
487+ {u'file': u'"/etc/bind/db.special"', u'type': u'master'}},
488+ config)
489+
490+ def test_parse_then_make_then_parse_generates_identical_config(self):
491+ testdata = dedent("""\
492+ acl canonical-int-ns { 91.189.90.151; 91.189.89.192; };
493+
494+ options {
495+ directory "/var/cache/bind";
496+
497+ forwarders {
498+ 91.189.94.2;
499+ 91.189.94.2;
500+ };
501+
502+ dnssec-validation auto;
503+
504+ auth-nxdomain no; # conform to RFC1035
505+ listen-on-v6 { any; };
506+
507+ allow-query { any; };
508+ allow-transfer { 10.222.64.1; canonical-int-ns; };
509+
510+ notify explicit;
511+ also-notify { 91.189.90.151; 91.189.89.192; };
512+
513+ allow-query-cache { 10.222.64.0/18; };
514+ recursion yes;
515+ };
516+
517+ zone "." { type master; file "/etc/bind/db.special"; };
518+ """)
519+ config = parse_isc_string(testdata)
520+ config_string = make_isc_string(config)
521+ config = parse_isc_string(config_string)
522+ self.assertEqual(
523+ {u'acl canonical-int-ns':
524+ {u'91.189.89.192': True, u'91.189.90.151': True},
525+ u'options': {u'allow-query': {u'any': True},
526+ u'allow-query-cache': {u'10.222.64.0/18': True},
527+ u'allow-transfer': {u'10.222.64.1': True,
528+ u'canonical-int-ns': True},
529+ u'also-notify': {u'91.189.89.192': True,
530+ u'91.189.90.151': True},
531+ u'auth-nxdomain': u'no',
532+ u'directory': u'"/var/cache/bind"',
533+ u'dnssec-validation': u'auto',
534+ u'forwarders': {u'91.189.94.2': True},
535+ u'listen-on-v6': {u'any': True},
536+ u'notify': u'explicit',
537+ u'recursion': u'yes'},
538+ u'zone "."':
539+ {u'file': u'"/etc/bind/db.special"', u'type': u'master'}},
540+ config)
541
542=== modified file 'versions.cfg'
543--- versions.cfg 2014-10-02 15:28:09 +0000
544+++ versions.cfg 2015-07-01 01:43:38 +0000
545@@ -38,7 +38,6 @@
546 extras = 0.0.3
547 fixtures = 0.3.14
548 httplib2 = 0.8
549-iscpy = 1.05
550 iso8601 = 0.1.4
551 junitxml = 0.6
552 keyring = 0.7.1

Subscribers

People subscribed via source and target branches

to all changes: