Merge ~cgrabowski/maas:backport_glue_zone_fix into maas:3.3

[Christian Grabowski]

use rndc freeze and thaw to allow BIND to handle receiving full reloads and dynamic updates more rapidly
ensure updates are only for the correct subnet
(cherry picked from commit 0eb397ff84eb81f38a4a3dedae81855de55da32e)

[Christian Grabowski (cgrabowski)]

self-approving backport

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b backport_glue_zone_fix lp:~cgrabowski/maas/+git/maas into -b 3.3 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas-tester/1818/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b backport_glue_zone_fix lp:~cgrabowski/maas/+git/maas into -b 3.3 lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 7aa2fa996039e7f286717acfeeb107129205c18a

review: Approve

diff --git a/src/maasserver/dns/tests/test_zonegenerator.py b/src/maasserver/dns/tests/test_zonegenerator.py
index 598074f..ee649a4 100644
--- a/src/maasserver/dns/tests/test_zonegenerator.py
+++ b/src/maasserver/dns/tests/test_zonegenerator.py
@@ -514,7 +514,9 @@ class TestZoneGenerator(MAASServerTestCase):
     def test_glue_receives_correct_dynamic_updates(self):
         domain = factory.make_Domain()
         subnet = factory.make_Subnet(cidr=str(IPNetwork("10/29").cidr))
+        other_subnet = factory.make_Subnet()
         sip = factory.make_StaticIPAddress(subnet=subnet)
+        other_sip = factory.make_StaticIPAddress(subnet=other_subnet)
         factory.make_Node_with_Interface_on_Subnet(
             subnet=subnet, vlan=subnet.vlan, fabric=subnet.vlan.fabric
         )
@@ -528,7 +530,14 @@ class TestZoneGenerator(MAASServerTestCase):
                 zone=domain.name,
                 rectype="A",
                 answer=sip.ip,
-            )
+            ),
+            DynamicDNSUpdate(
+                operation="INSERT",
+                name=update_rec.name,
+                zone=domain.name,
+                rectype="A",
+                answer=other_sip.ip,
+            ),
         ]
         zones = ZoneGenerator(
             domain,
diff --git a/src/maasserver/dns/zonegenerator.py b/src/maasserver/dns/zonegenerator.py
index 658147f..2a87990 100644
--- a/src/maasserver/dns/zonegenerator.py
+++ b/src/maasserver/dns/zonegenerator.py
@@ -458,7 +458,7 @@ class ZoneGenerator:
                 for update in dynamic_updates
                 if update.answer
                 and update.answer_is_ip
-                and (IPAddress(update.answer) in IPNetwork(subnet.cidr))
+                and (update.answer_as_ip in IPNetwork(subnet.cidr))
             ]
 
             yield DNSReverseZoneConfig(
@@ -490,11 +490,16 @@ class ZoneGenerator:
                     if (
                         update.answer
                         and update.answer_is_ip
-                        and IPAddress(update.answer) in exclude_net
+                        and update.answer_as_ip in exclude_net
                     ):
                         glue_update = False
                         break
-                if glue_update:
+                if (
+                    glue_update
+                    and update.answer
+                    and update.answer_is_ip
+                    and update.answer_as_ip in network
+                ):
                     domain_updates.append(
                         DynamicDNSUpdate.as_reverse_record_update(
                             update, str(network)
diff --git a/src/provisioningserver/dns/actions.py b/src/provisioningserver/dns/actions.py
index bafbac9..7749ff5 100644
--- a/src/provisioningserver/dns/actions.py
+++ b/src/provisioningserver/dns/actions.py
@@ -42,6 +42,48 @@ def bind_reconfigure():
         raise
 
 
+def bind_freeze_zone(zone=None, timeout=2):
+    cmd = ("freeze",)  # freeze all zones
+    if zone:
+        cmd = ("freeze", zone)  # freeze one zone
+
+    try:
+        execute_rndc_command(cmd, timeout=timeout)
+    except CalledProcessError as e:
+        maaslog.error(
+            f"Freezing {zone if zone else 'all zones'} for update failed"
+        )
+        ExternalProcessError.upgrade(e)
+        raise
+    except TimeoutExpired as e:
+        maaslog.error(
+            f"Freezing {zone if zone else 'all zones'} for update timed out"
+        )
+        ExternalProcessError.upgrade(e)
+        raise
+
+
+def bind_thaw_zone(zone=None, timeout=2):
+    cmd = ("thaw",)  # thaw all zones
+    if zone:
+        cmd = ("thaw", zone)  # thaw one zone
+
+    try:
+        execute_rndc_command(cmd, timeout=timeout)
+    except CalledProcessError as e:
+        maaslog.error(
+            f"Thawing {zone if zone else 'all zones'} for update failed"
+        )
+        ExternalProcessError.upgrade(e)
+        raise
+    except TimeoutExpired as e:
+        maaslog.error(
+            f"Thawing {zone if zone else 'all zones'} for update timed out"
+        )
+        ExternalProcessError.upgrade(e)
+        raise
+
+
 def bind_reload(timeout=2):
     """Ask BIND to reload its configuration and all zone files.  This operation
     is 'best effort' (with logging) as the server may not be running, and there
diff --git a/src/provisioningserver/dns/config.py b/src/provisioningserver/dns/config.py
index ef5a166..729b570 100644
--- a/src/provisioningserver/dns/config.py
+++ b/src/provisioningserver/dns/config.py
@@ -9,6 +9,7 @@ from contextlib import contextmanager
 from dataclasses import dataclass
 from datetime import datetime
 import errno
+from functools import cached_property
 import grp
 import os
 import os.path
@@ -48,25 +49,17 @@ class DynamicDNSUpdate:
     answer: Optional[str] = None
 
     @classmethod
-    def _is_ip(cls, answer):
-        if not answer:
-            return False
-        try:
-            IPAddress(answer)
-        except AddrFormatError:
-            return False
-        else:
-            return True
-
-    @classmethod
     def create_from_trigger(cls, **kwargs):
         answer = kwargs.get("answer")
         rectype = kwargs.pop("rectype")
         if answer:
             del kwargs["answer"]
         # the DB trigger is unable to figure out if an IP is v6, so we do it here instead
-        if cls._is_ip(answer):
+        try:
             ip = IPAddress(answer)
+        except AddrFormatError:
+            pass
+        else:
             if ip.version == 6:
                 rectype = "AAAA"
         return cls(answer=answer, rectype=rectype, **kwargs)
@@ -86,9 +79,16 @@ class DynamicDNSUpdate:
             rectype="PTR",
         )
 
-    @property
+    @cached_property
+    def answer_as_ip(self):
+        try:
+            return IPAddress(self.answer)
+        except AddrFormatError:
+            return None
+
+    @cached_property
     def answer_is_ip(self):
-        return DynamicDNSUpdate._is_ip(self.answer)
+        return self.answer_as_ip is not None
 
 
 def get_dns_config_dir():
diff --git a/src/provisioningserver/dns/tests/test_actions.py b/src/provisioningserver/dns/tests/test_actions.py
index 1087bc0..7920448 100644
--- a/src/provisioningserver/dns/tests/test_actions.py
+++ b/src/provisioningserver/dns/tests/test_actions.py
@@ -68,15 +68,38 @@ class TestReconfigure(MAASTestCase):
         self.assertRaises(ExternalProcessError, actions.bind_reconfigure)
 
 
+class TestFreezeZone(MAASTestCase):
+    """Tests for :py:func:`actions.bind_freeze_zone`."""
+
+    def test_executes_rndc_command(self):
+        self.patch_autospec(actions, "execute_rndc_command")
+        zone = factory.make_name()
+        actions.bind_freeze_zone(zone=zone)
+        actions.execute_rndc_command.assert_called_once_with(
+            ("freeze", zone), timeout=2
+        )
+
+
+class TestThawZone(MAASTestCase):
+    """Tests for :py:func:`actions.bind_freeze_zone`."""
+
+    def test_executes_rndc_command(self):
+        self.patch_autospec(actions, "execute_rndc_command")
+        zone = factory.make_name()
+        actions.bind_thaw_zone(zone=zone)
+        actions.execute_rndc_command.assert_called_once_with(
+            ("thaw", zone), timeout=2
+        )
+
+
 class TestReload(MAASTestCase):
     """Tests for :py:func:`actions.bind_reload`."""
 
     def test_executes_rndc_command(self):
         self.patch_autospec(actions, "execute_rndc_command")
         actions.bind_reload()
-        self.assertThat(
-            actions.execute_rndc_command,
-            MockCalledOnceWith(("reload",), timeout=2),
+        actions.execute_rndc_command.assert_called_once_with(
+            ("reload",), timeout=2
         )
 
     def test_logs_subprocess_error(self):
diff --git a/src/provisioningserver/dns/tests/test_config.py b/src/provisioningserver/dns/tests/test_config.py
index 60b4c58..41a1bc5 100644
--- a/src/provisioningserver/dns/tests/test_config.py
+++ b/src/provisioningserver/dns/tests/test_config.py
@@ -666,6 +666,28 @@ class TestDynamicDNSUpdate(MAASTestCase):
         )
         self.assertEqual(update.rectype, "AAAA")
 
+    def test_answer_as_ip_returns_ip_when_answer_is_an_ip(self):
+        domain = factory.make_name()
+        update = DynamicDNSUpdate(
+            operation="INSERT",
+            zone=domain,
+            name=f"{factory.make_name()}.{domain}",
+            rectype="A",
+            answer=factory.make_ip_address(),
+        )
+        self.assertIsNotNone(update.answer_as_ip)
+
+    def test_answer_as_ip_returns_none_when_answer_is_not_an_ip(self):
+        domain = factory.make_name()
+        update = DynamicDNSUpdate(
+            operation="INSERT",
+            zone=domain,
+            name=f"{factory.make_name()}.{domain}",
+            rectype="CNAME",
+            answer=factory.make_name(),
+        )
+        self.assertIsNone(update.answer_as_ip)
+
     def test_answer_is_ip_returns_true_when_answer_is_an_ip(self):
         domain = factory.make_name()
         update = DynamicDNSUpdate(
diff --git a/src/provisioningserver/dns/zoneconfig.py b/src/provisioningserver/dns/zoneconfig.py
index 8dec9b6..59e714b 100644
--- a/src/provisioningserver/dns/zoneconfig.py
+++ b/src/provisioningserver/dns/zoneconfig.py
@@ -12,7 +12,11 @@ from pathlib import Path
 from netaddr import IPAddress, IPNetwork, spanning_cidr
 from netaddr.core import AddrFormatError
 
-from provisioningserver.dns.actions import NSUpdateCommand
+from provisioningserver.dns.actions import (
+    bind_freeze_zone,
+    bind_thaw_zone,
+    NSUpdateCommand,
+)
 from provisioningserver.dns.config import (
     compose_zone_file_config_path,
     render_dns_template,
@@ -333,24 +337,31 @@ class DNSForwardZoneConfig(DomainConfigBase):
             else:
                 Path(f"{zi.target_path}.jnl").unlink(missing_ok=True)
                 self.requires_reload = True
-                self.write_zone_file(
-                    zi.target_path,
-                    self.make_parameters(),
-                    {
-                        "mappings": {
-                            "A": self.get_A_mapping(
-                                self._mapping, self._ipv4_ttl
-                            ),
-                            "AAAA": self.get_AAAA_mapping(
-                                self._mapping, self._ipv6_ttl
+                needs_freeze_thaw = self.zone_file_exists(zi)
+                if needs_freeze_thaw:
+                    bind_freeze_zone(zone=self.domain)
+                try:
+                    self.write_zone_file(
+                        zi.target_path,
+                        self.make_parameters(),
+                        {
+                            "mappings": {
+                                "A": self.get_A_mapping(
+                                    self._mapping, self._ipv4_ttl
+                                ),
+                                "AAAA": self.get_AAAA_mapping(
+                                    self._mapping, self._ipv6_ttl
+                                ),
+                            },
+                            "other_mapping": enumerate_rrset_mapping(
+                                self._other_mapping
                             ),
+                            "generate_directives": {"A": generate_directives},
                         },
-                        "other_mapping": enumerate_rrset_mapping(
-                            self._other_mapping
-                        ),
-                        "generate_directives": {"A": generate_directives},
-                    },
-                )
+                    )
+                finally:
+                    if needs_freeze_thaw:
+                        bind_thaw_zone(zone=self.domain)
 
 
 class DNSReverseZoneConfig(DomainConfigBase):