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

[Mike Pontillo]

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

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.

[Mike Pontillo (mpontillo)]

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)]

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)]

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)]

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)]

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)]

Please don't forget to land https://code.launchpad.net/~rvb/maas/packaging.utopic-1413388/+merge/263477 once this is landed.

=== modified file 'buildout.cfg'
--- buildout.cfg	2014-10-01 15:22:21 +0000
+++ buildout.cfg	2015-07-01 01:43:38 +0000
@@ -72,7 +72,6 @@
   djorm-ext-pgarray
   docutils
   crochet
-  iscpy
 entry-points =
   maas-region-admin=django.core.management:execute_from_command_line
 initialization =
 
=== modified file 'src/maasserver/management/commands/edit_named_options.py'
--- src/maasserver/management/commands/edit_named_options.py	2014-03-19 02:13:47 +0000
+++ src/maasserver/management/commands/edit_named_options.py	2015-07-01 01:43:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Django command: Edit the named.conf.options file so that it includes
@@ -28,9 +28,9 @@
     BaseCommand,
     CommandError,
     )
-from iscpy import (
-    MakeISC,
-    ParseISCString,
+from maasserver.utils.isc import (
+    make_isc_string,
+    parse_isc_string,
     )
 from provisioningserver.dns.config import MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME
 
@@ -60,12 +60,12 @@
         return options_file
 
     def parse_file(self, config_path, options_file):
-        """Read the named.conf.options file and parse it with iscpy.
+        """Read the named.conf.options file and parse it.
 
-        We also use iscpy to insert the include statement that we need.
+        Then insert the include statement that we need.
         """
         try:
-            config_dict = ParseISCString(options_file)
+            config_dict = parse_isc_string(options_file)
         except Exception as e:
             # Yes, it throws bare exceptions :(
             raise CommandError("Failed to parse %s: %s" % (
@@ -80,7 +80,7 @@
         return config_dict
 
     def set_up_include_statement(self, options_block, config_path):
-        """Insert the 'include' directive into the iscpy-parsed options."""
+        """Insert the 'include' directive into the parsed options."""
         dir = os.path.join(os.path.dirname(config_path), "maas")
         options_block['include'] = '"%s%s%s"' % (
             dir, os.path.sep, MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
@@ -116,7 +116,7 @@
         # Modify the config.
         self.set_up_include_statement(options_block, config_path)
         self.remove_forwarders(options_block)
-        new_content = MakeISC(config_dict)
+        new_content = make_isc_string(config_dict)
 
         # Back up and write new file.
         self.back_up_existing_file(config_path)
 
=== added file 'src/maasserver/utils/isc.py'
--- src/maasserver/utils/isc.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/isc.py	2015-07-01 01:43:38 +0000
@@ -0,0 +1,283 @@
+# Copyright (c) 2009, Purdue University
+# Copyright (c) 2015, Canonical Ltd.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are met:
+#
+# Redistributions of source code must retain the above copyright notice, this
+# list of conditions and the following disclaimer.
+#
+# Redistributions in binary form must reproduce the above copyright notice,
+# this list of conditions and the following disclaimer in the documentation
+# and/or other materials provided with the distribution.
+#
+# Neither the name of the Purdue University nor the names of its contributors
+# may be used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+# ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+# LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+# POSSIBILITY OF SUCH DAMAGE.
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+)
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'make_isc_string',
+    'parse_isc_string',
+]
+
+import copy
+
+
+def _clip(char_list):
+    """Clips char_list to individual stanza.
+
+    Inputs:
+      char_list: partial of char_list from _parse_tokens
+
+    Outputs:
+      tuple: (int: skip to char list index, list: shortened char_list)
+    """
+    assert char_list[0] == '{'
+    char_list.pop(0)
+    skip = 0
+    for index, item in enumerate(char_list):
+        if item == '{':
+            skip += 1
+        elif item == '}' and skip == 0:
+            return index, char_list[:index]
+        elif item == '}':
+            skip -= 1
+    raise Exception("Invalid brackets.")
+
+
+def _parse_tokens(char_list):
+    """Parses exploded isc named.conf portions.
+
+    Inputs:
+      char_list: List of isc file parts
+
+    Outputs:
+      dict: fragment or full isc file dict
+      Recursive dictionary of isc file, dict values can be of 3 types,
+      dict, string and bool. Boolean values are always true. Booleans are false
+      if key is absent. Booleans represent situations in isc files such as:
+        acl "registered" { 10.1.0/32; 10.1.1:/32;}}
+
+      Example:
+
+      {'stanza1 "new"': 'test_info', 'stanza1 "embedded"': {'acl "registered"':
+          {'10.1.0/32': True, '10.1.1/32': True}}}
+    """
+    index = 0
+    dictionary_fragment = {}
+    new_char_list = copy.deepcopy(char_list)
+    if type(new_char_list) == str:
+        return new_char_list
+    if type(new_char_list) == dict:
+        return new_char_list
+    last_open = None
+    continuous_line = False
+    temp_list = []
+
+    # Prevent "may be referenced before assignment" error
+    key = None
+
+    while index < len(new_char_list):
+        if new_char_list[index] == '{':
+            last_open = index
+        if new_char_list[index] == ';' and continuous_line:
+            dictionary_fragment = temp_list
+            temp_list = []
+            continuous_line = False
+        if new_char_list[index] == ';':
+            continuous_line = False
+        if (len(new_char_list) > index + 1 and
+           new_char_list[index] == '}' and
+           new_char_list[index + 1] != ';'):
+            skip, value = _clip(new_char_list[last_open:])
+            temp_list.append({key: copy.deepcopy(_parse_tokens(value))})
+            continuous_line = True
+        if len(new_char_list) > index + 1 and new_char_list[index + 1] == '{':
+            # assert key is not None
+            key = new_char_list.pop(index)
+            skip, dict_value = _clip(new_char_list[index:])
+            if continuous_line:
+                temp_list.append(
+                    {key: copy.deepcopy(_parse_tokens(dict_value))})
+            else:
+                dictionary_fragment[key] = copy.deepcopy(
+                    _parse_tokens(dict_value))
+            index += skip
+        else:
+            if len(new_char_list[
+                   index].split()) == 1 and '{' not in new_char_list:
+                for item in new_char_list:
+                    if item in [';']:
+                        continue
+                    dictionary_fragment[item] = True
+
+            # If there are more than 1 'keywords' at new_char_list[index]
+            # ex - "recursion no;"
+            elif len(new_char_list[index].split()) >= 2:
+                dictionary_fragment[
+                    new_char_list[index].split()[0]] = (
+                    ' '.join(new_char_list[index].split()[1:]))
+                index += 1
+
+            # If there is just 1 'keyword' at new_char_list[index]
+            # ex "recursion;" (not a valid option, but for example's sake it's
+            # fine)
+            elif new_char_list[index] not in ['{', ';', '}']:
+                key = new_char_list[index]
+                dictionary_fragment[key] = ''
+                index += 1
+            index += 1
+
+    return dictionary_fragment
+
+
+def _scrub_comments(isc_string):
+    """Clears comments from an isc file
+
+    Inputs:
+      isc_string: string of isc file
+    Outputs:
+      string: string of scrubbed isc file
+    """
+    isc_list = []
+    if isc_string is None:
+        return ''
+    expanded_comment = False
+    for line in isc_string.split('\n'):
+        no_comment_line = ""
+        # Vet out any inline comments
+        if '/*' in line.strip():
+            try:
+                striped_line = line.strip()
+                chars = enumerate(striped_line)
+                while True:
+                    i, c = chars.next()
+                    try:
+                        if c == '/' and striped_line[i + 1] == '*':
+                            expanded_comment = True
+                            chars.next()  # Skip '*'
+                            continue
+                        elif c == '*' and striped_line[i + 1] == '/':
+                            expanded_comment = False
+                            chars.next()  # Skip '/'
+                            continue
+                    except IndexError:
+                        continue  # We are at the end of the line
+                    if expanded_comment:
+                        continue
+                    else:
+                        no_comment_line += c
+            except StopIteration:
+                if no_comment_line:
+                    isc_list.append(no_comment_line)
+                continue
+
+        if expanded_comment:
+            if '*/' in line.strip():
+                expanded_comment = False
+                isc_list.append(line.split('*/')[-1])
+                continue
+            else:
+                continue
+        if line.strip().startswith(('#', '//')):
+            continue
+        else:
+            isc_list.append(line.split('#')[0].split('//')[0].strip())
+    return '\n'.join(isc_list)
+
+
+def _explode(isc_string):
+    """Explodes isc file into relevant tokens.
+
+    Inputs:
+      isc_string: String of isc file
+
+    Outputs:
+      list: list of isc file tokens delimited by brackets and semicolons
+        ['stanza1 "new"', '{', 'test_info', ';', '}']
+    """
+    str_array = []
+    temp_string = []
+    for char in isc_string:
+        if char in ['\n']:
+            continue
+        if char in ['{', '}', ';']:
+            if ''.join(temp_string).strip() == '':
+                str_array.append(char)
+            else:
+                str_array.append(''.join(temp_string).strip())
+                str_array.append(char)
+                temp_string = []
+        else:
+            temp_string.append(char)
+    return str_array
+
+
+def parse_isc_string(isc_string):
+    """Makes a dictionary from an ISC file string
+
+    Inputs:
+      isc_string: string of isc file
+
+    Outputs:
+      dict: dictionary of ISC file representation
+    """
+    return _parse_tokens(_explode(_scrub_comments(isc_string)))
+
+
+def make_isc_string(isc_dict, terminate=True):
+    """Outputs an isc formatted file string from a dict
+
+  Inputs:
+    isc_dict: a recursive dictionary to be turned into an isc file
+              (from ParseTokens)
+
+  Outputs:
+    str: string of isc file without indentation
+  """
+    if terminate:
+        terminator = ';'
+    else:
+        terminator = ''
+    if type(isc_dict) == str:
+        return isc_dict
+    isc_list = []
+    for option in isc_dict:
+        if type(isc_dict[option]) == bool:
+            isc_list.append('%s%s' % (option, terminator))
+        elif (type(isc_dict[option]) == str or
+              type(isc_dict[option]) == unicode):
+            isc_list.append('%s %s%s' % (option, isc_dict[option], terminator))
+        elif type(isc_dict[option]) == list:
+            new_list = []
+            for item in isc_dict[option]:
+                new_list.append(make_isc_string(item, terminate=False))
+            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:
+            isc_list.append('%s { %s }%s' % (
+                option, make_isc_string(isc_dict[option]), terminator))
+    return '\n'.join(isc_list)
 
=== added file 'src/maasserver/utils/tests/test_isc.py'
--- src/maasserver/utils/tests/test_isc.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/tests/test_isc.py	2015-07-01 01:43:38 +0000
@@ -0,0 +1,179 @@
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test ISC configuration file parser/generator."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+)
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+
+from textwrap import dedent
+
+from maasserver.utils.isc import (
+    make_isc_string,
+    parse_isc_string,
+    )
+from maastesting.testcase import MAASTestCase
+
+
+class TestParseISCString(MAASTestCase):
+
+    def test_parses_simple_bind_options(self):
+        testdata = dedent("""\
+            options {
+                directory "/var/cache/bind";
+
+                dnssec-validation auto;
+
+                auth-nxdomain no;    # conform to RFC1035
+                listen-on-v6 { any; };
+            };
+            """)
+        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)
+
+    def test_parses_bind_acl(self):
+        testdata = dedent("""\
+            acl goodclients {
+                192.0.2.0/24;
+                localhost;
+                localnets;
+            };
+            """)
+        acl = parse_isc_string(testdata)
+        self.assertEqual(
+            {u'acl goodclients': {u'192.0.2.0/24': True,
+                                  u'localhost': True,
+                                  u'localnets': True}}, acl)
+
+    def test_parses_multiple_forwarders(self):
+        testdata = dedent("""\
+            forwarders {
+                91.189.94.2;
+                91.189.94.3;
+                91.189.94.4;
+                91.189.94.5;
+                91.189.94.6;
+            };
+            """)
+        forwarders = parse_isc_string(testdata)
+        self.assertEqual(
+            {u'forwarders': {u'91.189.94.2': True,
+                             u'91.189.94.3': True,
+                             u'91.189.94.4': True,
+                             u'91.189.94.5': True,
+                             u'91.189.94.6': True}}, forwarders)
+
+    def test_parses_bug_1413388_config(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"; };
+            """)
+        config = parse_isc_string(testdata)
+        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},
+                          u'allow-query-cache': {u'10.222.64.0/18': True},
+                          u'allow-transfer': {u'10.222.64.1': True,
+                                              u'canonical-int-ns': True},
+                          u'also-notify': {u'91.189.89.192': True,
+                                           u'91.189.90.151': True},
+                          u'auth-nxdomain': u'no',
+                          u'directory': u'"/var/cache/bind"',
+                          u'dnssec-validation': u'auto',
+                          u'forwarders': {u'91.189.94.2': True},
+                          u'listen-on-v6': {u'any': True},
+                          u'notify': u'explicit',
+                          u'recursion': u'yes'},
+             u'zone "."':
+             {u'file': u'"/etc/bind/db.special"', u'type': u'master'}},
+            config)
+
+    def test_parse_then_make_then_parse_generates_identical_config(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"; };
+            """)
+        config = parse_isc_string(testdata)
+        config_string = make_isc_string(config)
+        config = parse_isc_string(config_string)
+        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},
+                          u'allow-query-cache': {u'10.222.64.0/18': True},
+                          u'allow-transfer': {u'10.222.64.1': True,
+                                              u'canonical-int-ns': True},
+                          u'also-notify': {u'91.189.89.192': True,
+                                           u'91.189.90.151': True},
+                          u'auth-nxdomain': u'no',
+                          u'directory': u'"/var/cache/bind"',
+                          u'dnssec-validation': u'auto',
+                          u'forwarders': {u'91.189.94.2': True},
+                          u'listen-on-v6': {u'any': True},
+                          u'notify': u'explicit',
+                          u'recursion': u'yes'},
+             u'zone "."':
+             {u'file': u'"/etc/bind/db.special"', u'type': u'master'}},
+            config)
 
=== modified file 'versions.cfg'
--- versions.cfg	2014-10-02 15:28:09 +0000
+++ versions.cfg	2015-07-01 01:43:38 +0000
@@ -38,7 +38,6 @@
 extras = 0.0.3
 fixtures = 0.3.14
 httplib2 = 0.8
-iscpy = 1.05
 iso8601 = 0.1.4
 junitxml = 0.6
 keyring = 0.7.1