Merge ~cgrabowski/maas:backport_lp2056050_3.4 into maas:3.4

[Christian Grabowski]

fix: allow non-default ports for forward dns servers

[MAAS Lander (maas-lander)]

UNIT TESTS
-b backport_lp2056050_3.4 lp:~cgrabowski/maas/+git/maas into -b 3.4 lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 2813fddd11a7a573d793c0746cbc47565c5712b4

review: Approve

[Björn Tillenius (bjornt)]

+1

review: Approve

diff --git a/src/maasserver/dns/config.py b/src/maasserver/dns/config.py
index b03d47d..4709fe6 100644
--- a/src/maasserver/dns/config.py
+++ b/src/maasserver/dns/config.py
@@ -60,7 +60,7 @@ def forward_domains_to_forwarded_zones(forward_domains):
         (
             domain.name,
             [
-                fwd_dns_srvr.ip_address
+                (fwd_dns_srvr.ip_address, fwd_dns_srvr.port)
                 for fwd_dns_srvr in domain.forward_dns_servers
             ],
         )
diff --git a/src/maasserver/dns/tests/test_config.py b/src/maasserver/dns/tests/test_config.py
index 44507a0..620cf44 100644
--- a/src/maasserver/dns/tests/test_config.py
+++ b/src/maasserver/dns/tests/test_config.py
@@ -102,7 +102,10 @@ class TestDNSUtilities(MAASServerTestCase):
         )
         self.assertCountEqual(
             fwd_zones,
-            [(name1, [fwd_srvr1.ip_address]), (name2, [fwd_srvr2.ip_address])],
+            [
+                (name1, [(fwd_srvr1.ip_address, fwd_srvr1.port)]),
+                (name2, [(fwd_srvr2.ip_address, fwd_srvr2.port)]),
+            ],
         )
 
 
diff --git a/src/maasserver/fields.py b/src/maasserver/fields.py
index 661a4d6..f4bd280 100644
--- a/src/maasserver/fields.py
+++ b/src/maasserver/fields.py
@@ -5,6 +5,7 @@
 
 from itertools import chain
 import re
+import urllib
 
 from django import forms
 from django.core.exceptions import ObjectDoesNotExist, ValidationError
@@ -332,6 +333,44 @@ class IPListFormField(forms.CharField):
             return " ".join(ips)
 
 
+class IPPortListFormField(IPListFormField):
+    def __init__(self, default_port=None, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self._default_port = default_port
+
+    def clean(self, value):
+        if value is None:
+            return None
+        else:
+            ip_ports = re.split(self.separators, value)
+            ip_ports = [
+                ip_port.strip() for ip_port in ip_ports if ip_ports != ""
+            ]
+            result = []
+            for ip_port in ip_ports:
+                if "." in ip_port or ("[" == ip_port[0] and "]:" in ip_port):
+                    sock_addr = urllib.parse.urlsplit("//" + ip_port)
+                    ip = sock_addr.hostname
+                    port = (
+                        int(sock_addr.port)
+                        if sock_addr.port
+                        else self._default_port
+                    )
+                else:
+                    ip = ip_port
+                    port = self._default_port
+                try:
+                    GenericIPAddressField().clean(ip, model_instance=None)
+                    IntegerField().clean(port, model_instance=None)
+                except ValidationError:
+                    raise ValidationError(
+                        f"Invalid IP and port combination: {ip_port};"
+                        f"please provide a list of space-separated IP addresses {'and port' if not self._default_port else ''}"
+                    )
+                result.append((ip, port))
+        return result
+
+
 class HostListFormField(forms.CharField):
     """Accepts a space/comma separated list of hostnames or IP addresses.
 
diff --git a/src/maasserver/forms/domain.py b/src/maasserver/forms/domain.py
index b530f3f..d5739a4 100644
--- a/src/maasserver/forms/domain.py
+++ b/src/maasserver/forms/domain.py
@@ -7,7 +7,7 @@
 from django import forms
 from django.core.exceptions import ValidationError
 
-from maasserver.fields import IPListFormField
+from maasserver.fields import IPPortListFormField
 from maasserver.forms import APIEditMixin, MAASModelForm
 from maasserver.models.domain import Domain
 from maasserver.models.forwarddnsserver import ForwardDNSServer
@@ -24,16 +24,16 @@ class DomainForm(MAASModelForm):
 
     authoritative = forms.NullBooleanField(required=False)
 
-    forward_dns_servers = IPListFormField(required=False)
+    forward_dns_servers = IPPortListFormField(default_port=53, 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:
+            for fwd_srvr_ip, fwd_srvr_port in fwd_srvrs:
                 fwd_srvr = ForwardDNSServer.objects.get_or_create(
-                    ip_address=fwd_srvr_ip
+                    ip_address=fwd_srvr_ip,
+                    port=fwd_srvr_port,
                 )[0]
                 fwd_srvr.domains.add(self.instance)
                 fwd_srvr.save()
diff --git a/src/maasserver/forms/tests/test_domain.py b/src/maasserver/forms/tests/test_domain.py
index 043dc47..e9a3a3e 100644
--- a/src/maasserver/forms/tests/test_domain.py
+++ b/src/maasserver/forms/tests/test_domain.py
@@ -104,6 +104,30 @@ class TestDomainForm(MAASServerTestCase):
                 for fwd_dns_srvr in domain.forward_dns_servers
             ],
         )
+        for fwd_dns_srvr in domain.forward_dns_servers:
+            self.assertEqual(fwd_dns_srvr.port, 53)
+
+    def test_can_create_forward_dns_server_with_port(self):
+        name = factory.make_name("domain")
+        forward_dns_servers = [
+            f"{factory.make_ip_address(ipv6=False)}:5353" 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_and_port
+                for fwd_dns_srvr in domain.forward_dns_servers
+            ],
+        )
 
     def test_validate_authority(self):
         name = factory.make_name("domain")
diff --git a/src/maasserver/migrations/maasserver/0318_add_port_to_forward_dns_servers.py b/src/maasserver/migrations/maasserver/0318_add_port_to_forward_dns_servers.py
 new file mode 100644
index 0000000..322927f
--- /dev/null
+++ b/src/maasserver/migrations/maasserver/0318_add_port_to_forward_dns_servers.py
@@ -0,0 +1,17 @@
+# Generated by Django 3.2.12 on 2024-03-07 17:16
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("maasserver", "0304_interface_params_no_autoconf"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="forwarddnsserver",
+            name="port",
+            field=models.IntegerField(default=53),
+        ),
+    ]
diff --git a/src/maasserver/models/forwarddnsserver.py b/src/maasserver/models/forwarddnsserver.py
index 5dcf740..64eb2e8 100644
--- a/src/maasserver/models/forwarddnsserver.py
+++ b/src/maasserver/models/forwarddnsserver.py
@@ -6,7 +6,12 @@ __all__ = [
 ]
 
 
-from django.db.models import GenericIPAddressField, Manager, ManyToManyField
+from django.db.models import (
+    GenericIPAddressField,
+    IntegerField,
+    Manager,
+    ManyToManyField,
+)
 
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.domain import Domain
@@ -31,4 +36,10 @@ class ForwardDNSServer(CleanSave, TimestampedModel):
         null=False, default=None, editable=False, unique=True
     )
 
+    port = IntegerField(null=False, default=53)
+
     domains = ManyToManyField(Domain)
+
+    @property
+    def ip_and_port(self):
+        return f"{self.ip_address}:{self.port}"
diff --git a/src/maasserver/tests/test_fields.py b/src/maasserver/tests/test_fields.py
index 643366f..7af6607 100644
--- a/src/maasserver/tests/test_fields.py
+++ b/src/maasserver/tests/test_fields.py
@@ -12,6 +12,7 @@ from testtools import ExpectedException
 from maasserver.fields import (
     HostListFormField,
     IPListFormField,
+    IPPortListFormField,
     LargeObjectField,
     LargeObjectFile,
     MODEL_NAME_VALIDATOR,
@@ -304,6 +305,54 @@ class TestIPListFormField(MAASTestCase):
         )
 
 
+class TestIPPortListFormField(MAASTestCase):
+    def test_accepts_ipv4_with_port(self):
+        ips = [factory.make_ip_address(ipv6=False) for _ in range(5)]
+        ip_ports = [f"{ip}:80" for ip in ips]
+        input = ",".join(ip_ports)
+        self.assertEqual(
+            [(ip, 80) for ip in ips], IPPortListFormField().clean(input)
+        )
+
+    def test_accepts_ipv4_without_port(self):
+        ips = [factory.make_ip_address(ipv6=False) for _ in range(5)]
+        input = ",".join(ips)
+        self.assertEqual(
+            [(ip, 80) for ip in ips],
+            IPPortListFormField(default_port=80).clean(input),
+        )
+
+    def test_accepts_ipv6_with_port(self):
+        ips = [factory.make_ip_address(ipv6=True) for _ in range(5)]
+        ip_ports = [f"[{ip}]:80" for ip in ips]
+        input = ",".join(ip_ports)
+        self.assertEqual(
+            [(ip, 80) for ip in ips], IPPortListFormField().clean(input)
+        )
+
+    def test_accepts_ipv6_without_port(self):
+        ips = [factory.make_ip_address(ipv6=True) for _ in range(5)]
+        input = ",".join(ips)
+        self.assertEqual(
+            [(ip, 80) for ip in ips],
+            IPPortListFormField(default_port=80).clean(input),
+        )
+
+    def test_rejects_invalid_ip(self):
+        ip_ports = [
+            f"{factory.make_ip_address(ipv6=False)}:80" for _ in range(4)
+        ]
+        invalid = f"{factory.make_name()}:80"
+        ip_ports.append(invalid)
+        input = ",".join(ip_ports)
+        error = self.assertRaises(
+            ValidationError, IPPortListFormField().clean, input
+        )
+        self.assertIn(
+            f"Invalid IP and port combination: {invalid}", error.message
+        )
+
+
 class TestHostListFormField(MAASTestCase):
     def test_accepts_none(self):
         self.assertIsNone(HostListFormField().clean(None))
diff --git a/src/provisioningserver/dns/tests/test_config.py b/src/provisioningserver/dns/tests/test_config.py
index 8c57ce4..9c7ad2f 100644
--- a/src/provisioningserver/dns/tests/test_config.py
+++ b/src/provisioningserver/dns/tests/test_config.py
@@ -601,7 +601,7 @@ 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])]
+        forwarded_zones = [(name, [(ip, None)])]
         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)
@@ -611,7 +611,7 @@ class TestDNSConfig(MAASTestCase):
             type forward;
             forward only;
             forwarders {{
-                {ip};
+                {ip} port 53;
             }};
         }};
         """
@@ -620,6 +620,30 @@ class TestDNSConfig(MAASTestCase):
         expected = parse_isc_string(expected_content)
         self.assertEqual(expected[f'zone "{name}"'], config[f'zone "{name}"'])
 
+    def test_write_config_with_forwarded_zones_with_custom_port(self):
+        name = factory.make_name("domain")
+        ip = factory.make_ip_address()
+        port = 5353
+        forwarded_zones = [(name, [(ip, port)])]
+        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} port {port};
+            }};
+        }};
+        """
+        )
+        config = read_isc_file(config_path)
+        print(config)
+        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 7217ca8..a909560 100644
--- a/src/provisioningserver/templates/dns/named.conf.template
+++ b/src/provisioningserver/templates/dns/named.conf.template
@@ -21,7 +21,7 @@ zone "{{forwarded_zone[0]}}" {
     forward only;
     forwarders {
     {{for forward_server in forwarded_zone[1]}}
-        {{forward_server}};
+        {{forward_server[0]}} port {{ forward_server[1] if forward_server[1] else 53 }};
     {{endfor}}
     };
 };