Merge ~ddstreet/maas:lp1896684-28 into maas:2.8

[Dan Streetman]

LP: #1896684 - If no subnet gateway, only use in-subnet dns addresses

If DHCP is not providing any gateway, it should not provide any
DNS server addresses that aren't directly reachable.

Similarly, if the network preseed is not configuring any gateway,
it should not provide per-interface DNS server addresses, nor default
dns server addresses, that aren't directly reachable.

Only apply rack DNS server address if subnet.allow_dns == True

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1896684-28 lp:~ddstreet/maas/+git/maas into -b 2.8 lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 843b1c54a4c01853dbdf7ae37929c92483d410fd

review: Approve

[Lee Trager (ltrager)]

review: Approve

[MAAS Lander (maas-lander)]

UNABLE TO START LANDING

STATUS: MISSING COMMIT MESSAGE

diff --git a/src/maasserver/dhcp.py b/src/maasserver/dhcp.py
index 848ff1b..05e9f7a 100644
--- a/src/maasserver/dhcp.py
+++ b/src/maasserver/dhcp.py
@@ -438,7 +438,15 @@ def make_subnet_config(
     dns_servers = []
     if subnet.allow_dns and default_dns_servers:
         # If the MAAS DNS server is enabled make sure that is used first.
-        dns_servers += default_dns_servers
+        if subnet.gateway_ip:
+            dns_servers += default_dns_servers
+        else:
+            # if there is no gateway, only provide in-subnet dns servers
+            dns_servers += [
+                ipaddress
+                for ipaddress in default_dns_servers
+                if ipaddress in ip_network
+            ]
     if subnet.dns_servers:
         # Add DNS user defined DNS servers
         dns_servers += [IPAddress(server) for server in subnet.dns_servers]
@@ -557,6 +565,10 @@ def get_default_dns_servers(rack_controller, subnet, use_rack_proxy=True):
     :param use_rack_proxy: Whether to proxy DNS through the rack controller
       or not.
     """
+    if not subnet.allow_dns:
+        # This subnet isn't allowed to use region or rack addresses for dns
+        return []
+
     ip_version = subnet.get_ip_version()
     default_region_ip = get_source_address(subnet.get_ipnetwork())
     try:
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 7e867cd..1d371f4 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -4905,6 +4905,8 @@ class Node(CleanSave, TimestampedModel):
         if ipv4 and gateways.ipv4 is not None:
             subnet = Subnet.objects.get(id=gateways.ipv4.subnet_id)
             if subnet.dns_servers:
+                if not subnet.allow_dns:
+                    return subnet.dns_servers
                 rack_dns = []
                 for rack in {
                     self.get_boot_primary_rack_controller(),
@@ -4931,6 +4933,8 @@ class Node(CleanSave, TimestampedModel):
         if ipv6 and gateways.ipv6 is not None:
             subnet = Subnet.objects.get(id=gateways.ipv6.subnet_id)
             if subnet.dns_servers:
+                if not subnet.allow_dns:
+                    return subnet.dns_servers
                 rack_dns = []
                 for rack in {
                     self.get_boot_primary_rack_controller(),
@@ -4977,18 +4981,9 @@ class Node(CleanSave, TimestampedModel):
                 if filtered_addresses:
                     routable_addrs_map[node] = filtered_addresses
 
-        # No default gateway subnet has specific DNS servers defined, so
-        # use MAAS for the default DNS server.
         if gateways.ipv4 is None and gateways.ipv6 is None:
-            # If there are no default gateways, the default is the MAAS
-            # region IP address.
-            maas_dns_servers = get_dns_server_addresses(
-                rack_controller=self.get_boot_rack_controller(),
-                ipv4=ipv4,
-                ipv6=ipv6,
-                include_alternates=True,
-                default_region_ip=default_region_ip,
-            )
+            # node with no gateway can only use routable addrs
+            maas_dns_servers = []
             routable_addrs_map = {
                 node: [
                     address
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 3b9a6fb..6121686 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -8132,6 +8132,26 @@ class TestGetDefaultDNSServers(MAASServerTestCase):
             node.get_default_dns_servers(), [ipv6_subnet_dns]
         )
 
+    def test_ignores_other_unroutable_rack_controllers_ipv4(self):
+        # Regression test for LP:1896684
+        rack_v4, rack_v6, node = self.make_Node_with_RackController(
+            ipv4=True, ipv4_gateway=False, ipv6=False, ipv6_gateway=False
+        )
+        vlan = node.boot_interface.vlan
+        rack = vlan.primary_rack
+        rackif = rack.interface_set.first()
+        rack_ips = [rack_v4]
+        for _ in range(3):
+            subnet = factory.make_Subnet(vlan=vlan, version=4)
+            ip = factory.make_StaticIPAddress(subnet=subnet, interface=rackif)
+            rack_ips.append(ip.ip)
+        rack.save()
+        resolve_hostname = self.patch(server_address, "resolve_hostname")
+        resolve_hostname.side_effect = lambda hostname, version: set(
+            map(IPAddress, rack_ips)
+        )
+        self.assertItemsEqual(node.get_default_dns_servers(), [rack_v4])
+
 
 class TestNode_Start(MAASTransactionServerTestCase):
     """Tests for Node.start()."""
diff --git a/src/maasserver/preseed_network.py b/src/maasserver/preseed_network.py
index 779d1a6..99a3ce3 100644
--- a/src/maasserver/preseed_network.py
+++ b/src/maasserver/preseed_network.py
@@ -373,32 +373,37 @@ class InterfaceConfiguration:
                         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,
-                        ],
-                        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"]
-                        ):
+                    if subnet.allow_dns:
+                        for ip in StaticIPAddress.objects.filter(
+                            interface__node__in=[
+                                subnet.vlan.primary_rack,
+                                subnet.vlan.secondary_rack,
+                            ],
+                            subnet__vlan=subnet.vlan,
+                            alloc_type__in=[
+                                IPADDRESS_TYPE.AUTO,
+                                IPADDRESS_TYPE.STICKY,
+                            ],
+                        ).exclude(ip=None):
+                            if ip.ip in v2_nameservers["addresses"]:
+                                continue
+                            ip_address = IPAddress(ip.ip)
+                            if ip_address.version != subnet.get_ip_version():
+                                continue
+                            if not subnet.gateway_ip:
+                                if ip_address not in subnet.get_ipnetwork():
+                                    # without gateway, only use in-subnet addrs
+                                    continue
                             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
-                        v2_nameservers["addresses"] += subnet.dns_servers
+                    for ip in subnet.dns_servers:
+                        if ip in v2_nameservers["addresses"]:
+                            continue
+                        v1_subnet_operation["dns_nameservers"].append(ip)
+                        v2_nameservers["addresses"].append(ip)
 
                     if len(matching_subnet_routes) > 0 and version == 1:
                         # For the v1 YAML, the list of routes is rendered
diff --git a/src/maasserver/tests/test_dhcp.py b/src/maasserver/tests/test_dhcp.py
index d53f8bb..f445146 100644
--- a/src/maasserver/tests/test_dhcp.py
+++ b/src/maasserver/tests/test_dhcp.py
@@ -1158,7 +1158,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
             search_list=default_domain.name,
@@ -1457,7 +1457,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
         )
@@ -1474,7 +1474,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
         )
@@ -1495,7 +1495,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
         )
@@ -1518,7 +1518,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
             search_list=search_list,
@@ -1546,7 +1546,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv6_address()],
             [factory.make_name("ntp")],
             default_domain,
             search_list=search_list,
@@ -1581,7 +1581,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
         )
@@ -1609,7 +1609,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
             failover_peer=failover_peer,
@@ -1643,7 +1643,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
         )
@@ -1664,7 +1664,7 @@ class TestMakeSubnetConfig(MAASServerTestCase):
         config = dhcp.make_subnet_config(
             rack_controller,
             subnet,
-            [factory.make_name("dns")],
+            [factory.make_ipv4_address()],
             [factory.make_name("ntp")],
             default_domain,
             subnets_dhcp_snippets=dhcp_snippets,
@@ -1681,6 +1681,40 @@ class TestMakeSubnetConfig(MAASServerTestCase):
             config["dhcp_snippets"],
         )
 
+    def test_subnet_without_gateway_restricts_nameservers(self):
+        network1 = IPNetwork("10.9.8.0/24")
+        network2 = IPNetwork("10.9.9.0/24")
+        rackip1 = "10.9.8.1"
+        rackip2 = "10.9.9.1"
+        rack_controller = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN()
+        subnet1 = factory.make_Subnet(cidr=str(network1.cidr), vlan=vlan)
+        subnet2 = factory.make_Subnet(
+            cidr=str(network2.cidr), vlan=vlan, gateway_ip=None
+        )
+        factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller
+        )
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            rack_controller,
+            subnet1,
+            [rackip1, rackip2],
+            [factory.make_name("ntp")],
+            default_domain,
+        )
+        self.assertIn(rackip1, config["dns_servers"])
+        self.assertIn(rackip2, config["dns_servers"])
+        config = dhcp.make_subnet_config(
+            rack_controller,
+            subnet2,
+            [rackip1, rackip2],
+            [factory.make_name("ntp")],
+            default_domain,
+        )
+        self.assertNotIn(rackip1, config["dns_servers"])
+        self.assertIn(rackip2, config["dns_servers"])
+
 
 class TestMakeHostsForSubnet(MAASServerTestCase):
     def tests__returns_defined_hosts(self):
diff --git a/src/maasserver/tests/test_preseed_network.py b/src/maasserver/tests/test_preseed_network.py
index 9c3a326..5fa1dd5 100644
--- a/src/maasserver/tests/test_preseed_network.py
+++ b/src/maasserver/tests/test_preseed_network.py
@@ -1737,6 +1737,55 @@ class TestNetplan(MAASServerTestCase):
             v2["network"]["ethernets"][iface.name]["nameservers"],
         )
 
+    def test_allow_dns_false_does_not_include_rack_controllers(self):
+        # Regression test for LP:1896684
+        vlan = factory.make_VLAN()
+        nameserver = "1.1.1.1"
+        subnet = factory.make_Subnet(
+            dns_servers=[nameserver], vlan=vlan, allow_dns=False
+        )
+        rack = factory.make_RackController(subnet=subnet)
+        rack_iface = rack.interface_set.first()
+        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": [nameserver]},
+            v2["network"]["ethernets"][iface.name]["nameservers"],
+        )
+
+    def test_no_gateway_does_not_include_unroutable_controllers(self):
+        # Regression test for LP:1896684
+        vlan = factory.make_VLAN()
+        subnet1 = factory.make_Subnet(
+            dns_servers=[], vlan=vlan, version=4, gateway_ip=None
+        )
+        subnet2 = factory.make_Subnet(dns_servers=[], vlan=vlan, version=4)
+        rack = factory.make_RackController(subnet=subnet1)
+        rack_iface = rack.interface_set.first()
+        rack_ip1 = factory.make_StaticIPAddress(
+            subnet=subnet1, interface=rack_iface
+        )
+        factory.make_StaticIPAddress(subnet=subnet2, interface=rack_iface)
+        vlan.primary_rack = rack
+        vlan.save()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            status=NODE_STATUS.DEPLOYING, subnet=subnet1
+        )
+        iface = node.interface_set.first()
+        factory.make_StaticIPAddress(subnet=subnet1, interface=iface)
+        v2 = self._render_netplan_dict(node)
+        self.assertDictEqual(
+            {"search": ["maas"], "addresses": [rack_ip1.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.