fix(zone_sync): freeze before writing and keep serials above dynamic updates

mdb/management/commands/zone_sync.py

@@ -50,19 +50,45 @@ class Command(BaseCommand):
             if zone is None:
                 self.stderr.write("error: zone '%s' not found\n" % zone_name)
                 raise SystemExit(1)
-            service.sync_zone(zone)
+            zones = [zone]
         else:
-            service.sync_all_zones()
+            zones = service.pending_zones()
 
-        if service.changes and not debug:
-            self.reload_and_notify(service, service.changes)
+        if not zones:
+            return
+
+        if debug:
+            for zone in zones:
+                service.sync_zone(zone)
+            return
+
+        self.freeze(service, zones)
+        try:
+            for zone in zones:
+                service.sync_zone(zone)
+        except BaseException:
+            service.reload()
+            raise
+        self.reload_and_notify(service, service.changes)
+
+    def freeze(self, service, zones):
+        self.stdout.write("freezing zones... ")
+        success, failed_command, output = service.freeze()
+        if not success:
+            self.stdout.write("fail\n")
+            # A failed freeze may have frozen some zones before stopping.
+            service.run_commands([service.thaw_command])
+            self.notify_failure(
+                "Failed to freeze zones, nothing was written",
+                failed_command,
+                output,
+                [z.domain_name for z in zones],
+            )
+        self.stdout.write("ok\n")
 
     def reload_and_notify(self, service, changes):
         self.stdout.write("reloading zones... ")
-        success, failed_command, output = service.run_reload_commands()
-
-        zones = list(changes.keys())
-        zone_list = "\n".join("- " + z for z in zones)
+        success, failed_command, output = service.reload()
 
         if not success:
             self.stdout.write("fail\n")
@@ -70,19 +96,14 @@ class Command(BaseCommand):
                 [v["errors"] for v in changes.values() if v["errors"]]
             )
             diffs = "\n\n\n".join([v["diff"] for v in changes.values() if v["diff"]])
-            message = (
-                "Failed to reload zones, please inspect...\n\n"
-                "%s@%s$ %s\n%s\n%s"
-                % (os.getlogin(), os.uname()[1], failed_command, output, zone_list)
+            self.notify_failure(
+                "Failed to reload zones",
+                failed_command,
+                output,
+                list(changes.keys()),
+                errors,
+                diffs,
             )
-            if errors:
-                message += "\n\n" + errors
-            discord.notify(
-                subject="⚠️ FAIL: bind",
-                message=message,
-                preformatted=diffs or None,
-            )
-            raise SystemExit(1)
 
         self.stdout.write("ok\n")
         for zone, info in changes.items():
@@ -94,3 +115,24 @@ class Command(BaseCommand):
                 message=message,
                 preformatted=info["diff"] or None,
             )
+
+    def notify_failure(
+        self, summary, failed_command, output, zones, errors="", diffs=""
+    ):
+        zone_list = "\n".join("- " + z for z in zones)
+        message = "%s, please inspect...\n\n%s@%s$ %s\n%s\n%s" % (
+            summary,
+            os.getlogin(),
+            os.uname()[1],
+            failed_command,
+            output,
+            zone_list,
+        )
+        if errors:
+            message += "\n\n" + errors
+        discord.notify(
+            subject="⚠️ FAIL: bind",
+            message=message,
+            preformatted=diffs or None,
+        )
+        raise SystemExit(1)

mdb/services/zone_sync.py

@@ -1,4 +1,5 @@
 import os
+import re
 import shutil
 import sys
 import tempfile
@@ -11,10 +12,9 @@ from mdb.models import Domain, Ip4Subnet, Ip6Subnet
 
 class ZoneSyncService:
     rndc_bin = shutil.which("rndc") or "/usr/sbin/rndc"
-    reload_command = "%s reload" % rndc_bin
     freeze_command = "%s freeze" % rndc_bin
+    reload_command = "%s reload" % rndc_bin
     thaw_command = "%s thaw" % rndc_bin
-    commands_in_order = [freeze_command, reload_command, thaw_command]
     checkzone_bin = shutil.which("named-checkzone") or "/usr/sbin/named-checkzone"
     compilezone_bin = shutil.which("named-compilezone") or "/usr/sbin/named-compilezone"
     checkzone_dir = "/tmp/mdb-checkzone"
@@ -31,18 +31,30 @@ class ZoneSyncService:
                 print("ERROR: no such file %s, exiting..." % f)
                 sys.exit(1)
 
-    def run_reload_commands(self):
-        """Execute freeze/reload/thaw in order.
+    def run_commands(self, commands):
+        """Execute commands in order, stopping at the first failure.
 
         Returns (success, failed_command, output). On success, failed_command
         and output are empty strings.
         """
-        for command in self.commands_in_order:
+        for command in commands:
             status, output = getstatusoutput(command)
             if status != 0:
                 return False, command, output
         return True, "", ""
 
+    def freeze(self):
+        """Stop dynamic updates and flush each dynamic zone's journal to its file.
+
+        Must run before zone files are written: freeze dumps the zone BIND
+        holds in memory, which would overwrite a file written earlier.
+        """
+        return self.run_commands([self.freeze_command])
+
+    def reload(self):
+        """Load the written zone files and resume dynamic updates."""
+        return self.run_commands([self.reload_command, self.thaw_command])
+
     def find_zone(self, name):
         for model in (Domain, Ip4Subnet, Ip6Subnet):
             try:
@@ -51,18 +63,31 @@ class ZoneSyncService:
                 pass
         return None
 
-    def sync_all_zones(self):
-        for zone in chain(
-            Domain.objects.all(), Ip4Subnet.objects.all(), Ip6Subnet.objects.all()
-        ):
-            if zone.domain_serial == zone.domain_active_serial and not self.force:
-                continue
-            if self.sync_zone(zone):
-                self.changes[zone.domain_name]["success"] = True
+    def pending_zones(self):
+        return [
+            zone
+            for zone in chain(
+                Domain.objects.all(), Ip4Subnet.objects.all(), Ip6Subnet.objects.all()
+            )
+            if zone.domain_serial != zone.domain_active_serial or self.force
+        ]
 
     def sync_zone(self, zone):
         zonetype = getattr(zone._meta, "verbose_name", None) or zone._meta.model_name
 
+        try:
+            with open(zone.domain_filename, "r") as f:
+                old = f.read()
+        except IOError:
+            old = ""
+
+        # Dynamic updates (ACME dns-01 challenges) raise the serial in the
+        # frozen file past the one mdb generated. Secondaries ignore a zone
+        # whose serial does not increase.
+        live_serial = self.read_serial(old)
+        if live_serial is not None and live_serial >= zone.domain_serial:
+            zone.domain_serial = live_serial + 1
+
         self.changes[zone.domain_name] = {
             "old_serial": zone.domain_active_serial,
             "new_serial": zone.domain_serial,
@@ -85,12 +110,6 @@ class ZoneSyncService:
         # generated content always use the same format.
         new = self.normalize_zone(zone, zone.zone_file_contents())
 
-        try:
-            with open(zone.domain_filename, "r") as f:
-                old = f.read()
-        except IOError:
-            old = ""
-
         self.changes[zone.domain_name]["diff"] = self.calculate_diff(new, old, zone)
 
         sys.stdout.write("\t- validating... ")
@@ -121,6 +140,11 @@ class ZoneSyncService:
         self.changes[zone.domain_name]["success"] = True
         return True
 
+    def read_serial(self, contents):
+        """Return the SOA serial of a zone file, or None if there is none."""
+        match = re.search(r"^\s*(\d+)\s*;\s*serial", contents, re.MULTILINE)
+        return int(match.group(1)) if match else None
+
     def normalize_zone(self, zone, contents):
         """Convert zone file contents to BIND's canonical format.
 

tests/test_zone_sync.py

@@ -160,6 +160,27 @@ def test_check_zone_accepts_zone_file_contents(db_domain, service):
     assert output.endswith("OK")
 
 
+def test_read_serial_from_normalized_zone():
+    assert ZoneSyncService().read_serial(EXPECTED_NORMALIZED) == 2024010101
+
+
+def test_read_serial_from_generated_zone():
+    assert ZoneSyncService().read_serial(SAMPLE_ZONE) == 2024010101
+
+
+def test_read_serial_none_without_soa():
+    assert ZoneSyncService().read_serial("") is None
+
+
+def test_pending_zones_skips_synced_zones(db_domain, service):
+    assert service.pending_zones() == [db_domain]
+    db_domain.domain_active_serial = db_domain.domain_serial
+    db_domain.save()
+    assert service.pending_zones() == []
+    service.force = True
+    assert service.pending_zones() == [db_domain]
+
+
 def test_find_zone_returns_matching_domain(db_domain, service):
     assert service.find_zone("example.org") == db_domain
 
@@ -193,6 +214,35 @@ def test_sync_zone_records_success_and_diff(db_domain, tmp_path):
     assert change["new_serial"] == 2024010101
 
 
+def test_sync_zone_raises_serial_past_live_zone(db_domain, tmp_path):
+    """Dynamic updates can leave the live serial above the one mdb generated."""
+    with open(db_domain.domain_filename, "w") as f:
+        f.write(EXPECTED_NORMALIZED.replace("2024010101", "2024010137"))
+    svc = ZoneSyncService(debug=False)
+    svc.checkzone_dir = str(tmp_path)
+
+    svc.sync_zone(db_domain)
+
+    db_domain.refresh_from_db()
+    assert db_domain.domain_serial == 2024010138
+    assert db_domain.domain_active_serial == 2024010138
+    assert svc.changes["example.org"]["new_serial"] == 2024010138
+    with open(db_domain.domain_filename) as f:
+        assert svc.read_serial(f.read()) == 2024010138
+
+
+def test_sync_zone_keeps_serial_above_live_zone(db_domain, tmp_path):
+    with open(db_domain.domain_filename, "w") as f:
+        f.write(EXPECTED_NORMALIZED.replace("2024010101", "2024010099"))
+    svc = ZoneSyncService(debug=False)
+    svc.checkzone_dir = str(tmp_path)
+
+    svc.sync_zone(db_domain)
+
+    db_domain.refresh_from_db()
+    assert db_domain.domain_serial == 2024010101
+
+
 def add_host(domain, hostname, allow_email, ip4address=None):
     host_type = HostType.objects.create(host_type="server", description="")
     operating_system = OperatingSystem.objects.create(

tests/test_zone_sync_command.py

@@ -1,8 +1,10 @@
+import os
 from io import StringIO
 from unittest.mock import MagicMock, patch
 
 import pytest
 from mdb.management.commands.zone_sync import Command as CommandClass
+from mdb.services.zone_sync import ZoneSyncService
 
 
 @pytest.fixture
@@ -18,30 +20,45 @@ def make_service(changes=None):
     svc.changes = changes or {
         "example.org": {"errors": None, "diff": "- old\n+ new", "success": True}
     }
-    svc.run_reload_commands.return_value = (True, "", "")
+    svc.reload.return_value = (True, "", "")
     return svc
 
 
-# --- run_reload_commands ---
+# --- run_commands, freeze, reload ---
 
 
-def test_run_reload_commands_calls_all_on_success(service):
+def test_run_commands_calls_all_on_success(service):
     with patch("mdb.services.zone_sync.getstatusoutput", return_value=(0, "")) as mock:
-        success, _, _ = service.run_reload_commands()
+        success, _, _ = service.run_commands(["a", "b", "c"])
     assert success is True
     assert mock.call_count == 3
 
 
-def test_run_reload_commands_stops_on_first_failure(service):
+def test_run_commands_stops_on_first_failure(service):
     with patch(
         "mdb.services.zone_sync.getstatusoutput",
         side_effect=[(0, ""), (1, "connection refused"), (0, "")],
     ) as mock:
-        success, failed_cmd, output = service.run_reload_commands()
+        success, failed_cmd, output = service.run_commands(["a", "b", "c"])
     assert success is False
-    assert "reload" in failed_cmd
+    assert failed_cmd == "b"
     assert output == "connection refused"
-    assert mock.call_count == 2  # thaw never reached
+    assert mock.call_count == 2
+
+
+def test_freeze_runs_only_freeze(service):
+    with patch("mdb.services.zone_sync.getstatusoutput", return_value=(0, "")) as mock:
+        service.freeze()
+    assert [c.args[0] for c in mock.call_args_list] == [service.freeze_command]
+
+
+def test_reload_runs_reload_then_thaw(service):
+    with patch("mdb.services.zone_sync.getstatusoutput", return_value=(0, "")) as mock:
+        service.reload()
+    assert [c.args[0] for c in mock.call_args_list] == [
+        service.reload_command,
+        service.thaw_command,
+    ]
 
 
 # --- reload_and_notify ---
@@ -79,7 +96,7 @@ def test_reload_and_notify_success_passes_diff_as_preformatted(cmd):
 
 def test_reload_and_notify_failure_writes_fail_and_exits(cmd):
     svc = make_service()
-    svc.run_reload_commands.return_value = (False, "/usr/sbin/rndc freeze", "timed out")
+    svc.reload.return_value = (False, "/usr/sbin/rndc reload", "timed out")
     with patch("mdb.management.commands.zone_sync.discord"):
         with patch("os.getlogin", return_value="testuser"):
             with pytest.raises(SystemExit) as exc:
@@ -90,7 +107,7 @@ def test_reload_and_notify_failure_writes_fail_and_exits(cmd):
 
 def test_reload_and_notify_failure_discord_includes_command_and_output(cmd):
     svc = make_service()
-    svc.run_reload_commands.return_value = (False, "/usr/sbin/rndc freeze", "timed out")
+    svc.reload.return_value = (False, "/usr/sbin/rndc reload", "timed out")
     with patch("mdb.management.commands.zone_sync.discord") as mock_discord:
         with patch("os.getlogin", return_value="testuser"):
             with pytest.raises(SystemExit):
@@ -98,7 +115,7 @@ def test_reload_and_notify_failure_discord_includes_command_and_output(cmd):
     subject = mock_discord.notify.call_args.kwargs["subject"]
     message = mock_discord.notify.call_args.kwargs["message"]
     assert "⚠️" in subject
-    assert "rndc freeze" in message
+    assert "rndc reload" in message
     assert "timed out" in message
     assert "- example.org" in message
 
@@ -123,6 +140,70 @@ def test_handle_skips_reload_in_debug_mode(cmd, db_domain):
 
 @pytest.mark.django_db
 def test_handle_calls_reload_and_notify_when_changes_exist(cmd, db_domain):
-    with patch.object(cmd, "reload_and_notify") as mock_reload:
+    with (
+        patch.object(cmd, "freeze"),
+        patch.object(cmd, "reload_and_notify") as mock_reload,
+    ):
         cmd.handle(debug=False, force=True, zone="example.org")
     mock_reload.assert_called_once()
+
+
+@pytest.mark.django_db
+def test_handle_does_nothing_without_pending_zones(cmd):
+    with patch.object(ZoneSyncService, "freeze") as mock_freeze:
+        cmd.handle(debug=False, force=False, zone=None)
+    mock_freeze.assert_not_called()
+
+
+@pytest.mark.django_db
+def test_handle_freezes_before_writing_and_reloads_after(cmd, db_domain):
+    """rndc freeze dumps the in-memory zone, so it must not run after the write."""
+    seen = []
+
+    def freeze():
+        seen.append(("freeze", os.path.exists(db_domain.domain_filename)))
+        return True, "", ""
+
+    def reload():
+        seen.append(("reload", os.path.exists(db_domain.domain_filename)))
+        return True, "", ""
+
+    with (
+        patch.object(ZoneSyncService, "freeze", side_effect=freeze),
+        patch.object(ZoneSyncService, "reload", side_effect=reload),
+        patch("mdb.management.commands.zone_sync.discord"),
+    ):
+        cmd.handle(debug=False, force=False, zone=None)
+
+    assert seen == [("freeze", False), ("reload", True)]
+
+
+@pytest.mark.django_db
+def test_handle_writes_nothing_when_freeze_fails(cmd, db_domain):
+    with (
+        patch.object(
+            ZoneSyncService, "freeze", return_value=(False, "rndc freeze", "no")
+        ),
+        patch.object(ZoneSyncService, "run_commands") as mock_run,
+        patch("mdb.management.commands.zone_sync.discord") as mock_discord,
+        patch("os.getlogin", return_value="testuser"),
+        pytest.raises(SystemExit),
+    ):
+        cmd.handle(debug=False, force=False, zone=None)
+
+    assert not os.path.exists(db_domain.domain_filename)
+    mock_run.assert_called_once_with([ZoneSyncService.thaw_command])
+    assert "- example.org" in mock_discord.notify.call_args.kwargs["message"]
+
+
+@pytest.mark.django_db
+def test_handle_thaws_when_sync_raises(cmd, db_domain):
+    with (
+        patch.object(ZoneSyncService, "freeze", return_value=(True, "", "")),
+        patch.object(ZoneSyncService, "reload") as mock_reload,
+        patch.object(ZoneSyncService, "sync_zone", side_effect=RuntimeError),
+        pytest.raises(RuntimeError),
+    ):
+        cmd.handle(debug=False, force=False, zone=None)
+
+    mock_reload.assert_called_once()