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

[Mike Pontillo]

Merge revision 5169 from trunk.

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

[Mike Pontillo (mpontillo)]

Self-reviewed backport.

review: Approve

=== modified file 'docs/changelog.rst'
--- docs/changelog.rst	2016-07-15 02:22:48 +0000
+++ docs/changelog.rst	2016-07-15 15:53:19 +0000
@@ -15,7 +15,9 @@
 
 LP: #1603147    Commissioning dropdown is grey and checkmarks are missing.
 
-LP: #1576116    MAAS adds regions controller as DNS resolver
+LP: #1576116    [2.0rc1] MAAS does not respect default subnet's DNS server when choosing default DNS
+
+LP: #1600720    [2.0rc1] MAAS doesn't honor DNS settings for a subnet for DHCP
 
 
 2.0.0 (rc2)
 
=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2016-04-13 13:18:40 +0000
+++ src/maasserver/dhcp.py	2016-07-15 15:53:19 +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-05-12 19:07:37 +0000
+++ src/maasserver/tests/test_dhcp.py	2016-07-15 15:53:19 +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)