From 8a88729792263ad44e74ad57dd2d0ff8b1840830 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 14:02:25 -0400 Subject: [PATCH 1/8] chore: pin octodns and ignore the local virtualenv --- .gitignore | 16 +++++++++++++++- requirements.txt | 8 ++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 requirements.txt diff --git a/.gitignore b/.gitignore index 2eea525..ffb58fe 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,15 @@ -.env \ No newline at end of file +# Secrets. Never commit a Cloudflare token. +.env +.env.* + +# Local Python virtualenv used for `bin/*` scripts. +env/ +venv/ +.venv/ +__pycache__/ +*.pyc + +# Generated plan and dump output. +plan.md +plan.json +.live/ diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 0000000..3922eb5 --- /dev/null +++ b/requirements.txt @@ -0,0 +1,8 @@ +# Pinned so that a nightly job and a pull request plan always agree. +# Bump with a pull request, never in place. +octodns==1.14.0 +octodns-cloudflare==1.0.0 + +# Used by tools/merge_live.py to merge Cloudflare state back into the zone +# files without destroying the ownership comments. +ruamel.yaml==0.18.6 From d61e9dc13eda7ee87b2ca71e2b801f0f3117cb73 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 14:02:25 -0400 Subject: [PATCH 2/8] feat: add plan, dump and validate scripts and an octodns-meta record --- bin/dry-run | 4 +++- bin/dump | 24 ++++++++++++++++++++ bin/plan | 12 ++++++++++ bin/sync | 9 +++++++- bin/validate | 9 ++++++++ bin/zones | 10 +++++++++ config/config.yaml | 56 +++++++++++++++++++++++++++++++++++++++------- 7 files changed, 114 insertions(+), 10 deletions(-) create mode 100755 bin/dump create mode 100755 bin/plan create mode 100755 bin/validate create mode 100755 bin/zones diff --git a/bin/dry-run b/bin/dry-run index 90b426b..124f5a4 100755 --- a/bin/dry-run +++ b/bin/dry-run @@ -1,3 +1,5 @@ #!/bin/sh +# Alias for ./bin/plan, kept because older docs and habits refer to it. +set -eu -octodns-sync --config-file=./config/config.yaml \ No newline at end of file +exec "$(dirname "$0")/plan" "$@" diff --git a/bin/dump b/bin/dump new file mode 100755 index 0000000..7b43d9f --- /dev/null +++ b/bin/dump @@ -0,0 +1,24 @@ +#!/bin/sh +# Dump the live Cloudflare state into a directory as octoDNS zone files. +# +# ./bin/dump .live +# +# This is what the nightly Cloudflare sync uses to find records that somebody +# added or edited in the Cloudflare dashboard. It never writes to the zone +# files at the repository root. Use tools/merge_live.py for that. +# +# Needs CLOUDFLARE_TOKEN in the environment. A read-only token is enough. +set -eu + +out="${1:-.live}" +mkdir -p "$out" + +"$(dirname "$0")/zones" | while read -r zone; do + [ -n "$zone" ] || continue + echo "dumping $zone" + octodns-dump \ + --config-file=./config/config.yaml \ + --output-dir="$out" \ + --lenient \ + "$zone" cloudflare +done diff --git a/bin/plan b/bin/plan new file mode 100755 index 0000000..e2a58d1 --- /dev/null +++ b/bin/plan @@ -0,0 +1,12 @@ +#!/bin/sh +# Show what a deploy would change, without changing anything. +# +# The plan is written to stdout as Markdown, so it can be captured and posted +# as a pull request comment. octoDNS log output goes to stderr. +# +# ./bin/plan > plan.md +# +# Needs CLOUDFLARE_TOKEN in the environment. A read-only token is enough. +set -eu + +exec octodns-sync --config-file=./config/config.yaml "$@" diff --git a/bin/sync b/bin/sync index 87641cf..a09cdee 100755 --- a/bin/sync +++ b/bin/sync @@ -1,3 +1,10 @@ #!/bin/sh +# Apply the zone files to Cloudflare. This changes production DNS. +# +# Only .github/workflows/deploy.yml should run this. Run it by hand only when +# you are recovering from a failed deploy, and read docs/runbook.md first. +# +# Needs CLOUDFLARE_TOKEN in the environment with write access. +set -eu -octodns-sync --config-file=./config/config.yaml --doit \ No newline at end of file +exec octodns-sync --config-file=./config/config.yaml --doit "$@" diff --git a/bin/validate b/bin/validate new file mode 100755 index 0000000..09aa2a4 --- /dev/null +++ b/bin/validate @@ -0,0 +1,9 @@ +#!/bin/sh +# Check that the config and the zone files parse and that every record is +# valid. Does not contact Cloudflare, so it needs no real token. +set -eu + +CLOUDFLARE_TOKEN="${CLOUDFLARE_TOKEN:-validate-only-not-a-real-token}" +export CLOUDFLARE_TOKEN + +exec octodns-validate --config-file=./config/config.yaml "$@" diff --git a/bin/zones b/bin/zones new file mode 100755 index 0000000..413b9f6 --- /dev/null +++ b/bin/zones @@ -0,0 +1,10 @@ +#!/bin/sh +# Print every zone named in config/config.yaml, one per line, so that scripts +# and workflows never keep their own copy of the list. +set -eu + +exec python3 -c " +import yaml +with open('config/config.yaml') as fh: + print('\n'.join(yaml.safe_load(fh)['zones'])) +" diff --git a/config/config.yaml b/config/config.yaml index bb01242..4f400d6 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -1,21 +1,61 @@ --- +# octoDNS manager configuration. +# +# The YAML files at the repository root are the source of truth. Cloudflare is +# the target. Everything here is applied by .github/workflows/deploy.yml after a +# pull request merges to main. +# +# Docs: https://octodns.readthedocs.io/en/stable/ + +manager: + # Where a plan is written. PlanLogger writes to stderr, so it shows up in the + # workflow log. PlanMarkdown writes to stdout, so `bin/plan` can capture it + # and post it as a pull request comment. + plan_outputs: + logger: + class: octodns.provider.plan.PlanLogger + level: info + markdown: + class: octodns.provider.plan.PlanMarkdown + + # Applied after every zone's own processors. See the `processors` block below. + post_processors: + - meta + +processors: + # Writes a TXT record that records when the zone was last applied, by which + # octoDNS version, and against which provider. Use it to confirm that a + # deploy reached Cloudflare and that the change propagated: + # + # dig +short TXT octodns-meta.witcc.dev + # + # The record only changes when something else in the zone changes, so it does + # not create a deploy every night on its own. + meta: + class: octodns.processor.meta.MetaProcessor + record_name: octodns-meta + include_time: true + include_provider: true + include_version: true + ttl: 60 + providers: config: class: octodns.provider.yaml.YamlProvider directory: ./ + # Records do not have to be alphabetical. Keep them alphabetical anyway, + # because it keeps diffs small. enforce_order: False cloudflare: class: octodns_cloudflare.CloudflareProvider token: env/CLOUDFLARE_TOKEN - # Production best practices - plan_type: free # Cloudflare plan type - min_ttl: 120 # Cloudflare minimum - # Enhanced reliability settings - retry_count: 5 # More retries for production - retry_period: 600 # 10 minute wait on rate limits + plan_type: free + min_ttl: 120 + retry_count: 5 + retry_period: 600 pagerules: false - zones_per_page: 50 # API pagination - records_per_page: 100 # API pagination + zones_per_page: 50 + records_per_page: 100 zones: witcc.dev.: From 63eed6f3ed0aa595658ace7a66cb228a4cbcde6b Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 14:02:25 -0400 Subject: [PATCH 3/8] feat: merge cloudflare state back into the zone files, keeping comments --- tools/merge_live.py | 230 +++++++++++++++++++++++++++++++++++++++ tools/test_merge_live.py | 219 +++++++++++++++++++++++++++++++++++++ 2 files changed, 449 insertions(+) create mode 100755 tools/merge_live.py create mode 100644 tools/test_merge_live.py diff --git a/tools/merge_live.py b/tools/merge_live.py new file mode 100755 index 0000000..cd350bb --- /dev/null +++ b/tools/merge_live.py @@ -0,0 +1,230 @@ +#!/usr/bin/env python3 +"""Merge the live Cloudflare state back into the repository zone files. + +People sometimes add or change a record in the Cloudflare dashboard instead of +opening a pull request. The nightly workflow dumps the live zones with +``bin/dump`` and then runs this script to fold those changes into the zone +files at the repository root. + +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. + +Usage: + + python3 tools/merge_live.py --live-dir .live --repo-dir . \ + --zone witcc.dev. --zone hackwit.org. --summary-out summary.md + +The script writes a Markdown summary to ``--summary-out`` and prints one line +per zone to stdout. It exits 0 when nothing changed and 0 when something did. +Use ``git status`` to find out which files it touched. +""" + +import argparse +import io +import re +import sys +from datetime import date +from pathlib import Path + +from ruamel.yaml import YAML +from ruamel.yaml.comments import CommentedMap + +# Records that octoDNS itself manages. They live in Cloudflare but never in the +# zone files, so merging them back would create an endless nightly diff. +DEFAULT_IGNORED = ("octodns-meta",) + + +def _strip_root_ns(live): + """Drop the NS records at the apex of the zone. + + Cloudflare assigns the nameservers for a zone and reports them over the + API. octoDNS refuses to apply a plan that changes the apex NS records + without `--force`, so pulling them into the zone files would break every + later deploy. Cloudflare owns them. Leave them alone. + """ + root = live.get("") + 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"] + if kept: + live[""] = kept + else: + del live[""] + return live + + +def _yaml(): + """A YAML handler that reads and 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): + """`witcc.dev.` -> `witcc.dev.yaml`.""" + return f"{zone.rstrip('.')}.yaml" + + +def _space_records(text): + """Put exactly one blank line between top level records. + + 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. + """ + 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" + + +def merge_zone(zone, live_dir, repo_dir, ignored, today, keep_root_ns=False): + """Merge one zone. Returns (added, updated, removed) record name lists.""" + handler = _yaml() + name = _zone_file(zone) + live_path = Path(live_dir) / name + repo_path = Path(repo_dir) / name + + 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 = {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) + + if not (added or updated or removed): + return [], [], [] + + merged = CommentedMap() + for key in sorted(live, key=lambda k: (k != "", k)): + 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())) + return added, updated, removed + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.split("\n")[0]) + parser.add_argument("--live-dir", required=True, help="output of bin/dump") + parser.add_argument("--repo-dir", default=".", help="where the zone files live") + parser.add_argument( + "--zone", + dest="zones", + action="append", + required=True, + help="a zone to merge, with the trailing dot, repeatable", + ) + parser.add_argument( + "--ignore-record", + dest="ignored", + action="append", + default=[], + help="a record name to leave out of the merge, repeatable", + ) + parser.add_argument( + "--keep-root-ns", + action="store_true", + help="pull the apex NS records in too. Cloudflare owns them, so this " + "will make later deploys fail. Only use it to inspect a zone.", + ) + parser.add_argument("--summary-out", help="write a Markdown summary here") + args = parser.parse_args(argv) + + ignored = set(DEFAULT_IGNORED) | set(args.ignored) + today = date.today().isoformat() + + summary = [] + changed = False + for zone in args.zones: + added, updated, removed = merge_zone( + zone, args.live_dir, args.repo_dir, ignored, today, args.keep_root_ns + ) + if not (added or updated or removed): + print(f"{zone} in sync") + continue + changed = True + print( + f"{zone} {len(added)} added, {len(updated)} updated, " + f"{len(removed)} removed" + ) + summary.append(f"### `{zone.rstrip('.')}`\n") + for label, names in ( + ("Added in Cloudflare", added), + ("Changed in Cloudflare", updated), + ("Removed in Cloudflare", removed), + ): + if names: + summary.append(f"**{label}**\n") + summary.extend(f"- `{n or '@'}`" for n in sorted(names)) + summary.append("") + + if args.summary_out: + text = "\n".join(summary) if changed else "No drift found.\n" + if any("Removed in Cloudflare" in line for line in summary): + text += ( + "\n> **Check the removals.** A record is listed as removed " + "because it is in the zone file but not in Cloudflare. That " + "usually means somebody deleted it in the dashboard. It can " + "also mean the last deploy failed. Confirm which one it is " + "before you merge.\n" + ) + Path(args.summary_out).write_text(text) + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/test_merge_live.py b/tools/test_merge_live.py new file mode 100644 index 0000000..0eb64e5 --- /dev/null +++ b/tools/test_merge_live.py @@ -0,0 +1,219 @@ +"""Tests for tools/merge_live.py. + +Run them with: + + python3 -m unittest discover -s tools -p 'test_*.py' -v +""" + +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from merge_live import merge_zone # noqa: E402 + +ZONE = "witcc.dev." + +REPO_FILE = """--- + +# Wentworth Coding Club, witcc.dev. + +api: # mayonej@wit.edu + - ttl: 600 + type: CNAME + value: api.example.com. + +zeta: # lambertl@wit.edu + - ttl: 600 + type: A + value: 192.0.2.1 +""" + + +class MergeZoneTest(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.root = Path(self.tmp.name) + self.live_dir = self.root / "live" + self.repo_dir = self.root / "repo" + self.live_dir.mkdir() + self.repo_dir.mkdir() + (self.repo_dir / "witcc.dev.yaml").write_text(REPO_FILE) + + def write_live(self, body): + (self.live_dir / "witcc.dev.yaml").write_text(body) + + def merge(self, keep_root_ns=False): + return merge_zone( + ZONE, + self.live_dir, + self.repo_dir, + {"octodns-meta"}, + "2026-09-21", + keep_root_ns, + ) + + def result(self): + return (self.repo_dir / "witcc.dev.yaml").read_text() + + def test_no_drift_leaves_the_file_alone(self): + self.write_live(REPO_FILE) + added, updated, removed = self.merge() + self.assertEqual(([], [], []), (added, updated, removed)) + self.assertEqual(REPO_FILE, self.result()) + + def test_new_record_is_added_and_flagged_for_an_owner(self): + self.write_live( + REPO_FILE + + """ +blog: + - ttl: 300 + type: CNAME + value: blog.example.com. +""" + ) + added, updated, removed = self.merge() + self.assertEqual((["blog"], [], []), (added, updated, removed)) + result = self.result() + self.assertIn("blog: # TODO owner unknown, added from Cloudflare on 2026-09-21", result) + self.assertIn("value: blog.example.com.", result) + + def test_changed_record_keeps_its_owner_comment(self): + self.write_live(REPO_FILE.replace("192.0.2.1", "192.0.2.99")) + added, updated, removed = self.merge() + self.assertEqual(([], ["zeta"], []), (added, updated, removed)) + result = self.result() + self.assertIn("zeta: # lambertl@wit.edu", result) + self.assertIn("192.0.2.99", result) + self.assertNotIn("192.0.2.1\n", result) + # An untouched record keeps its comment too. + self.assertIn("api: # mayonej@wit.edu", result) + + def test_record_missing_from_cloudflare_is_removed(self): + self.write_live( + """--- +api: # mayonej@wit.edu + - ttl: 600 + type: CNAME + value: api.example.com. +""" + ) + added, updated, removed = self.merge() + self.assertEqual(([], [], ["zeta"]), (added, updated, removed)) + self.assertNotIn("zeta", self.result()) + + def test_octodns_meta_is_ignored(self): + self.write_live( + REPO_FILE + + """ +octodns-meta: + - ttl: 60 + type: TXT + value: time=2026-09-21T00:00:00 +""" + ) + added, updated, removed = self.merge() + self.assertEqual(([], [], []), (added, updated, removed)) + self.assertNotIn("octodns-meta", self.result()) + + def test_records_are_sorted_with_the_root_first(self): + self.write_live( + """--- +zeta: # lambertl@wit.edu + - ttl: 600 + type: A + value: 192.0.2.1 +"": # eboard + - ttl: 300 + type: A + value: 192.0.2.5 +api: # mayonej@wit.edu + - ttl: 600 + type: CNAME + value: api.example.com. +""" + ) + self.merge() + result = self.result() + order = [result.index(k) for k in ('""', "api:", "zeta:")] + self.assertEqual(sorted(order), order) + + def test_output_is_still_valid_yaml(self): + self.write_live( + REPO_FILE + + """ +blog: + - ttl: 300 + type: CNAME + value: blog.example.com. +""" + ) + self.merge() + from ruamel.yaml import YAML + + parsed = YAML(typ="safe").load(self.result()) + self.assertEqual({"api", "zeta", "blog"}, set(parsed)) + self.assertEqual("blog.example.com.", parsed["blog"][0]["value"]) + + def test_root_ns_from_cloudflare_is_left_out(self): + # Cloudflare owns the apex nameservers. Pulling them into the zone file + # makes every later deploy fail on octoDNS's root NS safety check. + self.write_live( + REPO_FILE + + """ +"": + - ttl: 3600 + type: NS + values: + - ns1.cloudflare.com. + - ns2.cloudflare.com. +""" + ) + added, updated, removed = self.merge() + self.assertEqual(([], [], []), (added, updated, removed)) + self.assertNotIn("cloudflare.com.", self.result()) + + def test_root_ns_is_kept_when_asked_for(self): + self.write_live( + REPO_FILE + + """ +"": + - ttl: 3600 + type: NS + values: + - ns1.cloudflare.com. +""" + ) + added, _, _ = self.merge(keep_root_ns=True) + self.assertEqual([""], added) + + def test_other_root_records_survive_the_ns_filter(self): + self.write_live( + REPO_FILE + + """ +"": + - ttl: 3600 + type: NS + values: + - ns1.cloudflare.com. + - ttl: 300 + type: A + value: 192.0.2.7 +""" + ) + added, _, _ = self.merge() + self.assertEqual([""], added) + result = self.result() + self.assertIn("192.0.2.7", result) + self.assertNotIn("ns1.cloudflare.com.", result) + + def test_missing_dump_is_an_error(self): + with self.assertRaises(FileNotFoundError): + self.merge() + + +if __name__ == "__main__": + unittest.main() From 71c6123c8998317fc443211878c9d5c720e1006d Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 14:02:25 -0400 Subject: [PATCH 4/8] feat: rework workflows for plan comments, drift gating and nightly sync --- .github/workflows/assign-reviewer.yml | 96 +++++++++++++ .github/workflows/deploy.yaml | 22 --- .github/workflows/deploy.yml | 66 +++++++++ .github/workflows/plan.yml | 151 +++++++++++++++++++++ .github/workflows/sync-from-cloudflare.yml | 125 +++++++++++++++++ .github/workflows/test.yml | 32 ----- .github/workflows/validate.yml | 60 ++++++-- 7 files changed, 488 insertions(+), 64 deletions(-) create mode 100644 .github/workflows/assign-reviewer.yml delete mode 100644 .github/workflows/deploy.yaml create mode 100644 .github/workflows/deploy.yml create mode 100644 .github/workflows/plan.yml create mode 100644 .github/workflows/sync-from-cloudflare.yml delete mode 100644 .github/workflows/test.yml diff --git a/.github/workflows/assign-reviewer.yml b/.github/workflows/assign-reviewer.yml new file mode 100644 index 0000000..5403121 --- /dev/null +++ b/.github/workflows/assign-reviewer.yml @@ -0,0 +1,96 @@ +name: assign-reviewer + +# Picks the next person in the review rotation and asks them to review. +# +# GitHub can do this by itself with team review assignment, but that feature +# needs a paid organisation plan. This is the same idea in a workflow: the +# rotation is .github/dns-reviewers.txt and the turn is decided by the pull +# request number, so it moves on by one with every pull request. +# +# This workflow never checks out or runs code from the pull request, which is +# why `pull_request_target` is safe here. + +on: + pull_request_target: + types: [opened, ready_for_review] + +permissions: + contents: read + +jobs: + assign: + name: pick the next reviewer + runs-on: ubuntu-latest + if: github.event.pull_request.draft == false + permissions: + contents: read + pull-requests: write + env: + # CODEOWNERS asks the whole team for a review. Set this to false if you + # would rather leave that request in place and only add the individual. + REMOVE_TEAM_REQUEST: 'true' + TEAM_SLUG: dns-managers + steps: + - name: Check out main (for the rotation file) + uses: actions/checkout@v7 + with: + ref: ${{ github.event.pull_request.base.ref }} + sparse-checkout: .github/dns-reviewers.txt + sparse-checkout-cone-mode: false + + - name: Request a review + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number }} + AUTHOR: ${{ github.event.pull_request.user.login }} + run: | + set -eu + + rotation=$(grep -vE '^\s*(#|$)' .github/dns-reviewers.txt | tr -d ' \r') + count=$(printf '%s\n' "$rotation" | grep -c . || true) + if [ "$count" -eq 0 ]; then + echo "::warning title=No reviewers::.github/dns-reviewers.txt is empty." + exit 0 + fi + + # Already has somebody on it, so leave it alone. This keeps a reopen + # or a "ready for review" from piling on more reviewers. + existing=$(gh pr view "$PR" --repo "$REPO" \ + --json reviewRequests --jq '.reviewRequests | map(.login // empty) | length') + if [ "$existing" -gt 0 ]; then + echo "A reviewer is already requested. Nothing to do." + exit 0 + fi + + # Start at the pull request number and walk forward, so consecutive + # pull requests go to consecutive people, and skip the author. + reviewer='' + i=0 + while [ "$i" -lt "$count" ]; do + pick=$(( (PR + i) % count + 1 )) + candidate=$(printf '%s\n' "$rotation" | sed -n "${pick}p") + if [ "$candidate" != "$AUTHOR" ]; then + reviewer="$candidate" + break + fi + i=$(( i + 1 )) + done + + if [ -z "$reviewer" ]; then + echo "::notice title=No reviewer::$AUTHOR is the only person in the rotation." + exit 0 + fi + + echo "Asking $reviewer to review #$PR." + gh pr edit "$PR" --repo "$REPO" \ + --add-reviewer "$reviewer" \ + --add-assignee "$reviewer" + + if [ "$REMOVE_TEAM_REQUEST" = 'true' ]; then + # CODEOWNERS still requires an approval from the team. Dropping the + # team's review request only stops everybody being notified. + gh api -X DELETE "repos/$REPO/pulls/$PR/requested_reviewers" \ + -f "team_reviewers[]=$TEAM_SLUG" >/dev/null 2>&1 \ + || echo "The team was not requested, or could not be removed." + fi diff --git a/.github/workflows/deploy.yaml b/.github/workflows/deploy.yaml deleted file mode 100644 index 616370d..0000000 --- a/.github/workflows/deploy.yaml +++ /dev/null @@ -1,22 +0,0 @@ -name: deploy - -on: - workflow_dispatch: - push: - branches: - - main - -jobs: - octodns: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-python@v5 - with: - python-version: '3' - - name: Install OctoDNS - run: pip install 'octodns>=1.5.0' octodns-dnsimple octodns-cloudflare - - name: Sync w/ production DNS providers - run: ./bin/sync - env: - CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN }} \ No newline at end of file diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml new file mode 100644 index 0000000..3bd412d --- /dev/null +++ b/.github/workflows/deploy.yml @@ -0,0 +1,66 @@ +name: deploy + +# Applies the zone files to Cloudflare. This is the only workflow that holds a +# Cloudflare token with write access. + +on: + push: + branches: [main] + paths: + - '*.yaml' + - 'config/**' + - 'bin/**' + - 'requirements.txt' + - '.github/workflows/deploy.yml' + workflow_dispatch: + +permissions: + contents: read + +# Never let two deploys apply to Cloudflare at the same time, and never cancel +# one halfway through. +concurrency: + group: deploy-cloudflare + cancel-in-progress: false + +jobs: + deploy: + name: apply to cloudflare + runs-on: ubuntu-latest + # Add reviewers or a wait timer to this environment in the repository + # settings if you ever want a second pair of eyes between merge and apply. + environment: production + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-python@v7 + with: + python-version: '3.12' + cache: pip + + - name: Install octoDNS + run: pip install -r requirements.txt + + - name: Show what will be applied + env: + CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN }} + run: ./bin/plan | tee "$GITHUB_STEP_SUMMARY" + + - name: Apply to Cloudflare + env: + CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN }} + run: ./bin/sync + + - name: Confirm Cloudflare now matches main + env: + CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN }} + run: | + set -eu + ./bin/plan > after.md + if grep -q 'No changes were planned' after.md; then + echo "Cloudflare matches main." + exit 0 + fi + echo "::error title=Deploy did not finish::Cloudflare still does not match main after the apply." + cat after.md + exit 1 diff --git a/.github/workflows/plan.yml b/.github/workflows/plan.yml new file mode 100644 index 0000000..7f99eb1 --- /dev/null +++ b/.github/workflows/plan.yml @@ -0,0 +1,151 @@ +name: plan + +# Shows a reviewer exactly which DNS records a pull request would change, and +# blocks the merge when Cloudflare has drifted away from main. +# +# SECURITY. This workflow uses `pull_request_target`, so it can read secrets +# even on a pull request from a fork. That is only safe because it never runs +# code that came from the fork: +# +# 1. It checks out main, which is trusted, and runs main's scripts. +# 2. From the fork it copies only the zone files at the repository root. +# Those are data. octoDNS parses them, it does not execute them. +# +# Do not add a step that runs a script, a Makefile, an action, or a dependency +# install from the pull request. That would hand the Cloudflare token to +# whoever opened the pull request. + +on: + pull_request_target: + types: [opened, synchronize, reopened] + +permissions: + contents: read + +concurrency: + group: plan-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + plan: + name: plan the change + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + steps: + - name: Check out main (trusted code) + uses: actions/checkout@v7 + with: + ref: ${{ github.event.pull_request.base.ref }} + + - name: Check out the pull request (data only) + uses: actions/checkout@v7 + with: + repository: ${{ github.event.pull_request.head.repo.full_name }} + ref: ${{ github.event.pull_request.head.sha }} + path: .pull-request + persist-credentials: false + + - name: Take the zone files from the pull request + run: | + set -eu + # Replace only the zone files. Everything else, including bin/ and + # config/, stays on main's trusted version. + rm -f ./*.yaml + cp .pull-request/*.yaml ./ 2>/dev/null || true + rm -rf .pull-request + ls -1 ./*.yaml || echo "The pull request leaves no zone files." + + - uses: actions/setup-python@v7 + with: + python-version: '3.12' + cache: pip + + - name: Install octoDNS + run: pip install -r requirements.txt + + - name: Plan the change + env: + CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN_READ_ONLY }} + run: ./bin/plan > plan.md + + - name: Post the plan on the pull request + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: | + set -eu + marker='' + + { + printf '%s\n' "$marker" + printf '## octoDNS plan\n\n' + if grep -q 'No changes were planned' plan.md; then + printf 'This pull request changes no DNS records.\n' + elif [ "$(wc -c < plan.md)" -gt 50000 ]; then + head -c 50000 plan.md + printf '\n\n_Plan truncated. Open the workflow log for all of it._\n' + else + cat plan.md + fi + printf '\n\n---\n' + printf 'Planned against `%s` with a read-only Cloudflare token. ' "$BASE_REF" + printf 'Only the zone files at the repository root are included. ' + printf 'Changes to `config/`, `bin/` or `.github/` are **not** in ' + printf 'this plan, so review those by hand.\n' + } > comment.md + + id=$(gh api "repos/$REPO/issues/$PR/comments" --paginate \ + --jq "map(select(.body | startswith(\"$marker\"))) | .[0].id // empty") + if [ -n "$id" ]; then + gh api -X PATCH "repos/$REPO/issues/comments/$id" -F body=@comment.md + else + gh api -X POST "repos/$REPO/issues/$PR/comments" -F body=@comment.md + fi + + drift: + name: cloudflare in sync + runs-on: ubuntu-latest + # The nightly sync pull request exists to fix drift, so it must not be + # blocked by drift. The repository check matters: without it, anybody could + # skip this gate by naming their fork's branch `cloudflare-sync`. + if: >- + !(github.event.pull_request.head.repo.full_name == github.repository && + github.event.pull_request.head.ref == 'cloudflare-sync') + steps: + - name: Check out main + uses: actions/checkout@v7 + with: + ref: ${{ github.event.pull_request.base.ref }} + + - uses: actions/setup-python@v7 + with: + python-version: '3.12' + cache: pip + + - name: Install octoDNS + run: pip install -r requirements.txt + + - name: Compare Cloudflare against main + env: + CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN_READ_ONLY }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: | + set -eu + ./bin/plan > drift.md + if grep -q 'No changes were planned' drift.md; then + echo "Cloudflare matches $BASE_REF." + exit 0 + fi + + echo "::error title=Cloudflare has drifted::Cloudflare does not match $BASE_REF. Merging now would undo changes that somebody made in the Cloudflare dashboard." + echo + echo "Difference between Cloudflare and $BASE_REF:" + cat drift.md + echo + echo "Fix it by merging the open 'cloudflare-sync' pull request, or by" + echo "running the 'sync-from-cloudflare' workflow to create one." + exit 1 diff --git a/.github/workflows/sync-from-cloudflare.yml b/.github/workflows/sync-from-cloudflare.yml new file mode 100644 index 0000000..680c084 --- /dev/null +++ b/.github/workflows/sync-from-cloudflare.yml @@ -0,0 +1,125 @@ +name: sync-from-cloudflare + +# Pulls records that somebody added or changed in the Cloudflare dashboard back +# into the zone files, and opens a pull request with them. +# +# Without this, a manual change in Cloudflare is silently undone the next time +# somebody merges a pull request. With it, the change shows up as a reviewable +# diff, keeps its history, and can be given an owner. +# +# The pull request needs DNS_BOT_TOKEN to be checked. GitHub does not start +# workflows for commits pushed with the built-in GITHUB_TOKEN, so without that +# secret the pull request gets no checks and cannot satisfy a required check. +# See docs/runbook.md for how to create it. + +on: + schedule: + # 07:17 UTC, which is the small hours in Boston. The odd minute keeps this + # off the top of the hour, when GitHub's scheduler is busiest. + - cron: '17 7 * * *' + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: sync-from-cloudflare + cancel-in-progress: false + +jobs: + sync: + name: pull cloudflare into git + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + issues: write + steps: + - uses: actions/checkout@v7 + with: + token: ${{ secrets.DNS_BOT_TOKEN || secrets.GITHUB_TOKEN }} + + - uses: actions/setup-python@v7 + with: + python-version: '3.12' + cache: pip + + - name: Install octoDNS + run: pip install -r requirements.txt + + - name: Dump the live Cloudflare zones + env: + CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN_READ_ONLY }} + run: ./bin/dump .live + + - name: Merge Cloudflare into the zone files + run: | + set -eu + zones=$(./bin/zones | sed 's/^/--zone /' | tr '\n' ' ') + + # shellcheck disable=SC2086 + python tools/merge_live.py \ + --live-dir .live \ + --repo-dir . \ + --summary-out summary.md \ + $zones + rm -rf .live + + - name: Open or update the pull request + env: + GH_TOKEN: ${{ secrets.DNS_BOT_TOKEN || secrets.GITHUB_TOKEN }} + HAS_BOT_TOKEN: ${{ secrets.DNS_BOT_TOKEN != '' }} + run: | + set -eu + + if git diff --quiet -- ./*.yaml; then + echo "Cloudflare matches the zone files. Nothing to do." + rm -f summary.md + exit 0 + fi + + gh label create cloudflare-drift \ + --color FBCA04 \ + --description "Records changed in the Cloudflare dashboard" \ + --force + + git config user.name 'github-actions[bot]' + git config user.email '41898282+github-actions[bot]@users.noreply.github.com' + git switch -C cloudflare-sync + git add -- ./*.yaml + git commit -m 'chore: pull manual Cloudflare changes into the zone files' + git push --force origin cloudflare-sync + + { + printf 'Somebody changed DNS in the Cloudflare dashboard instead ' + printf 'of opening a pull request. This brings the zone files back ' + printf 'in line so that the next deploy does not undo the change.\n\n' + cat summary.md + printf '\n### Before you merge\n\n' + printf -- '- Give every record marked `TODO owner unknown` an owner, ' + printf 'in a comment on the same line.\n' + printf -- '- Check that each change is one the club wants to keep.\n' + printf -- '- If a change should not have been made, revert it in ' + printf 'Cloudflare and close this pull request. It reopens tomorrow ' + printf 'if the change is still there.\n' + if [ "$HAS_BOT_TOKEN" != 'true' ]; then + printf '\n> **No checks will run on this pull request.** ' + printf 'The `DNS_BOT_TOKEN` secret is not set, so GitHub will not ' + printf 'start workflows for these commits. See `docs/runbook.md`.\n' + fi + } > body.md + + number=$(gh pr list --head cloudflare-sync --state open \ + --json number --jq '.[0].number // empty') + if [ -n "$number" ]; then + gh pr edit "$number" --body-file body.md + echo "Updated pull request #$number." + else + gh pr create \ + --title 'chore: pull manual Cloudflare changes into the zone files' \ + --body-file body.md \ + --base main \ + --head cloudflare-sync \ + --label cloudflare-drift + fi + rm -f summary.md body.md diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml deleted file mode 100644 index bfa46e3..0000000 --- a/.github/workflows/test.yml +++ /dev/null @@ -1,32 +0,0 @@ -name: test - -on: ['push', 'pull_request_target'] - -jobs: - octodns: - runs-on: ubuntu-latest - steps: - # For forked PRs: checkout the PR head (fork + SHA) - - name: Checkout PR head - if: github.event_name == 'pull_request_target' - uses: actions/checkout@v4 - with: - repository: ${{ github.event.pull_request.head.repo.full_name }} - ref: ${{ github.event.pull_request.head.sha }} - - # For normal pushes: default checkout - - name: Checkout (push) - if: github.event_name != 'pull_request_target' - uses: actions/checkout@v4 - - - uses: actions/setup-python@v5 - with: - python-version: '3' - - - name: Install OctoDNS - run: pip install 'octodns>=1.5.0' octodns-cloudflare octodns-dnsimple - - - name: Do a dry run - run: ./bin/dry-run - env: - CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN_READ_ONLY }} \ No newline at end of file diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 7110fc0..854b0be 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -1,23 +1,63 @@ -name: json-yaml-validate +name: validate + +# Runs on every pull request, including pull requests from forks. It holds no +# secrets and never contacts Cloudflare, so it is safe to run on code that +# somebody else wrote. + on: push: - branches: - - main + branches: [main] pull_request: workflow_dispatch: permissions: contents: read - pull-requests: write jobs: - json-yaml-validate: + yaml: + name: yaml syntax + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - name: Check YAML and JSON syntax + uses: GrantBirki/json-yaml-validate@v5 + with: + # Not "true". A pull request from a fork gets a read-only token, so + # the action could not comment, and the job would fail for every + # outside contributor. The failure is visible in the check itself. + comment: "false" + + records: + name: dns records + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-python@v7 + with: + python-version: '3.12' + cache: pip + + - name: Install octoDNS + run: pip install -r requirements.txt + + - name: Validate every record + run: ./bin/validate + + tools: + name: tools tests runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - - name: json-yaml-validate - id: json-yaml-validate - uses: GrantBirki/json-yaml-validate@v2.7.1 + - uses: actions/setup-python@v7 with: - comment: "true" \ No newline at end of file + python-version: '3.12' + cache: pip + + - name: Install dependencies + run: pip install -r requirements.txt + + - name: Run the tests + run: python -m unittest discover -s tools -p 'test_*.py' -v From 1c26cd0dacd0e83f4779eec5ba95bb7e3ba33c71 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 14:02:25 -0400 Subject: [PATCH 5/8] feat: require a dns-managers review and rotate the reviewer --- .github/CODEOWNERS | 17 ++++- .github/ISSUE_TEMPLATE/config.yml | 5 ++ .github/ISSUE_TEMPLATE/dns-problem.yml | 31 ++++++++ .github/ISSUE_TEMPLATE/subdomain-request.yml | 46 ++++++++++++ .github/PULL_REQUEST_TEMPLATE.md | 28 +++++++ .github/dns-reviewers.txt | 13 ++++ docs/ruleset-main.json | 77 ++++++++++++++++++++ 7 files changed, 215 insertions(+), 2 deletions(-) create mode 100644 .github/ISSUE_TEMPLATE/config.yml create mode 100644 .github/ISSUE_TEMPLATE/dns-problem.yml create mode 100644 .github/ISSUE_TEMPLATE/subdomain-request.yml create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 .github/dns-reviewers.txt create mode 100644 docs/ruleset-main.json diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index f934c0b..88f54fd 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,3 +1,16 @@ -* @jaspermayone +# Every change here reaches production DNS, so every change needs an approving +# review from the DNS Managers team. +# +# https://github.com/orgs/WITCodingClub/teams/dns-managers +# +# The team must keep write access to this repository, otherwise GitHub ignores +# these rules. -.github/** @jaspermayone +* @WITCodingClub/dns-managers + +# These decide what gets applied and how. A mistake here is worse than a +# mistake in a single record, so they are listed again to make that clear. +/.github/ @WITCodingClub/dns-managers +/bin/ @WITCodingClub/dns-managers +/config/ @WITCodingClub/dns-managers +/tools/ @WITCodingClub/dns-managers diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 0000000..d3364e6 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1,5 @@ +blank_issues_enabled: true +contact_links: + - name: Ask in Discord + url: https://discord.gg/witcodingclub + about: Quicker than an issue for anything that is not a DNS change. diff --git a/.github/ISSUE_TEMPLATE/dns-problem.yml b/.github/ISSUE_TEMPLATE/dns-problem.yml new file mode 100644 index 0000000..2a21745 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/dns-problem.yml @@ -0,0 +1,31 @@ +name: DNS problem +description: Report a subdomain that does not resolve or points somewhere wrong. +title: "[problem] " +labels: ["bug"] +body: + - type: input + id: name + attributes: + label: Which name is wrong? + placeholder: docs.witcc.dev + validations: + required: true + + - type: textarea + id: expected + attributes: + label: What did you expect, and what happened instead? + validations: + required: true + + - type: textarea + id: dig + attributes: + label: Output of dig + description: | + Run this and paste the result. + + dig +short docs.witcc.dev + render: shell + validations: + required: false diff --git a/.github/ISSUE_TEMPLATE/subdomain-request.yml b/.github/ISSUE_TEMPLATE/subdomain-request.yml new file mode 100644 index 0000000..175a30b --- /dev/null +++ b/.github/ISSUE_TEMPLATE/subdomain-request.yml @@ -0,0 +1,46 @@ +name: Subdomain request +description: Ask for a subdomain when you cannot open a pull request yourself. +title: "[subdomain] " +labels: ["enhancement"] +body: + - type: markdown + attributes: + value: | + A pull request is faster than an issue. See the README for how to open + one. Use this form if you are stuck, or if you are not sure what the + record should be. + + - type: input + id: subdomain + attributes: + label: Subdomain + description: The full name you want. + placeholder: docs.witcc.dev + validations: + required: true + + - type: input + id: target + attributes: + label: Where should it point? + description: A domain name or an IP address. + placeholder: docs-site.netlify.app + validations: + required: true + + - type: input + id: owner + attributes: + label: Who owns it? + description: The WIT email of whoever we should ask when it breaks. + placeholder: yourname@wit.edu + validations: + required: true + + - type: textarea + id: purpose + attributes: + label: What is it for? + description: Which club project, event, or service does this serve? + validations: + required: true diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000..dfae5b3 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,28 @@ +## What does this change? + + + +## Who owns the records? + + + +## Checklist + +- [ ] The record is for a club project, event, or service. +- [ ] Every record I added or changed has an owner in a comment. +- [ ] Records are in alphabetical order in the file. +- [ ] CNAME values end with a dot. A and AAAA values do not. +- [ ] I have read the `octoDNS plan` comment on this pull request and it + changes only what I expected. + +## Anything else a reviewer should know? + + diff --git a/.github/dns-reviewers.txt b/.github/dns-reviewers.txt new file mode 100644 index 0000000..a63baa0 --- /dev/null +++ b/.github/dns-reviewers.txt @@ -0,0 +1,13 @@ +# Round robin review rotation for DNS pull requests. +# +# One GitHub username per line. Lines that start with # are ignored. +# +# Everybody here must be a member of the @WITCodingClub/dns-managers team, +# because CODEOWNERS requires an approval from that team. This file only +# decides whose turn it is to be asked. Anybody on the team can still approve. +# +# To hand the rotation to somebody new, add their username and open a pull +# request. To pause somebody, comment their line out. + +jaspermayone +Cattn diff --git a/docs/ruleset-main.json b/docs/ruleset-main.json new file mode 100644 index 0000000..c124ac9 --- /dev/null +++ b/docs/ruleset-main.json @@ -0,0 +1,77 @@ +{ + "name": "main", + "target": "branch", + "enforcement": "active", + "conditions": { + "ref_name": { + "exclude": [], + "include": [ + "~DEFAULT_BRANCH" + ] + } + }, + "bypass_actors": [ + { + "actor_id": 5, + "actor_type": "RepositoryRole", + "bypass_mode": "always" + } + ], + "rules": [ + { + "type": "deletion" + }, + { + "type": "non_fast_forward" + }, + { + "type": "creation" + }, + { + "type": "update" + }, + { + "type": "pull_request", + "parameters": { + "required_approving_review_count": 1, + "dismiss_stale_reviews_on_push": true, + "required_reviewers": [], + "require_code_owner_review": true, + "dismissal_restriction": { + "enabled": false, + "allowed_actors": [] + }, + "require_last_push_approval": true, + "required_review_thread_resolution": true, + "require_extra_approval_for_unattributed_changes": true, + "allowed_merge_methods": [ + "squash" + ] + } + }, + { + "type": "required_status_checks", + "parameters": { + "strict_required_status_checks_policy": false, + "do_not_enforce_on_create": false, + "required_status_checks": [ + { + "context": "yaml syntax" + }, + { + "context": "dns records" + }, + { + "context": "tools tests" + }, + { + "context": "plan the change" + }, + { + "context": "cloudflare in sync" + } + ] + } + } + ] +} From f1fc2ce7e41c6c3510b3b9e367646fd4ad307e91 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 14:02:25 -0400 Subject: [PATCH 6/8] docs: rewrite the readme and add a contributing guide and runbook --- CONTRIBUTING.md | 90 ++++++++++++++++++ README.md | 236 ++++++++++++++++++++++++++++++------------------ SECURITY.md | 50 ++++++++++ docs/runbook.md | 171 +++++++++++++++++++++++++++++++++++ 4 files changed, 457 insertions(+), 90 deletions(-) create mode 100644 CONTRIBUTING.md create mode 100644 SECURITY.md create mode 100644 docs/runbook.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..9f29552 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,90 @@ +# Contributing + +Thank you for helping run the club's DNS. This file covers the rules. The +[README](./README.md) covers how to add a record. + +## For everybody + +1. Fork the repository and make your change on a branch. +2. One pull request does one thing. Do not add three unrelated subdomains in + one pull request. +3. Keep records in alphabetical order inside a zone file. +4. Give every record an owner in a comment on the same line as its name. +5. Read the `octoDNS plan` comment on your pull request before you ask for a + review. It is the exact list of changes the merge will make. +6. Answer review comments on the same pull request. Do not close it and open a + new one. + +### Commit messages + +Use [Conventional Commits](https://www.conventionalcommits.org/): + +``` +feat: add docs.witcc.dev for the club handbook +fix: point api.witcc.dev at the new host +chore: bump octodns to 1.14.0 +``` + +Pull requests are squash merged, so the pull request title becomes the commit +message on `main`. Write the title the same way. + +## For DNS Managers + +You are on the [DNS Managers +team](https://github.com/orgs/WITCodingClub/teams/dns-managers). A pull request +cannot merge without an approving review from one of you. + +### Reviews go round the team + +A workflow asks one person per pull request, in rotation. The rotation is +[`.github/dns-reviewers.txt`](./.github/dns-reviewers.txt). It skips the author +of the pull request. + +Being asked does not make it only your job. Anybody on the team can approve. +If you cannot get to a review, say so on the pull request so that somebody else +picks it up. + +To change the rotation, edit the file and open a pull request. Comment out a +line to pause somebody. Add a line to bring somebody in. + +### What to check in a review + +- **Read the plan comment.** It is the truth about what merging will do. The + diff is not. A small diff can produce a large plan. +- **Does the record have an owner?** Reject it if not. An unowned record is one + nobody can clean up later. +- **Does the name belong to a club thing?** See the README. +- **Does the plan delete anything?** A delete that the author did not mention + is the most common sign of a mistake or a stale branch. +- **Did the pull request touch `config/`, `bin/`, `.github/` or `tools/`?** + Those are not in the plan comment. Read them by hand. + +Approve, then squash merge. The deploy runs by itself. + +### Never change DNS in the Cloudflare dashboard + +Use a pull request. A dashboard change is invisible to everybody who is not +looking at the dashboard, and the next deploy would undo it. + +The nightly sync catches a dashboard change and opens a pull request for it. +That is a safety net, not a workflow. Treat a `cloudflare-drift` pull request +as a thing to explain, not a thing to rubber stamp. + +If you need an emergency change and you cannot wait for a review, make it in +the dashboard, then say so in the club Discord straight away. Merge the sync +pull request the next morning. + +## Changing the tooling + +`tools/` has tests. Run them and keep them passing: + +```console +$ python -m unittest discover -s tools -p 'test_*.py' -v +``` + +Add a test with any change to `tools/merge_live.py`. That script edits the zone +files by itself every night, so a bug in it is a bug in production DNS. + +Never loosen the security note at the top of +[`.github/workflows/plan.yml`](./.github/workflows/plan.yml) without +understanding it. That workflow can read secrets on a pull request from a fork. diff --git a/README.md b/README.md index 39e12c5..ca02d82 100644 --- a/README.md +++ b/README.md @@ -1,144 +1,200 @@ # Wentworth Coding Club DNS -[![test](https://github.com/WITCodingClub/dns/workflows/test/badge.svg)](https://github.com/WITCodingClub/dns/actions?query=workflow%3Atest) -[![deploy](https://github.com/WITCodingClub/dns/workflows/deploy/badge.svg)](https://github.com/WITCodingClub/dns/actions?query=workflow%3Adeploy) +[![validate](https://github.com/WITCodingClub/dns/actions/workflows/validate.yml/badge.svg)](https://github.com/WITCodingClub/dns/actions/workflows/validate.yml) +[![deploy](https://github.com/WITCodingClub/dns/actions/workflows/deploy.yml/badge.svg)](https://github.com/WITCodingClub/dns/actions/workflows/deploy.yml) +[![sync-from-cloudflare](https://github.com/WITCodingClub/dns/actions/workflows/sync-from-cloudflare.yml/badge.svg)](https://github.com/WITCodingClub/dns/actions/workflows/sync-from-cloudflare.yml) -This repository is used for managing the Wentworth Coding Club's DNS configuration through [OctoDNS](https://github.com/octodns/octodns). OctoDNS enables version-controlled, automated DNS management with validation and testing before changes go live. +This repository holds the DNS records for the Wentworth Coding Club. The YAML +files are the source of truth. [octoDNS](https://github.com/octodns/octodns) +applies them to Cloudflare when a pull request merges to `main`. -## Managed Domains +You get a subdomain by opening a pull request. You do not need Cloudflare +access. -- **witcc.dev** - Primary club domain -- **hackwit.org** - For HackWIT Hackathon +## Managed domains -## Adding a Subdomain +| Domain | Zone file | Used for | +|---|---|---| +| `witcc.dev` | [`witcc.dev.yaml`](./witcc.dev.yaml) | The club and its projects | +| `hackwit.org` | [`hackwit.org.yaml`](./hackwit.org.yaml) | The HackWIT hackathon | -### Step 1: Fork the Repository +## Get a subdomain -[Create a fork](https://docs.github.com/en/free-pro-team@latest/github/getting-started-with-github/fork-a-repo) of this repository to your GitHub account. +### 1. Fork and edit -### Step 2: Edit the Domain Configuration File - -Open either [witcc.dev.yaml](./witcc.dev.yaml) or [hackwit.org.yaml](./hackwit.org.yaml) depending on which domain you want to add a subdomain to. - -Add the following entry alphabetically based on the subdomain name: +[Fork this repository](https://github.com/WITCodingClub/dns/fork), then open the +zone file for the domain you want. Add your record in alphabetical order: ```yaml -SUBDOMAIN_NAME: # yourwitemail@wit.edu +docs: # mayonej@wit.edu - ttl: 600 type: CNAME - value: SOURCE_DOMAIN_OR_IP. + value: docs-site.netlify.app. ``` -### Step 3: Configure Your Subdomain +That creates `docs.witcc.dev` and points it at `docs-site.netlify.app`. + +Three rules decide whether it works: + +- **The name is the part before the domain.** `docs` becomes `docs.witcc.dev`. +- **A `CNAME` value ends with a dot.** An `A` or `AAAA` value does not. +- **Every record needs an owner.** Put a WIT email in a comment on the same + line as the name. We use it to find out who to ask when the record breaks. + List more than one person if more than one person is responsible. + +### 2. Open a pull request + +A bot adds two things to your pull request: + +- **A plan.** It lists every record the merge would create, change, or delete. + Read it. If it shows something you did not intend, fix your branch. +- **A reviewer.** The rotation in + [`.github/dns-reviewers.txt`](./.github/dns-reviewers.txt) decides whose turn + it is. -- **SUBDOMAIN_NAME**: Replace with your desired subdomain name - - Example: `hello` would create `hello.witcc.dev` -- **SOURCE_DOMAIN_OR_IP**: Replace with the target domain or IP address - - For domains: Use `CNAME` and include the trailing `.` - - Example: `example.com.` - - For IP addresses: Change `type: CNAME` to `type: A` and remove the trailing `.` - - Example: `192.0.2.1` -- **Contact info**: Add your wit email in a comment above your entry. This way we know who is responsible for the subdomain. If you're making the PR but it makes more sense for someone else to "own" the subdomain, you can add their email there instead. Feel free to list multiple people. +Push more commits to the same branch if the reviewer asks for changes. Do not +close the pull request and open a new one. -### Example Configurations +### 3. Wait for the deploy + +Cloudflare gets the change within about a minute of the merge. Most resolvers +follow within the TTL. A few take up to 24 hours. + +## Record types + +| Type | Points at | Example value | +|---|---|---| +| `A` | An IPv4 address | `192.0.2.1` | +| `AAAA` | An IPv6 address | `2001:db8::1` | +| `CNAME` | Another domain | `example.com.` | +| `TXT` | Text, for verification or SPF | `"a-verification-string"` | +| `MX` | A mail server | See the zone file for the current setup | + +### More than one record on one name -#### CNAME Record (Domain) ```yaml -myproject: # mayonej@wit.edu +docs: # mayonej@wit.edu, lambertl@wit.edu - ttl: 600 type: CNAME - value: myproject.vercel.app. + value: docs-site.netlify.app. + - ttl: 600 + type: TXT + value: "a-verification-string" ``` -#### A Record (IP Address) +### Behind the Cloudflare proxy + +Add the `octodns` block to put a record behind Cloudflare: + ```yaml -server: # lambertl@wit.edu - - ttl: 600 +app: # mayonej@wit.edu + - ttl: 300 type: A value: 192.0.2.1 + octodns: + cloudflare: + proxied: true ``` -#### Multiple Records -```yaml -docs: # team@hackwit.org, mayonej@wit.org, lambertl@wit.edu - - ttl: 600 - type: CNAME - value: docs-site.netlify.app. - - ttl: 600 - type: TXT - value: "verification-token-here" +## How it works + +```mermaid +flowchart TD + A[You edit a zone file] --> B[Pull request] + B --> C{validate} + B --> D{plan} + B --> E{cloudflare in sync} + C -->|YAML and records are valid| F + D -->|Posts the plan as a comment| F + E -->|Cloudflare still matches main| F[Review by DNS Managers] + F -->|Approved and squash merged| G[deploy] + G --> H[(Cloudflare)] + + I[Nightly sync] -->|Reads Cloudflare| H + I -->|Finds a manual change| J[Opens a pull request] + J --> F ``` -### Step 4: Submit Pull Request +Four workflows do the work: + +| Workflow | Runs when | Does what | +|---|---|---| +| [`validate`](./.github/workflows/validate.yml) | Every pull request | Checks the YAML, every record, and the tools tests. Holds no secrets, so it is safe on forks. | +| [`plan`](./.github/workflows/plan.yml) | Every pull request | Posts the plan as a comment, and fails if Cloudflare has drifted away from `main`. | +| [`deploy`](./.github/workflows/deploy.yml) | Push to `main` | Applies the zone files to Cloudflare, then confirms they match. | +| [`sync-from-cloudflare`](./.github/workflows/sync-from-cloudflare.yml) | Nightly | Pulls manual Cloudflare changes back into the zone files as a pull request. | -1. Commit your changes to your fork -2. [Create a pull request](https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request-from-a-fork) back to the main repository -3. Wait for a maintainer to review your PR +### Why the nightly sync exists -**Note**: If changes are requested, update your existing PR by committing to your fork rather than closing and creating a new one. +Cloudflare lets a club officer change a record in the dashboard. octoDNS does +not know about that change, so the next deploy would undo it. -## Common Record Types +The nightly job reads Cloudflare, folds anything new into the zone files, and +opens a pull request. The change keeps its history and gets an owner. The +`cloudflare in sync` check blocks other merges until that pull request lands, +so nobody can overwrite the change by accident. -| Type | Usage | Example | -|------|-------|---------| -| **A** | Points to an IPv4 address | `value: 192.0.2.1` | -| **AAAA** | Points to an IPv6 address | `value: 2001:0db8::1` | -| **CNAME** | Points to another domain | `value: example.com.` | -| **TXT** | Text records (verification, SPF, etc.) | `value: "verification-string"` | -| **MX** | Mail server records | See email configuration | +See [`docs/runbook.md`](./docs/runbook.md) for what to do when a check fails. +### The `octodns-meta` record +Every zone has a `octodns-meta` TXT record. octoDNS writes it on each deploy. +Use it to confirm that a deploy reached Cloudflare: -## Testing Changes Locally +```console +$ dig +short TXT octodns-meta.witcc.dev +"octodns-version=1.14.0" "provider=cloudflare" "time=2026-09-21T07:17:03+00:00" +``` -If you want to validate your changes before submitting a PR: +It only changes when something else in the zone changes, so it does not create +a deploy of its own every night. Do not add it to a zone file. The nightly sync +leaves it out on purpose. -### Prerequisites +## Work on this locally -```bash -# Install Python 3 and pip -# Install OctoDNS and the Cloudflare provider -pip install 'octodns>=1.5.0' octodns-cloudflare -``` +You do not need a Cloudflare token to check your own change. -### Validate Configuration +```console +$ python3 -m venv env +$ ./env/bin/pip install -r requirements.txt +$ export PATH="$PWD/env/bin:$PATH" -```bash -# Run a dry-run to check for errors -./bin/dry-run +$ ./bin/validate # parse the config and check every record +$ ./bin/zones # list the zones this repository manages ``` -This will validate your YAML syntax and check for DNS configuration errors without making any actual changes. +These need a Cloudflare token in `CLOUDFLARE_TOKEN`. Ask a DNS Manager for a +read-only one. -## How It Works +```console +$ ./bin/plan # show what a deploy would change +$ ./bin/dump .live # write the live Cloudflare state to .live/ +``` + +`./bin/sync` applies to production. Only the `deploy` workflow should run it. -1. **Configuration**: DNS records are defined in YAML files (witcc.dev.yaml, hackwit.org.yaml) -2. **Validation**: GitHub Actions automatically validates changes on every PR -3. **Review**: A maintainer reviews and approves your changes -4. **Deployment**: Upon merge to main, changes are automatically deployed to Cloudflare -5. **Propagation**: DNS changes typically propagate within minutes but can take up to 24 hours +Run the tests for the sync tooling: -## Project Eligibility +```console +$ ./env/bin/python -m unittest discover -s tools -p 'test_*.py' -v +``` -Subdomains are available for: -- Official Wentworth Coding Club projects -- Club-affiliated events and initiatives +## Who can approve and merge -For questions about eligibility, reach out to a club E-Board member on discord. +The [DNS Managers](https://github.com/orgs/WITCodingClub/teams/dns-managers) +team owns every file in this repository. A pull request needs an approving +review from that team before it can merge. -## Need Help? +[`CONTRIBUTING.md`](./CONTRIBUTING.md) covers the review rules. +[`docs/runbook.md`](./docs/runbook.md) covers the jobs a DNS Manager has to do. -- Check the [OctoDNS documentation](https://github.com/octodns/octodns) -- Open an issue in this repository -- Ask in the club's Discord -- Contact a repository maintainer (primarilly @jaspermayone) +## Who can have a subdomain -## Contributing +Subdomains are for club projects, club events, and club services. Ask an +E-Board member on Discord if you are not sure whether yours counts. -We welcome contributions! Please: -- Follow the existing format and alphabetical ordering -- Include your contact information in comments -- Provide a clear description in your PR -- Be responsive to review feedback +## Get help ---- +- Open an [issue](https://github.com/WITCodingClub/dns/issues/new/choose). +- Ask in the club Discord. +- Read the [octoDNS documentation](https://octodns.readthedocs.io/en/stable/). diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..6d0d2bb --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,50 @@ +# Security + +## Reporting a problem + +Do not open a public issue for a security problem. Email +`mayonej@wit.edu`, or send a direct message to a DNS Manager in the club +Discord. + +Tell us what you found and how to reproduce it. We will reply within a few +days. + +## What matters here + +This repository controls DNS for `witcc.dev` and `hackwit.org`. Somebody who +can change a record can point a club name at a host they control. Treat these +as serious: + +- A way to make a workflow run code from a pull request while it holds a + Cloudflare token. +- A way to merge to `main` without an approving review from a DNS Manager. +- A leaked Cloudflare token. + +## How the workflows protect the tokens + +`plan.yml` runs on `pull_request_target`, so it can read secrets even for a +pull request from a fork. It only stays safe because of one rule: + +> It checks out `main` and runs `main`'s scripts. From the pull request it +> copies only the zone files at the repository root, which are data. + +Anything that runs code, an action, or a dependency install from the pull +request breaks that rule and hands the Cloudflare token to whoever opened the +pull request. `validate.yml` is the workflow that runs against pull request +code, and it holds no secrets. + +The token used for a plan is read only. Only `deploy.yml`, which runs after a +merge, uses a token that can write. + +## If a token leaks + +1. Delete the token in the Cloudflare dashboard. This takes effect at once. +2. Create a new one and update the repository secret. +3. Compare Cloudflare against this repository: + + ```console + $ gh workflow run sync-from-cloudflare.yml + ``` + + Any record an attacker added shows up in the pull request it opens. +4. Check the Cloudflare audit log for what the token did. diff --git a/docs/runbook.md b/docs/runbook.md new file mode 100644 index 0000000..ecce5f4 --- /dev/null +++ b/docs/runbook.md @@ -0,0 +1,171 @@ +# DNS runbook + +For DNS Managers. It covers the one time setup and the things that go wrong. + +## One time setup + +Do these in order. The order matters, because a required check that does not +exist yet blocks every merge. + +### 1. Secrets + +| Secret | What it is | Used by | +|---|---|---| +| `CLOUDFLARE_TOKEN` | Cloudflare API token, **edit** DNS for both zones | `deploy` | +| `CLOUDFLARE_TOKEN_READ_ONLY` | Cloudflare API token, **read** DNS for both zones | `plan`, `sync-from-cloudflare` | +| `DNS_BOT_TOKEN` | Fine grained personal access token | `sync-from-cloudflare` | + +The two Cloudflare tokens already exist. Create them at +**Cloudflare > My Profile > API Tokens** with the `Edit zone DNS` template, and +scope each one to `witcc.dev` and `hackwit.org` only. + +`DNS_BOT_TOKEN` is new and you have to create it. GitHub does not start +workflows for commits pushed with the built in `GITHUB_TOKEN`. Without this +token, the nightly sync pull request gets no checks, so it can never satisfy a +required check and can never merge. + +1. Go to **Settings > Developer settings > Personal access tokens > Fine + grained tokens** on an account that is a DNS Manager. +2. Resource owner: `WITCodingClub`. Repository access: only `WITCodingClub/dns`. +3. Repository permissions: `Contents: Read and write`, + `Pull requests: Read and write`, `Issues: Read and write`. +4. Set an expiry you will remember. Put a reminder in the club calendar. +5. Save it as the `DNS_BOT_TOKEN` repository secret. + +### 2. Team access + +The `dns-managers` team needs **write** access or better on this repository. +GitHub ignores a `CODEOWNERS` entry for a team that cannot write. + +```console +$ gh api orgs/WITCodingClub/teams/dns-managers/repos/WITCodingClub/dns \ + --jq .permissions +``` + +### 3. Required checks and code owner review + +Do this **after** these workflows are on `main`. A required check that has +never run on `main` leaves every pull request waiting forever. + +The `main` ruleset already requires one approving review, squash merge, and +resolved review threads. Add code owner review and the three checks: + +```console +$ gh api -X PUT repos/WITCodingClub/dns/rulesets/9465567 \ + --input docs/ruleset-main.json +``` + +Then check it: + +```console +$ gh api repos/WITCodingClub/dns/rulesets/9465567 --jq '.rules[] | select(.type=="pull_request" or .type=="required_status_checks")' +``` + +### 4. Turn on the nightly sync + +The schedule starts by itself once the workflow is on `main`. Run it once by +hand first, so that the first drift pull request appears while you are watching: + +```console +$ gh workflow run sync-from-cloudflare.yml +$ gh run watch +``` + +Expect the first run to open a large pull request. The zone files are nearly +empty today and Cloudflare holds the real records. + +## The checks + +### `cloudflare in sync` failed + +Cloudflare does not match `main`. Somebody changed DNS in the dashboard, or a +deploy failed. + +1. Look at the failed check. It prints the difference. +2. If there is an open `cloudflare-sync` pull request, review and merge it. +3. If there is not, make one: + + ```console + $ gh workflow run sync-from-cloudflare.yml + ``` + +4. Re-run the failed check on the blocked pull request. + +Do not merge past this check. Merging undoes whatever is in Cloudflare and not +in `main`. + +### `plan` failed + +Read the workflow log. The usual causes: + +- **A record is not valid.** octoDNS names the record and says why. +- **The Cloudflare token expired.** The log shows a 401 or 403 from the API. +- **A root `NS` change.** octoDNS refuses these without `--force`. Cloudflare + owns the nameservers for a zone. The zone files must not contain root `NS` + records. `tools/merge_live.py` leaves them out for this reason. + +### `deploy` failed + +The zone files and Cloudflare now disagree. Fix it, do not leave it. + +- **`TooMuchChange`.** octoDNS refuses a plan that updates or deletes more than + 30% of a zone. This is the safety net working. Read the plan. If the change + really is correct, apply it by hand: + + ```console + $ export CLOUDFLARE_TOKEN=... # the edit token + $ ./bin/plan # read this first + $ ./bin/sync --force + ``` + +- **Rate limited.** octoDNS retries five times and waits ten minutes. Re-run + the workflow. +- **The apply half finished.** Run the `deploy` workflow again. octoDNS works + out what is left to do. + +## Common jobs + +### Roll a Cloudflare token + +1. Create the new token in Cloudflare. +2. Update the repository secret. +3. Run `gh workflow run deploy.yml` and confirm it passes. +4. Delete the old token in Cloudflare. + +### Add somebody to the rotation + +1. Add them to the `dns-managers` team. +2. Add their GitHub username to `.github/dns-reviewers.txt` in a pull request. + +### Remove a subdomain + +Delete the record from the zone file in a pull request. The deploy removes it +from Cloudflare. Tell the owner first. + +### Check that a deploy landed + +```console +$ dig +short TXT octodns-meta.witcc.dev +``` + +The `time=` value is when octoDNS last changed that zone. + +### See the live Cloudflare state + +```console +$ export CLOUDFLARE_TOKEN=... # the read-only token is enough +$ ./bin/dump .live +$ cat .live/witcc.dev.yaml +``` + +`.live/` is ignored by git. It never touches the zone files. + +## If everything is broken + +DNS for both zones is in Cloudflare. Cloudflare is the live system. This +repository is how we change it, not how it serves. + +1. Fix the record in the Cloudflare dashboard. The site comes back. +2. Post in the club Discord that you did it. +3. Run `gh workflow run sync-from-cloudflare.yml` and merge the pull request it + opens, so the repository catches up. From 2cf9fe586b47d5abffbeaadc7dc6ee60a738dd9e Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 14:05:41 -0400 Subject: [PATCH 7/8] fix: block a mass delete and pin octodns to the current release --- .github/workflows/deploy.yml | 36 ++++++++++++++++- README.md | 2 +- docs/runbook.md | 76 ++++++++++++++++++++++++++---------- requirements.txt | 6 +-- 4 files changed, 95 insertions(+), 25 deletions(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 3bd412d..8448b9f 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -13,10 +13,23 @@ on: - 'requirements.txt' - '.github/workflows/deploy.yml' workflow_dispatch: + inputs: + allow_mass_delete: + description: 'Apply even when the plan deletes more records than the limit' + type: boolean + default: false permissions: contents: read +env: + # octoDNS refuses a plan that deletes more than 30% of a zone, but only for a + # zone that already has at least 10 records. MIN_EXISTING_RECORDS is a + # constant in octoDNS and cannot be configured. hackwit.org has fewer records + # than that, so octoDNS would delete every one of them without complaining. + # This is the guard for that case. + MAX_DELETES: '3' + # Never let two deploys apply to Cloudflare at the same time, and never cancel # one halfway through. concurrency: @@ -44,7 +57,28 @@ jobs: - name: Show what will be applied env: CLOUDFLARE_TOKEN: ${{ secrets.CLOUDFLARE_TOKEN }} - run: ./bin/plan | tee "$GITHUB_STEP_SUMMARY" + run: | + set -eu + ./bin/plan > plan.md + cat plan.md >> "$GITHUB_STEP_SUMMARY" + cat plan.md + + - name: Refuse a mass delete + if: inputs.allow_mass_delete != true + run: | + set -eu + deletes=$(grep -c '^| Delete |' plan.md || true) + echo "The plan deletes $deletes records. The limit is $MAX_DELETES." + + if [ "$deletes" -gt "$MAX_DELETES" ]; then + echo "::error title=Too many deletes::The plan deletes $deletes records, and the limit is $MAX_DELETES. Nothing has been applied." + echo + echo "Read the plan in the job summary. If every delete is correct," + echo "run this workflow by hand with 'allow_mass_delete' turned on:" + echo + echo " gh workflow run deploy.yml -f allow_mass_delete=true" + exit 1 + fi - name: Apply to Cloudflare env: diff --git a/README.md b/README.md index ca02d82..34e7a93 100644 --- a/README.md +++ b/README.md @@ -121,7 +121,7 @@ Four workflows do the work: |---|---|---| | [`validate`](./.github/workflows/validate.yml) | Every pull request | Checks the YAML, every record, and the tools tests. Holds no secrets, so it is safe on forks. | | [`plan`](./.github/workflows/plan.yml) | Every pull request | Posts the plan as a comment, and fails if Cloudflare has drifted away from `main`. | -| [`deploy`](./.github/workflows/deploy.yml) | Push to `main` | Applies the zone files to Cloudflare, then confirms they match. | +| [`deploy`](./.github/workflows/deploy.yml) | Push to `main` | Applies the zone files to Cloudflare, then confirms they match. Refuses a plan that deletes more than three records. | | [`sync-from-cloudflare`](./.github/workflows/sync-from-cloudflare.yml) | Nightly | Pulls manual Cloudflare changes back into the zone files as a pull request. | ### Why the nightly sync exists diff --git a/docs/runbook.md b/docs/runbook.md index ecce5f4..5f55f44 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -4,8 +4,16 @@ For DNS Managers. It covers the one time setup and the things that go wrong. ## One time setup -Do these in order. The order matters, because a required check that does not -exist yet blocks every merge. +Do these in order. The order matters twice over. A required check that has +never run on `main` blocks every merge. And the zone files do not yet hold what +Cloudflare holds, so the drift check fails until the first sync pull request +lands. + +> **The zone files are nearly empty and Cloudflare is not.** As of this +> writing, `witcc.dev` has 14 records in Cloudflare and one in the zone file. +> `hackwit.org` has 4 records in Cloudflare and none in the zone file. Those +> include the `MX`, SPF, DMARC and DKIM records for club email. Step 3 is what +> fixes this. Do not run `./bin/sync --force` before it. ### 1. Secrets @@ -19,6 +27,10 @@ The two Cloudflare tokens already exist. Create them at **Cloudflare > My Profile > API Tokens** with the `Edit zone DNS` template, and scope each one to `witcc.dev` and `hackwit.org` only. +> **Rotate `CLOUDFLARE_TOKEN_READ_ONLY` once.** The workflow it replaces ran +> scripts from a pull request while holding it, so anybody who opened a pull +> request could have read it. See [`SECURITY.md`](../SECURITY.md). + `DNS_BOT_TOKEN` is new and you have to create it. GitHub does not start workflows for commits pushed with the built in `GITHUB_TOKEN`. Without this token, the nightly sync pull request gets no checks, so it can never satisfy a @@ -42,10 +54,31 @@ $ gh api orgs/WITCodingClub/teams/dns-managers/repos/WITCodingClub/dns \ --jq .permissions ``` -### 3. Required checks and code owner review +### 3. Pull Cloudflare into git + +Run the nightly sync by hand, then review and merge the pull request it opens. +Expect it to be large. Cloudflare holds the real records today. + +```console +$ gh workflow run sync-from-cloudflare.yml +$ gh run watch +``` + +Give every record marked `TODO owner unknown` an owner before you merge. + +Once it has merged, confirm that Cloudflare and `main` agree: + +```console +$ export CLOUDFLARE_TOKEN=... # the read-only token +$ ./bin/plan +``` + +It should print `## No changes were planned`. Until it does, the +`cloudflare in sync` check fails on every pull request, which is the point. -Do this **after** these workflows are on `main`. A required check that has -never run on `main` leaves every pull request waiting forever. +### 4. Required checks and code owner review + +Do this **last**, after the workflows are on `main` and step 3 has landed. The `main` ruleset already requires one approving review, squash merge, and resolved review threads. Add code owner review and the three checks: @@ -61,18 +94,8 @@ Then check it: $ gh api repos/WITCodingClub/dns/rulesets/9465567 --jq '.rules[] | select(.type=="pull_request" or .type=="required_status_checks")' ``` -### 4. Turn on the nightly sync - -The schedule starts by itself once the workflow is on `main`. Run it once by -hand first, so that the first drift pull request appears while you are watching: - -```console -$ gh workflow run sync-from-cloudflare.yml -$ gh run watch -``` - -Expect the first run to open a large pull request. The zone files are nearly -empty today and Cloudflare holds the real records. +The nightly schedule starts by itself once the workflow is on `main`. There is +nothing else to turn on. ## The checks @@ -108,9 +131,22 @@ Read the workflow log. The usual causes: The zone files and Cloudflare now disagree. Fix it, do not leave it. -- **`TooMuchChange`.** octoDNS refuses a plan that updates or deletes more than - 30% of a zone. This is the safety net working. Read the plan. If the change - really is correct, apply it by hand: +- **`Too many deletes`.** The deploy refuses a plan that deletes more than + `MAX_DELETES` records. Read the plan in the job summary. If every delete is + correct, run the deploy by hand: + + ```console + $ gh workflow run deploy.yml -f allow_mass_delete=true + ``` + + This guard exists because octoDNS's own guard has a hole. octoDNS refuses a + plan that updates or deletes more than 30% of a zone, but only for a zone + that already has at least 10 records. `MIN_EXISTING_RECORDS` is a constant in + octoDNS and cannot be configured. `hackwit.org` has fewer records than that, + so octoDNS would delete every one of them without complaining. + +- **`TooMuchChange`.** This is octoDNS's own guard, for a zone with 10 records + or more. Read the plan. If the change really is correct, apply it by hand: ```console $ export CLOUDFLARE_TOKEN=... # the edit token diff --git a/requirements.txt b/requirements.txt index 3922eb5..a27b100 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,8 +1,8 @@ # Pinned so that a nightly job and a pull request plan always agree. # Bump with a pull request, never in place. -octodns==1.14.0 -octodns-cloudflare==1.0.0 +octodns==1.22.0 +octodns-cloudflare==1.2.0 # Used by tools/merge_live.py to merge Cloudflare state back into the zone # files without destroying the ownership comments. -ruamel.yaml==0.18.6 +ruamel.yaml==0.19.1 From dee35ac55a0893234892b744b2f9be7eb833e2f0 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 14:28:56 -0400 Subject: [PATCH 8/8] feat: enforce record order and match octodns natural sort --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- CONTRIBUTING.md | 4 ++- README.md | 22 ++++++++++--- config/config.yaml | 13 ++++++-- docs/ruleset-main.json | 5 +++ docs/runbook.md | 23 +++++++++---- requirements.txt | 5 +++ tools/merge_live.py | 9 +++++- tools/test_merge_live.py | 55 ++++++++++++++++++++++++++++++++ witcc.dev.yaml | 6 ++-- 10 files changed, 125 insertions(+), 19 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index dfae5b3..8637416 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -18,7 +18,7 @@ know who to ask when it breaks. For example: - [ ] The record is for a club project, event, or service. - [ ] Every record I added or changed has an owner in a comment. -- [ ] Records are in alphabetical order in the file. +- [ ] `./bin/validate` passes, so the records are in the order octoDNS wants. - [ ] CNAME values end with a dot. A and AAAA values do not. - [ ] I have read the `octoDNS plan` comment on this pull request and it changes only what I expected. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9f29552..ef4297b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -8,7 +8,9 @@ Thank you for helping run the club's DNS. This file covers the rules. The 1. Fork the repository and make your change on a branch. 2. One pull request does one thing. Do not add three unrelated subdomains in one pull request. -3. Keep records in alphabetical order inside a zone file. +3. Keep records in order inside a zone file. The `dns records` check enforces + it. The order is natural, so `ns2` comes before `ns10`, and inside a record + `octodns` comes before `ttl`, `type` and `value`. Run `./bin/validate`. 4. Give every record an owner in a comment on the same line as its name. 5. Read the `octoDNS plan` comment on your pull request before you ask for a review. It is the exact list of changes the merge will make. diff --git a/README.md b/README.md index 34e7a93..eaa3033 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,18 @@ Three rules decide whether it works: line as the name. We use it to find out who to ask when the record breaks. List more than one person if more than one person is responsible. +### Order is checked + +The `dns records` check fails on a zone file that is out of order, so this is +a rule and not a request. The order is **natural**, not plain alphabetical: + +- Records go in order by name, and `ns2` comes before `ns10`. +- The apex record, written `""`, comes first. +- Inside a record, `octodns` comes before `ttl`, `type` and `value`. + +Run `./bin/validate` to check before you push. The nightly sync writes files +in this order by itself. + ### 2. Open a pull request A bot adds two things to your pull request: @@ -88,14 +100,16 @@ Add the `octodns` block to put a record behind Cloudflare: ```yaml app: # mayonej@wit.edu - - ttl: 300 - type: A - value: 192.0.2.1 - octodns: + - octodns: cloudflare: proxied: true + ttl: 300 + type: A + value: 192.0.2.1 ``` +`octodns` comes before `ttl`. See **Order is checked** below. + ## How it works ```mermaid diff --git a/config/config.yaml b/config/config.yaml index 4f400d6..7dc21dc 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -43,9 +43,16 @@ providers: config: class: octodns.provider.yaml.YamlProvider directory: ./ - # Records do not have to be alphabetical. Keep them alphabetical anyway, - # because it keeps diffs small. - enforce_order: False + # Records must be in order, and so must the keys inside a record. This is + # checked, not asked for: `bin/validate` fails on a file that is out of + # order. It keeps diffs small and stops two people adding the same name in + # two places. + # + # The order is natural, not plain alphabetical, so `ns2` comes before + # `ns10`. Inside a record it puts `octodns` before `ttl`, `type` and + # `value`. `tools/merge_live.py` and `octodns-dump` both write it this way. + enforce_order: True + order_mode: natural cloudflare: class: octodns_cloudflare.CloudflareProvider token: env/CLOUDFLARE_TOKEN diff --git a/docs/ruleset-main.json b/docs/ruleset-main.json index c124ac9..54ebae1 100644 --- a/docs/ruleset-main.json +++ b/docs/ruleset-main.json @@ -15,6 +15,11 @@ "actor_id": 5, "actor_type": "RepositoryRole", "bypass_mode": "always" + }, + { + "actor_id": 65788728, + "actor_type": "User", + "bypass_mode": "always" } ], "rules": [ diff --git a/docs/runbook.md b/docs/runbook.md index 5f55f44..252c158 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -78,22 +78,33 @@ It should print `## No changes were planned`. Until it does, the ### 4. Required checks and code owner review -Do this **last**, after the workflows are on `main` and step 3 has landed. +**This is already applied.** Ruleset `23783065` on `main` requires one +approving review from a code owner, squash merge, resolved review threads, and +these five checks: -The `main` ruleset already requires one approving review, squash merge, and -resolved review threads. Add code owner review and the three checks: +`yaml syntax`, `dns records`, `tools tests`, `plan the change`, +`cloudflare in sync`. + +`docs/ruleset-main.json` is a copy of it. Restore it with: ```console -$ gh api -X PUT repos/WITCodingClub/dns/rulesets/9465567 \ +$ gh api -X PUT repos/WITCodingClub/dns/rulesets/23783065 \ --input docs/ruleset-main.json ``` -Then check it: +Read it back with: ```console -$ gh api repos/WITCodingClub/dns/rulesets/9465567 --jq '.rules[] | select(.type=="pull_request" or .type=="required_status_checks")' +$ gh api repos/WITCodingClub/dns/rulesets/23783065 --jq '.rules[] | select(.type=="pull_request" or .type=="required_status_checks")' ``` +> **The ruleset went on before the workflows did.** `plan.yml` runs on +> `pull_request_target`, which reads the workflow from `main`. Until it is on +> `main`, `plan the change` and `cloudflare in sync` never start, so any pull +> request open right now waits on checks that cannot run. A repository admin +> has `bypass_mode: always` and can merge the first one anyway. Every pull +> request after that gets real checks. + The nightly schedule starts by itself once the workflow is on `main`. There is nothing else to turn on. diff --git a/requirements.txt b/requirements.txt index a27b100..bffb0f5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -6,3 +6,8 @@ octodns-cloudflare==1.2.0 # Used by tools/merge_live.py to merge Cloudflare state back into the zone # files without destroying the ownership comments. ruamel.yaml==0.19.1 + +# Imported directly by tools/merge_live.py so that it sorts records exactly +# the way octoDNS's enforce_order checks them. It arrives with octodns anyway, +# but a direct import deserves a direct pin. +natsort==8.4.0 diff --git a/tools/merge_live.py b/tools/merge_live.py index cd350bb..736a29f 100755 --- a/tools/merge_live.py +++ b/tools/merge_live.py @@ -28,9 +28,15 @@ from datetime import date from pathlib import Path +from natsort import natsort_keygen from ruamel.yaml import YAML from ruamel.yaml.comments import CommentedMap +# octoDNS checks record order with a natural sort, so `ns2` comes before +# `ns10`. Plain `sorted` gets that backwards and would write a file that +# `bin/validate` then rejects. Use the same key octoDNS uses. +_natsort_key = natsort_keygen() + # Records that octoDNS itself manages. They live in Cloudflare but never in the # zone files, so merging them back would create an endless nightly diff. DEFAULT_IGNORED = ("octodns-meta",) @@ -138,7 +144,8 @@ def merge_zone(zone, live_dir, repo_dir, ignored, today, keep_root_ns=False): return [], [], [] merged = CommentedMap() - for key in sorted(live, key=lambda k: (k != "", k)): + # 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: diff --git a/tools/test_merge_live.py b/tools/test_merge_live.py index 0eb64e5..fe1b018 100644 --- a/tools/test_merge_live.py +++ b/tools/test_merge_live.py @@ -119,6 +119,61 @@ def test_octodns_meta_is_ignored(self): self.assertEqual(([], [], []), (added, updated, removed)) self.assertNotIn("octodns-meta", self.result()) + def test_output_passes_octodns_enforce_order(self): + # This is the test that matters. config/config.yaml sets + # enforce_order: True, so a file this script writes has to be one + # octoDNS will load, or the nightly sync breaks every later build. + from octodns.yaml import safe_load + + self.write_live( + """--- +zeta: # lambertl@wit.edu + - ttl: 600 + type: A + value: 192.0.2.1 +ns10: + - ttl: 600 + type: A + value: 192.0.2.10 +ns2: + - ttl: 600 + type: A + value: 192.0.2.2 +"": + - ttl: 300 + type: A + value: 192.0.2.5 +api: # mayonej@wit.edu + - octodns: + cloudflare: + proxied: true + ttl: 600 + type: A + value: 192.0.2.9 +""" + ) + self.merge() + safe_load(self.result(), enforce_order=True, order_mode="natural") + + def test_records_use_natural_order_not_plain_alphabetical(self): + # natsort puts ns2 before ns10. Plain sorted() does the opposite and + # octoDNS would reject the file. + self.write_live( + """--- +ns10: + - ttl: 600 + type: A + value: 192.0.2.10 +ns2: + - ttl: 600 + type: A + value: 192.0.2.2 +""" + ) + self.merge() + result = self.result() + self.assertLess(result.index("ns2:"), result.index("ns10:")) + def test_records_are_sorted_with_the_root_first(self): self.write_live( """--- diff --git a/witcc.dev.yaml b/witcc.dev.yaml index 2a8cb33..59ad1d5 100644 --- a/witcc.dev.yaml +++ b/witcc.dev.yaml @@ -1,9 +1,9 @@ --- strings: + - octodns: + cloudflare: + auto-ttl: true ttl: 300 type: A value: 129.213.163.213 - octodns: - cloudflare: - auto-ttl: true