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

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: 1278
Merged at revision: 1276
Proposed branch: lp:~jtv/maas/bug-1066958
Merge into: lp:maas/trunk
Diff against target: 53 lines (+25/-6)
2 files modified
src/provisioningserver/dns/config.py (+9/-6)
src/provisioningserver/dns/tests/test_config.py (+16/-0)
To merge this branch: bzr merge lp:~jtv/maas/bug-1066958
Reviewer Review Type Date Requested Status
John A Meinel (community) Approve
Review via email: mp+129798@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

-----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
lp:~jtv/maas/bug-1066958 updated
1278. By Jeroen T. Vermeulen

Review suggestion.

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Implemented your suggestion.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/dns/config.py'
2--- src/provisioningserver/dns/config.py 2012-10-05 08:21:10 +0000
3+++ src/provisioningserver/dns/config.py 2012-10-16 09:34:35 +0000
4@@ -304,13 +304,16 @@
5 def get_cname_mapping(self):
6 """Return a generator with the mapping: hostname->generated hostname.
7
8- Note that we return a list of tuples instead of a dictionary in order
9- to be able to return a generator.
10+ The mapping only contains hosts for which the two host names differ.
11+
12+ :return: A generator of tuples: (host name, generated host name).
13 """
14- return (
15- (hostname, generated_hostname(ip))
16- for hostname, ip in self.mapping.items()
17- )
18+ # We filter out cases where the two host names are identical: it
19+ # would be wrong to define a CNAME that maps to itself.
20+ for hostname, ip in self.mapping.items():
21+ generated_name = generated_hostname(ip)
22+ if generated_name != hostname:
23+ yield (hostname, generated_name)
24
25 def get_static_mapping(self):
26 """Return a generator with the mapping fqdn->ip for the generated ips.
27
28=== modified file 'src/provisioningserver/dns/tests/test_config.py'
29--- src/provisioningserver/dns/tests/test_config.py 2012-10-05 08:21:10 +0000
30+++ src/provisioningserver/dns/tests/test_config.py 2012-10-16 09:34:35 +0000
31@@ -282,6 +282,22 @@
32 MatchesAll(
33 IsInstance(Iterable), Not(IsInstance(Sequence))))
34
35+ def test_forward_zone_get_cname_mapping_skips_identity(self):
36+ # We don't write cname records to map host names to themselves.
37+ # Without this, a node would get an invalid cname record upon
38+ # enlistment.
39+ zone = factory.make_name('zone')
40+ network = IPNetwork('10.250.99.0/24')
41+ ip = factory.getRandomIPInNetwork(network)
42+ generated_name = generated_hostname(ip)
43+ dns_zone_config = DNSForwardZoneConfig(
44+ zone, networks=[network],
45+ dns_ip=factory.getRandomIPInNetwork(network),
46+ mapping={generated_name: ip})
47+ self.assertNotIn(
48+ generated_name,
49+ dict(dns_zone_config.get_cname_mapping()))
50+
51 def test_get_static_mapping(self):
52 name = factory.getRandomString()
53 network = IPNetwork('192.12.0.1/30')