Merge ~ddstreet/maas:lp1896684 into maas:master

[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 lp:~ddstreet/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8418/console
COMMIT: 69acdb0067cf0a7159a34678f824f59651750e5b

review: Needs Fixing

[Dan Streetman (ddstreet)]

> STATUS: FAILED
> LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8418/console

I'm not able to access this CI test system to see the logs.

[MAAS Lander (maas-lander)]

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

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8419/console
COMMIT: 99132e5871c0a99448db0db8ad59cb612c7f1b2b

review: Needs Fixing

[MAAS Lander (maas-lander)]

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

STATUS: SUCCESS
COMMIT: fdc4a764ddd43050b749df10a2a749437146ba3c

review: Approve

[Lee Trager (ltrager)]

Thanks for finding and fixing this!

This seems to fix the bug for static IP address, have you tested DHCP? Looking at the code[1] I think its correct but it would be good to verify. Couple of other things

1. This needs commit message to land. It should start with "LP: #1896684 - "
2. Unit tests need to be added to ensure we don't regress this.
3. One comment inline below.

[1] https://git.launchpad.net/maas/tree/src/maasserver/dhcp.py#n420

review: Needs Fixing

bae3958... by Dan Streetman on 2020-09-24

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.

[Dan Streetman (ddstreet)]

> Thanks for finding and fixing this!
>
> This seems to fix the bug for static IP address, have you tested DHCP? Looking
> at the code[1] I think its correct but it would be good to verify.

It does look like it needed a change, i included that in the latest patch

> Couple of
> other things
>
> 1. This needs commit message to land. It should start with "LP: #1896684 - "

ok, done

> 2. Unit tests need to be added to ensure we don't regress this.

if these changes look ok to you, i'll work on adding unit tests for them

> 3. One comment inline below.

response inline (with the old diff) - basically i'm confused as to why there's a in-vlan restriction but no in-subnet restriction, without a gateway (or defined route) a node can't access a separate subnet, even if it's in the same vlan. and with a gateway (or defined route), there's no need for the separate subnet to be in the same vlan.

>
>
> [1] https://git.launchpad.net/maas/tree/src/maasserver/dhcp.py#n420

[Dan Streetman (ddstreet)]

review: Needs Resubmitting

[MAAS Lander (maas-lander)]

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

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

review: Needs Fixing

[Lee Trager (ltrager)]

That looks good but this still needs unit tests. Also LaunchPad requires the commit message be added on LaunchPad, it doesn't automatically take whats in git.

[MAAS Lander (maas-lander)]

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

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8438/console
COMMIT: 692f3ca6b93e20fcb26c5c9f4e083b3afe86bd19

review: Needs Fixing

[MAAS Lander (maas-lander)]

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

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8440/console
COMMIT: 44003e8d41e36812a7b4b0df45650e494c809f69

review: Needs Fixing

b60e8e9... by Dan Streetman on 2020-09-30

add unit tests

[Dan Streetman (ddstreet)]

updated commits, and added test cases...let's see if it passes jenkins

review: Needs Resubmitting

[MAAS Lander (maas-lander)]

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

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

review: Needs Fixing

[MAAS Lander (maas-lander)]

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

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8451/console
COMMIT: 82b401ad22dd3298d69eb00a2934c2979e07af09

review: Needs Fixing

[MAAS Lander (maas-lander)]

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

STATUS: SUCCESS
COMMIT: 5e806d38e90f341cc86acb4bb89974b84c63124e

review: Approve

[MAAS Lander (maas-lander)]

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

STATUS: SUCCESS
COMMIT: b60e8e96657ffce7939970ad8a911042888114cc

review: Approve

[Lee Trager (ltrager)]

LGTM! Thanks for the fix!

review: Approve

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 d3e40c2..4882eec 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -4937,6 +4937,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(),
@@ -4963,6 +4965,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(),
@@ -5009,18 +5013,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 33339a7..b6ac6a4 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -8203,6 +8203,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 8188603..395f3b3 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 c19283a..279c203 100644
--- a/src/maasserver/tests/test_preseed_network.py
+++ b/src/maasserver/tests/test_preseed_network.py
@@ -1761,6 +1761,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.