From feb4fa07d72f9d3e9f29f02aeef8a8818207837b Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Thu, 1 Oct 2026 17:26:31 -0400 Subject: [PATCH] fix: only pull real cloudflare drift into the zone files --- tools/merge_live.py | 234 +++++++++++++++++++++++++++------------ tools/test_merge_live.py | 150 +++++++++++++++++++++++++ 2 files changed, 316 insertions(+), 68 deletions(-) diff --git a/tools/merge_live.py b/tools/merge_live.py index a62ae98..ec25d78 100755 --- a/tools/merge_live.py +++ b/tools/merge_live.py @@ -8,8 +8,13 @@ A plain ``octodns-dump`` overwrite would work, but it would delete every comment in the file, and this repository keeps the owner of each subdomain in a -comment. So this script edits the existing file in place with ruamel.yaml, -which keeps comments attached to their record. +comment. So this script edits only the records that really differ, and leaves +every other line of the file as it is. + +A record differs when octoDNS would change it on the next deploy. The script +asks the Cloudflare provider, the same code that the deploy uses. So a TTL that +Cloudflare ignores on a proxied record, a TXT value in a different order, or a +TTL that the dump leaves out because it is the default, is not a difference. Usage: @@ -23,14 +28,18 @@ import argparse import io -import re import sys +import tempfile from datetime import date from pathlib import Path from natsort import natsort_keygen +from octodns.provider.yaml import YamlProvider +from octodns.zone import Zone +from octodns_cloudflare import CloudflareProvider from ruamel.yaml import YAML -from ruamel.yaml.comments import CommentedMap + +from check_zones import _top_level_keys # octoDNS checks record order with a natural sort, so `ns2` comes before # `ns10`. Plain `sorted` gets that backwards and would write a file that @@ -41,6 +50,67 @@ # zone files, so merging them back would create an endless nightly diff. DEFAULT_IGNORED = ("octodns-meta",) +# `octodns-dump` writes its files with the YamlProvider default TTL, so a record +# with this TTL has no `ttl` key in the dump. +DUMP_DEFAULT_TTL = 3600 + +# Used when the repository has no config/config.yaml, as in the tests. +REPO_DEFAULT_TTL = 3600 +CLOUDFLARE_MIN_TTL = 120 + + +def _settings(repo_dir): + """Read the default TTL and the Cloudflare minimum TTL from the config.""" + path = Path(repo_dir) / "config" / "config.yaml" + if not path.exists(): + return REPO_DEFAULT_TTL, CLOUDFLARE_MIN_TTL + providers = YAML(typ="safe").load(path.read_text()).get("providers", {}) + default_ttl = providers.get("config", {}).get("default_ttl", REPO_DEFAULT_TTL) + min_ttl = providers.get("cloudflare", {}).get("min_ttl", CLOUDFLARE_MIN_TTL) + return default_ttl, min_ttl + + +def _load_zone(zone, directory, default_ttl): + """Load one zone file into an octoDNS Zone.""" + loaded = Zone(zone, []) + YamlProvider( + "merge-live", + str(directory), + default_ttl=default_ttl, + enforce_order=False, + escaped_semicolons=True, + ).populate(loaded, lenient=True) + return loaded + + +def _changed_names(zone, live_records, repo_dir, names): + """The names, out of `names`, that the next deploy would change. + + This is the comparison that `octoDNS plan` makes against Cloudflare, with + the live dump standing in for the Cloudflare API. `live_records` is the + dump after the ignored records and the root NS records are taken out. + """ + default_ttl, min_ttl = _settings(repo_dir) + with tempfile.TemporaryDirectory() as live_dir: + with open(Path(live_dir) / _zone_file(zone), "w") as handle: + YAML().dump(dict(live_records), handle) + live = _load_zone(zone, live_dir, DUMP_DEFAULT_TTL) + repo = _load_zone(zone, repo_dir, default_ttl) + for loaded in (live, repo): + for record in list(loaded.records): + if record.name not in names: + loaded.remove_record(record) + + # The token is never used. Nothing here calls the Cloudflare API. + cloudflare = CloudflareProvider("cloudflare", token="unused", min_ttl=min_ttl) + changes = [c for c in live.changes(repo, cloudflare) if cloudflare._include_change(c)] + changes += cloudflare._extra_changes(existing=live, desired=repo, changes=changes) + return {c.record.name for c in changes} + + +def _is_root_ns(record): + return record.get("type") == "NS" + def _strip_root_ns(live): """Drop the NS records at the apex of the zone. @@ -54,7 +124,7 @@ def _strip_root_ns(live): if root is None: return live records = root if isinstance(root, list) else [root] - kept = [r for r in records if r.get("type") != "NS"] + kept = [r for r in records if not _is_root_ns(r)] if kept: live[""] = kept else: @@ -63,48 +133,67 @@ def _strip_root_ns(live): def _yaml(): - """A YAML handler that reads and writes the octoDNS zone file style.""" + """A YAML handler that writes the octoDNS zone file style.""" handler = YAML() - handler.explicit_start = True handler.preserve_quotes = True handler.width = 4096 handler.indent(mapping=2, sequence=4, offset=2) return handler -def _normalize(value): - """Strip ruamel types so that live and repo values compare as plain data.""" - if isinstance(value, dict): - return {k: _normalize(v) for k, v in value.items()} - if isinstance(value, list): - return [_normalize(v) for v in value] - return value - - def _zone_file(zone): """`patchworklabs.org.` -> `patchworklabs.org.yaml`.""" return f"{zone.rstrip('.')}.yaml" -def _space_records(text): - """Put exactly one blank line between top level records. +def _blocks(lines): + """Split a zone file into a head and one block per record. - ruamel keeps a comment with its own record but does not reliably keep the - blank lines around it once records are reordered. Normalizing the spacing - afterwards is simpler and gives a stable diff. + A block starts at the comment lines directly above a record name and ends + at the blank line before the next block. Returns (head, {name: (start, + end)}), where `head` is the index of the first block. + """ + keys = [(number - 1, name) for number, name, _ in _top_level_keys("\n".join(lines))] + starts = [] + for index, _ in keys: + start = index + while start > 0 and lines[start - 1].startswith("#"): + start -= 1 + starts.append(start) + blocks = {} + for position, (index, name) in enumerate(keys): + end = starts[position + 1] if position + 1 < len(keys) else len(lines) + while end > index + 1 and lines[end - 1].strip() == "": + end -= 1 + blocks[name] = (starts[position], end) + head = starts[0] if starts else len(lines) + return head, blocks + + +def _with_ttl(value): + """Give every record in a dumped value an explicit TTL. + + The dump leaves out a TTL of 3600, its own default. The zone files have a + different default, so a record copied without its TTL would change. """ - lines = text.split("\n") - out = [] - top_level = re.compile(r"^[^\s#-][^:]*:") - for line in lines: - if top_level.match(line) and out: - while out and out[-1].strip() == "": - out.pop() - if out and out[-1].strip() != "---": - out.append("") - out.append(line) - text = "\n".join(out) - return re.sub(r"\n{3,}", "\n\n", text).rstrip("\n") + "\n" + for record in value if isinstance(value, list) else [value]: + if "ttl" not in record: + record.insert(1 if "octodns" in record else 0, "ttl", DUMP_DEFAULT_TTL) + return value + + +def _render(handler, name, value, comment_line): + """Write one record as zone file lines, with `comment_line` as its key line.""" + value = _with_ttl(value) + buffer = io.StringIO() + handler.dump({name: value}, buffer) + body = buffer.getvalue().rstrip("\n").split("\n")[1:] + return [comment_line, *body] + + +def _key_line(name, comment): + key = '""' if name == "" else name + return f"{key}: # {comment}" if comment else f"{key}:" def merge_zone(zone, live_dir, repo_dir, ignored, today, keep_root_ns=False): @@ -117,49 +206,58 @@ def merge_zone(zone, live_dir, repo_dir, ignored, today, keep_root_ns=False): if not live_path.exists(): raise FileNotFoundError(f"no dump for {zone} at {live_path}") - live = handler.load(live_path.read_text()) or CommentedMap() + live = YAML().load(live_path.read_text()) or {} live = {k: v for k, v in live.items() if k not in ignored} if not keep_root_ns: live = _strip_root_ns(live) - existing = CommentedMap() - doc_comment = None - if repo_path.exists(): - loaded = handler.load(repo_path.read_text()) - if isinstance(loaded, CommentedMap): - existing = loaded - doc_comment = loaded.ca.comment - - added, updated, removed = [], [], [] - for key in live: - if key not in existing: - added.append(key) - elif _normalize(existing[key]) != _normalize(live[key]): - updated.append(key) - for key in existing: - if key not in live and key not in ignored: - removed.append(key) + text = repo_path.read_text() if repo_path.exists() else "---\n" + lines = text.rstrip("\n").split("\n") + head, blocks = _blocks(lines) + comments = {n: c for _, n, c in _top_level_keys(text)} + + added = sorted((k for k in live if k not in blocks), key=_natsort_key) + removed = sorted( + (k for k in blocks if k not in live and k not in ignored), key=_natsort_key + ) + common = {k for k in live if k in blocks} + updated = [] + if common: + changed = _changed_names(zone, live, repo_dir, common) + updated = sorted(changed, key=_natsort_key) if not (added or updated or removed): return [], [], [] - merged = CommentedMap() - # The apex record, whose name is the empty string, sorts first by itself. - for key in sorted(live, key=_natsort_key): - merged[key] = live[key] - carried = existing.ca.items.get(key) - if carried is not None: - merged.ca.items[key] = carried - elif key in added: - merged.yaml_add_eol_comment( - f"TODO owner unknown, added from Cloudflare on {today}", key - ) - if doc_comment is not None: - merged.ca.comment = doc_comment - - buffer = io.StringIO() - handler.dump(merged, buffer) - repo_path.write_text(_space_records(buffer.getvalue())) + replacements = {} + for key in updated: + replacements[key] = _render(handler, key, live[key], _key_line(key, comments.get(key))) + for key in added: + owner = f"TODO owner unknown, added from Cloudflare on {today}" + replacements[key] = _render(handler, key, live[key], _key_line(key, owner)) + + # Rebuild the file block by block, in natural order. A block that did not + # change is copied as it was, comments and all. + kept = [k for k in blocks if k not in removed] + names = sorted(set(kept) | set(added), key=_natsort_key) + out = lines[:head] + while out and out[-1].strip() == "": + out.pop() + for key in names: + if key in replacements: + start, end = blocks.get(key, (None, None)) + above = [] + if start is not None: + key_index = next(i for i in range(start, end) if not lines[i].startswith("#")) + above = lines[start:key_index] + block = above + replacements[key] + else: + start, end = blocks[key] + block = lines[start:end] + if out and out[-1].strip() != "---": + out.append("") + out.extend(block) + repo_path.write_text("\n".join(out).rstrip("\n") + "\n") return added, updated, removed diff --git a/tools/test_merge_live.py b/tools/test_merge_live.py index 2f49905..db52fb3 100644 --- a/tools/test_merge_live.py +++ b/tools/test_merge_live.py @@ -265,6 +265,156 @@ def test_other_root_records_survive_the_ns_filter(self): self.assertIn("192.0.2.7", result) self.assertNotIn("ns1.cloudflare.com.", result) + def write_config(self, default_ttl): + (self.repo_dir / "config").mkdir() + (self.repo_dir / "config" / "config.yaml").write_text( + "providers:\n" + f" config:\n default_ttl: {default_ttl}\n" + " cloudflare:\n min_ttl: 120\n" + ) + + def test_auto_ttl_on_a_proxied_record_is_not_drift(self): + # Cloudflare reports a proxied record with TTL 1, which the dump writes + # as auto-ttl and 300. Cloudflare ignores the TTL on a proxied record, + # so the deploy does not change it, and neither should this script. + repo = """--- +www: # ada@patchworklabs.org + octodns: + cloudflare: + proxied: true + ttl: 120 + type: A + value: 192.0.2.1 +""" + (self.repo_dir / "patchworklabs.org.yaml").write_text(repo) + self.write_live( + """--- +www: + octodns: + cloudflare: + auto-ttl: true + proxied: true + ttl: 300 + type: A + value: 192.0.2.1 +""" + ) + self.assertEqual(([], [], []), self.merge()) + self.assertEqual(repo, self.result()) + + def test_txt_values_in_another_order_are_not_drift(self): + repo = """--- +"": # ada@patchworklabs.org + ttl: 600 + type: TXT + values: + - b=2 + - a=1 +""" + (self.repo_dir / "patchworklabs.org.yaml").write_text(repo) + self.write_live( + """--- +'': + ttl: 600 + type: TXT + values: + - a=1 + - b=2 +""" + ) + self.assertEqual(([], [], []), self.merge()) + self.assertEqual(repo, self.result()) + + def test_default_ttls_are_not_drift(self): + # The zone files default to 600 and the dump defaults to 3600, so the + # same TTL can be left out on one side and written on the other. + self.write_config(600) + repo = """--- +long: # ada@patchworklabs.org + ttl: 3600 + type: TXT + value: x +short: # ada@patchworklabs.org + type: TXT + value: y +""" + (self.repo_dir / "patchworklabs.org.yaml").write_text(repo) + self.write_live( + """--- +long: + type: TXT + value: x +short: + ttl: 600 + type: TXT + value: y +""" + ) + self.assertEqual(([], [], []), self.merge()) + self.assertEqual(repo, self.result()) + + def test_a_copied_record_keeps_the_dump_default_ttl(self): + self.write_config(600) + self.write_live( + REPO_FILE + + """ +blog: + type: CNAME + value: blog.example.com. +""" + ) + self.merge() + self.assertIn( + "blog: # TODO owner unknown, added from Cloudflare on 2026-09-21\n" + " ttl: 3600\n", + self.result(), + ) + + def test_unchanged_records_keep_their_exact_text(self): + repo = """"": # ada@patchworklabs.org + - ttl: 600 + type: A + value: 192.0.2.1 + +# Owned by the API team. +api: # @patchworklabsorg/api + octodns: + cloudflare: + proxied: true + ttl: 600 + type: A + value: 192.0.2.2 + +# Grace runs this one. +zeta: # grace@patchworklabs.org + ttl: 600 + type: A + value: 192.0.2.3 +""" + (self.repo_dir / "patchworklabs.org.yaml").write_text(repo) + self.write_live( + """--- +'': + - ttl: 600 + type: A + value: 192.0.2.1 +api: + octodns: + cloudflare: + proxied: true + ttl: 600 + type: A + value: 192.0.2.2 +zeta: + ttl: 600 + type: A + value: 192.0.2.99 +""" + ) + self.assertEqual(([], ["zeta"], []), self.merge()) + expected = repo.replace("192.0.2.3", "192.0.2.99") + self.assertEqual(expected, self.result()) + def test_missing_dump_is_an_error(self): with self.assertRaises(FileNotFoundError): self.merge()