Future deepening: extract the status application + deepen the Flow module #54

New issue

Open

opened 2026-09-16 19:31:27 +00:00 by ginjiruu · 0 comments

ginjiruu commented

2026-09-16 19:31:27 +00:00

Owner

Context

The leader status-write race (concurrent Builds from the three per-kind controllers + the TTL refresh racing on the CRD statuses and the managed Registry Secret) was fixed in internal/registry/state_loader.go: buildMu serializes whole Builds, and applyStatuses wraps each status update in retry.RetryOnConflict with a fresh-copy re-check. The behavior is pinned by the envtest concurrency canary in internal/controller/providers/build_concurrency_test.go (8 goroutines × 3 Builds + an external writer; red pre-fix, green post-fix).

This issue tracks the two follow-on deepening opportunities from the architecture review of 16 Sep 2026. They are independent; each is a self-contained piece of work.

Status (updated 21 Sep 2026)

Context race fix: committed (115ed51). buildMu serializes whole Builds and applyStatuses wraps each status update in retry.RetryOnConflict with a fresh-copy re-check; pinned by the envtest canary in internal/controller/providers/build_concurrency_test.go.
Availability onto the App: OIDC + Exchange done, callback still inlined (f88415a). App.OIDCAvailable() / App.ExchangeAvailable() exist and are used by the OIDC/Exchange gates and Snapshot.Servable(). The callback's requirement (TokenEndpoint + JWKS) is still inlined in callbackUnavailableReason (internal/serve/callback.go) — "finish moving availability onto the App" is still pending for the callback.
Neither deepening piece started. applyStatuses / statusChanged / mergeConditions are still on the Builder in state_loader.go (no extracted module); there is no shared resolveApp — the three host-resolving Flows still each call Snapshot.AppForHostname directly.

1. Extract the status application from the leader's Builder

The Builder (state_loader.go) still mixes eight responsibilities: leader gating, Cookie-key bootstrap, cluster listing, referenced-Secret checks, IdP resolution, blob encoding, Secret write, and status application. The status application (applyStatuses + statusChanged + mergeConditions) is a self-contained behavior with its own invariants and deserves its own module with a small interface (e.g. Apply(ctx, statuses)), leaving the Builder a thinner orchestrator over the pure Build. Extracting the Cookie-key bootstrap (ensureCookieKeySecret, with its create-if-absent + BYO invariants) is the natural second step.

Constraints settled during the design discussion (do not re-litigate without cause):

Keep cross-kind status writes. Every build writes all three kinds' statuses. Per-kind filtering (each controller writes only its own kind) was rejected: cross-kind effects (e.g. a policy change degrading a client) would go stale for up to one RefreshInterval (5m), and the TTL build's all-kinds write would break single-writer anyway.
Keep the RetryOnConflict + fresh-copy re-check semantics (skip the write when the status is already current — no resourceVersion bump).
Keep the statusChanged List-based fast path outside the retry (avoids a Get per object in the common case).
The concurrency canary must stay green throughout.

2. Deepen the Flow module

The Flow concept — resolve the App, gate on the fail-closed model (ADR 0011), verify, respond — has no module. Four one-method structs in internal/serve (oidcService, callbackService, exchangeService, logoutService) each carry their own copy of: the availability gate (three near-identical *UnavailableReason functions), App resolution (three host strategies — gRPC host verbatim, logout port-stripped, callback by appKey — plus Snapshot.AppForHostname is an exact-match index with no port normalization), and response shaping. Every single-Flow test today brings up the whole Server (three listeners, four structs, a real gRPC dial) to exercise one handler.

State at the time of writing (the working tree already moved part-way):

The shared IdP http.Client is consolidated on the Server (the three duplicated clients are gone).
Availability is half-moved onto the data side: App.OIDCAvailable() / App.ExchangeAvailable() exist in internal/snapshot/snapshot.go and are used by the OIDC/Exchange gates and Snapshot.Servable(). The callback's requirement (TokenEndpoint + JWKS) is still inlined in callbackUnavailableReason in serve.

Direction settled during the design discussion:

One Flow module per Endpoint on a common base (shared state: Snapshot provider + http client; shared sequence: resolve → gate → verify → respond). The transport method stays per Flow (gRPC Check vs HTTP ServeHTTP) — an abstracted request/decision type was rejected as a shallow abstraction (it would have to express status codes, headers, set-cookie, redirects, and gRPC denied_response).
Finish moving availability onto the App (the codebase already chose this side of the gate-shape question), including the callback's requirement.
One resolveApp in serve that strips the port before the exact-match lookup, used by all host-resolving Flows (unifies the gRPC verbatim vs logout port-stripped split; normalizing inside Snapshot.AppForHostname was rejected as a serving concern in the snapshot package).
Token-endpoint grants are out of scope — the three grant constructions (renew, authorizationCodeExchange, tokenExchange, with three divergent test doubles) are a separate candidate: one Token Endpoint module whose interface is Grant(ctx, grant) → tokens, hiding the x/oauth2 + hstern trio (ADR 0013 pins the libraries, not the module shape) and the Public-client secret rule, with two adapters at the seam (real HTTP, in-memory double).

Test goal: single-Flow tests cross the Flow's interface (Snapshot + token-endpoint double) instead of bringing up the whole Server; Server-level tests shrink to wiring coverage (listener bind, callback/logout dispatch, warm loop).

Pointed-at references

docs/references/kanidm-service-account-token-exchange.md (Exchange Flow semantics) before touching the Exchange Flow.
docs/agents/deploy.md for live deploy/test on the portable cluster.
ADRs 0003/0004 (endpoints + per-Flow kinds), 0007/0010/0011/0012 (session/state/status model), 0013 (protocol trio).

## Context The leader status-write race (concurrent Builds from the three per-kind controllers + the TTL refresh racing on the CRD statuses and the managed Registry Secret) was fixed in `internal/registry/state_loader.go`: `buildMu` serializes whole Builds, and `applyStatuses` wraps each status update in `retry.RetryOnConflict` with a fresh-copy re-check. The behavior is pinned by the envtest concurrency canary in `internal/controller/providers/build_concurrency_test.go` (8 goroutines × 3 Builds + an external writer; red pre-fix, green post-fix). This issue tracks the two follow-on deepening opportunities from the architecture review of 16 Sep 2026. They are independent; each is a self-contained piece of work. ## Status (updated 21 Sep 2026) - **Context race fix: committed** (`115ed51`). `buildMu` serializes whole Builds and `applyStatuses` wraps each status update in `retry.RetryOnConflict` with a fresh-copy re-check; pinned by the envtest canary in `internal/controller/providers/build_concurrency_test.go`. - **Availability onto the App: OIDC + Exchange done, callback still inlined** (`f88415a`). `App.OIDCAvailable()` / `App.ExchangeAvailable()` exist and are used by the OIDC/Exchange gates and `Snapshot.Servable()`. The callback's requirement (`TokenEndpoint` + `JWKS`) is still inlined in `callbackUnavailableReason` (internal/serve/callback.go) — "finish moving availability onto the App" is still pending for the callback. - **Neither deepening piece started.** `applyStatuses` / `statusChanged` / `mergeConditions` are still on the `Builder` in state_loader.go (no extracted module); there is no shared `resolveApp` — the three host-resolving Flows still each call `Snapshot.AppForHostname` directly. ## 1. Extract the status application from the leader's Builder The `Builder` (state_loader.go) still mixes eight responsibilities: leader gating, Cookie-key bootstrap, cluster listing, referenced-Secret checks, IdP resolution, blob encoding, Secret write, and status application. The status application (`applyStatuses` + `statusChanged` + `mergeConditions`) is a self-contained behavior with its own invariants and deserves its own module with a small interface (e.g. `Apply(ctx, statuses)`), leaving the `Builder` a thinner orchestrator over the pure `Build`. Extracting the Cookie-key bootstrap (`ensureCookieKeySecret`, with its create-if-absent + BYO invariants) is the natural second step. Constraints settled during the design discussion (do not re-litigate without cause): - **Keep cross-kind status writes.** Every build writes all three kinds' statuses. Per-kind filtering (each controller writes only its own kind) was rejected: cross-kind effects (e.g. a policy change degrading a client) would go stale for up to one `RefreshInterval` (5m), and the TTL build's all-kinds write would break single-writer anyway. - **Keep the `RetryOnConflict` + fresh-copy re-check semantics** (skip the write when the status is already current — no resourceVersion bump). - **Keep the `statusChanged` List-based fast path** outside the retry (avoids a Get per object in the common case). - The concurrency canary must stay green throughout. ## 2. Deepen the Flow module The Flow concept — resolve the App, gate on the fail-closed model (ADR 0011), verify, respond — has no module. Four one-method structs in `internal/serve` (`oidcService`, `callbackService`, `exchangeService`, `logoutService`) each carry their own copy of: the availability gate (three near-identical `*UnavailableReason` functions), App resolution (three host strategies — gRPC host verbatim, logout port-stripped, callback by appKey — plus `Snapshot.AppForHostname` is an exact-match index with no port normalization), and response shaping. Every single-Flow test today brings up the whole `Server` (three listeners, four structs, a real gRPC dial) to exercise one handler. State at the time of writing (the working tree already moved part-way): - The shared IdP `http.Client` is consolidated on the `Server` (the three duplicated clients are gone). - Availability is half-moved onto the data side: `App.OIDCAvailable()` / `App.ExchangeAvailable()` exist in `internal/snapshot/snapshot.go` and are used by the OIDC/Exchange gates and `Snapshot.Servable()`. The **callback's** requirement (`TokenEndpoint` + `JWKS`) is still inlined in `callbackUnavailableReason` in serve. Direction settled during the design discussion: - **One Flow module per Endpoint on a common base** (shared state: Snapshot provider + http client; shared sequence: resolve → gate → verify → respond). The transport method stays per Flow (gRPC `Check` vs HTTP `ServeHTTP`) — an abstracted request/decision type was rejected as a shallow abstraction (it would have to express status codes, headers, set-cookie, redirects, and gRPC `denied_response`). - **Finish moving availability onto the App** (the codebase already chose this side of the gate-shape question), including the callback's requirement. - **One `resolveApp` in serve** that strips the port before the exact-match lookup, used by all host-resolving Flows (unifies the gRPC verbatim vs logout port-stripped split; normalizing inside `Snapshot.AppForHostname` was rejected as a serving concern in the snapshot package). - **Token-endpoint grants are out of scope** — the three grant constructions (`renew`, `authorizationCodeExchange`, `tokenExchange`, with three divergent test doubles) are a separate candidate: one Token Endpoint module whose interface is `Grant(ctx, grant) → tokens`, hiding the x/oauth2 + hstern trio (ADR 0013 pins the libraries, not the module shape) and the Public-client secret rule, with two adapters at the seam (real HTTP, in-memory double). Test goal: single-Flow tests cross the Flow's interface (Snapshot + token-endpoint double) instead of bringing up the whole `Server`; `Server`-level tests shrink to wiring coverage (listener bind, callback/logout dispatch, warm loop). ## Pointed-at references - `docs/references/kanidm-service-account-token-exchange.md` (Exchange Flow semantics) before touching the Exchange Flow. - `docs/agents/deploy.md` for live deploy/test on the portable cluster. - ADRs 0003/0004 (endpoints + per-Flow kinds), 0007/0010/0011/0012 (session/state/status model), 0013 (protocol trio).

ginjiruu added the

ready-for-agent

label

2026-09-16 19:31:30 +00:00