Home-Network/home-network-k3s-impl
2
0 Fork 0
Code Issues 2 Pull Requests 5 Actions Packages Projects Releases Wiki Activity

chore(decisions): record the CI reading, and why the metrics policy is NOT here
gate / gate (pull_request) Failing after 1m43s
Details
gate / conformance (pull_request) Has been skipped
Details

Browse Source 
Follow-up to the branch's one substantive commit; no code change.

CI on 99560c2d, read from the commit's /statuses rather than from a tick:
  gate / gate (pull_request)         state=failure   desc='Failing after 1m44s'
  gate / conformance (pull_request)  state=pending   desc='Blocked by required conditions'
The second line is PENDING, not the `success` + "Has been skipped" trap -- the
conformance job is gated on the gate job, which failed, so it never ran.

MY LOCAL `approval-policy: green` WAS MEANINGLESS AND I NEARLY REPORTED IT AS
EVIDENCE. ci/approval-policy.sh:47 reads GITHUB_EVENT_NAME, unset outside CI, so
it short-circuits to "N/A": a clean local gate STRUCTURALLY cannot catch a
sensitive-path problem. Verified by re-running with
GITHUB_EVENT_NAME=pull_request PR_NUMBER=34, where the stage evaluates and fails
CLOSED on the missing token while every other stage stays green -- which bounds
the CI failure to that one stage. I could NOT read CI's job log to confirm it
from CI's own output: this Gitea answers 404 on /actions/runs and on
/actions/jobs/{task}/logs, so only /actions/tasks is available. Attribution
therefore rests on the local reproduction plus the sensitive-path rule, not on
CI's log -- weaker evidence than PR #28 had, named rather than papered over.

Branch protection and the runner re-measured against the live API (CLAUDE.md
2b), not inherited from the CLAUDE.md table: protected=True,
user_can_push=False, enable_status_check=True, contexts=['gate / gate
(pull_request)'], required_approvals=1; runner id=1 incluster-dind online. So a
human is required on every PR here regardless of path. I did NOT provoke a
rejected push to main, so user_can_push is the API's word only.

DECLINED, with reasons: bundling Ines Dubois's metrics-ingress NetworkPolicy.
The coordinator asked for my judgement and the answer is to queue it as the next
PR, not to drop it. Principally because it is a MODEL question, not a policy to
bolt on: schema 1.6.0's observability.metricsEndpoint already renders the
monitoring-namespace ingress allow, and what the model cannot express is a
SECOND metrics endpoint belonging to a sidecar the RENDERER injected. Keying a
bespoke policy off the pod label auth-mode=forward-auth hardcodes "forward-auth
implies 44180" instead of deriving it -- the "list, don't derive" anti-pattern
oauth2-proxy-cookie-names-distinct assertion (5) forbids, one directory from the
check that forbids it. Also: it is a trust-boundary ingress allow and a
different requirement set (R-NET-01/02, R-OBS-01 vs R-IDP-02), so it should not
be reviewed in the shadow of an --api-route headline; the conflict argument does
not apply to my diff, which regenerates TWO goldens not five, and a
CiliumNetworkPolicy is a different document in files git merges independently;
every fact in the finding is hers and I have measured none of it; and the
PodMonitor it serves is in HER PR #29, so shipping the policy alone would open
an ingress allow for a scraper that selects nothing.

Also records that the sidecars' metrics remain UNSCRAPABLE and that this branch
does not change that.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
jakman85andClaude Opus 5 committed 2026-10-01 22:30:01 -05:00
1 parent 99560c2d22
commit f1d16f44b8
1 file changed
+141 -3
Download Patch File Download Diff File Expand all files Collapse all files
decisions/2026-10-01-oauth2-proxy-api-route-per-app.md
+141 -3
View File
@@ -83,6 +83,57 @@ Note the consequence of his stage change for anyone reading this later: once PR
in CI the way rc 3 is** — a selftest failure will block. Which is the right outcome, and it is why
a selftest that prints nothing was worth fixing before that lands rather than after.
### Declined, with reasons: the metrics-ingress NetworkPolicy
The coordinator asked for my judgement on bundling Ines Dubois's finding — that #28's metrics
listener genuinely serves (she measured `http=200`, 10,208 bytes,
`oauth2_proxy_requests_total{code="302"} 2` via port-forward) but the scrape will be **dropped**,
because namespace `jackett`'s default-deny has **no `ingress` key at all** and
`jackett-allow-gateway-ingress` admits `fromEntities: [ingress]` only. The fix belongs in each
app's own rendered policy, i.e. in my file and my goldens. **I declined to bundle it**, and asked
for it as the immediate next PR. Five reasons, in the order that decided it:
1. **The real reason is that it is a MODEL question, not a policy to bolt on.** Schema 1.6.0
already owns this: `observability.metricsEndpoint` renders the Service port **and** the
monitoring-namespace ingress allow. What the model cannot currently express is a **second**
metrics endpoint on the same pod belonging to a *sidecar the renderer injected*, rather than to
the workload the operator declared. Keying a bespoke policy off the pod label
`homelab.keithjackson.dev/auth-mode: forward-auth` works today, but it hardcodes "forward-auth
implies a metrics port on 44180" into a policy instead of **deriving** it from the sidecar
declaration that is already the single source of that port — the exact "list, don't derive"
anti-pattern `oauth2-proxy-cookie-names-distinct` assertion (5) exists to forbid, and which I
would be introducing one directory away from the check that forbids it. That deserves its own
review, and it is squarely the kind of decision I was brought in to make deliberately.
2. **Different subject, and it is a trust-boundary change.** Mine is the auth *response code* on
API paths (R-IDP-02). That is an ingress allow (R-NET-01/02, R-OBS-01) admitting a new
namespace to a new port. Bundling a security-relevant ingress rule into a PR whose headline is
`--api-route` is how such a rule gets waved through on the strength of the other half — and this
PR already needs a human for a *different* sensitive-path reason, so the second subject would be
reviewed in the shadow of the first.
3. **The conflict argument does not actually apply to my diff.** It assumes I am regenerating five
goldens; **I am regenerating two** (jackett, radarr), because the flag renders only where
declared. A metrics-ingress policy renders a *different document* — a CiliumNetworkPolicy — in
all five. Different hunks, in files git merges independently; and goldens regenerate
deterministically from the renderer, so the follow-up rebases onto `main` cleanly. The N-1
rebase problem `decisions/README.md` records was two branches appending to **the same hunk**,
which this is not. I checked my own diff before answering rather than accepting the premise.
4. **Every fact in the finding is hers and I have measured none of it** — not the 200/10,208 bytes,
not the `wget: download timed out` on `:44180` *and* `:4180`, not the Flux sibling ordering that
makes `oauth2-proxy` and `apps-wave-0` unordered. Shipping a security-adjacent rule on wholly
inherited evidence, as a footnote in someone else's PR, is precisely what CLAUDE.md §2b exists
to stop. In its own PR I would measure it first.
5. **Sequencing: the PodMonitor is in HER PR #29, not mine.** If I ship the policy and her
PodMonitor is reworked or delayed, I have opened an ingress allow for a scraper that selects
nothing — a hole with no consumer. The policy is only useful paired with the PodMonitor, which
argues for it landing adjacent to that, not here. It also drags in a
`KNOWN_EXTERNAL_MONITORS` entry in `conformance/tests/l0-static/servicemonitor-ports-resolve/`,
a second conformance interaction on top of the new check this PR already adds.
Diff size is the tiebreaker rather than the argument: **1,245 insertions across 13 files**,
including a ~400-line new conformance check. I would rather hand a reviewer one subject they can
actually falsify. **Queued, not dropped** — and if the next person takes it, the model note in (1)
is the part worth arguing about, not the YAML.
### Which apps got which routes, and the measurement behind each
| App | `auth.apiRoutes` | Evidence |
@@ -224,6 +275,33 @@ rendered sidecar against its **own ServiceDefinition**, which is the only readin
r-nosuchreq-99 --include-scope whole-repo --print-selection` lists
`tests/l0-static/oauth2-proxy-api-routes-per-app` among **21** tests (it was 20), i.e. it runs
even when the touched-requirement filter matches nothing.
- **THE SCHEMA GUARD IS PROVEN WITH THE PINNED VALIDATOR `ci/l0.sh` ACTUALLY DRIVES**, not by
reading the pattern. `./schemas/validate.sh` (`check-jsonschema, version 0.29.4`) against seven
throwaway ServiceDefinitions written outside the repo:
good ["^/api/", "^/signalr/"] rc 0
bad-root ["^/"] rc 1 apiRoutes ... does not match
bad-dotstar ["^/.*"] rc 1 apiRoutes ... does not match
bad-unanchored ["/api/"] rc 1 apiRoutes ... does not match
bad-alt ["^/api/|^/"] rc 1 apiRoutes ... does not match
bad-everything [".*"] rc 1 apiRoutes ... does not match
bad-mode ["^/api/"] on mode: oidc rc 1 const 'forward-auth'
So a too-broad regex is rejected **where a human types it**, on every gate run, by the real tool
— and the shape invariant fires too.
- **THE RENDERER'S REFUSAL REACHES `render-golden` AS A READABLE FAILURE, NOT A TRACEBACK.** With
`^/signalr/` swapped for `^/` in a *copy* of the catalog (outside the repo), both invocation
shapes exit **rc 1 with 0 tracebacks** — the single-SD form and `--all <dir> -o <dir>
--wave-layout --no-image-automation`, which is the one `ci/render-golden.sh` drives:
render.py: radarr: auth.apiRoutes entry '^/' is not an anchored path regex. Each entry
must start with '^/' and continue with literal path characters (e.g. '^/api/',
'^/signalr/', '^/api/v3/.*'). ... A too-broad --api-route answers a PAGE LOAD with 401, so
a logged-out operator sees a bare error page and is never offered a sign-in: the service
looks permanently broken rather than merely unauthenticated.
So the breadth guard is gated by `render-golden` even on a run where the schema stage skipped,
and the operator is told *why* rather than handed a stack trace.
- **The schema's new shape invariant.** A non-empty `auth.apiRoutes` requires
`auth.mode: forward-auth`, because the flag renders onto the per-app sidecar and no sidecar exists
for `oidc`/`native`/`none` — declaring it elsewhere would be a **silent no-op**, which is the
@@ -245,9 +323,63 @@ rendered sidecar against its **own ServiceDefinition**, which is the only readin
watched. Nobody has added a Jackett indexer or recovered a Radarr tab through this
configuration. The human test below is the first real evidence this change will ever have.
- **Local gate: rc 3 (UNDETERMINED), not 0.** Green: `spec-immutability`, `decisions-log`, `lint`,
`l0`, `render-golden`, `l1`, `approval-policy`. **Skipped:** `gitea-settings` (no
`SETTINGS_API_TOKEN` locally) and **L2/L3 conformance** (`CLUSTER_AVAILABLE` unset). Nothing
FAILED; nothing runtime was certified either. **rc 3 is UNDETERMINED, never green.**
`l0`, `render-golden`, `l1`. **Skipped:** `gitea-settings` (no `SETTINGS_API_TOKEN` locally) and
**L2/L3 conformance** (`CLUSTER_AVAILABLE` unset). Nothing FAILED; nothing runtime was certified
either. **rc 3 is UNDETERMINED, never green.**
- **MY LOCAL `approval-policy: green` WAS MEANINGLESS, AND I ALMOST REPORTED IT AS EVIDENCE.**
`ci/approval-policy.sh:47` reads `GITHUB_EVENT_NAME`, which is unset outside CI, so the stage
short-circuits at line 50: `ok "not a pull_request event (event='none') — approval policy N/A"`.
**A clean local gate therefore CANNOT catch a sensitive-path problem — structurally, not by
accident.** Flagged to me by the coordinator from Ines Dubois's round, and I verified it by
re-running the gate with `GITHUB_EVENT_NAME=pull_request PR_NUMBER=34`: the stage then
**evaluates** and fails closed on the missing token —
[fail] no SETTINGS_API_TOKEN — cannot read this PR's changed files or its reviews.
[fail] stage approval-policy: FAIL
[fail] approval-policy: a sensitive change lacks a non-author approval -> blocks merge
[fail] MERGE GATE: BLOCKED (rc 1, not 3)
— while **every other stage stays green** (`spec-immutability`, `decisions-log`, `lint`, `l0`,
`render-golden`, `l1`). That is the useful result: it bounds the CI failure to this one stage
rather than leaving "something is red" as the whole report. Fail-closed is correct here and
deliberate: unlike `gitea-settings`, where a missing credential is a lock-out, a skip of this
stage would be a bypass (CLAUDE.md §1).
- **CI, read from the commit's `/statuses` rather than from a tick.** On the branch head
`df1b25b6`, both contexts have reported and **only one of them ran**:
gate / gate (pull_request) state=failure desc='Failing after 1m45s'
gate / conformance (pull_request) state=success desc='Has been skipped'
**That second line is §2b's trap in the wild**: a *skipped* Gitea job reports
`state=success`. The conformance job did not execute, so its green tick certifies nothing —
read the description, not the colour. Worth recording the earlier reading too, because the
**same context changed its answer**: on the first push (`99560c2d`) it was
`state=pending desc='Blocked by required conditions'`, i.e. still waiting on the gate job it
depends on; once that job settled, the skip resolved to `success`. Anyone sampling a status
once can therefore see either a pending or a green tick for a job that never ran.
**I could NOT read the CI job log to confirm the failing stage from CI's
own output**: this Gitea's `/api/v1/repos/{owner}/{repo}/actions/runs` endpoint answers
**404** (only `/actions/tasks` exists, which lists tasks without logs), and
`/actions/jobs/{task_id}/logs` answers `404 Not found` for the task id. So my attribution of the
CI failure to `approval-policy` rests on the local event-set reproduction above plus the
sensitive-path rule, **not** on CI's own log. That is weaker evidence than PR #28 had and I am
naming it rather than borrowing its wording.
- **Branch protection and the runner, RE-MEASURED this session against the live API** (§2b),
not inherited from CLAUDE.md §2's table:
GET /repos/Home-Network/home-network-k3s-impl/branches/main
protected True
user_can_push False
enable_status_check True
status_check_contexts ['gate / gate (pull_request)']
required_approvals 1
GET /admin/actions/runners
id=1 name=incluster-dind status=online
So `required_approvals=1` applies to **every** PR here regardless of path — this needs a human
even without the sensitive-path rule. I did **not** provoke a rejected push to `main` this
session; `user_can_push: False` is the API's word, which is exactly the weaker form of evidence
CLAUDE.md §2 warns about, and I did not strengthen it.
- **`^/api/` ON RADARR IS NOT MY MEASUREMENT.** It is PR #30's, taken on the **pre-fix** pod. The
current pod has logged **zero** `/api/` traffic in 23 h, so I could not reproduce it. I judge the
breadth risk nil (`/api/v3/` is pure JSON; the UI is at `/`), but if someone wants a strictly
@@ -301,6 +433,12 @@ rendered sidecar against its **own ServiceDefinition**, which is the only readin
(R-TEST-06, read-only). `schemas/renderer-contract.md` is updated in **this** repo only; the
canonical spec `docs/13-service-catalog.md` is **not** updated, because it is immutable from here.
Someone with spec write access should mirror `auth.apiRoutes` into docs/13 and docs/15.
- **THE OAUTH2-PROXY SIDECARS' METRICS ARE STILL UNSCRAPABLE, AND THIS PR DOES NOT FIX IT.** See
the declined-bundle section above. #28 shipped `--metrics-address` as a prerequisite and said
plainly that it was not scraped; Ines has now measured that the listener **serves** and that the
scrape is **dropped by each app namespace's default-deny**. Nothing in my change touches that.
Anyone reading this entry as "the fleet gates are observable now" is wrong, and the next PR is
where that changes.
- **Alternation is refused, and that is a judgement I may have to revisit.** No app in this catalog
needs it, and the flag is repeatable — but if some future app genuinely needs a regex the ratchet
rejects, the fix is to widen the pattern **with a reason** on all three surfaces at once, not to