Merge ~ltrager/maas:lp1881133 into maas:master

[Lee Trager]

LP: #1881133 - Always include rack controller IPs for DNS servers.

MAAS was only including the configured URL for controllers which did not
always include all rack controllers the VLAN is configured to use. This
would also sometimes include the region controller's IP even when the
machine is unable to directly access the rack controller.

Snap of this branch https://private-fileshare.canonical.com/~ltrager/maas_2.9.0~alpha1-8767-g.274991f9d_amd64.snap

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: e73e635eb3fd03943cd400d6d36542b627168395

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8029/console
COMMIT: e58fb6df87b8f2244b95803a58a9211c62c5fa9b

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8041/console
COMMIT: 2a52feecdcc91988b569f65d0fdb90833449d6dd

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8042/console
COMMIT: 154abccbb8f6031c057580f0707bdc4629cde19c

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 913b9489646c07b8122355ed01343202c31b700b

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8077/console
COMMIT: 274991f9d990fbd081dd5504884ff3ccb9dae427

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8097/console
COMMIT: efd4d4470bfd3fadee6ce7be6c7978779d4fe185

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8122/console
COMMIT: 6942f03547c2f25e4a0477d50adc7a8bab08923a

review: Needs Fixing

1c15e20... by Lee Trager on 2020-08-13

adam-collard fixes

[Lee Trager (ltrager)]

Thanks for the review! Made changes as suggested, let me know if there is anything else.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8139/console
COMMIT: 1c15e2019ad33436a7db30495ea7608c11b6668a

review: Needs Fixing

[Adam Collard (adam-collard)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1881133 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 1c15e2019ad33436a7db30495ea7608c11b6668a

review: Approve

[Adam Collard (adam-collard)]

review: Approve

diff --git a/src/maasserver/dhcp.py b/src/maasserver/dhcp.py
index d9b9299..848ff1b 100644
--- a/src/maasserver/dhcp.py
+++ b/src/maasserver/dhcp.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """DHCP management module."""
@@ -558,8 +558,8 @@ def get_default_dns_servers(rack_controller, subnet, use_rack_proxy=True):
       or not.
     """
     ip_version = subnet.get_ip_version()
+    default_region_ip = get_source_address(subnet.get_ipnetwork())
     try:
-        default_region_ip = get_source_address(subnet.get_ipnetwork())
         dns_servers = get_dns_server_addresses(
             rack_controller,
             ipv4=(ip_version == 4),
@@ -570,16 +570,30 @@ def get_default_dns_servers(rack_controller, subnet, use_rack_proxy=True):
     except UnresolvableHost:
         dns_servers = None
 
+    if default_region_ip:
+        default_region_ip = IPAddress(default_region_ip)
     if use_rack_proxy:
         # Add the IP address for the rack controllers on the subnet before the
         # region DNS servers.
         rack_ips = get_dns_server_addresses_for_rack(rack_controller, subnet)
         if dns_servers:
             dns_servers = rack_ips + [
-                server for server in dns_servers if server not in rack_ips
+                server
+                for server in dns_servers
+                if server not in rack_ips and server != default_region_ip
             ]
         elif rack_ips:
             dns_servers = rack_ips
+    elif default_region_ip in dns_servers:
+        # Make sure the region DNS server comes last
+        dns_servers = [
+            server for server in dns_servers if server != default_region_ip
+        ] + [default_region_ip]
+    # If no DNS servers were found give the region IP. This won't go through
+    # the rack but its better than nothing.
+    if not dns_servers:
+        log.warn("No DNS servers found, DHCP defaulting to region IP.")
+        dns_servers = [default_region_ip]
 
     return dns_servers
 
diff --git a/src/maasserver/preseed_network.py b/src/maasserver/preseed_network.py
index f8a782f..11dc858 100644
--- a/src/maasserver/preseed_network.py
+++ b/src/maasserver/preseed_network.py
@@ -8,13 +8,10 @@ __all__ = []
 from collections import defaultdict, OrderedDict
 from operator import attrgetter
 
-from netaddr import IPNetwork
+from netaddr import IPAddress, IPNetwork
 import yaml
 
-from maasserver.dns.zonegenerator import (
-    get_dns_search_paths,
-    get_dns_server_addresses,
-)
+from maasserver.dns.zonegenerator import get_dns_search_paths
 from maasserver.enum import (
     BRIDGE_TYPE,
     INTERFACE_TYPE,
@@ -22,7 +19,7 @@ from maasserver.enum import (
     IPADDRESS_TYPE,
     NODE_STATUS,
 )
-from maasserver.models import Interface
+from maasserver.models import Interface, StaticIPAddress
 from maasserver.models.staticroute import StaticRoute
 from provisioningserver.utils.netplan import (
     get_netplan_bond_parameters,
@@ -362,40 +359,42 @@ class InterfaceConfiguration:
                         v1_subnet_operation if version == 1 else v2_config,
                         version,
                     )
-                    if (
-                        subnet.dns_servers is not None
-                        and len(subnet.dns_servers) > 0
-                    ):
+
+                    if "dns_nameservers" not in v1_subnet_operation:
                         v1_subnet_operation["dns_nameservers"] = []
                         v1_subnet_operation[
                             "dns_search"
                         ] = self.node_config.default_search_list
-                        if "nameservers" not in v2_config:
-                            v2_config["nameservers"] = v2_nameservers
-                            v2_config["nameservers"][
-                                "search"
-                            ] = self.node_config.default_search_list
-                            if "addresses" not in v2_nameservers:
-                                v2_nameservers["addresses"] = []
-                        for rack in {
+                    if "nameservers" not in v2_config:
+                        v2_nameservers[
+                            "search"
+                        ] = self.node_config.default_search_list
+                        v2_config["nameservers"] = v2_nameservers
+                        if "addresses" not in v2_nameservers:
+                            v2_nameservers["addresses"] = []
+
+                    for ip in StaticIPAddress.objects.filter(
+                        interface__node__in=[
                             subnet.vlan.primary_rack,
                             subnet.vlan.secondary_rack,
-                        }:
-                            if rack is None:
-                                continue
-                            for ip in get_dns_server_addresses(
-                                rack_controller=rack,
-                                ipv4=(subnet.get_ip_version() == 4),
-                                ipv6=(subnet.get_ip_version() == 6),
-                                include_alternates=True,
-                            ):
-                                if ip.is_loopback():
-                                    continue
-                                ip_str = str(ip)
-                                v1_subnet_operation["dns_nameservers"].append(
-                                    ip_str
-                                )
-                                v2_nameservers["addresses"].append(ip_str)
+                        ],
+                        subnet__vlan=subnet.vlan,
+                        alloc_type__in=[
+                            IPADDRESS_TYPE.AUTO,
+                            IPADDRESS_TYPE.STICKY,
+                        ],
+                    ).exclude(ip=None):
+                        if (
+                            IPAddress(ip.get_ip()).version
+                            == subnet.get_ip_version()
+                            and ip.get_ip() not in v2_nameservers["addresses"]
+                        ):
+                            v1_subnet_operation["dns_nameservers"].append(
+                                ip.ip
+                            )
+                            v2_nameservers["addresses"].append(ip.ip)
+
+                    if subnet.dns_servers:
                         v1_subnet_operation[
                             "dns_nameservers"
                         ] += subnet.dns_servers
@@ -408,6 +407,13 @@ class InterfaceConfiguration:
                             matching_subnet_routes, version=version
                         )
                         v1_subnet_operation["routes"] = routes
+
+                    # Delete if no DNS servers were added.
+                    if len(v1_subnet_operation["dns_nameservers"]) == 0:
+                        del v1_subnet_operation["dns_nameservers"]
+                        del v1_subnet_operation["dns_search"]
+                    if len(v2_nameservers["addresses"]) == 0:
+                        del v2_config["nameservers"]
             if dhcp_type:
                 v1_config.append({"type": dhcp_type})
                 if dhcp_type == "dhcp":
diff --git a/src/maasserver/tests/test_dhcp.py b/src/maasserver/tests/test_dhcp.py
index 6894dd3..d53f8bb 100644
--- a/src/maasserver/tests/test_dhcp.py
+++ b/src/maasserver/tests/test_dhcp.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for DHCP management."""
@@ -1089,7 +1089,7 @@ class TestGetDefaultDNSServers(MAASServerTestCase):
         )
         servers = get_default_dns_servers(r1, subnet, False)
         self.assertThat(
-            servers, Equals([IPAddress("10.0.0.1"), IPAddress(address.ip)])
+            servers, Equals([IPAddress(address.ip), IPAddress("10.0.0.1")])
         )
 
     def test_racks_on_subnet_comes_before_region(self):
@@ -1117,18 +1117,32 @@ class TestGetDefaultDNSServers(MAASServerTestCase):
             subnet=subnet,
             alloc_type=IPADDRESS_TYPE.STICKY,
         )
-        servers = get_default_dns_servers(r1, subnet)
-        self.assertThat(
-            servers,
-            Equals(
-                [
-                    IPAddress(r1_address.ip),
-                    IPAddress(r2_address.ip),
-                    IPAddress("10.0.0.1"),
-                ]
-            ),
+        servers = get_default_dns_servers(r1, subnet, False)
+        self.assertItemsEqual(
+            servers[0:-1], [IPAddress(r1_address.ip), IPAddress(r2_address.ip)]
+        )
+        self.assertEquals(IPAddress("10.0.0.1"), servers[-1])
+
+    def test_doesnt_include_remote_region_ip(self):
+        # Regression test for LP:1881133
+        mock_get_source_address = self.patch(dhcp, "get_source_address")
+        mock_get_source_address.return_value = "192.168.122.209"
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=vlan, cidr="192.168.200.0/24")
+        rack = factory.make_RackController(interface=False)
+        iface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack
+        )
+        factory.make_StaticIPAddress(
+            interface=iface,
+            subnet=subnet,
+            alloc_type=IPADDRESS_TYPE.STICKY,
+            ip="192.168.200.1",
         )
 
+        servers = get_default_dns_servers(rack, subnet)
+        self.assertThat(servers, Equals([IPAddress("192.168.200.1")]))
+
 
 class TestMakeSubnetConfig(MAASServerTestCase):
     """Tests for `make_subnet_config`."""
diff --git a/src/maasserver/tests/test_preseed_network.py b/src/maasserver/tests/test_preseed_network.py
index bc49c79..c19283a 100644
--- a/src/maasserver/tests/test_preseed_network.py
+++ b/src/maasserver/tests/test_preseed_network.py
@@ -1739,6 +1739,28 @@ class TestNetplan(MAASServerTestCase):
         }
         self.expectThat(v1, Equals(expected_v1))
 
+    def test_dns_includes_rack_controllers(self):
+        # Regression test for LP:1881133
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(dns_servers=[], vlan=vlan)
+        rack = factory.make_RackController(subnet=subnet)
+        rack_iface = rack.interface_set.first()
+        rack_ip = factory.make_StaticIPAddress(
+            subnet=subnet, interface=rack_iface
+        )
+        vlan.primary_rack = rack
+        vlan.save()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            status=NODE_STATUS.DEPLOYING, subnet=subnet
+        )
+        iface = node.interface_set.first()
+        factory.make_StaticIPAddress(subnet=subnet, interface=iface)
+        v2 = self._render_netplan_dict(node)
+        self.assertDictEqual(
+            {"search": ["maas"], "addresses": [rack_ip.ip]},
+            v2["network"]["ethernets"][iface.name]["nameservers"],
+        )
+
     def test_commissioning_dhcp_config(self):
         # Verifies dhcp config is given when commissioning has run
         # or just run and no AUTOIP has been acquired.