Merge lp:~jtv/maas/bug-1066958 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Don't produce CNAMEs that are identical to hosts' respective auto-generated names.

This implements Raphael's suggestion in bug 1066958, so that we no longer create invalid CNAME records when we enlist nodes. I did not have a pre-imp; I just reproduced the bug in a unit test, and then changed the code to make the test pass. No other tests were affected.

Jeroen

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/16/2012 8:13 AM, Jeroen T. Vermeulen wrote:
> Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1066958
> into lp:maas.
>
> Commit message: Don't produce CNAMEs that are identical to hosts'
> respective auto-generated names.
>
> Requested reviews: Launchpad code reviewers (launchpad-reviewers)
> Related bugs: Bug #1066958 in MAAS: "DNS config is invalid after a
> node gets enlisted." https://bugs.launchpad.net/maas/+bug/1066958
>
> For more details, see:
> https://code.launchpad.net/~jtv/maas/bug-1066958/+merge/129798
>
> This implements Raphael's suggestion in bug 1066958, so that we no
> longer create invalid CNAME records when we enlist nodes. I did
> not have a pre-imp; I just reproduced the bug in a unit test, and
> then changed the code to make the test pass. No other tests were
> affected.
>
>
> Jeroen
>

This seems like a case where using the 'yield' form for a generator
would be better, so that you don't have to double-compute the value.

for hostname, ip in self.mapping.items():
  gen_hostname = generated_hostname(ip)
  if gen_hostname != hostname:
    yield (hostname, generated_hostname)

Otherwise:
 review: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlB854gACgkQJdeBCYSNAAM7PgCgxBWSVkpZo4SFy1+qhVo+ApAZ
Mj0An2plDHYeivlwbn6jF+APx9E9n1Wf
=wOxy
-----END PGP SIGNATURE-----

review: Approve

[Jeroen T. Vermeulen (jtv)]

Implemented your suggestion.

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2012-10-05 08:21:10 +0000
+++ src/provisioningserver/dns/config.py	2012-10-16 09:34:35 +0000
@@ -304,13 +304,16 @@
     def get_cname_mapping(self):
         """Return a generator with the mapping: hostname->generated hostname.
 
-        Note that we return a list of tuples instead of a dictionary in order
-        to be able to return a generator.
+        The mapping only contains hosts for which the two host names differ.
+
+        :return: A generator of tuples: (host name, generated host name).
         """
-        return (
-            (hostname, generated_hostname(ip))
-            for hostname, ip in self.mapping.items()
-        )
+        # 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():
+            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.

=== modified file 'src/provisioningserver/dns/tests/test_config.py'
--- src/provisioningserver/dns/tests/test_config.py	2012-10-05 08:21:10 +0000
+++ src/provisioningserver/dns/tests/test_config.py	2012-10-16 09:34:35 +0000
@@ -282,6 +282,22 @@
             MatchesAll(
                 IsInstance(Iterable), Not(IsInstance(Sequence))))
 
+    def test_forward_zone_get_cname_mapping_skips_identity(self):
+        # 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()))
+
     def test_get_static_mapping(self):
         name = factory.getRandomString()
         network = IPNetwork('192.12.0.1/30')