Revert "fix: allow non-default ports for forward dns servers"

This reverts commit 99b3d1f.
canonical · Mar 13, 2024 · a552d25 · a552d25
1 parent 99b3d1f
commit a552d25
Show file tree

Hide file tree

Showing 10 changed files with 11 additions and 178 deletions.
diff --git 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.port)
+                fwd_dns_srvr.ip_address
                 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
@@ -102,10 +102,7 @@ def test_forward_domains_to_forwarded_zones(self):
         )
         self.assertCountEqual(
             fwd_zones,
-            [
-                (name1, [(fwd_srvr1.ip_address, fwd_srvr1.port)]),
-                (name2, [(fwd_srvr2.ip_address, fwd_srvr2.port)]),
-            ],
+            [(name1, [fwd_srvr1.ip_address]), (name2, [fwd_srvr2.ip_address])],
         )
 
 

diff --git a/src/maasserver/fields.py b/src/maasserver/fields.py
@@ -5,7 +5,6 @@
 
 from itertools import chain
 import re
-import urllib
 
 from django import forms
 from django.core.exceptions import ObjectDoesNotExist, ValidationError
@@ -333,44 +332,6 @@ def clean(self, value):
             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
@@ -7,7 +7,7 @@
 from django import forms
 from django.core.exceptions import ValidationError
 
-from maasserver.fields import IPPortListFormField
+from maasserver.fields import IPListFormField
 from maasserver.forms import APIEditMixin, MAASModelForm
 from maasserver.models.domain import Domain
 from maasserver.models.forwarddnsserver import ForwardDNSServer
@@ -24,16 +24,16 @@ class Meta:
 
     authoritative = forms.NullBooleanField(required=False)
 
-    forward_dns_servers = IPPortListFormField(default_port=53, 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:
-            for fwd_srvr_ip, fwd_srvr_port in fwd_srvrs:
+            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,
-                    port=fwd_srvr_port,
+                    ip_address=fwd_srvr_ip
                 )[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
@@ -104,30 +104,6 @@ def test_can_create_forward_dns_server(self):
                 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/0305_add_port_to_forward_dns_servers.py b/src/maasserver/migrations/maasserver/0305_add_port_to_forward_dns_servers.py
diff --git a/src/maasserver/models/forwarddnsserver.py b/src/maasserver/models/forwarddnsserver.py
@@ -6,12 +6,7 @@
 ]
 
 
-from django.db.models import (
-    GenericIPAddressField,
-    IntegerField,
-    Manager,
-    ManyToManyField,
-)
+from django.db.models import GenericIPAddressField, Manager, ManyToManyField
 
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.domain import Domain
@@ -36,10 +31,4 @@ 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
@@ -12,7 +12,6 @@
 from maasserver.fields import (
     HostListFormField,
     IPListFormField,
-    IPPortListFormField,
     LargeObjectField,
     LargeObjectFile,
     MODEL_NAME_VALIDATOR,
@@ -305,54 +304,6 @@ def test_separators_dont_conflict_with_ipv6_address(self):
         )
 
 
-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
@@ -601,7 +601,7 @@ def test_write_config_writes_config(self):
     def test_write_config_with_forwarded_zones(self):
         name = factory.make_name("domain")
         ip = factory.make_ip_address()
-        forwarded_zones = [(name, [(ip, None)])]
+        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)
@@ -611,7 +611,7 @@ def test_write_config_with_forwarded_zones(self):
             type forward;
             forward only;
             forwarders {{
-                {ip} port 53;
+                {ip};
             }};
         }};
         """
@@ -620,30 +620,6 @@ def test_write_config_with_forwarded_zones(self):
         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
@@ -21,7 +21,7 @@ zone "{{forwarded_zone[0]}}" {
     forward only;
     forwarders {
     {{for forward_server in forwarded_zone[1]}}
-        {{forward_server[0]}} port {{ forward_server[1] if forward_server[1] else 53 }};
+        {{forward_server}};
     {{endfor}}
     };
 };