diff --git a/.github/ISSUE_TEMPLATE/subdomain-request.yml b/.github/ISSUE_TEMPLATE/subdomain-request.yml index f741932..b85a7b8 100644 --- a/.github/ISSUE_TEMPLATE/subdomain-request.yml +++ b/.github/ISSUE_TEMPLATE/subdomain-request.yml @@ -32,8 +32,8 @@ body: id: owner attributes: label: Who owns it? - description: The Patchwork email of whoever we should ask when it breaks. - placeholder: yourname@patchworklabs.org + description: The Patchwork id and email of whoever we should ask when it breaks. + placeholder: PWL0123456789 / yourname@patchworklabs.org validations: required: true diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index ad71c8e..cc8acb4 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -8,7 +8,7 @@ Every record needs an owner in a comment on the same line as its name, so we know who to ask when it breaks. For example: - docs: # yourname@patchworklabs.org + docs: # PWL0123456789 / yourname@patchworklabs.org - ttl: 600 type: CNAME value: docs-site.netlify.app. @@ -17,7 +17,8 @@ know who to ask when it breaks. For example: ## Checklist - [ ] The record is for a Patchwork project, event, or service. -- [ ] Every record I added or changed has an owner in a comment. +- [ ] Every record I added or changed has an owner in a comment: an email, a + Patchwork id (`PWL...`), or both. - [ ] `./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 diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 211dca3..c7bd370 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -25,9 +25,8 @@ permissions: 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. hackathon.help has fewer records - # than that, so octoDNS would delete every one of them without complaining. - # This is the guard for that case. + # constant in octoDNS and cannot be configured. A zone with fewer records than + # that has no guard from octoDNS at all. This is the guard for that case. MAX_DELETES: '3' # Never let two deploys apply to Cloudflare at the same time, and never cancel diff --git a/CLAUDE.md b/CLAUDE.md index 8ba0780..58e4192 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,18 +4,16 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Project Overview -This is a DNS management repository using OctoDNS to manage DNS records for two domains: -- `patchworklabs.org` -- `hackathon.help` +This is a DNS management repository using OctoDNS to manage DNS records for `patchworklabs.org`. The repository uses OctoDNS with Cloudflare as the DNS provider and YAML configuration files to define DNS records declaratively. The setup mirrors `WITCodingClub/dns`. `README.md`, `CONTRIBUTING.md` and `docs/runbook.md` are the full reference. ## Architecture - **Configuration**: `config/config.yaml` defines providers, processors and zone mappings. `enforce_order` with `order_mode: natural` is on. -- **DNS Records**: Domain-specific YAML files (`patchworklabs.org.yaml`, `hackathon.help.yaml`) contain DNS record definitions. Every new record needs an owner email in a comment on the same line as its name. +- **DNS Records**: The zone file `patchworklabs.org.yaml` contains DNS record definitions. Every record needs an owner (an email, a Patchwork id such as `PWL7A1CE1F3CB`, or both; a GitHub team for team-owned records) in a comment on the same line as its name. - **Scripts**: Shell scripts in `bin/` handle DNS operations. They read the zone list from `config/config.yaml` through `bin/zones`. -- **Tools**: `tools/merge_live.py` merges live Cloudflare state back into the zone files and keeps comments. Tests are in `tools/test_merge_live.py`. +- **Tools**: `tools/merge_live.py` merges live Cloudflare state back into the zone files and keeps comments. `tools/check_zones.py` checks the repository rules (owner comment on every record, TTL of 120 or more, no apex NS, no `octodns-meta`, proxy only on A/AAAA/CNAME). `./bin/validate` runs it. Tests are in `tools/test_*.py`. - **Workflows**: `validate` (no secrets), `plan` (`pull_request_target`, posts the plan and checks drift), `deploy` (push to `main`), `sync-from-cloudflare` (nightly). - **Dependencies**: Python dependencies pinned in `requirements.txt`. @@ -39,7 +37,7 @@ export CLOUDFLARE_TOKEN=YOUR_READ_ONLY_TOKEN_HERE ## Configuration Structure - **Provider Config**: Cloudflare provider configured in `config/config.yaml` with API token from the `CLOUDFLARE_TOKEN` environment variable -- **Zone Sources**: Both domains use the YAML provider as source and Cloudflare as target +- **Zone Sources**: The zone uses the YAML provider as source and Cloudflare as target - **Meta record**: The `meta` processor writes an `octodns-meta` TXT record on each deploy. Do not add it to a zone file. - **DNS Records**: Defined in domain-specific YAML files with standard DNS record types (MX, TXT, CNAME, etc.) @@ -78,11 +76,12 @@ Use appropriate TTL values based on record type and change frequency: ### Record Comments Format ```yaml # Google Workspace email routing - DO NOT MODIFY without IT approval -mx: - values: - - exchange: mx1.example.com - priority: 10 +"": # @patchworklabsorg/infra ttl: 3600 + type: MX + values: + - exchange: mx1.example.com. + preference: 10 ``` ## Important Notes @@ -91,4 +90,4 @@ mx: - Never change DNS in the Cloudflare dashboard. Use a pull request. The nightly sync opens a `cloudflare-sync` pull request for any dashboard change. - The `--force` flag bypasses safety checks and should be used carefully - DNS records include critical configurations like DMARC, Google Site Verification, and email routing -- Both domains are owned by Patchwork Labs Inc with similar DNS configurations \ No newline at end of file +- The domain is owned by Patchwork Labs Inc \ No newline at end of file diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 554751b..539b3ea 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,7 +11,9 @@ Thank you for helping run the Patchwork Labs DNS. This file covers the rules. Th 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. +4. Give every record an owner in a comment on the same line as its name. Use + an email, a Patchwork id (`PWL...`), or both. The `dns records` check + enforces it. 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 @@ -82,7 +84,7 @@ pull request the next morning. $ 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 +Add a test with any change to `tools/merge_live.py` or `tools/check_zones.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 diff --git a/README.md b/README.md index d67f3c2..047ce58 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,6 @@ access. | Domain | Zone file | Used for | |---|---|---| | `patchworklabs.org` | [`patchworklabs.org.yaml`](./patchworklabs.org.yaml) | Patchwork Labs and its projects | -| `hackathon.help` | [`hackathon.help.yaml`](./hackathon.help.yaml) | Hackathon resources | ## Get a subdomain @@ -38,9 +37,13 @@ Three rules decide whether it works: - **The name is the part before the domain.** `docs` becomes `docs.patchworklabs.org`. - **A `CNAME` value ends with a dot.** An `A` or `AAAA` value does not. -- **Every record needs an owner.** Put a Patchwork 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. +- **Every record needs an owner.** Put an email, a Patchwork id, or both in a + comment on the same line as the name, for example + `# PWL0123456789 / ada@patchworklabs.org`. A record that a team owns can + name a GitHub team instead, for example `# @patchworklabsorg/infra`. A + personal GitHub handle does not count. We use it to find out who to ask when the + record breaks. List more than one owner if more than one person is + responsible. The `dns records` check fails on a record without an owner. ### Order is checked @@ -54,6 +57,21 @@ a rule and not a request. The order is **natural**, not plain alphabetical: Run `./bin/validate` to check before you push. The nightly sync writes files in this order by itself. +### Other checks + +`./bin/validate` also runs [`tools/check_zones.py`](./tools/check_zones.py). +It fails when: + +- A record has no owner comment, or only a `TODO owner unknown` comment. +- A TTL is lower than 120 seconds. Cloudflare raises a lower TTL to 120, so + the zone file would never match Cloudflare. +- A zone file holds the apex `NS` records. Cloudflare owns them. +- A zone file holds the `octodns-meta` record. octoDNS writes it. +- A record that is not `A`, `AAAA` or `CNAME` is behind the Cloudflare proxy. +- A zone in `config/config.yaml` has no zone file, or a YAML file at the + repository root is not a zone. +- One name appears twice in one file. + ### 2. Open a pull request A bot posts a **plan** on your pull request. It lists every record the merge diff --git a/SECURITY.md b/SECURITY.md index b915a3f..00daa4f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -12,7 +12,7 @@ days. ## What matters here -This repository controls DNS for `patchworklabs.org` and `hackathon.help`. +This repository controls DNS for `patchworklabs.org`. Somebody who can change a record can point a Patchwork name at a host they control. Treat these as serious: diff --git a/bin/validate b/bin/validate index 09aa2a4..f6ce168 100755 --- a/bin/validate +++ b/bin/validate @@ -1,9 +1,13 @@ #!/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. +# valid. Then check the repository rules that octoDNS does not know about, +# such as the owner comment on every record. See tools/check_zones.py. +# +# 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 "$@" +octodns-validate --config-file=./config/config.yaml "$@" +python3 tools/check_zones.py diff --git a/config/config.yaml b/config/config.yaml index 4138cbf..263ec44 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -72,9 +72,3 @@ zones: - config targets: - cloudflare - - hackathon.help.: - sources: - - config - targets: - - cloudflare diff --git a/docs/runbook.md b/docs/runbook.md index 44e24b7..e5f3a15 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -18,8 +18,8 @@ lands. | 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` | +| `CLOUDFLARE_TOKEN` | Cloudflare API token, **edit** DNS for `patchworklabs.org` | `deploy` | +| `CLOUDFLARE_TOKEN_READ_ONLY` | Cloudflare API token, **read** DNS for `patchworklabs.org` | `plan`, `sync-from-cloudflare` | | `DNS_BOT_PRIVATE_KEY` | Private key of the Patchwork DNS Bot GitHub App, the whole `.pem` | `sync-from-cloudflare` | There is also one repository **variable**, not a secret: @@ -28,9 +28,25 @@ There is also one repository **variable**, not a secret: |---|---| | `DNS_BOT_CLIENT_ID` | Client ID of the same App. An identifier, not a credential | -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 `patchworklabs.org` and `hackathon.help` only. +The zone is in the **Patchwork Labs** Cloudflare account. An account API +token can only see the zones in its own account, so create each token in that +account at **Cloudflare > Manage Account > Account API Tokens**: + +1. Select **Create Custom Token**. +2. **Permissions**: `Zone` `Zone` `Read`, and `Zone` `DNS` `Edit` for + `CLOUDFLARE_TOKEN` or `Zone` `DNS` `Read` for + `CLOUDFLARE_TOKEN_READ_ONLY`. +3. **Zone Resources**: `Include` `Specific zone` `patchworklabs.org`. +4. Save the value straight into the repository secret. Do not paste it + anywhere else: + + ```console + $ gh secret set CLOUDFLARE_TOKEN --repo patchworklabsorg/dns + ``` + +A token that cannot see a zone makes octoDNS try to create that zone. The +deploy then fails with `Invalid account identifier passed in your organization +variable`. > **Rotate `CLOUDFLARE_TOKEN_READ_ONLY` once.** The old `test.yml` workflow > ran scripts from a pull request while holding it, so anybody who opened a @@ -202,8 +218,8 @@ The zone files and Cloudflare now disagree. Fix it, do not leave it. 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. `hackathon.help` has fewer records than that, - so octoDNS would delete every one of them without complaining. + octoDNS and cannot be configured. A zone with fewer records than that has no + guard from octoDNS at all. - **`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: @@ -262,7 +278,7 @@ $ cat .live/patchworklabs.org.yaml ## If everything is broken -DNS for both zones is in Cloudflare. Cloudflare is the live system. This +DNS for the zone 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. diff --git a/hackathon.help.yaml b/hackathon.help.yaml deleted file mode 100644 index dccc31c..0000000 --- a/hackathon.help.yaml +++ /dev/null @@ -1,32 +0,0 @@ -"": - - ttl: 1 - type: MX - values: - - exchange: aspmx.l.google.com. - preference: 1 - - exchange: alt1.aspmx.l.google.com. - preference: 5 - - exchange: alt2.aspmx.l.google.com. - preference: 5 - - exchange: alt3.aspmx.l.google.com. - preference: 10 - - exchange: alt4.aspmx.l.google.com. - preference: 10 - - ttl: 1 - type: TXT - values: - - google-site-verification=dWzvYUBk_oc6spUhOFmqPl8wYeRMvpfhhARS1tb21ag - - Owned by Patchwork Labs Inc (patchworklabs.org) a registered 501(c)(3) nonprofit organization. - - v=spf1 include:_spf.google.com ~all - - slack-domain-verification=hwBycY5FO958m1HWCSDHQIWCM6z566RY95Swq8qo - -_dmarc: - ttl: 1 - type: TXT - value: v=DMARC1\; p=quarantine\; rua=mailto:dmark_reports@hackathon.help\; pct=100\; ruf=mailto:dmark_reports@hackathon.help\; adkim=s\; aspf=s - -# Google Workspace DKIM authentication -google._domainkey: - ttl: 3600 - type: TXT - value: v=DKIM1\; k=rsa\; p=MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1ZPaz12FXM3cVvaD96N1XbWsc1Um/EZYE8UMV+CQqupWmI/aqwfT5bQWHOCwp8RU+eI8ONee12ah7xnUThU4Ls+BNXyyrsRUAVfKowk0pxXpDwLHS0kf8sXBrsOLqQDwNOrSQ7P33YxhghExdwvdQ5O0qL557wjWU+zhbjiF1HzJm6Ved2Nya98cXu1UbkVGQsmlMJVb0nEZIvmD19sIxNkhXFUV6KIALqa7iai+YT+tapiiCc2XUzw4GTcqfIyS9leKn5Gz1gWCCgMCL3n03kxuzxR6PkS5YlgNZPur4MohsUU3UQZsAoF+NNoGTA67uY0HpLT91CVWuNfjMkTtFQIDAQAB diff --git a/patchworklabs.org.yaml b/patchworklabs.org.yaml index 2149495..2782358 100644 --- a/patchworklabs.org.yaml +++ b/patchworklabs.org.yaml @@ -1,5 +1,5 @@ -"": - - ttl: 1 +"": # @patchworklabsorg/infra + - ttl: 120 type: MX values: - exchange: aspmx.l.google.com. @@ -12,7 +12,7 @@ preference: 10 - exchange: alt4.aspmx.l.google.com. preference: 10 - - ttl: 1 + - ttl: 120 type: TXT values: - google-site-verification=OjTBuEBXcUdIOVzjidh1sCMfvAYvabpQWnkGfFFQQA4 @@ -23,61 +23,87 @@ - octodns: cloudflare: proxied: true - ttl: 1 + ttl: 120 type: A value: 216.198.79.1 -_atproto: - ttl: 1 +_atproto: # @patchworklabsorg/infra + ttl: 120 type: TXT value: did=did:plc:bpd7j2a34mmnyu7t64gzptg7 -_dmarc: - ttl: 1 +_dmarc: # @patchworklabsorg/infra + ttl: 120 type: TXT - value: v=DMARC1\; p=quarantine\; rua=mailto:dmark_reports@patchworklabs.org\; pct=100\; ruf=mailto:dmark_reports@patchworklabs.org\; adkim=s\; aspf=s + value: v=DMARC1\; p=quarantine\; np=reject\; rua=mailto:dmarc_reports@patchworklabs.org\; ruf=mailto:dmarc_reports@patchworklabs.org\; adkim=s\; aspf=s -_gh-patchworklabsorg-o: +# Customer.io sending domain (Mailgun) +_dmarc.cio: # PWL7A1CE1F3CB / jasper@patchworklabs.org ttl: 3600 + type: TXT + value: v=DMARC1\; p=quarantine\; np=reject\; rua=mailto:dmarc_reports@patchworklabs.org\; ruf=mailto:dmarc_reports@patchworklabs.org\; adkim=s\; aspf=s + +_gh-patchworklabsorg-o: # @patchworklabsorg/infra type: TXT value: cee2c56f89 -admin.forms: +admin.forms: # @patchworklabsorg/infra octodns: cloudflare: proxied: true - ttl: 1 + ttl: 120 type: A value: 65.19.76.238 -forms: +cio160924.cio: # PWL7A1CE1F3CB / jasper@patchworklabs.org + - ttl: 300 + type: MX + values: + - exchange: mxa.mailgun.org. + preference: 10 + - exchange: mxb.mailgun.org. + preference: 10 + - ttl: 3600 + type: TXT + value: v=spf1 include:mailgun.org ~all + +cio._domainkey.cio160924.cio: # PWL7A1CE1F3CB / jasper@patchworklabs.org + ttl: 3600 + type: TXT + value: k=rsa\; p=MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA0eVU0HCCkUK5FPCvMoX03lgTqTNWEkYA61pKilwmMif+B/L+AlLqQVywt22RLAWCpiJv3hRUFi4UmdXbAZzWjhVY5Mem8F1XqWvu6Xv1yAIC08tX+3vMH+5gdp/SJ8I1z2tmJXNMYbeAC3jK6uxOoY+Mcs63VLtstVJqqmFfJv454D4A8m5ckqb/ltkf6+dlxpC5mHx0IAf7ybek4fCKSlSIwCRI3M1rX2Jhh3KpvQkce4x53Hinj14t5w5u+eCdNUljUuyjjZBq1BMQn65p0FeJ4A6y8jxQXJM44Wajtd+zuDFK1AL8rrbFTgtkezurZvZCSZQSa1UfUkIegAbx2wIDAQAB + +forms: # @patchworklabsorg/infra octodns: cloudflare: proxied: true - ttl: 1 + ttl: 120 type: A value: 65.19.76.238 # Google Workspace DKIM authentication -google._domainkey: - ttl: 3600 +google._domainkey: # @patchworklabsorg/infra type: TXT value: v=DKIM1\; k=rsa\; p=MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAzEuGHXgOxIj0qk83hV4ajl/OIpXbhE/MTKhXU1VZDBISO/+yBCQsVyq1j4F13tWxUpYLlw78OJl+7TlAyZ4llYY5z2Oj+EiAIQZCRBLmKN7zDpnbwbiM4hfam5vnhCvFwdgg0aKUY221T/Pe5Mjz11e0VtyOJ3D3enPId010bi99p93Dimf+rFo9YFwps7U/V4O5rWaRyG9snFcl9tshvKTgQ6OoHEvLbvQIU1QTiXR6oHAI5KP/8BzA26djgIKqNuHaU9S70KSVUH/9CBSAjHP50j4i4rGGEr6PopNjxQxN5owwu2jUDEIOrv13RLpNPISu1NaPCgMnGVyfsQ77yQIDAQAB -idp: - ttl: 1 +idp: # @patchworklabsorg/infra + ttl: 120 type: A value: 129.213.163.213 -openpgpkey: +openpgpkey: # @patchworklabsorg/infra ttl: 300 type: CNAME value: wkd.keys.openpgp.org. -www: +weave-kyc-proposal: # PWL7A1CE1F3CB / jasper@patchworklabs.org + ttl: 300 + type: CNAME + value: e4ec487349d24e0b.vercel-dns-017.com. + +www: # @patchworklabsorg/infra octodns: cloudflare: proxied: true - ttl: 1 + ttl: 120 type: CNAME value: 52fd2b2210caa11f.vercel-dns-017.com. diff --git a/tools/check_zones.py b/tools/check_zones.py new file mode 100644 index 0000000..f76cb38 --- /dev/null +++ b/tools/check_zones.py @@ -0,0 +1,195 @@ +#!/usr/bin/env python3 +"""Check the zone files for the rules that octoDNS does not know about. + +``octodns-validate`` checks that each record is valid DNS. It does not know the +rules of this repository, so this script checks them: + +1. Every record has an owner in a comment on the same line as its name. An + owner is an email address or a Patchwork id, or both, for example + ``# PWL7A1CE1F3CB / ada@patchworklabs.org``. A record that a team owns can + name a GitHub team instead, for example ``# @patchworklabsorg/infra``. A + personal GitHub handle is not an owner. A ``TODO owner unknown`` comment + from the nightly sync is not an owner. +2. Every TTL is at least the Cloudflare minimum of 120 seconds. A lower value + is silently raised by Cloudflare, so the zone file would never match it. +3. No zone file holds the apex NS records. Cloudflare owns them. +4. No zone file holds the ``octodns-meta`` record. octoDNS writes it. +5. Only A, AAAA and CNAME records are behind the Cloudflare proxy. +6. Every zone in ``config/config.yaml`` has a zone file, and every YAML file + at the repository root belongs to a zone. +7. No record name appears twice in one file. + +Usage: + + python3 tools/check_zones.py [--repo-dir .] + +Exits 0 when every file passes and 1 when any rule fails. Each failure is +printed as ``file:line: message``. +""" + +import argparse +import re +import sys +from pathlib import Path + +import yaml + +MIN_TTL = 120 +PROXIABLE = {"A", "AAAA", "CNAME"} +META_RECORD = "octodns-meta" + +# A top level key: `name:`, `"name":` or `'name':`, then an optional comment. +_TOP_LEVEL = re.compile( + r"""^(?:"(?P[^"]*)"|'(?P[^']*)'|(?P[^\s#"'-][^:#]*?))\s*:""" + r"""(?:\s+(?P.*))?$""" +) +_EMAIL = re.compile(r"[\w.+-]+@[\w-]+(?:\.[\w-]+)+") +# A Patchwork id: `PWL` and ten upper case hex digits. +_PWL_ID = re.compile(r"\bPWL[0-9A-F]{10}\b") +# A GitHub team, `@org/team`. A personal handle does not count. +_TEAM = re.compile(r"(?:^|[\s,/])@[A-Za-z0-9][A-Za-z0-9-]*/[\w.-]+") + + +def _owner_comment(rest): + """Return the comment on a key line, or None.""" + if not rest: + return None + if rest.startswith("#"): + return rest[1:].strip() + match = re.search(r"\s#(.*)$", rest) + return match.group(1).strip() if match else None + + +def has_owner(comment): + """True when the comment names at least one owner.""" + if not comment or comment.lower().startswith("todo"): + return False + return bool( + _EMAIL.search(comment) or _PWL_ID.search(comment) or _TEAM.search(comment) + ) + + +def _top_level_keys(text): + """Yield (line number, name, comment) for each record in a zone file.""" + for number, line in enumerate(text.split("\n"), start=1): + if line == "---": + continue + match = _TOP_LEVEL.match(line) + if not match: + continue + name = next( + g for g in (match.group("dq"), match.group("sq"), match.group("bare")) + if g is not None + ) + yield number, name, _owner_comment(match.group("rest")) + + +def _records(value): + """A record name holds one record or a list of them.""" + if isinstance(value, list): + return value + return [value] + + +def check_file(path): + """Return a list of `file:line: message` strings for one zone file.""" + text = path.read_text() + errors = [] + + lines = {} + for number, name, comment in _top_level_keys(text): + if name in lines: + errors.append( + f"{path.name}:{number}: `{name or '@'}` is already defined on " + f"line {lines[name]}" + ) + continue + lines[name] = number + if not has_owner(comment): + errors.append( + f"{path.name}:{number}: `{name or '@'}` has no owner. Add an " + f"email, a Patchwork id or both in a comment on the same line" + ) + + data = yaml.safe_load(text) or {} + if not isinstance(data, dict): + return errors + [f"{path.name}:1: the file is not a mapping of records"] + + for name, value in data.items(): + number = lines.get(name, 1) + label = name or "@" + if name == META_RECORD: + errors.append( + f"{path.name}:{number}: `{META_RECORD}` is written by octoDNS. " + f"Remove it from the zone file" + ) + for record in _records(value): + if not isinstance(record, dict): + continue + rtype = str(record.get("type", "")).upper() + ttl = record.get("ttl") + if ttl is not None and ttl < MIN_TTL: + errors.append( + f"{path.name}:{number}: `{label}` {rtype} has ttl {ttl}. " + f"The Cloudflare minimum is {MIN_TTL}" + ) + if name == "" and rtype == "NS": + errors.append( + f"{path.name}:{number}: the apex NS records belong to " + f"Cloudflare. Remove them from the zone file" + ) + octodns = record.get("octodns") or {} + proxied = (octodns.get("cloudflare") or {}).get("proxied") + if proxied and rtype not in PROXIABLE: + errors.append( + f"{path.name}:{number}: `{label}` {rtype} cannot be " + f"proxied. Only {', '.join(sorted(PROXIABLE))} can" + ) + return errors + + +def _config_zones(repo_dir): + with open(Path(repo_dir) / "config" / "config.yaml") as fh: + return [z.rstrip(".") for z in yaml.safe_load(fh)["zones"]] + + +def check_repo(repo_dir): + """Return every error for the repository.""" + repo = Path(repo_dir) + zones = _config_zones(repo) + errors = [] + + files = {p.name for p in repo.glob("*.yaml")} | { + p.name for p in repo.glob("*.yml") + } + expected = {f"{zone}.yaml" for zone in zones} + for name in sorted(expected - files): + errors.append(f"{name}:1: the zone is in config/config.yaml but the file is missing") + for name in sorted(files - expected): + errors.append( + f"{name}:1: this file is not a zone in config/config.yaml, so " + f"octoDNS would ignore it" + ) + + for name in sorted(expected & files): + errors.extend(check_file(repo / name)) + return errors + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.split("\n")[0]) + parser.add_argument("--repo-dir", default=".", help="where the zone files live") + args = parser.parse_args(argv) + + errors = check_repo(args.repo_dir) + for error in errors: + print(error) + if errors: + print(f"\n{len(errors)} problem(s). See README.md for the rules.") + return 1 + print("Every zone file follows the repository rules.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/merge_live.py b/tools/merge_live.py index 2b19510..a62ae98 100755 --- a/tools/merge_live.py +++ b/tools/merge_live.py @@ -14,7 +14,7 @@ Usage: python3 tools/merge_live.py --live-dir .live --repo-dir . \ - --zone patchworklabs.org. --zone hackathon.help. --summary-out summary.md + --zone patchworklabs.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. diff --git a/tools/test_check_zones.py b/tools/test_check_zones.py new file mode 100644 index 0000000..c1dc764 --- /dev/null +++ b/tools/test_check_zones.py @@ -0,0 +1,235 @@ +"""Tests for tools/check_zones.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 check_zones import check_file, check_repo, has_owner # noqa: E402 +from merge_live import merge_zone # noqa: E402 + +CONFIG = """--- +zones: + patchworklabs.org.: + sources: [config] + targets: [cloudflare] +""" + +GOOD = """--- +"": # @patchworklabsorg/infra + - ttl: 120 + type: MX + values: + - exchange: aspmx.l.google.com. + preference: 1 + +# A comment above a record is fine as well. +api: # ada@patchworklabs.org, @grace + - octodns: + cloudflare: + proxied: true + ttl: 300 + type: CNAME + value: api.example.com. + +docs: # grace@patchworklabs.org + type: TXT + value: no-ttl-uses-the-default +""" + + +class HasOwnerTest(unittest.TestCase): + def test_email(self): + self.assertTrue(has_owner("ada@patchworklabs.org")) + + def test_pwl_id(self): + self.assertTrue(has_owner("PWL7A1CE1F3CB")) + + def test_pwl_id_and_email(self): + self.assertTrue(has_owner("PWL7A1CE1F3CB / jasper@patchworklabs.org")) + + def test_malformed_pwl_id(self): + self.assertFalse(has_owner("PWL123")) + self.assertFalse(has_owner("pwl7a1ce1f3cb")) + + def test_github_user_is_not_an_owner(self): + self.assertFalse(has_owner("@jaspermayone")) + + def test_github_team(self): + self.assertTrue(has_owner("@patchworklabsorg/infra")) + + def test_several(self): + self.assertTrue(has_owner("ada@patchworklabs.org, @patchworklabsorg/infra")) + + def test_none(self): + self.assertFalse(has_owner(None)) + self.assertFalse(has_owner("")) + + def test_free_text(self): + self.assertFalse(has_owner("Google Workspace DKIM")) + + def test_todo_from_the_nightly_sync(self): + self.assertFalse( + has_owner("TODO owner unknown, added from Cloudflare on 2026-09-21") + ) + + +class CheckFileTest(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.path = Path(self.tmp.name) / "patchworklabs.org.yaml" + + def check(self, body): + self.path.write_text(body) + return check_file(self.path) + + def assertOneError(self, body, fragment): + errors = self.check(body) + self.assertEqual(len(errors), 1, errors) + self.assertIn(fragment, errors[0]) + + def test_good_file_passes(self): + self.assertEqual(self.check(GOOD), []) + + def test_missing_owner(self): + self.assertOneError( + "---\napi:\n ttl: 300\n type: A\n value: 192.0.2.1\n", + "patchworklabs.org.yaml:2: `api` has no owner", + ) + + def test_owner_on_the_line_above_does_not_count(self): + self.assertOneError( + "---\n# ada@patchworklabs.org\napi:\n ttl: 300\n type: A\n" + " value: 192.0.2.1\n", + "`api` has no owner", + ) + + def test_apex_without_owner_is_named_at(self): + self.assertOneError( + '---\n"":\n ttl: 300\n type: A\n value: 192.0.2.1\n', + "`@` has no owner", + ) + + def test_single_quoted_apex(self): + body = "---\n'': # PWL7A1CE1F3CB\n ttl: 300\n type: A\n value: 192.0.2.1\n" + self.assertEqual(self.check(body), []) + + def test_todo_owner_fails(self): + self.assertOneError( + "---\napi: # TODO owner unknown, added from Cloudflare on 2026-09-21\n" + " ttl: 300\n type: A\n value: 192.0.2.1\n", + "has no owner", + ) + + def test_low_ttl(self): + self.assertOneError( + "---\napi: # ada@patchworklabs.org\n ttl: 1\n type: A\n value: 192.0.2.1\n", + "`api` A has ttl 1. The Cloudflare minimum is 120", + ) + + def test_low_ttl_inside_a_list(self): + self.assertOneError( + "---\napi: # ada@patchworklabs.org\n - ttl: 300\n type: A\n value: 192.0.2.1\n" + " - ttl: 60\n type: TXT\n value: hello\n", + "`api` TXT has ttl 60", + ) + + def test_apex_ns(self): + self.assertOneError( + '---\n"": # ada@patchworklabs.org\n ttl: 3600\n type: NS\n values:\n' + " - ns1.example.com.\n", + "apex NS records belong to Cloudflare", + ) + + def test_ns_on_a_subdomain_is_fine(self): + body = ( + "---\nsub: # ada@patchworklabs.org\n ttl: 3600\n type: NS\n values:\n" + " - ns1.example.com.\n" + ) + self.assertEqual(self.check(body), []) + + def test_meta_record(self): + self.assertOneError( + "---\noctodns-meta: # ada@patchworklabs.org\n ttl: 120\n type: TXT\n value: x\n", + "`octodns-meta` is written by octoDNS", + ) + + def test_proxied_txt(self): + self.assertOneError( + "---\napi: # ada@patchworklabs.org\n octodns:\n cloudflare:\n proxied: true\n" + " ttl: 300\n type: TXT\n value: x\n", + "`api` TXT cannot be proxied", + ) + + def test_duplicate_name(self): + errors = self.check( + "---\napi: # ada@patchworklabs.org\n ttl: 300\n type: A\n value: 192.0.2.1\n\n" + "api: # ada@patchworklabs.org\n ttl: 300\n type: A\n value: 192.0.2.2\n" + ) + self.assertTrue( + any("`api` is already defined on line 2" in e for e in errors), errors + ) + + def test_errors_carry_the_line_number(self): + errors = self.check(GOOD.replace("api: # ada@patchworklabs.org, @grace", "api:")) + self.assertEqual(len(errors), 1, errors) + self.assertTrue(errors[0].startswith("patchworklabs.org.yaml:10:"), errors) + + +class CheckRepoTest(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.repo = Path(self.tmp.name) + (self.repo / "config").mkdir() + (self.repo / "config" / "config.yaml").write_text(CONFIG) + + def test_good_repo(self): + (self.repo / "patchworklabs.org.yaml").write_text(GOOD) + self.assertEqual(check_repo(self.repo), []) + + def test_missing_zone_file(self): + errors = check_repo(self.repo) + self.assertEqual(len(errors), 1, errors) + self.assertIn("the file is missing", errors[0]) + + def test_stray_yaml_file(self): + (self.repo / "patchworklabs.org.yaml").write_text(GOOD) + (self.repo / "patchworklabs.com.yaml").write_text(GOOD) + errors = check_repo(self.repo) + self.assertEqual(len(errors), 1, errors) + self.assertIn("patchworklabs.com.yaml:1: this file is not a zone", errors[0]) + + +class NightlySyncTest(unittest.TestCase): + """The file that the nightly sync writes must be readable by this check.""" + + def test_new_record_from_cloudflare_needs_an_owner(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "live").mkdir() + (root / "repo").mkdir() + (root / "repo" / "patchworklabs.org.yaml").write_text(GOOD) + live = GOOD + "\nnew:\n ttl: 300\n type: A\n value: 192.0.2.9\n" + (root / "live" / "patchworklabs.org.yaml").write_text(live) + + merge_zone( + "patchworklabs.org.", root / "live", root / "repo", + {"octodns-meta"}, "2026-09-21", + ) + errors = check_file(root / "repo" / "patchworklabs.org.yaml") + + self.assertEqual(len(errors), 1, errors) + self.assertIn("`new` has no owner", errors[0]) + + +if __name__ == "__main__": + unittest.main()