Merge lp:~mpontillo/maas/dns-template-changes-trunk into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Allow users to define allow-* parameters in named.conf.options; prevent writing them to named.conf.options.inside.maas in that case.

 * Add code to parse /etc/bind/named.conf.options before writing out the template and check if we're about to write duplicate options to the configuration. If so, suppress those options from being written from within the template.

 * Move ISC parsing utilities to provisioningserver so they can be used within the DNS writing code (which seems like it doesn't belong in provisioningserver in the first place, but moving that seemed more risky than moving a single utility, and the ISC parsing utilities might one day be used on the cluster.)

[Andres Rodriguez (andreserl)]

lgtm! Although, I'd like someone else to review as well.

review: Approve

[Raphaël Badin (rvb)]

Looks good. I've got a question and a bunch of remarks but nothing major.

review: Approve

[Mike Pontillo (mpontillo)]

Thanks for the review. I went back and re-did this with the "bzr mv" command, and addressed your other concerns.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/dns-template-changes-trunk into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [63.5 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:3 http://security.ubuntu.com trusty-security/main Sources [87.9 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Get:5 http://security.ubuntu.com trusty-security/universe Sources [28.1 kB]
Get:6 http://security.ubuntu.com trusty-security/main amd64 Packages [309 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:7 http://nova.clouds.archive.ubuntu.com trusty-updates Release [63.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [111 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [213 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [125 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [569 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [295 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,867 kB in 3s (579 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mock python-netaddr python-netifaces python-n...

=== modified file 'etc/maas/templates/dns/named.conf.options.inside.maas.template'
--- etc/maas/templates/dns/named.conf.options.inside.maas.template	2015-04-24 19:49:30 +0000
+++ etc/maas/templates/dns/named.conf.options.inside.maas.template	2015-07-09 22:46:20 +0000
@@ -8,6 +8,13 @@
 
 dnssec-validation {{dnssec_validation}};
 
+
+{{if not upstream_allow_query}}
 allow-query { any; };
+{{endif}}
+{{if not upstream_allow_recursion}}
 allow-recursion { trusted; };
+{{endif}}
+{{if not upstream_allow_query_cache}}
 allow-query-cache { trusted; };
+{{endif}}
 
=== modified file 'src/maasserver/management/commands/edit_named_options.py'
--- src/maasserver/management/commands/edit_named_options.py	2015-07-07 20:11:36 +0000
+++ src/maasserver/management/commands/edit_named_options.py	2015-07-09 22:46:20 +0000
@@ -33,7 +33,7 @@
     CommandError,
 )
 from maasserver.models import Config
-from maasserver.utils.isc import (
+from provisioningserver.utils.isc import (
     ISCParseException,
     make_isc_string,
     parse_isc_string,
 
=== modified file 'src/maasserver/tests/test_commands_edit_named_options.py'
--- src/maasserver/tests/test_commands_edit_named_options.py	2015-07-07 22:40:48 +0000
+++ src/maasserver/tests/test_commands_edit_named_options.py	2015-07-09 22:46:20 +0000
@@ -30,12 +30,12 @@
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils import get_one
-from maasserver.utils.isc import (
+from provisioningserver.dns.config import MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME
+from provisioningserver.utils.isc import (
     make_isc_string,
     parse_isc_string,
     read_isc_file,
 )
-from provisioningserver.dns.config import MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME
 from testtools.matchers import (
     Contains,
     Equals,
 
=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/dns/config.py	2015-07-09 22:46:20 +0000
@@ -31,10 +31,12 @@
 
 from provisioningserver.utils import locate_config
 from provisioningserver.utils.fs import atomic_write
+from provisioningserver.utils.isc import read_isc_file
 from provisioningserver.utils.shell import call_and_check
 import tempita
 
 
+NAMED_CONF_OPTIONS = 'named.conf.options'
 MAAS_NAMED_CONF_NAME = 'named.conf.maas'
 MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME = 'named.conf.options.inside.maas'
 MAAS_NAMED_RNDC_CONF_NAME = 'named.conf.rndc.maas'
@@ -53,6 +55,18 @@
         return setting
 
 
+def get_bind_config_dir():
+    """Location of bind configuration files."""
+    setting = os.getenv(
+        "MAAS_BIND_CONFIG_DIR",
+        locate_config(os.path.pardir, "bind"))
+    if isinstance(setting, bytes):
+        fsenc = sys.getfilesystemencoding()
+        return setting.decode(fsenc)
+    else:
+        return setting
+
+
 def get_dns_rndc_port():
     """RNDC port to be configured by MAAS to communicate with BIND."""
     setting = os.getenv("MAAS_DNS_RNDC_PORT", "954")
@@ -193,6 +207,25 @@
     # template that uses this value.
     kwargs.setdefault("upstream_dns")
     kwargs.setdefault("dnssec_validation", "auto")
+
+    # Parse the options file and make sure MAAS doesn't define any options
+    # that the user has already customized.
+    allow_user_override_options = [
+        "allow-query",
+        "allow-recursion",
+        "allow-query-cache",
+    ]
+
+    try:
+        parsed_options = read_isc_file(
+            compose_bind_config_path(NAMED_CONF_OPTIONS))
+    except IOError:
+        parsed_options = {}
+
+    options = parsed_options.get('options', {})
+    for option in allow_user_override_options:
+        kwargs['upstream_' + option.replace('-', '_')] = option in options
+
     try:
         rendered = template.substitute(kwargs)
     except NameError as error:
@@ -207,6 +240,11 @@
     return os.path.join(get_dns_config_dir(), filename)
 
 
+def compose_bind_config_path(filename):
+    """Return the full path for a DNS config or zone file."""
+    return os.path.join(get_bind_config_dir(), filename)
+
+
 def render_dns_template(template_name, *parameters):
     """Generate contents for a DNS configuration or zone file.
 
 
=== modified file 'src/provisioningserver/dns/testing.py'
--- src/provisioningserver/dns/testing.py	2015-05-11 09:51:49 +0000
+++ src/provisioningserver/dns/testing.py	2015-07-09 22:46:20 +0000
@@ -32,6 +32,8 @@
         config_dir = config_dir.encode(fsenc)
     testcase.useFixture(
         EnvironmentVariable(b"MAAS_DNS_CONFIG_DIR", config_dir))
+    testcase.useFixture(
+        EnvironmentVariable(b"MAAS_BIND_CONFIG_DIR", config_dir))
     return config_dir.decode(fsenc)
 
 
 
=== modified file 'src/provisioningserver/dns/tests/test_config.py'
--- src/provisioningserver/dns/tests/test_config.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/dns/tests/test_config.py	2015-07-09 22:46:20 +0000
@@ -39,6 +39,7 @@
     MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME,
     MAAS_NAMED_RNDC_CONF_NAME,
     MAAS_RNDC_CONF_NAME,
+    NAMED_CONF_OPTIONS,
     render_dns_template,
     report_missing_config_dir,
     set_up_options_conf,
@@ -54,12 +55,16 @@
     DNSReverseZoneConfig,
 )
 from provisioningserver.utils import locate_config
+from provisioningserver.utils.isc import read_isc_file
 from testtools.matchers import (
+    AllMatch,
     Contains,
     ContainsAll,
     EndsWith,
+    Equals,
     FileContains,
     FileExists,
+    Is,
     IsInstance,
     MatchesAll,
     Not,
@@ -70,6 +75,47 @@
 from twisted.python.filepath import FilePath
 
 
+NAMED_CONF_OPTIONS_CONTENTS = dedent("""\
+    options {
+        forwarders {
+            8.8.8.8;
+            8.8.4.4;
+        };
+        dnssec-validation auto;
+        allow-query { any; };
+        allow-recursion { trusted; };
+        allow-query-cache { trusted; };
+        auth-nxdomain no;
+        listen-on-v6 { any; };
+    };
+    """)
+
+NAMED_CONF_OPTIONS_WITH_ALLOW_QUERY_CONTENTS = dedent("""\
+    options {
+        forwarders {
+            8.8.8.8;
+            8.8.4.4;
+        };
+        dnssec-validation auto;
+        allow-query { any; };
+        auth-nxdomain no;
+        listen-on-v6 { any; };
+    };
+    """)
+
+NAMED_CONF_OPTIONS_NO_ALLOW_CONTENTS = dedent("""\
+    options {
+        forwarders {
+            8.8.8.8;
+            8.8.4.4;
+        };
+        dnssec-validation auto;
+        auth-nxdomain no;
+        listen-on-v6 { any; };
+    };
+    """)
+
+
 class TestHelpers(MAASTestCase):
 
     def test_get_dns_config_dir_defaults_to_etc_bind_maas(self):
@@ -90,6 +136,24 @@
                 IsInstance(unicode),
             ))
 
+    def test_get_bind_config_dir_defaults_to_etc_bind_maas(self):
+        self.useFixture(EnvironmentVariable("MAAS_BIND_CONFIG_DIR"))
+        self.assertThat(
+            config.get_bind_config_dir(), MatchesAll(
+                SamePath(locate_config("../bind")),
+                IsInstance(unicode),
+            ))
+
+    def test_get_bind_config_dir_checks_environ_first(self):
+        directory = self.make_dir()
+        self.useFixture(EnvironmentVariable(
+            "MAAS_BIND_CONFIG_DIR", directory.encode("ascii")))
+        self.assertThat(
+            config.get_bind_config_dir(), MatchesAll(
+                SamePath(directory),
+                IsInstance(unicode),
+            ))
+
     def test_get_dns_root_port_defaults_to_954(self):
         self.useFixture(EnvironmentVariable("MAAS_DNS_RNDC_PORT"))
         self.assertEqual(954, config.get_dns_rndc_port())
@@ -142,6 +206,59 @@
                 FileContains(matcher=Contains(address))
                 for address in fake_dns)))
 
+    def test_set_up_options_conf_write_config_assumes_no_overrides(self):
+        dns_conf_dir = patch_dns_config_path(self)
+        set_up_options_conf()
+        target_file = os.path.join(
+            dns_conf_dir, MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
+        target = read_isc_file(target_file)
+        self.assertThat([
+            target['allow-query']['any'],
+            target['allow-recursion']['trusted'],
+            target['allow-query-cache']['trusted'],
+        ], AllMatch(Equals(True)))
+
+    def test_set_up_options_conf_write_config_allows_overrides(self):
+        dns_conf_dir = patch_dns_config_path(self)
+        factory.make_file(
+            location=dns_conf_dir, name=NAMED_CONF_OPTIONS,
+            contents=NAMED_CONF_OPTIONS_CONTENTS)
+        set_up_options_conf()
+        target_file = os.path.join(
+            dns_conf_dir, MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
+        target = read_isc_file(target_file)
+        self.assertThat([
+            target.get('allow-query'),
+            target.get('allow-recursion'),
+            target.get('allow-query-cache'),
+        ], AllMatch(Is(None)))
+
+    def test_set_up_options_conf_write_config_allows_zero_overrides(self):
+        dns_conf_dir = patch_dns_config_path(self)
+        factory.make_file(
+            location=dns_conf_dir, name=NAMED_CONF_OPTIONS,
+            contents=NAMED_CONF_OPTIONS_NO_ALLOW_CONTENTS)
+        set_up_options_conf()
+        target_file = os.path.join(
+            dns_conf_dir, MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
+        target = read_isc_file(target_file)
+        self.assertThat([
+            target['allow-query']['any'],
+            target['allow-recursion']['trusted'],
+            target['allow-query-cache']['trusted'],
+        ], AllMatch(Equals(True)))
+
+    def test_set_up_options_conf_write_config_allows_single_override(self):
+        dns_conf_dir = patch_dns_config_path(self)
+        factory.make_file(
+            location=dns_conf_dir, name=NAMED_CONF_OPTIONS,
+            contents=NAMED_CONF_OPTIONS_WITH_ALLOW_QUERY_CONTENTS)
+        set_up_options_conf()
+        target_file = os.path.join(
+            dns_conf_dir, MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME)
+        target = read_isc_file(target_file)
+        self.assertIsNone(target.get('allow-query'))
+
     def test_set_up_options_conf_handles_no_upstream_dns(self):
         dns_conf_dir = patch_dns_config_path(self)
         set_up_options_conf()
 
=== renamed file 'src/maasserver/utils/isc.py' => 'src/provisioningserver/utils/isc.py'
--- src/maasserver/utils/isc.py	2015-07-07 20:50:41 +0000
+++ src/provisioningserver/utils/isc.py	2015-07-09 22:46:20 +0000
@@ -144,6 +144,8 @@
             # If there are more than 1 'keywords' at new_char_list[index]
             # ex - "recursion no;"
             elif len(new_char_list[index].split()) >= 2:
+                if type(dictionary_fragment) == list:
+                    raise ISCParseException("Syntax error")
                 dictionary_fragment[
                     new_char_list[index].split()[0]] = (
                     ' '.join(new_char_list[index].split()[1:]))
@@ -155,7 +157,7 @@
             elif new_char_list[index] not in ['{', ';', '}']:
                 key = new_char_list[index]
                 if type(dictionary_fragment) == list:
-                    raise ISCParseException("Dictionary expected; got a list")
+                    raise ISCParseException("Syntax error")
                 dictionary_fragment[key] = ''
                 index += 1
             index += 1
 
=== renamed file 'src/maasserver/utils/tests/test_isc.py' => 'src/provisioningserver/utils/tests/test_isc.py'
--- src/maasserver/utils/tests/test_isc.py	2015-07-07 20:50:41 +0000
+++ src/provisioningserver/utils/tests/test_isc.py	2015-07-09 22:46:20 +0000
@@ -18,13 +18,13 @@
 from collections import OrderedDict
 from textwrap import dedent
 
-from maasserver.utils.isc import (
+from maastesting.testcase import MAASTestCase
+from provisioningserver.utils.isc import (
     ISCParseException,
     make_isc_string,
     parse_isc_string,
     read_isc_file,
 )
-from maastesting.testcase import MAASTestCase
 from testtools import ExpectedException
 
 
@@ -220,6 +220,10 @@
         with ExpectedException(ISCParseException):
             parse_isc_string("forwarders {{}a;;b}")
 
+    def test_parse_forgotten_semicolons_throw_iscparseexception(self):
+        with ExpectedException(ISCParseException):
+            parse_isc_string("a { b; } { c; } d e;")
+
     def test_read_isc_file(self):
         testdata = dedent("""\
             acl canonical-int-ns { 91.189.90.151; 91.189.89.192;  };