Merge ~ack/maas:1871584-subnet-no-dns-fix into maas:master

[Alberto Donato]

LP: #1871584 - include MAAS IP in DNS zone even if subnet has allow_dns false

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1871584-subnet-no-dns-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/7384/console
COMMIT: be5346aa4319a7204e72bb4327eee044091bcb85

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1871584-subnet-no-dns-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/7385/console
COMMIT: 1f635f1e3dfaf74f188b8a180d92a998c72886cb

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1871584-subnet-no-dns-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 9c194b55e3c937f7a62fe14c51e2ce5faaa15f8a

review: Approve

[Lee Trager (ltrager)]

This would break LP:1847537. Users need to have a way to completely disable MAAS DNS. Even if DNS is disabled they may want to continue to access MAAS by DNS via their own DNS servers.

Users should be warned that this will disable MAAS DNS but it still needs to be allowed.

review: Disapprove

[Alberto Donato (ack)]

Why would this change break it?

DNS IPs for subnets where allow_dns=False are still not included when a rack_controller is passed to the function, which is what happens when asking for the list of DNSs for a node.

The change only affects getting the list of entries to populate the DNS zone file.

[Lee Trager (ltrager)]

I think this would break MAAS in a different way then I was envisioning originally. preseed_network calls Node.get_default_dns_servers() to check if DNS servers should be listed in the generated Netplan.yaml. get_dns_server_addresses() is called by Node.get_default_dns_servers(), the Rack's passed come from Node.get_boot_primary_rack_controller() and Node.get_boot_secondary_rack_controller(). Both of those functions return None when DHCP is disabled on the VLAN.

With this patch if a user deploys on a network with DHCP disabled on the boot interface(using a static IP configured in the firmware) and allow_dns is set to True no DNS servers will be added even though it was requested.

[Alberto Donato (ack)]

I see, I didn't know that was the case.

I've updated the change to add an explicit flag to toggle filtering out on networks with allow_dns=False, which is on by default and switched off when generating DNS zones.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1871584-subnet-no-dns-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/7398/console
COMMIT: 212b7bd00a0b9b184e3d960e6f37034ca89b7496

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1871584-subnet-no-dns-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 619bf924d3ac295159817e13a45604758a304a14

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1871584-subnet-no-dns-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 3cab34e0e47d0d1a7ce201847ee5d8e6e4ef0835

review: Approve

[Lee Trager (ltrager)]

LGTM!

review: Approve

diff --git a/src/maasserver/dns/tests/test_zonegenerator.py b/src/maasserver/dns/tests/test_zonegenerator.py
index 7499924..5655873 100644
--- a/src/maasserver/dns/tests/test_zonegenerator.py
+++ b/src/maasserver/dns/tests/test_zonegenerator.py
@@ -27,6 +27,7 @@ from maasserver.dns import zonegenerator
 from maasserver.dns.zonegenerator import (
     get_dns_search_paths,
     get_dns_server_address,
+    get_dns_server_addresses,
     get_hostname_dnsdata_mapping,
     get_hostname_ip_mapping,
     InternalDomain,
@@ -82,7 +83,7 @@ class TestGetDNSServerAddress(MAASServerTestCase):
         self.assertThat(
             patch,
             MockCalledOnceWith(
-                ANY,
+                rack_controller=None,
                 include_alternates=False,
                 ipv4=ipv4,
                 ipv6=ipv6,
@@ -135,6 +136,37 @@ class TestGetDNSServerAddress(MAASServerTestCase):
         self.assertIsNone(get_dns_server_address(rack_controller))
 
 
+class TestGetDNSServerAddresses(MAASServerTestCase):
+    def test_no_rack_all_subnets(self):
+        subnet1 = factory.make_Subnet(cidr="10.10.0.0/24", allow_dns=False)
+        subnet2 = factory.make_Subnet(cidr="10.20.0.0/24", allow_dns=True)
+        ip1 = factory.make_StaticIPAddress(subnet=subnet1)
+        ip2 = factory.make_StaticIPAddress(subnet=subnet2)
+        ips = {IPAddress(ip1.ip), IPAddress(ip2.ip)}
+        resolver = self.patch(server_address, "resolve_hostname")
+        resolver.return_value = ips
+        rack_controller = factory.make_RackController()
+        self.assertCountEqual(
+            get_dns_server_addresses(
+                rack_controller=rack_controller, filter_allowed_dns=False
+            ),
+            ips,
+        )
+
+    def test_with_rack_only_allow_dns(self):
+        subnet1 = factory.make_Subnet(cidr="10.10.0.0/24", allow_dns=False)
+        subnet2 = factory.make_Subnet(cidr="10.20.0.0/24", allow_dns=True)
+        ip1 = factory.make_StaticIPAddress(subnet=subnet1)
+        ip2 = factory.make_StaticIPAddress(subnet=subnet2)
+        resolver = self.patch(server_address, "resolve_hostname")
+        resolver.return_value = {IPAddress(ip1.ip), IPAddress(ip2.ip)}
+        rack_controller = factory.make_RackController()
+        self.assertCountEqual(
+            get_dns_server_addresses(rack_controller=rack_controller),
+            [IPAddress(ip2.ip)],
+        )
+
+
 class TestGetDNSSearchPaths(MAASServerTestCase):
     def test__returns_all_authoritative_domains(self):
         domain_names = get_dns_search_paths()
@@ -541,6 +573,24 @@ class TestZoneGenerator(MAASServerTestCase):
         )
         self.assertEqual({}, zones[2]._mapping)
 
+    def test_forward_zone_includes_subnets_with_allow_dns_false(self):
+        default_ttl = random.randint(10, 300)
+        Config.objects.set_config("default_dns_ttl", default_ttl)
+        default_domain = Domain.objects.get_default_domain()
+        subnet = factory.make_Subnet(cidr="10.10.0.0/24", allow_dns=False)
+        ip = factory.make_StaticIPAddress(subnet=subnet)
+        resolver = self.patch(server_address, "resolve_hostname")
+        resolver.return_value = {IPAddress(ip.ip)}
+        zones = ZoneGenerator(
+            [default_domain], subnet, serial=random.randint(0, 65535)
+        )
+        [forward_zone] = [
+            zone for zone in zones if isinstance(zone, DNSForwardZoneConfig)
+        ]
+        self.assertEqual(
+            forward_zone._other_mapping["@"].rrset, {(default_ttl, "A", ip.ip)}
+        )
+
     def rfc2317_network(self, network):
         """Returns the network that rfc2317 glue goes in, if any."""
         net = network
diff --git a/src/maasserver/dns/zonegenerator.py b/src/maasserver/dns/zonegenerator.py
index 140871d..cda8222 100644
--- a/src/maasserver/dns/zonegenerator.py
+++ b/src/maasserver/dns/zonegenerator.py
@@ -120,6 +120,7 @@ def get_dns_server_addresses(
     ipv6=True,
     include_alternates=False,
     default_region_ip=None,
+    filter_allowed_dns=True,
 ):
     """Return the DNS server's IP addresses.
 
@@ -136,13 +137,15 @@ def get_dns_server_addresses(
     :param include_alternates: Include IP addresses from other regions?
     :param default_region_ip: The default source IP address to be used, if a
         specific URL is not defined.
+    :param filter_allowed_dns: If true, only include addresses for subnets
+        with allow_dns=True.
     :return: List of IPAddress to use.  Loopback addresses are removed from the
         list, unless there are no non-loopback addresses.
 
     """
     try:
-        iplist = get_maas_facing_server_addresses(
-            rack_controller,
+        ips = get_maas_facing_server_addresses(
+            rack_controller=rack_controller,
             ipv4=ipv4,
             ipv6=ipv6,
             include_alternates=include_alternates,
@@ -157,22 +160,21 @@ def get_dns_server_addresses(
             "local_config_set --maas-url' command." % e.strerror
         )
 
-    # LP:1847537 - Filter out MAAS DNS servers running on subnets which do not
-    # allow DNS to be provided from MAAS.
-    filtered_list = [
-        ip
-        for ip in iplist
-        if getattr(
-            Subnet.objects.get_best_subnet_for_ip(ip), "allow_dns", True
-        )
-    ]
-    non_loop = [ip for ip in filtered_list if not ip.is_loopback()]
-    if len(non_loop) > 0:
+    if filter_allowed_dns:
+        ips = [
+            ip
+            for ip in ips
+            if getattr(
+                Subnet.objects.get_best_subnet_for_ip(ip), "allow_dns", True
+            )
+        ]
+    non_loop = [ip for ip in ips if not ip.is_loopback()]
+    if non_loop:
         return non_loop
     else:
-        for ip in filtered_list:
+        for ip in ips:
             warn_loopback(ip)
-        return filtered_list
+        return ips
 
 
 def get_dns_search_paths():
@@ -237,7 +239,7 @@ class ZoneGenerator:
         internal_domains,
     ):
         """Generator of forward zones, collated by domain name."""
-        dns_ip_list = get_dns_server_addresses()
+        dns_ip_list = get_dns_server_addresses(filter_allowed_dns=False)
         domains = set(domains)
 
         # For each of the domains that we are generating, create the zone from: