Merge ~cgrabowski/maas:per_zone_dns_forwarders into maas:master

[Christian Grabowski]

add forwarded_zones to config

fix form validation unit

add param comments

fix form validation

move forwarddnsserver into its own model to get around bad migration

add test case for no authoritative and forward servers

switch forward_dns_servers to IP type

add forward_dns_servers to domain form

add forward_dns_servers field to domain model

[MAAS Lander (maas-lander)]

UNIT TESTS
-b per_zone_dns_forwarders lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

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

review: Needs Fixing

3590a2c... by Christian Grabowski on 2021-06-16

format changes

[MAAS Lander (maas-lander)]

UNIT TESTS
-b per_zone_dns_forwarders lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10254/console
COMMIT: 3590a2ca641f2b3e303db4320c699cb836b49d5c

review: Needs Fixing

4f028a6... by Christian Grabowski on 2021-06-16

fix migration collisions

[MAAS Lander (maas-lander)]

UNIT TESTS
-b per_zone_dns_forwarders lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10256/console
COMMIT: 72f0180fee999864c6c9246baefa0d85fa1135b0

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b per_zone_dns_forwarders lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10257/console
COMMIT: 04ffac1779589494625eba967bdc3463c46bdd7e

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b per_zone_dns_forwarders lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10258/console
COMMIT: 606afce58303ed689286496c9e243801f3cbfba2

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b per_zone_dns_forwarders lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

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

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b per_zone_dns_forwarders lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 4f028a6f777f468ec4ad546068a552a492ed46e6

review: Approve

[Alexsander de Souza (alexsander-souza)]

review: Approve

diff --git a/src/maasserver/api/domains.py b/src/maasserver/api/domains.py
index 51417c6..a88fe88 100644
--- a/src/maasserver/api/domains.py
+++ b/src/maasserver/api/domains.py
@@ -62,6 +62,9 @@ class DomainsHandler(OperationsHandler):
         @param (string) "authoritative" [required=false] Class type of the
         domain.
 
+        @param (string) "forward_dns_servers" [required=false] List of forward dns
+        server IP addresses when MAAS is not authorititative.
+
         @success (http-status-code) "server-success" 200
         @success (json) "success-json" A JSON object containing the new domain
         object.
@@ -175,6 +178,9 @@ class DomainHandler(OperationsHandler):
 
         @param (string) "ttl" [required=false] The default TTL for this domain.
 
+        @param (string) "forward_dns_servers" [required=false] List of IP addresses for
+        forward DNS servers when MAAS is not authoritative for this domain.
+
         @success (http-status-code) "server-success" 200
         @success (json) "success-json" A JSON object containing
         information about the updated domain.
diff --git a/src/maasserver/dns/config.py b/src/maasserver/dns/config.py
index dedfb5f..38693a2 100644
--- a/src/maasserver/dns/config.py
+++ b/src/maasserver/dns/config.py
@@ -47,6 +47,20 @@ def dns_force_reload():
     DNSPublication(source="Force reload").save()
 
 
+def forward_domains_to_forwarded_zones(forward_domains):
+    # converted to a list of tuple to keep model within maasserver code
+    return [
+        (
+            domain.name,
+            [
+                fwd_dns_srvr.ip_address
+                for fwd_dns_srvr in domain.forward_dns_servers
+            ],
+        )
+        for domain in forward_domains
+    ]
+
+
 def dns_update_all_zones(reload_retry=False, reload_timeout=2):
     """Update all zone files for all domains.
 
@@ -61,6 +75,9 @@ def dns_update_all_zones(reload_retry=False, reload_timeout=2):
         return
 
     domains = Domain.objects.filter(authoritative=True)
+    forwarded_zones = forward_domains_to_forwarded_zones(
+        Domain.objects.get_forward_domains()
+    )
     subnets = Subnet.objects.exclude(rdns_mode=RDNS_MODE.DISABLED)
     default_ttl = Config.objects.get_config("default_dns_ttl")
     serial = current_zone_serial()
@@ -87,7 +104,11 @@ def dns_update_all_zones(reload_retry=False, reload_timeout=2):
     # recursive queries to the upstream DNS servers. Again, this is legacy,
     # where the "trusted" ACL ended up in the same configuration file as the
     # zone stanzas, and so both need to be rewritten at the same time.
-    bind_write_configuration(zones, trusted_networks=get_trusted_networks())
+    bind_write_configuration(
+        zones,
+        trusted_networks=get_trusted_networks(),
+        forwarded_zones=forwarded_zones,
+    )
 
     # Reloading with retries may be a legacy from Celery days, or it may be
     # necessary to recover from races during start-up. We're not sure if it is
diff --git a/src/maasserver/dns/tests/test_config.py b/src/maasserver/dns/tests/test_config.py
index c1f4e3f..7dc69c9 100644
--- a/src/maasserver/dns/tests/test_config.py
+++ b/src/maasserver/dns/tests/test_config.py
@@ -27,6 +27,7 @@ from maasserver.dns.config import (
     current_zone_serial,
     dns_force_reload,
     dns_update_all_zones,
+    forward_domains_to_forwarded_zones,
     get_internal_domain,
     get_resource_name_for_subnet,
     get_trusted_acls,
@@ -78,6 +79,27 @@ class TestDNSUtilities(MAASServerTestCase):
             MatchesStructure.byEquality(source="Force reload"),
         )
 
+    def test_forward_domains_to_forwarded_zones(self):
+        name1 = factory.make_name("domain")
+        name2 = factory.make_name("other")
+        domain1 = factory.make_Domain(name=name1)
+        domain2 = factory.make_Domain(name=name2)
+        ip1 = factory.make_ip_address()
+        ip2 = factory.make_ip_address()
+        fwd_srvr1 = factory.make_ForwardDNSServer(
+            ip_address=ip1, domains=[domain1]
+        )
+        fwd_srvr2 = factory.make_ForwardDNSServer(
+            ip_address=ip2, domains=[domain2]
+        )
+        fwd_zones = forward_domains_to_forwarded_zones(
+            Domain.objects.get_forward_domains()
+        )
+        self.assertItemsEqual(
+            fwd_zones,
+            [(name1, [fwd_srvr1.ip_address]), (name2, [fwd_srvr2.ip_address])],
+        )
+
 
 class TestDNSServer(MAASServerTestCase):
     """A base class to perform real-world DNS-related tests.
diff --git a/src/maasserver/forms/domain.py b/src/maasserver/forms/domain.py
index 05ab79c..9a81f9f 100644
--- a/src/maasserver/forms/domain.py
+++ b/src/maasserver/forms/domain.py
@@ -5,9 +5,12 @@
 
 
 from django import forms
+from django.core.exceptions import ValidationError
 
+from maasserver.fields import IPListFormField
 from maasserver.forms import APIEditMixin, MAASModelForm
 from maasserver.models.domain import Domain
+from maasserver.models.forwarddnsserver import ForwardDNSServer
 
 
 class DomainForm(MAASModelForm):
@@ -21,6 +24,31 @@ class DomainForm(MAASModelForm):
 
     authoritative = forms.NullBooleanField(required=False)
 
+    forward_dns_servers = IPListFormField(required=False)
+
+    def save(self):
+        super(MAASModelForm, self).save()
+        fwd_srvrs = self.cleaned_data.get("forward_dns_servers")
+        if fwd_srvrs is not None:
+            fwd_srvrs_list = fwd_srvrs.split(" ")
+            for fwd_srvr_ip in fwd_srvrs_list:
+                fwd_srvr = ForwardDNSServer.objects.get_or_create(
+                    ip_address=fwd_srvr_ip
+                )[0]
+                fwd_srvr.domains.add(self.instance)
+                fwd_srvr.save()
+            del self.cleaned_data["forward_dns_servers"]
+        return self.instance
+
+    def clean(self):
+        if self.data.get("authoritative") and len(
+            self.data.get("forward_dns_servers", "")
+        ):
+            raise ValidationError(
+                "a domain cannot be both authoritative and have forward dns servers"
+            )
+        super(DomainForm, self).clean()
+
     def _post_clean(self):
         # ttl=None needs to make it through.  See also APIEditMixin
         self.cleaned_data = {
diff --git a/src/maasserver/forms/tests/test_domain.py b/src/maasserver/forms/tests/test_domain.py
index c423600..f50f780 100644
--- a/src/maasserver/forms/tests/test_domain.py
+++ b/src/maasserver/forms/tests/test_domain.py
@@ -84,3 +84,35 @@ class TestDomainForm(MAASServerTestCase):
         self.assertEqual(name, domain.name)
         self.assertEqual(authoritative, domain.authoritative)
         self.assertEqual(None, domain.ttl)
+
+    def test_can_create_forward_dns_server(self):
+        name = factory.make_name("domain")
+        forward_dns_servers = [factory.make_ip_address() for _ in range(0, 2)]
+        form = DomainForm(
+            {
+                "name": name,
+                "authoritative": False,
+                "forward_dns_servers": "  ".join(forward_dns_servers),
+            }
+        )
+        self.assertTrue(form.is_valid(), form.errors)
+        domain = form.save()
+        self.assertEqual(
+            forward_dns_servers,
+            [
+                fwd_dns_srvr.ip_address
+                for fwd_dns_srvr in domain.forward_dns_servers
+            ],
+        )
+
+    def test_validate_authority(self):
+        name = factory.make_name("domain")
+        forward_dns_servers = [factory.make_ip_address() for _ in range(0, 2)]
+        form = DomainForm(
+            {
+                "name": name,
+                "authoritative": True,
+                "forward_dns_servers": " ".join(forward_dns_servers),
+            }
+        )
+        self.assertRaises(ValueError, form.save)
diff --git a/src/maasserver/migrations/maasserver/0242_forwarddnsserver.py b/src/maasserver/migrations/maasserver/0242_forwarddnsserver.py
new file mode 100644
index 0000000..dc5fdd9
--- /dev/null
+++ b/src/maasserver/migrations/maasserver/0242_forwarddnsserver.py
@@ -0,0 +1,42 @@
+# Generated by Django 2.2.12 on 2021-06-14 23:05
+
+from django.db import migrations, models
+
+import maasserver.models.cleansave
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("maasserver", "0241_physical_interface_default_node_numanode"),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name="ForwardDNSServer",
+            fields=[
+                (
+                    "id",
+                    models.AutoField(
+                        auto_created=True,
+                        primary_key=True,
+                        serialize=False,
+                        verbose_name="ID",
+                    ),
+                ),
+                ("created", models.DateTimeField(editable=False)),
+                ("updated", models.DateTimeField(editable=False)),
+                (
+                    "ip_address",
+                    models.GenericIPAddressField(
+                        default=None, editable=False, unique=True
+                    ),
+                ),
+                ("domains", models.ManyToManyField(to="maasserver.Domain")),
+            ],
+            options={
+                "abstract": False,
+            },
+            bases=(maasserver.models.cleansave.CleanSave, models.Model),
+        ),
+    ]
diff --git a/src/maasserver/models/__init__.py b/src/maasserver/models/__init__.py
index e3f96e6..29904d0 100644
--- a/src/maasserver/models/__init__.py
+++ b/src/maasserver/models/__init__.py
@@ -34,6 +34,7 @@ __all__ = [
     "FileStorage",
     "Filesystem",
     "FilesystemGroup",
+    "ForwardDNSServer",
     "GlobalDefault",
     "Interface",
     "IPRange",
@@ -136,6 +137,7 @@ from maasserver.models.filesystemgroup import (
     VMFS,
     VolumeGroup,
 )
+from maasserver.models.forwarddnsserver import ForwardDNSServer
 from maasserver.models.globaldefault import GlobalDefault
 from maasserver.models.interface import (
     BondInterface,
diff --git a/src/maasserver/models/domain.py b/src/maasserver/models/domain.py
index a131a5b..6e510e8 100644
--- a/src/maasserver/models/domain.py
+++ b/src/maasserver/models/domain.py
@@ -97,6 +97,15 @@ class DomainManager(Manager, DomainQueriesMixin):
 
         return GlobalDefault.objects.instance().domain
 
+    def get_forward_domains(self):
+        rows = self.raw(
+            """SELECT * FROM maasserver_domain domain
+            WHERE EXISTS (
+                SELECT id FROM maasserver_forwarddnsserver_domains WHERE domain_id = domain.id
+            );"""
+        )
+        return list(rows)
+
     def get_or_create_default_domain(self):
         """Return the default domain."""
         now = datetime.datetime.now()
@@ -240,6 +249,13 @@ class Domain(CleanSave, TimestampedModel):
             count += len(resource.dnsdata_set.all())
         return count
 
+    @property
+    def forward_dns_servers(self):
+        # avoid circular import
+        from maasserver.models.forwarddnsserver import ForwardDNSServer
+
+        return ForwardDNSServer.objects.filter(domains=self)
+
     def add_delegations(self, mapping, ns_host_name, dns_ip_list, default_ttl):
         """Find any subdomains that need to be added to this domain, and add
         them.
@@ -366,9 +382,17 @@ class Domain(CleanSave, TimestampedModel):
         if self.name is not None and self.name.endswith("."):
             self.name = self.name[:-1]
 
+    def validate_authority(self):
+        if self.authoritative and len(self.forward_dns_servers) > 0:
+            raise ValidationError(
+                "A Domain cannot be both authoritative and have"
+                "forward DNS servers"
+            )
+
     def clean(self, *args, **kwargs):
         super().clean(*args, **kwargs)
         self.clean_name()
+        self.validate_authority()
 
     def render_json_for_related_rrdata(
         self, for_list=False, include_dnsdata=True, as_dict=False, user=None
diff --git a/src/maasserver/models/forwarddnsserver.py b/src/maasserver/models/forwarddnsserver.py
new file mode 100644
index 0000000..9fe07be
--- /dev/null
+++ b/src/maasserver/models/forwarddnsserver.py
@@ -0,0 +1,33 @@
+""" Forward DNS Server Objects."""
+
+__all__ = [
+    "ForwardDNSServerManager",
+    "ForwardDNSServer",
+]
+
+
+from django.db.models import GenericIPAddressField, Manager, ManyToManyField
+
+from maasserver.models.cleansave import CleanSave
+from maasserver.models.domain import Domain
+from maasserver.models.timestampedmodel import TimestampedModel
+
+
+class ForwardDNSServerManager(Manager):
+    pass
+
+
+# Due to migration 0155 calling Domain's manager directly, we cannot add forward dns servers as a column to a Domain
+# so a separate table where one or more ForwardDNSServer(s) can be used in many Domains.
+class ForwardDNSServer(CleanSave, TimestampedModel):
+    """A `ForwardDNSServer`.
+    :ivar ip_address: The IP address of the forward DNS server to forward queries to.
+    :ivar domains: A many to many reference to domains that forward to this server."""
+
+    objects = ForwardDNSServerManager()
+
+    ip_address = GenericIPAddressField(
+        null=False, default=None, editable=False, unique=True
+    )
+
+    domains = ManyToManyField(Domain)
diff --git a/src/maasserver/models/tests/test_domain.py b/src/maasserver/models/tests/test_domain.py
index 840e792..356042d 100644
--- a/src/maasserver/models/tests/test_domain.py
+++ b/src/maasserver/models/tests/test_domain.py
@@ -135,6 +135,15 @@ class TestDomainManager(MAASServerTestCase):
             Domain.objects.filter_by_specifiers("name:%s" % name), [domain]
         )
 
+    def test_get_forward_domains(self):
+        name1 = factory.make_name("domain")
+        domain1 = factory.make_Domain(name=name1, authoritative=False)
+        name2 = factory.make_name("other")
+        factory.make_Domain(name=name2, authoritative=True)
+        fwd_ip = factory.make_ip_address()
+        factory.make_ForwardDNSServer(ip_address=fwd_ip, domains=[domain1])
+        self.assertItemsEqual(Domain.objects.get_forward_domains(), [domain1])
+
 
 class DomainTest(MAASServerTestCase):
     def test_creates_domain(self):
@@ -181,6 +190,17 @@ class DomainTest(MAASServerTestCase):
         domain.delete()
         self.assertItemsEqual([], Domain.objects.filter(name=name))
 
+    def test_validate_authority_raises_exception_when_both_authoritative_and_has_forward_dns_servers(
+        self,
+    ):
+        name = factory.make_name("name")
+        forward_server = factory.make_ip_address()
+        domain = factory.make_Domain(name=name, authoritative=True)
+        factory.make_ForwardDNSServer(
+            ip_address=forward_server, domains=[domain]
+        )
+        self.assertRaises(ValidationError, domain.validate_authority)
+
     def test_cant_be_deleted_if_contains_resources(self):
         domain = factory.make_Domain()
         factory.make_DNSResource(domain=domain)
diff --git a/src/maasserver/testing/factory.py b/src/maasserver/testing/factory.py
index 929c159..bdefa86 100644
--- a/src/maasserver/testing/factory.py
+++ b/src/maasserver/testing/factory.py
@@ -67,6 +67,7 @@ from maasserver.models import (
     FileStorage,
     Filesystem,
     FilesystemGroup,
+    ForwardDNSServer,
     IPRange,
     KeySource,
     LargeFile,
@@ -703,6 +704,15 @@ class Factory(maastesting.factory.Factory):
         domain.save()
         return domain
 
+    def make_ForwardDNSServer(self, ip_address=None, domains=None):
+        if ip_address is None:
+            ip_address = self.make_ip_address()
+        fwd_dns_srvr = ForwardDNSServer(ip_address=ip_address)
+        fwd_dns_srvr.save()
+        fwd_dns_srvr.domains.set(domains)
+        fwd_dns_srvr.save()
+        return fwd_dns_srvr
+
     def pick_rrset(self, rrtype=None, rrdata=None, exclude=[]):
         while rrtype is None:
             rrtype = self.pick_choice(
diff --git a/src/provisioningserver/dns/actions.py b/src/provisioningserver/dns/actions.py
index eabb3e8..7b8b1ca 100644
--- a/src/provisioningserver/dns/actions.py
+++ b/src/provisioningserver/dns/actions.py
@@ -92,7 +92,7 @@ def bind_reload_zones(zone_list):
     return ret
 
 
-def bind_write_configuration(zones, trusted_networks):
+def bind_write_configuration(zones, trusted_networks, forwarded_zones=None):
     """Write BIND's configuration.
 
     :param zones: Those zones to include in main config.
@@ -107,7 +107,7 @@ def bind_write_configuration(zones, trusted_networks):
     assert not isinstance(trusted_networks, (bytes, str))
     assert isinstance(trusted_networks, Sequence)
 
-    dns_config = DNSConfig(zones=zones)
+    dns_config = DNSConfig(zones=zones, forwarded_zones=forwarded_zones)
     dns_config.write_config(trusted_networks=trusted_networks)
 
 
diff --git a/src/provisioningserver/dns/config.py b/src/provisioningserver/dns/config.py
index 904ae8d..fe4c5a2 100644
--- a/src/provisioningserver/dns/config.py
+++ b/src/provisioningserver/dns/config.py
@@ -289,10 +289,13 @@ class DNSConfig:
     template_file_name = "named.conf.template"
     target_file_name = MAAS_NAMED_CONF_NAME
 
-    def __init__(self, zones=None):
+    def __init__(self, zones=None, forwarded_zones=None):
         if zones is None:
             zones = ()
+        if forwarded_zones is None:
+            forwarded_zones = ()
         self.zones = zones
+        self.forwarded_zones = forwarded_zones
 
     def write_config(self, overwrite=True, **kwargs):
         """Write out this DNS config file.
@@ -303,6 +306,7 @@ class DNSConfig:
         trusted_networks = kwargs.pop("trusted_networks", "")
         context = {
             "zones": self.zones,
+            "forwarded_zones": self.forwarded_zones,
             "DNS_CONFIG_DIR": get_dns_config_dir(),
             "named_rndc_conf_path": get_named_rndc_conf_path(),
             "trusted_networks": trusted_networks,
diff --git a/src/provisioningserver/dns/tests/test_config.py b/src/provisioningserver/dns/tests/test_config.py
index 042df69..97fa15b 100644
--- a/src/provisioningserver/dns/tests/test_config.py
+++ b/src/provisioningserver/dns/tests/test_config.py
@@ -63,7 +63,7 @@ from provisioningserver.dns.zoneconfig import (
     DNSReverseZoneConfig,
 )
 from provisioningserver.utils import locate_config
-from provisioningserver.utils.isc import read_isc_file
+from provisioningserver.utils.isc import parse_isc_string, read_isc_file
 
 NAMED_CONF_OPTIONS_CONTENTS = dedent(
     """\
@@ -547,6 +547,28 @@ class TestDNSConfig(MAASTestCase):
             ),
         )
 
+    def test_write_config_with_forwarded_zones(self):
+        name = factory.make_name("domain")
+        ip = factory.make_ip_address()
+        forwarded_zones = [(name, [ip])]
+        target_dir = patch_dns_config_path(self)
+        DNSConfig(forwarded_zones=forwarded_zones).write_config()
+        config_path = os.path.join(target_dir, MAAS_NAMED_CONF_NAME)
+        expected_content = dedent(
+            f"""
+        zone "{name}" {{
+            type forward;
+            forward only;
+            forwarders {{
+                {ip};
+            }};
+        }};
+        """
+        )
+        config = read_isc_file(config_path)
+        expected = parse_isc_string(expected_content)
+        self.assertEqual(expected[f'zone "{name}"'], config[f'zone "{name}"'])
+
     def test_write_config_makes_config_world_readable(self):
         target_dir = patch_dns_config_path(self)
         DNSConfig().write_config()
diff --git a/src/provisioningserver/templates/dns/named.conf.template b/src/provisioningserver/templates/dns/named.conf.template
index 938d772..f0f9841 100644
--- a/src/provisioningserver/templates/dns/named.conf.template
+++ b/src/provisioningserver/templates/dns/named.conf.template
@@ -1,6 +1,6 @@
 include "{{named_rndc_conf_path}}";
 
-# Zone declarations.
+# Authoritative Zone declarations.
 {{for zone in zones}}
 {{for zoneinfo in zone.zone_info}}
 zone "{{zoneinfo.zone_name}}" {
@@ -10,6 +10,19 @@ zone "{{zoneinfo.zone_name}}" {
 {{endfor}}
 {{endfor}}
 
+# Forwarded Zone declarations.
+{{for forwarded_zone in forwarded_zones}}
+zone "{{forwarded_zone[0]}}" {
+    type forward;
+    forward only;
+    forwarders {
+    {{for forward_server in forwarded_zone[1]}}
+        {{forward_server}};
+    {{endfor}}
+    };
+};
+{{endfor}}
+
 # Access control for recursive queries.  See named.conf.options.inside.maas
 # for the directives used on this ACL.
 acl "trusted" {