From d749b2663ce1f05485c8e6f4bcf6353777e31a35 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 14 Sep 2026 01:40:44 +0200 Subject: [PATCH] chore: seed the lode and enable the pre-PR gate dash-proxy's behaviour lives in a 58k-line Go tree and in two years of review threads; neither is readable at the start of a task. This writes what the code does, and why, into lode/ so it is read before the code rather than rediscovered from it. The lode holds thirteen baseline and subsystem files: summary, terminology, practices, the map, the workflow profile, plans/README, and one summary each for the request path, certificates (issuance, and store recovery), dynamic sources, the cache, resilience, CLI/RPC, observability and testing/CI. Every line range and count in them was computed with a script rather than read off the screen, and every constant was checked against its declaration. lode/review/ carries 112 rules across 8 files, rewritten from review voice into statements about the system and re-verified against the current code. They cite 90 of cubic's 92 accepted learnings for this repo; 18 rules merge two or more learnings, because cubic records one learning per thread and a dozen threads about the certificate-store export are one rule about path validation. The 2 uncited learnings are assertion-wording notes for specific tests, not invariants. 15 rules cite merged PR review threads (#91, #92, #95, #97, #103, #105, #112, #113, #116), which is where the reasoned rejections live: 5 rules are "Not a bug" entries that stop a future reviewer re-raising a settled question. A further 15 rules were found in the code during seeding with no learning behind them. Nothing was dropped because its subject had gone -- all 42 functions the learnings name still exist. Two claims in the drafts were wrong and are fixed rather than carried: the dynamic-domain payload caps are 1 MB and 10 000 entries, not 5 MB and 50 000; and "writeFileAtomic for every state file" is really four staging shapes with four different durability guarantees. Retired seven local commands the plugin now supersedes -- lfg, plan, tdd, finish-prs, github-review-pr, github-review-failures, github-review-comments -- and pointed CLAUDE.md's table at their /lode: equivalents. debug-flaky never existed here. architect, perf, review-pr and security are repo-specific and stay. Doc-versus-code disagreements found while reading are reported with the PR rather than recorded in the lode, which describes only the tree it ships in. Claude-Session: https://claude.ai/code/session_01Hga3idYBp7fWdj7GmmfofF --- .claude/commands/finish-prs.md | 188 ---------- .claude/commands/github-review-comments.md | 254 -------------- .claude/commands/github-review-failures.md | 196 ----------- .claude/commands/github-review-pr.md | 181 ---------- .claude/commands/lfg.md | 344 ------------------- .claude/commands/plan.md | 100 ------ .claude/commands/tdd.md | 182 ---------- .claude/settings.json | 13 + .gitignore | 1 + CLAUDE.md | 33 +- lode/cache/summary.md | 140 ++++++++ lode/certs/store-and-recovery.md | 151 ++++++++ lode/certs/summary.md | 184 ++++++++++ lode/cli-and-rpc/summary.md | 107 ++++++ lode/dynamic-sources/summary.md | 150 ++++++++ lode/lode-map.md | 42 +++ lode/observability/summary.md | 87 +++++ lode/plans/README.md | 15 + lode/practices.md | 142 ++++++++ lode/request-path/summary.md | 156 +++++++++ lode/resilience/summary.md | 159 +++++++++ lode/review/acme-providers.md | 85 +++++ lode/review/cert-store-export.md | 80 +++++ lode/review/cert-store-restore.md | 110 ++++++ lode/review/certs-directories-and-renewal.md | 91 +++++ lode/review/certs-issuance.md | 97 ++++++ lode/review/dynamic-domains.md | 81 +++++ lode/review/dynamic-redirects.md | 118 +++++++ lode/review/sources-and-refresh.md | 78 +++++ lode/summary.md | 32 ++ lode/terminology.md | 87 +++++ lode/testing-and-ci/summary.md | 124 +++++++ lode/workflow.md | 219 ++++++++++++ 33 files changed, 2573 insertions(+), 1454 deletions(-) delete mode 100644 .claude/commands/finish-prs.md delete mode 100644 .claude/commands/github-review-comments.md delete mode 100644 .claude/commands/github-review-failures.md delete mode 100644 .claude/commands/github-review-pr.md delete mode 100644 .claude/commands/lfg.md delete mode 100644 .claude/commands/plan.md delete mode 100644 .claude/commands/tdd.md create mode 100644 .claude/settings.json create mode 100644 lode/cache/summary.md create mode 100644 lode/certs/store-and-recovery.md create mode 100644 lode/certs/summary.md create mode 100644 lode/cli-and-rpc/summary.md create mode 100644 lode/dynamic-sources/summary.md create mode 100644 lode/lode-map.md create mode 100644 lode/observability/summary.md create mode 100644 lode/plans/README.md create mode 100644 lode/practices.md create mode 100644 lode/request-path/summary.md create mode 100644 lode/resilience/summary.md create mode 100644 lode/review/acme-providers.md create mode 100644 lode/review/cert-store-export.md create mode 100644 lode/review/cert-store-restore.md create mode 100644 lode/review/certs-directories-and-renewal.md create mode 100644 lode/review/certs-issuance.md create mode 100644 lode/review/dynamic-domains.md create mode 100644 lode/review/dynamic-redirects.md create mode 100644 lode/review/sources-and-refresh.md create mode 100644 lode/summary.md create mode 100644 lode/terminology.md create mode 100644 lode/testing-and-ci/summary.md create mode 100644 lode/workflow.md diff --git a/.claude/commands/finish-prs.md b/.claude/commands/finish-prs.md deleted file mode 100644 index 6720757..0000000 --- a/.claude/commands/finish-prs.md +++ /dev/null @@ -1,188 +0,0 @@ ---- -description: "Drive a set of open dash-proxy PRs to merge-ready, one at a time, in a given order. Merges the base forward (never rebases — published branches are shared), runs /github-review-pr (conflicts, then CI failures, then review comments) on each, then waits for the user to merge before syncing and advancing to the next. Use to clear a stack of stacked/parallel PRs without manual merge churn." -model: opus -argument-hint: "ordered PR list (e.g. '12 14 15 18'); optional 'automerge' to enable gh auto-merge; empty = auto-discover your open PRs" -allowed-tools: Bash(gh pr list:*), Bash(gh pr view:*), Bash(gh pr checks:*), Bash(gh pr diff:*), Bash(gh pr comment:*), Bash(gh pr merge:*), Bash(gh api:*), Bash(gh run view:*), Bash(git:*), Bash(make:*), Bash(go test:*), Bash(go vet:*), Bash(go mod:*), Bash(gofmt:*), Bash(cd:*), Read, Write, Edit, Glob, Grep, Agent, Skill, TaskCreate, TaskUpdate, TaskGet, TaskList, ScheduleWakeup ---- - -# Finish PRs (ordered merge-ready loop): $ARGUMENTS - -You are driving a set of open pull requests on `zoolutions/dash-proxy` (**dash-proxy**) to **merge-ready** state, one at a time, in a defined order, minimizing the manual sync/CI back-and-forth that stacked or parallel PRs create. - -## The fork constraints that shape this loop - -This is a fork, and its branch model changes what "sync the PR" means. Read `.claude/rules/git-workflow.md` and `.claude/rules/upstream-sync.md` if you have not; the non-negotiables: - -| Rule | Consequence for this command | -|---|---| -| PRs target `dash`, never `main` | The base you sync against is `origin/dash`. A PR with `baseRefName: main` is a bug — report it, don't process it. | -| **Never rebase a published branch** | Every branch here has a PR, so it is published. Sync with **`git merge origin/dash`**, never `git rebase`. There is therefore **no force-push anywhere in this command** — merge commits push cleanly. | -| Merging a PR lands on `dash`, not `main` | `main` doesn't move when a PR merges, so the *upstream* base is stable. What each merge invalidates is the others' relationship to `dash` — that's what the re-sync in Phase 2a absorbs. | -| `git rerere` is enabled | Previously-seen conflicts auto-replay their recorded resolutions. Always `git diff --staged` before trusting a replay — a resolution recorded in a different context can be wrong. | -| The long-lived cert branches collide | `san-certificate-batching` and `wildcard-certs` both touch `run.go` / `config.go` / `router.go`. When two queued PRs descend from those branches, expect union-shaped conflicts on exactly those files — the playbook in `upstream-sync.md` names the resolution for each. | -| `kamal-proxy` naming is load-bearing | Module, binary, RPC method names, and socket path stay `kamal-proxy` — no PR in the queue may rename them, whatever a review comment suggests. | - -**This command does NOT merge PRs itself** unless the user passed `automerge`. Default behavior: make each PR merge-ready, then pause and let the user merge; when a merge lands, re-sync the remaining PRs and continue. - -**This command does not re-implement conflict resolution.** `/github-review-pr` owns that (its Phase A0), along with CI failures (Phase A) and review comments (Phase B). This command owns *ordering, sequencing, and the wait-for-merge gate*. - ---- - -## Phase 0: Parse the PR list and order - -`$ARGUMENTS` may be: - -- A space/comma-separated ordered list of PR numbers: `12 14 15 18` (also accepts `#12`, `PR12`). -- The word `automerge` anywhere in the args → enable `gh pr merge --auto --squash` on each PR once it is green + approved (still respects branch protection; GitHub merges when gates pass). Strip it out before parsing numbers. -- Empty → auto-discover: - - ```bash - gh pr list --repo zoolutions/dash-proxy --author=@me --base main --state=open --limit 100 \ - --json number,title,headRefName,baseRefName,createdAt - ``` - - Order **oldest-first** (`createdAt` ascending). The explicit `--limit` matters — `gh pr list` defaults to 30, so without it discovery silently drops older PRs once the queue grows past 30. Oldest-first is the safe default: the earliest PR is usually the one others were cut alongside, so merging it first minimizes downstream re-syncs. Show the discovered order and proceed. - -**Verify every PR's base is `dash`.** Any PR based on `main` is a mistake in the fork model — surface it immediately and exclude it from the queue rather than processing it. - -**Order matters.** Each merge into `main` invalidates the others' merge base against `main`. Processing in a fixed order means you merge the base forward into each remaining PR exactly once per upstream merge, not repeatedly. If the user gave an explicit order, honor it exactly — they may know a dependency the metadata doesn't show. When PRs descend from `san-certificate-batching` and `wildcard-certs`, order them deliberately: whichever lands first defines the shape the other must union into. - -Create a task list (TaskCreate) with one task per PR, in order, so progress is visible. Mark the current PR `in_progress`. - -Confirm the plan in one line: `Finishing N PRs in order: #a → #b → #c. Mode: .` - ---- - -## Phase 1: Locate each PR's working tree - -For each PR you need a checkout of its branch to merge and push. Prefer, in order: - -1. An existing worktree already on that branch: `git worktree list` — match the branch. -2. If none, create one: `git worktree add .claude/worktrees/finish- ` (fetch the branch first: `git fetch origin `). - -Never merge into a branch that is currently checked out in the **main working directory** — operate in a worktree so the user's main checkout is undisturbed. Never touch `main` at all. - ---- - -## Phase 2: Per-PR loop - -Process PRs strictly in order. For the current PR: - -### 2a. Sync the branch onto the latest `dash` - -```bash -git fetch origin main --quiet -cd -git merge origin/dash -``` - -**Merge, never rebase.** If the merge conflicts, do NOT resolve it here — `/github-review-pr` Phase A0 owns conflict resolution and carries the per-file playbook (`go.mod`/`go.sum` → main's toolchain + deps, keep `go-acme/lego/v4`, then `go mod tidy`; `internal/cmd/run.go` → union of flags but register `--acme-email`/`--acme-directory` exactly once, since pflag panics on duplicates; `internal/server/config.go`, `router.go` → union of both cert subsystems' fields and methods; `internal/server/service.go` → read both sides, preserve upstream changes AND feature wiring; `Dockerfile`/`Makefile`/`script/release` → always upstream's). Abort the merge (`git merge --abort`) and let step 2d handle it — Phase A0 runs first inside that command by design. - -If the merge is clean, commit it (git's default merge message is fine) and continue. - -### 2b. Settle the module files if the merge disturbed them - -`go.mod` / `go.sum` are the mechanically-resolvable files in this repo. If the merge touched either, or `git status` shows them dirty: - -```bash -go mod tidy -git add go.mod go.sum -``` - -Never hand-merge `go.sum`. Confirm `go-acme/lego/v4` survived — the fork's ACME work depends on it and a careless resolution to upstream's side drops it. - -### 2c. Verify and push the synced branch - -Run the gates before pushing — a merge that doesn't build wastes a full CI cycle: - -```bash -gofmt -l internal/ cmd/ # must be empty; CI enforces formatting -go vet ./... -make test -git push origin -``` - -**No `--force`, no `--force-with-lease`** — this command never rewrites history, so a plain push always suffices. If a plain push is rejected, someone else pushed; fetch and merge again rather than reaching for force. - -### 2d. Run the full review pass - -Invoke `/github-review-pr ` (via the Skill tool). It runs **conflicts (A0) → CI failures (A) → review comments (B)** — do not re-implement any of it. It will: - -- Resolve any merge conflict with `dash` semantically, per the conflict playbook, and push the merge commit. -- Fix red CI checks (formatting, `go vet`, `go test ./...`, build) and push. -- Address every unresolved review thread: implement valid fixes, push back with reasoning on wrong ones, resolve threads. - -Wait for it to finish. If it reports a persistent failure it could not fix (or a conflict it could not resolve without a decision), surface that for this PR and move it to a `needs-user` state — do not block the whole queue on one stuck PR; note it and continue to the next PR, then return. - -### 2e. Verify merge-ready - -```bash -gh pr view --json mergeable,mergeStateStatus,reviewDecision,baseRefName \ - --jq '{mergeable,mergeStateStatus,reviewDecision,baseRefName}' -gh pr checks -``` - -Merge-ready means: `baseRefName=dash`, `mergeable=MERGEABLE`, no failing checks (green or pending-green), and `reviewDecision` is `APPROVED` or empty (not `CHANGES_REQUESTED`). A `BLOCKED` mergeStateStatus with everything else green usually means "awaiting required approval" — expected, not a defect. - -`mergeable=UNKNOWN` is common right after a push and can persist for minutes. Don't poll it; verify locally per the `git merge-tree --write-tree --name-only origin/dash FETCH_HEAD` recipe in `/github-review-pr` Phase A0. - -### 2f. Hand off for merge - -- **`automerge` mode:** `gh pr merge --auto --squash` (GitHub merges when gates pass). Then go to Phase 3 to wait for the merge to land before advancing. -- **Default (pause) mode:** report this PR as ✅ merge-ready with its URL and a one-line "what's in it," and tell the user it's ready to merge. Then **wait** (Phase 3). - -Mark the PR's task `completed` (merge-ready) — or `needs-user` via a metadata note if it got stuck in 2d. - ---- - -## Phase 3: Wait for the merge, then advance - -The loop is **gated on the target PR merging**, because each merge into `main` is what the next PR needs to absorb. - -- **automerge mode:** poll `gh pr view --json state --jq .state` until `MERGED`. Use `ScheduleWakeup` with a delay matched to CI duration (Go builds + `go test ./...` run a few minutes; poll ~240s) rather than a busy sleep. When merged, advance. -- **default mode:** the user merges manually and will tell you (or you are re-invoked). On the next turn, re-check `gh pr view --json state`. If `MERGED`, advance to the next PR and repeat Phase 2 (its re-sync now picks up the just-merged changes). If not yet merged, report current status and stop — do not spin. - -When you advance, **always re-fetch and merge `origin/dash` forward into the next PR** (Phase 2a) before doing anything else — the merge that just landed is exactly the change it needs to absorb. - -If the user merges a PR **out of the planned order**, adapt: drop it from the remaining list and re-sync whatever is now next. - ---- - -## Phase 4 (optional): upstream drift and release ordering - -Two fork-specific things worth surfacing once, at the end, rather than fixing mid-queue: - -- **Upstream drift.** If several PRs in the queue conflicted against `main` in the same file, `main` may have moved and `dash` may be behind it. The durable fix is the routine sync in `.claude/rules/upstream-sync.md` (`git checkout main && git merge --ff-only upstream/main`, then merge `main` forward into the cert branches and `dash`) — commits to shared branches, so mention it, don't do it unprompted. -- **Release ordering.** If the merged work changes proxy behavior the gem depends on, the proxy image releases **first** and the gem's `MINIMUM_VERSION` names an already-published tag second. The image has no version command — the tag IS the version. Flag this when the queue contains anything the `dash` gem will need to pin. - ---- - -## Phase 5: Final report - -When the queue is drained (all merged, or all merge-ready-and-handed-off, or blocked-on-user): - -| PR | Result | Note | -|----|--------|------| -| #a | ✅ merged / ✅ merge-ready / ⏳ awaiting-merge / ⚠️ needs-user | one line | - -Then: - -1. What the user must do next (merge the ready ones, decide on any `needs-user` items). -2. Any collision you resolved between the two cert subsystems — the union resolutions in `run.go` / `config.go` / `router.go` are the ones most likely to be subtly wrong, and worth a human read. -3. Whether upstream drift or a release-ordering issue (Phase 4) is worth acting on. - ---- - -## Important notes - -- **Never rebase anything** — every branch here is published; merge forward only. -- **Never force-push** — this command never rewrites history, so a plain `git push` always suffices. -- **Never touch `main`** — not a commit, not a merge, not a push. It is a fast-forward-only mirror of `basecamp/kamal-proxy`. -- **Never process a PR based on `main`** — report it as a fork-model mistake instead. -- **Never resolve conflicts here** — abort and let `/github-review-pr` Phase A0 do it with the full playbook. -- **Prefer leaving `Dockerfile`, `Makefile`, and `script/release` as basecamp has them** — fewer conflicts when merging their fixes forward; the fork owns `script/release-dash`. -- **Never rename the module, binary, RPC methods, or socket path** away from `kamal-proxy`. -- **Don't re-implement `/github-review-pr`, `/github-review-failures`, or `/github-review-comments`** — invoke them. -- **One stuck PR must not block the rest** — mark it `needs-user`, continue the queue, return to it in the final report. -- **Never hand-merge `go.sum`** — resolve `go.mod`, then `go mod tidy`. -- **Register each pflag exactly once** — duplicate `--acme-email` / `--acme-directory` registration panics at startup, and a naive union of `run.go` is how it happens. diff --git a/.claude/commands/github-review-comments.md b/.claude/commands/github-review-comments.md deleted file mode 100644 index 25de7b6..0000000 --- a/.claude/commands/github-review-comments.md +++ /dev/null @@ -1,254 +0,0 @@ ---- -description: "Use when a PR has unresolved review comments that need responses -- evaluates each comment, implements valid fixes, pushes back on incorrect suggestions, and resolves all threads." -model: sonnet -argument-hint: "PR number (e.g., 123 or #123)" -allowed-tools: Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(git log:*), Bash(git blame:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(make test:*), Bash(make lint:*), Bash(gofmt:*), Bash(go test:*), Bash(go vet:*), Read, Write, Edit, Glob, Grep, Agent ---- - -# Review GitHub PR Comments: $ARGUMENTS - -You are reviewing and responding to all unresolved review comments on a GitHub pull request against `zoolutions/dash-proxy`. Apply technical rigour -- evaluate each comment against the actual codebase before accepting or rejecting it. - -**Fork context first.** This repo is a fork of `basecamp/kamal-proxy`, not a normal project. Before evaluating anything, know: - -- PRs target `dash`, never `main` -- `main` is a fast-forward-only mirror of upstream and never receives commits -- `kamal-proxy` naming (module, binary, RPC service, socket) is load-bearing -- see `CLAUDE.md` Critical Rules #1; never accept a rename suggestion -- Tags are four-segment `vX.Y.Z.N`, never suffix forms like `v1.0.0-rc1` -- see `.claude/rules/git-workflow.md` -- Full sync/branch rules: `.claude/rules/upstream-sync.md`, `.claude/rules/git-workflow.md` - ---- - -## Phase 0: Determine the PR Number - -The user may provide a PR number as `$ARGUMENTS`. Parse it flexibly: - -- `PR123`, `PR 123`, `pr123` -> PR 123 -- `123` -> PR 123 -- `#123` -> PR 123 -- Empty/blank -> auto-detect from current branch - -**If no PR number is provided**, detect it automatically: - -```bash -gh pr list --author=@me --head="$(git branch --show-current)" --state=open --json number,title -``` - -If exactly one open PR exists for the current branch, use it. If none or multiple, ask the user. - -Once you have the PR number, confirm it -- and confirm the base branch is `dash`, not `main`: - -```bash -gh pr view --json title,state,url,baseRefName -``` - -If `baseRefName` is `main`, stop and flag it -- that PR is misdirected (see Branch Model above); don't process review comments on it as if it were normal. - ---- - -## Phase 1: Fetch All Unresolved Review Comments - -Retrieve all review comments and identify unresolved ones: - -```bash -# Get all review comments (not resolved) -gh api "repos/zoolutions/dash-proxy/pulls//comments" --paginate - -# Get all review threads to check resolution status -gh api graphql -f query=' - query($owner: String!, $repo: String!, $pr: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pr) { - reviewThreads(first: 100) { - nodes { - id - isResolved - path - line - comments(first: 20) { - nodes { - id - databaseId - body - author { login } - createdAt - } - } - } - } - } - } - } -' -f owner=mhenrixon -f repo=kamal-proxy -F pr= -``` - -For each unresolved thread, extract: -- Thread ID (for resolving) -- Comment body (the review feedback) -- File path and line number (if inline) -- Author (to understand context) - -Filter to only **unresolved** threads. Skip bot comments (CodeRabbit, dependabot), resolved threads, and PR description comments. - -If there are no unresolved review comments, report that and stop. - ---- - -## Phase 2: Read and Categorise Each Comment - -For each unresolved comment, read the full body and categorise it: - -| Category | Action | -|----------|--------| -| Valid fix needed | Implement the fix | -| Valid test gap | Add the missing test (table-driven, per `.claude/rules/testing.md`) | -| Valid style/consistency issue | Fix it | -| Incorrect suggestion | Push back with technical reasoning | -| Suggestion conflicts with architecture | Push back, reference `CLAUDE.md` layers (`cmd/kamal-proxy` -> `internal/cmd` -> RPC -> `internal/server`) | -| Renames `kamal-proxy` module/binary/RPC/socket | Reject outright -- `CLAUDE.md` Critical Rules #1, load-bearing across 9 RPC client call sites and the Dockerfile | -| Targets `main` or suggests committing there | Reject outright -- `main` is ff-only, see Branch Model | -| Over-engineering / YAGNI | Push back, explain why it's unnecessary | -| Unclear | Ask for clarification (do NOT implement) | - -**Before categorising**, always: -1. Read the actual file and line being commented on -2. Check if the suggestion is technically correct for THIS codebase -3. Check if it would break existing functionality (run the relevant package's tests, not just read the diff) -4. Check if existing patterns/conventions contradict the suggestion (`internal/server/router_test.go`, `load_balancer_test.go` helpers are the house style for tests) -5. Check `CLAUDE.md` and `.claude/rules/*.md` -- project conventions override reviewer preferences, including fork-specific constraints (naming, branch model, tag grammar) - ---- - -## Phase 3: Implement Accepted Fixes - -For all comments you've decided to accept: - -1. **Make the code changes** -- edit the relevant files -2. **Format and vet**: - ```bash - gofmt -l internal/ cmd/ # must print nothing -- CI enforces this - go vet ./... - ``` -3. **Run affected tests**, then the full suite: - ```bash - go test ./internal/server/... # or the specific package touched - make test # go test ./... -- full suite, no Docker - ``` - `make lint` (golangci-lint) is a real local gate — install the version `ci.yml` pins with `go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.3`. `gofmt -l` alone does not catch what staticcheck does. -4. **Commit** all fixes together with a conventional-commit message (`fix(scope): ...`, scope = package/feature area, e.g. `router`, `san-cert`, `rpc`): - ```bash - git commit -m "$(cat <<'EOF' - fix(router): address PR review feedback - - - Description of fix 1 - - Description of fix 2 - EOF - )" - ``` -5. **Push** to the remote branch: - ```bash - git push - ``` - ---- - -## Phase 4: Reply to Every Comment - -For **each** unresolved thread, reply: - -### For accepted fixes: - -Reply with what was fixed and the commit SHA: - -```bash -gh api "repos/zoolutions/dash-proxy/pulls//comments//replies" \ - --method POST \ - -f 'body=Fixed in . .' -``` - -### For rejected suggestions: - -Reply with technical reasoning: - -```bash -gh api "repos/zoolutions/dash-proxy/pulls//comments//replies" \ - --method POST \ - -f 'body=' -``` - -### Resolving threads (via GraphQL): - -After replying, resolve the thread: - -```bash -gh api graphql -f query=' - mutation($threadId: ID!) { - resolveReviewThread(input: {threadId: $threadId}) { - thread { isResolved } - } - } -' -f threadId= -``` - -### For general PR comments (not inline review threads): - -Reply directly: - -```bash -gh pr comment --body "" -``` - ---- - -## Phase 5: Verify Completion - -After processing all comments, verify no unresolved threads remain: - -```bash -gh api graphql -f query=' - query($owner: String!, $repo: String!, $pr: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pr) { - reviewThreads(first: 100) { - totalCount - nodes { isResolved } - } - } - } - } -' -f owner=mhenrixon -f repo=kamal-proxy -F pr= -``` - -Report the final tally: how many comments were accepted/fixed, how many were pushed back on, and confirm all threads are resolved. - ---- - -## Response Style - -When replying to comments: - -- **No performative agreement** -- never say "Great point!" or "You're absolutely right!" -- **No gratitude** -- never say "Thanks for catching that" -- **Be direct** -- state the fix or the reasoning, nothing more -- **Reference commits** -- always include the short SHA when a fix was made -- **Be specific** -- when pushing back, reference actual code, not abstract principles - -When pushing back: - -- Use technical reasoning grounded in the actual codebase -- Reference existing patterns if the suggestion contradicts them (e.g. `testRouter(t)`, `testBackend(t, ...)` helpers over hand-rolled servers) -- Reference `CLAUDE.md` / `.claude/rules/*.md` when applicable -- especially fork constraints (naming, branch model, tag grammar, release ordering) -- Explain what would break or what edge case the reviewer missed -- If the suggestion is valid in principle but wrong for this context, say so - ---- - -## Important Notes - -- Always read the actual code before evaluating a comment -- reviewers sometimes misread diffs -- If a comment reveals a genuine bug you missed, fix it without defensiveness -- If multiple comments suggest the same change, implement it once and reference the fix in all replies -- Bot reviewers (CodeRabbit, etc.) sometimes suggest changes that conflict with project or fork conventions -- verify against `CLAUDE.md` and `.claude/rules/*.md`, not just general Go idiom -- Watch for suggestions that are correct for upstream `basecamp/kamal-proxy` but wrong here (e.g. "just use one cert manager") -- the fork's SAN batching and wildcard DNS-01 features are intentional divergence, see `CLAUDE.md` Branch map -- If a new round of review comments appears after your push (from re-review), report that to the user rather than entering an infinite loop - -Now begin by determining the PR number from `$ARGUMENTS` or the current branch. diff --git a/.claude/commands/github-review-failures.md b/.claude/commands/github-review-failures.md deleted file mode 100644 index 5d52a98..0000000 --- a/.claude/commands/github-review-failures.md +++ /dev/null @@ -1,196 +0,0 @@ ---- -description: "Use when CI checks are failing on a PR — fetches failure logs, diagnoses root causes, implements fixes, and pushes until CI is green." -model: sonnet -argument-hint: "PR number (e.g., 41 or #41)" -allowed-tools: Bash(gh pr view:*), Bash(gh pr checks:*), Bash(gh pr diff:*), Bash(gh api:*), Bash(gh run view:*), Bash(git log:*), Bash(git diff:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(go test:*), Bash(go build:*), Bash(go vet:*), Bash(gofmt:*), Bash(golangci-lint:*), Bash(make build:*), Bash(make test:*), Bash(make lint:*), Read, Write, Edit, Glob, Grep, Agent ---- - -# Fix GitHub CI Failures: $ARGUMENTS - -You are diagnosing and fixing CI failures on a `zoolutions/dash-proxy` pull request. Work systematically: identify failures, read logs, diagnose root causes, fix locally, verify, push. - -**Fork boundary first**: confirm the PR's base branch is `dash` (or a feature branch merging into it), never `main` — `main` is a fast-forward-only mirror of upstream and this command must never push a fix there. See `.claude/rules/upstream-sync.md`. - -## Phase 0: Determine the PR Number - -The user may provide a PR number as `$ARGUMENTS`. Parse it flexibly: - -- `PR41`, `PR 41`, `pr41` -> PR 41 -- `41` -> PR 41 -- `#41` -> PR 41 -- Empty/blank -> auto-detect from current branch - -**If no PR number is provided**, detect it automatically: - -```bash -gh pr list --repo zoolutions/dash-proxy --author=@me --head="$(git branch --show-current)" --state=open --json number,title -``` - -If exactly one open PR exists for the current branch, use it. If none or multiple, ask the user. - -Once you have the PR number, confirm it and its base branch: - -```bash -gh pr view --repo zoolutions/dash-proxy --json title,state,url,baseRefName,mergeable -``` - -**Pre-flight: merge conflicts (detection only).** If `mergeable` is `CONFLICTING`, STOP — do not diagnose CI on a conflicted branch (the merge itself may fix or cause the failures). Report the conflict and hand off to `/github-review-pr`, whose Phase A0 owns the resolution runbook (including this fork's merge-forward rules) — this command's toolset deliberately does not include the merge machinery. If `mergeable` is `UNKNOWN`, note it and proceed: the orchestrator resolves the ambiguity; a standalone run shouldn't block on GitHub's recompute. - ---- - -## Phase 1: Identify Failing Checks - -```bash -gh pr checks --repo zoolutions/dash-proxy -``` - -`ci.yml` runs two jobs per push/PR against `main` and `dash`; `docker-publish.yml` only fires on tag push, so it is never a PR check. - -| Check | Job | What it runs | How to get logs | -|---|---|---|---| -| GitHub Actions audit | `lint-actions` | actionlint + zizmor over `.github/workflows/*.yml` | `gh run view --job= --log-failed` | -| Build | `build` (build step) | `make build` (`CGO_ENABLED=0 go build -trimpath -o bin/ ./cmd/...`) | `gh run view --job= --log-failed` | -| Test | `build` (test step) | `make test` (`go test ./...`) | `gh run view --job= --log-failed` | -| Lint | `build` (lint step) | `make lint` (`golangci-lint run`, v2.11.3 pinned in CI) | `gh run view --job= --log-failed` | - -Extract the run ID and job IDs from the check URLs. The URL format is: -`https://github.com/zoolutions/dash-proxy/actions/runs//job/` - -If all checks pass or are pending, report that and stop. - ---- - -## Phase 2: Fetch Failure Logs - -For each failing check, get the logs: - -```bash -# Get the failed job logs (condensed output) -gh run view --job= --log-failed -``` - -If `--log-failed` output is too large or unclear, try: - -```bash -# Full log for a specific job -gh run view --job= --log 2>&1 | tail -100 -``` - ---- - -## Phase 3: Diagnose Each Failure - -For each failure, determine the root cause: - -### Lint Failures (`golangci-lint`) - -**golangci-lint runs locally** once installed at the version `ci.yml` pins — reproduce this check by running `make lint` in this sandbox. Diagnose from the CI log alone: -- Linter name, file:line, and message are printed per finding -- `gofmt` issues surface here too — those you *can* fix and verify locally (see below) - -### Actions Audit Failures (`actionlint` / `zizmor`) - -Look for: -- `actionlint`: YAML/expression syntax errors in `.github/workflows/*.yml` -- `zizmor`: workflow security findings (unpinned actions, injectable `${{ }}` expressions, excess `permissions:`). Workflow files pin actions by SHA with a version comment (e.g. `actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1`) — preserve that pattern in any fix. - -### Build Failures (`make build`) - -Look for: -- Compile errors: type mismatches, unresolved imports, unused variables/imports (Go treats these as hard errors) -- Module errors: run `go mod tidy` and check `go.mod`/`go.sum` are in sync — the fork carries `go-acme/lego/v4` on top of upstream's dep graph; don't drop it while tidying - -### Test Failures (`go test ./...`) - -Look for: -- Test name and package path -- Error class/message and relevant backtrace lines (ignore `testing` framework noise) -- Whether it's a genuine regression vs an environment-only failure - -**Key patterns**: -- `undefined: X` / `X.Y undefined (type *Z has no field or method Y)` -> API drift, check recent upstream merges (`git log --oneline main..dash -- internal/`) -- `panic: runtime error` -> nil deref or index bug, read the failing test's setup -- `expected X, got Y` -> logic bug or the test needs updating for a real behavior change -- Cert/ACME test failures -> check `internal/server/acme/` and `san_cert_manager.go` first; these are the fork's own code, most likely to regress on a `main` merge - ---- - -## Phase 4: Fix Locally - -For each diagnosed failure: - -1. **Read the relevant file** to understand context before fixing -2. **Make the fix** — edit the file -3. **Verify locally** before committing: - -```bash -# Formatting (CI enforces this; golangci-lint itself you cannot run locally) -gofmt -l internal/ cmd/ # must print nothing - -# Compile -go build ./... - -# Targeted test -go test ./internal/server/... -run TestName -v - -# Full validation -make test -``` - -### Fix Priority Order - -1. **`gofmt` / build errors** first (fast, deterministic, verifiable locally) -2. **Test failures** second (may require understanding the code change) -3. **Actions-audit findings** third (usually a pin/permissions tweak in workflow YAML) -4. **golangci-lint findings** last, and flag them for a follow-up CI run since you cannot verify locally before pushing - ---- - -## Phase 5: Commit and Push - -```bash -git add -git commit -m "$(cat <<'EOF' -fix(ci): - -- Fix 1 description -- Fix 2 description -EOF -)" -git push -``` - -Never push directly to `main`. If the PR's base is `main`, stop and tell the user — that branch only fast-forwards from upstream. - ---- - -## Phase 6: Verify - -After pushing, check if CI has been re-triggered: - -```bash -gh pr checks --repo zoolutions/dash-proxy -``` - -If there are still pending checks, report which checks are running and what was fixed. Do NOT poll in a loop — report the status and let the user know. - -If you can identify that certain failures will persist for environmental reasons, flag that explicitly rather than chasing them: - -| Known flaky/env failure | Why | -|---|---| -| golangci-lint findings that can't be reproduced locally | linter isn't installed in this sandbox; CI is the only source of truth, so expect at least one extra push/verify cycle | -| Integration-style tests exercising a published proxy image | need `ghcr.io/zoolutions/dash-proxy` at a real tag; nothing to fix if the image itself hasn't been released yet — see release ordering in `../kamal/.claude/rules/upstream-sync.md` | -| Architecture-dependent test failures surfaced only on `dash`'s multi-arch build | check whether the failure is amd64/arm64-specific before "fixing" logic that's actually fine on the developer's arch | - ---- - -## Important Notes - -- **Read before fixing** — always read the actual failing code before attempting a fix -- **Fix the root cause** — don't add `//nolint` to bypass lint; fix the actual issue -- **Don't fix unrelated failures** — if a test was already failing on `dash`, note it but don't fix it in this PR -- **Respect `CLAUDE.md`'s Never Do list** — no renaming `kamal-proxy` (module/binary/RPC/socket), no suffix tags like `v1.0.0-rc1`, and prefer leaving `Dockerfile`/`Makefile` as basecamp has them so their fixes merge cleanly (per `.claude/rules/upstream-sync.md`) -- **Flaky tests** — if a test passes locally but fails in CI, note it as potentially flaky rather than adding workarounds -- **Don't retry CI blindly** — diagnose first, fix, then push. Each push triggers a full CI run. - -Now begin by determining the PR number and fetching the failing checks. diff --git a/.claude/commands/github-review-pr.md b/.claude/commands/github-review-pr.md deleted file mode 100644 index 0e9f81c..0000000 --- a/.claude/commands/github-review-pr.md +++ /dev/null @@ -1,181 +0,0 @@ ---- -description: "Use when a PR needs full review — resolves merge conflicts with the base first, then fixes CI failures, then addresses unresolved review comments. Conflicts first so CI diagnoses the post-merge reality; failures before comments because comment fixes trigger new CI runs that obscure the original failures." -model: opus -argument-hint: "PR number (e.g., 156 or #156)" -allowed-tools: Bash(gh pr list:*), Bash(gh pr view:*), Bash(gh pr checks:*), Bash(gh pr checkout:*), Bash(gh pr diff:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(gh run view:*), Bash(git log:*), Bash(git blame:*), Bash(git diff:*), Bash(git status:*), Bash(git switch:*), Bash(git fetch:*), Bash(git merge:*), Bash(git merge-tree:*), Bash(git rev-parse:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(gofmt:*), Bash(go test:*), Bash(go build:*), Bash(go mod:*), Bash(make test:*), Bash(make build:*), Read, Write, Edit, Glob, Grep, Agent ---- - -# Review GitHub PR (full pass): $ARGUMENTS - -You are running a full review pass on a **dash-proxy** (`zoolutions/dash-proxy`) pull request. The pass has three phases that MUST run in this order: - -1. **Phase A0: merge conflicts** — bring the branch up to date with its base and resolve any conflicts before anything else. -2. **Phase A: CI failures** — fix anything red before touching review comments. -3. **Phase B: review comments** — only after Phase A leaves CI green (or pending green after a push). - -## Why this order matters - -**Conflicts before failures**: CI results only matter for the code that will actually merge. On a conflicted (or stale) branch you'd diagnose failures against a base that no longer exists — and the conflict resolution itself changes code, invalidating the run you just fixed. Resolving conflicts first means Phase A reads CI for the post-merge reality, and you spend exactly one extra CI cycle instead of two. - -**Failures before comments**: if you fix review comments first, every commit pushes a new CI run. By the time the review-comment fixes finish, the original failure logs are buried under new pipeline runs. Symptoms: - -- The `go test ./...` failure log you needed to read is now from a stale run; the latest run is still in progress on top of your unrelated comment fixes. -- A review-comment fix accidentally repairs the CI failure as a side effect, and you lose the chance to verify the failure was real. -- A review-comment fix accidentally INTRODUCES a CI failure, and you can't tell whether the new failure was pre-existing or your fault. - -Conflicts-first, then failures-first eliminates this confusion. CI is either green or red on a known commit against the current base; the review-comment fixes layer cleanly on top. - -## Fork context — read before touching anything - -This is a maintained fork, not a normal repo. Before either phase, ground yourself in: - -- `CLAUDE.md` — Critical Rules: `kamal-proxy` module/binary/RPC/socket naming is load-bearing, `main` is fast-forward-only (never commit to it), tags are four-segment `vX.Y.Z.N`, no `git push --tags`. -- `.claude/rules/git-workflow.md` — commit message format, branch model, pre-commit checklist. -- `.claude/rules/upstream-sync.md` — merge conflict playbook if the PR touches `internal/cmd/run.go`, `internal/server/config.go`, `internal/server/router.go`, or `internal/server/service.go` (the two cert branches' overlap zone). -- `.claude/rules/testing.md` — 100% coverage floor for Router, LoadBalancer, SANCertManager, cert registry, RPC commands. - -**The PR must target `dash`, not `main`.** If `gh pr view` shows a base of `main`, stop and flag it — that branch only accepts fast-forward merges from upstream. - -## Phase 0: Determine the PR Number - -The user may provide a PR number as `$ARGUMENTS`. Parse it flexibly: - -- `PR156`, `PR 156`, `pr156` → PR 156 -- `156` → PR 156 -- `#156` → PR 156 -- Empty/blank → auto-detect from current branch - -**If no PR number is provided**, detect it automatically: - -```bash -gh pr list --repo zoolutions/dash-proxy --author=@me --head="$(git branch --show-current)" --state=open --json number,title -``` - -If exactly one open PR exists for the current branch, use it. If none or multiple, ask the user. - -Once you have the PR number, confirm it and its base branch: - -```bash -gh pr view --repo zoolutions/dash-proxy --json title,state,url,baseRefName -``` - ---- - -## Phase A0: Merge conflicts - -Check whether the branch merges cleanly into its base: - -```bash -gh pr view --repo zoolutions/dash-proxy --json mergeable,mergeStateStatus,baseRefName -``` - -| `mergeable` | Action | -|-------------|--------| -| `MERGEABLE` | Skip to Phase A. | -| `UNKNOWN` | GitHub is recomputing (common right after pushes, and it can stay UNKNOWN for minutes). Don't poll it — verify **locally**, against the PR's actual head (NOT `HEAD`, which may be some other checked-out branch): `git fetch origin ` and `git fetch origin pull//head`, verify both refs resolve (`git rev-parse --verify origin/^{commit}` and `git rev-parse --verify FETCH_HEAD^{commit}` — a bad ref also exits 1 from merge-tree, so exit code alone can't be trusted), then `git merge-tree --write-tree --name-only origin/ FETCH_HEAD`. Clean exit → no conflicts, skip to Phase A. Exit 1 **with conflict output** → resolve below (the `--name-only` file list is your work list). | -| `CONFLICTING` | Resolve, below. | - -### Which branch do you merge? (fork-specific — decide BEFORE merging) - -`dash` is this fork's main branch and feature branches root off it, so the answer is normally simple: - -1. **Merge `origin/dash`** — this is the sanctioned forward merge. Branches are no longer kept upstream-PR-able, so there is nothing to contaminate. -2. **Only reach for `git merge origin/main`** on an old branch that still roots off `main`, or when you specifically want upstream fixes that have not yet reached `dash`. Re-check mergeability against `main` afterwards. - -Note `git rerere` is enabled: previously-seen conflicts auto-replay their resolutions — review what rerere staged before trusting it. - -### Resolution procedure - -1. Check out the PR's branch (`gh pr checkout `) with a clean tree (`git status`). Stash nothing — if the tree is dirty, stop and ask the user. -2. `git fetch origin ` then **`git merge`** the branch chosen above — MERGE, never rebase. The branch is shared (it has a PR); a rebase would require a force-push, which `.claude/rules/git-workflow.md` forbids on shared branches. -3. Resolve every conflicted file **semantically** — read both sides and produce the version that preserves BOTH changes' intent. Never blanket `--ours`/`--theirs` a source file. Repo-specific rules (the authoritative playbook is `.claude/rules/upstream-sync.md`): - - **`go.mod` / `go.sum`**: never hand-merge `go.sum`. Resolve `go.mod` semantically (union of requires; take the incoming (merged-in) branch's toolchain + dep versions — main's on a main-forward merge — keep `go-acme/lego/v4`), then run `go mod tidy` to regenerate `go.sum`. - - **The cert overlap zone** (`internal/cmd/run.go`, `internal/server/config.go`, `internal/server/router.go`, `internal/server/service.go`): follow the union rules in `.claude/rules/upstream-sync.md`'s conflict playbook — e.g. register `--acme-email`/`--acme-directory` ONCE (pflag panics on duplicates), keep both cert init blocks, keep both `sanCertManager` and `certRegistry`. - - **`Dockerfile`, `Makefile`, `script/release`**: always upstream's/the base's — the fork never edits them. - - **`.github/workflows/*.yml`**: preserve the SHA-pinned-action-with-version-comment pattern (e.g. `actions/checkout@34e11487… # v4.3.1`) — actionlint/zizmor gate these in CI. -4. Run the verification gates BEFORE pushing the merge: - ```bash - gofmt -l internal/ cmd/ # must print nothing - make build - make test - # golangci-lint (make lint) runs locally once installed; run it before pushing rather than expecting CI to be the verifier - ``` -5. Commit the merge (keep git's standard merge-commit message; add a body line naming any non-obvious resolution choice) and `git push` — a merge commit never needs force. - -### Phase A0 exit criteria - -- The PR reports `MERGEABLE` (or the local `git merge-tree` check is clean), AND the merge commit (if one was needed) is pushed. -- If the merge produced changes, CI is now re-running — that's expected; Phase A reads the fresh run. -- If a conflict cannot be resolved with confidence (both sides rewrote the same logic and the correct combination isn't decidable from the code, or the correct combination is genuinely ambiguous), **stop and ask the user** — a guessed resolution that compiles is worse than a question. - ---- - -## Phase A: Run `/github-review-failures` - -Invoke the existing `/github-review-failures` slash command with the same `$ARGUMENTS` value. Its purpose: fix every failing CI check, push, leave the branch in a state where CI is either green or running-pending-toward-green. - -Follow that command's full process. The slash command is at `.claude/commands/github-review-failures.md`. Its workflow: - -1. Identify failing checks via `gh pr checks ` — expect `build`, `test`, `golangci-lint`, `actionlint`/`zizmor` from `ci.yml`. -2. Fetch failure logs via `gh run view --job= --log-failed`. -3. Diagnose root cause for each — `gofmt` drift, `go test ./...` failure, `golangci-lint` finding (reproduce locally with `make lint`), or a build break. -4. Fix locally — `gofmt -l internal/ cmd/` first (fast, deterministic, the one local proxy for `golangci-lint`), then `make test`, then `make build` issues. -5. Verify locally before commit (`gofmt -l internal/ cmd/` must be empty, `make test` green, `make build` clean). -6. Commit + push + report which checks are now running. - -### Phase A exit criteria - -Before moving to Phase B, one of these must be true: - -- All CI checks are green on the latest pushed commit. OR -- All CI checks are pending (running) on the latest pushed commit, AND no checks failed in the most recent completed run on this commit. OR -- A persistent CI failure exists that is **not caused by changes on this branch** (e.g., a flaky test on `dash`, an `actionlint`/`zizmor` finding pre-existing on the base branch). Report this explicitly and proceed to Phase B with the caveat noted. - -If failures persist on this branch's changes, **do NOT proceed to Phase B**. Report what's still failing, what's been tried, and ask the user how to proceed. - ---- - -## Phase B: Run `/github-review-comments` - -Once Phase A's exit criteria are met, invoke `/github-review-comments` with the same `$ARGUMENTS`. Its purpose: address every unresolved review thread on the PR, push fixes, reply with commit SHAs, and resolve the threads. - -The slash command is at `.claude/commands/github-review-comments.md`. Its workflow: - -1. Fetch all unresolved review threads via the GitHub GraphQL API (`repo: kamal-proxy`, `owner: mhenrixon`). -2. Read and categorise each comment (valid fix / invalid suggestion / unclear) against `CLAUDE.md` Critical Rules and the architecture layers — a suggestion to rename `kamal-proxy`, edit `Dockerfile`/`Makefile`/`script/release-dash`, or commit to `main` is an automatic reject, not a judgment call. -3. Implement accepted fixes; verify locally (`make test`, `gofmt -l internal/ cmd/`). -4. Commit all fixes together with a clear conventional-commit message; push. -5. Reply to every thread with the commit SHA (for accepted fixes) or technical reasoning (for rejections). -6. Resolve each thread via the GraphQL `resolveReviewThread` mutation. -7. Verify no unresolved threads remain. - -### Phase B exit criteria - -- All unresolved review threads have been replied to and resolved (or the user has explicitly approved leaving a specific thread open). -- The branch has been pushed with all accepted fixes. - ---- - -## Phase C: Final report - -Before reporting, re-check mergeability once more (`gh pr view --repo zoolutions/dash-proxy --json mergeable`, or the local `git merge-tree` check if UNKNOWN) — the base can move underneath a long pass. If a NEW conflict appeared, loop back to Phase A0. - -After all phases complete, report: - -1. **Phase A0 summary**: whether the branch was conflicted, which files conflicted, how each was resolved (which branch was merged forward, and the merge commit SHA) — or "clean merge, no action". -2. **Phase A summary**: which CI failures were diagnosed and fixed. Note the commit SHAs for the fixes. -3. **Phase B summary**: which review comments were accepted (with commit SHAs), which were pushed back on (with reasoning), and the final unresolved-thread count (should be 0). -4. **End state**: final mergeability + CI status on the latest commit. -5. **Outstanding work**: anything that still needs attention — e.g., CI was pending at the end of Phase B and the user should verify the latest run after the comment fixes; or the PR is ready for `make docker` smoke-testing before merge to `dash`. - ---- - -## Important Notes - -- **Do not interleave the phases.** Don't fix a CI failure, then a review comment, then another CI failure. The whole point of this command is the strict ordering. -- **A new CI failure emerging during Phase B** (e.g., a comment fix breaks `go test ./...`) means looping back to Phase A — fix the new failure before continuing comment work. Likewise, **a new conflict appearing mid-pass** (the base moved) means looping back to Phase A0. These loop-backs are the only allowed reverse directions. -- **If the PR is already merged**, there is nothing to review — report that and stop. (A stale `$ARGUMENTS` or a just-merged PR shows up as `state: MERGED` in Phase 0's confirm step.) -- **If the PR merges cleanly, has no failures AND no unresolved comments**, report "PR is clean" and stop. -- **If `$ARGUMENTS` is the same as the current open PR**, the two child slash commands will see the same PR. They share state through the git branch and the GitHub API, not through any in-process variable. -- **Don't re-implement the child slash commands' logic**. Invoke them and let them do their work. This command is the orchestrator. -- **Never fix by disabling** — no `//nolint`, no skipped tests, no `gofmt`-fighting. Fix the root cause; see `.claude/rules/testing.md` and `.claude/rules/git-workflow.md`. diff --git a/.claude/commands/lfg.md b/.claude/commands/lfg.md deleted file mode 100644 index 5d56ed5..0000000 --- a/.claude/commands/lfg.md +++ /dev/null @@ -1,344 +0,0 @@ ---- -description: "Executes full autonomous engineering workflow with verification. Use when implementing complete features, tackling GitHub issues, or running end-to-end development cycles on the dash-proxy fork." -model: opus -argument-hint: "GitHub issue number/URL or feature description" -allowed-tools: Bash(gh issue view:*), Bash(gh search:*), Bash(gh issue list:*), Bash(gh pr create:*), Bash(gh pr view:*), Bash(make:*), Bash(go test:*), Bash(gofmt:*), Bash(go vet:*), Bash(git:*), Read, Write, Edit, Glob, Grep, Agent ---- - -# LFG - Full Autonomous Workflow - -Execute a complete engineering workflow with verification at each phase, respecting this repo's fork branch model. - -## Phase 0: Branch Setup - -**BEFORE any other work, prepare the git branch. This is a fork — `main` is a fast-forward-only mirror of `basecamp/kamal-proxy`. NEVER commit to it.** - -1. Check the current branch: `git branch --show-current` -2. If NOT on `main`, switch: `git checkout main` -3. Sync with upstream (do not assume `origin/main` is current): `git fetch upstream --tags --prune && git merge --ff-only upstream/main && git push origin main` -4. Create feature branch **off `main`** (`dash` is this fork's main branch): `git checkout -b feature/{description}` (or `fix/{description}`, `issue-{number}-{brief-description}`) -5. The branch merges **forward** into `main` at PR time — never rebase it once pushed. See `.claude/rules/git-workflow.md` and `.claude/rules/upstream-sync.md`. - ---- - -## Phase 1: Understand - -### Step 1: Gather Requirements - -If `$ARGUMENTS` is a GitHub issue number or URL: - -```bash -gh issue view --json title,body,labels,assignees,comments -``` - -If `$ARGUMENTS` is a description, use it directly. - -### Step 2: Define Acceptance Criteria - -**MANDATORY:** Write explicit acceptance criteria: - -- **GIVEN** [context/setup] -- **WHEN** [action taken] -- **THEN** [expected outcome] - -You MUST NOT proceed until you can articulate these clearly. - -### Step 3: Comprehension Gate - -Before proceeding, you must: - -1. State the problem/feature in one sentence -2. Explain WHY this is needed (business context — check `ROADMAP.md` for an existing anchor before inventing one) -3. List what will change from the user's perspective (CLI flag? RPC arg? deploy behavior?) -4. Identify edge cases not explicitly mentioned -5. Explain the data flow or code path involved, in terms of this repo's layers: `cmd/kamal-proxy` → `internal/cmd` (cobra CLI + RPC client) → unix socket RPC → `internal/server` (Router, Service, LoadBalancer, cert managers) - -If you cannot complete ALL five items, investigate further. - -### Step 4: Create Task List - -Create a TaskCreate todo list with specific implementation steps. - ---- - -## Phase 2: Explore - -1. Find related files (Glob/Grep or Explore agent) -2. Read existing patterns in similar features — e.g. how `ServiceOptions` (`internal/server/service.go:82`) or `TargetOptions` (`internal/server/target.go:65`) added a prior knob -3. Understand dependencies and integration points across the three layers -4. Check existing test coverage (`*_test.go` next to the file you're touching) -5. If touching RPC surface, review `internal/server/commands.go` (RPC name registration + arg structs) and the ~9 client call sites in `internal/cmd/` -6. If touching persisted config, check `Service.MarshalJSON`/`UnmarshalJSON` (`internal/server/service.go:273/294`) for round-trip/default-safety against old state files -7. Check `ROADMAP.md` for a code anchor already scoped for this work - ---- - -## Phase 3: Plan - -1. List files to modify with specific changes -2. List new files to create with purpose -3. Identify flag/RPC changes needed: `internal/cmd/{deploy,run}.go` flags → `server.DeployArgs`/`server.GlobalConfig` → RPC arg structs in `internal/server/commands.go` -4. Plan test coverage (TDD: tests FIRST) — table-driven `go test`, `testify/assert`, follow patterns in existing `*_test.go` -5. Update task list with implementation steps -6. Consider backwards compatibility: old state files (JSON-persisted `ServiceOptions`/`TargetOptions`) must still load; never rename module/binary/RPC/socket away from `kamal-proxy` - ---- - -## Phase 4: Implement (TDD) - -### The deviation log (keep it from the first edit) - -The plan is the map; the codebase is the territory. The moment reality forces a choice the plan or issue didn't settle, log it in `implementation-notes.md` at the repo root — one line, at the moment it happens, not reconstructed later: - -- **Deviations** — the plan said X, you did Y, because Z -- **Discoveries** — facts about the codebase the plan didn't know (an options struct that doesn't round-trip, an upstream-owned file in the path, a cert-manager collision with the other fork branch) -- **Judgment calls** — choices the user might have made differently (flag naming, defaults, scope cuts) - -Pick the conservative option and keep going. The log is how the user audits your judgment afterwards. Never commit the file: its contents move into the PR body (Phase 7), then the file is deleted. - -For each logical unit: - -### 4.1: Write Failing Test First - -Create a test that demonstrates the expected behavior. Run it to confirm it FAILS: - -```bash -go test ./internal/server/... -run TestYourNewBehavior -v -``` - -### 4.2: Implement Minimum Code - -Write the MINIMUM code to make the test pass. Follow project patterns: - -| Never Do | Always Do | -|----------|-----------| -| Rename module/binary/RPC service/socket away from `kamal-proxy` | Keep it load-bearing-identical (Dockerfile, kamal gem `exec` calls, RPC registration all depend on it) | -| Hand-roll RPC dialing | Reuse the `net/rpc` client pattern in `internal/cmd/util.go` | -| Add a knob only to `ServiceOptions` and forget the flag | Wire flag (`internal/cmd/deploy.go` or `run.go`) → arg struct (`commands.go`) → `ServiceOptions`/`TargetOptions`/`DeploymentOptions` | -| Skip JSON round-trip for new persisted fields | Update `MarshalJSON`/`UnmarshalJSON` and default old state files safely | -| Ignore the streaming/SSE bypass when touching response middleware | Check `response_buffer_middleware.go:86` bypass logic first | -| Edit `Dockerfile`, `Makefile`, or `script/release*` casually | These stay upstream's on `main`; fork-only changes belong on `dash`/feature branches per `.claude/rules/upstream-sync.md` | - -### 4.3: Refactor - -Once green, refactor while keeping tests passing. - -### 4.4: Validate - -```bash -gofmt -l internal/ cmd/ # must print nothing — CI enforces formatting -go vet ./... -``` - -`make lint` (golangci-lint) is a real local gate — install the version `ci.yml` pins with `go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.3`. `gofmt -l` alone does not catch what staticcheck does. - -### 4.5: Repeat - -Move to next logical unit. Mark task items complete. - ---- - -## Phase 5: Deep Root Cause Analysis (Bug Fixes Only) - -**If this is a bug fix, apply deep investigation before implementing:** - -### Trace the Request Lifecycle - -For the request/connection causing the issue: -- Where did it enter — `internal/server/server.go` listener, then `Router`, then `Service`, then `LoadBalancer.StartRequest` (`load_balancer.go:174`), then `Target.createProxyHandler` (`target.go:293`)? -- What timeout/deadline applies at that point (`ResponseHeaderTimeout` at `target.go:302`, health check intervals, cert renewal windows)? -- What ASSUMPTIONS does the code make at the failure point? -- Which assumption was violated, and WHY? - -### Use Git History - -```bash -git log --oneline -20 -git blame -``` - -- When was the code written — upstream, or one of the fork's cert branches (`san-certificate-batching`, `wildcard-certs`)? -- Has a later `main` merge changed an assumption the fork code relied on? Check `.claude/rules/upstream-sync.md`'s conflict playbook for this file. - -### Map All Callers - -Don't just look at the method that failed: -- Use Grep to find all call sites (RPC client in `internal/cmd/`, RPC server in `internal/server/commands.go`, direct calls within `internal/server/`) -- Different contexts (CLI deploy path vs RPC server vs test harness in `internal/server/testing.go`)? -- Does the error only happen in ONE context? Why? - -### Five Whys - -Keep asking WHY until you reach a meaningful fix point: - -1. Error: X happened -> Why? -2. Because Y -> Why was Y in that state? -3. Because Z -> Why wasn't Z prevented? -4. Because no check existed -> Why not? -5. **THIS** is where the fix belongs - -### Fix Location Principle - -The best fix is usually NOT where the error is raised: -- Nil target in load balancer -> fix in `Service` that should never register a nil target -- Certificate not found -> fix in the manager that should ensure provisioning before serving -- Race condition -> fix at the state-file / mutex boundary, not with a retry loop -- RPC arg mismatch -> fix the arg struct/version contract in `commands.go`, not the symptom at the call site - -**Ask: "Where is the EARLIEST point I could prevent this error?" Fix there.** - -### Unacceptable Superficial Fixes -- DO NOT DO THESE - -- `if err != nil { return nil }` swallowing an error without understanding why it occurs -- Ignoring an error return (`_ = fn()`) to silence a failure path -- Nil-checking a pointer defensively without understanding why it could be nil -- Wrapping goroutines in blanket `recover()` to hide panics -- Increasing a timeout to mask a deadlock/race instead of fixing it - -**These HIDE bugs. The root cause continues causing issues elsewhere.** - ---- - -## Phase 6: Verify - -**ALL of these must pass before committing:** - -```bash -gofmt -l internal/ cmd/ # Formatting — must be empty -make test # go test ./... -go vet ./... -``` - -If you have `golangci-lint` installed locally, also run `make lint` — but its absence is not a blocker; CI runs it on `main` and `dash`. - -### Solution Verification - -Re-read the original requirements and verify: -- "If I were the requester, would I consider this fully resolved?" -- "Have I addressed the ROOT CAUSE, not just the symptom?" -- "Do my tests prove the issue is ACTUALLY fixed, not just suppressed?" -- "Does this maintain backwards compatibility with existing state files and the `kamal` gem's RPC/CLI expectations?" - ---- - -## Phase 7: Commit & PR - -### Commit - -```bash -git add -git commit -m "$(cat <<'EOF' -feat(scope): brief description - -## Summary -[What changed and why] - -## Test Coverage -- TestX: validates requirement X -- TestY: validates edge case Y - -## Verification -- [x] gofmt -l internal/ cmd/ clean -- [x] make test passes -EOF -)" -``` - -Scope = the package/feature area, e.g. `san-cert`, `wildcard-certs`, `router`, `rpc`. See `.claude/rules/git-workflow.md` for commit conventions. - -### Push & PR - -**PRs target `dash`, never `main`** — `main` only ever fast-forwards from upstream. - -```bash -git push -u origin $(git branch --show-current) - -gh pr create --base main --title "feat(scope): brief description" --body "$(cat <<'EOF' -## Summary -- Key change 1 touching `internal/server/foo.go` -- Key change 2 - -Closes # - -## Test plan -- [ ] Scenario 1 -- [ ] Scenario 2 -EOF -)" -``` - -**Markdown inside the quoted heredoc is literal — do not escape.** The single-quoted `<<'EOF'` delimiter disables shell expansion on the body, so: - -- Write backticks as backticks: `` `foo` ``. Do NOT write `\`foo\``; that writes a literal backslash-backtick and breaks the code span. -- Write dollar signs as-is: `$HOME`. No escaping needed. -- Write backslashes as-is: `\n` stays `\n`. - -The body is copied verbatim into the PR / commit message. If you would not type a backslash in a GitHub comment, do not type one in the heredoc. - -If the body is long or contains many backticks / tables, prefer writing it to a temp file and passing `--body-file`: - -```bash -cat > /tmp/pr-body.md << 'EOF' -## Summary -...any markdown... -EOF -gh pr create --base main --title "..." --body-file /tmp/pr-body.md -rm /tmp/pr-body.md -``` - -The `--body-file` path avoids the double-layer of shell interpretation entirely and makes long PR bodies easier to read in the terminal buffer. - -The PR body MUST end with a `## Deviations & judgment calls` section copied from -`implementation-notes.md` (then delete the file). If the plan held completely, -write "None — the plan held." This section is read FIRST in review — it is the -audit trail for every decision the plan didn't make. - ---- - -## Release (only if this workflow ends in a release) - -Not part of the default flow — only after a PR is merged to `dash` and a release is explicitly requested. Full runbook: `.claude/rules/upstream-sync.md`. - -```bash -git checkout dash -script/release-dash v1.0.0.0 # validates vX.Y.Z.N grammar, runs make test, tags, pushes the tag -# CI publishes ghcr.io/zoolutions/dash-proxy:v1.0.0.0 (+ :latest) -docker buildx imagetools inspect ghcr.io/zoolutions/dash-proxy:v1.0.0.0 # verify amd64+arm64 -``` - -The image has no version command — the tag IS the version. Release the proxy **before** the `dash` gem; the gem's `MINIMUM_VERSION` must name an already-published tag. - ---- - -## Phase 8: Comprehension Close-Out - -The tests prove the CODE is right; this phase keeps the USER's mental model right. After the PR is up, end your final message with: - -1. **The decisions, not the diff** — the 3–5 non-obvious choices in this change someone must understand to maintain it. Lead with anything from the deviation log; the user has never seen those. -2. **Three merge-gate questions** the user should be able to answer before merging. If any answer isn't obvious to them, offer a walkthrough — an unanswerable question is comprehension debt, and merging anyway is how it compounds. - ---- - -## Verification Checklist - -- [ ] All acceptance criteria met -- [ ] Tests written BEFORE implementation -- [ ] `gofmt -l internal/ cmd/` clean -- [ ] `make test` passes -- [ ] `go vet ./...` clean -- [ ] Backwards compatibility maintained (state files, RPC contract, `kamal-proxy` naming untouched) -- [ ] Branch rooted off `main`, PR opened against `main` -- [ ] PR created with description -- [ ] PR body ends with `## Deviations & judgment calls` (from implementation-notes.md, since deleted) -- [ ] Comprehension close-out delivered (decisions + three merge-gate questions) - ---- - -## Handoff - -When complete: -- All phases executed -- Verification passed -- PR created against `main` and linked - -Now, execute this workflow for the provided issue or feature. diff --git a/.claude/commands/plan.md b/.claude/commands/plan.md deleted file mode 100644 index 16f4317..0000000 --- a/.claude/commands/plan.md +++ /dev/null @@ -1,100 +0,0 @@ ---- -description: "Investigates the codebase, designs a solution, and produces a durable plan artifact — a GitHub issue or a plan markdown under docs/plans/. Read-only: never edits Go source. Use before implementation for anything non-trivial." -model: fable -argument-hint: "issue | md | " -allowed-tools: Bash(gh issue create:*), Bash(gh issue list:*), Bash(gh issue view:*), Bash(gh search:*), Bash(gh label list:*), Bash(git log:*), Bash(git diff:*), Bash(git branch:*), Bash(date:*), Read, Grep, Glob, Write, Agent, AskUserQuestion ---- - -# Plan — design expensive, execute cheap - -You are the planning specialist for **dash-proxy**, the Go fork of `basecamp/kamal-proxy`. This command runs on the most capable model deliberately: the thinking happens here, the execution happens later on a cheaper model. That split only works if the plan is **self-contained** — an executor with none of this session's context must be able to implement it without guessing. - -## Output mode from $ARGUMENTS - -| $ARGUMENTS starts with | Artifact | -|------------------------|----------| -| `issue` | GitHub issue on `zoolutions/dash-proxy` (default — feeds directly into implementation) | -| `md` or `file` | Markdown file at `docs/plans/YYYY-MM-DD-.md` (date from `date +%F`) | -| anything else | GitHub issue | - -## Hard constraints - -- **Read-only for source code.** Never edit `.go` files, never commit, never create branches. The only file you may Write is a new plan markdown under `docs/plans/`. -- **Never reproduce secrets** (ACME account keys, DNS provider API tokens, ghcr credentials) in the plan, even redacted ones you encounter while reading config or state files. -- **Dedupe before creating an issue**: `gh issue list --search "" --repo zoolutions/dash-proxy` — if an existing issue covers this, extend it in your summary instead of duplicating. -- **Respect the fork boundary.** `dash` is this fork's main branch; plan work onto a feature branch rooted off `main`, merging back into `main`. `main` is a fast-forward-only mirror of upstream — never plan work that lands there. Upstream mergeability is **not** a constraint: design what is best for `dash` and diverge from basecamp where that is better. -- **Check upstream before porting.** When an issue says "port basecamp/kamal-proxy#N", verify that PR is still open and unmerged before planning a port — several have been superseded or merged since the issues were written (#63→#225, #197→#228). Diff against `upstream/main` first. - -## Phase 1 — Investigate - -Protect this session's context: delegate mechanical exploration to cheaper subagents and keep Fable for judgment. - -1. Fan out Explore agents for file discovery and call-site sweeps (e.g. "find every RPC client call site for `commands.go`"); use a general-purpose agent when a subsystem needs to be read and summarized. Launch independent explorations in parallel — see `.claude/rules/agents.md` for this repo's exploration surfaces (`internal/cmd` = CLI/RPC client, `internal/server` = router/service/load-balancer/cert managers). -2. Read the load-bearing files yourself — the ones the design decision actually hinges on. Don't design from subagent summaries alone. -3. Check `ROADMAP.md` first — planned work already has a code anchor (e.g. `internal/server/domain_renewal.go`, `internal/server/load_balancer.go:174`). If $ARGUMENTS matches a roadmap item, start from its anchor and evidence links instead of re-deriving them. -4. Check the architecture layers and Critical Rules in `CLAUDE.md` — `kamal-proxy` naming is load-bearing (module/binary/RPC/socket), the branch map, and the "image tag IS the version" model constrain any design. -5. Check `git log` and `git branch -a` for recent related work on `main`, `dash`, `san-certificate-batching`, `wildcard-certs` — the design should extend it, not fight it or duplicate a branch that already carries it. - -## Phase 2 — Surface the unknowns (blindspot pass + interview) - -Investigation tells you what the codebase says; this phase finds what the REQUEST doesn't say. Run it BEFORE designing — a wrong assumption caught here costs one question; caught in review it costs a rewrite. - -1. **Blindspot pass.** Write down the unknowns you are carrying into the design: - - decisions the request leaves open (flag naming, defaults, which options struct the knob belongs in, whether it persists in service state) - - edge cases the codebase makes possible that the request never mentions (old state files without the field, rollout while a deployment is in flight, cert manager interaction) - - anything with no precedent in this repo or in `ROADMAP.md` — flag it explicitly as unknown-unknown territory - - whether the feature needs a gem-side half in `../kamal`, which forces release ordering (proxy image before gem) -2. **Interview the user** with AskUserQuestion, one question at a time, prioritized by blast radius: architecture-changing answers first, then the operator-facing surface (CLI flags, RPC args, persisted state), then ergonomics. Rules: - - Skip anything the codebase, `CLAUDE.md`, `ROADMAP.md`, or an existing issue already answers. - - 2–5 questions is the sweet spot; zero is fine when the request is genuinely unambiguous — say so rather than inventing questions. - - Every question offers concrete options with a recommended default, never an open-ended essay prompt. -3. **Record the answers** in the plan's Decision section as `Settled in interview:` bullets — constraints the executor must not re-litigate. - -## Phase 3 — Design - -- Develop 2-3 candidate approaches with real tradeoffs. Pick one and say why; record why the others lost. -- The chosen design must respect project invariants: never rename module/binary/RPC/socket away from `kamal-proxy`; new per-service knobs go in `ServiceOptions` (`internal/server/service.go:82`), per-target in `TargetOptions` (`internal/server/target.go:65`), one-shot in `DeploymentOptions` (`internal/server/service.go:76`); flags register in `internal/cmd/deploy.go` / `internal/cmd/run.go`; RPC arg structs in `internal/server/commands.go:19-63`; anything JSON-persisted must round-trip `Service.MarshalJSON/UnmarshalJSON` and be default-safe against old state files. -- If the change touches `internal/server/san_cert_manager.go`, `internal/server/san_cert_issuance.go`, or `internal/server/acme/`, flag the merge-conflict surface against `main` per `.claude/rules/upstream-sync.md`'s conflict playbook — design the diff to minimize collision with the other cert branch. -- If the feature needs a gem-side flag to reach `kamal deploy`, note the plumbing point in `../kamal/lib/kamal/configuration/proxy.rb` (or the three-file path when the loadbalancer tier must carry it too) so the plan doesn't stop at the proxy half. -- Decide the test strategy: table-driven `_test.go` alongside the changed package, `go test ./...` scope, whether a benchmark belongs in `make bench`. - -## Phase 4 — Emit the plan artifact - -Use this structure for the issue body or markdown file. Every section is load-bearing — an executor uses Context to avoid re-discovery, Steps to act, Gates to verify, Boundaries to stop. - -```markdown -# - -## Problem / Goal -<What's wrong or missing, who it affects, what done looks like.> - -## Context (read these first) -<Bullet list: `internal/server/file.go:line` — why it matters to this change. Include CLI/RPC, router/service, load-balancer, and cert-manager layers as relevant. Self-contained: no references to "as discussed" or this session.> - -## Decision -<Chosen approach and rationale. Then: alternatives considered and why each was rejected. End with `Settled in interview:` bullets for every constraint the user confirmed in the interview phase — the executor must not re-litigate these.> - -## Implementation steps -<Ordered, small, each mapped to the appropriate architecture layer (RPC arg struct → server handler → CLI flag → docs). Tests come before or alongside the code they cover. Name exact files to create or change.> - -## Verification gates -<Exact commands + expected outcome:> -- `make test` — all green (`go test ./...`) -- `gofmt -l internal/ cmd/` — empty output -- `make build` — `bin/kamal-proxy` builds clean -- (if touching cert managers or the request path) `make docker && docker run --rm kamal-proxy kamal-proxy -h` — image smoke test - -## Out of scope -<Explicit boundaries — the adjacent things an eager executor must NOT do. Always include: no edits to Dockerfile/Makefile/script/release (upstream's), no renaming kamal-proxy module/binary/RPC/socket, no touching main.> - -## Execution -Implement on a branch rooted off `main` (or the relevant feature branch — `san-certificate-batching` / `wildcard-certs` — if this extends fork-only cert work), PR against `main`. -``` - -For GitHub issues: create with `gh issue create --repo zoolutions/dash-proxy --title "..." --body-file <tmpfile>`. Write the body to a temp file first; do not use inline heredoc with `--body` (code fences get mangled by shell interpolation). - -For markdown files: Write to `docs/plans/YYYY-MM-DD-<slug>.md`. Leave it uncommitted — committing is the user's call. - -## Phase 5 — Handoff - -Report back: link to the issue (or file path), the chosen approach in 2-3 sentences, which branch it roots off, and the exact next command. Stop there — do not start implementing. diff --git a/.claude/commands/tdd.md b/.claude/commands/tdd.md deleted file mode 100644 index 26d7e28..0000000 --- a/.claude/commands/tdd.md +++ /dev/null @@ -1,182 +0,0 @@ ---- -description: "Use when implementing any feature or fixing any bug -- enforces RED-GREEN-REFACTOR: write failing test first, implement minimum code to pass, then refactor." -model: sonnet -argument-hint: "[package or file, e.g. internal/server/san_cert_manager]" -allowed-tools: Read, Edit, Write, Bash(go test:*), Bash(make test:*), Bash(make build:*), Bash(gofmt:*) ---- - -# TDD Command - -Enforce test-driven development with RED -> GREEN -> REFACTOR. Applies to both halves of this fork pair: Go in this repo (kamal-proxy), Ruby in `../kamal` (the `dash` gem). - -## The TDD Cycle - -```text -RED -> GREEN -> REFACTOR -> REPEAT - -RED: Write a failing test (test MUST fail first) -GREEN: Write MINIMAL code to pass (nothing more) -REFACTOR: Improve code while keeping tests green -REPEAT: Next scenario -``` - -## When to Use - -- Implementing new features (SAN cert batching, wildcard DNS-01, RPC commands) -- Fixing bugs (write the test that reproduces the bug FIRST) -- Refactoring `internal/server` (router, load balancer, cert managers) or `internal/cmd` -- Changing RPC call sites — every `internal/cmd` command dials `kamal-proxy.sock`; a broken contract fails silently at runtime, not compile time -- Touching the gem side (`../kamal`): CLI commands, `Kamal::` classes - -## Workflow - -### Step 1: Write Failing Tests (RED) - -Go — table-driven test with `testify`, colocated as `<file>_test.go` in the same package: - -```go -// internal/server/san_cert_manager_test.go -package server - -import ( - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestSANCertManager_RegisterDomain_NotReady(t *testing.T) { - tmpDir := t.TempDir() - config := SANCertManagerConfig{ - Email: "test@example.com", - CachePath: filepath.Join(tmpDir, "certs"), - StatePath: filepath.Join(tmpDir, "acme.state"), - } - - manager, err := NewSANCertManager(config) - require.NoError(t, err) - - err = manager.RegisterDomain("app.example.com", "service1") - assert.ErrorIs(t, err, ErrManagerNotReady) -} -``` - -Ruby (gem, `../kamal`) — Minitest, mirrors upstream kamal's own suite layout: - -```ruby -# test/commands/dash_test.rb -class CommandsDashTest < ActiveSupport::TestCase - test "release image before gem enforces minimum version" do - command = Kamal::Commands::Dash.new(config) - - assert_equal "ghcr.io/zoolutions/dash-proxy:v0.9.2.1", command.minimum_version_image - end -end -``` - -### Step 2: Run Tests — Verify FAIL - -```bash -# Go — this repo -go test ./internal/server/... -run TestSANCertManager_RegisterDomain_NotReady -v - -# Ruby — ../kamal -cd ../kamal && bin/test test/commands/dash_test.rb -``` - -```text -FAIL - undefined: ErrManagerNotReady / NoMethodError -``` - -**Tests MUST fail before implementing.** This confirms: -- Tests are actually running (not silently skipped) -- Tests are testing the right thing -- Implementation doesn't already exist - -### Step 3: Implement Minimal Code (GREEN) - -Write the minimum code to make the test pass. No speculative branches, no unrequested flags. - -### Step 4: Run Tests — Verify PASS - -```bash -go test ./internal/server/... -run TestSANCertManager_RegisterDomain_NotReady -v -# PASS -``` - -### Step 5: Refactor (IMPROVE) - -Keep tests green while you: -- Extract functions to cut complexity -- Improve naming -- Remove duplication -- Check concurrency safety — `internal/server` is accessed from RPC handlers and health-check goroutines concurrently; guard shared state (see `sync.Mutex` usage in `load_balancer.go`, `san_cert_manager.go`) - -### Step 6: Run Full Suite + Format Gate - -```bash -make test # go test ./... -gofmt -l internal/ cmd/ # must print nothing — CI enforces this, it's not auto-fixed -``` - -`make lint` (golangci-lint) is a real local gate — install the version `ci.yml` pins with `go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.3`. `gofmt -l` alone does not catch what staticcheck does. - -## Coverage Requirements - -No coverage tool is wired into CI (`ci.yml` runs build + test + golangci-lint + actionlint/zizmor, no codecov). Treat these as review bar, not a gate: - -| Code Type | Minimum Coverage | -|-----------|------------------| -| All code | 80% | -| `internal/server/san_cert_manager.go`, `san_cert_issuance.go` (cert issuance) | 100% | -| `internal/server/router.go`, `load_balancer.go` | 100% | -| `internal/cmd/*` (RPC client commands) | 100% | -| Gem `lib/kamal/commands/dash.rb` (proxy image gating) | 100% | - -## Test Types to Include - -### Unit Tests (config structs, cookie scope, buffer, health check) -- Happy path -- Edge cases (empty target pool, expired cert, zero-length body) -- Error conditions (`require.Error` / `assert.ErrorIs`) - -### Integration Tests (router, service, RPC round-trip) -- `internal/cmd` command -> unix socket -> `internal/server` RPC handler -> response -- Deploy/rollout lifecycle (`rollout_controller_test.go` pattern: drain, swap, health-gate) -- SAN batching / wildcard DNS-01 issuance against a staging ACME directory (`acme.DefaultStagingDirectory`), never production Let's Encrypt - -### Cross-repo Tests (gem side, when changing the RPC contract) -- Gem's integration tests exec the real `kamal-proxy` binary — a proxy-side RPC signature change breaks them at the boundary, not at compile time. Build `bin/kamal-proxy` locally and point `../kamal`'s test config at it before changing shared commands. - -## Best Practices - -**DO:** -- Write the test FIRST, before any implementation -- Run tests and verify they FAIL before implementing -- Write MINIMAL code to make tests pass -- Refactor only after tests are green -- Use `httptest.NewServer` / `t.TempDir()` to avoid real network and filesystem state (see `health_check_test.go`, `san_cert_manager_test.go`) -- Test against `acme.DefaultStagingDirectory`, never real Let's Encrypt, in cert tests -- Table-drive scenarios with `t.Run(name, func(t *testing.T) {...})` subtests - -**DON'T:** -- Write implementation before tests -- Skip running tests after each change -- Write too much code at once -- Ignore failing tests -- Test implementation details — test behavior (RPC response, HTTP status, routing decision) -- Skip testing error paths (`RegisterDomain` when registry isn't ready, RPC dial failure, cert renewal failure) -- Touch `Dockerfile`, `Makefile`, or `script/release*` to make a test pass — those stay upstream's, per `.claude/rules/upstream-sync.md` - -## Checklist - -- [ ] Tests written BEFORE implementation -- [ ] Tests fail initially (RED phase verified) -- [ ] Minimal code written to pass (GREEN) -- [ ] Code refactored with tests still passing -- [ ] `gofmt -l internal/ cmd/` clean -- [ ] `make test` passes full suite -- [ ] Edge cases and error paths covered -- [ ] RPC contract changes verified against `../kamal` integration tests if `internal/cmd` or `internal/server/commands.go` touched -- [ ] `kamal-proxy` binary/RPC/socket naming untouched (see `CLAUDE.md` Critical Rules) diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..90b8653 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,13 @@ +{ + "extraKnownMarketplaces": { + "zoolutions": { + "source": { + "source": "github", + "repo": "zoolutions/claude-plugins" + } + } + }, + "enabledPlugins": { + "lode@zoolutions": true + } +} diff --git a/.gitignore b/.gitignore index 5e56e04..f08302c 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ /bin +/lode/tmp/ diff --git a/CLAUDE.md b/CLAUDE.md index 68248ab..cdc959f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,6 +2,10 @@ **dash-proxy** (`zoolutions/dash-proxy`). Started as a fork of [basecamp/kamal-proxy](https://github.com/basecamp/kamal-proxy); the break is now clean — no upstream remote, no sync branch, their code arrives only by deliberate cherry-pick if ever. Carries the cert features they don't ship: SAN certificate batching and wildcard certs via DNS-01. Published as `ghcr.io/zoolutions/dash-proxy`; the Go module, binary, RPC service, socket, and image title label all stay `kamal-proxy` on purpose until the server-artifact rename ships a migration bridge. Consumed by the `dash` gem in `../kamal`. +## Memory + +Durable project memory lives in `lode/` (index: `lode/lode-map.md`). Read it before exploring the code. `lode/review/` holds accepted review findings as rules about the system; `/lode:gate` enforces them before any push, and `/lode:learn` adds to them. `lode/workflow.md` is the profile the shared `/lode:` workflow commands read. + ## Tech Stack - **Go**: version from `go.mod` (tracks upstream's toolchain bumps) @@ -70,24 +74,35 @@ Tag push (`vX.Y.Z.N`) → `.github/workflows/docker-publish.yml` → multi-arch ## Slash Commands +Shared commands come from the `lode@zoolutions` plugin and read `lode/workflow.md` for everything repo-specific. + +| Command | Purpose | +|---------|---------| +| `/lode:lfg` | Full autonomous workflow: branch off `main` → understand → plan → TDD → verify → PR into `main` | +| `/lode:plan` | Read-only planning → GitHub issue or `docs/plans/` markdown (execute with `/lode:lfg`) | +| `/lode:tdd` | Enforce RED → GREEN → REFACTOR with Go table-driven tests | +| `/lode:review-pr` | Full PR pass: conflicts, then CI failures, then review comments | +| `/lode:finish-prs` | Drive a stack of open PRs to merge-ready, one at a time, in order | +| `/lode:debug-flaky` | Root-cause an intermittent test — evidence → repro → stress-proofed fix | +| `/lode:gate` | Pre-PR gate: fresh-context review against the rules and `lode/review/`, loops until clean | +| `/lode:learn` | Write accepted review findings into `lode/review/` | +| `/lode:sync` | Keep `lode/` true to the code after a change; `audit`, `handover` | + +Repo-local commands that the plugin does not cover: + | Command | Purpose | |---------|---------| -| `/lfg` | Full autonomous workflow: branch off `main` → understand → plan → TDD → verify → PR into `main` | -| `/plan` | Read-only planning → GitHub issue or `docs/plans/` markdown (execute with `/lfg`) | | `/architect` | Coordinate work across the cmd → RPC → server layers | -| `/tdd` | Enforce RED → GREEN → REFACTOR with Go table-driven tests | | `/security` | Audit TLS/cert handling, request parsing, header forwarding, ACME, the unix socket | | `/perf` | Baseline vs `main` in a worktree via `make bench` on the real hot paths | -| `/review-pr` | Review a PR for pattern + fork-constraint compliance | -| `/github-review-pr` | Full PR pass: fix CI failures, then process review comments | -| `/github-review-failures` | Diagnose + fix CI failures until green | -| `/github-review-comments` | Process unresolved PR review comments | +| `/review-pr` | Review a PR for pattern + constraint compliance (read-only; `/lode:review-pr` is the one that fixes) | -Commands pin a model tier via frontmatter aliases (`sonnet` implementation, `opus` orchestration/security/review, `fable` read-only planning) so they track the latest model per tier. +The repo-local commands pin a model tier via frontmatter aliases (`sonnet` implementation, `opus` orchestration/security/review, `fable` read-only planning) so they track the latest model per tier. ## More Documentation - `ROADMAP.md` — proxy-side roadmap with code anchors (strategy + sequencing in ../kamal/ROADMAP.md) - `.claude/rules/` — coding-style, git-workflow, testing, agents, performance, upstream-sync -- `.claude/commands/` — the slash commands above +- `.claude/commands/` — the repo-local slash commands above +- `lode/` — durable memory: subsystem summaries, terminology, practices, the workflow profile and the review rules - Gem fork: `../kamal/CLAUDE.md` — gem-side contract and release ordering diff --git a/lode/cache/summary.md b/lode/cache/summary.md new file mode 100644 index 0000000..0023ad0 --- /dev/null +++ b/lode/cache/summary.md @@ -0,0 +1,140 @@ +# Response cache + +An opt-in shared HTTP cache sitting between the per-service checks and the load +balancer. Storing is opt-in twice over: the service must be deployed with +`--cache`, and the target must mark the response `public` with a lifetime. +Because the cache sits *below* every access check, a stored response can only +ever reach a client the target would have been asked on behalf of. + +The rules follow RFC 9111 and RFC 5861, and are deliberately stricter than +either in two places: storing requires an explicit `public` directive rather +than a heuristic, and a request carrying credentials never touches the cache. + +## Stores + +`NewCacheStore` (`internal/server/cache_store.go`) accepts three `--cache-store` +forms; `ParseCacheStoreURL` rejects a typo at startup rather than at the first +cached request. + +| Form | Type | Shared? | Leases? | +|---|---|---|---| +| `memory`, or empty | `memoryCacheStore` | no — per process | no | +| `file://<dir>` | `fileCacheStore` | no — single host | no | +| `redis://`, `rediss://` | `redisCacheStore` | yes — the fleet | yes | + +A bare `file://` naming no directory is a typo rather than a request for some +default location, so it is refused at startup instead of quietly caching into +the working directory. `DefaultCacheMemorySize` (256 MB) caps the in-process +store and the file store's index budget; `DefaultCacheStoreTimeout` (100ms) +bounds each shared-store operation, short on purpose — a cache that takes +longer than that has already cost more than the miss it was meant to save. + +Every implementation **fails open**: an unreachable store turns lookups into +misses and writes into logged errors, never into a failed request. + +`CacheLeaser` is the optional half, implemented only by the shared store. On a +single proxy the in-process single flight already *is* the lease, and +arbitrating with nobody would cost a round trip to learn what is already known +— so the memory and file stores deliberately have none of these methods, and +the middleware's nil check is the whole of what a single-node deployment pays. +Every leaser method fails open too: a lease in any doubt **grants**, because a +duplicate fetch is cheaper than an origin outage. `ProbeLease` returns the entry +and the lease-held flag in one round trip on purpose — a probe that took two +could see the lease released between them and give up on an entry that was +already there. + +## Two-level keys: index and variant + +A resource that negotiates is stored as two records +(`internal/server/cache_variant.go`): a **variant index** at the resource's own +key naming the fields it varies on, and a **variant** at the key those fields' +values produce. A non-varying resource — the overwhelming majority — is one +record at its own key, one lookup, one write, which is why the index is a +separate record rather than a wrapper around every entry. + +`entryIsResponse` (`StatusCode >= 100`) and `entryIsIndex` (`StatusCode == 0` +and a non-empty `VaryOn`) are how the two are told apart, and every path that +hands bytes to a client goes through the former. + +`cacheKeyVersion` (`"2"`) salts every key this build computes so entries written +by a proxy predating variant indexes live in a disjoint key space. This is not +belt and braces: gob silently drops fields the receiving type lacks, so an older +binary decodes an index as a response with `StatusCode` 0 and a nil body, and +replaying that calls `WriteHeader(0)`, which panics `net/http`. The cost is one +lifetime of cold shared cache at one release, on a fleet that just restarted. + +`maxVaryFields` (8) bounds how many dimensions one response may negotiate on. +`unkeyableVaryFields` — `authorization`, `cookie`, `referer`, `user-agent` — +differ for practically every client, so keying on them would store one copy per +client. Naming one in `--cache-vary-header` is the override, and it is +deliberately awkward: it moves the field into the primary key for the whole +service. `DefaultCacheMaxVariants` (32) bounds representations per resource. + +`Accept-Encoding` is the one Vary dimension the key deliberately does not carry: +the cache stores the representation the target produced and sits *inside* the +compression middleware, so every response is encoded for its own client on the +way out and one stored entry serves them all. A response the target encoded +itself is refused instead (`cacheRefusalContentEncoding`), because that one +really is encoding-specific. + +## Serving and fetching + +`CacheMiddleware.ServeHTTP` (105-133) looks up, following an index to its +variant when it finds one, and hands off to `serve` (137-157): + +- needs revalidation → replay the stale copy now and refresh behind it (RFC 5861) +- servable → replay as a hit +- otherwise → fall through to `fetch` + +`fetch` (161-222) keys the in-process single flight on the **resource**, not the +variant: with a two-level scheme nobody knows which variant this is until the +response arrives. That makes the key a collapsing hint and nothing more — +`entryAnswers` re-derives the variant from each waiter's own headers, so a wrong +guess costs a fetch and never a wrong body. A follower whose variant does not +match the leader's entry goes to the origin itself, and does **not** register as +a new leader: whatever made the response unshareable will do so again. + +Only the node's leader touches the lease, so fleet-wide lease traffic is one +operation per node per key rather than one per request. When a fetch turns out +to have nothing storable, the key is handed back immediately rather than held +for the whole TTL. + +A request may refuse the stored copy for itself and still populate the cache for +everyone behind it — which is what makes a reload useful. + +## Refusals + +`cacheRefusal` (`internal/server/cache_refusal.go`) is a low-cardinality reason +a response was not stored, surfaced as `cache_refusals_total{reason=…}` and, for +the ones an operator can act on, as advice. There are 18 reasons plus +`cacheRefusalNone` (the empty string, meaning storable): + +`disabled`, `status`, `not_public`, `no_store`, `private`, `no_cache`, +`no_lifetime`, `set_cookie`, `content_range`, `content_encoding`, +`event_stream`, `vary`, `vary_unkeyable`, `vary_too_many`, `variant_limit`, +`head_request`, `too_large`, `hijacked`. + +`cacheRefusal.advice` (54-75) names the lever for the subset an operator can do +something about; the rest report the reason only. The offending Vary *field* is +logged but never used as a metric label — an application-chosen string there is +a cardinality bomb. + +`cacheableStatuses` (`internal/server/cache_policy.go`) is RFC 9111 §15.1's list +minus 206: a partial response describes a byte range this proxy never tracks, +and storing one would answer a later full request with a fragment. + +`s-maxage` wins outright over `max-age` when present — it exists precisely to +say something different to shared caches, which is the only kind this is. +`must-revalidate` and `proxy-revalidate` are folded together for the same +reason. `MaxTTL` caps the lifetime the target asks for, so a mistaken +`s-maxage` of a year cannot pin content until the next restart. + +## Keys and namespacing + +`cacheKeyPrefix` (`kp:c:`) namespaces every key this proxy writes, so a Redis +instance shared with an application's own data cannot collide with it. + +## Related + +- `../request-path/summary.md` — where the cache sits in the per-request order +- `../observability/summary.md` — the five cache metric families diff --git a/lode/certs/store-and-recovery.md b/lode/certs/store-and-recovery.md new file mode 100644 index 0000000..10e6d81 --- /dev/null +++ b/lode/certs/store-and-recovery.md @@ -0,0 +1,151 @@ +# Certificate store: export, verify, restore, import + +The certificate estate is four things in the data directory: `acme.state` (the +index), `certs/` (one directory per certificate plus the ACME account keys), +`dynamic-domains.state`, and nothing else. `CertStorePaths` +(`internal/server/cert_store_export.go`) names them; +`Config.CertStorePaths()` builds it. + +The archive mirrors that layout exactly, so a restore is a faithful extraction: + +``` +acme.state +dynamic-domains.state +certs/acme_user.json primary ACME account key +certs/acme_user_staging.json per-directory account keys (--tls-staging) +certs/acme_user_<8 lowercase hex>.json +certs/<sanitized cert id>/cert.pem +certs/<sanitized cert id>/key.pem +``` + +`isExtraAccountKeyFile` is the **closed** predicate for the per-directory +names — `acme_user_staging.json`, or `acme_user_` + exactly 8 lowercase hex +digits + `.json` — shared by the exporter and the archive reader so nothing is +exported that a restore would refuse, and no arbitrary `acme_user_*.json` is +adopted as an ACME identity. + +## Export + +`CommandHandler.CertsExport` → `SANCertManager.ExportStore` takes `stateMu`, the +store's disk-write lock, so a backup taken mid-renewal is never torn; with no +certificate manager the store has no writers and `ExportCertificateStore` runs +directly. `dash-proxy export certs <path>` dials the socket first and falls back +to an offline read only when **nothing answers** — an RPC error whose text +starts with `rpc: can't find method kamal-proxy.CertsExport` means a live but +outdated proxy, and that case fails with upgrade guidance rather than reading a +directory something is writing. + +`ExportCertificateStore` in order: + +1. `safeOutputPath` resolves symlinks on the destination and on the store paths, + compares resolved strings, and then compares filesystem identity + (`os.SameFile`) against each existing ancestor — an alias or a + case-insensitive spelling of `acme.state` is refused too. +2. `collectStateEntry` reads `acme.state`. It is the one file whose unparseable + presence **aborts** the export: a backup without a usable index cannot + restore. `validateManagerState` runs here. +3. `collectCertsEntries` walks `certs/`, capturing the account keys and every + complete pair. `collectCertPair` parses each pair with `tls.X509KeyPair` and + skips an invalid one with a warning rather than archiving it. + `warnMissingStateCerts` flags certificates the state names but disk lacks. +4. Certificates present with no state file is refused. Account keys — primary + and per-directory — do not count as certificates, so an account-only estate + (registered, nothing issued yet) still exports. +5. An empty file set is `ErrCertStoreEmpty`: a cron job faithfully archiving + nothing is worse than a loud failure. +6. `writeCertArchive` pins the output directory as an `os.Root`, + re-validates that handle's identity against the certificate tree + (`rejectPinnedRootInsideStore` → `dirInsidePinnedTree`, walking the tree + through its own pinned root so neither side of the comparison is a + re-resolvable pathname), creates a short fixed-pattern temp file with + `O_EXCL` and mode 0600, writes, **reads the staged archive back through + `readCertStoreArchive`**, fsyncs, renames, and fsyncs the directory. A + verification failure removes the temp file. Directory-open and + directory-sync failures fail the export, except `ENOTSUP`/`EINVAL`. +7. Warnings the staged verification produced are merged into the summary, + except `warnMissingCertificate` ones the collection pass already reported. + +## Reading an archive + +`readCertStoreArchive` (`internal/server/cert_store_archive.go`) is shared by +verify, restore and the exporter's self-check. Structural problems are errors — +a backup that fails here cannot be trusted — while an expired certificate is +not: a faithful backup of an expired certificate is still a backup. + +Bounds: `maxCertArchiveBytes` (512 MiB decompressed), `maxCertArchiveEntries` +(100 000 headers), and `maxCertArchiveHeaderBytes` (64 MiB) for the PAX/GNU +metadata `tar.Reader.Next()` consumes before the entry counter can run. The +whole decompressed gzip stream goes through `cappedReader`, whose boundary +semantics are exact: a zero-length read returns `(0, nil)` without touching the +underlying reader; at an exhausted budget it probes one byte, treating `io.EOF` +as "ended exactly at the cap" and any byte as overflow. After tar EOF the stream +is drained to its own EOF so the gzip trailer and checksum are validated. + +`placeEntry` routes each entry and refuses any name the exporter would never +write, including a certificate directory that is not already in +`sanitizeFilename` form — two spellings collapsing to one on-disk path is how a +restore overwrites the wrong certificate. + +`validateManagerState` requires: both maps present, no null record, every +identifier equal to its map key, every sanitized identifier safe as a directory +name and unique after sanitization, and every `DomainMap` key covered by its +target's `Domains` under the same wildcard-aware `identifiersCover` the issuance +path uses. The reader additionally cross-checks each state-referenced pair's +leaf DNS names (order-independent set equality) and expiry (second precision) +against the state record. + +The ACME account payload is validated against the boot-time loader's contract: +only an `*ecdsa.PrivateKey` is usable. A missing, invalid or non-ECDSA key is +**not** fatal — the account is omitted with a `warnAccountKey` warning saying +the next boot will register a fresh one. + +## Restore and verify + +Both run offline against a stopped proxy: `dash-proxy import certs --archive +<path>` (add `--force` to overwrite a non-empty store, `--verify` to report +without touching it). `RestoreCertificateStore` writes certificate pairs, then +the account keys, then `dynamic-domains.state`, and `acme.state` **last**, so an +interrupted restore never leaves an index naming files that are not there. +Every write goes through `writeFileStaged`: a unique same-directory temp file, +mode 0600, fsync, rename, and removal on every failure path. + +`removeStaleCertDirs` runs **after** the state commit, deliberately: a restore +that fails midway then leaves the old store intact rather than an old index +pointing at deleted directories. It counts a removal only when `Lstat` confirms +the directory exists, fails the restore on any inspection error other than +`os.ErrNotExist`, and syncs `certsPath` only when at least one real unlink +happened. + +Recovery is certificates **only**. The routing table is a separate file: restore +`dash-proxy.state` (or its `.bak`) while the proxy is stopped, before starting +it; without that, redeploy the TLS services after boot to rebuild routing. +`dash-proxy domains list` shows dynamic `--tls-domains-source` domains only, so +it is not evidence that static `--tls --host` services came back — verify those +with a TLS handshake and the expiry metrics. + +## Importers + +- **Traefik** (`internal/server/traefik_import.go`) — + `import certs --traefik-acme <acme.json> [--resolver <name>]`, offline. + Certificates are accepted on their validity window alone; the + registered-versus-dynamic policy stays in `SANCertManager.GetCertificate`. + `loadStateForImport` decodes existing state into a zero-value `managerState` + and refuses nil required maps, leaving an invalid state file untouched. + `cert.pem` and `key.pem` are replaced per file through temp+rename — an + interrupted pair is caught by `loadState`'s pair check and falls through to + ordinary provisioning, so no staged directory swap is needed. +- **Legacy HTTP-01 cache** (`internal/server/san_cert_import.go`) — + `importLegacyHTTP01Cache` runs from `Initialize` with **no** manager or store + lock held, because adoption takes its own. + +`import certs` creates a non-empty `--data-dir` before importing, so an empty or +wholly skipped import still writes `acme.state`. Its flag groups: one of +`--traefik-acme`/`--archive` is required, those two are mutually exclusive, +`--archive` excludes `--resolver`, and `--verify`/`--force`/`--traefik-acme` are +mutually exclusive — which is what leaves `--archive --verify` and +`--archive --force` the only valid combinations of those three. + +## Related + +- `summary.md` — issuance, batching, renewal +- `../review/cert-store-export.md`, `../review/cert-store-restore.md` diff --git a/lode/certs/summary.md b/lode/certs/summary.md new file mode 100644 index 0000000..a7320fb --- /dev/null +++ b/lode/certs/summary.md @@ -0,0 +1,184 @@ +# Certificates: issuance, batching and renewal + +The proxy has one ACME system when `--acme-email` is set: `SANCertManager` +(`internal/server/san_cert_manager.go`, 1030 lines). `internal/server/cert.go` +holds the `CertManager` interface (`GetCertificate` + `HTTPHandler`); three +types satisfy it — `SANCertManager`, `StaticCertManager`, and `autocert.Manager` +from `golang.org/x/crypto`. + +`Service.createCertManager` (`internal/server/service.go:761-823`) picks one, in +this order, and the order is the contract: + +1. clear this service's SAN directory override, whatever happens next +2. TLS disabled → no manager at all +3. both `--tls-certificate-path` and `--tls-private-key-path` → `StaticCertManager` +4. any host containing `*` → `ErrorAutomaticTLSDoesNotSupportWildcards` +5. a shared SAN manager exists **and** no `--tls-on-demand-url` → record the + per-service directory, register every non-empty host, return the shared + manager. An explicit on-demand URL is a per-service opt-in, so it wins. +6. otherwise an `autocert.Manager` over a `DirCache`, with either a host + whitelist or the on-demand policy + +`--tls-domains-source` without `--acme-email` warns at deploy and issues +nothing: step 5 is the only path that reaches dynamic issuance. + +```mermaid +flowchart TD + H[TLS handshake] --> R[Router.GetCertificate] + R -->|no service for SNI| X[ErrorUnknownServerName] + R --> S[service.certManager.GetCertificate] + S --> M{SANCertManager.GetCertificate} + M -->|valid cert, right directory| Serve[serve it] + M -->|due for replacement, still valid| Hold[serve held cert + queue issuer] + M -->|registered, expired or directory flip| P[provisionCertificate] + M -->|dynamic| Q[domainIssuer queue] + M -->|neither| X2[ErrCertNotFound] + P --> PF[preflightTrigger] --> B[batch + partition] --> O[ACME order] + O -->|ok| A[adoptCertificate] + O -->|fail| QU[attributeBatchFailure -> quarantine] +``` + +## Who may have a certificate + +Two allowlists, both consulted before any order: + +- `registeredDomains` — hosts named by `deploy --host` on a TLS service, + installed by `Service.createCertManager` → `RegisterDomain`. +- `dynamicDomains` — hosts learned from a `--tls-domains-source` poll, installed + by `SetDynamicDomains` (`internal/server/san_cert_dynamic.go`). + +`GetCertificate` (`internal/server/san_cert_manager.go:430-512`) serves a +covering certificate that is more than 24h from expiry and issued at the right +directory. Otherwise: a still-valid certificate keeps serving while a +replacement is queued asynchronously — reaching the expiry window means +proactive renewal was already failing, and a handshake that errors with a valid +certificate in hand is a self-inflicted outage. The one exception is a +**registered** domain whose certificate came from a directory its service has +moved away from: that is fresh operator intent with a presumably healthy CA, so +the handshake reprovisions synchronously. Dynamic domains stay on the +serve-stale path even then, so one `--tls-staging` flip cannot fail every tenant +handshake at once. A name on neither list is refused outright. + +## Batching a handshake order + +`provisionCertificate` (`internal/server/san_cert_manager.go:525-672`): + +1. `preflightTrigger` probes the triggering domain **before** taking any lock — + a domain that cannot answer must cost nothing. Wildcards and domains in a + zone with a DNS-01 provider are not probed, because neither needs to route + here. +2. The single-flight key is `"service:" + owner`. Batches never span services, + so a global slot would make one service's handshake wait out — and then + spuriously fail after — another service's unrelated order. +3. Candidates are the triggering domain plus pending domains of the **same + service and the same ACME directory**, skipping quarantined ones so they do + not consume batch slots, up to `MaxSANsPerCertificate` (100). +4. `filterBatchMates` probes every non-trigger mate — history is not a pass, an + expiring host whose DNS moved away must not ride in — and quarantines the + unreachable ones. The trigger itself is never dropped. +5. `planIssuanceDomains` may collapse siblings into a wildcard + (`DomainGrouper`, `--acme-prefer-wildcard`); the identifier set is sorted so + `sanCertID` is stable. +6. `splitByProviderZone` narrows the order to one DNS-provider partition — one + order never spans providers — and the deferred members go back to pending. +7. `bucket.Take` spends an ACME token. One `tokenBucket` serves every issuance + path in the process (handshake, issuer, renewer), so they cannot add up past + the limit. +8. Success: `adoptCertificate` + `clearBatchQuarantine`. Failure: + `attributeBatchFailure` quarantines the identified culprits and restores the + survivors to pending. + +`adoptCertificateAt` takes `stateMu` then `mu`, publishes the maps, writes the +certificate files and persists the state under that one hold, so an export or a +concurrent persist can never capture state naming files that do not exist yet. + +## Per-service ACME directories + +`internal/server/san_cert_directories.go` carries the `--tls-staging` story. +`SetServiceDirectory` records a service's override — `createCertManager` clears +it first, on every path, so a service that stops using the shared manager cannot +leave one behind. `directoryForDomains` prefers concrete batch domains so a +synthesized wildcard inherits its originating service's directory rather than a +coverage scan; `wildcardOwnerLocked` resolves a wildcard only through domains +the certificate actually serves. `clientsForDirectory` lazily builds a client +bundle per directory behind `directoryInitMu` (it does network I/O and so cannot +ride `mu`), with account keys named by `accountFileForDirectory`: +`acme_user.json` for the run-level directory, `acme_user_staging.json` for +Let's Encrypt staging, `acme_user_<8 hex>.json` otherwise. + +## Zones and providers + +`internal/server/san_cert_zones.go` turns `--acme-dns-provider` entries into +obtainers. `ProviderSelection.ProviderFor` picks the longest matching zone +suffix, else the default; the matched zone doubles as the partition key that +keeps one order on one provider. `orderObtainer` refuses a batch whose members +resolve to different providers. `wildcardAnchors` and `outsideWildcardZones` +keep a wildcard order inside the zone its provider can answer for. + +## Quarantine and release + +`domainQuarantine` (`internal/server/domain_quarantine.go`) keeps a per-domain +failure count and a current hold, kinded `quarantinePreflight`, `quarantineACME` +or `quarantineRateLimited`. `RecordRateLimited` holds until the CA's advertised +time plus `rateLimitHoldMargin`; a zero or already-passed time falls back to the +ladder. `Release` lifts a hold but keeps the count, so a flapping domain keeps +climbing; `Clear` (successful issuance, or the domain left its source) drops the +history. A state file written before `Kind` existed decodes as +`quarantineACME`, the conservative default. + +`releaseProber` (`internal/server/domain_release.go`) sweeps held domains on +`--acme-release-probe-interval` (default 1m, negative disables) and lifts the +hold from those that now route here. It deliberately records nothing on a +failing probe, and it excludes rate-limit holds and unprobeable domains in +`candidates` rather than leaving them to `probeDomains` — that function *skips* +them, so a sweep reading "not failed" as "passed" would release every wildcard +hold on its first tick. + +## Asynchronous issuance + +`domainIssuer` (`internal/server/domain_issuer.go`) drains a queue of dynamic +domains into orders under the shared token bucket and a concurrency cap. +`nextBatch` pops up to the service's batch size, all for one service, dropping +requests that became ineligible. `takePending`/`releasePending` let the renewer +top up an under-filled batch without racing a handshake into a duplicate order. + +## Renewal + +`certRenewer` (`internal/server/domain_renewal.go`) renews inside the ARI +suggested window when the server offers one, else at two-thirds of the +certificate's lifetime plus a per-certificate jitter — nothing assumes a 90-day +or 45-day era. `reconcile` retires certificates with no renewable domains, +renews the rest, and publishes `certificate_renewals_deferred` **only after a +completed pass**: a mid-loop `ctx.Err()` return skips the gauge rather than +publishing a partial count. + +`renew` (`internal/server/domain_renewal.go:239-358`) defers rather than shrinks +while there is time — a shrunken identifier set both unmaps live members and +forfeits the identical-set renewal exemption — and compacts only inside +`compactionWindowFor`, which is a week earlier for certificates covering a +deploy-registered host, wildcards included. It then splits by desired directory +and by provider zone, issues one order per partition, and gives the ARI +`replaces` marker to at most one of them: the first partition still at the +certificate's **recorded** directory. The marker is spent when the CA accepts +the order, not when adoption succeeds — re-sending an identifier the CA already +honoured would have the next order rejected. An account-level rate limit breaks +only that directory's partition loop and holds its unsubmitted partitions until +the advertised time; another directory is a separate bucket and still proceeds. + +## The other cert managers + +- **Static** (`StaticCertManager`, `internal/server/cert.go`) — `--tls-certificate-path` + plus `--tls-private-key-path`, loaded at deploy so a bad path fails there. +- **On-demand** (`internal/server/tls_on_demand.go`) — `--tls-on-demand-url` + builds an `autocert.Manager` whose `HostPolicy` asks an endpoint whether a + host may have a certificate. It beats the shared SAN manager for that + service (step 5 above). A path-shaped URL is answered by the service's + own handler chain through a `boundedResponseRecorder`; an absolute URL is + fetched. Any 2xx approves (204 included); a redirect does not, because + following it could turn a denial into an approval. + +## Related + +- `store-and-recovery.md` — export, verify, restore, and the Traefik/legacy importers +- `../dynamic-sources/summary.md` — where dynamic domains come from +- `../review/certs-issuance.md`, `../review/certs-directories-and-renewal.md` diff --git a/lode/cli-and-rpc/summary.md b/lode/cli-and-rpc/summary.md new file mode 100644 index 0000000..e1a9e22 --- /dev/null +++ b/lode/cli-and-rpc/summary.md @@ -0,0 +1,107 @@ +# CLI and RPC + +Two processes, one binary. `dash-proxy run` is the server; every other verb is a +client that dials a unix socket and makes one `net/rpc` call. The split is what +keeps the request path free of configuration parsing and the command path free +of request-path locks. + +## The binary + +`cmd/dash-proxy/main.go` calls `cmd.Execute` (`internal/cmd/root.go`), which +registers fourteen top-level commands: `run`, `deploy`, `remove`, `pause`, +`stop`, `resume`, `list`, `rollout`, `domains`, `cache`, `drain`, `import`, +`export`, `hold`. + +`rootCmd.Use` is still `kamal-proxy`, so `dash-proxy -h` prints +`kamal-proxy [command]`; the environment prefix in `internal/cmd/util.go` is +still `KAMAL_PROXY_`. Both are noted in the seeding discrepancies — the stage-3c +rename moved the binary, socket, user, data directory and image label, not +these. + +There is **no `version` command**. The image tag is the version: the `dash` gem +docker-inspects the running container and compares the tag with `Gem::Version`. + +## The socket and the RPC name + +`CommandHandler.Start` (`internal/server/commands.go`) registers the service as +`"kamal-proxy"` once, behind a `sync.Once`, and listens on +`Config.SocketPath()`. That path is `DASH_PROXY_SOCKET`, else +`KAMAL_PROXY_SOCKET` (the gem sets it on containers booted before the rename, +and a running proxy is reached over whatever socket it opened), else +`<runtime dir>/dash-proxy.sock`. + +The RPC name is load-bearing and does not move: **16** call sites in +`internal/cmd` dial `client.Call("kamal-proxy.<Verb>", …)`, one per verb — +`Deploy`, `Remove`, `Pause`, `Stop`, `Resume`, `List`, `Drain`, +`RolloutDeploy`, `RolloutSet`, `RolloutStop`, `DomainsStatus`, `DomainsRefresh`, +`DomainsRetry`, `CertsExport`, `CachePurge`, `CacheStats`. It is internal, +spoken over a unix socket inside one container, and renaming it buys nothing +while breaking every deployed gem mid-upgrade. + +Each connection is served on its own goroutine; `net.ErrClosed` on `Accept` is +the shutdown signal, and any other accept error is logged and retried. + +## Wire types are append-only + +`DeployArgs` carries everything a deploy configures: the service name, target +and reader URLs, `DeploymentOptions`, `ServiceOptions` and `TargetOptions`. +`ServiceDescription` (`internal/server/router.go`) is the listing shape. + +Both are gob on the wire, and the CLI and the server can briefly be different +versions during a proxy replacement — gob tolerates an added field but not a +changed one. Fields are therefore added, never renamed or retyped. + +The status types carry their own documentation of the domain model: +`DomainsServiceStatus` (source, domains, `fetched_at`, `held_removals`), +`QuarantineStatus` (`until`, `failures`, and a `kind` of `preflight`, `acme` or +`rate_limited`), `RegisteredDomainStatus` (service, certified, `expires_at`). + +## Where configuration is parsed + +Twice, deliberately. `runCommand.preRun` rejects a bad `--min-tls`, +`--log-format`, `--trace-context`, `--acme-dns-provider` or `--cache-store` +before anything registers an ACME account. The same values are parsed again +where they are used (`Server.startHTTPServers`, `Server.startMetricsServer`), +because a `Config` built directly — by a test, or a future caller — must not +start a listener that silently ignores a setting. + +Defaulting lives on the **server** side for the same reason: +`TargetOptions.poolSettings` resolves every zero to the proxy's default rather +than relying on cobra flag defaults, because restored state, rollouts and older +RPC clients all bypass the CLI entirely. + +Per-service validation is `ServiceOptions.Validate` +(`internal/server/service.go:235`) and `TargetOptions.Validate` +(`internal/server/target_pool.go:105`), run once on the deploy so anything a +deploy could get wrong fails *there* rather than on a request. The individual +`validate*` helpers they call live in +`internal/server/service_options_validation.go`. + +## Environment variables + +`internal/cmd/util.go` reads `KAMAL_PROXY_<KEY>` first and bare `<KEY>` second, +through `getEnvString` / `getEnvInt` / `getEnvDuration` / `getEnvBool`. An +unparseable value falls back to the default rather than failing — the flag is +the authoritative surface and the env var is the convenience. + +`ensureDataDir` creates `--data-dir` when one was supplied, shared by the two +commands that write into it (`run`, `import certs`). + +## Offline commands + +`import certs` and `export certs --verify` run against a **stopped** proxy and +never dial the socket. `export certs` dials first and falls back to an offline +read only when nothing answers — see `../review/cert-store-export.md` for why +an outdated-but-live proxy is a failure rather than a fallback. + +`hold` is a hidden command that blocks on a signal, so a container can own a +shared network namespace without running a proxy in it. + +`drain` and `SIGTERM` converge on the same path: close the public listeners, +finish in-flight requests up to a timeout, save state, exit. + +## Related + +- `../request-path/summary.md` — what `DeployArgs` becomes +- `../certs/store-and-recovery.md` — the `import`/`export` verbs in detail +- `../review/cert-store-export.md`, `../review/cert-store-restore.md` diff --git a/lode/dynamic-sources/summary.md b/lode/dynamic-sources/summary.md new file mode 100644 index 0000000..0aade23 --- /dev/null +++ b/lode/dynamic-sources/summary.md @@ -0,0 +1,150 @@ +# Dynamic sources: domains and redirects + +Two subsystems that poll an application endpoint and turn the answer into proxy +behaviour: `--tls-domains-source` feeds tenant hostnames into certificate +issuance, `--redirects-source` feeds a host-scoped redirect map into the request +path. They are deliberately separate managers with different responsibilities +and different persisted schemas; what they share is `sourcePoller` and +`refreshNudge`. + +## sourcePoller + +`internal/server/source_poller.go`. One poller per service per subsystem. It +owns the schedule, the conditional-request state and the transport; the payload +semantics live entirely in the `OnBody` callback. + +- **Endpoint** — a `Source` starting with `/` is path mode, resolved at poll + time against one healthy target of the service (`endpointFor`), with the + service's first configured host sent as `Host`. A path-mode poller with no + resolver fails the poll rather than panicking in the poll goroutine. Anything + else is used as an absolute URL. +- **Transport** — path mode gets `&http.Transport{Proxy: nil}`: an `HTTP_PROXY` + variable must neither intercept a request to one specific container nor see + the bearer token on that plain-HTTP leg. Absolute URLs keep the default + transport, where proxy variables are intentional. +- **Schedule** — an immediate poll on `Start`, then `jitteredInterval` (±10%) so + a fleet does not thundering-herd the app. `Refresh` collapses onto a + single-slot channel. +- **ETag** — `If-None-Match` from the last response. The new ETag is stored + **before** `OnBody` so the parser's consumers can read it, and rolled back + when `OnBody` returns an error, so a broken payload keeps being fetched + instead of 304ing until the content next changes. +- **Failure reporting** — `OnPollError` fires for an unresolvable endpoint, a + transport error, an unexpected status, and an unreadable gzip body. A 304 is a + healthy poll and is not counted. + +## refreshNudge + +`internal/server/refresh_nudge.go`, served by both subsystems so an ordering or +status fix cannot drift between them. `POST /.kamal-proxy/domains/refresh` and +`POST /.kamal-proxy/redirects/refresh`, in this order: + +1. **Hidden first, whatever the method** — no token configured, or no sources — + is a 404 for every method, because a 405 would reveal the endpoint. +2. Non-POST → 405 with `Allow: POST`. +3. Bearer token compared with `tokensEqual` (constant time, over SHA-256 + digests so length does not leak) against `KAMAL_PROXY_REFRESH_TOKEN`. +4. One nudge per `refreshMinInterval` (10s) → 429 with `Retry-After`. +5. 202 with an empty body. The count of sources nudged is logged, not + returned — the caller has nothing to act on either way. + +The nudge carries no data: the poll stays the single source of truth, replays +are harmless, and it works from any host. + +## Dynamic domains + +`DynamicDomainManager` (`internal/server/dynamic_domains.go`) owns the pollers, +`domainIssuer`, `domainQuarantine`, `certRenewer` and `releaseProber`, plus +`dynamic-domains.state`. + +Payload: `{"domains": ["tenant.example.com", …]}`, capped by `parseDomainList` +at `maxDomainListBody` (1 MB) and `maxDomainListEntries` (10 000) — either +exceeded rejects the whole payload. Entries are lowercased, trimmed and +de-dotted, then de-duplicated; wildcards are skipped (they need DNS-01 and have +no name to answer on) and anything failing the RFC 1123 grammar in +`validDynamicDomain` is skipped with a warning. A malformed payload is an +error, so the last good set keeps serving. + +`applyDomains` (`internal/server/dynamic_domains.go:429-507`) is the whole +reconcile: + +- A poll for a service the router no longer knows, or whose poller entry is + gone, is dropped. +- **Shrink guard** — removals above `shrinkGuardThreshold` (30% of the applied + set) are held until `shrinkGuardConfirmations` (3) consecutive over-threshold + polls. While held, the applied set is the previous set plus whatever the poll + added, and the poller's ETag is cleared (`SeedETag("")`) — otherwise an + unchanged source answers 304 forever and the confirmation count can never + advance. So a single or transient empty or truncated response evicts nothing; + three consecutive confirming polls do evict. +- Issuance follows the **polled** list, not the held union: a name the source + stopped reporting keeps its allowlist entry and any live certificate but earns + no new orders while its removal is in question. +- `ServiceDeployed` clears the service's hold, so confirmations counted against + a replaced source never carry over. + +`Retry` (the CLI's `domains retry`) is the operator's blunt escape hatch: unlike +the release prober it also clears rate-limit holds. + +Boot order matters: `loadState` serves the last-known domain sets before the app +is up, `ServiceDeployed` serves the persisted set immediately and lets the first +poll reconcile. + +## Dynamic redirects + +`DynamicRedirectManager` (`internal/server/dynamic_redirects.go`) and the map +itself (`internal/server/redirect_map.go`). Unlike the domain manager this does +not need ACME — redirects are useful on a plain HTTP proxy — so `run.go` always +builds it. + +Payload: `{"hosts": {"<host>": {redirect_to, status, preserve_path, +trailing_slash, paths:[{from,to,status}]}}}`, capped by `parseRedirectPayload` at +`maxRedirectListBody` (10 MB), `maxRedirectHosts` (100 000) and +`maxRedirectPathRules` (100 000). A document with **no `hosts` key at all** is an error, so a +half-deployed app answering with the wrong document cannot wipe live redirects; +an explicit `{"hosts": {}}` is the sanctioned way to clear. A parseable payload +whose every entry fails validation is also refused. + +`compileRedirectMap` walks host keys in sorted order and skips normalized +duplicates with a warning, so a collision resolves identically on every proxy in +a fleet, and counts rules only from the retained entry. One tenant's broken +regex is skipped rather than failing the payload. Patterns are compiled once and +the compiled rule retained. `normalizeRedirectHost` is applied both at compile +time and at lookup, so `Old.Example.COM.` meets its entry either way. + +`applyPayload` checks that the poller delivering the body is still the +service's registered poller before installing anything, so an in-flight poll +from a previous deployment cannot apply stale redirects. The compiled map is +swapped into the service through an `atomic.Pointer`, so matching on the request +path takes no lock. `Service.initialize` stores nil whenever `RedirectsSource` +is empty, so removing the source cannot leave stale redirects serving until the +manager catches up. + +`Router.DeployService` calls `dynamicRedirectManager.ServiceDeployed` +immediately after `installLoadBalancer` and **before** draining the old +targets — new traffic must see the new source's redirects as soon as the load +balancer is installed, not after a full drain timeout. The domain manager is +reconciled after the drain; that is a separate subsystem decision. + +`PublishMetrics` exists because maps restored from state are installed before +`metrics.Enable()` runs; `run.go` calls it once the server is up, or a proxy +serving only persisted redirects would report no map at all. + +## Persistence + +`dynamic-domains.state` (domain sets, ETags, fetch times, quarantine records) +and `dynamic-redirects.state` (the raw host map as the app sent it, its ETag, +fetch time and source identity) are separate schemas with separate load/save +code, and they do not even share a staging helper: the redirect state goes +through `writeFileAtomic`, the domain state through its own `os.WriteFile` to +`<path>.tmp` plus `os.Rename`. Both save paths hold a dedicated save +lock across snapshot, marshal and rename, because concurrent polls share one +temp file path. Runtime-only data (the `Host` header for path-mode polls) is +cached in memory; source identity lives in persisted state so a changed source +resets its ETag before the first new poll while the persisted map keeps serving. + +## Related + +- `../certs/summary.md` — what the domain sets feed +- `../request-path/summary.md` — where the redirect map is consulted +- `../review/dynamic-domains.md`, `../review/dynamic-redirects.md` diff --git a/lode/lode-map.md b/lode/lode-map.md new file mode 100644 index 0000000..8cfd39c --- /dev/null +++ b/lode/lode-map.md @@ -0,0 +1,42 @@ +# Lode map + +The index of this repository's durable memory. Read this first; it beats a +directory listing. Every file describes dash-proxy as it is now, with the +reasoning behind it — never what changed. + +- `summary.md` — what the proxy is, the two paths, the three invariants +- `terminology.md` — the words this repo uses (slot, batch-mate, pre-flight probe, quarantine, shrink guard, variant index, refusal, idle controller…) +- `practices.md` — patterns `../.claude/rules/` does not state: additive state schemas, staged writes and stated durability, double-parsed configuration, fail-open-or-closed per subsystem, seams instead of live dependencies, lock ordering, the generated provider table +- `workflow.md` — the profile the shared `/lode:` workflow skills read: commands, branches, layers, shapes, constraints, docs, CI, flake sources, conflicts, verification +- `plans/README.md` — plans live in `../docs/plans/`; scratch in `tmp/` (git-ignored) + +## Subsystems + +- `request-path/summary.md` — listeners, the ten-handler root chain, `Router`, `Service`'s per-request order, `LoadBalancer` and `Target`; the only I/O is the cache store, a certificate lookup and the proxied request +- `certs/summary.md` — `SANCertManager`: the two allowlists, handshake batching, provider zones, per-service ACME directories, quarantine and release, asynchronous issuance, renewal, and the three other cert managers +- `certs/store-and-recovery.md` — the archive layout, export, the shared archive reader, restore and verify, the Traefik and legacy importers +- `dynamic-sources/summary.md` — `sourcePoller` and `refreshNudge`, then the domain manager (shrink guard, issuance) and the redirect manager (compiled map, poller identity), and their two separate persisted schemas +- `cache/summary.md` — the three stores and the optional leaser, two-level variant keys, the single flight, and the 18 refusal reasons +- `resilience/summary.md` — health-check regimes, retries, the one `forwardedResolver` behind allow/deny/rate-limit, basic auth, pause and rollout, path timeouts, scale-to-zero +- `cli-and-rpc/summary.md` — the fourteen commands, the unix socket, the `kamal-proxy` RPC name and its 16 call sites, append-only wire types, where configuration is parsed +- `observability/summary.md` — the sixteen metrics and the atomic tracker swap, log formats, trace-context modes +- `testing-and-ci/summary.md` — 1080 tests across 115 files, the shared helpers, what a test never does, the three workflows, and the release script + +## Review rules (`review/`) + +Accepted review findings rewritten as rules about the system, each verified +against the current code and carrying the test that proves it. `/lode:gate` +reads every file here before reviewing a diff; `/lode:learn` adds to them. + +- `review/certs-issuance.md` — service-scoped single flight, trigger and batch-mate probing, quarantine slots and clearing, failure attribution, provider partitions, waiter re-checks; one *Not a bug* (a stale `Retry-After` still ends the hold) +- `review/certs-directories-and-renewal.md` — directory-matched coverage, the registered/dynamic split on a mismatch, wildcard ownership, per-directory account keys, renewal partitioning, the ARI `replaces` boundary, compaction windows, completed-pass gauges +- `review/cert-store-export.md` — the live-proxy fallback rule, resolved-path plus identity validation, pinned roots, fail-closed containment, staged and self-verified archives, what counts as a certificate +- `review/cert-store-restore.md` — archive bounds and capped-reader semantics, entry and state validation, the ECDSA account-key contract, write ordering, stale-directory removal, import flag modes; one *Not a bug* (per-file Traefik replacement) +- `review/dynamic-domains.md` — the shrink guard and its ETag clearing, issuance following the polled list, hold resets, release-prober candidate selection, quarantine kinds +- `review/dynamic-redirects.md` — the `hosts`-key requirement, cleared state, poller identity, deterministic normalization, exempt paths, encoded paths, compile-once; two declined suggestions +- `review/sources-and-refresh.md` — path-versus-URL transports, ETag ordering, poll-failure counting, the five-step nudge, constant-time tokens; one *Not a bug* (Warn-level rejection logging) +- `review/acme-providers.md` — the registry as the single provider list, `ParseProviderName`, the generated README table and its drift test, Cloudflare's credential sets, zone matching; one *Not a bug* (mixed-namespace credentials) + +## Not memory + +- `tmp/` — git-ignored: gate reports, handovers, scratch diff --git a/lode/observability/summary.md b/lode/observability/summary.md new file mode 100644 index 0000000..1e5148d --- /dev/null +++ b/lode/observability/summary.md @@ -0,0 +1,87 @@ +# Observability: metrics, logs, traces + +Three surfaces, all opt-in-ish and all built so a monitoring concern can never +fail a request. + +## Metrics + +`internal/metrics/metrics.go`. Sixteen collectors in four families: + +| Family | Metrics | +|---|---| +| Requests | `http_requests_total`, `http_request_duration_seconds`, `http_in_flight_requests` | +| Certificates | `certificates_total`, `certificate_expiry_timestamp_seconds`, `certificate_renewals_total`, `certificate_renewals_deferred` | +| Cache | `cache_events_total`, `cache_refusals_total`, `cache_evictions_total`, `cache_leases_total`, `cache_lease_waits_total` | +| Dynamic redirects, denials | `dynamic_redirects_total`, `dynamic_redirect_map_size`, `dynamic_redirect_polls_total`, `denials_total` | + +`EventTracker` is the interface every emission goes through; `Tracker` is a +process-wide `delegatingTracker` holding its delegate behind an +`atomic.Pointer`. Installation must not race with request goroutines emitting +through it — and in tests, background work outlives the test that started it, +which is what the atomic swap is really for. + +`Enable()` registers the Prometheus collectors exactly once per process +(`enableOnce`): they live in the default registry, which panics on +re-registration, so a second metrics-enabled server — or `go test -count=2` — +must reuse them. Later calls also leave the *active* tracker alone, so a test +fake installed in between keeps receiving events. `SetTracker` returns the +tracker it replaced, so a test can restore it. + +Until `Enable` runs, `Tracker` is a no-op. That is why +`DynamicRedirectManager.PublishMetrics` exists: maps restored from state are +installed during boot, before the real tracker, so `run.go` republishes their +gauges once the server is up. + +Metrics calls are made **after** the relevant lock is released — +`memoryCacheStore.store` returns its evictions for the caller to report — so +the Prometheus registry never sits behind a request-path mutex. + +`--metrics-port` (0 disables) and `--metrics-allow-ip`. `startMetricsServer` +parses the allow list **before** the disabled check, so a typo fails the boot +rather than waiting until someone turns metrics on. The metrics listener is the +first thing `Server.Start` opens. + +Per-service, `--exclude-metrics-path` drops a path from request metrics; the +match is on `RoutedTargetPath(r)`, the same value every other downstream check +uses, so a path prefix being stripped does not change what an operator has to +write. + +## Logging + +One `slog` handler for the whole process: the access log in +`LoggingMiddleware` and the server's own messages both go through +`slog.Default()`. + +`--log-format` is `json` (`DefaultLogFormat`, the shape kamal-proxy has always +written, so the default changes nothing) or `text`. `logfmt` is accepted as a +synonym for `text` — slog's text handler writes logfmt, which is what an +operator asking for it wants, and accepting the name saves them guessing. An +empty or blank value means the default, so an unset `LOG_FORMAT` boots rather +than failing. + +`--log-request-header` and `--log-response-header` add named headers to the +access log line. + +`WithLoggingMiddleware` sits at position 7 of the root chain, which puts it +*below* the ping handler — a liveness probe writes no access-log line at all, +the deliberate cost of an endpoint a monitor hits forever. + +## Trace context + +`internal/server/trace_context.go`. `--trace-context` is one of three modes: + +- `off` — the header is ignored entirely +- `propagate` (`DefaultTraceContextMode`) — the W3C `traceparent` is left + untouched on the wire, but its trace and span ids are recorded on the logging + request context +- `generate` — a `traceparent` is minted when the client sent none + +`ParseTraceContextMode` treats an empty value as the default. The middleware +sits *inside* `WithLoggingMiddleware` (position 8), because the trace is +recorded on the request context that the logging middleware creates. + +## Related + +- `../request-path/summary.md` — the middleware chain positions +- `../cache/summary.md` — what each cache metric counts +- `../resilience/summary.md` — what `denials_total` counts diff --git a/lode/plans/README.md b/lode/plans/README.md new file mode 100644 index 0000000..70e7eda --- /dev/null +++ b/lode/plans/README.md @@ -0,0 +1,15 @@ +# Plans + +Implementation plans for this repository live in `../../docs/plans/`, one +markdown file per plan, named `YYYY-MM-DD-<slug>.md` +(`docs/plans/2026-07-29-scale-to-zero.md` is the existing one and the shape to +follow). Larger pieces of work may instead be tracked as GitHub issues on +zoolutions/dash-proxy; `.github/ISSUE_TEMPLATE/preapproved.md` is the template +for work that already has a decision behind it. + +`/lode:plan --file` writes here. Do not move the existing plan, and do not put +plans in `lode/` — the lode describes the system as it is, `docs/plans/` +describes work that has not happened yet. + +Session handovers, gate reports and other scratch go in `../tmp/`, which is +git-ignored. diff --git a/lode/practices.md b/lode/practices.md new file mode 100644 index 0000000..3a9d280 --- /dev/null +++ b/lode/practices.md @@ -0,0 +1,142 @@ +# Practices + +Patterns this codebase holds to that `../.claude/rules/` does not already state. +Style, file size, error wrapping, mutex discipline, TDD and the git workflow +live in `../.claude/rules/coding-style.md`, `../.claude/rules/testing.md`, +`../.claude/rules/git-workflow.md` and `../.claude/rules/performance.md`. + +## State files evolve by addition only + +An option added to `ServiceOptions` or `TargetOptions` carries `omitempty` or +`omitzero` and a zero value that means "the feature is off", because the state +file outlives the binary that wrote it and a restored service must behave the +way it did before the option existed. The rule governs additions, not the whole +struct: 26 of `ServiceOptions`' 42 JSON fields and 10 of `TargetOptions`' 21 +carry the tag, and the bare ones are the original kamal-proxy fields (`hosts`, +`tls_enabled`, `response_timeout` …) that every state file has always had. Copy +the newer neighbours, not the older ones. `Service.UnmarshalJSON` still maps four +pre-plural legacy keys (`active_target`, `rollout_target`, `hosts`, +`path_prefixes`) onto their current fields. `IdleState` is persisted as a *name* +rather than an enum ordinal, and `ParseIdleState` folds anything unrecognised — +including the empty string every pre-scale-to-zero file carries — to active. + +`ServiceDescription` (`internal/server/router.go`) carries the same rule for the +wire: the CLI and the server can briefly be different versions during a proxy +replacement, and gob tolerates an added field but not a changed one, so fields +there are append-only. + +`Router.decodeStateServices` accepts both the bare JSON array the writer emits +and a `{"version":…,"services":[…]}` envelope, so the writer can switch once +every deployed generation reads both. + +## Every durable write is staged, and the durability level is stated + +There are four shapes, and which one a file gets says how much its durability is +worth: + +- `writeFileAtomic` (`internal/server/util.go`) — fixed `<path>.tmp`, explicit + `Chmod` because `O_CREATE` applies `perm` only to a file it creates and umask + masks it even then, `fsync`, rename. Used for the routing table and its + `.bak` (`internal/server/router.go`) and `dynamic-redirects.state` + (`internal/server/dynamic_redirect_state.go`). +- Hand-rolled `os.WriteFile` to `<path>.tmp` then `os.Rename`, with no fsync: + `dynamic-domains.state` (`internal/server/dynamic_domain_state.go`) and + `acme.state` (`writeManagerStateFile`). Torn reads are prevented; power loss + is not, which is acceptable because both are rebuilt from a poll or a + handshake. +- `writeFileStaged` (`internal/server/cert_store_restore.go`) — unique + same-directory temp, mode 0600, fsync, rename, removed on every failure path. + The restore path only, where a planted symlink or an inherited permission on a + private key is the threat. +- Plain `os.WriteFile` at 0600 for the ACME account key + (`SANCertManager`, saving `acme_user*.json`): it is written once at + registration, and a torn one is re-registered on the next boot. + +Certificate pairs go through `writeCertificateFiles`, the same staged path the +offline importers use, so a crash or a concurrent reader never sees a torn pair; +callers hold `stateMu`. +- Directory fsync policy lives in exactly one place, `syncOpenDir`, which + excuses `ENOTSUP`/`EINVAL` and nothing else; `syncDir` (pathname) and + `syncRootDir` (pinned `os.Root`) delegate to it. +- `Router.RestoreLastSavedState` deletes a leftover `.tmp` before reading: under + the atomic protocol a surviving temp file is by definition an aborted write. + +## Configuration is parsed twice, deliberately + +`runCommand.preRun` rejects a bad `--min-tls`, `--log-format`, +`--trace-context`, `--acme-dns-provider` or `--cache-store` before anything +registers an ACME account. The same values are parsed again where they are used +(`Server.startHTTPServers`, `Server.startMetricsServer`), because a `Config` +built directly — by a test, or by a future caller — must not start a listener +that silently ignores a setting. The duplication is the point; do not remove +either half. + +The same reasoning puts defaulting on the server side: `TargetOptions.poolSettings` +resolves every zero to the proxy's default rather than relying on cobra flag +defaults, because restored state, rollouts and older RPC clients all bypass the +CLI. + +## Fail open or fail closed, decided per subsystem and written down + +- A cache store that is unreachable turns lookups into misses and writes into + logged errors, never a failed request (`CacheStore`'s contract). A lease in + any doubt **grants**: a duplicate fetch is cheaper than an origin outage. +- `ipAllowList` refuses the zero `netip.Addr`; `denyList` matches nothing for + it. Each fails in the direction it exists for. +- A certificate export aborts when `acme.state` exists but does not parse, and + when a containment check cannot read part of the certificate tree — a backup + that cannot restore must not be reported as taken. +- An unattributable ACME order failure quarantines the whole batch, so retries + back off rather than looping into a rate limit. + +Anywhere a new failure path is added, say which direction is the harmless one in +the comment, the way these do. + +## Network probes are bounded by count and by client timeout, not by context + +`probeDomains` runs at most `maxConcurrentProbes` (16) pre-flight probes at once +and each probe is bounded by `preflightTimeout` (5s) on the shared client. The +probes deliberately do **not** honour the caller's context: a probe that +completes benefits the next handshake even when this one has given up. This is a +reviewed decision, not an oversight — see `review/certs-issuance.md`. + +## Seams instead of live dependencies + +Anything that would otherwise need a live external service is an interface the +production path fills in: `certObtainer` (ACME), `ContainerLifecycle` (Docker), +`CacheStore`/`CacheLeaser` (Redis), `serviceResolver` (the router), +`metrics.EventTracker` (Prometheus), `renewalInfoGetter` / `directoryObtainer` +(optional ACME capabilities discovered with a type assertion). Adding a +capability to an obtainer means adding a small optional interface and asserting +for it, not widening `certObtainer`. + +## One table, generated docs, and a drift test + +The DNS provider registry (`internal/server/acme/providers/registry.go`) is the +only list of providers: `GetProviderInfo`, `NewProvider`, `detectionOrder`, the +`--acme-dns-provider` help string and the README table all derive from it. +`go generate ./internal/server/acme/providers` rewrites the README between the +`BEGIN/END GENERATED: dns-provider-table` markers, and +`TestREADMEProviderTable_MatchesRegistry` fails the build when the committed +table drifts. A second hand-maintained provider list is the thing this design +exists to prevent. + +## Locks are named for what they serialize, and the order is written down + +`SANCertManager` has `mu` (in-memory maps) and `stateMu` (every disk write to +the certificate store); `adoptCertificateAt` takes `stateMu` then `mu` so a +state-file snapshot can never name a certificate whose files are not written +yet. `Router` has `serviceLock` and `saveLock`, and `Service.MarshalJSON` +documents the router-then-service order that keeps it from inverting. Metrics +calls are made after the relevant lock is released (`memoryCacheStore.store` +returns evictions for the caller to report), so the Prometheus registry never +sits on a request-path mutex. + +## Internal endpoints live under `/.kamal-proxy/` and are exempt from redirects + +`redirectExemptPrefixes` (`internal/server/redirect_map.go`) keeps ACME +challenges and the proxy's own ping, pre-flight and refresh endpoints reachable +on every host; `isRedirectExemptPath` is checked before both the dynamic map and +the static `--redirect` rules. The TLS/canonical hop still applies to them. The +prefix is part of the deployed contract with the gem — renaming it is a breaking +change, and it kept the `kamal-proxy` spelling through the 3c rename. diff --git a/lode/request-path/summary.md b/lode/request-path/summary.md new file mode 100644 index 0000000..bc6ac1e --- /dev/null +++ b/lode/request-path/summary.md @@ -0,0 +1,156 @@ +# Request path + +Everything between a client connection and the backend. Nothing here reads the +registry or the ACME directory; the only I/O on this path is the cache store, +the certificate lookup for a handshake, and the proxied request itself. + +## Listeners + +`Server.Start` (`internal/server/server.go`) opens, in order, the metrics +listener, the HTTP and HTTPS listeners (plus HTTP/3 when `--http3`), and the RPC +command socket. `--reuse-port` puts `SO_REUSEPORT` on every listener so an +overlapping proxy generation can hold the same ports during a handoff; +`--proxy-protocol` wraps the two TCP listeners *before* TLS layering, because +the PROXY preamble arrives ahead of the ClientHello. + +Both TLS configs take `GetCertificate: s.router.GetCertificate`. +`clientCertificateConfig` is installed as `GetConfigForClient`: for a host whose +service was deployed with `--tls-client-ca-path` it returns a **clone** of the +listener's config with `ClientAuth`/`ClientCAs` set — a fresh config would drop +ALPN and the minimum version, downgrading mTLS hosts to HTTP/1.1 and breaking +`tls-alpn-01`. HTTP/3 is pinned to TLS 1.3 because QUIC is defined only over it, +so `--min-tls` neither lowers nor needs to raise that listener. + +## The root middleware chain + +`Server.buildHandler` (`internal/server/server.go:390-423`) composes handlers +inside-out; the outermost runs first: + +| Order | Handler | Why it is where it is | +|---|---|---| +| 1 | `WithPingMiddleware` | outermost and unconditional, which is what keeps `/.kamal-proxy/ping` out of the access log | +| 2 | `DynamicRedirectManager.WrapHandler` | `/.kamal-proxy/redirects/refresh`, only when a redirect manager exists | +| 3 | `DynamicDomainManager.WrapHandler` | `/.kamal-proxy/domains/refresh` and `/.kamal-proxy/preflight/<nonce>`, only when ACME is on | +| 4 | `SANCertManager.HTTPHandler` | HTTP-01 challenges, ahead of ordinary routing | +| 5 | `WithRequestStartMiddleware` | stamps the start time | +| 6 | `WithRequestIDMiddleware` | generates `X-Request-ID` | +| 7 | `WithLoggingMiddleware` | creates the logging request context everything below writes into | +| 8 | `WithTraceContextMiddleware` | inside logging, because the trace is recorded on that context | +| 9 | `WithErrorPageMiddleware(pages.DefaultErrorPages, root=true, …)` | the built-in 404/413/502/503/504 pages | +| 10 | `Router` | | + +A liveness probe therefore carries no request ID and never renders an error +page — the deliberate cost of an endpoint a monitor hits forever +(`internal/server/ping_handler.go`). There is no readiness variant: the +listeners only exist between `Start` and `BeginDrain`. + +## Router + +`Router` (`internal/server/router.go`) owns a `ServiceMap` behind +`serviceLock` (RWMutex) and a separate `saveLock` for state writes. +`ServeHTTP` resolves `(service, matchedPrefix)` through +`ServiceMap.ServiceForRequest`, answers 404 when nothing matches, and attaches a +`routingContext` carrying the matched prefix when the service strips prefixes. +`RoutedTargetPath(r)` is what every downstream check — health-check detection, +path timeouts, metrics exclusion — matches on. + +`Router.GetCertificate` (`internal/server/router.go:565-603`) is the allowlist +gate for the whole handshake path. An empty SNI falls back to +`ServiceMap.DefaultTLSHostname`; a name that routes to no service, or to a +service with no `certManager`, is refused with `ErrorUnknownServerName`. Before +that refusal existed, any DNS record pointed at the proxy could drive a real +Let's Encrypt order. + +State: `RestoreLastSavedState` removes a stale `.tmp`, reads +`dash-proxy.state`, falls back to `dash-proxy.state.bak` on a decode failure +(repairing the primary from it), and accepts both the bare-array and the +versioned-envelope encodings. `saveStateSnapshot` marshals under the read lock +and writes under `saveLock` via `writeFileAtomic`. + +## Service + +`Service` (`internal/server/service.go`) is where per-service behaviour lives. +`initialize` — called by `NewService`, `UpdateOptions` and `UnmarshalJSON` — +builds the cert manager, loads the client CA bundle, composes the middleware +chain, compiles the redirect and rewrite rules, builds the cache handler and +configures the idle controller. Anything a deploy could get wrong is resolved +**here**, not on a request: a bad CA path fails the deploy rather than silently +serving without client verification. + +`Service.createMiddleware` (`internal/server/service.go:845-893`) wraps +`serviceRequestWithTarget` inside-out: request deadline (innermost), error +interception, custom error pages, compression, the cert manager's ACME handler, +then `--client-ip-header`. The ACME handler sits above compression because a +challenge is a handful of bytes served to a CA. + +`Service.serviceRequestWithTarget` (`internal/server/service.go:895-943`) is the +per-request order, and each step's position is load-bearing: + +1. deny rules (before the allow list — an address on both is denied) +2. IP allow list +3. plaintext-only service refusing a TLS request → 503 +4. redirects (TLS, canonical host, dynamic map, static rules) +5. rate limit — after the redirect so a 301 spends no token, before the auth + challenge so a password flood is limited +6. basic auth — after the redirect so credentials are never solicited over + plaintext, before the pause check so protection does not lapse while paused +7. paused/stopped handling (health checks still answer 200 so downstreams do + not drop the proxy) +8. rewrite rules — last, so everything above saw the client's own path +9. the cache handler, which is the load balancer directly when no cache is on + +Because the cache sits below every check, a stored response can only ever reach +a client the target would have been asked on behalf of. + +`Service.redirectURLIfNeeded` (`internal/server/service.go:992-1043`) composes +the TLS and canonical-host hop with the rule sets: exempt paths +(`/.well-known/acme-challenge/`, `/.kamal-proxy/`) skip both rule sources but +still get the TLS/canonical hop, the dynamic per-host map answers before the +static service-wide rules, and a rule naming a path is completed with the scheme +and host the request was already headed for — which is also what keeps a +captured `//evil.example.com` a path on this host. + +## LoadBalancer and Target + +`LoadBalancer` (`internal/server/load_balancer.go`) holds every target plus the +healthy writers and readers, rebuilt by `updateHealthyTargets` on each state +change. `StartRequest` returns a closure so target selection and the pause wait +happen after the service lock is released; with a retry policy it defers +selection entirely into `serveWithRetries`. + +Read/write split: a GET or HEAD is a read request unless it is a WebSocket +upgrade and `--read-target-websockets` is off. A read is served by a reader +unless the client holds an unexpired `kamal-writer` cookie, which +`loadBalancerResponseWriter` sets on every write response unless the target +answered `X-Writer-Affinity: false`. Weighted pools go through +`nextWeighted`; unweighted ones use the plain rotating index, so a deployment +that sets no weights behaves exactly as it did before weights existed. + +A single-target pool stops health-checking once its target is healthy — taking +it out of the pool would not help — **except** when `persistentHealthChecks` is +set by `RecheckHealth`, which is how `--recheck-targets-on-restore` keeps a +restored target under observation. + +`Target` (`internal/server/target.go`) owns one `httputil.ReverseProxy` per +response timeout in play (`pathProxyHandlers` plus the default), each with its +own transport built by `newProxyTransport` — `Proxy: nil`, HTTP/1 only, because +targets are always plain `http` (`parseTargetURL` supplies the scheme and +`hostRegex` rejects any other). `rewrite` sets the forwarded headers, preserves +the raw query verbatim (Go's default drops unparseable params), rewrites the +path through `RoutedTargetPath`, and applies the request header rules **last** +so a rule can override the proxy's own `X-Forwarded-*`. + +`handleProxyError` maps a failure to a status in a fixed order: a retryable +error is recorded on the attempt and rendered by nobody, then +`MaxBytesError` → 413, a `net.Error` timeout → 504, the request's own deadline +→ 504, client cancellation → 499, draining → 504, a chunked-encoding error → +400, everything else → 502. + +`Drain` cancels hijacked requests immediately (they are long-lived), waits out +the rest until the deadline, then cancels what is left. + +## Related + +- `../resilience/summary.md` — health checks, retries, limits, auth, scale-to-zero +- `../cache/summary.md` — what sits between the checks and the load balancer +- `../certs/summary.md` — what answers `Router.GetCertificate` diff --git a/lode/resilience/summary.md b/lode/resilience/summary.md new file mode 100644 index 0000000..535df92 --- /dev/null +++ b/lode/resilience/summary.md @@ -0,0 +1,159 @@ +# Resilience: health, retries, limits, access control, scale-to-zero + +The per-service machinery that decides whether a request is served at all, and +which target serves it. Everything here runs inside +`Service.serviceRequestWithTarget` or below the load balancer; the order of the +checks is in `../request-path/summary.md` and every position is load-bearing. + +## Health checks + +`HealthCheck` (`internal/server/health_check.go`) probes one target on +`--health-check-path` and reports to a `HealthCheckConsumer`. Two regimes: + +- **Pre-healthy** — nothing is routed to the target and a probe is cheap, so the + delay doubles from `initialHealthCheckDelay` (50ms) but never past + `maxPreHealthyDelay` (2s). Without the cap a 20s configured interval let the + gap grow to 12.75s and then 25.55s, so a target ready at 13s was not noticed + until 25.55s. +- **Healthy** — the configured interval governs. + +`preHealthyFastWindow` (60s) bounds how long the ceiling applies. A normal +deploy disposes a target that misses its deploy timeout, but `deploy --force` +skips that wait, and a target that never comes up must not be probed at boot +cadence forever; past the window the backoff resumes doubling toward the +configured interval. + +A single-target pool stops health-checking once its target is healthy — taking +it out of the pool would not help — **except** under `persistentHealthChecks`, +set by `RecheckHealth`, which is how `--recheck-targets-on-restore` keeps a +restored target under observation. + +## Retries + +`RetryPolicy` (`internal/server/retry.go`) is `TargetTryDuration` plus +`TargetTryInterval` (`DefaultTargetTryInterval`, 250ms, when unset). A zero +duration means one attempt — exactly how the proxy behaves with no retries +configured, so an untouched deployment is unaffected. `--target-try-interval` +without `--target-try-duration` is a validation error rather than a silently +ignored flag. + +With a policy, `LoadBalancer.StartRequest` defers target selection entirely into +`serveWithRetries`, so each attempt picks a currently-healthy target rather than +retrying into the one that just failed. `Target.handleProxyError` (472-517) +records a retryable error on the attempt and renders nothing — no bytes may +reach the client before the retry. + +`WithRetryPolicy` and `WithSessionAffinity` are chained onto `NewLoadBalancer` +rather than folded into its signature, so its call sites stay uniform. + +## Limits and access control + +All three per-client decisions embed one `forwardedResolver` +(`internal/server/ip_allow_list.go`), so the trust rules have exactly one +implementation. Three bypasses closed there — repeated header lines, +IPv4-mapped IPv6, unresolvable chains — would otherwise have to be fixed once +per caller, and the one that got missed would be the vulnerable one. + +The rule: with no trusted proxies declared, the client is always the peer. +Otherwise, when the peer is one of ours, the client is taken from the forwarded +chain by walking it from the nearest hop backwards past any other proxy of ours. +`X-Forwarded-For`, `X-Real-IP` and whatever `--client-ip-header` names are +written by the client and are consulted only when the peer is a declared proxy. +Under `--proxy-protocol` the peer itself is rewritten from the PROXY preamble, +and is then only as trustworthy as `--proxy-protocol-allow-ip` makes it. + +| Thing | Zero address | Because | +|---|---|---| +| `ipAllowList` | never permitted, not even by a default route | an unparseable peer is not a reason to admit | +| `denyList` | matches nothing | each fails in the direction it exists for | +| `rateLimiter` | shares the overflow bucket | a cap, not a refusal | + +`denyList` (`internal/server/deny_list.go`) is `--deny-ip` and +`--deny-user-agent`. `emptyUserAgentPattern` (`^$`) is the one pattern an absent +User-Agent matches: every other rule, including `.*`, requires an agent to +actually be present, because half the non-browser HTTP clients in the world send +none and absence is not a crime. Deny runs before allow, so an address on both +lists is denied. + +`rateLimiter` (`internal/server/rate_limit.go`) counts IPv6 clients at a `/64` +(`rateLimitIPv6PrefixBits`): keying on the full `/128` would make the limit +decorative, since every IPv6 client holds at least a `/64` and can pick a fresh +source address per request — which both escapes the budget and allocates a +bucket each time. `rateLimitMaxTrackedClients` (50 000) bounds the bucket map, +otherwise a memory-exhaustion primitive handed to anyone who can vary a source +address; clients past the cap share an overflow bucket. Sweeping is lazy +(`rateLimitSweepInterval`, 1 minute) rather than run from a goroutine, so a +redeploy cannot leak one. Throttle logging is burst-limited per service +(`rateLimitLogBurst` 10, `rateLimitLogInterval` 10s). + +`basicAuthCredential` (`internal/server/basic_auth.go`) stores a `sha256` +digest and a 16-byte salt, deliberately **not** an adaptive hash: verification +runs on the *unauthenticated* request path, so a slow one hands an uncacheable +CPU-exhaustion primitive to exactly the people the password is there to keep +out. The salt defeats precomputation against a leaked state file; it does not +defend a weak password against an offline dictionary attack. A credential that +could not be read back from saved state sets `denyAll`, and that service refuses +every request rather than serving unprotected. + +## Pause, stop, rollout, affinity + +`PauseController` holds requests for up to a timeout (`pause`) or answers them +with a message (`stop`). Health checks still answer 200 while paused, so a +downstream load balancer does not drop the proxy itself. The pause wait happens +inside the closure `StartRequest` returns, after the service lock is released. + +`RolloutController` (`internal/server/rollout_controller.go`) sends a percentage +— or an explicit allowlist, matched on the `kamal-rollout` cookie — to the +`TargetSlotRollout` load balancer. + +`SessionAffinityPolicy` (`internal/server/session_affinity.go`) pins a client to +the target that first served it, for apps whose session state lives in the +instance rather than a shared store. The cookie is +`DefaultSessionAffinityCookieName` (`kamal-session`) unless the deployment names +one. Disabled is the default, and leaves target selection exactly as it is +without it. + +## Path-scoped timeouts + +`PathTimeout` (`internal/server/path_timeouts.go`) overrides a timeout below a +path prefix. A **zero** `Timeout` disables the timeout for that prefix rather +than falling back to the service-wide value — the distinction an upload or an +SSE endpoint needs. `NormalizePathTimeouts` canonicalizes prefixes the way +service path prefixes are canonicalized and orders them longest-first, so +`ResolvePathTimeout` returns the most specific match without sorting on every +request. `Target` builds one `httputil.ReverseProxy` per distinct response +timeout in play, so an override costs no per-request work. + +## Scale to zero + +`IdleController` (`internal/server/idle_controller.go`) is a four-state machine +per service: `IdleStateActive`, `IdleStateStopping`, `IdleStateSleeping`, +`IdleStateWaking`. It stops the containers after `--sleep-after` and starts them +on the next request that needs them, within `--wake-timeout`. + +Everything it needs from its `Service` arrives as a hook — `Suspend`, `Resume`, +`Persist`, plus a `ContainerLifecycle` — so the controller never touches a load +balancer, a target, or their locks. + +State is persisted as a **name**, not an enum ordinal. `ParseIdleState` folds +`stopping` and `waking` down to `sleeping` in both directions: a proxy that died +mid-transition cannot know whether the container moved, and waking from sleeping +is the safe assumption because starting an already-running container succeeds. +Anything unrecognised — including the empty string every pre-feature state file +carries — restores `active`. + +`Service.UnmarshalJSON` (578-630) records the restored state rather than acting +on it: the lifecycle is still nil at decode time, so a controller built there +could reach `StopContainer` on a nil interface. `SetContainerLifecycle` creates +it, after the router has decoded the whole state file. + +`ContainerLifecycle` (`internal/server/container_lifecycle.go`) is the seam. The +shipped implementation talks to the Docker socket directly — the smallest +opt-in approach, and also root-equivalent access to the host; a restricted +host-side start/stop service can replace it without the controller changing. + +## Related + +- `../request-path/summary.md` — where each of these sits in the per-request order +- `../cache/summary.md` — what sits between the checks and the load balancer +- `../observability/summary.md` — what these emit diff --git a/lode/review/acme-providers.md b/lode/review/acme-providers.md new file mode 100644 index 0000000..b8ee95b --- /dev/null +++ b/lode/review/acme-providers.md @@ -0,0 +1,85 @@ +The DNS-01 provider registry, and the rule that there is exactly one list of +providers in this codebase. + +### `internal/server/acme/providers/registry.go` is the only provider list; everything else derives from it +- **Holds because:** a second hand-maintained list — a `switch` in the factory, an ordered detection slice, a flag help string, a README table — drifts the first time a provider is added, and the copy that drifts is the one nobody is looking at. The registry map holds all eight entries (`ProviderCloudflare`, `ProviderRoute53`, `ProviderDigitalOcean`, `ProviderGoogleCloud`, `ProviderNamecheap`, `ProviderGoDaddy`, `ProviderHetzner`, `ProviderVultr`); `GetProviderInfo`, `NewProvider`, `detectionOrder`, the `--acme-dns-provider` help and the README table all read it. +- **Where:** `internal/server/acme/providers/registry.go` (`registry` at 65, `detectionOrder` at 139) +- **Proven by:** `TestREADMEProviderTable_MatchesRegistry`, `TestRunCommand_DNSProviderHelpMatchesRegistry` +- **Origin:** cubic learnings d6216872, fc516966 + +### Provider-name validation goes through `acme.ParseProviderName`, never through `providers.Names()` +- **Holds because:** `providers` imports `acme`, so validating from the registry package would create an import cycle. `ParseProviderName` carries the aliases as well as the canonical names (`cf`, `aws`/`r53`, `do`, `google`/`gcp`/`googledns`, `nc`, `gd`, `hz`, `vr`), which a derived list would silently drop. +- **Where:** `internal/server/acme/provider.go#ParseProviderName` (44-69), `internal/server/acme/mapping.go#ParseProviderEntries` +- **Proven by:** `TestRunCommand_ACMEDNSProviderZoneMappings`, `TestRunCommand_ACMEDNSProviderEnvSupportsMappings` +- **Origin:** cubic learning d6216872 + +### The only values that disable DNS-01 are the empty string and `none` +- **Holds because:** an undocumented alias such as `off` that quietly parsed as "disabled" would leave an operator believing DNS-01 was armed. `ParseProviderName` returns `ErrProviderNotSupported` for anything not in the switch, so a typo is a startup error rather than a silent HTTP-01 fallback. The generated help and the README stay aligned with the same set. +- **Where:** `internal/server/acme/provider.go#ParseProviderName` (44-69) +- **Proven by:** `TestRunCommand_DNSProviderDefaultsOff`, `TestRunCommand_DNSProviderHelpMatchesRegistry` +- **Origin:** cubic learning 468019dc + +### The generated table's `Name` column carries the exact parser-accepted flag value, not the display name +- **Holds because:** the table is what an operator copies into `--acme-dns-provider`. Printing "AWS Route53" there gives them a value the parser rejects. `DisplayName` is a separate field for the human-readable column. +- **Where:** `internal/server/acme/providers/registry.go` (`DisplayName`), `internal/server/acme/providers/docs.go` +- **Proven by:** `TestREADMEProviderTable_MatchesRegistry` +- **Origin:** cubic learning acaac3bf + +### The README table is generated, and a drift test fails the build +- **Holds because:** a committed table is the one artefact that cannot be derived at runtime, so it needs a guard. `go generate ./internal/server/acme/providers` rewrites `README.md` between `<!-- BEGIN GENERATED: dns-provider-table … -->` and `<!-- END GENERATED: dns-provider-table -->` (README.md:1142-1153). When the test detects drift it reports *every* stale or missing row — including changed credential or optional columns — and names the command to regenerate, because a failure listing one row sends the author back for a second run. +- **Where:** `internal/server/acme/providers/docs.go` (`//go:generate go run ./gen`, markers at 19-20), `internal/server/acme/providers/gen/` +- **Proven by:** `TestREADMEProviderTable_MatchesRegistry` +- **Origin:** cubic learning fc516966 + +### Cloudflare's credential rule lists exactly the four canonical same-namespace sets lego accepts +- **Holds because:** the boot check exists to refuse a configuration that cannot construct. Vouching for one that can't is the failure that shipped (`CF_API_TOKEN`, which lego never reads, #115) — it fails **open**. The `Alternatives` OR-of-ANDs is `{CF_DNS_API_TOKEN}`, `{CLOUDFLARE_DNS_API_TOKEN}`, `{CF_API_KEY, CF_API_EMAIL}`, `{CLOUDFLARE_API_KEY, CLOUDFLARE_EMAIL}` — token or key+email, in either namespace, never mixed across namespaces. +- **Where:** `internal/server/acme/providers/registry.go` (66-83) +- **Proven by:** `TestNewProvider_CloudflareCredentialSetsConstruct` +- **Origin:** cubic learning 23da0116; PR #116 + +### Not a bug: a mixed-namespace Cloudflare pair is refused even though lego would construct it +- **Holds because:** lego resolves each field independently, so `CF_API_KEY` + `CLOUDFLARE_EMAIL` constructs there and fails this check. The two directions are not symmetric: #115's bug failed **open** (the check vouched for a config that could not construct), while this gap fails **closed**, with an error naming exactly which variables to set — safe and self-correcting. Enumerating the cross-namespace mixes would grow the registry to six credential sets, all rendered into the generated README table, documenting a mixed-namespace hygiene nobody should adopt. +- **Where:** `internal/server/acme/providers/registry.go` (66-83), `#NewProvider` +- **Proven by:** `TestNewProvider_CloudflareCredentialSetsConstruct` (the canonical sets); the mixed-namespace direction is asserted to fail closed +- **Origin:** PR #116 review thread (partially accepted, remainder declined with reasoning) + +### `NoBootCheck` and `Detect` are separate fields because Route53 needs them to differ +- **Holds because:** the AWS SDK resolves credentials from the environment, shared config or an IAM role on its own, so an empty environment is not an error and the boot check must be skipped. Auto-detection still has to key off something *visible*, which is what `Detect` (`AWS_ACCESS_KEY_ID` or `AWS_PROFILE`) supplies. Folding them into one field would either break detection on an instance role or make an empty environment fatal. +- **Where:** `internal/server/acme/providers/registry.go` (84-95), `#credentialSets`, `#detectSets` +- **Proven by:** `TestSANCertManager_InitDNSClients_MappedProviderWithoutCredentialsIsFatal`, `TestSANCertManager_InitDNSClients_DefaultProviderFallbackStaysSoft` +- **Origin:** verified from the code during seeding + +### With `auto`, the boot warning names the provider that actually constructed +- **Holds because:** `DetectProviderName`'s credential match is looser than construction — it can point at a provider whose `New()` then fails, so the warning would name one provider while another (or none) is armed. The name is taken from the successful construction walk. +- **Where:** `internal/server/san_cert_manager.go#initDNSClients` +- **Proven by:** `TestSANCertManager_InitDNSClients_NoProviderConfiguredStaysHTTP01`, `TestSANCertManager_InitDNSClients_DefaultProviderFallbackStaysSoft` +- **Origin:** cubic learning 3120d37a + +### A mapped provider with no credentials is fatal at boot; a *default* provider's fallback stays soft +- **Holds because:** a zone mapping is an explicit statement that this zone needs DNS-01 — booting anyway leaves wildcards in that zone quietly unissuable. A default provider is a preference for zones that have no mapping, and those zones can still be served over HTTP-01, so it degrades rather than refusing to start. +- **Where:** `internal/server/san_cert_manager.go#initDNSClients` +- **Proven by:** `TestSANCertManager_InitDNSClients_MappedProviderWithoutCredentialsIsFatal`, `TestSANCertManager_InitDNSClients_DefaultProviderFallbackStaysSoft`, `TestSANCertManager_InitDNSClients_NoProviderConfiguredStaysHTTP01` +- **Origin:** cubic learning 3120d37a + +### `ProviderFor` picks the longest matching zone suffix, on label boundaries +- **Holds because:** a fleet can map `example.com` to one host and `eu.example.com` to another, and the more specific mapping has to win. Matching on a bare string suffix would make `notexample.com` match `example.com`; the comparison requires the candidate to equal the zone or end in `"." + zone`. The matched zone doubles as the partition key that keeps one ACME order on one provider. +- **Where:** `internal/server/acme/mapping.go#ProviderFor` (31-49) +- **Proven by:** `TestSANCertManager_SplitByProviderZone_NestedWildcardZonesLongestWins`, `TestSANCertManager_SplitByProviderZone_MappedSubzoneStaysOutOfWildcardOrder`, `TestRunCommand_ACMEDNSProviderZoneMappings` +- **Origin:** verified from the code during seeding + +### `auto` cannot be mapped to a zone +- **Holds because:** `auto` means "pick the one default from visible credentials", and a zone mapping is by definition explicit. Allowing `zone=auto` would make the answer depend on the environment rather than on what the operator wrote. +- **Where:** `internal/server/acme/mapping.go#ParseProviderEntries` (56-103) +- **Proven by:** `TestRunCommand_ACMEDNSProviderZoneMappings` +- **Origin:** verified from the code during seeding + +### A stub ACME directory handler reports encoding failures with `t.Error`, not `require.NoError` +- **Holds because:** the handler runs on the `httptest` server's goroutine, and `require` calls `t.FailNow`, which is only legal on the test's own goroutine — there it aborts the handler without failing the test, so the assertion silently stops asserting. +- **Where:** `internal/server/san_cert_manager_test.go` (stub directory handler) +- **Proven by:** the convention itself; it is a test-code rule +- **Origin:** cubic learning 982751b2 + +## Related + +- `certs-issuance.md` — how a provider zone partitions an order +- `../certs/summary.md` — the subsystem as a whole diff --git a/lode/review/cert-store-export.md b/lode/review/cert-store-export.md new file mode 100644 index 0000000..f999dff --- /dev/null +++ b/lode/review/cert-store-export.md @@ -0,0 +1,80 @@ +Writing a certificate-store archive: where it may be written, what goes in it, +and what the reader will refuse. A backup that cannot restore is worse than no +backup, so every rule here fails the export rather than shipping a doubtful one. + +### `export certs` falls back to an offline read only when nothing answers the socket +- **Holds because:** reading the certificate directory while a live proxy is writing it produces a torn archive. But a *live but outdated* proxy answers the socket and rejects the method, and a broad `strings.Contains(err, "can't find method")` would also swallow real export failures behind upgrade advice. The check is the exact net/rpc prefix `rpc: can't find method kamal-proxy.CertsExport`; that case fails with guidance to upgrade or stop the proxy, and never reads the directory behind its back. +- **Where:** `internal/cmd/export.go` +- **Proven by:** `TestExportCertsCommand_ExportsOffline`, `TestExportCertsCommand_EmptyStoreFails`, `TestSANCertManager_ExportStoreHoldsTheDiskLock` +- **Origin:** cubic learning bd7d0038 + +### The destination is validated by resolved path *and* by filesystem identity, against every existing ancestor +- **Holds because:** a string comparison of resolved paths misses aliases — a symlinked parent, a case-insensitive spelling of `acme.state`, a second mount of the same directory — and writing an archive over the live store destroys exactly what it was meant to preserve. `safeOutputPath` resolves symlinks on the destination (or on its deepest existing ancestor, rejoining the remainder) and on the store paths, compares those, and then compares `os.SameFile` identity against each existing ancestor and the certificate directory. +- **Where:** `internal/server/cert_store_export.go#safeOutputPath`, `#resolveForComparison` +- **Proven by:** `TestExportCertificateStore_RejectsOutputInsideTheStore`, `TestExportCertificateStore_RejectsSymlinkedOutputIntoTheStore` +- **Origin:** cubic learning caf46a10 + +### After validation the output directory is pinned as an `os.Root`, and its identity is re-checked against the certificate tree +- **Holds because:** validating a pathname and then using that pathname again is a TOCTOU: swapping a parent component between the two redirects the write. The resolved directory is opened once and that handle is reused for the staged verification read, the temp file, the rename and the directory sync. `rejectPinnedRootInsideStore` → `dirInsidePinnedTree` then walks the certificate tree *through its own pinned root* and compares directory identities against the pinned output handle, so neither side of the containment check is a re-resolvable pathname. +- **Where:** `internal/server/cert_store_export.go#writeCertArchive`, `#rejectPinnedRootInsideStore`, `#dirInsidePinnedTree` +- **Proven by:** `TestDirInsidePinnedTree`, `TestExportCertificateStore_RejectsSymlinkedOutputIntoTheStore` +- **Origin:** cubic learning caf46a10 + +### The containment walk fails closed: a `WalkDir` or `Info()` error aborts the export +- **Holds because:** a subtree that cannot be read or statted is a subtree whose identity was never compared. Treating it as "not a match" makes containment fail *open* — the one direction where the guard silently stops guarding. Propagating the error costs a failed export, which is recoverable. +- **Where:** `internal/server/cert_store_export.go#dirInsidePinnedTree`, `#rejectPinnedRootInsideStore` +- **Proven by:** `TestDirInsidePinnedTree` +- **Origin:** cubic learning caf46a10 + +### The archive is staged in a short, fixed-pattern temp file at mode 0600, verified, fsynced, renamed, and the directory fsynced +- **Holds because:** the archive carries private keys, so it must never exist world-readable, even briefly. A temp name derived from the destination basename can push past a filesystem's component-length limit for a near-limit destination and fail the export for a reason nobody will diagnose, so the pattern is short and fixed. The staged file is then read back through `readCertStoreArchive` before it is published: a backup nobody has verified is a backup nobody should trust. Directory-open and directory-sync failures fail the export — a rename that is not durable was not a successful backup — with only `ENOTSUP`/`EINVAL` excused. +- **Where:** `internal/server/cert_store_export.go#writeCertArchive`; `internal/server/cert_store_restore.go#syncOpenDir` +- **Proven by:** `TestExportCertificateStore_LongOutputBasename`, `TestExportCertificateStore_OverwritesAnExistingArchive`, `TestExportCertificateStore_ArchivesTheWholeEstate` +- **Origin:** cubic learnings 5538fcd9, 73645b0a, fab2ff92, 0ffc7900 + +### An unparseable `acme.state` aborts the export; a missing optional file does not +- **Holds because:** `acme.state` is the index — an archive without a usable one cannot be restored into a working estate, whatever else is in it. The optional files (`dynamic-domains.state`, per-directory account keys) are recoverable by other means, so their absence is a warning. +- **Where:** `internal/server/cert_store_export.go#collectStateEntry`, `#ExportCertificateStore` +- **Proven by:** `TestExportCertificateStore_UnparseableStateIsAnError`, `TestExportCertificateStore_OptionalFilesMayBeMissing`, `TestExportCertificateStore_SkipsInvalidOptionalFiles` +- **Origin:** cubic learning e1eecc33 + +### Real certificate pairs with no state file are refused; an account-key-only estate still exports +- **Holds because:** the archive reader cannot turn loose pairs into a working estate, so shipping that shape would produce a backup that fails at restore time. But ACME account keys under `certs/` are metadata, not certificates: a proxy that has registered an account and issued nothing yet has a perfectly restorable estate. The no-state check runs after filtering out both `acme_user.json` and every name `isExtraAccountKeyFile` recognises. +- **Where:** `internal/server/cert_store_export.go#ExportCertificateStore`, `#collectCertsEntries`, `#isExtraAccountKeyFile` +- **Proven by:** `TestExportCertificateStore_CertsWithoutStateIsAnError`, `TestExportCertificateStore_AccountKeyOnlyStoreExports`, `TestExportCertificateStore_AccountKeysOnlyStoreExports` +- **Origin:** cubic learnings e1eecc33, a4473d2d + +### Each pair is parsed with `tls.X509KeyPair` before it is archived; an invalid one is skipped with a warning +- **Holds because:** archiving bytes that are not a usable pair produces an archive that passes its own structural check and fails at restore. Skipping with a warning is the right direction: the rest of the estate is still worth backing up, and the operator is told which certificate is not. +- **Where:** `internal/server/cert_store_export.go#collectCertPair`, `#warnMissingStateCerts` +- **Proven by:** `TestExportCertificateStore_SkipsUnparseablePairs`, `TestExportCertificateStore_Warnings` +- **Origin:** cubic learning 295ad96c + +### An empty file set is an error, not an empty archive +- **Holds because:** a cron job faithfully archiving nothing every night is the failure mode that is discovered during an incident. `ErrCertStoreEmpty` is loud. +- **Where:** `internal/server/cert_store_export.go#ExportCertificateStore` +- **Proven by:** `TestExportCertificateStore_EmptyStoreIsAnError`, `TestExportCertsCommand_EmptyStoreFails` +- **Origin:** verified from the code during seeding + +### Warnings from the staged verification reach the summary, minus the ones the collection pass already reported +- **Holds because:** an unusable ACME account key is only detectable by *reading* the archive, and it is exactly the kind of thing the operator needs to know at backup time rather than at restore time. Re-reporting `warnMissingCertificate` would double every line the disk-side pass already printed. +- **Where:** `internal/server/cert_store_export.go#writeCertArchive`, `CertsExportSummary` +- **Proven by:** `TestExportCertificateStore_SurfacesUnrestorableAccountKeyWarning`, `TestCertsExportSummary_RoundTrips` +- **Origin:** cubic learnings ad0a86b9, 16f47aa3 + +### Warnings are structured kinds internally and flattened to text only at the reporting boundary +- **Holds because:** the exporter has to filter warnings by class (drop the missing-certificate ones the collection pass already made), and matching on rendered English to do it breaks the first time a sentence is reworded. +- **Where:** `internal/server/cert_store_archive.go` (warning kinds), `internal/server/cert_store_export.go` +- **Proven by:** `TestExportCertificateStore_Warnings`, `TestVerifyCertificateArchive_DropsInvalidAccountKey` +- **Origin:** cubic learning 16f47aa3 + +### `export certs <output-path>` takes the path as a required positional argument +- **Holds because:** validation is `cobra.ExactArgs(1)`, so the usage line has to show the argument or the error a user gets contradicts the usage they were shown. +- **Where:** `internal/cmd/export.go` +- **Proven by:** `TestExportCertsCommand_RequiresAnOutputPath` +- **Origin:** cubic learning b8523c0b + +## Related + +- `cert-store-restore.md` — reading an archive back, and the importers +- `../certs/store-and-recovery.md` — the subsystem as a whole diff --git a/lode/review/cert-store-restore.md b/lode/review/cert-store-restore.md new file mode 100644 index 0000000..d11ee8f --- /dev/null +++ b/lode/review/cert-store-restore.md @@ -0,0 +1,110 @@ +Reading an archive back, and the two importers. Structural doubt is an error +here; an expired certificate is not — a faithful backup of an expired +certificate is still a faithful backup. + +### The whole decompressed gzip stream is capped, not just the file entries +- **Holds because:** `tar.Reader.Next()` consumes PAX/GNU metadata records before any entry counter can see them, so a cap applied only to entry bodies leaves an unbounded work primitive in the header path. `maxCertArchiveBytes` (512 MiB), `maxCertArchiveEntries` (100 000 headers) and `maxCertArchiveHeaderBytes` (64 MiB) are all enforced, with the capped reader wrapping the gzip stream that feeds the tar reader. +- **Where:** `internal/server/cert_store_archive.go` (`maxCertArchiveBytes` 29, `maxCertArchiveEntries` 30, `maxCertArchiveHeaderBytes` 37), `#readCertStoreArchive` +- **Proven by:** `TestCappedReader_FailsBeyondTheLimit`, `TestCappedReader_ExactlyAtTheLimitIsNotOversized` +- **Origin:** cubic learning 1cedc5a5 + +### `cappedReader.Read`'s boundary semantics are exact, and a zero-length read never touches the underlying reader +- **Holds because:** an archive ending *exactly* at the cap is legal and must not be rejected, while one byte past it must be. At an exhausted budget the reader probes a single byte: `io.EOF` means it ended at the cap and is accepted; any positive count is overflow and fails with `errCertArchiveTooLarge`; a legal `(0, nil)` is passed through rather than read as overflow. A zero-length read returns `(0, nil)` at the top of `Read` regardless of budget, because `io.Reader` permits it and calling through would misreport the boundary. +- **Where:** `internal/server/cert_store_archive.go#cappedReader` +- **Proven by:** `TestCappedReader_ExactlyAtTheLimitIsNotOversized`, `TestCappedReader_FailsBeyondTheLimit` +- **Origin:** cubic learning 1cedc5a5 + +### After tar EOF the stream is drained to its own EOF, so the gzip trailer is validated +- **Holds because:** tar EOF is two zero blocks — it says nothing about whether the gzip stream that carried them is intact. Stopping there accepts an archive with a corrupt trailer or a failing CRC, and leaves any trailing decompressed data uncounted against the cap. +- **Where:** `internal/server/cert_store_archive.go#readCertStoreArchive` +- **Proven by:** `TestVerifyCertificateArchive_RejectsCorruptGzipTrailer` +- **Origin:** cubic learning 1cedc5a5 + +### Emptiness is counted in regular files, while the DoS cap counts every header +- **Holds because:** an archive of nothing but directory and metadata headers restores into an empty store, so it must be refused as empty — but those same headers are work, so they still count toward the entry cap. The two counters answer different questions. +- **Where:** `internal/server/cert_store_archive.go#readCertStoreArchive` +- **Proven by:** `TestVerifyCertificateArchive_DirectoryOnlyArchiveIsEmpty` +- **Origin:** cubic learning 1cedc5a5 + +### `placeEntry` refuses any name the exporter would never write, including an unsanitized certificate directory +- **Holds because:** two spellings that collapse to one on-disk path is how a restore overwrites the wrong certificate. The requirement is `dir == sanitizeFilename(dir)` — already in sanitized form, not sanitizable-to — so no alternate spelling is normalised into an existing directory. Symlink entries are refused outright. +- **Where:** `internal/server/cert_store_archive.go#placeEntry` +- **Proven by:** `TestReadCertStoreArchive_RejectsUnsanitizedDirNames`, `TestVerifyCertificateArchive_RejectsSymlinkEntries` +- **Origin:** cubic learnings 4d3f5498, cc2f2df4 + +### `validateManagerState` rejects identifier/key mismatches and sanitization collisions before anything is mapped to a pair +- **Holds because:** a record whose `Identifier` differs from its map key, or two distinct identifiers that sanitize to the same directory, produce certificates that fail renewal, removal and host coverage later — long after the archive was declared good. It also requires both maps present, no null record, and every `DomainMap` key covered by its target's `Domains` under the same wildcard-aware `identifiersCover` the issuance path uses, so the archive cannot disagree with issuance about what covers what. +- **Where:** `internal/server/cert_store_archive.go#validateManagerState` +- **Proven by:** `TestVerifyCertificateArchive_RejectsCorruptStateRecords`, `TestExportCertificateStore_InvalidStateIsAnError` +- **Origin:** cubic learnings a21f3e92, 4e3e8493 + +### Each state-referenced pair's leaf is cross-checked against its state record: DNS names as an exact set, expiry to the second +- **Holds because:** state that has drifted from the leaf is a store that will renew the wrong identifier set or refuse a host it is serving. The set equality is order-independent and exact in both directions — a leaf covering *more* than the record claims is as wrong as one covering less. +- **Where:** `internal/server/cert_store_archive.go#readCertStoreArchive` +- **Proven by:** `TestVerifyCertificateArchive_RejectsLeafDisagreeingWithState`, `TestVerifyCertificateArchive_ReportsDomainsAndExpiries` +- **Origin:** cubic learning 312e2ba2 + +### An unusable ACME account key is a warning, not a failure — and only `*ecdsa.PrivateKey` is usable +- **Holds because:** the boot-time loader accepts nothing else, so a valid RSA key would restore and then fail to load, which is worse than not restoring it. Losing the account costs a fresh registration on the next boot; losing the certificates would cost an outage. The warning says exactly that. +- **Where:** `internal/server/cert_store_archive.go` (account payload validation, `warnAccountKey`) +- **Proven by:** `TestVerifyCertificateArchive_DropsInvalidAccountKey` +- **Origin:** cubic learning d9a06e31 + +### A restore writes `acme.state` last, and every write goes through `writeFileStaged` +- **Holds because:** the index must never name files that are not on disk, so certificate pairs, then account keys, then `dynamic-domains.state`, then the index. `writeFileStaged` creates a unique same-directory temp at mode 0600, writes, fsyncs, closes, renames, syncs the directory, and removes the temp on every failure path — so a forced restore cannot follow a planted symlink, cannot inherit a pre-existing temp file's permissions for a private key, and cannot leave a truncated file or stray temp behind. The temp pattern is short and fixed for the same length reason as the exporter's. +- **Where:** `internal/server/cert_store_restore.go#RestoreCertificateStore`, `#writeFileStaged`, `#syncOpenDir` +- **Proven by:** `TestRestoreCertificateStore_RoundTrip`, `TestRestoreCertificateStore_RefusesNonEmptyStore`, `TestRestoreCertificateStore_RoundTripsExtraAccountKeysAndDirectories` +- **Origin:** cubic learnings 4688cd49, 18bcaeab, 5538fcd9 + +### Stale certificate directories are removed *after* the state commit, not before +- **Holds because:** a restore that fails midway must leave the old store intact. Removing first and failing later leaves an old index pointing at deleted directories — unrecoverable without the archive that just failed to apply. A post-commit removal failure is surfaced as an error against an already-restored state, which is the recoverable side of the trade and needs no quarantine-and-rollback machinery. +- **Where:** `internal/server/cert_store_restore.go#RestoreCertificateStore`, `#removeStaleCertDirs` +- **Proven by:** `TestRestoreCertificateStore_ForceRemovesStaleStateReferencedDirs`, `TestRestoreCertificateStore_ForceKeepsUnreferencedCertDirs` +- **Origin:** cubic learning 766ad149; PR #95 review thread + +### A removal counts only when `Lstat` confirms the directory existed, and `certsPath` is synced only if a real unlink happened +- **Holds because:** syncing a directory that was never changed — or that does not exist — either fails the restore after the state was committed or reports durability that was never at stake. Any inspection error other than `os.ErrNotExist` fails the restore: a path that cannot be statted is a path whose removal was never verified. The degenerate shape where state references a certificate whose directory is absent stays tolerated, succeeding with only the re-order warning. Directory-sync failures on a real removal remain fatal, or a crash resurrects the stale directories. +- **Where:** `internal/server/cert_store_restore.go#removeStaleCertDirs` +- **Proven by:** `TestRestoreCertificateStore_DegenerateArchiveIntoFreshStore`, `TestRestoreCertificateStore_ForceRemovesStaleStateReferencedDirs` +- **Origin:** cubic learnings 702a3400, 22f3ec11 + +### Directory-sync errno policy lives in exactly one place +- **Holds because:** `ENOTSUP`/`EINVAL` are the only excusable outcomes (some filesystems do not support directory fsync), and every other errno must fail. Spreading that judgment across the export path, the restore path and the pinned-root path guarantees the copy someone forgets is the one that silently swallows a real I/O error. `syncOpenDir(*os.File)` holds it; `syncDir` (pathname) and `syncRootDir` (pinned `os.Root`) delegate. +- **Where:** `internal/server/cert_store_restore.go#syncOpenDir` (241-246) and `#syncDir` (227-235); `internal/server/cert_store_export.go#syncRootDir` (628) +- **Proven by:** no direct test — the policy is one three-line function, asserted indirectly by every restore and export round-trip +- **Origin:** cubic learning b969eeb3 + +### Recovery is certificates only, so the runbook restores routing state separately, before startup +- **Holds because:** the archive holds `acme.state`, `certs/` and `dynamic-domains.state` — not the routing table. A restore that stops there boots a proxy with certificates and no services. `dash-proxy.state` (or its `.bak`) is restored while the proxy is stopped; without it, redeploy the TLS services after boot. And `domains list` reports only dynamic `--tls-domains-source` domains, so it is not evidence that static `--tls --host` services came back — verify those with a TLS handshake and the expiry metrics. +- **Where:** `lode/certs/store-and-recovery.md`; `internal/server/cert_store_export.go#CertStorePaths`, `internal/cmd/domains.go` +- **Proven by:** documentation rule, not code; the paths are asserted by `TestRestoreCertificateStore_RoundTrip` +- **Origin:** cubic learnings 4a7924d7, 192f9ab3 + +### `import certs` flag modes: `--verify`, `--force` and `--traefik-acme` are mutually exclusive +- **Holds because:** verifying and forcing are opposite intents, and the Traefik importer reads a completely different input. One of `--traefik-acme`/`--archive` is required and those two are exclusive; `--archive` excludes `--resolver`. What survives is `--archive --verify` and `--archive --force` as the only valid combinations of those three. +- **Where:** `internal/cmd/import.go` +- **Proven by:** `TestImportCertsCommand_FlagValidation`, `TestImportCertsCommand_RequiresTraefikAcmeFlag`, `TestImportCertsCommand_VerifyReportsWithoutWriting` +- **Origin:** cubic learning 0b344bc1 + +### `import certs` creates a non-empty `--data-dir` before importing +- **Holds because:** an import where every entry is skipped still has to write `acme.state`, and a missing directory turns that into a confusing write failure at the end of an otherwise successful run. +- **Where:** `internal/cmd/import.go`, `internal/cmd/util.go#ensureDataDir` +- **Proven by:** `TestImportCertsCommand_CreatesAMissingDataDir`, `TestImportCertsCommand_ImportsIntoTheDataDir` +- **Origin:** cubic learning e71772eb + +### The Traefik importer accepts certificates on their validity window alone, and leaves policy to `GetCertificate` +- **Holds because:** registered-versus-dynamic is a live routing question the importer cannot answer offline — it has no router. Deciding it at import time would bake a snapshot of the deployment into the store. `loadStateForImport` decodes existing state into a zero-value `managerState` and refuses nil required maps, so an invalid state file is left untouched rather than half-overwritten. +- **Where:** `internal/server/traefik_import.go#loadStateForImport`; `internal/server/san_cert_manager.go#GetCertificate` +- **Proven by:** `TestImportTraefikCertificates_StateLoadsIntoSANCertManager`, `TestImportTraefikCertificates_SkipsExpiredAndNotYetValid`, `TestImportTraefikCertificates_ImportsValidEntries` +- **Origin:** cubic learnings 8f4b1ef2, 68ca61cb + +### Not a bug: the Traefik importer replaces `cert.pem` and `key.pem` per file, with no pair-level transaction +- **Holds because:** both files are staged as `.tmp` before either rename, so a failed or partial *write* cannot break an existing pair. What remains is a process kill exactly between the two renames, and that already degrades gracefully: `loadState`'s `tls.LoadX509KeyPair` fails, the certificate loads with a nil `tls.Certificate`, and `GetCertificate` falls through to ordinary provisioning — a re-order, not certificate loss. A pair-level atomic swap is not implementable in the manager's fixed layout (POSIX `rename` cannot atomically replace a non-empty directory, and the running manager reads these exact paths), and the import is offline and idempotent. +- **Where:** `internal/server/traefik_import.go` +- **Proven by:** `TestImportTraefikCertificates_ReplacesShorterLivedExistingMapping`, `TestImportTraefikCertificates_KeepsLongerLivedExistingMapping` +- **Origin:** cubic learning bdc8c1f9; PR #91 review thread (partially accepted, remainder declined with reasoning) + +## Related + +- `cert-store-export.md` — writing an archive +- `../certs/store-and-recovery.md` — the subsystem as a whole diff --git a/lode/review/certs-directories-and-renewal.md b/lode/review/certs-directories-and-renewal.md new file mode 100644 index 0000000..816b85e --- /dev/null +++ b/lode/review/certs-directories-and-renewal.md @@ -0,0 +1,91 @@ +Per-service ACME directories (`--tls-staging`), and what the renewer does with a +certificate whose members no longer agree about where they should be issued. + +### A covering certificate only counts if it was issued at the owning service's current directory +- **Holds because:** a staging certificate covers the same names as a production one and is untrusted by every client. Without the directory check, a service flipped from `--tls-staging` to production would keep serving the staging leaf forever, because coverage alone said it was fine. `RegisterDomain`, `GetCertificate` and `HasValidCertificate` all require the match; a mismatch keeps the domain pending and reprovisions it. +- **Where:** `internal/server/san_cert_manager.go#RegisterDomain`, `#GetCertificate` (430-512), `#HasValidCertificate`; `internal/server/san_cert_directories.go#certMatchesServiceDirectoryLocked` +- **Proven by:** `TestSANCertManager_RegisterDomainRejectsCoverageFromAnotherDirectory`, `TestSANCertManager_CertDirectoryMismatched`, `TestSANCertManager_GetCertificate_MismatchedDirectoryCertReprovisionsSynchronously` +- **Origin:** cubic learning 905438d7 + +### A registered domain reprovisions synchronously on a directory mismatch; a dynamic one is served stale while the issuer replaces it +- **Holds because:** the serve-stale rule was written for the *degraded renewal* case — reaching the expiry window means proactive renewal was already failing, so a synchronous order on the handshake is most likely to fail too. A directory mismatch is the opposite: fresh operator intent, with a presumably healthy CA, and the staging leaf is untrusted by exactly the clients a staging→production flip is for. Dynamic domains stay on the serve-stale path even then, or one flag flip fails every tenant handshake at once while the issuer drains a rate-limited queue. +- **Where:** `internal/server/san_cert_manager.go#GetCertificate` (430-512) +- **Proven by:** `TestSANCertManager_GetCertificate_MismatchedDirectoryCertReprovisionsSynchronously`, `TestSANCertManager_GetCertificate_MismatchedDynamicCertServedWhileIssuerReplaces`, `TestSANCertManager_GetCertificate_ServesExpiringRegisteredCertAndQueuesReplacement`, `TestSANCertManager_GetCertificate_ExpiredRegisteredCertReprovisionsSynchronously` +- **Origin:** PR #103 review thread (accepted with a deliberate deviation) + +### `createCertManager` clears the service's directory override first, on every branch +- **Holds because:** the override is keyed by service name in a manager the service may no longer be using. Clearing only inside the shared-SAN branch leaves a stale `--tls-staging` behind when a service redeploys onto static certificates or off TLS entirely — and the next service to reuse that name inherits it. The clear happens before the TLS-mode branches; `ACMEDirectory` is then recorded before any host is registered, so the first `RegisterDomain` already sees the right directory. +- **Where:** `internal/server/service.go#createCertManager`; `internal/server/san_cert_directories.go#SetServiceDirectory` +- **Proven by:** `TestRouter_RedeployOffTheSANManagerClearsTheDirectoryOverride`, `TestRouter_DeployRegistersServiceDirectoryWithSANManager`, `TestSANCertManager_SetServiceDirectory` +- **Origin:** cubic learning 2fb499ef + +### A wildcard's directory comes from the concrete domains in its batch, and failing that only from names the certificate actually serves +- **Holds because:** a synthesized `*.example.com` has no service of its own. Resolving it by scanning everything the wildcard could cover picks up hosts from unrelated services and issues at the wrong directory. `directoryForDomains` prefers the concrete batch domains so the wildcard inherits its originating service; without batch context `wildcardOwnerLocked` consults only the certificate's own `Domains` and picks the smallest covered name, so the answer is deterministic. +- **Where:** `internal/server/san_cert_directories.go#directoryForDomains`, `#wildcardOwnerLocked` +- **Proven by:** `TestSANCertManager_DirectoryForDomains`, `TestSANCertManager_WildcardOwnerConsultsOnlyDomainsTheCertServes` +- **Origin:** cubic learning 90fb8c5f + +### Account keys are named per directory, and the naming predicate is closed +- **Holds because:** two directories are two ACME accounts; sharing one key between them makes the second registration fail. The run-level directory uses `acme_user.json`, Let's Encrypt staging `acme_user_staging.json`, anything else `acme_user_<8 lowercase hex>.json`. `isExtraAccountKeyFile` accepts exactly those two extra shapes and nothing else — an open `acme_user_*.json` glob would let a planted file in a restored archive be adopted as this proxy's ACME identity. +- **Where:** `internal/server/san_cert_directories.go#accountFileForDirectory`, `#clientsForDirectory`; `internal/server/cert_store_export.go#isExtraAccountKeyFile` +- **Proven by:** `TestSANCertManager_AccountFileForDirectory`, `TestSANCertManager_AccountFileForStagingWhenDefaultIsProduction`, `TestIsExtraAccountKeyFile`, `TestRestoreCertificateStore_RoundTripsExtraAccountKeysAndDirectories` +- **Origin:** cubic learning 800d1de2 + +### A renewal is partitioned by desired directory first, then by provider zone, and each partition issues under its own owner's identity +- **Holds because:** one order has exactly one ACME identity and one DNS provider. A certificate whose members' services have drifted to different directories cannot be renewed as one order at all; issuing it at the run-level default would silently move every member. `renew` resolves every owner it can, splits by directory, splits each of those by provider zone, and records each partition under the directory it was issued at. +- **Where:** `internal/server/domain_renewal.go#renew` (239-358), `#renewPartition` +- **Proven by:** `TestCertRenewer_SplitsMixedDirectoryCertificateAtRenewal`, `TestCertRenewer_SplitsMixedZoneCertificateAcrossProviders`, `TestCertRenewer_MixedZoneSplitKeepsOldCertWhenAPartitionFails` +- **Origin:** cubic learning 9b8e7442 + +### A partition whose owner cannot be resolved is pinned to the certificate's *recorded* directory, not the run-level default +- **Holds because:** an unresolved owner means the service is gone or renamed — which says nothing about where the certificate belongs. Falling back to the run-level directory silently migrates a staging certificate into production (or the reverse) on a routine renewal. Both the issuance and the adoption are pinned to the recorded value, so the record stays true. +- **Where:** `internal/server/domain_renewal.go#renew`, `#renewPartition` +- **Proven by:** `TestCertRenewer_UnresolvedOwnerRenewsAtRecordedDirectory` +- **Origin:** cubic learning 7e50719a + +### The ARI `replaces` marker rides at most one partition — the first still at the recorded directory — and is spent on CA acceptance +- **Holds because:** RFC 9773's marker identifies the certificate being replaced, and a CA rejects a second order naming an identifier it has already honoured. So it must not be attached to a partition that has switched directories (a different CA has never heard of it), and it must be spent at *order acceptance*, not at local adoption: `Obtain` succeeding means the replacement exists at the CA, and re-sending the marker after a locally failed adoption would have the next order refused. `renewPartition` returns `(renewed, adopted, ordered, accountLimit)` to make that boundary explicit, and the marker survives to a later same-directory partition when an earlier order is refused outright. +- **Where:** `internal/server/domain_renewal.go#renew`, `#renewPartition` +- **Proven by:** `TestCertRenewer_ARIMarkerSurvivesAFailedFirstPartition`, `TestCertRenewer_DirectorySwitchDropsARIReplaces`, `TestCertRenewer_HonorsARIWindow` +- **Origin:** cubic learning 19d3eba8; PR #105 review thread + +### An account-level rate limit halts that certificate's partition loop, and only that directory's +- **Holds because:** an account-level rejection names no identifiers, so there is nothing to quarantine and nothing to learn from trying the next partition — every one of them would be refused at the same account, spending attempts against a limit that is already tripped. `handleRenewalFailure` returns halt and `renew` breaks the loop for this pass. Another directory is a separate account with a separate bucket, so it still proceeds. +- **Where:** `internal/server/domain_renewal.go#handleRenewalFailure`, `#renew` +- **Proven by:** `TestCertRenewer_AccountLevelRateLimitStopsThePartitionLoop`, `TestCertRenewer_AccountLevelRateLimitIsScopedToItsDirectory` +- **Origin:** cubic learning 5e9e37e0 + +### Renewal defers rather than shrinks while there is still time, and compacts only inside the compaction window +- **Holds because:** dropping a quarantined member from the identifier set unmaps a host that is still being served, and it forfeits the CA's identical-set renewal exemption, so the shrunken order costs a fresh rate-limit slot. Deferring keeps both. `compactionWindowFor` is where the deferral stops being safe. +- **Where:** `internal/server/domain_renewal.go#renew` (239-358), `#compactionWindowFor` +- **Proven by:** `TestCertRenewer_DefersPartiallyQuarantinedBatchWhenTimeAllows`, `TestCertRenewer_CompactsQuarantinedDomainsNearExpiry`, `TestCertRenewer_SkipsRenewalWhenAllDomainsQuarantined` +- **Origin:** cubic learning 86e6a9df + +### A certificate covering any deploy-registered host compacts a week earlier — wildcards included +- **Holds because:** a registered host is one an operator named in `deploy --host`, so losing it is a visible outage rather than one tenant's page failing. The extra week is the margin that buys. A wildcard hides the fact: `compactionWindowFor` resolves the registered domains a wildcard member covers rather than looking only at literal names, or a `*.example.com` protecting a registered host would compact on the dynamic schedule. +- **Where:** `internal/server/domain_renewal.go#compactionWindowFor` +- **Proven by:** `TestCertRenewer_RegisteredCertCompactsEarlierThanDynamic`, `TestCertRenewer_WildcardCertCoveringRegisteredHostCompactsEarly` +- **Origin:** cubic learning 86e6a9df + +### The renewer's gauges are published only after a completed reconcile pass +- **Holds because:** a mid-loop `ctx.Err()` return means the counts describe part of the estate. A partial count is wrong in a less predictable way than a one-interval-stale one, and the cancellation path only runs at shutdown, when the metrics endpoint is going away anyway. `certificate_renewals_deferred` follows the same rule as every other renewer gauge — consistency here is the point, not a special case. +- **Where:** `internal/server/domain_renewal.go#reconcile`, `#reportMetrics` +- **Proven by:** `TestCertRenewer_ReportsDeferredRenewals` +- **Origin:** cubic learning f54bad2d; PR #105 review thread ("Not changed, deliberately") + +### Renewal timing comes from ARI when the CA offers it, and otherwise from a fraction of the certificate's own lifetime +- **Holds because:** hard-coding "renew at 60 days" breaks the moment a CA changes its lifetime — and Let's Encrypt now issues both 90-day and 6-day certificates. `shouldRenew` asks the obtainer for ARI first (via the optional `renewalInfoGetter` assertion) and falls back to `NotAfter - lifetime/3` — a third of the *observed* lifetime remaining, where the lifetime is `NotAfter - leaf.NotBefore` — plus a per-certificate `certJitter`, so a fleet that booted together does not renew together. +- **Where:** `internal/server/domain_renewal.go#shouldRenew` (211-232), `#reconcile` +- **Proven by:** `TestCertRenewer_HonorsARIWindow`, `TestCertRenewer_RenewsInsideFallbackWindow`, `TestCertRenewer_LeavesFreshCertificatesAlone` +- **Origin:** verified from the code during seeding; no cubic learning + +### A state file written before directories existed loads as the default directory +- **Holds because:** the field is absent, not empty-meaning-something-else, in every pre-feature state file. Decoding it as "" and then treating "" as a mismatch would make every restored certificate reprovision on its first handshake after an upgrade. +- **Where:** `internal/server/san_cert_directories.go` (state decode); `internal/server/san_cert_manager.go#adoptCertificateAt` stamps the directory on the way in +- **Proven by:** `TestSANCertManager_OldStateWithoutDirectoryLoadsAsDefault`, `TestSANCertManager_StateRoundTripsDirectory`, `TestSANCertManager_AdoptCertificateStampsDirectory` +- **Origin:** cubic learning 905438d7 + +## Related + +- `certs-issuance.md` — batching, probes, quarantine +- `../certs/summary.md` — the subsystem as a whole diff --git a/lode/review/certs-issuance.md b/lode/review/certs-issuance.md new file mode 100644 index 0000000..13c48d3 --- /dev/null +++ b/lode/review/certs-issuance.md @@ -0,0 +1,97 @@ +How a handshake turns into an ACME order: the batch, the probes, the token, and +what a failure does to the domains that were in it. + +### A handshake's provisioning single-flight is keyed on the service, never globally +- **Holds because:** batches never span services (the candidate loop in `provisionCertificate` skips any pending domain whose `registeredDomains` owner differs), so a global slot would make one service's handshake block on another service's unrelated order — and then be *satisfied* by it, returning without a certificate for the name that was asked for. The key is `"service:" + owner`; same-service handshakes still coalesce, which is the whole point of the flight. +- **Where:** `internal/server/san_cert_manager.go#provisionCertificate` (525-672) +- **Proven by:** `TestSANCertManager_HandshakeBatchNeverMixesServices`, `TestSANCertManager_HandshakeDoesNotWaitOnAnotherServicesOrder` +- **Origin:** cubic learning b8fc16fa + +### The triggering domain is probed before any lock is taken, and it is never dropped from its own batch +- **Holds because:** a domain that cannot answer a pre-flight probe must cost nothing — not an order, not a lock, not a wait. `preflightTrigger` runs first, outside the single flight. The inverse matters as much: `filterBatchMates` drops unreachable *mates*, but the trigger is the name a real client is handshaking for, so removing it would produce a certificate that does not cover the request that paid for it. +- **Where:** `internal/server/san_cert_manager.go#provisionCertificate`, `#filterBatchMates` +- **Proven by:** `TestBatchGuard_UnreachableTriggerIsRefusedWithoutBurningAnOrder`, `TestBatchGuard_TriggerDomainIsNeverDroppedFromItsOwnBatch`, `TestBatchGuard_ReachableTriggerProvisionsDespiteAStaleHold` +- **Origin:** cubic learnings 94f61938, c9fb7a60 + +### Every non-trigger batch-mate is probed, whatever its certificate history +- **Holds because:** history is not a pass. A host that held a certificate for a year and whose DNS has since moved away will fail its renewal and, riding along in someone else's batch, take that whole order down with it. Each unreachable mate is quarantined after its *first* failed probe, so the cost is paid once per backoff cycle rather than once per handshake. +- **Where:** `internal/server/san_cert_manager.go#filterBatchMates` +- **Proven by:** `TestBatchGuard_ExpiringBatchMateIsProbedAndExcludedWhenUnreachable`, `TestBatchGuard_UnreachableBatchMateIsQuarantinedWithoutBurningAnOrder` +- **Origin:** cubic learning c9fb7a60 + +### A domain that needs no probe is not probed: wildcards, and names in a zone with a DNS-01 provider +- **Holds because:** the probe answers one question — does this name route back to this proxy — and neither of those needs to. A wildcard has no single name to answer on, and a DNS-01 zone is validated at the registrar, not at the listener. Probing them would quarantine names whose issuance never depended on where they point. +- **Where:** `internal/server/san_cert_manager.go#filterBatchMates`, `internal/server/domain_release.go` (`Unprobeable`) +- **Proven by:** `TestBatchGuard_DNSSolvableBatchMateIsNotProbed`, `TestBatchGuard_DNSSolvableTriggerIsNotProbed`, `TestBatchGuard_HTTPOnlyBatchMateIsStillProbedAlongsideADNSZone` +- **Origin:** cubic learning 94f61938 + +### Probes are bounded by a concurrency cap and a client timeout, and deliberately ignore the caller's context +- **Holds because:** `probeDomains` runs at most `maxConcurrentProbes` (16, `internal/server/domain_failure.go:20`) at a time, each bounded by `preflightTimeout` (5s, `internal/server/dynamic_domains.go:29`) on the shared client. Cancelling on the handshake's context would throw away a probe result that the *next* handshake needs, and the bound is already fixed and small. This exemption is scoped to these probes; it is not a licence for unbounded external calls. +- **Where:** `internal/server/domain_failure.go#probeDomains`, used by `#filterBatchMates` and `#identifyFailedDomains` +- **Proven by:** the bounds are constants read by `probeDomains`; no test asserts the concurrency figure directly +- **Origin:** cubic learnings 94f61938, 406abada; PR #97 (mhenrixon: "Left the probes non-ctx-aware deliberately") + +### Quarantined pending domains are skipped before the batch-size limit is applied, not after +- **Holds because:** filtering after the `MaxSANsPerCertificate` (100) cut would let held domains occupy slots they cannot use, so a later batch never refills past them and healthy pending domains wait behind names that are known to fail. +- **Where:** `internal/server/san_cert_manager.go#provisionCertificate`, the inline candidate loop (there is no separate collector function) +- **Proven by:** `TestBatchGuard_QuarantinedDomainsDoNotConsumeBatchSlots`, `TestBatchGuard_QuarantinedBatchMateIsSkippedButStaysPending` +- **Origin:** cubic learning 218bf465 + +### A successful adoption clears the quarantine history of every domain in the batch, not just the trigger +- **Holds because:** the ladder's failure count is what decides the *next* hold's length. Leaving a count behind on a domain that has just been certified means its next unrelated failure starts several steps up the ladder, which is how a transient problem turns into a day-long hold. `clearBatchQuarantine` runs on the whole requested set after `adoptCertificate`. +- **Where:** `internal/server/san_cert_manager.go#provisionCertificate`, `#clearBatchQuarantine` +- **Proven by:** `TestBatchGuard_SuccessfulBatchClearsQuarantineHistory` +- **Origin:** cubic learning a934f6ab + +### Quarantine mutations notify the dynamic-domain save callback, so a hold survives a restart +- **Holds because:** the ladder exists to stop a failing domain looping against a CA's rate limits. A hold held only in memory is lifted by every proxy restart, and a crash-looping proxy would then hammer the CA exactly when it is least able to answer. Both the record and the clear notify. +- **Where:** `internal/server/san_cert_manager.go` (issuance-guard mutations) → `DynamicDomainManager`'s save callback +- **Proven by:** `TestBatchGuard_MutationsNotifyChangeForPersistence`, `TestDomainQuarantine_SnapshotRestore` +- **Origin:** cubic learning dda400b7 + +### An unattributable order failure quarantines the whole batch; a named one quarantines only the culprits +- **Holds because:** when the CA names the identifiers it refused, holding the innocent members would stall tenants for someone else's misconfiguration. When it names nobody, releasing everyone means the same batch re-forms on the next handshake and loops — so the safe direction inverts. A rate limit with no named identifier and no advertised retry time is the one unattributable case that restores everything: the CA has told us nothing to act on. +- **Where:** `internal/server/san_cert_manager.go#attributeBatchFailure`, `internal/server/domain_failure.go#identifyFailedDomains` +- **Proven by:** `TestBatchGuard_QuarantinesCulpritsAndRestoresSurvivorsOnFailure`, `TestBatchGuard_UnattributableFailureRestoresEverythingUnquarantined`, `TestBatchGuard_RateLimitedIdentifierIsHeldAndSurvivorsRestored`, `TestBatchGuard_UnnamedRateLimitWithRetryTimeHoldsWholeBatch`, `TestBatchGuard_UnnamedRateLimitWithoutRetryTimeRestoresEverything` +- **Origin:** cubic learnings 5e9e37e0, 406abada; PR #112 + +### One order never spans two DNS providers, and a wildcard order never leaves its provider's zone +- **Holds because:** DNS-01 is solved by writing a TXT record, and a provider can only write in the zones it holds credentials for. An order mixing zones fails at validation for the members the armed provider cannot answer for, taking the reachable ones with it. `splitByProviderZone` narrows to one partition and returns the deferred members to pending; `orderObtainer` refuses a batch that still spans providers, as a second line. +- **Where:** `internal/server/san_cert_zones.go#splitByProviderZone`, `#wildcardAnchors`, `#outsideWildcardZones` +- **Proven by:** `TestSANCertManager_ProvisionCertificate_NarrowsBatchToOnePartition`, `TestSANCertManager_ObtainCertificate_RefusesOrderSpanningProviders`, `TestSANCertManager_ProvisionCertificate_WildcardOrderExcludesForeignZones`, `TestSANCertManager_SplitByProviderZone_UnsolvableWildcardIsolatedAlone`, `TestSANCertManager_SplitByProviderZone_NestedWildcardZonesLongestWins` +- **Origin:** cubic learnings 90fb8c5f, 9b8e7442; PR #113 + +### A waiter on someone else's provisioning re-checks expiry and directory before it is served anything +- **Holds because:** a waiter wakes when the flight settles, not when it succeeded. Serving whatever certificate is in the map at that moment hands the client an expired leaf, or one from a directory its service has moved away from — and, worse, makes a failed order look locally successful, so nothing retries. `getServableCertForDomain` rejects past `NotAfter`, and a registered domain whose owner's directory still mismatches gets `ErrCertNotFound`. +- **Where:** `internal/server/san_cert_manager.go#getServableCertForDomain` +- **Proven by:** `TestSANCertManager_GetCertificate_WaiterRefusesExpiredCert`, `TestSANCertManager_GetCertificate_WaiterRefusesStillMismatchedCert` +- **Origin:** cubic learnings c3c755d5, 477c65ef + +### A certificate whose dynamic domains were all evicted keeps serving until it expires, and is reusable if they come back +- **Holds because:** eviction from a source is a statement about issuance, not about the bytes already on disk. Dropping the certificate would break live handshakes for a tenant whose source blipped, and re-issuing on recovery would spend an order for a certificate that is still valid. Normal replacement and renewal rules continue to apply to it. +- **Where:** `internal/server/san_cert_manager.go#GetCertificate`, `internal/server/domain_renewal.go#reconcile` +- **Proven by:** `TestSANCertManager_GetCertificate_ServesExistingCertForEvictedDomain`, `TestCertRenewer_KeepsFullyEvictedCertificateUntilExpiry`, `TestCertRenewer_EvictedCertificateSurvivesSourceRecovery`, `TestCertRenewer_RemovesFullyEvictedCertificateAfterExpiry` +- **Origin:** cubic learning 32580bd5 + +### The legacy HTTP-01 cache import runs with no manager or store lock held +- **Holds because:** adoption takes `stateMu` and `mu` itself. Calling it under either from `Initialize` deadlocks the proxy at boot, before any listener is up — the failure mode with the least diagnostic output available. +- **Where:** `internal/server/san_cert_import.go#importLegacyHTTP01Cache`, called from `SANCertManager.Initialize` +- **Proven by:** `TestSANCertManager_InitializeAdoptsLegacyCacheWithoutDeadlock` +- **Origin:** cubic learning d2c76f1e + +### `stateMu` spans both the in-memory publication and the file writes +- **Holds because:** an export or a concurrent persist that interleaves between the two captures a state file naming certificates whose `cert.pem` is not written yet — an archive that restores into a broken estate. `adoptCertificateAt` takes `stateMu` then `mu`, and `removeCertificate` holds the same span. +- **Where:** `internal/server/san_cert_manager.go#adoptCertificateAt`, `#removeCertificate` +- **Proven by:** `TestSANCertManager_ExportStoreHoldsTheDiskLock` +- **Origin:** cubic learning abe274ee + +### Not a bug: a stale `Retry-After` still ends the hold, rather than falling back to the ladder +- **Holds because:** `RecordRateLimited` holds until the advertised time plus `rateLimitHoldMargin` (1 minute, `internal/server/domain_failure.go:89`), so even a timestamp 30 seconds stale ends the hold 60 seconds *after* the CA said to retry — which is the margin's contract. Falling back to the ladder would delay an already-permitted retry by 15 minutes or more. A still-throttling server returns the same now-staler timestamp, `until` is no longer after `now`, and the existing condition drops to the ladder at failures=2; no tight loop is reachable. +- **Where:** `internal/server/domain_quarantine.go#RecordRateLimited` +- **Proven by:** `TestDomainQuarantine_RecordRateLimitedHoldsUntilAdvertisedTime`, `TestDomainQuarantine_RecordRateLimitedFallsBackToLadder` +- **Origin:** PR #112 review thread (rejected after analysis) + +## Related + +- `certs-directories-and-renewal.md` — per-service directories, ARI, renewal partitions +- `../certs/summary.md` — the subsystem as a whole diff --git a/lode/review/dynamic-domains.md b/lode/review/dynamic-domains.md new file mode 100644 index 0000000..5d3fdc1 --- /dev/null +++ b/lode/review/dynamic-domains.md @@ -0,0 +1,81 @@ +Tenant hostnames polled from an application, and what the proxy refuses to do +with them. The recurring theme: a source that is temporarily wrong must not be +able to take live domains off the air. + +### A mass removal is held until three consecutive polls confirm it +- **Holds because:** the source is an application endpoint, and a half-deployed app answering with an empty or truncated list is a routine event. Evicting on it drops every tenant's certificate at once. Removals above `shrinkGuardThreshold` (0.30 of the applied set, `internal/server/dynamic_domains.go:35`) are held until `shrinkGuardConfirmations` (3, line 39) consecutive over-threshold polls. So a single or transient empty poll evicts nothing; three consecutive confirming polls do evict — the guard defers a real shrink, it does not refuse one. +- **Where:** `internal/server/dynamic_domains.go#applyDomains` (429-507) +- **Proven by:** `TestDynamicDomainManager_ShrinkGuardHoldsMassRemovals`, `TestDynamicDomainManager_ShrinkGuardHoldsEmptyPoll`, `TestDynamicDomainManager_ShrinkGuardAppliesSmallRemovals`, `TestDynamicDomainManager_ShrinkGuardConfirmsAfterConsecutivePolls`, `TestDynamicDomainManager_ShrinkGuardCancelsOnRecovery` +- **Origin:** cubic learning 1ec3375d + +### While a shrink is held, both the poller's and the persisted ETag are cleared +- **Holds because:** the confirmation count only advances on a poll that *delivers a body*. With `If-None-Match` still set, an unchanged source answers 304 forever, the count never reaches three, and a genuine removal is held permanently. `SeedETag("")` on the poller plus clearing the persisted copy forces a refetch each interval for as long as the hold lasts. +- **Where:** `internal/server/dynamic_domains.go#applyDomains`; `internal/server/source_poller.go#SeedETag` +- **Proven by:** `TestDynamicDomainManager_ShrinkGuardClearsETagWhileHolding` +- **Origin:** cubic learning fe0938a1 + +### Issuance follows the polled list, not the held union +- **Holds because:** while a removal is in question the applied set is the previous set plus whatever the poll added, so live certificates keep serving. But spending ACME orders on a name the source has stopped reporting is a different decision: the domain keeps its allowlist entry and any certificate it has, and earns no *new* orders until its removal is resolved. +- **Where:** `internal/server/dynamic_domains.go#applyDomains` +- **Proven by:** `TestDynamicDomainManager_ShrinkGuardHoldsMassRemovals` +- **Origin:** cubic learning 1ec3375d + +### `ServiceDeployed` clears the service's shrink-hold confirmation state +- **Holds because:** a redeploy may point the service at a different source. Confirmations counted against the old source's answers say nothing about the new one, and carrying them over lets two polls from a replaced source plus one from its successor evict a set nobody confirmed. Stale callbacks from a *replaced poller* are a separate identity race, closed separately — clearing the hold does not address it. +- **Where:** `internal/server/dynamic_domains.go#ServiceDeployed` +- **Proven by:** `TestDynamicDomainManager_ShrinkGuardResetsOnRedeploy` +- **Origin:** cubic learning d33e8005 + +### A poll for a service the router no longer knows, or whose poller entry is gone, is dropped +- **Holds because:** an in-flight poll outlives the deployment that started it. Applying it re-registers domains for a service that has been removed, and the allowlist entry then outlives everything that could ever clean it up. +- **Where:** `internal/server/dynamic_domains.go#applyDomains` +- **Proven by:** `TestDynamicDomainManager_ServiceRemovedEvictsDomains`, `TestDynamicDomainManager_RedeployWithoutSourceEvictsDomains` +- **Origin:** cubic learning d33e8005 + +### A malformed payload is an error, so the last good set keeps serving +- **Holds because:** the alternative is that a JSON syntax error in the application's response deletes every tenant. `parseDomainList` returns an error for an oversize body (`maxDomainListBody`, 1 MB) or too many entries (`maxDomainListEntries`, 10 000) and for unparseable JSON; the poller then rolls its ETag back so the broken body keeps being refetched rather than 304ing until the content next changes. Individual bad *entries* are different: a wildcard or a name failing the RFC 1123 grammar is skipped with a warning, because one tenant's typo must not cost the rest. +- **Where:** `internal/server/domain_source.go#parseDomainList`, `#validDynamicDomain`; `internal/server/source_poller.go` +- **Proven by:** `TestDynamicDomainManager_LoadStateSkipsNilEntries`, `TestDynamicDomainManager_DeployedServicePollsAndIssues` +- **Origin:** verified from the code during seeding + +### The release prober excludes rate-limit holds and unprobeable domains in `candidates`, not in `probeDomains` +- **Holds because:** `probeDomains` *skips* a domain it cannot speak for and reports no failure for it. A sweep that read "not failed" as "passed" would release every wildcard hold on its first tick — the exact opposite of the quarantine's purpose. Selecting the candidates up front is what makes "no failure reported" mean "the probe succeeded". The prober also records nothing on a failing probe: it only ever lifts holds, never adds to the ladder. +- **Where:** `internal/server/domain_release.go#candidates`, `internal/server/domain_failure.go#probeDomains` +- **Proven by:** `TestDomainQuarantine_ReleaseLiftsTheHoldButKeepsTheLadder`, `TestDomainQuarantine_ReleaseReportsWhetherItHeld` +- **Origin:** verified from the code during seeding + +### `Release` keeps the failure count; `Clear` drops it +- **Holds because:** they answer different questions. A probe that now succeeds says the current hold is wrong, not that the domain has stopped flapping — keeping the count means a domain that keeps failing keeps climbing the ladder. A successful issuance, or the domain leaving its source, really does end the history. +- **Where:** `internal/server/domain_quarantine.go#Release`, `#Clear` +- **Proven by:** `TestDomainQuarantine_ReleaseLiftsTheHoldButKeepsTheLadder`, `TestDomainQuarantine_ExpiresAndClears`, `TestDomainQuarantine_ACMEBackoffProgression`, `TestDomainQuarantine_PreflightBackoffStartsGentler` +- **Origin:** verified from the code during seeding + +### A quarantine record written before `Kind` existed decodes as `quarantineACME` +- **Holds because:** ACME is the conservative default — it is the kind the release prober will *not* lift early. Decoding an unknown record as `preflight` would have the first sweep after an upgrade release holds that were placed for CA rejections. +- **Where:** `internal/server/domain_quarantine.go` +- **Proven by:** `TestDomainQuarantine_LegacyEntryWithoutKindDecodesAsACME`, `TestDomainQuarantine_KindSurvivesSnapshotRestore`, `TestDomainQuarantine_RecordsTheKindOfHold`, `TestDomainQuarantine_KindReflectsTheMostRecentFailure` +- **Origin:** verified from the code during seeding + +### `domains retry` clears rate-limit holds too; the release prober does not +- **Holds because:** the prober is automatic and must never argue with a CA that has dictated an end time. `retry` is an operator typing a command about a situation they have looked at, so it is the blunt escape hatch — it clears every kind. +- **Where:** `internal/server/dynamic_domains.go#Retry`, `internal/cmd/domains.go` +- **Proven by:** `TestDynamicDomainManager_RetryClearsRateLimitHoldsToo`, `TestDynamicDomainManager_RetryClearsAHoldAndResetsTheLadder`, `TestDynamicDomainManager_RetryWithoutADomainClearsEveryHold`, `TestDynamicDomainManager_RetryReportsNothingForAnUnheldDomain` +- **Origin:** verified from the code during seeding + +### The pre-flight probe refuses redirects +- **Holds because:** the probe asks whether `/.kamal-proxy/preflight/<per-boot nonce>` is answered *by this proxy*. Following a redirect lets a host that merely points somewhere that redirects here pass a check about where it points. +- **Where:** `internal/server/dynamic_domains.go#preflightProbe` +- **Proven by:** `TestDynamicDomainManager_PreflightProbeRefusesRedirects`, `TestDynamicDomainManager_PreflightProbe`, `TestDynamicDomainManager_PreflightEndpoint` +- **Origin:** verified from the code during seeding + +### `domains list` builds each service's held-removal lookup once, before the row loop +- **Holds because:** rebuilding the membership set per row is quadratic in a listing whose whole purpose is a tenant estate that can run to thousands of domains. +- **Where:** `internal/cmd/domains.go#domainsListCommand.run` +- **Proven by:** `TestDomainsListCommand_JSONOutputShape`, `TestSummarizeDomains` +- **Origin:** cubic learning 7c1248c1 + +## Related + +- `dynamic-redirects.md` — the sibling subsystem, and the rules they share +- `../dynamic-sources/summary.md` — both subsystems as they stand +- `certs-issuance.md` — what the domain sets feed diff --git a/lode/review/dynamic-redirects.md b/lode/review/dynamic-redirects.md new file mode 100644 index 0000000..3015f87 --- /dev/null +++ b/lode/review/dynamic-redirects.md @@ -0,0 +1,118 @@ +A host-scoped redirect map polled from an application. Same poller as the domain +source, entirely different persisted schema and failure policy — a redirect map +is served on the request path, so correctness rules here are about matching and +about what a bad deployment can do to live traffic. + +### A payload with no `hosts` key at all is an error; an explicit `{"hosts": {}}` clears +- **Holds because:** a half-deployed application answering with the wrong document — an index page, an error body, a different endpoint's JSON — decodes to a zero-value payload, and treating that as "no redirects" wipes every live redirect. Requiring the key present makes clearing an explicit act. A parseable payload whose every entry fails validation is refused for the same reason. +- **Where:** `internal/server/redirect_map.go#parseRedirectPayload`, `internal/server/dynamic_redirects.go#applyPayload` +- **Proven by:** `TestDynamicRedirectManager_KeepsLastGoodOnInvalidPayload`, `TestDynamicRedirectManager_ExplicitEmptyPayloadClearsRedirects`, `TestParseRedirectPayload` +- **Origin:** cubic learning 2802d104 + +### An explicit empty map with an ETag is valid cleared state, preserved across a 304 and a restart +- **Holds because:** "cleared" and "never loaded" look identical in a naive schema, so a restart after a deliberate clear would resurrect the previous map from state — or refetch and clear again, depending on the ETag. The persisted state distinguishes them, and a 304 preserves the cleared map rather than discarding it. +- **Where:** `internal/server/dynamic_redirect_state.go`, `internal/server/dynamic_redirects.go#applyPayload` +- **Proven by:** `TestDynamicRedirectManager_StateSurvivesRestart`, `TestDynamicRedirectManager_ExplicitEmptyPayloadClearsRedirects` +- **Origin:** cubic learning 2802d104 + +### A payload is applied only if the poller that delivered it is still the service's registered poller +- **Holds because:** a redeploy that changes `--redirects-source` leaves the old poller's in-flight request outstanding. Its body arrives after the new source is installed and overwrites it with the previous deployment's map. Each `OnBody` callback is bound to its originating poller and `applyPayload` checks identity before installing anything. +- **Where:** `internal/server/dynamic_redirects.go#applyPayload` +- **Proven by:** `TestDynamicRedirectManager_SupersededPollerCannotApply` +- **Origin:** cubic learning ff82e9f6 + +### A changed source records the new identity and clears its ETag before polling, while the persisted map keeps serving +- **Holds because:** the ETag belongs to the *old* endpoint. Sending it to a new one either gets a meaningless 304 or matches by coincidence, and either way the new source's first answer is never applied. Source identity lives in persisted state precisely so the comparison survives a restart. The old map keeps serving until the first new-source poll lands, so changing a source is not an outage. +- **Where:** `internal/server/dynamic_redirect_state.go`, `internal/server/dynamic_redirects.go` +- **Proven by:** `TestDynamicRedirectManager_SourceChangeDropsETag` +- **Origin:** cubic learnings 007cfef5, 967954fb + +### `compileRedirectMap` walks host keys in sorted order and skips normalized duplicates with a warning +- **Holds because:** Go map iteration is randomized, so `Old.Example.com` and `old.example.com.` colliding after normalization would resolve to a different winner on each proxy in a fleet — and on each restart. Sorting makes the winner deterministic everywhere. Rules are counted only from the retained entry, so the reported rule count matches the map that is actually installed. +- **Where:** `internal/server/redirect_map.go#compileRedirectMap`, `#normalizeRedirectHost` +- **Proven by:** `TestCompileRedirectMap_NormalizationCollisionsAreDeterministic`, `TestCompileRedirectMap_SkipsInvalidEntries` +- **Origin:** cubic learning e95966e2 + +### Request-time lookup applies the same `normalizeRedirectHost` that compilation applies +- **Holds because:** a host arrives from the client with whatever case and trailing dot it likes. Normalizing only at compile time means `Old.Example.COM.` never meets the entry written for it — the rule silently does nothing, which is the failure nobody reports as a bug. +- **Where:** `internal/server/redirect_map.go#normalizeRedirectHost`, used at compile and at lookup +- **Proven by:** `TestDynamicRedirectMap_HostRedirect`, `TestCompileRedirectMap_NormalizationCollisionsAreDeterministic` +- **Origin:** cubic learning 88204e7a + +### Exempt paths are checked before both rule sources +- **Holds because:** `redirectExemptPrefixes` protects `/.well-known/acme-challenge/` (issuance) and `/.kamal-proxy/` (ping, pre-flight, refresh nudges). A tenant redirect rule that shadowed either would stop certificates issuing or make the proxy's own endpoints unreachable on that host, and the tenant who wrote the rule would never see why. The check runs ahead of the dynamic map *and* the static `--redirect` rules; the TLS and canonical-host hop still applies to exempt paths. +- **Where:** `internal/server/service.go#redirectURLIfNeeded` (992-1043); `internal/server/redirect_map.go#isRedirectExemptPath` +- **Proven by:** `TestIsRedirectExemptPath`, `TestRouter_RedirectRulesNeverShadowInternalPaths` +- **Origin:** cubic learning ba5d6cf1 + +### `preserve_path` carries the request's escaped path, not its decoded one +- **Holds because:** `URL.Path` has already decoded `%2F`, so rebuilding the `Location` from it turns an encoded slash into a real path separator and sends the client somewhere else. `RawPath`/`EscapedPath` preserve what the client actually sent. +- **Where:** `internal/server/redirect_map.go` (host redirect construction) +- **Proven by:** `TestDynamicRedirectMap_PreservePathKeepsEncodedSlashes`, `TestDynamicRedirectMap_PathRules`, `TestDynamicRedirectMap_TrailingSlash` +- **Origin:** cubic learning a925a3b5 + +### An absolute `redirect_to` needs a non-empty *hostname*, not merely a non-empty authority +- **Holds because:** `http://:8080/` parses with a non-empty raw authority and an empty host — a `Location` no client can follow. The check is on the parsed hostname. +- **Where:** `internal/server/redirect_map.go#compileHostRedirect` +- **Proven by:** `TestCompileRedirectMap_SkipsInvalidEntries` +- **Origin:** cubic learning 6a3f1a38 + +### The redirect-loop guard compares URLs as a request would +- **Holds because:** a rule sending `/` to `` (or `http://Example.com/` to `http://example.com/`) is a loop the client will follow forever, but a byte comparison calls them different. Empty path and `/` are equal, scheme and host compare case-insensitively, and the query must match too — two URLs differing only in query are genuinely different destinations. +- **Where:** `internal/server/redirect_map.go` (loop guard) +- **Proven by:** `TestDynamicRedirectMap_HostRedirect`, `TestCompileRedirectMap_SkipsInvalidEntries` +- **Origin:** cubic learning efaecd01 + +### Each path pattern is compiled once and the compiled rule retained +- **Holds because:** validating through `newPathRuleSet` and then recompiling for use pays regex compilation twice per rule, on a map that may carry up to `maxRedirectPathRules` (100 000) of them, every poll. The compiled rule is what is kept. +- **Where:** `internal/server/redirect_map.go#compileHostRedirect` +- **Proven by:** `TestDynamicRedirectMap_PathRules`, `TestParseRedirectPayload_RejectsTooManyRules` +- **Origin:** cubic learning 0e0270b3 + +### One tenant's broken regex is skipped, not fatal to the payload +- **Holds because:** the map is multi-tenant. A single bad pattern failing the whole document would let one tenant's typo disable every other tenant's redirects. +- **Where:** `internal/server/redirect_map.go#compileRedirectMap` +- **Proven by:** `TestCompileRedirectMap_SkipsInvalidEntries` +- **Origin:** verified from the code during seeding + +### `Router.DeployService` reconciles redirects immediately after `installLoadBalancer`, before draining old targets +- **Holds because:** the moment the new load balancer is installed, new traffic is being served by the new deployment — and must see the new source's redirects. Waiting until after a drain that can run to the full drain timeout serves the previous deployment's redirects to the new deployment's traffic. The domain manager is reconciled *after* the drain; that is a separate subsystem decision, not an inconsistency. +- **Where:** `internal/server/router.go#DeployService` +- **Proven by:** `TestDynamicRedirectManager_DeployedServicePollsAndApplies`, `TestDynamicRedirectManager_EndToEndThroughRouter`, `TestRouter_DynamicRedirectsAnswerRequests` +- **Origin:** cubic learning e7b34a30 + +### `Service.initialize` stores nil whenever `RedirectsSource` is empty +- **Holds because:** removing the source is an instruction to stop redirecting, and waiting for the manager to catch up leaves the old compiled map serving on the request path in the meantime. Clearing at initialize makes the redeploy itself the moment it stops. +- **Where:** `internal/server/service.go#initialize` +- **Proven by:** `TestService_RedeployWithoutSourceClearsDynamicRedirects`, `TestDynamicRedirectManager_RedeployWithoutSourceEvictsRedirects`, `TestDynamicRedirectManager_ServiceRemovedEvictsRedirects` +- **Origin:** cubic learning 4f83fc43 + +### `PublishMetrics` is called once after server startup, because restored maps are installed before `metrics.Enable()` +- **Holds because:** the gauges are set when a map is installed, and a map restored from state is installed during boot, before the real tracker exists — so the emission goes to the no-op delegate. A proxy serving only persisted redirects would then report no map at all. The counts are cached and republished once `run.go` has the server up. +- **Where:** `internal/server/dynamic_redirects.go#PublishMetrics`, called from `internal/cmd/run.go` +- **Proven by:** `TestDynamicRedirectManager_StateSurvivesRestart` +- **Origin:** cubic learning 3cc25f3d + +### The redirect save path holds a dedicated save lock across snapshot, marshal and rename +- **Holds because:** concurrent polls for different services share one temp file path, so two interleaved saves write each other's bytes. The lock is separate from the map lock so a save never blocks a lookup. +- **Where:** `internal/server/dynamic_redirect_state.go` +- **Proven by:** `TestDynamicRedirectManager_StateSurvivesRestart` +- **Origin:** cubic learning 540e5b33 + +### The two dynamic subsystems stay separate managers with separate schemas +- **Holds because:** they persist different shapes — domains carry quarantine records and issuance state, redirects carry raw host maps and source identity — and they are wired at different points of the deploy flow. A generic persisted-state helper would abstract about forty lines of glue while coupling two subsystems' schemas to each other, and a shared `ServiceLifecycleManager` interface threaded through the router buys less than the two near-identical setters cost. What *is* shared is the part that must not drift: `sourcePoller` and `refreshNudge`. The persistence is not even shared at the primitive level — redirects go through `writeFileAtomic`, domains through their own `os.WriteFile`-to-`.tmp` plus `os.Rename` — so a change to one staging path does not silently change the other's durability. A third manager would tip the balance. +- **Where:** `internal/server/dynamic_domains.go`, `internal/server/dynamic_redirects.go`, `internal/server/router.go#DeployService` +- **Proven by:** structural; no single test +- **Origin:** cubic learnings 1aa22802, 2176ec95, 967954fb; PR #92 review threads (declined with reasoning) + +### Not a bug: a superseded poller's ETag is not rolled back when its service is gone +- **Holds because:** when the resolver no longer knows the service, the poller is orphaned and about to be stopped — its in-memory ETag dies with it, and the persisted ETag lives in `states[service]`, which `ServiceRemoved` has already deleted. Rolling back an ETag on a dying poller has no observable effect. The race that *was* real — a **replaced** poller applying — is closed by the poller-identity check instead. +- **Where:** `internal/server/dynamic_redirects.go#applyPayload`, `#ServiceRemoved` +- **Proven by:** `TestDynamicRedirectManager_ServiceRemovedEvictsRedirects`, `TestDynamicRedirectManager_SupersededPollerCannotApply` +- **Origin:** PR #92 review thread ("Not changed, with reasoning") + +## Related + +- `dynamic-domains.md` — the sibling subsystem +- `sources-and-refresh.md` — the poller and the refresh nudge both serve +- `../dynamic-sources/summary.md` — both subsystems as they stand diff --git a/lode/review/sources-and-refresh.md b/lode/review/sources-and-refresh.md new file mode 100644 index 0000000..a511017 --- /dev/null +++ b/lode/review/sources-and-refresh.md @@ -0,0 +1,78 @@ +The two pieces both dynamic subsystems share: the poller that fetches an +application endpoint, and the endpoint that asks a poller to run now. + +### Path-mode polls use a `Proxy: nil` transport; absolute-URL sources keep the default +- **Holds because:** a path-mode source resolves to one specific container on the internal network, over plain HTTP, carrying a bearer token. An `HTTP_PROXY` variable in the proxy's environment must neither intercept that request nor observe the token on that leg. An absolute URL is a different intent — the operator named an external endpoint, and proxy variables apply to it deliberately. +- **Where:** `internal/server/source_poller.go` +- **Proven by:** `TestDynamicRedirectManager_DeployedServicePollsAndApplies`, `TestDynamicDomainManager_DeployedServicePollsAndIssues` +- **Origin:** cubic learning 35ea345c; PR #92 review thread + +### A path-mode poller with no endpoint resolver fails the poll rather than panicking +- **Holds because:** the poll runs on its own goroutine, so a nil dereference there takes the process down with a stack that names nothing useful about which service was misconfigured. The base URL's trailing slash is trimmed before `Source` is appended, so a resolver answering `http://host/` does not produce `//path`. +- **Where:** `internal/server/source_poller.go#endpoint` (240-257: the nil check and the trim); the resolver itself is each manager's `endpointFor` +- **Proven by:** `TestDynamicRedirectManager_PollFailuresAreCounted` +- **Origin:** cubic learning 0f7fa0f8 + +### The new ETag is stored before `OnBody` runs, and rolled back if `OnBody` errors +- **Holds because:** the parser's consumers read the ETag during the apply, so storing it afterwards hands them the previous one. But a body that fails to parse must keep being fetched — holding the new ETag would 304 every subsequent poll until the *content* next changes, so a broken payload could persist for as long as the application kept serving it unchanged. +- **Where:** `internal/server/source_poller.go` +- **Proven by:** `TestDynamicRedirectManager_KeepsLastGoodOnInvalidPayload`, `TestDynamicRedirectManager_SourceChangeDropsETag` +- **Origin:** verified from the code during seeding + +### Poll failures are counted as `outcome="error"`; a 304 is a healthy poll and is not counted +- **Holds because:** the four ways a poll can fail — an unresolvable endpoint, a transport error, an unexpected status, an unreadable (gzip) body — are all "the source did not answer", and an operator watching the metric needs them together. A 304 *is* the source answering, so counting it as an error would make a correctly-behaving conditional source look permanently broken. +- **Where:** `internal/server/source_poller.go` (`OnPollError`), `internal/metrics/metrics.go` (`dynamic_redirect_polls_total`) +- **Proven by:** `TestDynamicRedirectManager_PollFailuresAreCounted` +- **Origin:** cubic learning 4576a7a6 + +### Polls are scheduled with a ±10% jitter, and `Refresh` collapses onto a single slot +- **Holds because:** a fleet of proxies deployed together would otherwise poll the same application endpoint in lockstep forever. The refresh channel is single-slot so a burst of nudges costs one extra poll, not one per nudge. +- **Where:** `internal/server/source_poller.go#jitteredInterval`, `#Refresh` +- **Proven by:** `TestDynamicDomainManager_RefreshEndpoint`, `TestDynamicRedirectManager_RefreshEndpoint` +- **Origin:** verified from the code during seeding + +### Both refresh endpoints serve through one `refreshNudge`, in a fixed order +- **Holds because:** two copies of an ordering rule is two places for it to drift, and the ordering is the security property. `internal/server/refresh_nudge.go#serve`: + 1. **hidden first, whatever the method** — no token or no sources is a 404 for every method, because a 405 would reveal that the endpoint exists + 2. non-POST → 405 with `Allow: POST` set before the write + 3. bearer token via `tokensEqual` + 4. one nudge per `refreshMinInterval` (10s) → 429 with `Retry-After` + 5. 202 with an empty body +- **Where:** `internal/server/refresh_nudge.go#serve` (35-66) +- **Proven by:** `TestDynamicDomainManager_RefreshEndpoint`, `TestDynamicDomainManager_RefreshEndpointDisabledWithoutToken`, `TestDynamicRedirectManager_RefreshEndpoint`, `TestDynamicRedirectManager_RefreshEndpointHiddenWithoutToken` +- **Origin:** cubic learnings 98ed443a, 6bb13b5e, a549c218 + +### Token comparison is constant time over SHA-256 digests, so length does not leak either +- **Holds because:** `subtle.ConstantTimeCompare` on the raw strings returns early on a length mismatch, which leaks the token's length to a timing attacker. Hashing first makes both operands 32 bytes. +- **Where:** `internal/server/refresh_nudge.go#tokensEqual` (74-78) +- **Proven by:** `TestDynamicDomainManager_RefreshEndpoint`, `TestDynamicRedirectManager_RefreshEndpoint` +- **Origin:** verified from the code during seeding + +### The nudge carries no data — the poll stays the source of truth +- **Holds because:** it means a replayed nudge is harmless, a lost one costs only latency, and it works from any host. Accepting a payload would make the endpoint a second, unauthenticated-in-substance way to set proxy state. +- **Where:** `internal/server/refresh_nudge.go#serve` +- **Proven by:** `TestDynamicDomainManager_RefreshEndpoint`, `TestDynamicRedirectManager_RefreshEndpoint` +- **Origin:** verified from the code during seeding + +### The two tokens have different jobs, and the help text says which is which +- **Holds because:** `KAMAL_PROXY_REDIRECTS_TOKEN` is sent *by* the proxy when it polls the application; `KAMAL_PROXY_REFRESH_TOKEN` is what a caller must present *to* the proxy to nudge it. Both are read from the running proxy's environment, which is the part an operator gets wrong — setting one in the application's environment and wondering why the poll is unauthorized. +- **Where:** `internal/cmd/deploy.go` (`--redirects-source` help), `internal/server/refresh_nudge.go` +- **Proven by:** `TestServiceOptions_ValidateDynamicRedirects` +- **Origin:** cubic learning cc5a91bf + +### An absolute `--redirects-source` is validated at deploy time for scheme and host +- **Holds because:** a typo in a source URL otherwise fails silently, once per interval, on a background goroutine — the deploy succeeds and the feature never works. The parse requires `http` or `https` and a non-empty hostname. +- **Where:** `internal/server/service_options_validation.go#validateDynamicRedirects` (23-46) +- **Proven by:** `TestServiceOptions_ValidateDynamicRedirects` +- **Origin:** cubic learning 85646590 + +### Not a bug: a rejected refresh logs at Warn, and the rate-limit slot is claimed after authentication +- **Holds because:** every request to the path already writes an access-log line through the logging middleware, so the extra Warn does not change attacker-controlled log volume — it is one line on top of one written regardless. Downgrading to Debug would hide the common real cause, a token mismatch between app and proxy, unless `--debug` is on. And claiming the rate-limit slot *before* authenticating would let an unauthenticated client exhaust the one refresh per 10s that legitimate callers need. +- **Where:** `internal/server/refresh_nudge.go#serve` +- **Proven by:** `TestDynamicDomainManager_RefreshEndpoint`, `TestDynamicRedirectManager_RefreshEndpoint` +- **Origin:** PR #92 review thread ("Not changed, with reasoning") + +## Related + +- `dynamic-domains.md`, `dynamic-redirects.md` — the two subsystems +- `../dynamic-sources/summary.md` — both, as they stand diff --git a/lode/summary.md b/lode/summary.md new file mode 100644 index 0000000..185d3b4 --- /dev/null +++ b/lode/summary.md @@ -0,0 +1,32 @@ +# dash-proxy + +An HTTP/HTTPS reverse proxy for zero-downtime container deploys, published as +`ghcr.io/zoolutions/dash-proxy` and driven by the `dash` gem. It began as a fork +of basecamp/kamal-proxy; since the 2026-08 clean break there is no upstream +remote and `main` is the only long-lived branch, so upstream code arrives only +by deliberate cherry-pick. What the fork adds over kamal-proxy is mostly +certificate work — SAN batching, wildcard certs over DNS-01, per-service ACME +directories, a quarantine ladder, and an exportable certificate store — plus a +response cache, dynamic domain and redirect sources, scale-to-zero, and a large +set of per-service request-path options. + +The process is two halves that meet at `Router`. A **request path** (listener → +middleware chain → `Router` → `Service` → `LoadBalancer` → `Target`) does no +disk or registry I/O beyond what a cache store and a certificate lookup need, +and a **command path** (`bin/dash-proxy <verb>` → `net/rpc` over a unix socket → +`CommandHandler` → `Router`) mutates it. Everything an operator configures +arrives over that socket as a `DeployArgs`, is validated once in +`ServiceOptions.Validate` / `TargetOptions.Validate`, and is persisted to +`dash-proxy.state` so a restart restores exactly what was serving. + +Three invariants govern changes. **The Go module path +(`github.com/basecamp/kamal-proxy`) and the `net/rpc` service name +(`"kamal-proxy"`) do not move** — 16 client call sites and the deployed gem +depend on the latter (`internal/server/commands.go#Start`). **No name provisions +a certificate unless something asked for it** — `Router.GetCertificate` refuses a +server name that routes nowhere, and `SANCertManager.GetCertificate` gates +issuance on the registered/dynamic allowlist. **Every persisted file is written +through a temp file and a rename, and read forgivingly** — a `.bak` fallback for +the routing table, and every added JSON field optional so an older state file +still restores. The staging helper differs per subsystem and the difference is +deliberate (`practices.md`). diff --git a/lode/terminology.md b/lode/terminology.md new file mode 100644 index 0000000..c91e6b5 --- /dev/null +++ b/lode/terminology.md @@ -0,0 +1,87 @@ +# Terminology + +The words this repository uses, and what they mean in its code. + +## Routing + +- **service** — one deployed application, named on the CLI (`deploy <service>`). + A `*Service` holds its `ServiceOptions`, `TargetOptions`, up to two load + balancers, and its per-service middleware chain. +- **target** — one backend address the proxy forwards to (`host[:port]`). A + `*Target` owns a health check and one `httputil.ReverseProxy` handler per + response timeout in play. +- **write target / read target** — `--target` vs `--read-target`. Reads + (GET/HEAD) rotate over readers unless the client holds a fresh writer-affinity + cookie; everything else goes to writers. +- **slot** — `TargetSlotActive` or `TargetSlotRollout`: the two load balancers a + service may hold at once (`internal/server/service.go`). +- **rollout** — a second target set plus a `RolloutController` that sends a + percentage (or an allowlist) of requests to it. +- **weight** — `--target 'host;weight=n'`. Weighted pools rotate by nginx's + smooth weighted round-robin (`internal/server/target_weight.go#nextWeighted`). +- **spec vs name** — a target's *name* is its address; its *spec* is the address + plus a non-default weight. Persisted state stores specs, listings show names. +- **path prefix** — `--path-prefix`. With `--strip-path-prefix` (default true) + the matched prefix is removed before forwarding; `RoutedTargetPath` is what + everything downstream matches on. + +## Certificates + +- **SAN certificate manager** — `SANCertManager`, the proxy's only ACME system + when `--acme-email` is set. One account per ACME directory, one cache, one + allowlist, HTTP-01 and DNS-01. +- **registered domain** — a host named by `deploy --host` on a TLS service. It + may provision synchronously on a handshake. +- **dynamic domain** — a host learned from a service's `--tls-domains-source` + poll. It is queued for asynchronous issuance, never issued on the handshake. +- **batch** — the set of domains put into one ACME order. Bounded by + `MaxSANsPerCertificate` (100) for handshake batches and by + `--tls-domains-batch-size` (max `MaxTLSDomainsBatchSize`, 25) for dynamic ones. +- **batch-mate** — a pending domain that joins another domain's order because it + shares that domain's service and ACME directory. +- **pre-flight probe** — an HTTP request the proxy makes to a domain for + `/.kamal-proxy/preflight/<per-boot nonce>`, proving the name routes back here + before an order is spent (`DynamicDomainManager#preflightProbe`). +- **quarantine** — a per-domain hold with an escalating backoff ladder, kinded + `preflight`, `acme` or `rate_limited` (`internal/server/domain_quarantine.go`). + Only the first two are lifted early by the release prober. +- **release prober** — the sweep that re-probes held domains and lifts holds the + probe contradicts, so a DNS cutover costs a probe interval, not a ladder step. +- **directory** — an ACME directory URL. Run-level (`--acme-directory`) or + per-service (`deploy --tls-staging`); a certificate records the one that + issued it, and a service moving between them forces a replacement. +- **ARI `replaces`** — the RFC 9773 renewal-information marker. It rides at most + one renewal order, and only one issued at the certificate's recorded directory. +- **compaction window** — how close to expiry a renewal may keep deferring while + members are held. Deploy-registered coverage compacts a week earlier. +- **shrink guard** — the rule that a poll removing more than 30% of a service's + applied domains is held until three consecutive polls confirm it. + +## Cache + +- **store** — where cached responses live: `memory`, `file://<dir>`, or a + `redis://`/`rediss://` URL shared by a fleet (`NewCacheStore`). +- **lease** — a shared store's claim that one proxy is fetching a key, so a + lifetime rolling over costs the origin one request for the fleet. +- **inflight group** — the in-process single flight. The lease is the same idea + one level up, and only a node's inflight leader ever touches it. +- **variant index** — a record at a resource's own key naming the fields it + varies on; the response itself lives at the key those values produce. +- **refusal** — a low-cardinality reason a response was not stored, surfaced as + `cache_refusals_total{reason=…}` and as operator advice. + +## Process and state + +- **data directory** — `--data-dir`, else `$HOME/.config/dash-proxy`. Holds + `dash-proxy.state`, `acme.state`, `dynamic-domains.state`, + `dynamic-redirects.state` and `certs/`. +- **drain** — close the public listeners, finish in-flight requests up to a + timeout, save state, exit. `SIGTERM` and `dash-proxy drain` converge on it. +- **hold** (the command) — `dash-proxy hold`, a hidden command that blocks on a + signal so a container can own a shared network namespace. Unrelated to a + quarantine hold. +- **idle controller** — the scale-to-zero state machine per service: active → + sleeping → waking, stopping containers after `--sleep-after` and starting them + on the next request. +- **refresh nudge** — an authenticated `POST` that makes a poller re-poll now. + The poll stays the source of truth; the nudge only removes latency. diff --git a/lode/testing-and-ci/summary.md b/lode/testing-and-ci/summary.md new file mode 100644 index 0000000..4e4da73 --- /dev/null +++ b/lode/testing-and-ci/summary.md @@ -0,0 +1,124 @@ +# Testing and CI + +The suite is pure Go unit tests. Nothing in `make test` needs Docker, a network +call, a real ACME directory or a real RPC socket, which is why the whole thing +runs in one CI job on one runner. + +## Shape of the suite + +| Package | Test files | `Test*` functions | +|---|---|---| +| `internal/server` | 98 | 977 | +| `internal/cmd` | 11 | 64 | +| `internal/server/acme/providers` | 3 | 28 | +| `internal/server/acme` | 2 | 6 | +| `internal/metrics` | 1 | 5 | +| **total** | **115** | **1080** | + +Plus 12 `Benchmark*` functions, run by `make bench` and by nothing else +(`-run=^#` excludes the tests). + +The convention is `TestSubject_WhatItDoes`, and the names carry the +specification: `TestBatchGuard_TriggerDomainIsNeverDroppedFromItsOwnBatch`, +`TestCertRenewer_ARIMarkerSurvivesAFailedFirstPartition`, +`TestCappedReader_ExactlyAtTheLimitIsNotOversized`. A rule in `../review/` cites +these names because the name is the claim. + +## Helpers + +`internal/server/testing.go` holds the shared fixtures, and a new test reuses +them rather than hand-rolling a server: + +- `testBackend(t, body, status)` / `testBackendWithHandler(t, handler)` — an + `httptest.Server` registered with `t.Cleanup` +- `testCountingBackend(t, handler)` — a backend counting accepted connections, + the only way to observe transport connection reuse from outside. It cannot + reuse `testBackendWithHandler`, because `httptest.NewServer` starts listening + before there is a seam to wrap the listener in. +- `testTarget` / `testReadOnlyTarget` / `testTargetWithOptions` +- `testConfig(t)` — ephemeral ports, `t.TempDir()` data dir, the run command's + listener timeout defaults +- `testServer(t, http3Enabled)` / `testServerWithConfig` +- `testRoutedHandler(t, router)` — wraps a router the way `Server.buildHandler` + does, so proxy-generated statuses render through the default error pages + rather than falling back to `http.Error`. A bare router has no error-page + middleware, so a test asserting on a body needs this. +- the `default*Options` values (`defaultServiceOptions`, `defaultTargetOptions`, + `defaultHealthCheckConfig`, `defaultDeploymentOptions`, `defaultEmptyReaders`) + +`testRouter(t)` lives in `router_test.go` and +`testLoadBalancerWithHandlers(t, …)` in `load_balancer_test.go`. + +`testify/require` for setup that must stop the test (`require.NoError` before +dereferencing), `testify/assert` for the assertions themselves. Table-driven +subtests are the house style for anything with more than two cases. + +A stub handler that runs on an `httptest` goroutine reports with `t.Error`, not +`require` — `require` calls `t.FailNow`, which is only legal on the test's own +goroutine, and there it aborts the handler without failing the test. + +## What is never done in a test + +- No real ACME or Let's Encrypt calls. `SANCertManagerConfig.Directory` defaults + to production, so a test passes `LetsEncryptStaging` or a local stub directory. +- No real DNS-01 provider calls — the `acme/providers` tests exercise factory + and config wiring, not live provider APIs. +- No Docker, no real RPC socket, no filesystem outside `t.TempDir()`. +- `internal/cmd` tests exercise flag and argument validation (`preRun`) + directly rather than starting an RPC server. + +## CI + +`.github/workflows/ci.yml` — on push to `main` and on pull requests into it. +Two jobs, both `permissions: contents: read`, both checking out with +`persist-credentials: false`: + +| Job | Steps | +|---|---| +| `lint-actions` | `rhysd/actionlint`, then `zizmorcore/zizmor-action` (`advanced-security: false`) | +| `build` | set up Go from `go-version-file: go.mod`, install golangci-lint **v2.11.3**, `go mod download`, `make build`, `make test`, `make lint` | + +Every `uses:` is pinned to a commit SHA with the version in a trailing comment. +There is no matrix: one Go version, taken from `go.mod` (currently `go 1.26.7`). + +Nothing measures or gates coverage — `make test` passes no `-cover`. The +coverage thresholds in `../../.claude/rules/testing.md` are a convention, not an +enforced check. + +`.golangci.yml` is deliberately small: format with `gofmt`, and `errcheck` +disabled. `make lint` is `golangci-lint run`. + +`.github/workflows/security.yml` — `govulncheck` on a cron (`17 6 * * *`) and on +demand, never on a pull request: advisories are published against code that has +not changed, so gating PRs on it would turn every newly published CVE into a red +build on unrelated work. It scans each released target (`linux/amd64`, +`linux/arm64`) under `CGO_ENABLED=0`, because govulncheck's source mode only +analyses the files the current build configuration selects. It reads and writes a +per-ref tracking issue, which is why `concurrency` is keyed by ref with +`cancel-in-progress: false`. + +`.github/workflows/docker-publish.yml` — on a tag matching +`v[0-9]+.[0-9]+.[0-9]+` or `v[0-9]+.[0-9]+.[0-9]+.[0-9]+`, or on +`workflow_dispatch` with a `tagInput`. Buildx, QEMU, multi-arch +`linux/amd64,linux/arm64`, pushed to `ghcr.io/zoolutions/dash-proxy:<tag>` and +`:latest` with `org.opencontainers.image.title=dash-proxy`. The tag filter and +`script/release-dash`'s grammar must stay in step: a tag the filter does not +match pushes silently and never builds an image. + +Dependabot runs weekly for `github-actions` and `gomod`, each grouped into one +PR, with a cooldown (7 days default; gomod 7/3/2 by semver level). + +## Releasing + +`script/release-dash vX.Y.Z.N` validates the grammar +(`^v[0-9]+\.[0-9]+\.[0-9]+(\.[0-9]+)?$`), refuses a tag that already exists, runs +`make test`, creates an annotated tag and pushes that single tag. CI publishes. + +`script/release` is upstream's and is **not** this repo's release path — it +`docker login`s and pushes `basecamp/kamal-proxy` images itself. See the seeding +discrepancies. + +## Related + +- `../../.claude/rules/testing.md` — the TDD workflow and the coverage convention +- `../cli-and-rpc/summary.md` — what the `internal/cmd` tests exercise diff --git a/lode/workflow.md b/lode/workflow.md new file mode 100644 index 0000000..1ea45c3 --- /dev/null +++ b/lode/workflow.md @@ -0,0 +1,219 @@ +# Workflow profile + +Everything the shared workflow skills (`/lode:lfg`, `/lode:review-pr`, +`/lode:finish-prs`, `/lode:debug-flaky`, `/lode:tdd`, `/lode:plan`) need to know +about dash-proxy that is not already in `../CLAUDE.md`, `../.claude/rules/` or +the rest of `lode/`. + +## Commands + +| Purpose | Command | Notes | +|---|---|---| +| fast loop (one package) | `go test ./internal/server/` (or `-run TestName`) | no network, no Docker; safe anywhere | +| full suite | `make test` (`go test ./...`) | no network, no Docker, no services. Safe in two worktrees at once: every test uses ephemeral ports and `t.TempDir()`. | +| vet | `go vet ./...` | not run by CI — `make lint` covers it via golangci-lint | +| lint | `make lint` (`golangci-lint run`) | `.golangci.yml`: `gofmt` formatter, `errcheck` disabled | +| formatting check | `gofmt -l internal/ cmd/` | must print nothing; CI fails on it through `make lint` | +| one CI cell locally | `make build && make test && make lint` | there is no matrix — one Go version, from `go.mod` | +| benchmarks | `make bench` (`go test -bench=. -benchmem -run=^# ./...`) | 12 `Benchmark*` functions | +| docs build / check | n/a | no docs site; `README.md` carries the reference | +| regenerate the provider table | `go generate ./internal/server/acme/providers` | rewrites README between the `dns-provider-table` markers | +| run the app | `make build && ./bin/dash-proxy run` | `./bin/dash-proxy -h` prints `kamal-proxy` — the binary moved, `rootCmd.Use` did not | +| image smoke test | `make docker && docker run --rm dash-proxy dash-proxy -h` | needs Docker; not part of `make test` | + +**Version pin that matters:** CI installs golangci-lint **v2.11.3** +(`.github/workflows/ci.yml`). A newer local binary reports findings CI will not, +and misses none it will. Install the pinned one before trusting a clean run: +`go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.3`. + +## Branches and PRs + +- Default branch: `main` — the only long-lived branch since the 2026-08 clean + break. There is no `upstream` remote and no `dash` branch; `origin` is + `zoolutions/dash-proxy`. +- Work branches: `feature/*`, `fix/*`, `refactor/*`, `ci/*`, `chore/*`, rooted + off fresh `origin/main`. +- Commits: conventional (`feat:`, `fix:`, `refactor:`, `perf:`, `docs:`, + `test:`, `chore:`, `ci:`), scoped to the area (`san-cert`, `wildcard-certs`, + `router`, `rpc`, `cache`, `acme`, `health-check`, `cmd`, `deps`, `metrics`). + The body says **why**. +- PR body sections, in order: Summary, Test plan, Deviations & judgment calls, + Gate. +- Merge policy: squash on `main` after green and approval — the ruleset requires + linear history. Never force-push a published branch; merge `main` forward + into it instead. +- Attribution: **no** `Co-Authored-By: Claude` and no "Generated with" line. + Commit messages end with the session's `Claude-Session:` line when one is + supplied; PR bodies end with the session URL. + +Dormant branches still on `origin`: `san-certificate-batching`, +`wildcard-certs` (merged history), `feat/loadbalancing` (superseded), and +several merged `feature/cache-*` branches. Do not branch from any of them. + +## Layers + +| Layer | Files | Edit rule | +|---|---|---| +| entry point | `cmd/dash-proxy/main.go` | owned here; a thin `cmd.Execute()` | +| CLI + RPC client | `internal/cmd/` | owned here. Flags are registered once per command — pflag **panics** on a duplicate registration, which is how a union-shaped merge conflict becomes a boot crash. | +| server, routing, certs, cache | `internal/server/` | owned here. The largest package by far; new behaviour goes in a new file next to its siblings, not into `service.go` or `san_cert_manager.go`. | +| ACME provider registry | `internal/server/acme/`, `internal/server/acme/providers/` | owned here, but `registry.go` is the single source — `NewProvider`, `detectionOrder`, the flag help and the README table all derive from it | +| generated | the README's `dns-provider-table` block | edit `registry.go`, then `go generate ./internal/server/acme/providers`; `TestREADMEProviderTable_MatchesRegistry` fails the build on drift | +| metrics | `internal/metrics/` | owned here | +| error pages | `internal/pages/` | embedded HTML for 404/413/502/503/504 | +| frozen names | the module path `github.com/basecamp/kamal-proxy`, the RPC service name `kamal-proxy`, the `/.kamal-proxy/` internal path prefix | **never rename.** 16 client call sites and the deployed gem depend on the RPC name; the path prefix is a deployed contract with the gem. | +| upstream's leftovers | `script/release` | prefer leaving it as basecamp wrote it; the fork's release path is `script/release-dash` | + +## Shapes + +Check a change against these before calling it done; a reviewer will name the +one you forgot. + +- **A restored state file written by an older binary** — an added option is + `omitempty`/`omitzero` with a zero value meaning "off", or the restore changes + behaviour for services that predate it. `Service.UnmarshalJSON` still maps four + pre-plural legacy keys. +- **A `Config` built directly, not by cobra** — tests, restored state, rollouts + and older RPC clients all bypass the CLI, so defaulting belongs server-side. +- **A gob wire type during a proxy replacement** — CLI and server can be + different versions; `ServiceDescription` and the `*Args` types are append-only. +- **A registered host versus a dynamic (source-learned) one** — they differ on + synchronous issuance, the compaction window, and probe eligibility. +- **A wildcard identifier** — has no single name to answer on, no service of its + own, and needs DNS-01. Every domain-resolution path needs a wildcard branch. +- **A domain in a DNS-01 zone versus an HTTP-01 one** — probe eligibility and + order partitioning both turn on it. +- **A second ACME directory** (`--tls-staging`) — a second account, a second key + file, a second rate-limit bucket, and a certificate that records which one + issued it. +- **An empty, truncated or malformed poll body** — a payload-level error keeps + the last good set; an entry-level one skips that entry. +- **A varying response, and a non-varying one** — the cache stores them as two + record shapes. +- **A shared store versus a local one** — only the shared one implements + `CacheLeaser`; every leaser call site needs the nil check. +- **The zero `netip.Addr`** — allow-list, deny-list and rate-limiter each answer + differently, on purpose. +- **`--proxy-protocol` and a trusted-proxy chain** — `r.RemoteAddr` is only as + trustworthy as the peers allowed to assert it. +- **Both TLS listeners plus HTTP/3** — HTTP/3 is pinned to TLS 1.3, so + `--min-tls` neither lowers nor raises it. +- **Go 1.26 (the `go.mod` floor)** — CI takes its toolchain from `go.mod`, not + from your laptop. + +## Constraints + +Reviewer suggestions that are wrong in this repository. + +| Suggestion | Why it is wrong here | +|---|---| +| "Rename the module / RPC service / socket to dash-proxy" | The binary, user, data directory, socket file and image label already moved (stage 3c). The **module path** and the **`net/rpc` service name** are deliberately deferred, each with its own follow-up issue; renaming them breaks every deployed gem mid-upgrade for no gain. | +| "Rename `/.kamal-proxy/`" | It is a deployed contract with the gem; the prefix kept its spelling through the rename on purpose. | +| "Make the pre-flight probes honour the caller's context" | Each is already bounded by a 5s client timeout and a 16-way concurrency cap, and a completed probe benefits the next handshake even when this one gave up. Reviewed and declined (PR #97). | +| "Publish partial renewer gauges on cancellation" | Every renewer gauge updates only after a completed reconcile; a half-loop count is wrong in a less predictable way, and the path only runs at shutdown. Declined (PR #105). | +| "Fall back to the quarantine ladder when `Retry-After` is stale" | The margin already guarantees the retry lands at least a minute after the advertised time; the ladder would delay a permitted retry by 15+ minutes. Declined (PR #112). | +| "Enumerate Cloudflare's mixed-namespace credential pairs" | That gap fails **closed** with an actionable error. Adding them would double the registry and document a hygiene nobody should adopt. Declined (PR #116). | +| "Extract a shared persisted-state helper for the two dynamic managers" | Different schemas; the shared part is already shared (`writeFileAtomic`, `sourcePoller`, `refreshNudge`). Declined (PR #92). | +| "Remove the duplicated config parsing in `preRun`" | Deliberate: `preRun` fails fast before an ACME account is registered, and the use-site parse protects a `Config` built without the CLI. | +| "Add `-cover` gating to CI" | Nothing measures coverage today; the thresholds in `.claude/rules/testing.md` are a convention. Adding a gate is a decision, not a fix. | +| "Use `require` in an `httptest` handler" | `t.FailNow` is illegal off the test goroutine — it aborts the handler without failing the test. Use `t.Error`. | +| "Tag a release candidate `v1.0.0-rc1`" | The gem compares the image tag with `Gem::Version`, which sorts a hyphen suffix *below* the release it names. | + +## Docs + +- User-facing docs: `README.md` (68 KB, the whole reference — there is no docs + site). A new flag or command is documented there in the same PR. +- The DNS-provider table inside `README.md` is **generated** between + `<!-- BEGIN GENERATED: dns-provider-table … -->` and its `END` marker; edit + `internal/server/acme/providers/registry.go` and run + `go generate ./internal/server/acme/providers`. +- `ROADMAP.md` — proxy-side roadmap with code anchors. +- Changelog: **none**. There is no `CHANGELOG.md`; the GitHub release and the + commit history are the record. +- A change to a flag, a command, an output line or a provider always updates + `README.md` in the same PR. +- Files that pin a version and drift after a release: none in this repo. The + `dash` gem's `MINIMUM_VERSION` must name an already-published tag, so the + proxy image releases **first** and the gem second. + +## CI + +- Workflows: + - `ci.yml` — on push to `main` and PRs into it. Jobs: `lint-actions` + (actionlint + zizmor) and `build` (`make build`, `make test`, `make lint`). + - `security.yml` — `govulncheck` on cron `17 6 * * *` and `workflow_dispatch` + only, never on a PR. Scans `linux/amd64` and `linux/arm64` under + `CGO_ENABLED=0`; writes a per-ref tracking issue. + - `docker-publish.yml` — on `v[0-9]+.[0-9]+.[0-9]+` or + `v[0-9]+.[0-9]+.[0-9]+.[0-9]+` tags, or `workflow_dispatch` with `tagInput`. + Multi-arch buildx to `ghcr.io/zoolutions/dash-proxy`. +- Matrix: **none.** One Go version, from `go-version-file: go.mod`. Cells that + differ from local: the Go toolchain (`go.mod` says 1.26.7) and golangci-lint + (pinned v2.11.3). +- Fetch a failure: `gh run view --job <id> --log-failed`, or + `gh pr checks <n>` to find the job. +- "Green" means both `ci.yml` jobs pass. `security.yml` never runs on a PR, so + it is never part of a PR's green. +- Known not-this-branch failures: none standing. A Dependabot `gomod` PR can + turn `make lint` red on a transitive API change — that is the PR's own + problem, not the branch's. +- Shared or rate-limited services the checks hit: **none.** No live ACME, no + registry, no Docker in `make test`, so PRs may run in parallel freely. + +## Flake sources + +- **Wall-clock and backoff timing.** The health-check backoff + (`initialHealthCheckDelay` 50ms → `maxPreHealthyDelay` 2s), the quarantine + ladder, `refreshMinInterval` (10s) and the renewal jitter all read real time. + Tests that assert on a deadline rather than injecting a clock are the first + suspects. +- **Goroutine lifetime past the test.** Pollers, health checks, the release + prober and the renewer all run on their own goroutines; background work + outliving its test is exactly why `metrics.Tracker` holds its delegate behind + an `atomic.Pointer`. +- **Process-wide singletons.** `metrics.enableOnce`, the Prometheus default + registry (which panics on re-registration) and `rpc.RegisterName`'s + `sync.Once` all mean `go test -count=2` exercises a different path than + `-count=1`. +- **Map iteration order.** Go randomizes it, so anything that must be + deterministic sorts first — `compileRedirectMap` walks host keys sorted for + exactly this reason. +- **`httptest` port reuse** under heavy parallelism; `testConfig` binds port 0 + to avoid it. + +Not a flake source: live CDNs or registries. Nothing in `make test` leaves the +machine. + +## Conflicts + +| File | Rule | +|---|---| +| `go.sum` | never hand-merge: resolve `go.mod`, then `go mod tidy` | +| `go.mod` | take `main`'s toolchain and dependency graph, keep `go-acme/lego/v4`, then `go mod tidy` | +| `internal/cmd/run.go` | union of the flag registrations, but each flag registered **exactly once** — pflag panics on a duplicate, so a naive union boots and crashes | +| `internal/server/config.go`, `router.go` | union of both sides' fields and methods; neither side's are optional | +| `internal/server/service.go` | read both sides — one usually carries a behaviour change and the other feature wiring; preserve both | +| `README.md`'s generated table | do not merge it: take either side, then `go generate ./internal/server/acme/providers` | +| `Dockerfile`, `Makefile`, `script/release` | prefer upstream's shape; the fork's release path is `script/release-dash` | +| test fixtures | add a second fixture rather than merging two shapes into one | + +`git rerere` is **not** enabled in this checkout. If you turn it on, always +`git diff --staged` before trusting a replayed resolution — one recorded in a +different context can be wrong. + +## Verification + +- The manual check a user of this change would do: `make build`, then + `./bin/dash-proxy run --http-port 8080 …` in one shell and + `./bin/dash-proxy deploy …` in another, and look at the access log and + `curl localhost:<metrics-port>/metrics`. For a flag, `./bin/dash-proxy <cmd> -h` + is the surface the user reads first. +- A hot-path change (`Router.ServeHTTP`, `Target.ServeHTTP`, `LoadBalancer` + selection, `SANCertManager.GetCertificate`) carries a `make bench` + before/after on the same machine, reporting ns/op **and** allocs/op — see + `../.claude/rules/performance.md`. +- Stress iterations for a flake proof: `go test -race -count=50 -run <TestName> + ./internal/server/` — `-race` as well as `-count`, because most of this + repo's non-determinism is concurrency rather than ordering. +- Where evidence goes: `lode/tmp/` (git-ignored), unless the PR needs an + auditable trail.