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

[Mike Pontillo]

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.)

[Gavin Panella (allenap)]

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)]

Thanks for taking a look. Some replies below.

[Gavin Panella (allenap)]

Thanks for the explanation; it all makes sense now.

review: Approve

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2016-04-13 13:18:40 +0000
+++ src/maasserver/dhcp.py	2016-07-12 20:50:38 +0000
@@ -316,10 +316,17 @@
 
 
 def make_subnet_config(
-        rack_controller, subnet, dns_servers, ntp_server, default_domain,
+        rack_controller, subnet, maas_dns_server, ntp_server, default_domain,
         failover_peer=None, subnets_dhcp_snippets=[]):
     """Return DHCP subnet configuration dict for a rack interface."""
     ip_network = subnet.get_ipnetwork()
+    if subnet.dns_servers is not None and len(subnet.dns_servers) > 0:
+        # Replace MAAS DNS with the servers defined on the subnet.
+        dns_servers = ", ".join(subnet.dns_servers)
+    elif maas_dns_server is not None and len(maas_dns_server) > 0:
+        dns_servers = maas_dns_server
+    else:
+        dns_servers = ""
     return {
         'subnet': str(ip_network.network),
         'subnet_mask': str(ip_network.netmask),
@@ -368,12 +375,10 @@
     from maasserver.dns.zonegenerator import get_dns_server_address
 
     try:
-        dns_servers = get_dns_server_address(
+        maas_dns_server = get_dns_server_address(
             rack_controller, ipv4=(ip_version == 4), ipv6=(ip_version == 6))
     except UnresolvableHost:
-        # No IPv6 DNS server addresses found.  As a space-separated string,
-        # that becomes the empty string.
-        dns_servers = ''
+        maas_dns_server = None
 
     # Select the best interface for this VLAN. This is an interface that
     # at least has an IP address.
@@ -382,9 +387,10 @@
     interface = get_best_interface(interfaces)
     if interface is None:
         raise DHCPConfigurationError(
-            "No interface on rack controller '%s' has an IP address on any "
-            "subnet on VLAN '%s.%d'." % (
-                rack_controller.hostname, vlan.fabric.name, vlan.vid))
+            "No IPv%d interface on rack controller '%s' has an IP address on "
+            "any subnet on VLAN '%s.%d'." % (
+                ip_version, rack_controller.hostname, vlan.fabric.name,
+                vlan.vid))
 
     # Generate the failover peer for this VLAN.
     if vlan.secondary_rack_id is not None:
@@ -406,7 +412,7 @@
     for subnet in subnets:
         subnet_configs.append(
             make_subnet_config(
-                rack_controller, subnet, dns_servers, ntp_server,
+                rack_controller, subnet, maas_dns_server, ntp_server,
                 domain, peer_name, subnets_dhcp_snippets))
 
     # Generate the hosts for all subnets.
@@ -470,31 +476,52 @@
     for vlan, (subnets_v4, subnets_v6) in vlan_subnets.items():
         # IPv4
         if len(subnets_v4) > 0:
-            failover_peer, subnets, hosts, interface = get_dhcp_configure_for(
-                4, rack_controller, vlan, subnets_v4,
-                ntp_server, default_domain, dhcp_snippets)
-            if failover_peer is not None:
-                failover_peers_v4.append(failover_peer)
-            shared_networks_v4.append({
-                "name": "vlan-%d" % vlan.id,
-                "subnets": subnets,
-            })
-            hosts_v4.extend(hosts)
-            interfaces_v4.add(interface)
-
+            try:
+                config = get_dhcp_configure_for(
+                    4, rack_controller, vlan, subnets_v4, ntp_server,
+                    default_domain, dhcp_snippets)
+            except DHCPConfigurationError as e:
+                # XXX bug #1602412: this silently breaks DHCPv4, but we cannot
+                # allow it to crash here since DHCPv6 might be able to run.
+                # This error may be irrelevant if there is an IPv4 network in
+                # the MAAS model which is not configured on the rack, and the
+                # user only wants to serve DHCPv6. But it is still something
+                # worth noting, so log it and continue.
+                log.err(e)
+            else:
+                failover_peer, subnets, hosts, interface = config
+                if failover_peer is not None:
+                    failover_peers_v4.append(failover_peer)
+                shared_networks_v4.append({
+                    "name": "vlan-%d" % vlan.id,
+                    "subnets": subnets,
+                })
+                hosts_v4.extend(hosts)
+                interfaces_v4.add(interface)
         # IPv6
         if len(subnets_v6) > 0:
-            failover_peer, subnets, hosts, interface = get_dhcp_configure_for(
-                6, rack_controller, vlan, subnets_v6,
-                ntp_server, default_domain, dhcp_snippets)
-            if failover_peer is not None:
-                failover_peers_v6.append(failover_peer)
-            shared_networks_v6.append({
-                "name": "vlan-%d" % vlan.id,
-                "subnets": subnets,
-            })
-            hosts_v6.extend(hosts)
-            interfaces_v6.add(interface)
+            try:
+                config = get_dhcp_configure_for(
+                    6, rack_controller, vlan, subnets_v6,
+                    ntp_server, default_domain, dhcp_snippets)
+            except DHCPConfigurationError as e:
+                # XXX bug #1602412: this silently breaks DHCPv6, but we cannot
+                # allow it to crash here since DHCPv4 might be able to run.
+                # This error may be irrelevant if there is an IPv6 network in
+                # the MAAS model which is not configured on the rack, and the
+                # user only wants to serve DHCPv4. But it is still something
+                # worth noting, so log it and continue.
+                log.err(e)
+            else:
+                failover_peer, subnets, hosts, interface = config
+                if failover_peer is not None:
+                    failover_peers_v6.append(failover_peer)
+                shared_networks_v6.append({
+                    "name": "vlan-%d" % vlan.id,
+                    "subnets": subnets,
+                })
+                hosts_v6.extend(hosts)
+                interfaces_v6.add(interface)
     return (
         get_omapi_key(),
         failover_peers_v4, shared_networks_v4, hosts_v4, interfaces_v4,
 
=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2016-06-21 10:29:11 +0000
+++ src/maasserver/tests/test_dhcp.py	2016-07-12 20:50:38 +0000
@@ -45,6 +45,7 @@
 from maastesting.twisted import (
     always_fail_with,
     always_succeed_with,
+    TwistedLoggerFixture,
 )
 from netaddr import (
     IPAddress,
@@ -627,23 +628,78 @@
                 'dhcp_snippets',
                 ]))
 
-    def test__sets_dns_and_ntp_from_arguments(self):
-        rack_controller = factory.make_RackController(interface=False)
-        vlan = factory.make_VLAN()
-        subnet = factory.make_Subnet(vlan=vlan)
-        factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
-        dns = '%s %s' % (
-            factory.make_ipv4_address(),
-            factory.make_ipv6_address(),
-            )
-        ntp = factory.make_name('ntp')
-        default_domain = Domain.objects.get_default_domain()
-        config = dhcp.make_subnet_config(
-            rack_controller, subnet, dns, ntp, default_domain)
-        self.expectThat(config['dns_servers'], Equals(dns))
+    def test__sets_ipv4_dns_from_arguments(self):
+        rack_controller = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=vlan, dns_servers=[], version=4)
+        factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
+        maas_dns = factory.make_ipv4_address()
+        ntp = factory.make_name('ntp')
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            rack_controller, subnet, maas_dns, ntp, default_domain)
+        self.expectThat(config['dns_servers'], Equals(maas_dns))
+
+    def test__sets_ipv6_dns_from_arguments(self):
+        rack_controller = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=vlan, dns_servers=[], version=6)
+        factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
+        maas_dns = factory.make_ipv6_address()
+        ntp = factory.make_name('ntp')
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            rack_controller, subnet, maas_dns, ntp, default_domain)
+        self.expectThat(config['dns_servers'], Equals(maas_dns))
+
+    def test__sets_ntp_from_arguments(self):
+        rack_controller = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=vlan, dns_servers=[])
+        factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
+        ntp = factory.make_name('ntp')
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            rack_controller, subnet, "", ntp, default_domain)
         self.expectThat(config['ntp_server'], Equals(ntp))
 
+    def test__overrides_ipv4_dns_from_subnet(self):
+        rack_controller = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=vlan, version=4)
+        maas_dns = factory.make_ipv4_address()
+        subnet_dns_servers = ["8.8.8.8", "8.8.4.4"]
+        subnet.dns_servers = subnet_dns_servers
+        subnet.save()
+        factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
+        ntp = factory.make_name('ntp')
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            rack_controller, subnet, maas_dns, ntp, default_domain)
+        self.expectThat(
+            config['dns_servers'], Equals(", ".join(subnet_dns_servers)))
+
+    def test__overrides_ipv6_dns_from_subnet(self):
+        rack_controller = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=vlan, version=6)
+        maas_dns = factory.make_ipv6_address()
+        subnet_dns_servers = ["2001:db8::1", "2001:db8::2"]
+        subnet.dns_servers = subnet_dns_servers
+        subnet.save()
+        factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
+        ntp = factory.make_name('ntp')
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            rack_controller, subnet, maas_dns, ntp, default_domain)
+        self.expectThat(
+            config['dns_servers'], Equals(", ".join(subnet_dns_servers)))
+
     def test__sets_domain_name_from_passed_domain(self):
         rack_controller = factory.make_RackController(interface=False)
         vlan = factory.make_VLAN()
@@ -1121,7 +1177,8 @@
         ha_vlan = factory.make_VLAN(
             dhcp_on=True, primary_rack=primary_rack,
             secondary_rack=secondary_rack)
-        ha_subnet = factory.make_ipv4_Subnet_with_IPRanges(vlan=ha_vlan)
+        ha_subnet = factory.make_ipv4_Subnet_with_IPRanges(
+            vlan=ha_vlan, dns_servers=['127.0.0.1'])
         ha_network = ha_subnet.get_ipnetwork()
         ha_dhcp_snippets = [
             factory.make_DHCPSnippet(subnet=ha_subnet, enabled=True)
@@ -1136,7 +1193,8 @@
         secondary_ip = factory.make_StaticIPAddress(
             alloc_type=IPADDRESS_TYPE.AUTO, subnet=ha_subnet,
             interface=secondary_interface)
-        other_subnet = factory.make_ipv4_Subnet_with_IPRanges(vlan=ha_vlan)
+        other_subnet = factory.make_ipv4_Subnet_with_IPRanges(
+            vlan=ha_vlan, dns_servers=['127.0.0.1'])
         other_network = other_subnet.get_ipnetwork()
         other_dhcp_snippets = [
             factory.make_DHCPSnippet(subnet=other_subnet, enabled=True)
@@ -1358,7 +1416,8 @@
         return cluster, cluster.ConfigureDHCPv4, cluster.ConfigureDHCPv6
 
     @transactional
-    def create_rack_controller(self, dhcp_on=True):
+    def create_rack_controller(
+            self, dhcp_on=True, missing_ipv4=False, missing_ipv6=False):
         """Create a `rack_controller` in a state that will call both
         `ConfigureDHCPv4` and `ConfigureDHCPv6` with data."""
         primary_rack = factory.make_RackController(interface=False)
@@ -1382,18 +1441,20 @@
             subnet_v6, "fd38:c341:27da:c831:0:1::",
             "fd38:c341:27da:c831:0:1:ffff:0")
 
-        factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
-            interface=primary_interface)
-        factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
-            interface=secondary_interface)
-        factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v6,
-            interface=primary_interface)
-        factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v6,
-            interface=secondary_interface)
+        if not missing_ipv4:
+            factory.make_StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
+                interface=primary_interface)
+            factory.make_StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
+                interface=secondary_interface)
+        if not missing_ipv6:
+            factory.make_StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v6,
+                interface=primary_interface)
+            factory.make_StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v6,
+                interface=secondary_interface)
 
         for _ in range(3):
             factory.make_DHCPSnippet(subnet=subnet_v4, enabled=True)
@@ -1448,6 +1509,26 @@
 
     @wait_for_reactor
     @inlineCallbacks
+    def test__logs_DHCPConfigurationError_ipv4(self):
+        self.patch(dhcp.settings, "DHCP_CONNECT", True)
+        with TwistedLoggerFixture() as logger:
+            yield deferToDatabase(
+                self.create_rack_controller, missing_ipv4=True)
+            self.assertDocTestMatches(
+                "...No IPv4 interface...", logger.output)
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test__logs_DHCPConfigurationError_ipv6(self):
+        self.patch(dhcp.settings, "DHCP_CONNECT", True)
+        with TwistedLoggerFixture() as logger:
+            yield deferToDatabase(
+                self.create_rack_controller, missing_ipv6=True)
+            self.assertDocTestMatches(
+                "...No IPv6 interface...", logger.output)
+
+    @wait_for_reactor
+    @inlineCallbacks
     def test__doesnt_call_configure_for_both_ipv4_and_ipv6(self):
         rack_controller, args = yield deferToDatabase(
             self.create_rack_controller)