MAAS

Merge lp:~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190 into lp:~maas-committers/maas/trunk

generate-dns-for-dynamic-pool-bug-1382190
Merge into trunk

Proposed by Graham Binns on 2014-10-28

Status:

Merged

Approved by:

Graham Binns on 2014-10-29

Approved revision:

no longer in the source branch.

Merged at revision:

3308

Proposed branch:

lp:~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190

Merge into:

lp:~maas-committers/maas/trunk

Diff against target:

802 lines (+551/-20)

6 files modified

etc/maas/templates/dns/zone.template (+6/-0)
src/maasserver/dns/tests/test_config.py (+4/-4)
src/maasserver/dns/tests/test_zonegenerator.py (+20/-3)
src/maasserver/dns/zonegenerator.py (+26/-7)
src/provisioningserver/dns/tests/test_zoneconfig.py (+353/-4)
src/provisioningserver/dns/zoneconfig.py (+142/-2)

To merge this branch:

bzr merge lp:~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190

Critical

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Raphaël Badin (community)		Approve on 2014-10-29
Gavin Panella (community)	2014-10-28	Approve on 2014-10-29
Review via email: mp+239912@code.launchpad.net

Commit message

Using $GENERATE directives, generate DNS A and PTR records for all addresses in each managed NodeGroupInterface's dynamic range, as long as that dynamic range uses IPv4.

Previously, hosts with addresses in the dynamic range were not resolvable through DNS, which caused problems with some Juju charms.

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-28:

Marking this work-in-progress because I think I see several ways that I can improve this branch before it lands — the advantage of a couple of hours of not looking at it.

Revision history for this message

Gavin Panella (allenap) wrote on 2014-10-29:

Partial review. More after lunch!

Revision history for this message

Gavin Panella (allenap) wrote on 2014-10-29:

Looks good. I have a lot of comments, and there's a 255/256 issue in several places that needs your attention (I may be wrong), but +1 anyway. I'm happy to look again before landing if you want.

review: Approve

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-29:

Download full text (34.1 KiB)

On 29 October 2014 12:25, Gavin Panella <email address hidden> wrote:
> Partial review. More after lunch!
>
> Diff comments:
>
>> === modified file 'etc/maas/templates/dns/zone.template'
>> --- etc/maas/templates/dns/zone.template 2012-10-04 15:59:56 +0000
>> +++ etc/maas/templates/dns/zone.template 2014-10-29 11:33:01 +0000
>> @@ -12,6 +12,12 @@
>> )
>>
>> IN NS {{domain}}.
>> +{{for type, directive in generate_directives.items()}}
>> +{{for iterator_values, rdns, hostname in directive}}
>> +$GENERATE {{iterator_values}} {{rdns}} IN {{type}} {{hostname}}
>> +{{endfor}}
>> +{{endfor}}
>> +
>> {{for type, mapping in mappings.items()}}
>> {{for item_from, item_to in mapping}}
>> {{item_from}} IN {{type}} {{item_to}}
>>
>> === modified file 'src/maasserver/dns/tests/test_zonegenerator.py'
>> --- src/maasserver/dns/tests/test_zonegenerator.py 2014-09-15 14:28:28 +0000
>> +++ src/maasserver/dns/tests/test_zonegenerator.py 2014-10-29 11:33:01 +0000
>> @@ -393,14 +393,28 @@
>> [interface] = nodegroup.get_managed_interfaces()
>> networks_dict = ZoneGenerator._get_networks()
>> retrieved_interface = networks_dict[nodegroup]
>> - self.assertEqual([interface.network], retrieved_interface)
>> + self.assertEqual(
>> + [
>> + (
>> + interface.network,
>> + (interface.ip_range_low, interface.ip_range_high)
>
> Consider using an IPRange here. NodeGroupInterface.get_dynamic_ip_range() returns one.
>
>
>> + )
>> + ],
>> + retrieved_interface)
>>
>> def test_get_networks_returns_multiple_networks(self):
>> nodegroups = [self.make_node_group() for _ in range(3)]
>> networks_dict = ZoneGenerator._get_networks()
>> for nodegroup in nodegroups:
>> [interface] = nodegroup.get_managed_interfaces()
>> - self.assertEqual([interface.network], networks_dict[nodegroup])
>> + self.assertEqual(
>> + [
>> + (
>> + interface.network,
>> + (interface.ip_range_low, interface.ip_range_high),
>> + ),
>> + ],
>> + networks_dict[nodegroup])
>>
>> def test_get_networks_returns_managed_networks(self):
>> nodegroups = [
>> @@ -415,7 +429,10 @@
>> self.assertEqual(
>> {
>> nodegroup: [
>> - interface.network
>> + (
>> + interface.network,
>> + (interface.ip_range_low, interface.ip_range_high),
>> + )
>> for interface in nodegroup.get_managed_interfaces()
>> ]
>> for nodegroup in nodegroups
>>
>> === modified file 'src/maasserver/dns/zonegenerator.py'
>> --- src/maasserver/dns/zonegenerator.py 2014-08-28 12:00:15 +0000
>> +++ src/maasserver/dns/zonegenerator.py 2014-10-29 11:33:01 +0000
>> @@ -29,7 +29,10 @@
>> from maasserver.models.config import Config
>> from maasserver.models.nod...

On 29 October 2014 12:25, Gavin Panella <gavin.panella@canonical.com> wrote:
> Partial review. More after lunch!
>
> Diff comments:
>
>> === modified file 'etc/maas/templates/dns/zone.template'
>> --- etc/maas/templates/dns/zone.template      2012-10-04 15:59:56 +0000
>> +++ etc/maas/templates/dns/zone.template      2014-10-29 11:33:01 +0000
>> @@ -12,6 +12,12 @@
>>                )
>>
>>      IN  NS  {{domain}}.
>> +{{for type, directive in generate_directives.items()}}
>> +{{for iterator_values, rdns, hostname in directive}}
>> +$GENERATE {{iterator_values}} {{rdns}} IN {{type}} {{hostname}}
>> +{{endfor}}
>> +{{endfor}}
>> +
>>  {{for type, mapping in mappings.items()}}
>>  {{for item_from, item_to in mapping}}
>>  {{item_from}} IN {{type}} {{item_to}}
>>
>> === modified file 'src/maasserver/dns/tests/test_zonegenerator.py'
>> --- src/maasserver/dns/tests/test_zonegenerator.py    2014-09-15 14:28:28 +0000
>> +++ src/maasserver/dns/tests/test_zonegenerator.py    2014-10-29 11:33:01 +0000
>> @@ -393,14 +393,28 @@
>>          [interface] = nodegroup.get_managed_interfaces()
>>          networks_dict = ZoneGenerator._get_networks()
>>          retrieved_interface = networks_dict[nodegroup]
>> -        self.assertEqual([interface.network], retrieved_interface)
>> +        self.assertEqual(
>> +            [
>> +                (
>> +                    interface.network,
>> +                    (interface.ip_range_low, interface.ip_range_high)
>
> Consider using an IPRange here. NodeGroupInterface.get_dynamic_ip_range() returns one.
>
>
>> +                )
>> +            ],
>> +            retrieved_interface)
>>
>>      def test_get_networks_returns_multiple_networks(self):
>>          nodegroups = [self.make_node_group() for _ in range(3)]
>>          networks_dict = ZoneGenerator._get_networks()
>>          for nodegroup in nodegroups:
>>              [interface] = nodegroup.get_managed_interfaces()
>> -            self.assertEqual([interface.network], networks_dict[nodegroup])
>> +            self.assertEqual(
>> +                [
>> +                    (
>> +                        interface.network,
>> +                        (interface.ip_range_low, interface.ip_range_high),
>> +                    ),
>> +                ],
>> +                networks_dict[nodegroup])
>>
>>      def test_get_networks_returns_managed_networks(self):
>>          nodegroups = [
>> @@ -415,7 +429,10 @@
>>          self.assertEqual(
>>              {
>>                  nodegroup: [
>> -                    interface.network
>> +                    (
>> +                        interface.network,
>> +                        (interface.ip_range_low, interface.ip_range_high),
>> +                    )
>>                      for interface in nodegroup.get_managed_interfaces()
>>                      ]
>>                  for nodegroup in nodegroups
>>
>> === modified file 'src/maasserver/dns/zonegenerator.py'
>> --- src/maasserver/dns/zonegenerator.py       2014-08-28 12:00:15 +0000
>> +++ src/maasserver/dns/zonegenerator.py       2014-10-29 11:33:01 +0000
>> @@ -29,7 +29,10 @@
>>  from maasserver.models.config import Config
>>  from maasserver.models.nodegroup import NodeGroup
>>  from maasserver.server_address import get_maas_facing_server_address
>> -from netaddr import IPAddress
>> +from netaddr import (
>> +    IPAddress,
>> +    IPRange,
>> +    )
>>  from provisioningserver.dns.zoneconfig import (
>>      DNSForwardZoneConfig,
>>      DNSReverseZoneConfig,
>> @@ -180,9 +183,15 @@
>>
>>      @staticmethod
>>      def _get_networks():
>> -        """Return a lazily evaluated nodegroup:network dict."""
>> -        return lazydict(lambda ng: [
>> -            interface.network for interface in ng.get_managed_interfaces()])
>> +        """Return a lazily evaluated nodegroup:network_details dict.
>> +
>> +        network_details takes the form of a tuple of (network,
>> +        (network.ip_range_low, network.ip_range_high)).
>> +        """
>> +        return lazydict(lambda ng: [(
>> +            interface.network,
>> +            (interface.ip_range_low, interface.ip_range_high))
>> +            for interface in ng.get_managed_interfaces()])
>
> This is hard to read. How about:
>
>         def get_network(nodegroup):
>             return [
>                 (iface.network, (iface.ip_range_low, iface.ip_range_high))
>                 for iface in nodegroup.get_managed_interfaces()
>             ]
>
>         return lazydict(get_network)

Ok.

> ?
>
>>
>>      @staticmethod
>>      def _get_srv_mappings():
>> @@ -209,6 +218,12 @@
>>          forward_nodegroups = sorted(nodegroups, key=get_domain)
>>          for domain, nodegroups in groupby(forward_nodegroups, get_domain):
>>              nodegroups = list(nodegroups)
>> +            dynamic_ranges = []
>> +            for nodegroup in nodegroups:
>> +                dynamic_ranges.extend(
>> +                    IPRange(interface.ip_range_low, interface.ip_range_high)
>
> NodeGroupInterface.get_dynamic_ip_range() will DTRT here.
>
> Also, you could write it in a single expression:
>
>             dynamic_ranges = [
>                 interface.get_dynamic_ip_range()
>                 for nodegroup in nodegroups
>                 for interface in nodegroup.get_managed_interfaces()
>             ]

Good points, both.

> They nest the same way as in a "normal" nested loop.
>
>> +                    for interface in nodegroup.get_managed_interfaces()
>> +                )
>>              # A forward zone encompassing all nodes in the same domain.
>>              yield DNSForwardZoneConfig(
>>                  domain, serial=serial, dns_ip=dns_ip,
>> @@ -217,7 +232,8 @@
>>                      for nodegroup in nodegroups
>>                      for hostname, ip in mappings[nodegroup].items()
>>                      },
>> -                srv_mapping=set(srv_mappings)
>> +                srv_mapping=set(srv_mappings),
>> +                dynamic_ranges=dynamic_ranges,
>>              )
>>
>>      @staticmethod
>> @@ -226,11 +242,11 @@
>>          get_domain = lambda nodegroup: nodegroup.name
>>          reverse_nodegroups = sorted(nodegroups, key=networks.get)
>>          for nodegroup in reverse_nodegroups:
>> -            for network in networks[nodegroup]:
>> +            for network, dynamic_range in networks[nodegroup]:
>>                  mapping = mappings[nodegroup]
>>                  yield DNSReverseZoneConfig(
>>                      get_domain(nodegroup), serial=serial, mapping=mapping,
>> -                    network=network
>> +                    network=network, dynamic_ranges=[IPRange(*dynamic_range)]
>>                  )
>>
>>      def __iter__(self):
>>
>> === modified file 'src/provisioningserver/dns/tests/test_zoneconfig.py'
>> --- src/provisioningserver/dns/tests/test_zoneconfig.py       2014-09-23 21:43:27 +0000
>> +++ src/provisioningserver/dns/tests/test_zoneconfig.py       2014-10-29 11:33:01 +0000
>> @@ -26,7 +26,9 @@
>>  from netaddr import (
>>      IPAddress,
>>      IPNetwork,
>> +    IPRange,
>>      )
>> +from maastesting.matchers import MockNotCalled
>>  from provisioningserver.dns.config import (
>>      get_dns_config_dir,
>>      SRVRecord,
>> @@ -35,15 +37,18 @@
>>  from provisioningserver.dns.zoneconfig import (
>>      DNSForwardZoneConfig,
>>      DNSReverseZoneConfig,
>> +    get_hostname_template_for_network,
>>      )
>>  from testtools.matchers import (
>>      Contains,
>>      ContainsAll,
>> +    Equals,
>>      FileContains,
>>      IsInstance,
>>      MatchesAll,
>>      MatchesStructure,
>>      Not,
>> +    HasLength,
>>      )
>>  from twisted.python.filepath import FilePath
>>
>> @@ -187,10 +192,13 @@
>>              ipv4_hostname: [ipv4_ip],
>>              ipv6_hostname: [ipv6_ip],
>>          }
>> +        expected_generate_directives = (
>> +            DNSForwardZoneConfig.get_GENERATE_directives(network))
>>          srv = self.make_srv_record()
>>          dns_zone_config = DNSForwardZoneConfig(
>>              domain, serial=random.randint(1, 100),
>> -            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv])
>> +            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv],
>> +            dynamic_ranges=[IPRange(network.first, network.last)])
>>          dns_zone_config.write_config()
>>          self.assertThat(
>>              os.path.join(target_dir, 'zone.%s' % domain),
>> @@ -201,6 +209,12 @@
>>                              srv.service, self.get_srv_item_output(srv)),
>>                          '%s IN A %s' % (ipv4_hostname, ipv4_ip),
>>                          '%s IN AAAA %s' % (ipv6_hostname, ipv6_ip),
>> +                    ] +
>> +                    [
>> +                        '$GENERATE %s %s IN A %s' % (
>> +                            iterator_values, reverse_dns, hostname)
>> +                        for iterator_values, reverse_dns, hostname in
>> +                        expected_generate_directives
>>                      ]
>>                  )
>>              )
>> @@ -222,6 +236,31 @@
>>                          '%s. IN A %s' % (dns_zone_config.domain, dns_ip),
>>                      ])))
>>
>> +    def test_ignores_generate_directives_for_v6_dynamic_ranges(self):
>> +        patch_dns_config_path(self)
>> +        domain = factory.make_string()
>> +        network = factory.make_ipv4_network()
>> +        dns_ip = factory.pick_ip_in_network(network)
>> +        ipv4_hostname = factory.make_name('host')
>> +        ipv4_ip = factory.pick_ip_in_network(network)
>> +        ipv6_hostname = factory.make_name('host')
>> +        ipv6_ip = factory.make_ipv6_address()
>> +        ipv6_network = IPNetwork("%s/64" % ipv6_ip)
>> +        dynamic_range = IPRange(ipv6_network.first, ipv6_network.last)
>> +        mapping = {
>> +            ipv4_hostname: [ipv4_ip],
>> +            ipv6_hostname: [ipv6_ip],
>> +        }
>> +        srv = self.make_srv_record()
>> +        dns_zone_config = DNSForwardZoneConfig(
>> +            domain, serial=random.randint(1, 100),
>> +            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv],
>> +            dynamic_ranges=[dynamic_range])
>> +        get_generate_directives = self.patch(
>> +            dns_zone_config, 'get_GENERATE_directives')
>> +        dns_zone_config.write_config()
>> +        self.assertThat(get_generate_directives, MockNotCalled())
>> +
>>      def test_config_file_is_world_readable(self):
>>          patch_dns_config_path(self)
>>          dns_zone_config = DNSForwardZoneConfig(
>> @@ -350,16 +389,43 @@
>>          target_dir = patch_dns_config_path(self)
>>          domain = factory.make_string()
>>          network = IPNetwork('192.168.0.1/22')
>> +        dynamic_network = IPNetwork('192.168.0.1/28')
>>          dns_zone_config = DNSReverseZoneConfig(
>> -            domain, serial=random.randint(1, 100), network=network)
>> +            domain, serial=random.randint(1, 100), network=network,
>> +            dynamic_ranges=[
>> +                IPRange(dynamic_network.first, dynamic_network.last)])
>>          dns_zone_config.write_config()
>>          reverse_file_name = 'zone.168.192.in-addr.arpa'
>> -        expected = Contains(
>> -            'IN  NS  %s' % domain)
>> +        expected_generate_directives = dns_zone_config.get_GENERATE_directives(
>> +            dynamic_network, domain)
>> +        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_generate_directives
>> +            ])
>>          self.assertThat(
>>              os.path.join(target_dir, reverse_file_name),
>>              FileContains(matcher=expected))
>>
>> +    def test_ignores_generate_directives_for_v6_dynamic_ranges(self):
>> +        patch_dns_config_path(self)
>> +        domain = factory.make_string()
>> +        network = IPNetwork('192.168.0.1/22')
>> +        dynamic_network = IPNetwork("%s/64" % factory.make_ipv6_address())
>
> There is make_ipv6_network() that'll do this for you.

There is!

>> +        dns_zone_config = DNSReverseZoneConfig(
>> +            domain, serial=random.randint(1, 100), network=network,
>> +            dynamic_ranges=[
>> +                IPRange(dynamic_network.first, dynamic_network.last)])
>> +        get_generate_directives = self.patch(
>> +            dns_zone_config, 'get_GENERATE_directives')
>> +        dns_zone_config.write_config()
>> +        self.assertThat(get_generate_directives, MockNotCalled())
>> +
>>      def test_reverse_config_file_is_world_readable(self):
>>          patch_dns_config_path(self)
>>          dns_zone_config = DNSReverseZoneConfig(
>> @@ -368,3 +434,270 @@
>>          dns_zone_config.write_config()
>>          filepath = FilePath(dns_zone_config.target_path)
>>          self.assertTrue(filepath.getPermissions().other.read)
>> +
>> +
>> +class TestDNSReverseZoneConfig_GetGenerateDirectives(MAASTestCase):
>> +    """Tests for `DNSReverseZoneConfig.get_GENERATE_directives()`."""
>> +
>> +    def get_expected_generate_directives(self, network, domain):
>> +        ip_parts = network.network.format().split('.')
>> +        relevant_ip_parts = ip_parts[:-2]
>> +
>> +        first_address = IPAddress(network.first).format()
>> +        first_address_parts = first_address.split(".")
>> +
>> +        if network.size < 255:
>
> Do you mean 256 here?

Yes (And to all the below).

>> +            last_address = IPAddress(network.last).format()
>> +            iterator_low = int(first_address_parts[-1])
>> +            iterator_high = last_address.split('.')[-1]
>> +        else:
>> +            iterator_low = 0
>> +            iterator_high = 255
>> +
>> +        second_octet_offset = int(first_address_parts[-2])
>
> first_address_parts[-2] is the third octet.
>
>> +        expected_generate_directives = []
>> +        directives_needed = network.size / 255
>
> Do you mean 256?
>
>> +
>> +        if directives_needed == 0:
>> +            directives_needed = 1
>> +        for num in range(directives_needed):
>> +            expected_address_base = "%s-%s" % tuple(relevant_ip_parts)
>> +            expected_address = "%s-%s-$" % (
>> +                expected_address_base, num + second_octet_offset)
>> +            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)
>> +            expected_generate_directives.append(
>> +                (
>> +                    "%s-%s" % (iterator_low, iterator_high),
>> +                    expected_rdns_template,
>> +                    "%s.%s." % (expected_address, domain)
>> +                ))
>> +            relevant_ip_parts.reverse()
>> +        return expected_generate_directives
>> +
>> +    def test_returns_single_entry_for_slash_24_network(self):
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        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(1))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_returns_single_entry_for_tiny_network(self):
>> +        network = IPNetwork("%s/28" % factory.make_ipv4_address())
>> +        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(1))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_returns_single_entry_for_weird_small_range(self):
>> +        ip_range = IPRange('10.0.0.1', '10.0.0.255')
>> +        domain = factory.make_string()
>> +        directives = DNSReverseZoneConfig.get_GENERATE_directives(
>> +            ip_range, domain)
>> +        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 / 255))
>> +
>> +    def test_returns_two_entries_for_slash_23_network(self):
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>> +        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))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_rejects_network_larger_than_slash_16(self):
>> +        network = IPNetwork("%s/15" % factory.make_ipv4_address())
>> +        error = self.assertRaises(
>> +            AssertionError, DNSReverseZoneConfig.get_GENERATE_directives,
>> +            network, None)
>> +        self.assertEqual(
>> +            "Cannot generate reverse zone mapping for any network larger "
>> +            "than /16.",
>> +            unicode(error))
>> +
>> +    def test_sorts_output(self):
>> +        network = IPNetwork("10.0.0.1/23")
>> +        domain = factory.make_string()
>> +
>> +        expected_hostname = "10-0-%s-$." + domain + "."
>> +        expected_rdns = "$.%s.0.10.in-addr.arpa."
>> +
>> +        directives = list(DNSReverseZoneConfig.get_GENERATE_directives(
>> +            network, domain))
>> +        self.expectThat(
>> +            directives[0], Equals(
>> +                ("0-255", expected_rdns % "0", expected_hostname % "0")))
>> +        self.expectThat(
>> +            directives[1], Equals(
>> +                ("0-255", expected_rdns % "1", expected_hostname % "1")))
>> +
>> +
>> +class TestDNSForwardZoneConfig_GetGenerateDirectives(MAASTestCase):
>> +    """Tests for `DNSForwardZoneConfig.get_GENERATE_directives()`."""
>> +
>> +    def get_expected_generate_directives(self, network):
>> +        ip_parts = network.network.format().split('.')
>> +        ip_parts[-1] = "$"
>> +        expected_hostname = "%s" % "-".join(ip_parts)
>> +        expected_address = ".".join(ip_parts)
>> +
>> +        first_address = IPAddress(network.first).format()
>> +        first_address_parts = first_address.split(".")
>> +        last_address = IPAddress(network.last).format()
>> +        last_address_parts = last_address.split(".")
>> +
>> +        if network.size < 255:
>> +            iterator_low = int(first_address_parts[-1])
>> +            if iterator_low == 0:
>> +                iterator_low = 1
>> +            iterator_high = last_address_parts[-1]
>> +        else:
>> +            iterator_low = 0
>> +            iterator_high = 255
>> +
>> +        expected_iterator_values = "%s-%s" % (iterator_low, iterator_high)
>> +
>> +        directives_needed = network.size / 255
>> +        if directives_needed == 0:
>> +            directives_needed = 1
>> +        expected_directives = []
>> +        for num in range(directives_needed):
>> +            ip_parts[-2] = unicode(num + int(ip_parts[-2]))
>> +            expected_address = ".".join(ip_parts)
>> +            expected_hostname = "%s" % "-".join(ip_parts)
>> +            expected_directives.append(
>> +                (expected_iterator_values, expected_hostname, expected_address))
>> +        return expected_directives
>> +
>> +    def test_returns_single_entry_for_slash_24_network(self):
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.expectThat(directives, HasLength(1))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_returns_single_entry_for_tiny_network(self):
>> +        network = IPNetwork("%s/31" % factory.make_ipv4_address())
>> +
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.assertEqual(1, len(expected_directives))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_returns_two_entries_for_slash_23_network(self):
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>> +
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.assertEqual(2, len(expected_directives))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_dtrt_for_larger_networks(self):
>> +        # For every other network size that we're not explicitly
>> +        # testing here,
>> +        # DNSForwardZoneConfig.get_GENERATE_directives() will return
>> +        # one GENERATE directive for every 255 addresses in the network.
>> +        for prefixlen in range(23, 16):
>> +            network = IPNetwork(
>> +                "%s/%s" % (factory.make_ipv4_address(), prefixlen))
>> +            directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +                network)
>> +            self.assertIsEqual(network.size / 255, len(directives))
>> +
>> +    def test_rejects_network_larger_than_slash_16(self):
>> +        network = IPNetwork("%s/15" % factory.make_ipv4_address())
>> +        error = self.assertRaises(
>> +            AssertionError, DNSForwardZoneConfig.get_GENERATE_directives,
>> +            network)
>> +        self.assertEqual(
>> +            "Cannot generate reverse zone mapping for any network larger "
>> +            "than /16.",
>> +            unicode(error))
>> +
>> +    def test_sorts_output(self):
>> +        network = IPNetwork("10.0.0.0/23")
>> +
>> +        expected_hostname = "10-0-%s-$"
>> +        expected_address = "10.0.%s.$"
>> +
>> +        directives = list(DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network))
>> +        self.expectThat(len(directives), Equals(2))
>> +        self.expectThat(
>> +            directives[0], Equals(
>> +                ("0-255", expected_hostname % "0", expected_address % "0")))
>> +        self.expectThat(
>> +            directives[1], Equals(
>> +                ("0-255", expected_hostname % "1", expected_address % "1")))
>> +
>> +
>> +class TestGetHostnameTemplateForNetwork(MAASTestCase):
>> +    """Tests for get_hostname_template_for_network()."""
>> +
>> +    def test_returns_sane_template_for_slash_24(self):
>> +        # For /24 networks, get_hostname_template_for_network() converts
>> +        # the last octet of the network address into a $GENERATE
>> +        # iterator placeholder.
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network))
>> +
>> +    def test_returns_sane_template_for_multiple_subnets(self):
>> +        # For networks larger than /24,
>> +        # get_hostname_template_for_network() converts the last octet of
>> +        # the network address into a $GENERATE iterator placeholder, and
>> +        # adds a "%(next_octet)s" placeholder in the place of the
>> +        # penultimate octet.
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        net_addr_parts[-2] = "%(next_octet)s"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network, subnets=2))
>> +
>> +    def test_returns_sane_template_for_smaller_networks(self):
>> +        # For networks smaller than /24,
>> +        # get_hostname_template_for_network() converts the last octet of
>> +        # the network address into a $GENERATE iterator placeholder, but
>> +        # leaves the rest of the address untouched.
>> +        network = IPNetwork("%s/26" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network))
>>
>> === modified file 'src/provisioningserver/dns/zoneconfig.py'
>> --- src/provisioningserver/dns/zoneconfig.py  2014-08-15 11:10:09 +0000
>> +++ src/provisioningserver/dns/zoneconfig.py  2014-10-29 11:33:01 +0000
>> @@ -23,7 +23,7 @@
>>  from itertools import chain
>>  import math
>>
>> -from netaddr import IPAddress
>> +from netaddr import IPAddress, IPRange, spanning_cidr
>>  from netaddr.core import AddrFormatError
>>  from provisioningserver.dns.config import (
>>      compose_config_path,
>> @@ -33,6 +33,16 @@
>>  from provisioningserver.utils.fs import incremental_write
>>
>>
>> +def intersect_iprange(network, iprange):
>> +    # IPSet is notoriously inefficient so we intersect ourselves here.
>> +    if network.last >= iprange.first and network.first <= iprange.last:
>> +        first = max(network.first, iprange.first)
>> +        last = min(network.last, iprange.last)
>> +        return IPRange(first, last)
>> +    else:
>> +        return None
>> +
>> +
>>  def get_fqdn_or_ip_address(target):
>>      """Returns the ip address is target is a valid ip address, otherwise
>>      returns the target with appended '.' if missing."""
>> @@ -52,6 +62,70 @@
>>              yield hostname, ip
>>
>>
>> +def get_details_for_ip_range(ip_range):
>> +    """For a given IPRange, return all subnets and a useable prefix.
>> +    :return: A tuple of:
>> +        All subnets of /24 (or smaller if there is no /24 subnet to be
>> +        found) in `ip_range`.
>> +        A prefix made from the first two octets in the range.
>> +    """
>> +    # Calculate a spanning network for the range above. There are
>> +    # 256 /24 networks in a /16, so that's the most /24s we're going
>> +    # to have to deal with; this matters later on when we iterate
>> +    # through the /24s within this network.
>> +    cidr = spanning_cidr(ip_range)
>> +    subnets = []
>> +    for subnet_size in range (24, 33):
>> +        # Find the first working subnet, starting with 24 and
>> +        # working down to 32. This allows us to account for really
>> +        # small ranges.
>> +        subnets = list(cidr.subnet(subnet_size))
>> +        if len(subnets) > 0:
>> +            break
>> +
>> +    # Split the spanning network into /24 subnets, then see if they fall
>> +    # entirely within the original network range, partially, or not at
>> +    # all.
>> +    intersecting_subnets = []
>> +    for subnet in subnets:
>> +        intersect = intersect_iprange(subnet, ip_range)
>> +        if intersect is None:
>> +            # The subnet does not fall within the original network.
>> +            pass
>> +        else:
>> +            # The subnet falls partially within the original network, so print
>> +            # out a $GENERATE expression for a subset of the /24.
>> +            intersecting_subnets.append(intersect)
>> +
>> +    # The first two octets of the network range formatted in the
>> +    # usual dotted-quad style. We can precalculate this because
>> +    # we're only ever dealing with /16 networks and smaller.
>> +    prefix = "%d.%d" % (
>> +        (cidr.value & 0xff000000) >> 24,
>> +        (cidr.value & 0x00ff0000) >> 16,
>> +    )
>> +    # Similarly, we can calculate what the reverse DNS suffix is going
>> +    # to look like.
>> +    rdns_suffix = "%d.%d.in-addr.arpa." % (
>> +        (cidr.value & 0x00ff0000) >> 16,
>> +        (cidr.value & 0xff000000) >> 24,
>> +        )
>> +    return intersecting_subnets, prefix, rdns_suffix
>> +
>> +
>> +def get_hostname_template_for_network(network, subnets=1):
>> +    """For a given network, return a hostname template.
>> +
>> +    The hostname template is made to be used in $GENERATE directives.
>> +    """
>> +    net_addr = IPAddress(network.first).format()
>> +    hostname_parts = net_addr.split(".")
>> +    hostname_parts[-1] = '$'
>> +    if subnets > 1:
>> +        hostname_parts[-2] = "%(next_octet)s"
>> +    return "-".join(hostname_parts)
>> +
>> +
>>  class DNSZoneConfigBase:
>>      """Base class for zone writers."""
>>
>> @@ -117,7 +191,8 @@
>>          """
>>          self._dns_ip = kwargs.pop('dns_ip', None)
>>          self._mapping = kwargs.pop('mapping', {})
>> -        self._network = None
>> +        self._network = kwargs.pop('network', None)
>> +        self._dynamic_ranges = kwargs.pop('dynamic_ranges', [])
>>          self._srv_mapping = kwargs.pop('srv_mapping', [])
>>          super(DNSForwardZoneConfig, self).__init__(
>>              domain, zone_name=domain, **kwargs)
>> @@ -187,8 +262,40 @@
>>                  target)
>>              yield (record.service, item)
>>
>> +    @classmethod
>> +    def get_GENERATE_directives(cls, dynamic_range):
>> +        """Return the GENERATE directives for the forward zone of a network.
>> +        """
>> +        assert dynamic_range.size <= 256**2, (
>> +            "Cannot generate reverse zone mapping for any network larger than "
>> +            "/16.")
>> +
>> +        generate_directives = set()
>> +        subnets, prefix, _ = get_details_for_ip_range(dynamic_range)
>> +        for subnet in subnets:
>> +            iterator = "%d-%d" %  (
>> +                (subnet.first & 0x000000ff),
>> +                (subnet.last & 0x000000ff))
>> +            hostname = get_hostname_template_for_network(
>> +                subnet, len(subnets)) % {
>> +                    'next_octet': (subnet.first & 0x0000ff00) >> 8}
>> +            ip_address = "%s.%d.$" % (
>> +                prefix,
>> +                (subnet.first & 0x0000ff00) >> 8)
>> +            generate_directives.add((iterator, hostname, ip_address))
>> +
>> +        return sorted(generate_directives)
>> +
>>      def write_config(self):
>>          """Write the zone file."""
>> +        generate_directives = []
>> +        for dynamic_range in self._dynamic_ranges:
>> +            if dynamic_range.version == 4:
>> +                # Only create GENERATE directives for IPv4 ranges.
>> +                generate_directives.extend(
>> +                    self.get_GENERATE_directives(dynamic_range))
>> +            else:
>> +                pass
>>          self.write_zone_file(
>>              self.target_path, self.make_parameters(),
>>              {
>> @@ -200,6 +307,9 @@
>>                      'AAAA': self.get_AAAA_mapping(
>>                          self._mapping, self.domain, self._dns_ip),
>>                  },
>> +                'generate_directives': {
>> +                    'A': generate_directives,
>> +                }
>>              })
>>
>>
>> @@ -225,6 +335,7 @@
>>          """
>>          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)
>>          super(DNSReverseZoneConfig, self).__init__(
>>              domain, zone_name=zone_name, **kwargs)
>> @@ -279,8 +390,40 @@
>>              if IPAddress(ip) in network
>>          )
>>
>> +    @classmethod
>> +    def get_GENERATE_directives(cls, dynamic_range, domain):
>> +        """Return the GENERATE directives for the reverse zone of a network."""
>> +        assert dynamic_range.size <= 256**2, (
>> +            "Cannot generate reverse zone mapping for any network larger than "
>> +            "/16.")
>> +
>> +        generate_directives = set()
>> +        subnets, _, rdns_suffix = get_details_for_ip_range(dynamic_range)
>> +        for subnet in subnets:
>> +            iterator = "%d-%d" %  (
>> +                (subnet.first & 0x000000ff),
>> +                (subnet.last & 0x000000ff))
>> +            hostname = get_hostname_template_for_network(
>> +                subnet, len(subnets)) % {
>> +                    'next_octet': (subnet.first & 0x0000ff00) >> 8}
>> +            rdns = "$.%d.%s" % (
>> +                (subnet.first & 0x0000ff00) >> 8,
>> +                rdns_suffix)
>> +            generate_directives.add(
>> +                (iterator, rdns, "%s.%s." % (hostname, domain)))
>> +
>> +        return sorted(generate_directives)
>> +
>>      def write_config(self):
>>          """Write the zone file."""
>> +        generate_directives = []
>> +        for dynamic_range in self._dynamic_ranges:
>> +            if dynamic_range.version == 4:
>> +                # Only create GENERATE directives for IPv4 ranges.
>> +                generate_directives.extend(
>> +                    self.get_GENERATE_directives(dynamic_range, self.domain))
>> +            else:
>> +                pass
>>          self.write_zone_file(
>>              self.target_path, self.make_parameters(),
>>              {
>> @@ -288,5 +431,8 @@
>>                      'PTR': self.get_PTR_mapping(
>>                          self._mapping, self.domain, self._network),
>>                  },
>> +                'generate_directives': {
>> +                    'PTR': generate_directives,
>> +                }
>>              }
>>          )
>>
>
>
> --
> https://code.launchpad.net/~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190/+merge/239912
> You are the owner of lp:~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190.

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-29:

Download full text (34.7 KiB)

On 29 October 2014 13:54, Gavin Panella <email address hidden> wrote:
> Review: Approve
>
> Looks good. I have a lot of comments, and there's a 255/256 issue in several places that needs your attention (I may be wrong), but +1 anyway. I'm happy to look again before landing if you want.
>
> Diff comments:
>
>> === modified file 'etc/maas/templates/dns/zone.template'
>> --- etc/maas/templates/dns/zone.template 2012-10-04 15:59:56 +0000
>> +++ etc/maas/templates/dns/zone.template 2014-10-29 11:33:01 +0000
>> @@ -12,6 +12,12 @@
>> )
>>
>> IN NS {{domain}}.
>> +{{for type, directive in generate_directives.items()}}
>> +{{for iterator_values, rdns, hostname in directive}}
>> +$GENERATE {{iterator_values}} {{rdns}} IN {{type}} {{hostname}}
>> +{{endfor}}
>> +{{endfor}}
>> +
>> {{for type, mapping in mappings.items()}}
>> {{for item_from, item_to in mapping}}
>> {{item_from}} IN {{type}} {{item_to}}
>>
>> === modified file 'src/maasserver/dns/tests/test_zonegenerator.py'
>> --- src/maasserver/dns/tests/test_zonegenerator.py 2014-09-15 14:28:28 +0000
>> +++ src/maasserver/dns/tests/test_zonegenerator.py 2014-10-29 11:33:01 +0000
>> @@ -393,14 +393,28 @@
>> [interface] = nodegroup.get_managed_interfaces()
>> networks_dict = ZoneGenerator._get_networks()
>> retrieved_interface = networks_dict[nodegroup]
>> - self.assertEqual([interface.network], retrieved_interface)
>> + self.assertEqual(
>> + [
>> + (
>> + interface.network,
>> + (interface.ip_range_low, interface.ip_range_high)
>> + )
>> + ],
>> + retrieved_interface)
>>
>> def test_get_networks_returns_multiple_networks(self):
>> nodegroups = [self.make_node_group() for _ in range(3)]
>> networks_dict = ZoneGenerator._get_networks()
>> for nodegroup in nodegroups:
>> [interface] = nodegroup.get_managed_interfaces()
>> - self.assertEqual([interface.network], networks_dict[nodegroup])
>> + self.assertEqual(
>> + [
>> + (
>> + interface.network,
>> + (interface.ip_range_low, interface.ip_range_high),
>> + ),
>> + ],
>> + networks_dict[nodegroup])
>>
>> def test_get_networks_returns_managed_networks(self):
>> nodegroups = [
>> @@ -415,7 +429,10 @@
>> self.assertEqual(
>> {
>> nodegroup: [
>> - interface.network
>> + (
>> + interface.network,
>> + (interface.ip_range_low, interface.ip_range_high),
>> + )
>> for interface in nodegroup.get_managed_interfaces()
>> ]
>> for nodegroup in nodegroups
>>
>> === modified file 'src/maasserver/dns/zonegenerator.py'
>> --- src/maasserver/dns/zonegenerator.py 2014-08-28 12:00:15 +0000
>> +++ src/maasserver/dns/zonegenerator.py 2014-10-29 11:33:01 +0000
>> @@ -29,7 +2...

On 29 October 2014 13:54, Gavin Panella <gavin.panella@canonical.com> wrote:
> Review: Approve
>
> Looks good. I have a lot of comments, and there's a 255/256 issue in several places that needs your attention (I may be wrong), but +1 anyway. I'm happy to look again before landing if you want.
>
> Diff comments:
>
>> === modified file 'etc/maas/templates/dns/zone.template'
>> --- etc/maas/templates/dns/zone.template      2012-10-04 15:59:56 +0000
>> +++ etc/maas/templates/dns/zone.template      2014-10-29 11:33:01 +0000
>> @@ -12,6 +12,12 @@
>>                )
>>
>>      IN  NS  {{domain}}.
>> +{{for type, directive in generate_directives.items()}}
>> +{{for iterator_values, rdns, hostname in directive}}
>> +$GENERATE {{iterator_values}} {{rdns}} IN {{type}} {{hostname}}
>> +{{endfor}}
>> +{{endfor}}
>> +
>>  {{for type, mapping in mappings.items()}}
>>  {{for item_from, item_to in mapping}}
>>  {{item_from}} IN {{type}} {{item_to}}
>>
>> === modified file 'src/maasserver/dns/tests/test_zonegenerator.py'
>> --- src/maasserver/dns/tests/test_zonegenerator.py    2014-09-15 14:28:28 +0000
>> +++ src/maasserver/dns/tests/test_zonegenerator.py    2014-10-29 11:33:01 +0000
>> @@ -393,14 +393,28 @@
>>          [interface] = nodegroup.get_managed_interfaces()
>>          networks_dict = ZoneGenerator._get_networks()
>>          retrieved_interface = networks_dict[nodegroup]
>> -        self.assertEqual([interface.network], retrieved_interface)
>> +        self.assertEqual(
>> +            [
>> +                (
>> +                    interface.network,
>> +                    (interface.ip_range_low, interface.ip_range_high)
>> +                )
>> +            ],
>> +            retrieved_interface)
>>
>>      def test_get_networks_returns_multiple_networks(self):
>>          nodegroups = [self.make_node_group() for _ in range(3)]
>>          networks_dict = ZoneGenerator._get_networks()
>>          for nodegroup in nodegroups:
>>              [interface] = nodegroup.get_managed_interfaces()
>> -            self.assertEqual([interface.network], networks_dict[nodegroup])
>> +            self.assertEqual(
>> +                [
>> +                    (
>> +                        interface.network,
>> +                        (interface.ip_range_low, interface.ip_range_high),
>> +                    ),
>> +                ],
>> +                networks_dict[nodegroup])
>>
>>      def test_get_networks_returns_managed_networks(self):
>>          nodegroups = [
>> @@ -415,7 +429,10 @@
>>          self.assertEqual(
>>              {
>>                  nodegroup: [
>> -                    interface.network
>> +                    (
>> +                        interface.network,
>> +                        (interface.ip_range_low, interface.ip_range_high),
>> +                    )
>>                      for interface in nodegroup.get_managed_interfaces()
>>                      ]
>>                  for nodegroup in nodegroups
>>
>> === modified file 'src/maasserver/dns/zonegenerator.py'
>> --- src/maasserver/dns/zonegenerator.py       2014-08-28 12:00:15 +0000
>> +++ src/maasserver/dns/zonegenerator.py       2014-10-29 11:33:01 +0000
>> @@ -29,7 +29,10 @@
>>  from maasserver.models.config import Config
>>  from maasserver.models.nodegroup import NodeGroup
>>  from maasserver.server_address import get_maas_facing_server_address
>> -from netaddr import IPAddress
>> +from netaddr import (
>> +    IPAddress,
>> +    IPRange,
>> +    )
>>  from provisioningserver.dns.zoneconfig import (
>>      DNSForwardZoneConfig,
>>      DNSReverseZoneConfig,
>> @@ -180,9 +183,15 @@
>>
>>      @staticmethod
>>      def _get_networks():
>> -        """Return a lazily evaluated nodegroup:network dict."""
>> -        return lazydict(lambda ng: [
>> -            interface.network for interface in ng.get_managed_interfaces()])
>> +        """Return a lazily evaluated nodegroup:network_details dict.
>> +
>> +        network_details takes the form of a tuple of (network,
>> +        (network.ip_range_low, network.ip_range_high)).
>> +        """
>> +        return lazydict(lambda ng: [(
>> +            interface.network,
>> +            (interface.ip_range_low, interface.ip_range_high))
>> +            for interface in ng.get_managed_interfaces()])
>>
>>      @staticmethod
>>      def _get_srv_mappings():
>> @@ -209,6 +218,12 @@
>>          forward_nodegroups = sorted(nodegroups, key=get_domain)
>>          for domain, nodegroups in groupby(forward_nodegroups, get_domain):
>>              nodegroups = list(nodegroups)
>> +            dynamic_ranges = []
>> +            for nodegroup in nodegroups:
>> +                dynamic_ranges.extend(
>> +                    IPRange(interface.ip_range_low, interface.ip_range_high)
>> +                    for interface in nodegroup.get_managed_interfaces()
>> +                )
>>              # A forward zone encompassing all nodes in the same domain.
>>              yield DNSForwardZoneConfig(
>>                  domain, serial=serial, dns_ip=dns_ip,
>> @@ -217,7 +232,8 @@
>>                      for nodegroup in nodegroups
>>                      for hostname, ip in mappings[nodegroup].items()
>>                      },
>> -                srv_mapping=set(srv_mappings)
>> +                srv_mapping=set(srv_mappings),
>> +                dynamic_ranges=dynamic_ranges,
>>              )
>>
>>      @staticmethod
>> @@ -226,11 +242,11 @@
>>          get_domain = lambda nodegroup: nodegroup.name
>>          reverse_nodegroups = sorted(nodegroups, key=networks.get)
>>          for nodegroup in reverse_nodegroups:
>> -            for network in networks[nodegroup]:
>> +            for network, dynamic_range in networks[nodegroup]:
>>                  mapping = mappings[nodegroup]
>>                  yield DNSReverseZoneConfig(
>>                      get_domain(nodegroup), serial=serial, mapping=mapping,
>> -                    network=network
>> +                    network=network, dynamic_ranges=[IPRange(*dynamic_range)]
>>                  )
>>
>>      def __iter__(self):
>>
>> === modified file 'src/provisioningserver/dns/tests/test_zoneconfig.py'
>> --- src/provisioningserver/dns/tests/test_zoneconfig.py       2014-09-23 21:43:27 +0000
>> +++ src/provisioningserver/dns/tests/test_zoneconfig.py       2014-10-29 11:33:01 +0000
>> @@ -26,7 +26,9 @@
>>  from netaddr import (
>>      IPAddress,
>>      IPNetwork,
>> +    IPRange,
>>      )
>> +from maastesting.matchers import MockNotCalled
>>  from provisioningserver.dns.config import (
>>      get_dns_config_dir,
>>      SRVRecord,
>> @@ -35,15 +37,18 @@
>>  from provisioningserver.dns.zoneconfig import (
>>      DNSForwardZoneConfig,
>>      DNSReverseZoneConfig,
>> +    get_hostname_template_for_network,
>>      )
>>  from testtools.matchers import (
>>      Contains,
>>      ContainsAll,
>> +    Equals,
>>      FileContains,
>>      IsInstance,
>>      MatchesAll,
>>      MatchesStructure,
>>      Not,
>> +    HasLength,
>>      )
>>  from twisted.python.filepath import FilePath
>>
>> @@ -187,10 +192,13 @@
>>              ipv4_hostname: [ipv4_ip],
>>              ipv6_hostname: [ipv6_ip],
>>          }
>> +        expected_generate_directives = (
>> +            DNSForwardZoneConfig.get_GENERATE_directives(network))
>>          srv = self.make_srv_record()
>>          dns_zone_config = DNSForwardZoneConfig(
>>              domain, serial=random.randint(1, 100),
>> -            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv])
>> +            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv],
>> +            dynamic_ranges=[IPRange(network.first, network.last)])
>>          dns_zone_config.write_config()
>>          self.assertThat(
>>              os.path.join(target_dir, 'zone.%s' % domain),
>> @@ -201,6 +209,12 @@
>>                              srv.service, self.get_srv_item_output(srv)),
>>                          '%s IN A %s' % (ipv4_hostname, ipv4_ip),
>>                          '%s IN AAAA %s' % (ipv6_hostname, ipv6_ip),
>> +                    ] +
>> +                    [
>> +                        '$GENERATE %s %s IN A %s' % (
>> +                            iterator_values, reverse_dns, hostname)
>> +                        for iterator_values, reverse_dns, hostname in
>> +                        expected_generate_directives
>>                      ]
>>                  )
>>              )
>> @@ -222,6 +236,31 @@
>>                          '%s. IN A %s' % (dns_zone_config.domain, dns_ip),
>>                      ])))
>>
>> +    def test_ignores_generate_directives_for_v6_dynamic_ranges(self):
>> +        patch_dns_config_path(self)
>> +        domain = factory.make_string()
>> +        network = factory.make_ipv4_network()
>> +        dns_ip = factory.pick_ip_in_network(network)
>> +        ipv4_hostname = factory.make_name('host')
>> +        ipv4_ip = factory.pick_ip_in_network(network)
>> +        ipv6_hostname = factory.make_name('host')
>> +        ipv6_ip = factory.make_ipv6_address()
>> +        ipv6_network = IPNetwork("%s/64" % ipv6_ip)
>> +        dynamic_range = IPRange(ipv6_network.first, ipv6_network.last)
>> +        mapping = {
>> +            ipv4_hostname: [ipv4_ip],
>> +            ipv6_hostname: [ipv6_ip],
>> +        }
>> +        srv = self.make_srv_record()
>> +        dns_zone_config = DNSForwardZoneConfig(
>> +            domain, serial=random.randint(1, 100),
>> +            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv],
>> +            dynamic_ranges=[dynamic_range])
>> +        get_generate_directives = self.patch(
>> +            dns_zone_config, 'get_GENERATE_directives')
>> +        dns_zone_config.write_config()
>> +        self.assertThat(get_generate_directives, MockNotCalled())
>> +
>>      def test_config_file_is_world_readable(self):
>>          patch_dns_config_path(self)
>>          dns_zone_config = DNSForwardZoneConfig(
>> @@ -350,16 +389,43 @@
>>          target_dir = patch_dns_config_path(self)
>>          domain = factory.make_string()
>>          network = IPNetwork('192.168.0.1/22')
>> +        dynamic_network = IPNetwork('192.168.0.1/28')
>>          dns_zone_config = DNSReverseZoneConfig(
>> -            domain, serial=random.randint(1, 100), network=network)
>> +            domain, serial=random.randint(1, 100), network=network,
>> +            dynamic_ranges=[
>> +                IPRange(dynamic_network.first, dynamic_network.last)])
>>          dns_zone_config.write_config()
>>          reverse_file_name = 'zone.168.192.in-addr.arpa'
>> -        expected = Contains(
>> -            'IN  NS  %s' % domain)
>> +        expected_generate_directives = dns_zone_config.get_GENERATE_directives(
>> +            dynamic_network, domain)
>> +        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_generate_directives
>> +            ])
>>          self.assertThat(
>>              os.path.join(target_dir, reverse_file_name),
>>              FileContains(matcher=expected))
>>
>> +    def test_ignores_generate_directives_for_v6_dynamic_ranges(self):
>> +        patch_dns_config_path(self)
>> +        domain = factory.make_string()
>> +        network = IPNetwork('192.168.0.1/22')
>> +        dynamic_network = IPNetwork("%s/64" % factory.make_ipv6_address())
>> +        dns_zone_config = DNSReverseZoneConfig(
>> +            domain, serial=random.randint(1, 100), network=network,
>> +            dynamic_ranges=[
>> +                IPRange(dynamic_network.first, dynamic_network.last)])
>> +        get_generate_directives = self.patch(
>> +            dns_zone_config, 'get_GENERATE_directives')
>> +        dns_zone_config.write_config()
>> +        self.assertThat(get_generate_directives, MockNotCalled())
>> +
>>      def test_reverse_config_file_is_world_readable(self):
>>          patch_dns_config_path(self)
>>          dns_zone_config = DNSReverseZoneConfig(
>> @@ -368,3 +434,270 @@
>>          dns_zone_config.write_config()
>>          filepath = FilePath(dns_zone_config.target_path)
>>          self.assertTrue(filepath.getPermissions().other.read)
>> +
>> +
>> +class TestDNSReverseZoneConfig_GetGenerateDirectives(MAASTestCase):
>> +    """Tests for `DNSReverseZoneConfig.get_GENERATE_directives()`."""
>> +
>> +    def get_expected_generate_directives(self, network, domain):
>> +        ip_parts = network.network.format().split('.')
>> +        relevant_ip_parts = ip_parts[:-2]
>> +
>> +        first_address = IPAddress(network.first).format()
>> +        first_address_parts = first_address.split(".")
>> +
>> +        if network.size < 255:
>> +            last_address = IPAddress(network.last).format()
>> +            iterator_low = int(first_address_parts[-1])
>> +            iterator_high = last_address.split('.')[-1]
>> +        else:
>> +            iterator_low = 0
>> +            iterator_high = 255
>> +
>> +        second_octet_offset = int(first_address_parts[-2])
>> +        expected_generate_directives = []
>> +        directives_needed = network.size / 255
>> +
>> +        if directives_needed == 0:
>> +            directives_needed = 1
>> +        for num in range(directives_needed):
>> +            expected_address_base = "%s-%s" % tuple(relevant_ip_parts)
>> +            expected_address = "%s-%s-$" % (
>> +                expected_address_base, num + second_octet_offset)
>> +            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)
>> +            expected_generate_directives.append(
>> +                (
>> +                    "%s-%s" % (iterator_low, iterator_high),
>> +                    expected_rdns_template,
>> +                    "%s.%s." % (expected_address, domain)
>> +                ))
>> +            relevant_ip_parts.reverse()
>> +        return expected_generate_directives
>
> There's a *lot* of machinery in here to come up with expected output. I can't tell you if it's right by looking at it. The tests are a battle between two implementations, but there's nothing to say that either of them is right.
>
> This is a situation where I think it's worth also having tests based around some static representative input and static, verbatim, expected output. Then use the impl-vs-impl tests to cover more ground.

Agreed. I've added explicit tests for both forward and reverse zones.

>> +
>> +    def test_returns_single_entry_for_slash_24_network(self):
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        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(1))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_returns_single_entry_for_tiny_network(self):
>> +        network = IPNetwork("%s/28" % factory.make_ipv4_address())
>> +        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(1))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_returns_single_entry_for_weird_small_range(self):
>> +        ip_range = IPRange('10.0.0.1', '10.0.0.255')
>> +        domain = factory.make_string()
>> +        directives = DNSReverseZoneConfig.get_GENERATE_directives(
>> +            ip_range, domain)
>> +        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 / 255))
>> +
>> +    def test_returns_two_entries_for_slash_23_network(self):
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>
> make_ipv4_network()

!!!!!

>> +        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))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_rejects_network_larger_than_slash_16(self):
>> +        network = IPNetwork("%s/15" % factory.make_ipv4_address())
>> +        error = self.assertRaises(
>> +            AssertionError, DNSReverseZoneConfig.get_GENERATE_directives,
>> +            network, None)
>> +        self.assertEqual(
>> +            "Cannot generate reverse zone mapping for any network larger "
>> +            "than /16.",
>> +            unicode(error))
>> +
>> +    def test_sorts_output(self):
>> +        network = IPNetwork("10.0.0.1/23")
>> +        domain = factory.make_string()
>> +
>> +        expected_hostname = "10-0-%s-$." + domain + "."
>> +        expected_rdns = "$.%s.0.10.in-addr.arpa."
>> +
>> +        directives = list(DNSReverseZoneConfig.get_GENERATE_directives(
>> +            network, domain))
>> +        self.expectThat(
>> +            directives[0], Equals(
>> +                ("0-255", expected_rdns % "0", expected_hostname % "0")))
>> +        self.expectThat(
>> +            directives[1], Equals(
>> +                ("0-255", expected_rdns % "1", expected_hostname % "1")))
>> +
>> +
>> +class TestDNSForwardZoneConfig_GetGenerateDirectives(MAASTestCase):
>> +    """Tests for `DNSForwardZoneConfig.get_GENERATE_directives()`."""
>> +
>> +    def get_expected_generate_directives(self, network):
>> +        ip_parts = network.network.format().split('.')
>> +        ip_parts[-1] = "$"
>> +        expected_hostname = "%s" % "-".join(ip_parts)
>> +        expected_address = ".".join(ip_parts)
>> +
>> +        first_address = IPAddress(network.first).format()
>> +        first_address_parts = first_address.split(".")
>> +        last_address = IPAddress(network.last).format()
>> +        last_address_parts = last_address.split(".")
>> +
>> +        if network.size < 255:
>
> Same here with the 255 or 256 thing. Everywhere you've got 255 actually.

Yep. Fixed.

>> +            iterator_low = int(first_address_parts[-1])
>> +            if iterator_low == 0:
>> +                iterator_low = 1
>> +            iterator_high = last_address_parts[-1]
>> +        else:
>> +            iterator_low = 0
>> +            iterator_high = 255
>> +
>> +        expected_iterator_values = "%s-%s" % (iterator_low, iterator_high)
>> +
>> +        directives_needed = network.size / 255
>> +        if directives_needed == 0:
>> +            directives_needed = 1
>> +        expected_directives = []
>> +        for num in range(directives_needed):
>> +            ip_parts[-2] = unicode(num + int(ip_parts[-2]))
>> +            expected_address = ".".join(ip_parts)
>> +            expected_hostname = "%s" % "-".join(ip_parts)
>> +            expected_directives.append(
>> +                (expected_iterator_values, expected_hostname, expected_address))
>> +        return expected_directives
>> +
>> +    def test_returns_single_entry_for_slash_24_network(self):
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.expectThat(directives, HasLength(1))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_returns_single_entry_for_tiny_network(self):
>> +        network = IPNetwork("%s/31" % factory.make_ipv4_address())
>> +
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.assertEqual(1, len(expected_directives))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_returns_two_entries_for_slash_23_network(self):
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>> +
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.assertEqual(2, len(expected_directives))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_dtrt_for_larger_networks(self):
>> +        # For every other network size that we're not explicitly
>> +        # testing here,
>> +        # DNSForwardZoneConfig.get_GENERATE_directives() will return
>> +        # one GENERATE directive for every 255 addresses in the network.
>> +        for prefixlen in range(23, 16):
>> +            network = IPNetwork(
>> +                "%s/%s" % (factory.make_ipv4_address(), prefixlen))
>> +            directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +                network)
>> +            self.assertIsEqual(network.size / 255, len(directives))
>> +
>> +    def test_rejects_network_larger_than_slash_16(self):
>> +        network = IPNetwork("%s/15" % factory.make_ipv4_address())
>> +        error = self.assertRaises(
>> +            AssertionError, DNSForwardZoneConfig.get_GENERATE_directives,
>> +            network)
>> +        self.assertEqual(
>> +            "Cannot generate reverse zone mapping for any network larger "
>> +            "than /16.",
>> +            unicode(error))
>> +
>> +    def test_sorts_output(self):
>> +        network = IPNetwork("10.0.0.0/23")
>> +
>> +        expected_hostname = "10-0-%s-$"
>> +        expected_address = "10.0.%s.$"
>> +
>> +        directives = list(DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network))
>> +        self.expectThat(len(directives), Equals(2))
>> +        self.expectThat(
>> +            directives[0], Equals(
>> +                ("0-255", expected_hostname % "0", expected_address % "0")))
>> +        self.expectThat(
>> +            directives[1], Equals(
>> +                ("0-255", expected_hostname % "1", expected_address % "1")))
>> +
>> +
>> +class TestGetHostnameTemplateForNetwork(MAASTestCase):
>> +    """Tests for get_hostname_template_for_network()."""
>> +
>> +    def test_returns_sane_template_for_slash_24(self):
>> +        # For /24 networks, get_hostname_template_for_network() converts
>> +        # the last octet of the network address into a $GENERATE
>> +        # iterator placeholder.
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network))
>> +
>> +    def test_returns_sane_template_for_multiple_subnets(self):
>> +        # For networks larger than /24,
>> +        # get_hostname_template_for_network() converts the last octet of
>> +        # the network address into a $GENERATE iterator placeholder, and
>> +        # adds a "%(next_octet)s" placeholder in the place of the
>> +        # penultimate octet.
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        net_addr_parts[-2] = "%(next_octet)s"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network, subnets=2))
>> +
>> +    def test_returns_sane_template_for_smaller_networks(self):
>> +        # For networks smaller than /24,
>> +        # get_hostname_template_for_network() converts the last octet of
>> +        # the network address into a $GENERATE iterator placeholder, but
>> +        # leaves the rest of the address untouched.
>> +        network = IPNetwork("%s/26" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network))
>>
>> === modified file 'src/provisioningserver/dns/zoneconfig.py'
>> --- src/provisioningserver/dns/zoneconfig.py  2014-08-15 11:10:09 +0000
>> +++ src/provisioningserver/dns/zoneconfig.py  2014-10-29 11:33:01 +0000
>> @@ -23,7 +23,7 @@
>>  from itertools import chain
>>  import math
>>
>> -from netaddr import IPAddress
>> +from netaddr import IPAddress, IPRange, spanning_cidr
>
> Format imports before landing.
>
>>  from netaddr.core import AddrFormatError
>>  from provisioningserver.dns.config import (
>>      compose_config_path,
>> @@ -33,6 +33,16 @@
>>  from provisioningserver.utils.fs import incremental_write
>>
>>
>> +def intersect_iprange(network, iprange):
>> +    # IPSet is notoriously inefficient so we intersect ourselves here.
>
> Docstring here, including this comment?

Whoops.

>> +    if network.last >= iprange.first and network.first <= iprange.last:
>> +        first = max(network.first, iprange.first)
>> +        last = min(network.last, iprange.last)
>> +        return IPRange(first, last)
>> +    else:
>> +        return None
>> +
>> +
>>  def get_fqdn_or_ip_address(target):
>>      """Returns the ip address is target is a valid ip address, otherwise
>>      returns the target with appended '.' if missing."""
>> @@ -52,6 +62,70 @@
>>              yield hostname, ip
>>
>>
>> +def get_details_for_ip_range(ip_range):
>> +    """For a given IPRange, return all subnets and a useable prefix.
>> +    :return: A tuple of:
>> +        All subnets of /24 (or smaller if there is no /24 subnet to be
>> +        found) in `ip_range`.
>> +        A prefix made from the first two octets in the range.
>> +    """
>> +    # Calculate a spanning network for the range above. There are
>> +    # 256 /24 networks in a /16, so that's the most /24s we're going
>> +    # to have to deal with; this matters later on when we iterate
>> +    # through the /24s within this network.
>> +    cidr = spanning_cidr(ip_range)
>> +    subnets = []
>> +    for subnet_size in range (24, 33):
>> +        # Find the first working subnet, starting with 24 and
>> +        # working down to 32. This allows us to account for really
>> +        # small ranges.
>> +        subnets = list(cidr.subnet(subnet_size))
>> +        if len(subnets) > 0:
>> +            break
>
> You can do this in a single jump:
>
>     # Split cidr into subnets not bigger than a /24.
>     subnets = cidr.subnet(max(24, cidr.prefixlen))

Good point!

>> +
>> +    # Split the spanning network into /24 subnets, then see if they fall
>> +    # entirely within the original network range, partially, or not at
>> +    # all.
>> +    intersecting_subnets = []
>> +    for subnet in subnets:
>> +        intersect = intersect_iprange(subnet, ip_range)
>> +        if intersect is None:
>> +            # The subnet does not fall within the original network.
>> +            pass
>> +        else:
>> +            # The subnet falls partially within the original network, so print
>> +            # out a $GENERATE expression for a subset of the /24.
>> +            intersecting_subnets.append(intersect)
>> +
>> +    # The first two octets of the network range formatted in the
>> +    # usual dotted-quad style. We can precalculate this because
>> +    # we're only ever dealing with /16 networks and smaller.
>> +    prefix = "%d.%d" % (
>> +        (cidr.value & 0xff000000) >> 24,
>> +        (cidr.value & 0x00ff0000) >> 16,
>> +    )
>> +    # Similarly, we can calculate what the reverse DNS suffix is going
>> +    # to look like.
>> +    rdns_suffix = "%d.%d.in-addr.arpa." % (
>> +        (cidr.value & 0x00ff0000) >> 16,
>> +        (cidr.value & 0xff000000) >> 24,
>> +        )
>> +    return intersecting_subnets, prefix, rdns_suffix
>> +
>> +
>> +def get_hostname_template_for_network(network, subnets=1):
>> +    """For a given network, return a hostname template.
>> +
>> +    The hostname template is made to be used in $GENERATE directives.
>> +    """
>> +    net_addr = IPAddress(network.first).format()
>> +    hostname_parts = net_addr.split(".")
>> +    hostname_parts[-1] = '$'
>> +    if subnets > 1:
>> +        hostname_parts[-2] = "%(next_octet)s"
>> +    return "-".join(hostname_parts)
>> +
>> +
>>  class DNSZoneConfigBase:
>>      """Base class for zone writers."""
>>
>> @@ -117,7 +191,8 @@
>>          """
>>          self._dns_ip = kwargs.pop('dns_ip', None)
>>          self._mapping = kwargs.pop('mapping', {})
>> -        self._network = None
>> +        self._network = kwargs.pop('network', None)
>> +        self._dynamic_ranges = kwargs.pop('dynamic_ranges', [])
>>          self._srv_mapping = kwargs.pop('srv_mapping', [])
>>          super(DNSForwardZoneConfig, self).__init__(
>>              domain, zone_name=domain, **kwargs)
>> @@ -187,8 +262,40 @@
>>                  target)
>>              yield (record.service, item)
>>
>> +    @classmethod
>> +    def get_GENERATE_directives(cls, dynamic_range):
>> +        """Return the GENERATE directives for the forward zone of a network.
>> +        """
>> +        assert dynamic_range.size <= 256**2, (
>> +            "Cannot generate reverse zone mapping for any network larger than "
>> +            "/16.")
>> +
>> +        generate_directives = set()
>> +        subnets, prefix, _ = get_details_for_ip_range(dynamic_range)
>> +        for subnet in subnets:
>> +            iterator = "%d-%d" %  (
>> +                (subnet.first & 0x000000ff),
>> +                (subnet.last & 0x000000ff))
>> +            hostname = get_hostname_template_for_network(
>
> The extraction of get_hostname_template_for_network() function seems to make this more complicated because of the next_octet stuff. If it's only used in this one place, try moving the implementation back in here and simplifying.

Yep. Tis dead. It was used one other place, too, but it's smaller as re-written.

>> +                subnet, len(subnets)) % {
>> +                    'next_octet': (subnet.first & 0x0000ff00) >> 8}
>> +            ip_address = "%s.%d.$" % (
>> +                prefix,
>> +                (subnet.first & 0x0000ff00) >> 8)
>> +            generate_directives.add((iterator, hostname, ip_address))
>> +
>> +        return sorted(generate_directives)
>> +
>>      def write_config(self):
>>          """Write the zone file."""
>> +        generate_directives = []
>> +        for dynamic_range in self._dynamic_ranges:
>> +            if dynamic_range.version == 4:
>> +                # Only create GENERATE directives for IPv4 ranges.
>> +                generate_directives.extend(
>> +                    self.get_GENERATE_directives(dynamic_range))
>> +            else:
>> +                pass
>
> You could do this in a single expression:
>
>         generate_directives = list(chain.from_iterable(
>             self.get_GENERATE_directives(dynamic_range)
>             for dynamic_range in self._dynamic_ranges
>             if dynamic_range.version == 4
>         ))

Good point.

>>          self.write_zone_file(
>>              self.target_path, self.make_parameters(),
>>              {
>> @@ -200,6 +307,9 @@
>>                      'AAAA': self.get_AAAA_mapping(
>>                          self._mapping, self.domain, self._dns_ip),
>>                  },
>> +                'generate_directives': {
>> +                    'A': generate_directives,
>> +                }
>>              })
>>
>>
>> @@ -225,6 +335,7 @@
>>          """
>>          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)
>>          super(DNSReverseZoneConfig, self).__init__(
>>              domain, zone_name=zone_name, **kwargs)
>> @@ -279,8 +390,40 @@
>>              if IPAddress(ip) in network
>>          )
>>
>> +    @classmethod
>> +    def get_GENERATE_directives(cls, dynamic_range, domain):
>> +        """Return the GENERATE directives for the reverse zone of a network."""
>> +        assert dynamic_range.size <= 256**2, (
>> +            "Cannot generate reverse zone mapping for any network larger than "
>> +            "/16.")
>> +
>> +        generate_directives = set()
>> +        subnets, _, rdns_suffix = get_details_for_ip_range(dynamic_range)
>> +        for subnet in subnets:
>> +            iterator = "%d-%d" %  (
>> +                (subnet.first & 0x000000ff),
>> +                (subnet.last & 0x000000ff))
>> +            hostname = get_hostname_template_for_network(
>> +                subnet, len(subnets)) % {
>> +                    'next_octet': (subnet.first & 0x0000ff00) >> 8}
>> +            rdns = "$.%d.%s" % (
>> +                (subnet.first & 0x0000ff00) >> 8,
>> +                rdns_suffix)
>> +            generate_directives.add(
>> +                (iterator, rdns, "%s.%s." % (hostname, domain)))
>> +
>> +        return sorted(generate_directives)
>
> Btw, is this just so it's stable, and thus easier to test? The sort order you're going to get out of this isn't particularly useful, i.e. dominated by strings like "45-87". Perhaps it would be better to change generate_directives to a list and return it as-is, in the order they're generated? Or yield them and let the caller put them into a list.

Hmm, I see you point. I want to keep the ordering not so much for
testing as for the sake of the output in the DNS file. I've changed
this to sort by hostname, which is a bit more predictable.

>> +
>>      def write_config(self):
>>          """Write the zone file."""
>> +        generate_directives = []
>> +        for dynamic_range in self._dynamic_ranges:
>> +            if dynamic_range.version == 4:
>> +                # Only create GENERATE directives for IPv4 ranges.
>> +                generate_directives.extend(
>> +                    self.get_GENERATE_directives(dynamic_range, self.domain))
>> +            else:
>> +                pass
>
> Single expression again, if you want.

Yep, done.

Revision history for this message

Raphaël Badin (rvb) wrote on 2014-10-29:

I added a bunch of comments on top of what Gavin already said.

I've tested that this works as expected on my NUCs and testing went great (but I didn't test the corner cases, just that this addresses the main issue); The main thing I suggested was the removal of the 'dynamic' prefix in the GENERATE records to preserve exactly the hostnames we had before in order to avoid upgrade nightmares; thanks for addressing this quickly gmb.

fwiw, I really wonder if this isn't a bit overly complicated. I understand this was done this way to limit the number of GENERATE records as much as possible but the end result is code that will be a bit painful to maintain.

Same as Gavin (in his comment that starts with "There's a *lot* of machinery in here to come up with expected output."), I think that the testing would benefit from using the expected string output when possible; additionally, this will help us be confident that the corner cases are covered.

review: Approve

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-29:

On 29 October 2014 16:47, Raphaël Badin <email address hidden> wrote:
> All I'm saying is that we need the proper limitations in place at the form level. What do you think?

I think I'm going to do that in a separate branch, and I'm already
working on it :)

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-29:

Download full text (35.9 KiB)

On 29 October 2014 16:47, Raphaël Badin <email address hidden> wrote:
> Review: Approve
>
> I added a bunch of comments on top of what Gavin already said.
>
> I've tested that this works as expected on my NUCs and testing went great (but I didn't test the corner cases, just that this addresses the main issue); The main thing I suggested was the removal of the 'dynamic' prefix in the GENERATE records to preserve exactly the hostnames we had before in order to avoid upgrade nightmares; thanks for addressing this quickly gmb.
>
> fwiw, I really wonder if this isn't a bit overly complicated. I understand this was done this way to limit the number of GENERATE records as much as possible but the end result is code that will be a bit painful to maintain.

I agree. I would have been happy to land the older version too, but
now that we're here…

> Same as Gavin (in his comment that starts with "There's a *lot* of machinery in here to come up with expected output."), I think that the testing would benefit from using the expected string output when possible; additionally, this will help us be confident that the corner cases are covered.

ISWYM. I think I'm channelling Jeroen's "Don't let it look like you're
relying on specific values" approach a bit too much :). I've fixed
that a bit by adding explicit tests which actually cover an odd IP
range. I don't want to block landing this for 1.7 on the tests being
readable, though. I'll file a tech-debt bug to re-work them.

On 29 October 2014 16:47, Raphaël Badin <raphael.badin@canonical.com> wrote:
> Review: Approve
>
> I added a bunch of comments on top of what Gavin already said.
>
> I've tested that this works as expected on my NUCs and testing went great (but I didn't test the corner cases, just that this addresses the main issue);  The main thing I suggested was the removal of the 'dynamic' prefix in the GENERATE records to preserve exactly the hostnames we had before in order to avoid upgrade nightmares; thanks for addressing this quickly gmb.
>
> fwiw, I really wonder if this isn't a bit overly complicated.  I understand this was done this way to limit the number of GENERATE records as much as possible but the end result is code that will be a bit painful to maintain.

I agree. I would have been happy to land the older version too, but
now that we're here…

>
>> === modified file 'etc/maas/templates/dns/zone.template'
>> --- etc/maas/templates/dns/zone.template      2012-10-04 15:59:56 +0000
>> +++ etc/maas/templates/dns/zone.template      2014-10-29 11:33:01 +0000
>> @@ -12,6 +12,12 @@
>>                )
>>
>>      IN  NS  {{domain}}.
>> +{{for type, directive in generate_directives.items()}}
>> +{{for iterator_values, rdns, hostname in directive}}
>> +$GENERATE {{iterator_values}} {{rdns}} IN {{type}} {{hostname}}
>> +{{endfor}}
>> +{{endfor}}
>> +
>>  {{for type, mapping in mappings.items()}}
>>  {{for item_from, item_to in mapping}}
>>  {{item_from}} IN {{type}} {{item_to}}
>>
>> === modified file 'src/maasserver/dns/tests/test_zonegenerator.py'
>> --- src/maasserver/dns/tests/test_zonegenerator.py    2014-09-15 14:28:28 +0000
>> +++ src/maasserver/dns/tests/test_zonegenerator.py    2014-10-29 11:33:01 +0000
>> @@ -393,14 +393,28 @@
>>          [interface] = nodegroup.get_managed_interfaces()
>>          networks_dict = ZoneGenerator._get_networks()
>>          retrieved_interface = networks_dict[nodegroup]
>> -        self.assertEqual([interface.network], retrieved_interface)
>> +        self.assertEqual(
>> +            [
>> +                (
>> +                    interface.network,
>> +                    (interface.ip_range_low, interface.ip_range_high)
>> +                )
>> +            ],
>> +            retrieved_interface)
>>
>>      def test_get_networks_returns_multiple_networks(self):
>>          nodegroups = [self.make_node_group() for _ in range(3)]
>>          networks_dict = ZoneGenerator._get_networks()
>>          for nodegroup in nodegroups:
>>              [interface] = nodegroup.get_managed_interfaces()
>> -            self.assertEqual([interface.network], networks_dict[nodegroup])
>> +            self.assertEqual(
>> +                [
>> +                    (
>> +                        interface.network,
>> +                        (interface.ip_range_low, interface.ip_range_high),
>> +                    ),
>> +                ],
>> +                networks_dict[nodegroup])
>>
>>      def test_get_networks_returns_managed_networks(self):
>>          nodegroups = [
>> @@ -415,7 +429,10 @@
>>          self.assertEqual(
>>              {
>>                  nodegroup: [
>> -                    interface.network
>> +                    (
>> +                        interface.network,
>> +                        (interface.ip_range_low, interface.ip_range_high),
>> +                    )
>>                      for interface in nodegroup.get_managed_interfaces()
>>                      ]
>>                  for nodegroup in nodegroups
>>
>> === modified file 'src/maasserver/dns/zonegenerator.py'
>> --- src/maasserver/dns/zonegenerator.py       2014-08-28 12:00:15 +0000
>> +++ src/maasserver/dns/zonegenerator.py       2014-10-29 11:33:01 +0000
>> @@ -29,7 +29,10 @@
>>  from maasserver.models.config import Config
>>  from maasserver.models.nodegroup import NodeGroup
>>  from maasserver.server_address import get_maas_facing_server_address
>> -from netaddr import IPAddress
>> +from netaddr import (
>> +    IPAddress,
>> +    IPRange,
>> +    )
>>  from provisioningserver.dns.zoneconfig import (
>>      DNSForwardZoneConfig,
>>      DNSReverseZoneConfig,
>> @@ -180,9 +183,15 @@
>>
>>      @staticmethod
>>      def _get_networks():
>> -        """Return a lazily evaluated nodegroup:network dict."""
>> -        return lazydict(lambda ng: [
>> -            interface.network for interface in ng.get_managed_interfaces()])
>> +        """Return a lazily evaluated nodegroup:network_details dict.
>> +
>> +        network_details takes the form of a tuple of (network,
>> +        (network.ip_range_low, network.ip_range_high)).
>> +        """
>> +        return lazydict(lambda ng: [(
>> +            interface.network,
>> +            (interface.ip_range_low, interface.ip_range_high))
>> +            for interface in ng.get_managed_interfaces()])
>>
>>      @staticmethod
>>      def _get_srv_mappings():
>> @@ -209,6 +218,12 @@
>>          forward_nodegroups = sorted(nodegroups, key=get_domain)
>>          for domain, nodegroups in groupby(forward_nodegroups, get_domain):
>>              nodegroups = list(nodegroups)
>> +            dynamic_ranges = []
>> +            for nodegroup in nodegroups:
>> +                dynamic_ranges.extend(
>> +                    IPRange(interface.ip_range_low, interface.ip_range_high)
>> +                    for interface in nodegroup.get_managed_interfaces()
>> +                )
>>              # A forward zone encompassing all nodes in the same domain.
>>              yield DNSForwardZoneConfig(
>>                  domain, serial=serial, dns_ip=dns_ip,
>> @@ -217,7 +232,8 @@
>>                      for nodegroup in nodegroups
>>                      for hostname, ip in mappings[nodegroup].items()
>>                      },
>> -                srv_mapping=set(srv_mappings)
>> +                srv_mapping=set(srv_mappings),
>> +                dynamic_ranges=dynamic_ranges,
>>              )
>>
>>      @staticmethod
>> @@ -226,11 +242,11 @@
>>          get_domain = lambda nodegroup: nodegroup.name
>>          reverse_nodegroups = sorted(nodegroups, key=networks.get)
>>          for nodegroup in reverse_nodegroups:
>> -            for network in networks[nodegroup]:
>> +            for network, dynamic_range in networks[nodegroup]:
>>                  mapping = mappings[nodegroup]
>>                  yield DNSReverseZoneConfig(
>>                      get_domain(nodegroup), serial=serial, mapping=mapping,
>> -                    network=network
>> +                    network=network, dynamic_ranges=[IPRange(*dynamic_range)]
>>                  )
>>
>>      def __iter__(self):
>>
>> === modified file 'src/provisioningserver/dns/tests/test_zoneconfig.py'
>> --- src/provisioningserver/dns/tests/test_zoneconfig.py       2014-09-23 21:43:27 +0000
>> +++ src/provisioningserver/dns/tests/test_zoneconfig.py       2014-10-29 11:33:01 +0000
>> @@ -26,7 +26,9 @@
>>  from netaddr import (
>>      IPAddress,
>>      IPNetwork,
>> +    IPRange,
>>      )
>> +from maastesting.matchers import MockNotCalled
>>  from provisioningserver.dns.config import (
>>      get_dns_config_dir,
>>      SRVRecord,
>> @@ -35,15 +37,18 @@
>>  from provisioningserver.dns.zoneconfig import (
>>      DNSForwardZoneConfig,
>>      DNSReverseZoneConfig,
>> +    get_hostname_template_for_network,
>>      )
>>  from testtools.matchers import (
>>      Contains,
>>      ContainsAll,
>> +    Equals,
>>      FileContains,
>>      IsInstance,
>>      MatchesAll,
>>      MatchesStructure,
>>      Not,
>> +    HasLength,
>>      )
>>  from twisted.python.filepath import FilePath
>>
>> @@ -187,10 +192,13 @@
>>              ipv4_hostname: [ipv4_ip],
>>              ipv6_hostname: [ipv6_ip],
>>          }
>> +        expected_generate_directives = (
>> +            DNSForwardZoneConfig.get_GENERATE_directives(network))
>>          srv = self.make_srv_record()
>>          dns_zone_config = DNSForwardZoneConfig(
>>              domain, serial=random.randint(1, 100),
>> -            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv])
>> +            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv],
>> +            dynamic_ranges=[IPRange(network.first, network.last)])
>>          dns_zone_config.write_config()
>>          self.assertThat(
>>              os.path.join(target_dir, 'zone.%s' % domain),
>> @@ -201,6 +209,12 @@
>>                              srv.service, self.get_srv_item_output(srv)),
>>                          '%s IN A %s' % (ipv4_hostname, ipv4_ip),
>>                          '%s IN AAAA %s' % (ipv6_hostname, ipv6_ip),
>> +                    ] +
>> +                    [
>> +                        '$GENERATE %s %s IN A %s' % (
>> +                            iterator_values, reverse_dns, hostname)
>> +                        for iterator_values, reverse_dns, hostname in
>> +                        expected_generate_directives
>>                      ]
>>                  )
>>              )
>> @@ -222,6 +236,31 @@
>>                          '%s. IN A %s' % (dns_zone_config.domain, dns_ip),
>>                      ])))
>>
>> +    def test_ignores_generate_directives_for_v6_dynamic_ranges(self):
>> +        patch_dns_config_path(self)
>> +        domain = factory.make_string()
>> +        network = factory.make_ipv4_network()
>> +        dns_ip = factory.pick_ip_in_network(network)
>> +        ipv4_hostname = factory.make_name('host')
>> +        ipv4_ip = factory.pick_ip_in_network(network)
>> +        ipv6_hostname = factory.make_name('host')
>> +        ipv6_ip = factory.make_ipv6_address()
>> +        ipv6_network = IPNetwork("%s/64" % ipv6_ip)
>> +        dynamic_range = IPRange(ipv6_network.first, ipv6_network.last)
>> +        mapping = {
>> +            ipv4_hostname: [ipv4_ip],
>> +            ipv6_hostname: [ipv6_ip],
>> +        }
>> +        srv = self.make_srv_record()
>> +        dns_zone_config = DNSForwardZoneConfig(
>> +            domain, serial=random.randint(1, 100),
>> +            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv],
>> +            dynamic_ranges=[dynamic_range])
>> +        get_generate_directives = self.patch(
>> +            dns_zone_config, 'get_GENERATE_directives')
>> +        dns_zone_config.write_config()
>> +        self.assertThat(get_generate_directives, MockNotCalled())
>> +
>>      def test_config_file_is_world_readable(self):
>>          patch_dns_config_path(self)
>>          dns_zone_config = DNSForwardZoneConfig(
>> @@ -350,16 +389,43 @@
>>          target_dir = patch_dns_config_path(self)
>>          domain = factory.make_string()
>>          network = IPNetwork('192.168.0.1/22')
>> +        dynamic_network = IPNetwork('192.168.0.1/28')
>>          dns_zone_config = DNSReverseZoneConfig(
>> -            domain, serial=random.randint(1, 100), network=network)
>> +            domain, serial=random.randint(1, 100), network=network,
>> +            dynamic_ranges=[
>> +                IPRange(dynamic_network.first, dynamic_network.last)])
>>          dns_zone_config.write_config()
>>          reverse_file_name = 'zone.168.192.in-addr.arpa'
>> -        expected = Contains(
>> -            'IN  NS  %s' % domain)
>> +        expected_generate_directives = dns_zone_config.get_GENERATE_directives(
>> +            dynamic_network, domain)
>> +        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_generate_directives
>> +            ])
>>          self.assertThat(
>>              os.path.join(target_dir, reverse_file_name),
>>              FileContains(matcher=expected))
>>
>> +    def test_ignores_generate_directives_for_v6_dynamic_ranges(self):
>> +        patch_dns_config_path(self)
>> +        domain = factory.make_string()
>> +        network = IPNetwork('192.168.0.1/22')
>> +        dynamic_network = IPNetwork("%s/64" % factory.make_ipv6_address())
>> +        dns_zone_config = DNSReverseZoneConfig(
>> +            domain, serial=random.randint(1, 100), network=network,
>> +            dynamic_ranges=[
>> +                IPRange(dynamic_network.first, dynamic_network.last)])
>> +        get_generate_directives = self.patch(
>> +            dns_zone_config, 'get_GENERATE_directives')
>> +        dns_zone_config.write_config()
>> +        self.assertThat(get_generate_directives, MockNotCalled())
>> +
>>      def test_reverse_config_file_is_world_readable(self):
>>          patch_dns_config_path(self)
>>          dns_zone_config = DNSReverseZoneConfig(
>> @@ -368,3 +434,270 @@
>>          dns_zone_config.write_config()
>>          filepath = FilePath(dns_zone_config.target_path)
>>          self.assertTrue(filepath.getPermissions().other.read)
>> +
>> +
>> +class TestDNSReverseZoneConfig_GetGenerateDirectives(MAASTestCase):
>> +    """Tests for `DNSReverseZoneConfig.get_GENERATE_directives()`."""
>> +
>> +    def get_expected_generate_directives(self, network, domain):
>> +        ip_parts = network.network.format().split('.')
>> +        relevant_ip_parts = ip_parts[:-2]
>> +
>> +        first_address = IPAddress(network.first).format()
>> +        first_address_parts = first_address.split(".")
>> +
>> +        if network.size < 255:
>> +            last_address = IPAddress(network.last).format()
>> +            iterator_low = int(first_address_parts[-1])
>> +            iterator_high = last_address.split('.')[-1]
>> +        else:
>> +            iterator_low = 0
>> +            iterator_high = 255
>> +
>> +        second_octet_offset = int(first_address_parts[-2])
>> +        expected_generate_directives = []
>> +        directives_needed = network.size / 255
>> +
>> +        if directives_needed == 0:
>> +            directives_needed = 1
>> +        for num in range(directives_needed):
>> +            expected_address_base = "%s-%s" % tuple(relevant_ip_parts)
>> +            expected_address = "%s-%s-$" % (
>> +                expected_address_base, num + second_octet_offset)
>> +            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)
>> +            expected_generate_directives.append(
>> +                (
>> +                    "%s-%s" % (iterator_low, iterator_high),
>> +                    expected_rdns_template,
>> +                    "%s.%s." % (expected_address, domain)
>> +                ))
>> +            relevant_ip_parts.reverse()
>> +        return expected_generate_directives
>> +
>> +    def test_returns_single_entry_for_slash_24_network(self):
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        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(1))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_returns_single_entry_for_tiny_network(self):
>> +        network = IPNetwork("%s/28" % factory.make_ipv4_address())
>> +        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(1))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_returns_single_entry_for_weird_small_range(self):
>> +        ip_range = IPRange('10.0.0.1', '10.0.0.255')
>> +        domain = factory.make_string()
>> +        directives = DNSReverseZoneConfig.get_GENERATE_directives(
>> +            ip_range, domain)
>> +        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 / 255))
>> +
>> +    def test_returns_two_entries_for_slash_23_network(self):
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>> +        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))
>> +        self.assertItemsEqual(expected_generate_directives, directives)
>> +
>> +    def test_rejects_network_larger_than_slash_16(self):
>> +        network = IPNetwork("%s/15" % factory.make_ipv4_address())
>> +        error = self.assertRaises(
>> +            AssertionError, DNSReverseZoneConfig.get_GENERATE_directives,
>> +            network, None)
>> +        self.assertEqual(
>> +            "Cannot generate reverse zone mapping for any network larger "
>> +            "than /16.",
>> +            unicode(error))
>> +
>> +    def test_sorts_output(self):
>> +        network = IPNetwork("10.0.0.1/23")
>> +        domain = factory.make_string()
>> +
>> +        expected_hostname = "10-0-%s-$." + domain + "."
>> +        expected_rdns = "$.%s.0.10.in-addr.arpa."
>> +
>> +        directives = list(DNSReverseZoneConfig.get_GENERATE_directives(
>> +            network, domain))
>> +        self.expectThat(
>> +            directives[0], Equals(
>> +                ("0-255", expected_rdns % "0", expected_hostname % "0")))
>> +        self.expectThat(
>> +            directives[1], Equals(
>> +                ("0-255", expected_rdns % "1", expected_hostname % "1")))
>> +
>> +
>> +class TestDNSForwardZoneConfig_GetGenerateDirectives(MAASTestCase):
>> +    """Tests for `DNSForwardZoneConfig.get_GENERATE_directives()`."""
>> +
>> +    def get_expected_generate_directives(self, network):
>> +        ip_parts = network.network.format().split('.')
>> +        ip_parts[-1] = "$"
>> +        expected_hostname = "%s" % "-".join(ip_parts)
>> +        expected_address = ".".join(ip_parts)
>> +
>> +        first_address = IPAddress(network.first).format()
>> +        first_address_parts = first_address.split(".")
>> +        last_address = IPAddress(network.last).format()
>> +        last_address_parts = last_address.split(".")
>> +
>> +        if network.size < 255:
>> +            iterator_low = int(first_address_parts[-1])
>> +            if iterator_low == 0:
>> +                iterator_low = 1
>> +            iterator_high = last_address_parts[-1]
>> +        else:
>> +            iterator_low = 0
>> +            iterator_high = 255
>> +
>> +        expected_iterator_values = "%s-%s" % (iterator_low, iterator_high)
>> +
>> +        directives_needed = network.size / 255
>> +        if directives_needed == 0:
>> +            directives_needed = 1
>> +        expected_directives = []
>> +        for num in range(directives_needed):
>> +            ip_parts[-2] = unicode(num + int(ip_parts[-2]))
>> +            expected_address = ".".join(ip_parts)
>> +            expected_hostname = "%s" % "-".join(ip_parts)
>> +            expected_directives.append(
>> +                (expected_iterator_values, expected_hostname, expected_address))
>> +        return expected_directives
>> +
>> +    def test_returns_single_entry_for_slash_24_network(self):
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.expectThat(directives, HasLength(1))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_returns_single_entry_for_tiny_network(self):
>> +        network = IPNetwork("%s/31" % factory.make_ipv4_address())
>> +
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.assertEqual(1, len(expected_directives))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_returns_two_entries_for_slash_23_network(self):
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>> +
>> +        expected_directives = self.get_expected_generate_directives(network)
>> +        directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network)
>> +        self.assertEqual(2, len(expected_directives))
>> +        self.assertItemsEqual(expected_directives, directives)
>> +
>> +    def test_dtrt_for_larger_networks(self):
>> +        # For every other network size that we're not explicitly
>> +        # testing here,
>> +        # DNSForwardZoneConfig.get_GENERATE_directives() will return
>> +        # one GENERATE directive for every 255 addresses in the network.
>> +        for prefixlen in range(23, 16):
>> +            network = IPNetwork(
>> +                "%s/%s" % (factory.make_ipv4_address(), prefixlen))
>> +            directives = DNSForwardZoneConfig.get_GENERATE_directives(
>> +                network)
>> +            self.assertIsEqual(network.size / 255, len(directives))
>> +
>> +    def test_rejects_network_larger_than_slash_16(self):
>> +        network = IPNetwork("%s/15" % factory.make_ipv4_address())
>> +        error = self.assertRaises(
>> +            AssertionError, DNSForwardZoneConfig.get_GENERATE_directives,
>> +            network)
>> +        self.assertEqual(
>> +            "Cannot generate reverse zone mapping for any network larger "
>> +            "than /16.",
>> +            unicode(error))
>> +
>> +    def test_sorts_output(self):
>> +        network = IPNetwork("10.0.0.0/23")
>> +
>> +        expected_hostname = "10-0-%s-$"
>> +        expected_address = "10.0.%s.$"
>> +
>> +        directives = list(DNSForwardZoneConfig.get_GENERATE_directives(
>> +            network))
>> +        self.expectThat(len(directives), Equals(2))
>> +        self.expectThat(
>> +            directives[0], Equals(
>> +                ("0-255", expected_hostname % "0", expected_address % "0")))
>> +        self.expectThat(
>> +            directives[1], Equals(
>> +                ("0-255", expected_hostname % "1", expected_address % "1")))
>> +
>> +
>> +class TestGetHostnameTemplateForNetwork(MAASTestCase):
>> +    """Tests for get_hostname_template_for_network()."""
>> +
>> +    def test_returns_sane_template_for_slash_24(self):
>> +        # For /24 networks, get_hostname_template_for_network() converts
>> +        # the last octet of the network address into a $GENERATE
>> +        # iterator placeholder.
>> +        network = IPNetwork("%s/24" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network))
>> +
>> +    def test_returns_sane_template_for_multiple_subnets(self):
>> +        # For networks larger than /24,
>> +        # get_hostname_template_for_network() converts the last octet of
>> +        # the network address into a $GENERATE iterator placeholder, and
>> +        # adds a "%(next_octet)s" placeholder in the place of the
>> +        # penultimate octet.
>> +        network = IPNetwork("%s/23" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        net_addr_parts[-2] = "%(next_octet)s"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network, subnets=2))
>> +
>> +    def test_returns_sane_template_for_smaller_networks(self):
>> +        # For networks smaller than /24,
>> +        # get_hostname_template_for_network() converts the last octet of
>> +        # the network address into a $GENERATE iterator placeholder, but
>> +        # leaves the rest of the address untouched.
>> +        network = IPNetwork("%s/26" % factory.make_ipv4_address())
>> +        net_addr = network.network.format()
>> +        net_addr_parts = net_addr.split(".")
>> +        net_addr_parts[-1] = "$"
>> +        self.assertEqual(
>> +            "-".join(net_addr_parts),
>> +            get_hostname_template_for_network(network))
>>
>> === modified file 'src/provisioningserver/dns/zoneconfig.py'
>> --- src/provisioningserver/dns/zoneconfig.py  2014-08-15 11:10:09 +0000
>> +++ src/provisioningserver/dns/zoneconfig.py  2014-10-29 11:33:01 +0000
>> @@ -23,7 +23,7 @@
>>  from itertools import chain
>>  import math
>>
>> -from netaddr import IPAddress
>> +from netaddr import IPAddress, IPRange, spanning_cidr
>>  from netaddr.core import AddrFormatError
>>  from provisioningserver.dns.config import (
>>      compose_config_path,
>> @@ -33,6 +33,16 @@
>>  from provisioningserver.utils.fs import incremental_write
>>
>>
>> +def intersect_iprange(network, iprange):
>
> I don't see any unit tests for this.  Might be work adding some.
>
>> +    # IPSet is notoriously inefficient so we intersect ourselves here.
>> +    if network.last >= iprange.first and network.first <= iprange.last:
>> +        first = max(network.first, iprange.first)
>> +        last = min(network.last, iprange.last)
>> +        return IPRange(first, last)
>> +    else:
>> +        return None
>> +
>> +
>>  def get_fqdn_or_ip_address(target):
>>      """Returns the ip address is target is a valid ip address, otherwise
>>      returns the target with appended '.' if missing."""
>> @@ -52,6 +62,70 @@
>>              yield hostname, ip
>>
>>
>> +def get_details_for_ip_range(ip_range):
>> +    """For a given IPRange, return all subnets and a useable prefix.
>> +    :return: A tuple of:
>> +        All subnets of /24 (or smaller if there is no /24 subnet to be
>> +        found) in `ip_range`.
>> +        A prefix made from the first two octets in the range.
>
> I see this also returns the reverse DNS suffix.

Ah, yes. Fixed the docstring.

>> +    """
>> +    # Calculate a spanning network for the range above. There are
>> +    # 256 /24 networks in a /16, so that's the most /24s we're going
>> +    # to have to deal with; this matters later on when we iterate
>> +    # through the /24s within this network.
>> +    cidr = spanning_cidr(ip_range)
>> +    subnets = []
>> +    for subnet_size in range (24, 33):
>> +        # Find the first working subnet, starting with 24 and
>> +        # working down to 32. This allows us to account for really
>> +        # small ranges.
>> +        subnets = list(cidr.subnet(subnet_size))
>> +        if len(subnets) > 0:
>> +            break
>> +
>> +    # Split the spanning network into /24 subnets, then see if they fall
>> +    # entirely within the original network range, partially, or not at
>> +    # all.
>> +    intersecting_subnets = []
>> +    for subnet in subnets:
>> +        intersect = intersect_iprange(subnet, ip_range)
>> +        if intersect is None:
>> +            # The subnet does not fall within the original network.
>> +            pass
>> +        else:
>> +            # The subnet falls partially within the original network, so print
>> +            # out a $GENERATE expression for a subset of the /24.
>> +            intersecting_subnets.append(intersect)
>
> You can condense this into:
> intersecting_subnets = filter(lambda subnet: intersect_iprange(subnet, ip_range), subnets)

MORE FOLDING OF CODE. I can scarce conceal my excitement.

Actually, I think you meant map() rather than filter() here, but
neither of those works, because you end can end up with Nones in
intersecting_subnets. I've left this as-is.

>> +
>> +    # The first two octets of the network range formatted in the
>> +    # usual dotted-quad style. We can precalculate this because
>> +    # we're only ever dealing with /16 networks and smaller.
>> +    prefix = "%d.%d" % (
>> +        (cidr.value & 0xff000000) >> 24,
>> +        (cidr.value & 0x00ff0000) >> 16,
>> +    )
>> +    # Similarly, we can calculate what the reverse DNS suffix is going
>> +    # to look like.
>> +    rdns_suffix = "%d.%d.in-addr.arpa." % (
>> +        (cidr.value & 0x00ff0000) >> 16,
>> +        (cidr.value & 0xff000000) >> 24,
>> +        )
>
> Maybe it's me, but you could make this a bit easier on the eyes by assigning (cidr.value & 0x00ff0000) >> 16 and (cidr.value & 0xff000000) >> 24 and using them to build both prefix and rdns_perfix.

Fair enough. Done.

>> +    return intersecting_subnets, prefix, rdns_suffix
>> +
>> +
>> +def get_hostname_template_for_network(network, subnets=1):
>> +    """For a given network, return a hostname template.
>
> The docstring doesn't explain a lot compared to the function name itself.  Maybe the fact that this is designed to produce 'GENERATE' records should be accounted for in the name somehow.

This function is DEAD now.

>> +
>> +    The hostname template is made to be used in $GENERATE directives.
>> +    """
>> +    net_addr = IPAddress(network.first).format()
>> +    hostname_parts = net_addr.split(".")
>> +    hostname_parts[-1] = '$'
>> +    if subnets > 1:
>> +        hostname_parts[-2] = "%(next_octet)s"
>> +    return "-".join(hostname_parts)
>> +
>> +
>>  class DNSZoneConfigBase:
>>      """Base class for zone writers."""
>>
>> @@ -117,7 +191,8 @@
>>          """
>>          self._dns_ip = kwargs.pop('dns_ip', None)
>>          self._mapping = kwargs.pop('mapping', {})
>> -        self._network = None
>> +        self._network = kwargs.pop('network', None)
>> +        self._dynamic_ranges = kwargs.pop('dynamic_ranges', [])
>>          self._srv_mapping = kwargs.pop('srv_mapping', [])
>>          super(DNSForwardZoneConfig, self).__init__(
>>              domain, zone_name=domain, **kwargs)
>> @@ -187,8 +262,40 @@
>>                  target)
>>              yield (record.service, item)
>>
>> +    @classmethod
>> +    def get_GENERATE_directives(cls, dynamic_range):
>> +        """Return the GENERATE directives for the forward zone of a network.
>> +        """
>> +        assert dynamic_range.size <= 256**2, (
>> +            "Cannot generate reverse zone mapping for any network larger than "
>> +            "/16.")
>
> I'm a bit concerned by having this checked only by an assertion in here (I understand most of the machinery here relies on this):
> - for ipv4 networks, we should prevent the user from entering huge networks at the form level.
> - for ipv6 networks, this restriction doesn't apply (AFAIK an IPv6 has no dynamic range but —I tested it— there is nothing that prevents you from entering one).
>
> All I'm saying is that we need the proper limitations in place at the form level.  What do you think?

I agree. I have a separate branch for that.

>> +
>> +        generate_directives = set()
>> +        subnets, prefix, _ = get_details_for_ip_range(dynamic_range)
>> +        for subnet in subnets:
>> +            iterator = "%d-%d" %  (
>> +                (subnet.first & 0x000000ff),
>> +                (subnet.last & 0x000000ff))
>> +            hostname = get_hostname_template_for_network(
>
> Not sure I agree with that, the extraction allows deeper testing.

But not *that* much deeper.

I don't think it matters, unless you've got a cluster with a gajillion
interfaces.

>> +                # Only create GENERATE directives for IPv4 ranges.
>> +                generate_directives.extend(
>> +                    self.get_GENERATE_directives(dynamic_range))
>> +            else:
>> +                pass
>>          self.write_zone_file(
>>              self.target_path, self.make_parameters(),
>>              {
>> @@ -200,6 +307,9 @@
>>                      'AAAA': self.get_AAAA_mapping(
>>                          self._mapping, self.domain, self._dns_ip),
>>                  },
>> +                'generate_directives': {
>> +                    'A': generate_directives,
>> +                }
>>              })
>>
>>
>> @@ -225,6 +335,7 @@
>>          """
>>          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)
>>          super(DNSReverseZoneConfig, self).__init__(
>>              domain, zone_name=zone_name, **kwargs)
>> @@ -279,8 +390,40 @@
>>              if IPAddress(ip) in network
>>          )
>>
>> +    @classmethod
>> +    def get_GENERATE_directives(cls, dynamic_range, domain):
>> +        """Return the GENERATE directives for the reverse zone of a network."""
>> +        assert dynamic_range.size <= 256**2, (
>> +            "Cannot generate reverse zone mapping for any network larger than "
>> +            "/16.")
>> +
>> +        generate_directives = set()
>> +        subnets, _, rdns_suffix = get_details_for_ip_range(dynamic_range)
>> +        for subnet in subnets:
>> +            iterator = "%d-%d" %  (
>> +                (subnet.first & 0x000000ff),
>> +                (subnet.last & 0x000000ff))
>> +            hostname = get_hostname_template_for_network(
>> +                subnet, len(subnets)) % {
>> +                    'next_octet': (subnet.first & 0x0000ff00) >> 8}
>> +            rdns = "$.%d.%s" % (
>> +                (subnet.first & 0x0000ff00) >> 8,
>> +                rdns_suffix)
>> +            generate_directives.add(
>> +                (iterator, rdns, "%s.%s." % (hostname, domain)))
>> +
>> +        return sorted(generate_directives)
>> +
>>      def write_config(self):
>>          """Write the zone file."""
>> +        generate_directives = []
>> +        for dynamic_range in self._dynamic_ranges:
>> +            if dynamic_range.version == 4:
>> +                # Only create GENERATE directives for IPv4 ranges.
>> +                generate_directives.extend(
>> +                    self.get_GENERATE_directives(dynamic_range, self.domain))
>> +            else:
>> +                pass
>>          self.write_zone_file(
>>              self.target_path, self.make_parameters(),
>>              {
>> @@ -288,5 +431,8 @@
>>                      'PTR': self.get_PTR_mapping(
>>                          self._mapping, self.domain, self._network),
>>                  },
>> +                'generate_directives': {
>> +                    'PTR': generate_directives,
>> +                }
>>              }
>>          )

Revision history for this message

Andres Rodriguez (andreserl) wrote on 2014-10-29:

I'll produce packages later today q
So we can start testing this immediately and ensure we have no regressions.
On Oct 29, 2014 4:15 PM, "Graham Binns" <email address hidden> wrote:

> The proposal to merge
> lp:~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190 into lp:maas has
> been updated.
>
> Status: Needs review => Approved
>
> For more details, see:
>
> https://code.launchpad.net/~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190/+merge/239912
> --
>
> https://code.launchpad.net/~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190/+merge/239912
> You are subscribed to branch lp:maas.
>

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-29:

On 29 October 2014 20:18, Andres Rodriguez <email address hidden> wrote:
> I'll produce packages later today q
> So we can start testing this immediately and ensure we have no regressions.

Cool. I'll run up a branch for 1.7, too.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Andres Rodriguez

Blake Rouse

Brendan Donegan

Dave Walker

Deepa

Enrique Chirivella Pérez

Fumihito YOSHIDA

Graham Binns

MAAS Committers

Mike Pontillo

james beedy

 === modified file 'etc/maas/templates/dns/zone.template'
 --- etc/maas/templates/dns/zone.template	2012-10-04 15:59:56 +0000
 +++ etc/maas/templates/dns/zone.template	2014-10-29 19:33:27 +0000
@@ -12,6 +12,12 @@
+               )
      IN  NS  {{domain}}.
++{{for type, directive in generate_directives.items()}}
++{{for iterator_values, rdns, hostname in directive}}
++$GENERATE {{iterator_values}} {{rdns}} IN {{type}} {{hostname}}
++{{endfor}}
++{{endfor}}
++
  {{for type, mapping in mappings.items()}}
  {{for item_from, item_to in mapping}}
  {{item_from}} IN {{type}} {{item_to}}
 === modified file 'src/maasserver/dns/tests/test_config.py'
 --- src/maasserver/dns/tests/test_config.py	2014-10-15 11:22:22 +0000
 +++ src/maasserver/dns/tests/test_config.py	2014-10-29 19:33:27 +0000
@@ -165,15 +165,15 @@
          # A forward lookup on the hostname returns the IP address.
          fqdn = "%s.%s" % (hostname, domain)
          forward_lookup_result = self.dig_resolve(fqdn, version=version)
--        self.assertEqual(
--            [ip], forward_lookup_result,
++        self.expectThat(
++            forward_lookup_result, Contains(ip),
              "Failed to resolve '%s' (results: '%s')." % (
                  fqdn, ','.join(forward_lookup_result)))
          # A reverse lookup on the IP address returns the hostname.
          reverse_lookup_result = self.dig_reverse_resolve(
              ip, version=version)
--        self.assertEqual(
--            ["%s." % fqdn], reverse_lookup_result,
++        self.expectThat(
++            reverse_lookup_result, Contains("%s." % fqdn),
              "Failed to reverse resolve '%s' (results: '%s')." % (
                  fqdn, ','.join(reverse_lookup_result)))
 === modified file 'src/maasserver/dns/tests/test_zonegenerator.py'
 --- src/maasserver/dns/tests/test_zonegenerator.py	2014-09-15 14:28:28 +0000
 +++ src/maasserver/dns/tests/test_zonegenerator.py	2014-10-29 19:33:27 +0000
@@ -393,14 +393,28 @@
          [interface] = nodegroup.get_managed_interfaces()
          networks_dict = ZoneGenerator._get_networks()
          retrieved_interface = networks_dict[nodegroup]
--        self.assertEqual([interface.network], retrieved_interface)
++        self.assertEqual(
++            [
++                (
++                    interface.network,
++                    (interface.ip_range_low, interface.ip_range_high)
++                )
++            ],
++            retrieved_interface)
      def test_get_networks_returns_multiple_networks(self):
          nodegroups = [self.make_node_group() for _ in range(3)]
          networks_dict = ZoneGenerator._get_networks()
          for nodegroup in nodegroups:
              [interface] = nodegroup.get_managed_interfaces()
--            self.assertEqual([interface.network], networks_dict[nodegroup])
++            self.assertEqual(
++                [
++                    (
++                        interface.network,
++                        (interface.ip_range_low, interface.ip_range_high),
++                    ),
++                ],
++                networks_dict[nodegroup])
      def test_get_networks_returns_managed_networks(self):
          nodegroups = [
@@ -415,7 +429,10 @@
          self.assertEqual(
+             {
                  nodegroup: [
--                    interface.network
++                    (
++                        interface.network,
++                        (interface.ip_range_low, interface.ip_range_high),
++                    )
                      for interface in nodegroup.get_managed_interfaces()
+                     ]
                  for nodegroup in nodegroups
 === modified file 'src/maasserver/dns/zonegenerator.py'
 --- src/maasserver/dns/zonegenerator.py	2014-08-28 12:00:15 +0000
 +++ src/maasserver/dns/zonegenerator.py	2014-10-29 19:33:27 +0000
@@ -29,7 +29,10 @@
  from maasserver.models.config import Config
  from maasserver.models.nodegroup import NodeGroup
  from maasserver.server_address import get_maas_facing_server_address
--from netaddr import IPAddress
++from netaddr import (
++    IPAddress,
++    IPRange,
++    )
  from provisioningserver.dns.zoneconfig import (
      DNSForwardZoneConfig,
      DNSReverseZoneConfig,
@@ -180,9 +183,18 @@
      @staticmethod
      def _get_networks():
--        """Return a lazily evaluated nodegroup:network dict."""
--        return lazydict(lambda ng: [
--            interface.network for interface in ng.get_managed_interfaces()])
++        """Return a lazily evaluated nodegroup:network_details dict.
++
++        network_details takes the form of a tuple of (network,
++        (network.ip_range_low, network.ip_range_high)).
++        """
++
++        def get_network(nodegroup):
++            return [
++                (iface.network, (iface.ip_range_low, iface.ip_range_high))
++                for iface in nodegroup.get_managed_interfaces()
++            ]
++        return lazydict(get_network)
      @staticmethod
      def _get_srv_mappings():
@@ -209,6 +221,12 @@
          forward_nodegroups = sorted(nodegroups, key=get_domain)
          for domain, nodegroups in groupby(forward_nodegroups, get_domain):
              nodegroups = list(nodegroups)
++            dynamic_ranges = [
++                interface.get_dynamic_ip_range()
++                for nodegroup in nodegroups
++                for interface in nodegroup.get_managed_interfaces()
++            ]
++
              # A forward zone encompassing all nodes in the same domain.
              yield DNSForwardZoneConfig(
                  domain, serial=serial, dns_ip=dns_ip,
@@ -217,7 +235,8 @@
                      for nodegroup in nodegroups
                      for hostname, ip in mappings[nodegroup].items()
                      },
--                srv_mapping=set(srv_mappings)
++                srv_mapping=set(srv_mappings),
++                dynamic_ranges=dynamic_ranges,
+             )
      @staticmethod
@@ -226,11 +245,11 @@
          get_domain = lambda nodegroup: nodegroup.name
          reverse_nodegroups = sorted(nodegroups, key=networks.get)
          for nodegroup in reverse_nodegroups:
--            for network in networks[nodegroup]:
++            for network, dynamic_range in networks[nodegroup]:
                  mapping = mappings[nodegroup]
                  yield DNSReverseZoneConfig(
                      get_domain(nodegroup), serial=serial, mapping=mapping,
--                    network=network
++                    network=network, dynamic_ranges=[IPRange(*dynamic_range)]
+                 )
      def __iter__(self):
 === modified file 'src/provisioningserver/dns/tests/test_zoneconfig.py'
 --- src/provisioningserver/dns/tests/test_zoneconfig.py	2014-09-23 21:43:27 +0000
 +++ src/provisioningserver/dns/tests/test_zoneconfig.py	2014-10-29 19:33:27 +0000
@@ -22,10 +22,12 @@
  import random
  from maastesting.factory import factory
++from maastesting.matchers import MockNotCalled
  from maastesting.testcase import MAASTestCase
  from netaddr import (
      IPAddress,
      IPNetwork,
++    IPRange,
+     )
  from provisioningserver.dns.config import (
      get_dns_config_dir,
@@ -35,11 +37,14 @@
  from provisioningserver.dns.zoneconfig import (
      DNSForwardZoneConfig,
      DNSReverseZoneConfig,
++    intersect_iprange,
+     )
  from testtools.matchers import (
      Contains,
      ContainsAll,
++    Equals,
      FileContains,
++    HasLength,
      IsInstance,
      MatchesAll,
      MatchesStructure,
@@ -187,10 +192,13 @@
              ipv4_hostname: [ipv4_ip],
              ipv6_hostname: [ipv6_ip],
+         }
++        expected_generate_directives = (
++            DNSForwardZoneConfig.get_GENERATE_directives(network))
          srv = self.make_srv_record()
          dns_zone_config = DNSForwardZoneConfig(
              domain, serial=random.randint(1, 100),
--            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv])
++            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv],
++            dynamic_ranges=[IPRange(network.first, network.last)])
          dns_zone_config.write_config()
          self.assertThat(
              os.path.join(target_dir, 'zone.%s' % domain),
@@ -201,6 +209,12 @@
                              srv.service, self.get_srv_item_output(srv)),
                          '%s IN A %s' % (ipv4_hostname, ipv4_ip),
                          '%s IN AAAA %s' % (ipv6_hostname, ipv6_ip),
++                    ] +
++                    [
++                        '$GENERATE %s %s IN A %s' % (
++                            iterator_values, reverse_dns, hostname)
++                        for iterator_values, reverse_dns, hostname in
++                        expected_generate_directives
+                     ]
+                 )
+             )
@@ -222,6 +236,31 @@
                          '%s. IN A %s' % (dns_zone_config.domain, dns_ip),
                      ])))
++    def test_ignores_generate_directives_for_v6_dynamic_ranges(self):
++        patch_dns_config_path(self)
++        domain = factory.make_string()
++        network = factory.make_ipv4_network()
++        dns_ip = factory.pick_ip_in_network(network)
++        ipv4_hostname = factory.make_name('host')
++        ipv4_ip = factory.pick_ip_in_network(network)
++        ipv6_hostname = factory.make_name('host')
++        ipv6_ip = factory.make_ipv6_address()
++        ipv6_network = factory.make_ipv6_network()
++        dynamic_range = IPRange(ipv6_network.first, ipv6_network.last)
++        mapping = {
++            ipv4_hostname: [ipv4_ip],
++            ipv6_hostname: [ipv6_ip],
++        }
++        srv = self.make_srv_record()
++        dns_zone_config = DNSForwardZoneConfig(
++            domain, serial=random.randint(1, 100),
++            mapping=mapping, dns_ip=dns_ip, srv_mapping=[srv],
++            dynamic_ranges=[dynamic_range])
++        get_generate_directives = self.patch(
++            dns_zone_config, 'get_GENERATE_directives')
++        dns_zone_config.write_config()
++        self.assertThat(get_generate_directives, MockNotCalled())
++
      def test_config_file_is_world_readable(self):
          patch_dns_config_path(self)
          dns_zone_config = DNSForwardZoneConfig(
@@ -350,16 +389,43 @@
          target_dir = patch_dns_config_path(self)
          domain = factory.make_string()
          network = IPNetwork('192.168.0.1/22')
++        dynamic_network = IPNetwork('192.168.0.1/28')
          dns_zone_config = DNSReverseZoneConfig(
--            domain, serial=random.randint(1, 100), network=network)
++            domain, serial=random.randint(1, 100), network=network,
++            dynamic_ranges=[
++                IPRange(dynamic_network.first, dynamic_network.last)])
          dns_zone_config.write_config()
          reverse_file_name = 'zone.168.192.in-addr.arpa'
--        expected = Contains(
--            'IN  NS  %s' % domain)
++        expected_generate_directives = dns_zone_config.get_GENERATE_directives(
++            dynamic_network, domain)
++        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_generate_directives
++            ])
          self.assertThat(
              os.path.join(target_dir, reverse_file_name),
              FileContains(matcher=expected))
++    def test_ignores_generate_directives_for_v6_dynamic_ranges(self):
++        patch_dns_config_path(self)
++        domain = factory.make_string()
++        network = IPNetwork('192.168.0.1/22')
++        dynamic_network = IPNetwork("%s/64" % factory.make_ipv6_address())
++        dns_zone_config = DNSReverseZoneConfig(
++            domain, serial=random.randint(1, 100), network=network,
++            dynamic_ranges=[
++                IPRange(dynamic_network.first, dynamic_network.last)])
++        get_generate_directives = self.patch(
++            dns_zone_config, 'get_GENERATE_directives')
++        dns_zone_config.write_config()
++        self.assertThat(get_generate_directives, MockNotCalled())
++
      def test_reverse_config_file_is_world_readable(self):
          patch_dns_config_path(self)
          dns_zone_config = DNSReverseZoneConfig(
@@ -368,3 +434,286 @@
          dns_zone_config.write_config()
          filepath = FilePath(dns_zone_config.target_path)
          self.assertTrue(filepath.getPermissions().other.read)
++
++
++class TestDNSReverseZoneConfig_GetGenerateDirectives(MAASTestCase):
++    """Tests for `DNSReverseZoneConfig.get_GENERATE_directives()`."""
++
++    def test_excplicitly(self):
++        # The other tests in this TestCase rely on
++        # get_expected_generate_directives(), which is quite dense. Here
++        # we test get_GENERATE_directives() explicitly.
++        ip_range = IPRange('192.168.0.1', '192.168.2.128')
++        expected_directives = [
++            ("1-255", "$.0.168.192.in-addr.arpa.", "192-168-0-$.domain."),
++            ("0-255", "$.1.168.192.in-addr.arpa.", "192-168-1-$.domain."),
++            ("0-128", "$.2.168.192.in-addr.arpa.", "192-168-2-$.domain."),
++            ]
++        self.assertItemsEqual(
++            expected_directives,
++            DNSReverseZoneConfig.get_GENERATE_directives(
++                ip_range, domain="domain"))
++
++    def get_expected_generate_directives(self, network, domain):
++        ip_parts = network.network.format().split('.')
++        relevant_ip_parts = ip_parts[:-2]
++
++        first_address = IPAddress(network.first).format()
++        first_address_parts = first_address.split(".")
++
++        if network.size < 256:
++            last_address = IPAddress(network.last).format()
++            iterator_low = int(first_address_parts[-1])
++            iterator_high = last_address.split('.')[-1]
++        else:
++            iterator_low = 0
++            iterator_high = 255
++
++        second_octet_offset = int(first_address_parts[-2])
++        expected_generate_directives = []
++        directives_needed = network.size / 256
++
++        if directives_needed == 0:
++            directives_needed = 1
++        for num in range(directives_needed):
++            expected_address_base = "%s-%s" % tuple(relevant_ip_parts)
++            expected_address = "%s-%s-$" % (
++                expected_address_base, num + second_octet_offset)
++            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)
++            expected_generate_directives.append(
++                (
++                    "%s-%s" % (iterator_low, iterator_high),
++                    expected_rdns_template,
++                    "%s.%s." % (expected_address, domain)
++                ))
++            relevant_ip_parts.reverse()
++        return expected_generate_directives
++
++    def test_returns_single_entry_for_slash_24_network(self):
++        network = IPNetwork("%s/24" % factory.make_ipv4_address())
++        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(1))
++        self.assertItemsEqual(expected_generate_directives, directives)
++
++    def test_returns_single_entry_for_tiny_network(self):
++        network = IPNetwork("%s/28" % factory.make_ipv4_address())
++        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(1))
++        self.assertItemsEqual(expected_generate_directives, directives)
++
++    def test_returns_single_entry_for_weird_small_range(self):
++        ip_range = IPRange('10.0.0.1', '10.0.0.255')
++        domain = factory.make_string()
++        directives = DNSReverseZoneConfig.get_GENERATE_directives(
++            ip_range, domain)
++        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))
++        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))
++        self.assertItemsEqual(expected_generate_directives, directives)
++
++    def test_rejects_network_larger_than_slash_16(self):
++        network = IPNetwork("%s/15" % factory.make_ipv4_address())
++        error = self.assertRaises(
++            AssertionError, DNSReverseZoneConfig.get_GENERATE_directives,
++            network, None)
++        self.assertEqual(
++            "Cannot generate reverse zone mapping for any network larger "
++            "than /16.",
++            unicode(error))
++
++    def test_sorts_output_by_hostname(self):
++        network = IPNetwork("10.0.0.1/23")
++        domain = factory.make_string()
++
++        expected_hostname = "10-0-%s-$." + domain + "."
++        expected_rdns = "$.%s.0.10.in-addr.arpa."
++
++        directives = list(DNSReverseZoneConfig.get_GENERATE_directives(
++            network, domain))
++        self.expectThat(
++            directives[0], Equals(
++                ("0-255", expected_rdns % "0", expected_hostname % "0")))
++        self.expectThat(
++            directives[1], Equals(
++                ("0-255", expected_rdns % "1", expected_hostname % "1")))
++
++
++class TestDNSForwardZoneConfig_GetGenerateDirectives(MAASTestCase):
++    """Tests for `DNSForwardZoneConfig.get_GENERATE_directives()`."""
++
++    def test_excplicitly(self):
++        # The other tests in this TestCase rely on
++        # get_expected_generate_directives(), which is quite dense. Here
++        # we test get_GENERATE_directives() explicitly.
++        ip_range = IPRange('192.168.0.1', '192.168.2.128')
++        expected_directives = [
++            ("1-255", "192-168-0-$", "192.168.0.$"),
++            ("0-255", "192-168-1-$", "192.168.1.$"),
++            ("0-128", "192-168-2-$", "192.168.2.$"),
++            ]
++        self.assertItemsEqual(
++            expected_directives,
++            DNSForwardZoneConfig.get_GENERATE_directives(ip_range))
++
++    def get_expected_generate_directives(self, network):
++        ip_parts = network.network.format().split('.')
++        ip_parts[-1] = "$"
++        expected_hostname = "%s" % "-".join(ip_parts)
++        expected_address = ".".join(ip_parts)
++
++        first_address = IPAddress(network.first).format()
++        first_address_parts = first_address.split(".")
++        last_address = IPAddress(network.last).format()
++        last_address_parts = last_address.split(".")
++
++        if network.size < 256:
++            iterator_low = int(first_address_parts[-1])
++            if iterator_low == 0:
++                iterator_low = 1
++            iterator_high = last_address_parts[-1]
++        else:
++            iterator_low = 0
++            iterator_high = 255
++
++        expected_iterator_values = "%s-%s" % (iterator_low, iterator_high)
++
++        directives_needed = network.size / 256
++        if directives_needed == 0:
++            directives_needed = 1
++        expected_directives = []
++        for num in range(directives_needed):
++            ip_parts[-2] = unicode(num + int(ip_parts[-2]))
++            expected_address = ".".join(ip_parts)
++            expected_hostname = "%s" % "-".join(ip_parts)
++            expected_directives.append(
++                (
++                    expected_iterator_values,
++                    expected_hostname,
++                    expected_address
++                ))
++        return expected_directives
++
++    def test_returns_single_entry_for_slash_24_network(self):
++        network = IPNetwork("%s/24" % factory.make_ipv4_address())
++        expected_directives = self.get_expected_generate_directives(network)
++        directives = DNSForwardZoneConfig.get_GENERATE_directives(
++            network)
++        self.expectThat(directives, HasLength(1))
++        self.assertItemsEqual(expected_directives, directives)
++
++    def test_returns_single_entry_for_tiny_network(self):
++        network = IPNetwork("%s/31" % factory.make_ipv4_address())
++
++        expected_directives = self.get_expected_generate_directives(network)
++        directives = DNSForwardZoneConfig.get_GENERATE_directives(
++            network)
++        self.assertEqual(1, len(expected_directives))
++        self.assertItemsEqual(expected_directives, directives)
++
++    def test_returns_two_entries_for_slash_23_network(self):
++        network = IPNetwork("%s/23" % factory.make_ipv4_address())
++
++        expected_directives = self.get_expected_generate_directives(network)
++        directives = DNSForwardZoneConfig.get_GENERATE_directives(
++            network)
++        self.assertEqual(2, len(expected_directives))
++        self.assertItemsEqual(expected_directives, directives)
++
++    def test_dtrt_for_larger_networks(self):
++        # For every other network size that we're not explicitly
++        # testing here,
++        # DNSForwardZoneConfig.get_GENERATE_directives() will return
++        # one GENERATE directive for every 255 addresses in the network.
++        for prefixlen in range(23, 16):
++            network = IPNetwork(
++                "%s/%s" % (factory.make_ipv4_address(), prefixlen))
++            directives = DNSForwardZoneConfig.get_GENERATE_directives(
++                network)
++            self.assertIsEqual(network.size / 256, len(directives))
++
++    def test_rejects_network_larger_than_slash_16(self):
++        network = IPNetwork("%s/15" % factory.make_ipv4_address())
++        error = self.assertRaises(
++            AssertionError, DNSForwardZoneConfig.get_GENERATE_directives,
++            network)
++        self.assertEqual(
++            "Cannot generate reverse zone mapping for any network larger "
++            "than /16.",
++            unicode(error))
++
++    def test_sorts_output(self):
++        network = IPNetwork("10.0.0.0/23")
++
++        expected_hostname = "10-0-%s-$"
++        expected_address = "10.0.%s.$"
++
++        directives = list(DNSForwardZoneConfig.get_GENERATE_directives(
++            network))
++        self.expectThat(len(directives), Equals(2))
++        self.expectThat(
++            directives[0], Equals(
++                ("0-255", expected_hostname % "0", expected_address % "0")))
++        self.expectThat(
++            directives[1], Equals(
++                ("0-255", expected_hostname % "1", expected_address % "1")))
++
++
++class TestIntersectIPRange(MAASTestCase):
++    """Tests for `intersect_iprange()`."""
++
++    def test_finds_intersection_between_two_ranges(self):
++        range_1 = IPRange('10.0.0.1', '10.0.0.255')
++        range_2 = IPRange('10.0.0.128', '10.0.0.200')
++        intersect = intersect_iprange(range_1, range_2)
++        self.expectThat(
++            IPAddress(intersect.first), Equals(IPAddress('10.0.0.128')))
++        self.expectThat(
++            IPAddress(intersect.last), Equals(IPAddress('10.0.0.200')))
++
++    def test_ignores_non_intersecting_ranges(self):
++        range_1 = IPRange('10.0.0.1', '10.0.0.255')
++        range_2 = IPRange('10.0.1.128', '10.0.1.200')
++        self.assertIsNone(intersect_iprange(range_1, range_2))
++
++    def test_finds_partial_intersection(self):
++        range_1 = IPRange('10.0.0.1', '10.0.0.128')
++        range_2 = IPRange('10.0.0.64', '10.0.0.200')
++        intersect = intersect_iprange(range_1, range_2)
++        self.expectThat(
++            IPAddress(intersect.first), Equals(IPAddress('10.0.0.64')))
++        self.expectThat(
++            IPAddress(intersect.last), Equals(IPAddress('10.0.0.128')))
 === modified file 'src/provisioningserver/dns/zoneconfig.py'
 --- src/provisioningserver/dns/zoneconfig.py	2014-08-15 11:10:09 +0000
 +++ src/provisioningserver/dns/zoneconfig.py	2014-10-29 19:33:27 +0000
@@ -23,7 +23,11 @@
  from itertools import chain
  import math
--from netaddr import IPAddress
++from netaddr import (
++    IPAddress,
++    IPRange,
++    spanning_cidr,
++    )
  from netaddr.core import AddrFormatError
  from provisioningserver.dns.config import (
      compose_config_path,
@@ -33,6 +37,19 @@
  from provisioningserver.utils.fs import incremental_write
++def intersect_iprange(network, iprange):
++    """Return the intersection between two IPNetworks or IPRanges.
++
++    IPSet is notoriously inefficient so we intersect ourselves here.
++    """
++    if network.last >= iprange.first and network.first <= iprange.last:
++        first = max(network.first, iprange.first)
++        last = min(network.last, iprange.last)
++        return IPRange(first, last)
++    else:
++        return None
++
++
  def get_fqdn_or_ip_address(target):
      """Returns the ip address is target is a valid ip address, otherwise
      returns the target with appended '.' if missing."""
@@ -52,6 +69,52 @@
              yield hostname, ip
++def get_details_for_ip_range(ip_range):
++    """For a given IPRange, return all subnets, a useable prefix and the
++    reverse DNS suffix calculated from that IP range.
++
++    :return: A tuple of:
++        All subnets of /24 (or smaller if there is no /24 subnet to be
++        found) in `ip_range`.
++        A prefix made from the first two octets in the range.
++        A RDNS suffix calculated from the first two octets in the range.
++    """
++    # Calculate a spanning network for the range above. There are
++    # 256 /24 networks in a /16, so that's the most /24s we're going
++    # to have to deal with; this matters later on when we iterate
++    # through the /24s within this network.
++    cidr = spanning_cidr(ip_range)
++    subnets = cidr.subnet(max(24, cidr.prefixlen))
++
++    # Split the spanning network into /24 subnets, then see if they fall
++    # entirely within the original network range, partially, or not at
++    # all.
++    intersecting_subnets = []
++    for subnet in subnets:
++        intersect = intersect_iprange(subnet, ip_range)
++        if intersect is None:
++            # The subnet does not fall within the original network.
++            pass
++        else:
++            # The subnet falls partially within the original network, so print
++            # out a $GENERATE expression for a subset of the /24.
++            intersecting_subnets.append(intersect)
++
++    octet_one = (cidr.value & 0xff000000) >> 24
++    octet_two = (cidr.value & 0x00ff0000) >> 16
++
++    # The first two octets of the network range formatted in the
++    # usual dotted-quad style. We can precalculate the start of any IP
++    # address in the range because we're only ever dealing with /16
++    # networks and smaller.
++    prefix = "%d.%d" % (octet_one, octet_two)
++
++    # Similarly, we can calculate what the reverse DNS suffix is going
++    # to look like.
++    rdns_suffix = "%d.%d.in-addr.arpa." % (octet_two, octet_one)
++    return intersecting_subnets, prefix, rdns_suffix
++
++
  class DNSZoneConfigBase:
      """Base class for zone writers."""
@@ -117,7 +180,8 @@
          """
          self._dns_ip = kwargs.pop('dns_ip', None)
          self._mapping = kwargs.pop('mapping', {})
--        self._network = None
++        self._network = kwargs.pop('network', None)
++        self._dynamic_ranges = kwargs.pop('dynamic_ranges', [])
          self._srv_mapping = kwargs.pop('srv_mapping', [])
          super(DNSForwardZoneConfig, self).__init__(
              domain, zone_name=domain, **kwargs)
@@ -187,8 +251,45 @@
                  target)
              yield (record.service, item)
++    @classmethod
++    def get_GENERATE_directives(cls, dynamic_range):
++        """Return the GENERATE directives for the forward zone of a network.
++        """
++        assert dynamic_range.size <= 256 ** 2, (
++            "Cannot generate reverse zone mapping for any network larger than "
++            "/16.")
++
++        generate_directives = set()
++        subnets, prefix, _ = 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('.', '-'),
++                # Calculate what the third quad (i.e. 10.0.X.1) value should
++                # be for this subnet.
++                (subnet.first & 0x0000ff00) >> 8,
++                )
++
++            ip_address = "%s.%d.$" % (
++                prefix,
++                (subnet.first & 0x0000ff00) >> 8)
++            generate_directives.add((iterator, hostname, ip_address))
++
++        return sorted(
++            generate_directives, key=lambda directive: directive[2])
++
      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(),
+             {
@@ -200,6 +301,9 @@
                      'AAAA': self.get_AAAA_mapping(
                          self._mapping, self.domain, self._dns_ip),
                  },
++                'generate_directives': {
++                    'A': generate_directives,
++                }
              })
@@ -225,6 +329,7 @@
          """
          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)
          super(DNSReverseZoneConfig, self).__init__(
              domain, zone_name=zone_name, **kwargs)
@@ -279,8 +384,40 @@
              if IPAddress(ip) in network
+         )
++    @classmethod
++    def get_GENERATE_directives(cls, dynamic_range, domain):
++        """Return the GENERATE directives for the reverse zone of a network."""
++        assert dynamic_range.size <= 256 ** 2, (
++            "Cannot generate reverse zone mapping for any network larger than "
++            "/16.")
++
++        generate_directives = set()
++        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)))
++
++        return sorted(
++            generate_directives, key=lambda directive: directive[2])
++
      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(),
+             {
@@ -288,5 +425,8 @@
                      'PTR': self.get_PTR_mapping(
                          self._mapping, self.domain, self._network),
                  },
++                'generate_directives': {
++                    'PTR': generate_directives,
++                }
+             }
+         )

MAAS

Merge lp:~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190 into lp:~maas-committers/maas/trunk

Commit message

Description of the change

Preview Diff

Subscribers