feat: dns-managers review, cloudflare drift checks and nightly sync

[jaspermayone]

Sets up review rules, a Cloudflare drift check, a nightly sync, and the docs.

What this does

flowchart TD
    A[Edit a zone file] --> B[Pull request]
    B --> C{validate}
    B --> D{plan}
    B --> E{cloudflare in sync}
    C --> F[Review by DNS Managers]
    D -->|Posts the plan as a comment| F
    E -->|Blocks if Cloudflare drifted| F
    F -->|Approved, squash merged| G[deploy]
    G --> H[(Cloudflare)]
    I[Nightly sync] -->|Reads| H
    I -->|Finds a manual change| J[Pull request]
    J --> F

Loading

Review by the DNS Managers team

CODEOWNERS now names @WITCodingClub/dns-managers for every file. Turning on
code owner review in the ruleset makes an approval from that team required.

Round robin assignment is a workflow, not a GitHub setting. Native team review
assignment needs a paid organisation plan and this org is on the free plan.
assign-reviewer.yml picks the next person from .github/dns-reviewers.txt
using the pull request number, skips the author, and drops the noisy
whole-team review request.

Nightly sync from Cloudflare

sync-from-cloudflare.yml runs at 07:17 UTC. It dumps the live zones and folds
manual dashboard changes back into the zone files as a pull request.

tools/merge_live.py does the merge with ruamel.yaml so the owner comments
survive. A plain octodns-dump overwrite would delete every comment in the
file. It also leaves out the apex NS records, which Cloudflare owns and
octoDNS refuses to change without --force. 11 unit tests cover it.

Drift gate before merge

The cloudflare in sync check plans main against Cloudflare and fails if
they differ. That stops a merge from silently undoing a dashboard change. It
skips itself on the cloudflare-sync branch, which exists to fix drift.

octodns-meta record

MetaProcessor writes a TXT record with the deploy time, provider and octoDNS
version. Use dig +short TXT octodns-meta.witcc.dev to confirm a deploy landed.
It only changes when something else in the zone changes, so it does not cause a
nightly deploy on its own.

Security fix

test.yml ran on pull_request_target, checked out the fork's code, and ran
./bin/dry-run from that fork with CLOUDFLARE_TOKEN_READ_ONLY in the
environment. Anybody could open a pull request that rewrote that script and
read the token.

plan.yml replaces it. It checks out main and runs main's scripts, and
takes only the zone files from the pull request. Those are data, not code.
validate.yml is the workflow that runs pull request code, and it holds no
secrets.

The token should still be rotated, because it was exposed to anyone who looked.

Before this can be enforced

Merge this first. Then follow docs/runbook.md:

1. Create the DNS_BOT_TOKEN secret. Without it the nightly pull request gets
   no checks and can never satisfy a required check.
2. Apply the ruleset: gh api -X PUT repos/WITCodingClub/dns/rulesets/9465567 --input docs/ruleset-main.json
3. Run the sync once by hand: gh workflow run sync-from-cloudflare.yml

Applying the ruleset before the checks exist on main would block every merge.

Note on the zone files

witcc.dev.yaml holds one record and hackwit.org.yaml is entirely commented
out. Cloudflare holds the real records. The first nightly sync will open a
large pull request that brings them all into git. That is expected.

[jaspermayone]

Findings from the first check run

The legacy octodns check failed on this branch, and the failure is worth reading. It is the current state of production, not a problem with this pull request.

hackwit.org.  Creates=1, Updates=0, Deletes=4,  Existing=4
witcc.dev.    Creates=2, Updates=0, Deletes=14, Existing=14

The zone files hold one record. Cloudflare holds 18, including the MX, SPF, DMARC and DKIM records for club email, the GitHub org verification record, and the calendar tunnel.

Two things follow

1. octoDNS's delete guard does not cover hackwit.org. It refuses a plan that deletes more than 30% of a zone, but only when the zone already has at least 10 records. MIN_EXISTING_RECORDS is a constant in octoDNS and cannot be configured. hackwit.org has 4, so octoDNS would delete all four without complaining. Only witcc.dev raised TooMuchChange and stopped the run.

deploy.yml now refuses any plan that deletes more than three records, with a manual override:

$ gh workflow run deploy.yml -f allow_mass_delete=true

2. Step 3 of the runbook is not optional. Run sync-from-cloudflare and merge its pull request before applying the ruleset. Until that lands, the cloudflare in sync check fails on every pull request, which is the correct behaviour.

Also

Pins updated to the current releases: octodns==1.22.0, octodns-cloudflare==1.2.0, ruamel.yaml==0.19.1. My first pass pinned what happened to be in the local virtualenv, which was older.

[jaspermayone]

enforce_order: True applied, plus three fixes it needed

Good suggestion. It does not stand alone though: turning it on by itself fails the build on merge. Three things had to change with it.

1. witcc.dev.yaml was already out of order. enforce_order checks every mapping, not just the top level, and inside a record octodns sorts before ttl. The existing record had octodns last:

witcc.dev.yaml: FAILS -> keys out of order: expected octodns got ttl at line 4, column 5

Reordered, and normalised to the list form the README documents.

2. tools/merge_live.py sorted the wrong way. octoDNS enforces a natural sort (natsort_keygen), not sorted(). They disagree:

natsort: ['', '_dmarc', 'api', 'cf2024-1._domainkey', 'ns2', 'ns10', ...]
plain  : ['', '_dmarc', 'api', 'cf2024-1._domainkey', 'ns10', 'ns2', ...]

The nightly sync would have written ns10 before ns2 and then failed its own validate check. It now uses the same keygen octoDNS does, and natsort is pinned explicitly since it is imported directly.

3. Two new tests. One asserts ns2 sorts before ns10. The other runs the merged output back through octodns.yaml.safe_load(enforce_order=True), which is the test that actually matters: the nightly job edits these files unattended, so its output has to be something octoDNS will load. 13 tests pass.

I also set order_mode: natural explicitly rather than leaning on the default, and documented the rule in the README, CONTRIBUTING and the PR template.

---

On the ruleset

Applied and correct: code owner review on, and the five checks required. I updated docs/runbook.md and docs/ruleset-main.json to the new id 23783065 (the old 9465567 is gone).

One consequence to know about. plan.yml runs on pull_request_target, which reads the workflow file from main. Until this PR merges, plan the change and cloudflare in sync cannot start, so this PR waits on two checks that will never report. You have bypass_mode: always as a repo admin, so merge this one through the bypass. Every PR after it gets real checks.

Your review is still pending as a draft, so the suggestion is not visible on the PR yet. Submit it whenever, the change is already in dee35ac.