Merge lp:~lamont/maas/dns-1.9 into lp:maas/1.9

[LaMont Jones]

Correctly manage reverse DNS: do not manage a /16 because the user added a /22 network.

Correctly manage reverse DNS: do not manage a /16 because the user added a /22 network.

[Andres Rodriguez (andreserl)]

I thought that we agarres that since this was a change of behavior we were
going to defer this?

On Thursday, January 7, 2016, LaMont Jones <email address hidden>
wrote:

> LaMont Jones has proposed merging lp:~lamont/maas/dns-1.9 into lp:maas/1.9.
>
> Commit message:
> Correctly manage reverse DNS: do not manage a /16 because the user added a
> /22 network.
>
> Requested reviews:
> MAAS Committers (maas-committers)
> Related bugs:
> Bug #1356012 in MAAS: "maas incorrectly overmanages DNS reverse zones"
> https://bugs.launchpad.net/maas/+bug/1356012
> Bug #1531868 in MAAS: "Add network fails for multiple bigger-than-/24
> networks in a single /16"
> https://bugs.launchpad.net/maas/+bug/1531868
>
> For more details, see:
> https://code.launchpad.net/~lamont/maas/dns-1.9/+merge/281879
>
> Correctly manage reverse DNS: do not manage a /16 because the user added a
> /22 network.
> --
> Your team MAAS Committers is requested to review the proposed merge of
> lp:~lamont/maas/dns-1.9 into lp:maas/1.9.
>

--
Andres Rodriguez (RoAkSoAx)
Ubuntu Server Developer
MSc. Telecom & Networking
Systems Engineer

[LaMont Jones (lamont)]

That was before a maas 1.8 user reported #1531868, wherein maas tries to reload DNS for changes, and fails utterly to do so, because the named.conf is invalid.

I think that this bugfix warrants reconsideration since it's breaking maas 1.8 users in the field.

[Andres Rodriguez (andreserl)]

Ok, let's discuss this next week.

Thanks!

On Thu, Jan 14, 2016 at 2:00 AM, LaMont Jones <email address hidden>
wrote:

> That was before a maas 1.8 user reported #1531868, wherein maas tries to
> reload DNS for changes, and fails utterly to do so, because the named.conf
> is invalid.
>
> I think that this bugfix warrants reconsideration since it's breaking maas
> 1.8 users in the field.
> --
> https://code.launchpad.net/~lamont/maas/dns-1.9/+merge/281879
> Your team MAAS Committers is requested to review the proposed merge of
> lp:~lamont/maas/dns-1.9 into lp:maas/1.9.
>

--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.

[lp:~lamont/maas/dns-1.9]

4520. By LaMont Jones on 2016-02-17

Merge from maas/1.9.

[Mike Pontillo (mpontillo)]

Does this still need review, or is it WIP? (I was under the impression that this code has changed significantly in trunk, and would need to be ported back to 1.9 in order for it to be usable there. Is that correct?)

Unmerged revisions

4520. By LaMont Jones on 2016-02-17

Merge from maas/1.9.

4519. By LaMont Jones on 2015-11-28

Cleanup errors introduced in doing review feedback

4518. By LaMont Jones on 2015-11-28

Review feedback

4517. By LaMont Jones on 2015-11-28

For reverse zones, get_GENERATE_directives needs to follow rfc2317.

4516. By LaMont Jones on 2015-11-28

Correctly cast subnet.first to avoid errors with trusty's netaddr.

4515. By LaMont Jones on 2015-11-27

Merge DNS fixes from trunk

=== modified file 'etc/maas/templates/dns/named.conf.template'
--- etc/maas/templates/dns/named.conf.template	2014-11-14 21:50:42 +0000
+++ etc/maas/templates/dns/named.conf.template	2016-02-17 21:43:45 +0000
@@ -2,11 +2,13 @@
 
 # Zone declarations.
 {{for zone in zones}}
-zone "{{zone.zone_name}}" {
+{{for zoneinfo in zone.zone_info}}
+zone "{{zoneinfo.zone_name}}" {
     type master;
-    file "{{zone.target_path}}";
+    file "{{zoneinfo.target_path}}";
 };
 {{endfor}}
+{{endfor}}
 
 # Access control for recursive queries.  See named.conf.options.inside.maas
 # for the directives used on this ACL.
 
=== modified file 'src/maasserver/dns/config.py'
--- src/maasserver/dns/config.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/dns/config.py	2016-02-17 21:43:45 +0000
@@ -50,7 +50,7 @@
     bind_reconfigure,
     bind_reload,
     bind_reload_with_retries,
-    bind_reload_zone,
+    bind_reload_zones,
     bind_write_configuration,
     bind_write_options,
     bind_write_zones,
@@ -144,9 +144,10 @@
 
     serial = next_zone_serial()
     for zone in ZoneGenerator(clusters, serial):
-        maaslog.info("Generating new DNS zone file for %s", zone.zone_name)
+        names = [zi.zone_name for zi in zone.zone_info]
+        maaslog.info("Generating new DNS zone file for %s", " ".join(names))
         bind_write_zones([zone])
-        bind_reload_zone(zone.zone_name)
+        bind_reload_zones(names)
 
 
 def dns_update_zones(clusters):
 
=== modified file 'src/provisioningserver/dns/actions.py'
--- src/provisioningserver/dns/actions.py	2015-10-22 00:06:43 +0000
+++ src/provisioningserver/dns/actions.py	2016-02-17 21:43:45 +0000
@@ -15,7 +15,7 @@
 __all__ = [
     "bind_reconfigure",
     "bind_reload",
-    "bind_reload_zone",
+    "bind_reload_zones",
     "bind_write_configuration",
     "bind_write_options",
     "bind_write_zones",
@@ -87,21 +87,26 @@
             sleep(interval)
 
 
-def bind_reload_zone(zone_name):
+def bind_reload_zones(zone_list):
     """Ask BIND to reload the zone file for the given zone.
 
-    :param zone_name: The name of the zone to reload.
+    :param zone_list: A list of zone names to reload, or a single name as a
+        string.
     :return: True if success, False otherwise.
     """
-    try:
-        execute_rndc_command(("reload", zone_name))
-        return True
-    except CalledProcessError as exc:
-        maaslog.error(
-            "Reloading BIND zone %r failed (is it running?): %s",
-            zone_name,
-            exc)
-        return False
+    ret = True
+    if not isinstance(zone_list, list):
+        zone_list = [zone_list]
+    for name in zone_list:
+        try:
+            execute_rndc_command(("reload", name))
+        except CalledProcessError as exc:
+            maaslog.error(
+                "Reloading BIND zone %r failed (is it running?): %s",
+                name,
+                exc)
+            ret = False
+    return ret
 
 
 def bind_write_configuration(zones, trusted_networks):
 
=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2015-07-09 22:25:27 +0000
+++ src/provisioningserver/dns/config.py	2016-02-17 21:43:45 +0000
@@ -29,6 +29,7 @@
 import re
 import sys
 
+from provisioningserver.logger import get_maas_logger
 from provisioningserver.utils import locate_config
 from provisioningserver.utils.fs import atomic_write
 from provisioningserver.utils.isc import read_isc_file
@@ -36,6 +37,7 @@
 import tempita
 
 
+maaslog = get_maas_logger("dns")
 NAMED_CONF_OPTIONS = 'named.conf.options'
 MAAS_NAMED_CONF_NAME = 'named.conf.maas'
 MAAS_NAMED_CONF_OPTIONS_INSIDE_NAME = 'named.conf.options.inside.maas'
@@ -319,7 +321,16 @@
 
     @classmethod
     def get_include_snippet(cls):
-        target_path = compose_config_path(cls.target_file_name)
-        assert '"' not in target_path, (
-            "DNS config path contains quote: %s." % target_path)
-        return 'include "%s";\n' % target_path
+        snippet = ""
+        if isinstance(cls.target_file_name, list):
+            target_file_names = cls.target_file_name
+        else:
+            target_file_names = [cls.target_file_name]
+        for target_file_name in target_file_names:
+            target_path = compose_config_path(target_file_name)
+            if '"' in target_path:
+                maaslog.error(
+                    "DNS config path contains quote: %s." % target_path)
+            else:
+                snippet += 'include "%s";\n' % target_path
+        return snippet
 
=== modified file 'src/provisioningserver/dns/tests/test_actions.py'
--- src/provisioningserver/dns/tests/test_actions.py	2015-10-22 00:06:43 +0000
+++ src/provisioningserver/dns/tests/test_actions.py	2016-02-17 21:43:45 +0000
@@ -144,11 +144,11 @@
 
 
 class TestReloadZone(MAASTestCase):
-    """Tests for :py:func:`actions.bind_reload_zone`."""
+    """Tests for :py:func:`actions.bind_reload_zones`."""
 
     def test__executes_rndc_command(self):
         self.patch_autospec(actions, "execute_rndc_command")
-        self.assertTrue(actions.bind_reload_zone(sentinel.zone))
+        self.assertTrue(actions.bind_reload_zones(sentinel.zone))
         self.assertThat(
             actions.execute_rndc_command,
             MockCalledOnceWith(("reload", sentinel.zone)))
@@ -157,7 +157,7 @@
         erc = self.patch_autospec(actions, "execute_rndc_command")
         erc.side_effect = factory.make_CalledProcessError()
         with FakeLogger("maas") as logger:
-            self.assertFalse(actions.bind_reload_zone(sentinel.zone))
+            self.assertFalse(actions.bind_reload_zones(sentinel.zone))
         self.assertDocTestMatches(
             "Reloading BIND zone ... failed (is it running?): "
             "Command ... returned non-zero exit status ...",
@@ -166,7 +166,7 @@
     def test__false_on_subprocess_error(self):
         erc = self.patch_autospec(actions, "execute_rndc_command")
         erc.side_effect = factory.make_CalledProcessError()
-        self.assertFalse(actions.bind_reload_zone(sentinel.zone))
+        self.assertFalse(actions.bind_reload_zones(sentinel.zone))
 
 
 class TestConfiguration(PservTestCase):
 
=== modified file 'src/provisioningserver/dns/tests/test_zoneconfig.py'
--- src/provisioningserver/dns/tests/test_zoneconfig.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/dns/tests/test_zoneconfig.py	2016-02-17 21:43:45 +0000
@@ -37,6 +37,7 @@
 from provisioningserver.dns.zoneconfig import (
     DNSForwardZoneConfig,
     DNSReverseZoneConfig,
+    DNSZoneInfo,
 )
 from testtools.matchers import (
     Contains,
@@ -102,7 +103,7 @@
         dns_zone_config = DNSForwardZoneConfig(domain)
         self.assertEqual(
             os.path.join(get_dns_config_dir(), 'zone.%s' % domain),
-            dns_zone_config.target_path)
+            dns_zone_config.zone_info[0].target_path)
 
     def test_get_a_mapping_returns_ipv4_mapping(self):
         name = factory.make_string()
@@ -266,7 +267,7 @@
             factory.make_string(), serial=random.randint(1, 100),
             dns_ip=factory.make_ipv4_address())
         dns_zone_config.write_config()
-        filepath = FilePath(dns_zone_config.target_path)
+        filepath = FilePath(dns_zone_config.zone_info[0].target_path)
         self.assertTrue(filepath.getPermissions().other.read)
 
 
@@ -290,34 +291,109 @@
 
     def test_computes_dns_config_file_paths(self):
         domain = factory.make_name('zone')
-        reverse_file_name = 'zone.168.192.in-addr.arpa'
+        reverse_file_name = [
+            'zone.%d.168.192.in-addr.arpa' % i for i in range(4)]
         dns_zone_config = DNSReverseZoneConfig(
             domain, network=IPNetwork("192.168.0.0/22"))
+        for i in range(4):
+            self.assertEqual(
+                os.path.join(get_dns_config_dir(), reverse_file_name[i]),
+                dns_zone_config.zone_info[i].target_path)
+
+    def test_computes_dns_config_file_paths_for_small_network(self):
+        domain = factory.make_name('zone')
+        reverse_file_name = 'zone.192-27.0.168.192.in-addr.arpa'
+        dns_zone_config = DNSReverseZoneConfig(
+            domain, network=IPNetwork("192.168.0.192/27"))
+        self.assertEqual(1, len(dns_zone_config.zone_info))
         self.assertEqual(
             os.path.join(get_dns_config_dir(), reverse_file_name),
-            dns_zone_config.target_path)
+            dns_zone_config.zone_info[0].target_path)
 
     def test_reverse_zone_file(self):
         # DNSReverseZoneConfig calculates the reverse zone file name
         # correctly for IPv4 and IPv6 networks.
+        # As long as the network size ends on a "nice" boundary (multiple of
+        # 8 for IPv4, multiple of 4 for IPv6), then there will be one reverse
+        # zone for the subnet.  When it isn't, then there will be 2-128
+        # individual reverse zones for the subnet.
+        # A special case is the small subnet (less than 256 hosts for IPv4,
+        # less than 16 hosts for IPv6), in which case, we follow RFC2317 with
+        # the modern adjustment of using '-' instead of '/'.
+        zn = "%d.0.0.0.0.0.0.0.0.0.0.0.4.0.1.f.1.0.8.a.b.0.1.0.0.2.ip6.arpa"
         expected = [
             # IPv4 networks.
-            (IPNetwork('192.168.0.1/22'), '168.192.in-addr.arpa'),
-            (IPNetwork('192.168.0.1/24'), '0.168.192.in-addr.arpa'),
+            # /22 ==> 4 /24 reverse zones
+            (
+                IPNetwork('192.168.0.1/22'),
+                [DNSZoneInfo(
+                    IPNetwork('192.168.%d.0/24' % i),
+                    '%d.168.192.in-addr.arpa' % i) for i in range(4)]
+            ),
+            # /24 ==> 1 reverse zone
+            (
+                IPNetwork('192.168.0.1/24'),
+                [DNSZoneInfo(
+                    IPNetwork('192.168.0.0/24'),
+                    '0.168.192.in-addr.arpa')]
+            ),
+            # /29 ==> 1 reverse zones, per RFC2317
+            (
+                IPNetwork('192.168.0.241/29'),
+                [DNSZoneInfo(
+                    IPNetwork('192.168.0.240/29'),
+                    '240-29.0.168.192.in-addr.arpa')]
+            ),
             # IPv6 networks.
-            (IPNetwork('3ffe:801::/32'), '1.0.8.0.e.f.f.3.ip6.arpa'),
-            (IPNetwork('2001:db8:0::/48'), '0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa'),
+            # /32, 48, 56, 64 ==> 1 reverse zones
+            (
+                IPNetwork('3ffe:801::/32'),
+                [DNSZoneInfo(
+                    IPNetwork('3ffe:801::32'),
+                    '1.0.8.0.e.f.f.3.ip6.arpa')]),
+            (
+                IPNetwork('2001:db8:0::/48'),
+                [DNSZoneInfo(
+                    IPNetwork('2001:db8:0::/48'),
+                    '0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa')]
+            ),
             (
                 IPNetwork('2001:ba8:1f1:400::/56'),
-                '4.0.1.f.1.0.8.a.b.0.1.0.0.2.ip6.arpa'
+                [DNSZoneInfo(
+                    IPNetwork('2001:ba8:1f1:400::/56'),
+                    '4.0.1.f.1.0.8.a.b.0.1.0.0.2.ip6.arpa')],
             ),
             (
                 IPNetwork('2610:8:6800:1::/64'),
-                '1.0.0.0.0.0.8.6.8.0.0.0.0.1.6.2.ip6.arpa',
-            ),
+                [DNSZoneInfo(
+                    IPNetwork('2610:8:6800:1::/64'),
+                    '1.0.0.0.0.0.8.6.8.0.0.0.0.1.6.2.ip6.arpa')],
+            ),
+            # /2 with hex digits ==> 4 /4 reverse zones
+            (
+                IPNetwork('8000::/2'),
+                [
+                    DNSZoneInfo(IPNetwork('8000::/4'), '8.ip6.arpa'),
+                    DNSZoneInfo(IPNetwork('9000::/4'), '9.ip6.arpa'),
+                    DNSZoneInfo(IPNetwork('a000::/4'), 'a.ip6.arpa'),
+                    DNSZoneInfo(IPNetwork('b000::/4'), 'b.ip6.arpa'),
+                ],
+            ),
+            # /103 ==> 2 /104 reverse zones
             (
                 IPNetwork('2001:ba8:1f1:400::/103'),
-                '0.0.0.0.0.0.0.0.0.0.0.4.0.1.f.1.0.8.a.b.0.1.0.0.2.ip6.arpa',
+                [DNSZoneInfo(
+                    IPNetwork('2001:ba8:1f1:400:0:0:%d00:0000/104' % i),
+                    zn % i) for i in range(2)],
+            ),
+            # /125 ==> 1 reverse zone, based on RFC2317
+            (
+                IPNetwork('2001:ba8:1f1:400::/125'),
+                [DNSZoneInfo(
+                    IPNetwork('2001:ba8:1f1:400::/125'),
+                    "0-125.%s" % (
+                        IPAddress('2001:ba8:1f1:400::').reverse_dns[2:-1]))],
+
             ),
 
         ]
@@ -325,8 +401,15 @@
         for network, _ in expected:
             domain = factory.make_name('zone')
             dns_zone_config = DNSReverseZoneConfig(domain, network=network)
-            results.append((network, dns_zone_config.zone_name))
-        self.assertEqual(expected, results)
+            results.append((network, dns_zone_config.zone_info))
+        # Make sure we have the right number of elements.
+        self.assertEqual(len(expected), len(results))
+        # And that the zone names chosen for each element are correct.
+        for net in range(len(expected)):
+            for zi in range(len(expected[net][1])):
+                self.assertItemsEqual(
+                    expected[net][1][zi].zone_name,
+                    results[net][1][zi].zone_name)
 
     def test_get_ptr_mapping(self):
         name = factory.make_string()
@@ -378,11 +461,12 @@
             factory.make_string(), serial=random.randint(1, 100),
             network=network)
         dns_zone_config.write_config()
-        self.assertThat(
-            os.path.join(
-                target_dir, 'zone.%s' % dns_zone_config.zone_name),
-            FileContains(
-                matcher=Contains('IN  NS  %s.' % dns_zone_config.domain)))
+        for zone_name in [zi.zone_name for zi in dns_zone_config.zone_info]:
+            self.assertThat(
+                os.path.join(
+                    target_dir, 'zone.%s' % zone_name),
+                FileContains(
+                    matcher=Contains('IN  NS  %s.' % dns_zone_config.domain)))
 
     def test_writes_reverse_dns_zone_config(self):
         target_dir = patch_dns_config_path(self)
@@ -394,9 +478,41 @@
             dynamic_ranges=[
                 IPRange(dynamic_network.first, dynamic_network.last)])
         dns_zone_config.write_config()
-        reverse_file_name = 'zone.168.192.in-addr.arpa'
-        expected_generate_directives = dns_zone_config.get_GENERATE_directives(
-            dynamic_network, domain)
+        for sub in range(4):
+            reverse_file_name = 'zone.%d.168.192.in-addr.arpa' % sub
+            expected_GEN_direct = dns_zone_config.get_GENERATE_directives(
+                dynamic_network, domain,
+                DNSZoneInfo(
+                    IPNetwork('192.168.%d.0/24' % sub),
+                    "%d.168.192.in-addr.arpa" % sub))
+            expected = ContainsAll(
+                [
+                    'IN  NS  %s' % domain
+                ] +
+                [
+                    '$GENERATE %s %s IN PTR %s' % (
+                        iterator_values, reverse_dns, hostname)
+                    for iterator_values, reverse_dns, hostname in
+                    expected_GEN_direct
+                ])
+            self.assertThat(
+                os.path.join(target_dir, reverse_file_name),
+                FileContains(matcher=expected))
+
+    def test_writes_reverse_dns_zone_config_for_small_network(self):
+        target_dir = patch_dns_config_path(self)
+        domain = factory.make_string()
+        network = IPNetwork('192.168.0.1/26')
+        dynamic_network = IPNetwork('192.168.0.1/28')
+        dns_zone_config = DNSReverseZoneConfig(
+            domain, serial=random.randint(1, 100), network=network,
+            dynamic_ranges=[
+                IPRange(dynamic_network.first, dynamic_network.last)])
+        dns_zone_config.write_config()
+        reverse_zone_name = '0-26.0.168.192.in-addr.arpa'
+        reverse_file_name = 'zone.0-26.0.168.192.in-addr.arpa'
+        expected_GEN_direct = dns_zone_config.get_GENERATE_directives(
+            dynamic_network, domain, DNSZoneInfo(network, reverse_zone_name))
         expected = ContainsAll(
             [
                 'IN  NS  %s' % domain
@@ -405,7 +521,7 @@
                 '$GENERATE %s %s IN PTR %s' % (
                     iterator_values, reverse_dns, hostname)
                 for iterator_values, reverse_dns, hostname in
-                expected_generate_directives
+                expected_GEN_direct
             ])
         self.assertThat(
             os.path.join(target_dir, reverse_file_name),
@@ -431,8 +547,9 @@
             factory.make_string(), serial=random.randint(1, 100),
             network=factory.make_ipv4_network())
         dns_zone_config.write_config()
-        filepath = FilePath(dns_zone_config.target_path)
-        self.assertTrue(filepath.getPermissions().other.read)
+        for tgt in [zi.target_path for zi in dns_zone_config.zone_info]:
+            filepath = FilePath(tgt)
+            self.assertTrue(filepath.getPermissions().other.read)
 
 
 class TestDNSReverseZoneConfig_GetGenerateDirectives(MAASTestCase):
@@ -451,7 +568,11 @@
         self.assertItemsEqual(
             expected_directives,
             DNSReverseZoneConfig.get_GENERATE_directives(
-                ip_range, domain="domain"))
+                ip_range,
+                domain="domain",
+                zone_info=DNSZoneInfo(
+                    IPNetwork('192.168.0.0/16'),
+                    "168.192.in-addr.arpa")))
 
     def get_expected_generate_directives(self, network, domain):
         ip_parts = network.network.format().split('.')
@@ -481,8 +602,11 @@
             relevant_ip_parts.reverse()
             expected_rdns_base = (
                 "%s.%s.in-addr.arpa." % tuple(relevant_ip_parts))
-            expected_rdns_template = "$.%s.%s" % (
-                num + second_octet_offset, expected_rdns_base)
+            if network.size >= 256:
+                expected_rdns_template = "$.%s.%s" % (
+                    num + second_octet_offset, expected_rdns_base)
+            else:
+                expected_rdns_template = "$"
             expected_generate_directives.append(
                 (
                     "%s-%s" % (iterator_low, iterator_high),
@@ -494,22 +618,25 @@
 
     def test_returns_single_entry_for_slash_24_network(self):
         network = IPNetwork("%s/24" % factory.make_ipv4_address())
+        reverse = ".".join(IPAddress(network).reverse_dns.split('.')[1:-1])
         domain = factory.make_string()
         expected_generate_directives = self.get_expected_generate_directives(
             network, domain)
         directives = DNSReverseZoneConfig.get_GENERATE_directives(
-            network, domain)
+            network, domain, DNSZoneInfo(network, reverse))
         self.expectThat(directives, HasLength(1))
         self.assertItemsEqual(expected_generate_directives, directives)
 
     def test_returns_single_entry_for_tiny_network(self):
         network = IPNetwork("%s/28" % factory.make_ipv4_address())
+        reverse = IPAddress(network).reverse_dns.split('.')
+        reverse = ".".join(["%s-28" % reverse[0]] + reverse[1:-1])
         domain = factory.make_string()
 
         expected_generate_directives = self.get_expected_generate_directives(
             network, domain)
         directives = DNSReverseZoneConfig.get_GENERATE_directives(
-            network, domain)
+            network, domain, DNSZoneInfo(network, reverse))
         self.expectThat(directives, HasLength(1))
         self.assertItemsEqual(expected_generate_directives, directives)
 
@@ -517,31 +644,22 @@
         ip_range = IPRange('10.0.0.1', '10.0.0.255')
         domain = factory.make_string()
         directives = DNSReverseZoneConfig.get_GENERATE_directives(
-            ip_range, domain)
+            ip_range, domain,
+            DNSZoneInfo(IPNetwork('10.0.0.0/24'), '0.0.10.in-addr.arpa'))
         self.expectThat(directives, HasLength(1))
 
-    def test_dtrt_for_larger_networks(self):
-        # For every other network size that we're not explicitly
-        # testing here,
-        # DNSReverseZoneConfig.get_GENERATE_directives() will return
-        # one GENERATE directive for every 255 addresses in the network.
-        for prefixlen in range(23, 17):
-            network = IPNetwork(
-                "%s/%s" % (factory.make_ipv4_address(), prefixlen))
-            domain = factory.make_string()
-            directives = DNSReverseZoneConfig.get_GENERATE_directives(
-                network, domain)
-            self.expectThat(directives, HasLength(network.size / 256))
-
-    def test_returns_two_entries_for_slash_23_network(self):
-        network = IPNetwork(factory.make_ipv4_network(slash=23))
+    # generate 2 zones, rather than 1 zone with 2 GENERATEs.
+    def test_returns_256_entries_for_slash_16_network(self):
+        network = IPNetwork(factory.make_ipv4_network(slash=16))
+        reverse = IPAddress(network.first).reverse_dns.split('.')[2:-1]
+        reverse = ".".join(reverse)
         domain = factory.make_string()
 
         expected_generate_directives = self.get_expected_generate_directives(
             network, domain)
         directives = DNSReverseZoneConfig.get_GENERATE_directives(
-            network, domain)
-        self.expectThat(directives, HasLength(2))
+            network, domain, DNSZoneInfo(network, reverse))
+        self.expectThat(directives, HasLength(256))
         self.assertItemsEqual(expected_generate_directives, directives)
 
     def test_ignores_network_larger_than_slash_16(self):
@@ -549,7 +667,8 @@
         self.assertEqual(
             [],
             DNSReverseZoneConfig.get_GENERATE_directives(
-                network, factory.make_string()))
+                network, factory.make_string(),
+                DNSZoneInfo(network, "do not care")))
 
     def test_ignores_networks_that_span_slash_16s(self):
         # If the upper and lower bounds of a range span two /16 networks
@@ -557,7 +676,8 @@
         # get_GENERATE_directives() will return early
         ip_range = IPRange('10.0.0.55', '10.1.0.54')
         directives = DNSReverseZoneConfig.get_GENERATE_directives(
-            ip_range, factory.make_string())
+            ip_range, factory.make_string(),
+            DNSZoneInfo(IPNetwork('10.0.0.0/15'), "do not care"))
         self.assertEqual([], directives)
 
     def test_sorts_output_by_hostname(self):
@@ -568,12 +688,19 @@
         expected_rdns = "$.%s.0.10.in-addr.arpa."
 
         directives = list(DNSReverseZoneConfig.get_GENERATE_directives(
-            network, domain))
+            network, domain,
+            DNSZoneInfo(IPNetwork('10.0.0.0/24'), '0.0.10.in-addr.arpa')))
         self.expectThat(
             directives[0], Equals(
                 ("0-255", expected_rdns % "0", expected_hostname % "0")))
+
+        expected_hostname = "10-0-%s-$." + domain + "."
+        expected_rdns = "$.%s.0.10.in-addr.arpa."
+        directives = list(DNSReverseZoneConfig.get_GENERATE_directives(
+            network, domain,
+            DNSZoneInfo(IPNetwork('10.0.1.0/24'), '1.0.10.in-addr.arpa')))
         self.expectThat(
-            directives[1], Equals(
+            directives[0], Equals(
                 ("0-255", expected_rdns % "1", expected_hostname % "1")))
 
 
 
=== modified file 'src/provisioningserver/dns/zoneconfig.py'
--- src/provisioningserver/dns/zoneconfig.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/dns/zoneconfig.py	2016-02-17 21:43:45 +0000
@@ -15,13 +15,13 @@
 __all__ = [
     'DNSForwardZoneConfig',
     'DNSReverseZoneConfig',
+    'DNSZoneInfo',
     ]
 
 
 from abc import ABCMeta
 from datetime import datetime
 from itertools import chain
-import math
 
 from netaddr import (
     IPAddress,
@@ -106,6 +106,24 @@
     return intersecting_subnets, prefix, rdns_suffix
 
 
+class DNSZoneInfo:
+    """Information about a DNS zone"""
+
+    def __init__(self, subnetwork, zone_name, target_path=None):
+        """
+        :param subnetwork: IPNetwork that this zone (chunk) is for.  None
+            for forward zones.
+        :param zone_name: Fully-qualified zone name
+        :param target_path: Optional, can be used to override the target path.
+        """
+        self.subnetwork = subnetwork
+        self.zone_name = zone_name
+        if target_path is None:
+            self.target_path = compose_config_path('zone.%s' % zone_name)
+        else:
+            self.target_path = target_path
+
+
 class DNSZoneConfigBase:
     """Base class for zone writers."""
 
@@ -113,17 +131,17 @@
 
     template_file_name = 'zone.template'
 
-    def __init__(self, domain, zone_name, serial=None):
+    def __init__(self, domain, zone_info, serial=None):
         """
         :param domain: The domain name of the forward zone.
-        :param zone_name: Fully-qualified zone name.
+        :param zone_info: list of DNSZoneInfo entries.
         :param serial: The serial to use in the zone file. This must increment
             on each change.
         """
         self.domain = domain
-        self.zone_name = zone_name
         self.serial = serial
-        self.target_path = compose_config_path('zone.%s' % self.zone_name)
+        self.zone_info = zone_info
+        self.target_base = compose_config_path('zone')
 
     def make_parameters(self):
         """Return a dict of the common template parameters."""
@@ -142,9 +160,12 @@
         support a resolution of one second, and so this method may set an
         unexpected modification time in order to maintain that property.
         """
-        content = render_dns_template(cls.template_file_name, *parameters)
-        with report_missing_config_dir():
-            incremental_write(content, output_file, mode=0644)
+        if not isinstance(output_file, list):
+            output_file = [output_file]
+        for outfile in output_file:
+            content = render_dns_template(cls.template_file_name, *parameters)
+            with report_missing_config_dir():
+                incremental_write(content, outfile, mode=0644)
 
 
 class DNSForwardZoneConfig(DNSZoneConfigBase):
@@ -175,7 +196,7 @@
         self._dynamic_ranges = kwargs.pop('dynamic_ranges', [])
         self._srv_mapping = kwargs.pop('srv_mapping', [])
         super(DNSForwardZoneConfig, self).__init__(
-            domain, zone_name=domain, **kwargs)
+            domain, zone_info=[DNSZoneInfo(None, domain)], **kwargs)
 
     @classmethod
     def get_mapping(cls, mapping, domain, dns_ip):
@@ -255,7 +276,7 @@
             return []
 
         generate_directives = set()
-        subnets, prefix, _ = get_details_for_ip_range(dynamic_range)
+        subnets, prefix, rdns_suffix = get_details_for_ip_range(dynamic_range)
         for subnet in subnets:
             iterator = "%d-%d" % (
                 (subnet.first & 0x000000ff),
@@ -279,27 +300,28 @@
     def write_config(self):
         """Write the zone file."""
         # Create GENERATE directives for IPv4 ranges.
-        generate_directives = list(
-            chain.from_iterable(
-                self.get_GENERATE_directives(dynamic_range)
-                for dynamic_range in self._dynamic_ranges
-                if dynamic_range.version == 4
-            ))
-        self.write_zone_file(
-            self.target_path, self.make_parameters(),
-            {
-                'mappings': {
-                    'SRV': self.get_srv_mapping(
-                        self._srv_mapping),
-                    'A': self.get_A_mapping(
-                        self._mapping, self.domain, self._dns_ip),
-                    'AAAA': self.get_AAAA_mapping(
-                        self._mapping, self.domain, self._dns_ip),
-                },
-                'generate_directives': {
-                    'A': generate_directives,
-                }
-            })
+        for zi in self.zone_info:
+            generate_directives = list(
+                chain.from_iterable(
+                    self.get_GENERATE_directives(dynamic_range)
+                    for dynamic_range in self._dynamic_ranges
+                    if dynamic_range.version == 4
+                ))
+            self.write_zone_file(
+                zi.target_path, self.make_parameters(),
+                {
+                    'mappings': {
+                        'SRV': self.get_srv_mapping(
+                            self._srv_mapping),
+                        'A': self.get_A_mapping(
+                            self._mapping, self.domain, self._dns_ip),
+                        'AAAA': self.get_AAAA_mapping(
+                            self._mapping, self.domain, self._dns_ip),
+                    },
+                    'generate_directives': {
+                        'A': generate_directives,
+                    }
+                })
 
 
 class DNSReverseZoneConfig(DNSZoneConfigBase):
@@ -325,32 +347,88 @@
         self._mapping = kwargs.pop('mapping', {})
         self._network = kwargs.pop("network", None)
         self._dynamic_ranges = kwargs.pop('dynamic_ranges', [])
-        zone_name = self.compose_zone_name(self._network)
+        zone_info = self.compose_zone_info(self._network)
         super(DNSReverseZoneConfig, self).__init__(
-            domain, zone_name=zone_name, **kwargs)
+            domain, zone_info=zone_info, **kwargs)
 
     @classmethod
-    def compose_zone_name(cls, network):
-        """Return the name of the reverse zone."""
+    def compose_zone_info(cls, network):
+        """Return the names of the reverse zones."""
         # Generate the name of the reverse zone file:
         # Use netaddr's reverse_dns() to get the reverse IP name
         # of the first IP address in the network and then drop the first
         # octets of that name (i.e. drop the octets that will be specified in
         # the zone file).
+        # returns a list of (IPNetwork, zone_name, zonefile_path) tuples
+        info = []
         first = IPAddress(network.first)
+        last = IPAddress(network.last)
         if first.version == 6:
             # IPv6.
-            # Use float division and ceil to cope with network sizes that
-            # are not divisible by 4.
-            rest_limit = int(math.ceil((128 - network.prefixlen) / 4.))
+            # 2001:89ab::/19 yields 8.1.0.0.2.ip6.arpa, and the full list
+            # is 8.1.0.0.2.ip6.arpa, 9.1.0.0.2.ip6.arpa
+            # The ipv6 reverse dns form is 32 elements of 1 hex digit each.
+            # How many elements of the reverse DNS name to we throw away?
+            # Prefixlen of 0-3 gives us 1, 4-7 gives us 2, etc.
+            # While this seems wrong, we always _add_ a base label back in,
+            # so it's correct.
+            rest_limit = (132 - network.prefixlen) / 4
+            # What is the prefix for each inner subnet (It will be the next
+            # smaller multiple of 4.)  If it's the smallest one, then RFC2317
+            # tells us that we're adding an extra blob to the front of the
+            # reverse zone name, and we want the entire prefixlen.
+            subnet_prefix = (network.prefixlen + 3) / 4 * 4
+            if subnet_prefix == 128:
+                subnet_prefix = network.prefixlen
+            # How big is the step between subnets?  Again, special case for
+            # extra small subnets.
+            step = 1 << ((128 - network.prefixlen) / 4 * 4)
+            if step < 16:
+                step = 16
+            # Grab the base (hex) and trailing labels for our reverse zone.
+            split_zone = first.reverse_dns.split('.')
+            zone_rest = ".".join(split_zone[rest_limit:-1])
+            base = int(split_zone[rest_limit - 1], 16)
         else:
             # IPv4.
-            # Use float division and ceil to cope with splits not done on
-            # octets boundaries.
-            rest_limit = int(math.ceil((32 - network.prefixlen) / 8.))
-        reverse_name = first.reverse_dns.split('.', rest_limit)[-1]
-        # Strip off trailing '.'.
-        return reverse_name[:-1]
+            # The logic here is the same as for IPv6, but with 8 instead of 4.
+            rest_limit = (40 - network.prefixlen) / 8
+            subnet_prefix = (network.prefixlen + 7) / 8 * 8
+            if subnet_prefix == 32:
+                subnet_prefix = network.prefixlen
+            step = 1 << ((32 - network.prefixlen) / 8 * 8)
+            if step < 256:
+                step = 256
+            # Grab the base (decimal) and trailing labels for our reverse
+            # zone.
+            split_zone = first.reverse_dns.split('.')
+            zone_rest = ".".join(split_zone[rest_limit:-1])
+            base = int(split_zone[rest_limit - 1])
+        while first <= last:
+            # Rest_limit has bounds of 1..labelcount+1 (5 or 33).
+            # If we're stripping any elements, then we just want base.name.
+            if rest_limit > 1:
+                if first.version == 6:
+                    new_zone = "%x.%s" % (base, zone_rest)
+                else:
+                    new_zone = "%d.%s" % (base, zone_rest)
+            # We didn't actually strip any elemnts, so base goes back with
+            # the prefixlen attached.
+            elif first.version == 6:
+                new_zone = "%x-%d.%s" % (base, network.prefixlen, zone_rest)
+            else:
+                new_zone = "%d-%d.%s" % (base, network.prefixlen, zone_rest)
+            info.append(DNSZoneInfo(
+                IPNetwork("%s/%d" % (first, subnet_prefix)),
+                new_zone))
+            base += 1
+            try:
+                first += step
+            except IndexError:
+                # IndexError occurs when we go from 255.255.255.255 to
+                # 0.0.0.0.  If we hit that, we're all fine and done.
+                break
+        return info
 
     @classmethod
     def get_PTR_mapping(cls, mapping, domain, network):
@@ -366,7 +444,7 @@
         :param mapping: A hostname:ip-addresses mapping for all known hosts in
             the reverse zone.
         :param domain: Zone's domain name.
-        :param network: Zone's network.
+        :param network: DNS Zone's network.
         :type network: :class:`netaddr.IPNetwork`
         """
         return (
@@ -380,7 +458,7 @@
         )
 
     @classmethod
-    def get_GENERATE_directives(cls, dynamic_range, domain):
+    def get_GENERATE_directives(cls, dynamic_range, domain, zone_info):
         """Return the GENERATE directives for the reverse zone of a network."""
         slash_16 = IPNetwork("%s/16" % IPAddress(dynamic_range.first))
         if (dynamic_range.size > 256 ** 2 or
@@ -391,19 +469,29 @@
             return []
 
         generate_directives = set()
+        # The largest subnet returned is a /24.
         subnets, prefix, rdns_suffix = get_details_for_ip_range(dynamic_range)
         for subnet in subnets:
-            iterator = "%d-%d" % (
-                (subnet.first & 0x000000ff),
-                (subnet.last & 0x000000ff))
-            hostname = "%s-%d-$" % (
-                prefix.replace('.', '-'),
-                (subnet.first & 0x0000ff00) >> 8)
-            rdns = "$.%d.%s" % (
-                (subnet.first & 0x0000ff00) >> 8,
-                rdns_suffix)
-            generate_directives.add(
-                (iterator, rdns, "%s.%s." % (hostname, domain)))
+            if (IPAddress(subnet.first) in zone_info.subnetwork):
+                iterator = "%d-%d" % (
+                    (subnet.first & 0x000000ff),
+                    (subnet.last & 0x000000ff))
+                hostname = "%s-%d-$" % (
+                    prefix.replace('.', '-'),
+                    (subnet.first & 0x0000ff00) >> 8)
+                # If we're at least a /24, then fully specify the name,
+                # rather than trying to figure out how much of the name
+                # is in the zone name.
+                if zone_info.subnetwork.prefixlen <= 24:
+                    rdns = "$.%d.%s" % (
+                        (subnet.first & 0x0000ff00) >> 8,
+                        rdns_suffix)
+                else:
+                    # Let the zone declaration provide the suffix.
+                    # rather than trying to calculate it.
+                    rdns = "$"
+                generate_directives.add(
+                    (iterator, rdns, "%s.%s." % (hostname, domain)))
 
         return sorted(
             generate_directives, key=lambda directive: directive[2])
@@ -411,21 +499,25 @@
     def write_config(self):
         """Write the zone file."""
         # Create GENERATE directives for IPv4 ranges.
-        generate_directives = list(
-            chain.from_iterable(
-                self.get_GENERATE_directives(dynamic_range, self.domain)
-                for dynamic_range in self._dynamic_ranges
-                if dynamic_range.version == 4
-            ))
-        self.write_zone_file(
-            self.target_path, self.make_parameters(),
-            {
-                'mappings': {
-                    'PTR': self.get_PTR_mapping(
-                        self._mapping, self.domain, self._network),
-                },
-                'generate_directives': {
-                    'PTR': generate_directives,
+        for zi in self.zone_info:
+            generate_directives = list(
+                chain.from_iterable(
+                    self.get_GENERATE_directives(
+                        dynamic_range,
+                        self.domain,
+                        zi)
+                    for dynamic_range in self._dynamic_ranges
+                    if dynamic_range.version == 4
+                ))
+            self.write_zone_file(
+                zi.target_path, self.make_parameters(),
+                {
+                    'mappings': {
+                        'PTR': self.get_PTR_mapping(
+                            self._mapping, self.domain, zi.subnetwork),
+                    },
+                    'generate_directives': {
+                        'PTR': generate_directives,
+                    }
                 }
-            }
-        )
+            )