Merge lp:~jtv/maas/dns-zones-explicit-data-flow into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Make data-passing in the DNS zone config classes more explicit.

This completes my refactoring of the DNS config module. It makes private some instance variables that are not effectively part of the classes' interfaces. Helper methods become class methods and receive their inputs as parameters, so that hopefully it'll be easier to assess the impact of future changes. It also makes them a bit easier to test, but that's just a happy coincidence. The main thing is improving our ability to understand and change the code.

A new method make_parameters does the common part of what the two get_context methods did. The remainder of what those methods did is now inlined into the write_config methods.

Jeroen

[Raphaël Badin (rvb)]

[0]

82 - def get_cname_mapping(self):
83 + @classmethod
84 + def get_cname_mapping(cls, mapping):
85 """Return a generator with the mapping: hostname->generated hostname.
86
87 The mapping only contains hosts for which the two host names differ.

98 - def get_static_mapping(self):
99 + @classmethod
100 + def get_static_mapping(cls, domain, networks, dns_ip):
101 """Return a generator with the mapping fqdn->ip for the generated ips.

Both these methods have been turned into class-level utilities, it might be worth updating their docstrings to describe the parameters.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Updated.

[MAAS Lander (maas-lander)]

The prerequisite https://code.launchpad.net/~jtv/maas/delegate-write_config has not yet been merged into https://code.launchpad.net/~maas-maintainers/maas/trunk.

=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py	2013-12-21 16:36:41 +0000
+++ src/maasserver/tests/test_dns.py	2014-01-22 11:01:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test DNS module."""
@@ -416,7 +416,7 @@
     return MatchesAll(
         IsInstance(DNSForwardZoneConfig),
         MatchesStructure.byEquality(
-            domain=domain, networks=networks))
+            domain=domain, _networks=networks))
 
 
 def reverse_zone(domain, network):
@@ -428,7 +428,7 @@
     return MatchesAll(
         IsInstance(DNSReverseZoneConfig),
         MatchesStructure.byEquality(
-            domain=domain, network=network))
+            domain=domain, _network=network))
 
 
 class TestZoneGenerator(MAASServerTestCase):

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2014-01-22 11:01:40 +0000
+++ src/provisioningserver/dns/config.py	2014-01-22 11:01:40 +0000
@@ -286,8 +286,16 @@
         self.serial = serial
         self.target_path = compose_config_path('zone.%s' % self.zone_name)
 
+    def make_parameters(self):
+        """Return a dict of the common template parameters."""
+        return {
+            'domain': self.domain,
+            'serial': self.serial,
+            'modified': unicode(datetime.today()),
+        }
+
     @classmethod
-    def write_zone_file(cls, output_file, parameters):
+    def write_zone_file(cls, output_file, *parameters):
         """Write a zone file based on the zone file template.
 
         There is a subtlety with zone files: their filesystem timestamp must
@@ -295,7 +303,7 @@
         support a resolution of one second, and so this method may set an
         unexpected modification time in order to maintain that property.
         """
-        content = render_dns_template(cls.template_file_name, parameters)
+        content = render_dns_template(cls.template_file_name, *parameters)
         with report_missing_config_dir():
             incremental_write(content, output_file, mode=0644)
 
@@ -306,6 +314,9 @@
     def __init__(self, domain, **kwargs):
         """See `DNSZoneConfigBase.__init__`.
 
+        :param domain: The domain name of the forward zone.
+        :param serial: The serial to use in the zone file. This must increment
+            on each change.
         :param networks: The networks that the mapping exists within.
         :type networks: Sequence of :class:`netaddr.IPNetwork`
         :param dns_ip: The IP address of the DNS server authoritative for this
@@ -313,57 +324,59 @@
         :param mapping: A hostname:ip-address mapping for all known hosts in
             the zone.
         """
-        self.networks = kwargs.pop('networks', [])
-        self.dns_ip = kwargs.pop('dns_ip', None)
-        self.mapping = kwargs.pop('mapping', {})
+        self._networks = kwargs.pop('networks', [])
+        self._dns_ip = kwargs.pop('dns_ip', None)
+        self._mapping = kwargs.pop('mapping', {})
         super(DNSForwardZoneConfig, self).__init__(
             domain, zone_name=domain, **kwargs)
 
-    def get_cname_mapping(self):
-        """Return a generator with the mapping: hostname->generated hostname.
+    @classmethod
+    def get_cname_mapping(cls, mapping):
+        """Return a generator mapping hostnames to generated hostnames.
 
         The mapping only contains hosts for which the two host names differ.
 
+        :param mapping: A dict mapping host names to IP addresses.
         :return: A generator of tuples: (host name, generated host name).
         """
         # We filter out cases where the two host names are identical: it
         # would be wrong to define a CNAME that maps to itself.
-        for hostname, ip in self.mapping.items():
+        for hostname, ip in mapping.items():
             generated_name = generated_hostname(ip)
             if generated_name != hostname:
                 yield (hostname, generated_name)
 
-    def get_static_mapping(self):
-        """Return a generator with the mapping fqdn->ip for the generated ips.
+    @classmethod
+    def get_static_mapping(cls, domain, networks, dns_ip):
+        """Return a generator mapping a network's generated fqdns to ips.
 
         The generated mapping is the mapping between the generated hostnames
         and the IP addresses for all the possible IP addresses in zone.
-        Note that we return a list of tuples instead of a dictionary in order
-        to be able to return a generator.
+        The return type is a sequence of tuples, not a dictionary, so that we
+        don't have to generate the whole thing at once.
+
+        :param domain: Zone's domain name.
+        :param networks: Sequence of :class:`netaddr.IPNetwork` describing
+            the networks whose IP-based generated host names should be mapped
+            to the corresponding IP addresses.
+        :param dns_ip: IP address for the zone's authoritative DNS server.
         """
-        ips = imap(unicode, chain.from_iterable(self.networks))
+        ips = imap(unicode, chain.from_iterable(networks))
         static_mapping = ((generated_hostname(ip), ip) for ip in ips)
         # Add A record for the name server's IP.
-        return chain([('%s.' % self.domain, self.dns_ip)], static_mapping)
-
-    def get_context(self):
-        """Return the dict used to render the DNS zone file.
-
-        That context dict is used to render the DNS zone file.
-        """
-        return {
-            'domain': self.domain,
-            'serial': self.serial,
-            'modified': unicode(datetime.today()),
-            'mappings': {
-                'CNAME': self.get_cname_mapping(),
-                'A': self.get_static_mapping(),
-                }
-            }
+        return chain([('%s.' % domain, dns_ip)], static_mapping)
 
     def write_config(self):
         """Write the zone file."""
-        self.write_zone_file(self.target_path, self.get_context())
+        self.write_zone_file(
+            self.target_path, self.make_parameters(),
+            {
+                'mappings': {
+                    'CNAME': self.get_cname_mapping(self._mapping),
+                    'A': self.get_static_mapping(
+                        self.domain, self._networks, self._dns_ip),
+                },
+            })
 
 
 class DNSReverseZoneConfig(DNSZoneConfigBase):
@@ -372,11 +385,14 @@
     def __init__(self, domain, **kwargs):
         """See `DNSZoneConfigBase.__init__`.
 
+        :param domain: The domain name of the forward zone.
+        :param serial: The serial to use in the zone file. This must increment
+            on each change.
         :param network: The network that the mapping exists within.
         :type network: :class:`netaddr.IPNetwork`
         """
-        self.network = kwargs.pop("network", None)
-        zone_name = self.compose_zone_name(self.network)
+        self._network = kwargs.pop("network", None)
+        zone_name = self.compose_zone_name(self._network)
         super(DNSReverseZoneConfig, self).__init__(
             domain, zone_name=zone_name, **kwargs)
 
@@ -402,33 +418,33 @@
         significant_octets = islice(reversed(ip.words), byte_num)
         return '.'.join(imap(unicode, significant_octets))
 
-    def get_static_mapping(self):
-        """Return the reverse generated mapping: (shortened) ip->fqdn.
+    @classmethod
+    def get_static_mapping(cls, domain, network):
+        """Return reverse mapping: shortened IPs to generated fqdns.
 
         The reverse generated mapping is the mapping between the IP addresses
         and the generated hostnames for all the possible IP addresses in zone.
+
+        :param domain: Zone's domain name.
+        :param network: Network whose IP addresses should be mapped to their
+            corresponding generated hostnames.
+        :type network: :class:`netaddr.IPNetwork`
         """
-        byte_num = 4 - self.network.netmask.words.count(255)
+        byte_num = 4 - network.netmask.words.count(255)
         return (
-            (self.shortened_reversed_ip(ip, byte_num),
-                '%s.%s.' % (generated_hostname(ip), self.domain))
-            for ip in self.network
-            )
-
-    def get_context(self):
-        """Return the dict used to render the DNS reverse zone file.
-
-        That context dict is used to render the DNS reverse zone file.
-        """
-        return {
-            'domain': self.domain,
-            'serial': self.serial,
-            'modified': unicode(datetime.today()),
-            'mappings': {
-                'PTR': self.get_static_mapping(),
-                }
-            }
+            (
+                cls.shortened_reversed_ip(ip, byte_num),
+                '%s.%s.' % (generated_hostname(ip), domain),
+            )
+            for ip in network
+            )
 
     def write_config(self):
         """Write the zone file."""
-        self.write_zone_file(self.target_path, self.get_context())
+        self.write_zone_file(
+            self.target_path, self.make_parameters(),
+            {
+                'mappings': {
+                    'PTR': self.get_static_mapping(self.domain, self._network),
+                },
+            })

=== modified file 'src/provisioningserver/dns/tests/test_config.py'
--- src/provisioningserver/dns/tests/test_config.py	2014-01-22 11:01:40 +0000
+++ src/provisioningserver/dns/tests/test_config.py	2014-01-22 11:01:40 +0000
@@ -387,8 +387,8 @@
             MatchesStructure.byEquality(
                 domain=domain,
                 serial=serial,
-                mapping=mapping,
-                networks=[network],
+                _mapping=mapping,
+                _networks=[network],
                 )
             )
 
@@ -400,15 +400,9 @@
             dns_zone_config.target_path)
 
     def test_forward_zone_get_cname_mapping_returns_iterator(self):
-        name = factory.getRandomString()
-        network = IPNetwork('192.12.0.1/30')
-        dns_ip = factory.getRandomIPInNetwork(network)
-        dns_zone_config = DNSForwardZoneConfig(
-            name, networks=[network], dns_ip=dns_ip,
-            mapping={
-                factory.make_name('hostname'): factory.getRandomIPAddress()})
         self.assertThat(
-            dns_zone_config.get_cname_mapping(),
+            DNSForwardZoneConfig.get_cname_mapping(
+                {factory.make_name('host'): factory.getRandomIPAddress()}),
             MatchesAll(
                 IsInstance(Iterable), Not(IsInstance(Sequence))))
 
@@ -416,24 +410,17 @@
         # We don't write cname records to map host names to themselves.
         # Without this, a node would get an invalid cname record upon
         # enlistment.
-        zone = factory.make_name('zone')
         network = IPNetwork('10.250.99.0/24')
         ip = factory.getRandomIPInNetwork(network)
         generated_name = generated_hostname(ip)
-        dns_zone_config = DNSForwardZoneConfig(
-            zone, networks=[network],
-            dns_ip=factory.getRandomIPInNetwork(network),
-            mapping={generated_name: ip})
         self.assertNotIn(
             generated_name,
-            dict(dns_zone_config.get_cname_mapping()))
+            dict(DNSForwardZoneConfig.get_cname_mapping({generated_name: ip})))
 
     def test_get_static_mapping(self):
         name = factory.getRandomString()
         network = IPNetwork('192.12.0.1/30')
         dns_ip = factory.getRandomIPInNetwork(network)
-        dns_zone_config = DNSForwardZoneConfig(
-            name, networks=[network], dns_ip=dns_ip)
         self.assertItemsEqual(
             [
                 ('%s.' % name, dns_ip),
@@ -442,17 +429,14 @@
                 (generated_hostname('192.12.0.2'), '192.12.0.2'),
                 (generated_hostname('192.12.0.3'), '192.12.0.3'),
             ],
-            dns_zone_config.get_static_mapping(),
-            )
+            DNSForwardZoneConfig.get_static_mapping(name, [network], dns_ip))
 
     def test_forward_zone_get_static_mapping_returns_iterator(self):
         name = factory.getRandomString()
         network = IPNetwork('192.12.0.1/30')
         dns_ip = factory.getRandomIPInNetwork(network)
-        dns_zone_config = DNSForwardZoneConfig(
-            name, networks=[network], dns_ip=dns_ip)
         self.assertThat(
-            dns_zone_config.get_static_mapping(),
+            DNSForwardZoneConfig.get_static_mapping(name, [network], dns_ip),
             MatchesAll(
                 IsInstance(Iterable), Not(IsInstance(Sequence))))
 
@@ -460,8 +444,6 @@
         name = factory.getRandomString()
         networks = IPNetwork('11.11.11.11/31'), IPNetwork('22.22.22.22/31')
         dns_ip = factory.getRandomIPInNetwork(networks[0])
-        dns_zone_config = DNSForwardZoneConfig(
-            name, networks=networks, dns_ip=dns_ip)
         self.assertItemsEqual(
             [
                 ('%s.' % name, dns_ip),
@@ -470,7 +452,7 @@
                 (generated_hostname('22.22.22.22'), '22.22.22.22'),
                 (generated_hostname('22.22.22.23'), '22.22.22.23'),
             ],
-            dns_zone_config.get_static_mapping(),
+            DNSForwardZoneConfig.get_static_mapping(name, networks, dns_ip),
             )
 
     def test_writes_dns_zone_config(self):
@@ -534,7 +516,7 @@
             MatchesStructure.byEquality(
                 domain=domain,
                 serial=serial,
-                network=network,
+                _network=network,
                 )
             )
 
@@ -586,17 +568,16 @@
             dns_zone_config.zone_name)
 
     def test_get_static_mapping_returns_iterator(self):
-        dns_zone_config = DNSReverseZoneConfig(
-            factory.getRandomString(), network=IPNetwork('192.12.0.1/30'))
         self.assertThat(
-            dns_zone_config.get_static_mapping(),
+            DNSReverseZoneConfig.get_static_mapping(
+                factory.make_name('zone'),
+                network=IPNetwork('192.12.0.1/30')),
             MatchesAll(
                 IsInstance(Iterable), Not(IsInstance(Sequence))))
 
     def test_get_static_mapping(self):
         name = factory.getRandomString()
         network = IPNetwork('192.12.0.1/30')
-        dns_zone_config = DNSReverseZoneConfig(name, network=network)
         self.assertItemsEqual(
             [
                 ('0', '%s.' % generated_hostname('192.12.0.0', name)),
@@ -604,8 +585,7 @@
                 ('2', '%s.' % generated_hostname('192.12.0.2', name)),
                 ('3', '%s.' % generated_hostname('192.12.0.3', name)),
             ],
-            dns_zone_config.get_static_mapping(),
-            )
+            DNSReverseZoneConfig.get_static_mapping(name, network))
 
     def test_writes_dns_zone_config_with_NS_record(self):
         target_dir = patch_dns_config_path(self)