Merge lp:~mpontillo/maas/subnet-dhcp-dns-settings--bug-1600720 into lp:maas/trunk

Proposed by Mike Pontillo on 2016-07-11
Status: Merged
Approved by: Mike Pontillo on 2016-07-12
Approved revision: 5171
Merged at revision: 5169
Proposed branch: lp:~mpontillo/maas/subnet-dhcp-dns-settings--bug-1600720
Merge into: lp:maas/trunk
Diff against target: 332 lines (+170/-62)
2 files modified
src/maasserver/dhcp.py (+59/-32)
src/maasserver/tests/test_dhcp.py (+111/-30)
To merge this branch: bzr merge lp:~mpontillo/maas/subnet-dhcp-dns-settings--bug-1600720
Reviewer Review Type Date Requested Status
Gavin Panella (community) 2016-07-11 Approve on 2016-07-12
Review via email: mp+299754@code.launchpad.net

Commit message

Fix DHCP configuration to properly override subnet DNS with user-configured DNS. (This makes MAAS DHCP consistent with what curtin does.)

Drive-by fix to log an error when DHCP is enabled for a particular address family, but no subnet can be found. (Previous behavior was to silently fail to write the configuration; now if IPv6 is misconfigured, it will not prevent the IPv4 configuration from being written, and vice versa.)

To post a comment you must log in.
5168. By Mike Pontillo on 2016-07-12

Fix lint.

Gavin Panella (allenap) wrote :

Several comments, but the only one I'm really concerned about is the "may be irrelevant" one. With my limited context it seems to me that it ought to be more precise.

review: Needs Information
Mike Pontillo (mpontillo) wrote :

Thanks for taking a look. Some replies below.

Gavin Panella (allenap) wrote :

Thanks for the explanation; it all makes sense now.

review: Approve
5169. By Mike Pontillo on 2016-07-12

Fix review comments.

5170. By Mike Pontillo on 2016-07-12

Clean up test cases related to logging.

5171. By Mike Pontillo on 2016-07-12

Fix lint.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/dhcp.py'
2--- src/maasserver/dhcp.py 2016-04-13 13:18:40 +0000
3+++ src/maasserver/dhcp.py 2016-07-12 20:50:38 +0000
4@@ -316,10 +316,17 @@
5
6
7 def make_subnet_config(
8- rack_controller, subnet, dns_servers, ntp_server, default_domain,
9+ rack_controller, subnet, maas_dns_server, ntp_server, default_domain,
10 failover_peer=None, subnets_dhcp_snippets=[]):
11 """Return DHCP subnet configuration dict for a rack interface."""
12 ip_network = subnet.get_ipnetwork()
13+ if subnet.dns_servers is not None and len(subnet.dns_servers) > 0:
14+ # Replace MAAS DNS with the servers defined on the subnet.
15+ dns_servers = ", ".join(subnet.dns_servers)
16+ elif maas_dns_server is not None and len(maas_dns_server) > 0:
17+ dns_servers = maas_dns_server
18+ else:
19+ dns_servers = ""
20 return {
21 'subnet': str(ip_network.network),
22 'subnet_mask': str(ip_network.netmask),
23@@ -368,12 +375,10 @@
24 from maasserver.dns.zonegenerator import get_dns_server_address
25
26 try:
27- dns_servers = get_dns_server_address(
28+ maas_dns_server = get_dns_server_address(
29 rack_controller, ipv4=(ip_version == 4), ipv6=(ip_version == 6))
30 except UnresolvableHost:
31- # No IPv6 DNS server addresses found. As a space-separated string,
32- # that becomes the empty string.
33- dns_servers = ''
34+ maas_dns_server = None
35
36 # Select the best interface for this VLAN. This is an interface that
37 # at least has an IP address.
38@@ -382,9 +387,10 @@
39 interface = get_best_interface(interfaces)
40 if interface is None:
41 raise DHCPConfigurationError(
42- "No interface on rack controller '%s' has an IP address on any "
43- "subnet on VLAN '%s.%d'." % (
44- rack_controller.hostname, vlan.fabric.name, vlan.vid))
45+ "No IPv%d interface on rack controller '%s' has an IP address on "
46+ "any subnet on VLAN '%s.%d'." % (
47+ ip_version, rack_controller.hostname, vlan.fabric.name,
48+ vlan.vid))
49
50 # Generate the failover peer for this VLAN.
51 if vlan.secondary_rack_id is not None:
52@@ -406,7 +412,7 @@
53 for subnet in subnets:
54 subnet_configs.append(
55 make_subnet_config(
56- rack_controller, subnet, dns_servers, ntp_server,
57+ rack_controller, subnet, maas_dns_server, ntp_server,
58 domain, peer_name, subnets_dhcp_snippets))
59
60 # Generate the hosts for all subnets.
61@@ -470,31 +476,52 @@
62 for vlan, (subnets_v4, subnets_v6) in vlan_subnets.items():
63 # IPv4
64 if len(subnets_v4) > 0:
65- failover_peer, subnets, hosts, interface = get_dhcp_configure_for(
66- 4, rack_controller, vlan, subnets_v4,
67- ntp_server, default_domain, dhcp_snippets)
68- if failover_peer is not None:
69- failover_peers_v4.append(failover_peer)
70- shared_networks_v4.append({
71- "name": "vlan-%d" % vlan.id,
72- "subnets": subnets,
73- })
74- hosts_v4.extend(hosts)
75- interfaces_v4.add(interface)
76-
77+ try:
78+ config = get_dhcp_configure_for(
79+ 4, rack_controller, vlan, subnets_v4, ntp_server,
80+ default_domain, dhcp_snippets)
81+ except DHCPConfigurationError as e:
82+ # XXX bug #1602412: this silently breaks DHCPv4, but we cannot
83+ # allow it to crash here since DHCPv6 might be able to run.
84+ # This error may be irrelevant if there is an IPv4 network in
85+ # the MAAS model which is not configured on the rack, and the
86+ # user only wants to serve DHCPv6. But it is still something
87+ # worth noting, so log it and continue.
88+ log.err(e)
89+ else:
90+ failover_peer, subnets, hosts, interface = config
91+ if failover_peer is not None:
92+ failover_peers_v4.append(failover_peer)
93+ shared_networks_v4.append({
94+ "name": "vlan-%d" % vlan.id,
95+ "subnets": subnets,
96+ })
97+ hosts_v4.extend(hosts)
98+ interfaces_v4.add(interface)
99 # IPv6
100 if len(subnets_v6) > 0:
101- failover_peer, subnets, hosts, interface = get_dhcp_configure_for(
102- 6, rack_controller, vlan, subnets_v6,
103- ntp_server, default_domain, dhcp_snippets)
104- if failover_peer is not None:
105- failover_peers_v6.append(failover_peer)
106- shared_networks_v6.append({
107- "name": "vlan-%d" % vlan.id,
108- "subnets": subnets,
109- })
110- hosts_v6.extend(hosts)
111- interfaces_v6.add(interface)
112+ try:
113+ config = get_dhcp_configure_for(
114+ 6, rack_controller, vlan, subnets_v6,
115+ ntp_server, default_domain, dhcp_snippets)
116+ except DHCPConfigurationError as e:
117+ # XXX bug #1602412: this silently breaks DHCPv6, but we cannot
118+ # allow it to crash here since DHCPv4 might be able to run.
119+ # This error may be irrelevant if there is an IPv6 network in
120+ # the MAAS model which is not configured on the rack, and the
121+ # user only wants to serve DHCPv4. But it is still something
122+ # worth noting, so log it and continue.
123+ log.err(e)
124+ else:
125+ failover_peer, subnets, hosts, interface = config
126+ if failover_peer is not None:
127+ failover_peers_v6.append(failover_peer)
128+ shared_networks_v6.append({
129+ "name": "vlan-%d" % vlan.id,
130+ "subnets": subnets,
131+ })
132+ hosts_v6.extend(hosts)
133+ interfaces_v6.add(interface)
134 return (
135 get_omapi_key(),
136 failover_peers_v4, shared_networks_v4, hosts_v4, interfaces_v4,
137
138=== modified file 'src/maasserver/tests/test_dhcp.py'
139--- src/maasserver/tests/test_dhcp.py 2016-06-21 10:29:11 +0000
140+++ src/maasserver/tests/test_dhcp.py 2016-07-12 20:50:38 +0000
141@@ -45,6 +45,7 @@
142 from maastesting.twisted import (
143 always_fail_with,
144 always_succeed_with,
145+ TwistedLoggerFixture,
146 )
147 from netaddr import (
148 IPAddress,
149@@ -627,23 +628,78 @@
150 'dhcp_snippets',
151 ]))
152
153- def test__sets_dns_and_ntp_from_arguments(self):
154- rack_controller = factory.make_RackController(interface=False)
155- vlan = factory.make_VLAN()
156- subnet = factory.make_Subnet(vlan=vlan)
157- factory.make_Interface(
158- INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
159- dns = '%s %s' % (
160- factory.make_ipv4_address(),
161- factory.make_ipv6_address(),
162- )
163- ntp = factory.make_name('ntp')
164- default_domain = Domain.objects.get_default_domain()
165- config = dhcp.make_subnet_config(
166- rack_controller, subnet, dns, ntp, default_domain)
167- self.expectThat(config['dns_servers'], Equals(dns))
168+ def test__sets_ipv4_dns_from_arguments(self):
169+ rack_controller = factory.make_RackController(interface=False)
170+ vlan = factory.make_VLAN()
171+ subnet = factory.make_Subnet(vlan=vlan, dns_servers=[], version=4)
172+ factory.make_Interface(
173+ INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
174+ maas_dns = factory.make_ipv4_address()
175+ ntp = factory.make_name('ntp')
176+ default_domain = Domain.objects.get_default_domain()
177+ config = dhcp.make_subnet_config(
178+ rack_controller, subnet, maas_dns, ntp, default_domain)
179+ self.expectThat(config['dns_servers'], Equals(maas_dns))
180+
181+ def test__sets_ipv6_dns_from_arguments(self):
182+ rack_controller = factory.make_RackController(interface=False)
183+ vlan = factory.make_VLAN()
184+ subnet = factory.make_Subnet(vlan=vlan, dns_servers=[], version=6)
185+ factory.make_Interface(
186+ INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
187+ maas_dns = factory.make_ipv6_address()
188+ ntp = factory.make_name('ntp')
189+ default_domain = Domain.objects.get_default_domain()
190+ config = dhcp.make_subnet_config(
191+ rack_controller, subnet, maas_dns, ntp, default_domain)
192+ self.expectThat(config['dns_servers'], Equals(maas_dns))
193+
194+ def test__sets_ntp_from_arguments(self):
195+ rack_controller = factory.make_RackController(interface=False)
196+ vlan = factory.make_VLAN()
197+ subnet = factory.make_Subnet(vlan=vlan, dns_servers=[])
198+ factory.make_Interface(
199+ INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
200+ ntp = factory.make_name('ntp')
201+ default_domain = Domain.objects.get_default_domain()
202+ config = dhcp.make_subnet_config(
203+ rack_controller, subnet, "", ntp, default_domain)
204 self.expectThat(config['ntp_server'], Equals(ntp))
205
206+ def test__overrides_ipv4_dns_from_subnet(self):
207+ rack_controller = factory.make_RackController(interface=False)
208+ vlan = factory.make_VLAN()
209+ subnet = factory.make_Subnet(vlan=vlan, version=4)
210+ maas_dns = factory.make_ipv4_address()
211+ subnet_dns_servers = ["8.8.8.8", "8.8.4.4"]
212+ subnet.dns_servers = subnet_dns_servers
213+ subnet.save()
214+ factory.make_Interface(
215+ INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
216+ ntp = factory.make_name('ntp')
217+ default_domain = Domain.objects.get_default_domain()
218+ config = dhcp.make_subnet_config(
219+ rack_controller, subnet, maas_dns, ntp, default_domain)
220+ self.expectThat(
221+ config['dns_servers'], Equals(", ".join(subnet_dns_servers)))
222+
223+ def test__overrides_ipv6_dns_from_subnet(self):
224+ rack_controller = factory.make_RackController(interface=False)
225+ vlan = factory.make_VLAN()
226+ subnet = factory.make_Subnet(vlan=vlan, version=6)
227+ maas_dns = factory.make_ipv6_address()
228+ subnet_dns_servers = ["2001:db8::1", "2001:db8::2"]
229+ subnet.dns_servers = subnet_dns_servers
230+ subnet.save()
231+ factory.make_Interface(
232+ INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
233+ ntp = factory.make_name('ntp')
234+ default_domain = Domain.objects.get_default_domain()
235+ config = dhcp.make_subnet_config(
236+ rack_controller, subnet, maas_dns, ntp, default_domain)
237+ self.expectThat(
238+ config['dns_servers'], Equals(", ".join(subnet_dns_servers)))
239+
240 def test__sets_domain_name_from_passed_domain(self):
241 rack_controller = factory.make_RackController(interface=False)
242 vlan = factory.make_VLAN()
243@@ -1121,7 +1177,8 @@
244 ha_vlan = factory.make_VLAN(
245 dhcp_on=True, primary_rack=primary_rack,
246 secondary_rack=secondary_rack)
247- ha_subnet = factory.make_ipv4_Subnet_with_IPRanges(vlan=ha_vlan)
248+ ha_subnet = factory.make_ipv4_Subnet_with_IPRanges(
249+ vlan=ha_vlan, dns_servers=['127.0.0.1'])
250 ha_network = ha_subnet.get_ipnetwork()
251 ha_dhcp_snippets = [
252 factory.make_DHCPSnippet(subnet=ha_subnet, enabled=True)
253@@ -1136,7 +1193,8 @@
254 secondary_ip = factory.make_StaticIPAddress(
255 alloc_type=IPADDRESS_TYPE.AUTO, subnet=ha_subnet,
256 interface=secondary_interface)
257- other_subnet = factory.make_ipv4_Subnet_with_IPRanges(vlan=ha_vlan)
258+ other_subnet = factory.make_ipv4_Subnet_with_IPRanges(
259+ vlan=ha_vlan, dns_servers=['127.0.0.1'])
260 other_network = other_subnet.get_ipnetwork()
261 other_dhcp_snippets = [
262 factory.make_DHCPSnippet(subnet=other_subnet, enabled=True)
263@@ -1358,7 +1416,8 @@
264 return cluster, cluster.ConfigureDHCPv4, cluster.ConfigureDHCPv6
265
266 @transactional
267- def create_rack_controller(self, dhcp_on=True):
268+ def create_rack_controller(
269+ self, dhcp_on=True, missing_ipv4=False, missing_ipv6=False):
270 """Create a `rack_controller` in a state that will call both
271 `ConfigureDHCPv4` and `ConfigureDHCPv6` with data."""
272 primary_rack = factory.make_RackController(interface=False)
273@@ -1382,18 +1441,20 @@
274 subnet_v6, "fd38:c341:27da:c831:0:1::",
275 "fd38:c341:27da:c831:0:1:ffff:0")
276
277- factory.make_StaticIPAddress(
278- alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
279- interface=primary_interface)
280- factory.make_StaticIPAddress(
281- alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
282- interface=secondary_interface)
283- factory.make_StaticIPAddress(
284- alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v6,
285- interface=primary_interface)
286- factory.make_StaticIPAddress(
287- alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v6,
288- interface=secondary_interface)
289+ if not missing_ipv4:
290+ factory.make_StaticIPAddress(
291+ alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
292+ interface=primary_interface)
293+ factory.make_StaticIPAddress(
294+ alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
295+ interface=secondary_interface)
296+ if not missing_ipv6:
297+ factory.make_StaticIPAddress(
298+ alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v6,
299+ interface=primary_interface)
300+ factory.make_StaticIPAddress(
301+ alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v6,
302+ interface=secondary_interface)
303
304 for _ in range(3):
305 factory.make_DHCPSnippet(subnet=subnet_v4, enabled=True)
306@@ -1448,6 +1509,26 @@
307
308 @wait_for_reactor
309 @inlineCallbacks
310+ def test__logs_DHCPConfigurationError_ipv4(self):
311+ self.patch(dhcp.settings, "DHCP_CONNECT", True)
312+ with TwistedLoggerFixture() as logger:
313+ yield deferToDatabase(
314+ self.create_rack_controller, missing_ipv4=True)
315+ self.assertDocTestMatches(
316+ "...No IPv4 interface...", logger.output)
317+
318+ @wait_for_reactor
319+ @inlineCallbacks
320+ def test__logs_DHCPConfigurationError_ipv6(self):
321+ self.patch(dhcp.settings, "DHCP_CONNECT", True)
322+ with TwistedLoggerFixture() as logger:
323+ yield deferToDatabase(
324+ self.create_rack_controller, missing_ipv6=True)
325+ self.assertDocTestMatches(
326+ "...No IPv6 interface...", logger.output)
327+
328+ @wait_for_reactor
329+ @inlineCallbacks
330 def test__doesnt_call_configure_for_both_ipv4_and_ipv6(self):
331 rack_controller, args = yield deferToDatabase(
332 self.create_rack_controller)