diff --git a/archreview/plans/00-INDEX.md b/archreview/plans/00-INDEX.md index 71b53886..ba631d98 100644 --- a/archreview/plans/00-INDEX.md +++ b/archreview/plans/00-INDEX.md @@ -11,6 +11,33 @@ > [`STATUS.md`](STATUS.md) for the PR/wave map, unit-sweep tallies, the 3 confirmed-pre-existing failures, and > the deferred heavy/live-pass checklist. +## Round 3 plans — tranche 1 (2026-08-20, verified at `7c77a8c7` ≡ `12b0035a`) + +The 2026-08-19 round-3 re-review ([`../00-OVERALL.md`](../00-OVERALL.md)) produced a 12-item action +list. **Tranche 1 = items 1–5** — the Critical, the four new Highs, and the two small Medium-Highs they +carry along. Items 6–12 are queued as tranche 2. Each plan follows the round-2 house format and carries a +co-located `.tasks.json`. + +| # | Plan | Findings | Tasks | Plan author | Executor | Reviewer | +|---|---|---|---|---|---|---| +| 1 | [R3-01 Mesh gRPC `.AllowAnonymous()`](R3-01-mesh-grpc-allowanonymous-plan.md) | 03/S14 **Critical** | 10 | opus | sonnet (+ fable for the `DisableLogin=false` rig gate) | opus | +| 2 | [R3-02 Stale-Good family](R3-02-stale-good-family-plan.md) | 03/S15, 05/STAB-19, 05/STAB-18, 02/S14 | TBD | **fable** | opus | opus | +| 3 | [R3-03 Restart re-subscribe + watchdog `max()`](R3-03-restart-resubscribe-watchdog-plan.md) | 03/S16, S17, U6 | TBD | opus | sonnet | opus | +| 4 | [R3-04 One role authority](R3-04-one-role-authority-plan.md) | 01/S-12 = 03/C5, 01/C-9 | 13 | opus | sonnet | opus | +| 5 | [R3-05 ACL hot path + key expiry](R3-05-acl-hot-path-expiry-plan.md) | 03/S18, S19 | TBD | opus | opus | opus | + +**Model rationale.** Plan-writing is where design judgment lives, so every plan is opus or better; R3-02 +spans four seams with one shared contract and a known vacuity trap (value-dedup), so it gets fable. Execution +of R3-01/03/04 is mechanical once planned (one-line-per-map, one re-push call, one normalizing authority) — +sonnet, with the adversarial review on opus. R3-02 and R3-05 change hot-path semantics (quality propagation; +the SDK session-activation callback) — opus executes. The rig gate that flips `DisableLogin=false` is the one +step that reasons about an observed failure rather than following a recipe — fable. + +**Execution shape.** One branch per plan (`fix/r3-0N-…`) in an isolated worktree; unit suite of the touched +projects green before review; opus code-review against the plan; `--no-ff` merge to master in the order +**R3-03 → R3-01 → R3-04 → R3-05 → R3-02** (R3-03 and R3-02 both touch `DriverHostActor`; the small one +lands first and R3-02 rebases). Live gates run on docker-dev after the merges. + ## Round 2 plans (2026-07-12, verified at `f6eaa267`) One plan per action item in the updated `00-OVERALL.md` prioritized list. Each verifies its findings diff --git a/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md b/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md new file mode 100644 index 00000000..a0400ede --- /dev/null +++ b/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md @@ -0,0 +1,354 @@ +# Design + Implementation Plan — R3-01 Mesh gRPC `AllowAnonymous` + +- **Source report:** `archreview/03-server-runtime.md` §S14 (2026-08-19 round-3 re-review at `12b0035a`) +- **Round-1/2 designs consumed:** none — **S14 is NEW this round**. No prior plan, branch or issue has ever targeted it. +- **Plan verified against tree at:** master `7c77a8c7` (clean; code identical to `12b0035a`) — every anchor re-opened and re-checked 2026-08-20 +- **Scope:** OVERALL prioritized action item **#1** — "`.AllowAnonymous()` on every mesh gRPC map (the PSK interceptors are the gate); one rig/gate pass with `DisableLogin=false`; a standing CI variant of `MeshGrpcBoundaryTests` with `UseAuthentication`/`UseAuthorization` + the FallbackPolicy" (03/S14, **Critical**). +- **Cross-cutting requirement (OVERALL theme 2, "verification that cannot see its own blind spot"):** the fix is one line per map; **the plan's real deliverable is the verification that would have caught it.** Two standing guards (a real-auth-stack boundary test + an endpoint-metadata census, both with falsifiability controls) and one rig change that makes the production middleware stack reachable on demand (`Security__Auth__DisableLogin` parameterised, default unchanged). A fix without those guards leaves the *next* map free to re-open the Critical. +- **Cross-cutting requirement (OVERALL theme 4, "one rule stated twice diverges"):** the five map sites move behind **one** mapper (`MapOtOpcUaMeshGrpcServices`) that both Program.cs and the census test drive, so the guard can never be testing a hand-copied shape — which is precisely how the existing listener tests (`ConfigServeListenerTests`, `LocalDbSyncListenerTests`, `MeshGrpcBoundaryTests`) came to build pipelines that omit the middleware that breaks production. + +## Verification summary + +S14 is **CONFIRMED at `7c77a8c7`** — not stale, not partially fixed, and every line anchor in the report is exact. One claim in the finding is **over-stated** and one is **incomplete**; both are corrected below and both change the work (they add one non-chainable map and one extra affected caller). + +| Report anchor | Claim | Status at `7c77a8c7` | +|---|---|---| +| `Security/ServiceCollectionExtensions.cs:143-148` | global `FallbackPolicy = RequireAuthenticatedUser` under the cookie scheme | **Exact.** File is `src/Server/ZB.MOM.WW.OtOpcUa.**Security**/ServiceCollectionExtensions.cs` (note: not under `Host/`, as the report's shorthand reads). `AddAuthorization` at `:143`, `FallbackPolicy` built `:145-148` naming `CookieAuthenticationDefaults.AuthenticationScheme`. | +| `Security/ServiceCollectionExtensions.cs:82-98` | `DisableLogin` registers a pass-everything handler **under the cookie scheme name** | **Exact.** `authBuilder.AddScheme(CookieAuthenticationDefaults.AuthenticationScheme, …)` at `:87-88`, with the "FallbackPolicy + FleetAdmin + DriverOperator … pass with all roles — zero policy changes" comment at `:84-86`. The masking mechanism is exactly as described. | +| `Host/Program.cs:520-523` | fallback registered on `hasAdmin` nodes only | **Exact.** `if (hasAdmin) { builder.Services.AddOtOpcUaAuth(builder.Configuration); … }` at `:520-523`. | +| `Host/Program.cs:732-733` | `UseAuthentication`/`UseAuthorization` in the pipeline | **Exact**, and **also inside `if (hasAdmin)`** (`:730-741`) — which is what makes the driver-only exemption structural rather than incidental. `UseAntiforgery()` follows at `:734` (harmless for gRPC: the antiforgery middleware only validates endpoints carrying `IAntiforgeryMetadata`). | +| `Host/Program.cs:724-728` | `MapStaticAssets().AllowAnonymous()` proves the author knew about the fallback | **Exact.** The comment at `:724-727` states the failure mode verbatim ("401s every CSS/JS request"). Two further precedents: `AdminUI/Api/DeployApiEndpoints.cs:61` (`.AllowAnonymous() // gated by the API key, not the cookie auth fallback`) and `Host/Observability/ObservabilityExtensions.cs:56` (`app.MapZbMetrics().AllowAnonymous()`). **Three** call sites already apply the pattern; the mesh maps are the omission. | +| `Host/Program.cs:749, 757, 767, 782-783` | five mesh maps, none carrying `AllowAnonymous` | **Exact line-for-line**, and **complete**: `grep -rn "MapGrpcService\|MapZbLocalDbSync" src` returns exactly these five sites and nothing else. | +| `MeshGrpcBoundaryTests.cs:334-346` | builds a pipeline with interceptors but no auth stack | **Confirmed, minor drift** — the builder block is `:333-348` (`WebApplication.CreateBuilder()` at `:333`, `AddGrpc(o => o.Interceptors.Add())` at `:342`, `MapGrpcService()` at `:347`). No `AddAuthentication`/`AddAuthorization`/`UseAuthentication`/`UseAuthorization` anywhere in the file. **Additional gap the report does not name:** the class only ever builds the **driver-node** (`NodeCommandService`) shape — the *central serve side* (`DeploymentArtifactService` + `CentralCommandService`), which is the fleet-wide choke point, has no boundary test at all. `ConfigServeListenerTests.cs:33-59` and `LocalDbSyncListenerTests.cs:25-27` build Kestrel-binding-only apps and say so in their own remarks ("shaped exactly like `Program.cs`'s … block"). | + +### Which maps are actually affected — determined, not assumed + +The fallback reaches an endpoint iff (a) the authorization middleware is in this node's pipeline, and (b) the endpoint carries no `IAuthorizeData` and no `IAllowAnonymous` metadata. Port is **irrelevant**: the dedicated h2c listeners (`Program.cs:692-703`) are extra Kestrel endpoints on the **same `WebApplication`**, so they share one middleware pipeline and one endpoint table. There is no `RequireHost`, no per-port branch and no second host anywhere in `Program.cs`. So: + +| Map | Role gate | Affected on a login-enabled node? | +|---|---|---| +| `DeploymentArtifactGrpcService` (`:757`) | `hasAdmin && ConfigServe:GrpcListenPort > 0` | **YES — always.** Admin-only map on an admin-only pipeline. Every `FetchAndCache` node's artifact fetch. | +| `CentralCommandGrpcService` (`:767`) | same | **YES — always.** Every node's `ReportApply` deploy ack, from *every* cluster. | +| `TelemetryStreamGrpcService` (`:782`) | `hasDriver && Telemetry:GrpcListenPort > 0` | **YES on a fused `admin,driver` node** (the docker-dev central pair, and any production fused central). **NO on a driver-only node.** | +| `NodeCommandGrpcService` (`:783`) | same | **YES on a fused node / NO on driver-only** — same pipeline. Central→central command dispatch is affected; central→site dispatch is not. | +| `MapZbLocalDbSync` (`:749`) | `hasDriver && LocalDb:SyncListenPort > 0` | **Conditionally — and NOT in the current fleet.** The report lists it unqualified; in fact the only nodes that set `LocalDb__SyncListenPort` are the driver-only **site-a** pair (`docker-dev/docker-compose.yml:657`), where no authorization middleware exists. It becomes affected the moment a **fused** node sets a sync port. Fixed anyway (it is one config key away from being a live outage) — and it is the one map that **cannot** take `.AllowAnonymous()` (see below). | + +**Driver-only nodes: confirmed unaffected, structurally.** `AddOtOpcUaAuth` is the only caller of `AddAuthorization` in the host graph (grep-verified), and it sits inside `if (hasAdmin)` (`:520-523`); `UseAuthentication`/`UseAuthorization` sit inside `if (hasAdmin)` (`:730-741`). A driver-only node therefore has **no authorization middleware at all** and no fallback policy to apply. The report's claim holds. (This is worth a standing test, not just a reading — a future "register auth everywhere for consistency" change would silently break every site node's telemetry and command ingress. T6.) + +**Correction that changes the work:** `MapZbLocalDbSync` returns **`IEndpointRouteBuilder`**, not an `IEndpointConventionBuilder`, so `.AllowAnonymous()` **cannot be chained onto it** (verified by reading the method signature out of `ZB.MOM.WW.LocalDb.Replication` 0.2.1's metadata: `MapZbLocalDbSync : returns IEndpointRouteBuilder (IEndpointRouteBuilder)`). The library also marks nothing anonymous itself — the assembly contains no `AllowAnonymous`/`Authorize` metadata reference at all. That map needs a different mechanism (resolved below). + +**Extra blast radius the finding does not spell out** (fused central, `DisableLogin=false`, `MeshTransport:Mode=Grpc`): central-1/central-2 carry `ClusterNode` rows with a `GrpcPort` (`docker-dev/seed/seed-clusters.sql:71-79`), and `CentralCommunicationActor` dials **one call per enabled row**, its own pair included. So the MAIN pair's *own* deploy dispatch 401s against its *own* `NodeCommandService`, and central's telemetry dialer 401s against its own pair's `TelemetryStreamService`. A login-enabled central pair therefore breaks the deploy plane for **all six** rig nodes, not only the four sites. + +Supporting facts verified: all five interceptors are **fail-closed** — a blank/whitespace expected key rejects every call, and the presented token is compared with `CryptographicOperations.FixedTimeEquals` (`ConfigServeAuthInterceptor.cs:112-141`, `TelemetryStreamAuthInterceptor.cs:112-145`, `MeshCommandAuthInterceptor.cs:125-154`, `CentralCommandAuthInterceptor.cs:142-190`, `LocalDbSyncAuthInterceptor.cs:117-147`); each returns early on a non-matching method prefix, so the shared `AddGrpc` pipeline (`Program.cs:586-602`) is a pass-through for foreign paths. `Program.cs:574` already states the design contract in so many words: "the interceptor is the ONLY inbound auth on any endpoint (the libraries verify nothing)". Metrics are already anonymous (`ObservabilityExtensions.cs:56`); `MapOtOpcUaHealth` delegates to `MapZbHealth()` (`Health/HealthEndpoints.cs:104`) and `ZB.MOM.WW.Health` 0.3.0's assembly *does* reference `AllowAnonymous` — the census in T2 settles it either way rather than trusting the string. Rig masking confirmed: `Security__Auth__DisableLogin: "true"` on central-1 (`docker-dev/docker-compose.yml:385`) and central-2 (`:511`); both are `OTOPCUA_ROLES=admin,driver,cluster-MAIN` (`:298`, `:448`). + +### Open design choices — resolved in this plan + +1. **Fix mechanism: `.AllowAnonymous()` per map**, not a policy, not a scheme, not a second host. The interceptors are verified fail-closed, the codebase already applies this exact pattern at three sites for the same reason, and it is the only option that leaves the failure statuses dialers already handle untouched. Alternatives weighed in *Proposed design*. +2. **LocalDbSync mechanism: a single `app.MapGroup("").AllowAnonymous()` scoped to that one call.** The library map is not chainable (verified above). The empty prefix leaves the gRPC route patterns byte-identical; the group is deliberately **not** extended over the four chainable maps, so the deviation stays as small as the problem. +3. **Extract the five maps behind `MapOtOpcUaMeshGrpcServices`** (Host, one file) rather than testing a hand-copied shape. This is the theme-4 move and it is what makes the census a *production* assertion. Rejected alternative: leave the maps inline and have the census rebuild them in test code — that is the same construction that produced S14. +4. **Two guards, not one.** The census (metadata) catches a map added to the helper without the convention; a **Program.cs source-scan** (T7, following the in-repo idiom at `tests/Drivers/Cli/…/FlushLoggingConventionTests.cs:28-45`) catches a map added to `Program.cs` *outside* the helper. Neither subsumes the other. +5. **Assert the interceptor's detail string, never the status alone.** `ConfigServeAuthInterceptor` rejects with `StatusCode.Unauthenticated` (`:127`), and `Grpc.Net.Client` maps an HTTP **401** to `Unauthenticated` as well — so "no-PSK call fails with Unauthenticated" is **vacuous on the artifact service**: it passes identically before and after the fix. The interceptor's messages are the discriminator (`"Config-serve authentication failed."`, `"Telemetry stream authentication failed."`, `"Node command authentication failed."`). +6. **The load-bearing assertion is the ALLOW.** A blocked call and a refused call both "fail"; only a *correctly-keyed call succeeding through the real auth stack* distinguishes the fixed build. Every deny assertion in T4/T5 is paired with the positive. +7. **Rig: parameterise, don't flip.** `Security__Auth__DisableLogin: "${OTOPCUA_DISABLE_LOGIN:-true}"` on the central pair — default behaviour byte-identical, and the production-faithful mode becomes a documented one-variable flip instead of an edit-and-remember-to-revert. Rejected: making `false` the rig default (every future rig session then needs an LDAP login through a round-robin `:9200`, and the shared GLAuth becomes a hard dependency of the whole rig). +8. **No deploy-plane retry/queueing work here.** Mesh dispatch is fire-and-forget by design (CLAUDE.md §Mesh command transport); S14 is an authorization defect, not a delivery-semantics one. Out of scope. + +--- + +## Finding 1 — 03/S14: The AdminUI cookie `FallbackPolicy` blocks every mesh gRPC ingress on a login-enabled admin node (Critical) + +### Restatement + +`AddOtOpcUaAuth` installs a global `FallbackPolicy = RequireAuthenticatedUser` bound to the cookie scheme (`Security/ServiceCollectionExtensions.cs:143-148`), on every `hasAdmin` node (`Program.cs:520-523`), with `UseAuthentication`/`UseAuthorization` in the pipeline (`:732-733`). ASP.NET Core's authorization middleware applies the fallback to **every endpoint that carries no authorization metadata** — gRPC endpoints included — and none of the five mesh maps carries `.AllowAnonymous()` (`:749`, `:757`, `:767`, `:782-783`). The PSK bearer is not an ASP.NET authentication scheme, and gRPC interceptors run *inside* the endpoint invocation, i.e. strictly after the authorization middleware, so a correctly-keyed node call is challenged by the cookie handler before the interceptor's verdict can matter. + +On a production (login-enabled) central this is a fleet-wide deploy-plane outage: every `FetchAndCache` node's artifact fetch fails on every endpoint (→ the apply fails fail-closed to last-known-good per #485 — loud, not corrupting), every gRPC `ReportApply` ack is dropped (→ deployments seal `TimedOut`), and on a fused central the pair's own command dispatch and telemetry ingress fail too. Nothing caught it because the docker-dev rig runs `DisableLogin=true`, which registers an always-succeeding handler *under the cookie scheme name* (`:82-98`) so the FallbackPolicy passes — i.e. **every live gate of the entire seven-phase mesh program ran with the one middleware that breaks production neutralized** — and because `MeshGrpcBoundaryTests` builds its pipelines without any auth stack (`:333-348`). + +### Verification + +Confirmed exact at `7c77a8c7` (see the summary table for line-by-line). Three refinements found while verifying: + +- **The set of affected maps is role-shaped**, not uniform: two are always affected on a login-enabled node (`DeploymentArtifactGrpcService`, `CentralCommandGrpcService`); two are affected only on a **fused** `admin,driver` node (`TelemetryStreamGrpcService`, `NodeCommandGrpcService`); one (`MapZbLocalDbSync`) is affected only on a fused node that also sets a sync port — a shape **no node in the fleet currently has**. Driver-only site nodes have no authorization middleware at all. +- **`MapZbLocalDbSync` is not chainable** (`returns IEndpointRouteBuilder`), so the "one-line-per-map fix" is one line for four maps and a group wrapper for the fifth. +- **The observable failure is not necessarily a 401.** `CookieAuthenticationEvents`'s default `OnRedirectToLogin` returns 401 only for an *AJAX-shaped* request (`X-Requested-With`); a gRPC call carries no such header, so the challenge is a **302 to `/login`**. Whether the caller then reports `Unknown` ("Bad gRPC response. HTTP status code: 302") or a content-type failure depends on whether the channel's handler follows the redirect. This is why T5 asserts the *interceptor's* identity rather than enumerating failure statuses, and why the live gate greps for `/login`/302/401 together rather than "401". + +### Root cause + +Two independent decisions, each locally correct: + +1. **A global fallback policy is the right default for a Blazor admin surface** — it means a newly-added page is protected by default rather than by remembering an attribute (the same instinct as `PageAuthorizationGuardTests`). Its cost is that *every* endpoint added to that host must now opt out explicitly, and the three prior opt-outs (static assets, deploy API, metrics) were each discovered by something breaking. +2. **The mesh gRPC endpoints authenticate in interceptors** because their credential is a per-cluster PSK, not a browser identity — deliberately documented as "the interceptor is the ONLY inbound auth on any endpoint" (`Program.cs:574`). That statement is true of the *gRPC* pipeline and false of the *HTTP* pipeline that precedes it, and nothing in the build or the tests could hold those two sentences next to each other. + +The verification gap is the same shape: every test that touches these endpoints builds the pipeline **by hand**, so each one reproduced the author's mental model rather than the host's actual composition. + +### Proposed design + +**Fix (5 map sites, one new file, no behavioural change to any authenticated surface):** + +```csharp +// src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/MeshGrpcEndpoints.cs (new) + +/// +/// Maps the mesh gRPC ingress endpoints (artifact serve, central ack, telemetry stream, node +/// command) and the LocalDb sync endpoint, each marked AllowAnonymous. +/// +/// +/// +/// Why AllowAnonymous (archreview 03/S14). On an admin-role node +/// AddOtOpcUaAuth installs a global FallbackPolicy = RequireAuthenticatedUser +/// under the cookie scheme, which the authorization middleware applies to EVERY endpoint +/// without explicit authorization metadata — gRPC included. These endpoints authenticate +/// with a per-cluster PSK bearer inside a gRPC interceptor, which runs INSIDE the endpoint +/// invocation, i.e. after that middleware. Without this opt-out a correctly-keyed node call +/// is challenged by the cookie handler before the interceptor's verdict matters: artifact +/// fetches fail, deploy acks are dropped, deployments seal TimedOut. Anonymous is safe here +/// because all five interceptors fail CLOSED — an unset key rejects every call and the +/// presented token is compared with FixedTimeEquals. +/// +/// +/// Do not "tidy" this into Program.cs. The census guard +/// (MeshGrpcAnonymousMetadataTests) drives THIS method, so a map that moves back +/// inline leaves the guard asserting a shape production no longer has — which is exactly +/// how S14 survived seven phases of live gates. +/// +/// +public static void MapOtOpcUaMeshGrpcServices( + this WebApplication app, + bool hasAdmin, + bool hasDriver, + int syncListenPort, + int configServeGrpcPort, + int telemetryListenPort) +{ + if (hasDriver && syncListenPort > 0) + { + // MapZbLocalDbSync returns IEndpointRouteBuilder, NOT an IEndpointConventionBuilder, so + // .AllowAnonymous() cannot be chained onto it. An empty-prefix group carries the convention + // to whatever it maps while leaving the gRPC route patterns byte-identical. + app.MapGroup(string.Empty).AllowAnonymous().MapZbLocalDbSync(); + } + + if (hasAdmin && configServeGrpcPort > 0) + { + app.MapGrpcService().AllowAnonymous(); + app.MapGrpcService().AllowAnonymous(); + } + + if (hasDriver && telemetryListenPort > 0) + { + app.MapGrpcService().AllowAnonymous(); + app.MapGrpcService().AllowAnonymous(); + } +} +``` + +`Program.cs:743-784` collapses to one call (`app.MapOtOpcUaMeshGrpcServices(hasAdmin, hasDriver, syncListenPort, configServeGrpcPort, telemetryListenPort);`) with the existing per-map rationale comments moved verbatim onto the new method's members — **no comment is deleted**; they are the record of why each endpoint rides which listener. + +**Alternatives considered:** + +- ***`RequireHost` / port-scoping the mesh endpoints*** — **does not fix this.** `RequireHost` constrains endpoint *matching*, not authorization; a matched endpoint is still subject to the fallback policy. It is a plausible *complement* (it would stop a mesh service being reachable over the AdminUI's Traefik-fronted port), but the ports are already dedicated and HTTP/2-only, so it buys a second place to keep in sync for no authorization benefit. Rejected as the fix; not adopted as hardening. +- ***A named authorization policy / a real PSK authentication scheme*** — architecturally the "correct" answer (auth in middleware, where the framework expects it) and worth naming as the eventual consolidation. Rejected here: it would duplicate five per-service key resolutions (three different option sources plus `IMeshPskProvider`'s per-cluster map) into a scheme, and it would change the failure surface every dialer already handles from the interceptors' `PermissionDenied`/`Unauthenticated` gRPC statuses to a middleware 403 — i.e. it converts a Critical one-liner into a cross-cutting redesign of the mesh's auth contract. Recorded as a follow-up note, not this plan. +- ***Host the mesh services on a second `WebApplication` with its own pipeline*** — true isolation, and the only option that makes the fallback structurally unable to reach them. Rejected: `Program.cs:604-623` documents how delicate the current single-host Kestrel takeover already is (any explicit `Listen*` discards `ASPNETCORE_URLS`; the surface must be re-bound exactly once), and a second host duplicates the DI graph the services resolve from. +- ***Branch `UseAuthorization` so it does not run for mesh paths (`UseWhen`)*** — path-sniffing middleware, evaluated before the endpoint is known in the general case, and it would silently exempt anything sharing a prefix. Rejected. +- ***Change the FallbackPolicy to something narrower*** — rejected outright: the fallback is what protects every AdminUI page and hub; weakening it to fix five endpoints trades a deploy-plane outage for an authorization hole. +- ***Extend the empty-prefix group over all five maps*** — tidier, but it puts four already-chainable maps behind an untested-in-this-repo route-group/gRPC interaction for no gain. Rejected; the group is scoped to the one map that needs it. + +**Guards (the actual deliverable):** + +1. **`MeshGrpcAuthStackTests`** (new, `Host.IntegrationTests`) — the CI-shaped variant of `MeshGrpcBoundaryTests` the action item asks for. It builds a `WebApplication` whose auth stack is the **production registration** — `builder.Services.AddOtOpcUaAuth(config)` with `Security:Auth:DisableLogin=false`, then `app.UseAuthentication(); app.UseAuthorization();` — and maps the mesh services through `MapOtOpcUaMeshGrpcServices`. Composition constraints found while verifying (all cheap, all must be in the fixture or it will not build): + - `AddOtOpcUaAuth` calls `AddDataProtection().PersistKeysToDbContext()` (`:73-75`) ⇒ the fixture registers an **InMemory** `OtOpcUaConfigDbContext` (the same `InMemoryConfigDbFactory` idiom `MeshGrpcBoundaryTests.cs:288-298` already uses, promoted to a shared fixture helper). + - Set `builder.Environment.EnvironmentName = "Production"` so `ValidateOnBuild`/`ValidateScopes` do not reject services (`ILdapAuthService`'s graph) that are never resolved without a login attempt. + - `Security:Auth:DisableLogin` must be **explicitly false** — the whole point is to exercise the cookie handler, not `AutoLoginAuthenticationHandler`. The fixture asserts the resolved scheme handler type, so a future default flip cannot silently re-mask the test. +2. **`MeshGrpcAnonymousMetadataTests`** (new, same project) — an endpoint-metadata census over `((IEndpointRouteBuilder)app).DataSources` after `MapOtOpcUaMeshGrpcServices`, in all three role shapes (admin-only, driver-only, fused). Asserts every mesh endpoint carries `IAllowAnonymous`, **and** (guard-of-the-guard) that the census found the expected endpoint count per shape — a census over zero endpoints passes vacuously, which is the exact failure class this plan exists to close. It also asserts the census is **scoped**: only the five known gRPC service prefixes are marked anonymous, so a future change that anonymises the AdminUI wholesale fails here. +3. **`MeshGrpcMapConventionTests`** (new, source scan over `Program.cs`, following `FlushLoggingConventionTests.cs:28-45`'s solution-root-walk idiom) — every `MapGrpcService<` / `MapZbLocalDbSync(` occurrence in `Program.cs` is inside `MeshGrpcEndpoints`' call, i.e. `Program.cs` contains **zero** direct map sites; the scan asserts the count is 0 and that `MapOtOpcUaMeshGrpcServices` is called exactly once. This is the guard the metadata census cannot be: it catches a map added to `Program.cs` that bypasses the helper entirely. + +**Rig (theme 2):** `docker-dev/docker-compose.yml` central-1 (`:385`) and central-2 (`:511`) become `Security__Auth__DisableLogin: "${OTOPCUA_DISABLE_LOGIN:-true}"`, with a header-comment paragraph naming the production-faithful gate mode and the LDAP users it needs. Default behaviour is byte-identical. + +### Implementation steps + +1. `src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/MeshGrpcEndpoints.cs` — new mapper (above), carrying the five map sites' existing rationale comments plus the S14 rationale. +2. `src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs` — replace `:743-784` with the single `MapOtOpcUaMeshGrpcServices(...)` call; keep the `syncListenPort`/`configServeGrpcPort`/`telemetryListenPort` locals and the Kestrel block (`:604-711`) untouched. +3. `docker-dev/docker-compose.yml` — parameterise `Security__Auth__DisableLogin` on central-1/central-2; document the flip in the file header. +4. `docs/security.md` — a short "Mesh gRPC ingress is AllowAnonymous by design" section: what the fallback does, why the interceptors are the gate, what fails when the opt-out is missing, and the two standing guards. +5. `docs/Configuration.md` §`MeshTransport` / §`ConfigSource`-`ConfigServe` and `docs/Telemetry.md` — one cross-reference line each, so an operator reading about the listener ports meets the constraint. + +### Tests + +- **`MeshGrpcAuthStackTests`** (new): real auth stack + `MapOtOpcUaMeshGrpcServices`, on an h2c Kestrel port. + - **Positive (load-bearing):** a `DeploymentArtifactService` fetch carrying the correct `ConfigServe:ApiKey` bearer **succeeds** through `UseAuthentication`/`UseAuthorization`; likewise a `CentralCommandService.ReportApply` and a `NodeCommandService.Execute` with their keys. Before the fix these fail; after it they pass. *This is the only assertion that distinguishes the two builds.* + - **Negative control (anti-vacuity):** the same fixture maps one extra gRPC service **without** `AllowAnonymous`; a correctly-keyed call to it must be refused by the cookie challenge. If this control passes trivially, the fixture's auth stack is not live and every other assertion in the class is void. + - **Handler-identity guard:** assert the cookie scheme resolves to the real `CookieAuthenticationHandler`, not `AutoLoginAuthenticationHandler`. +- **`MeshGrpcAuthStackTests` (status fidelity, T5):** a **no-PSK** call under the real auth stack fails with the *interceptor's* detail string (`"Config-serve authentication failed."` / `"Telemetry stream authentication failed."` / `"Node command authentication failed."`), never a cookie challenge. ⚠️ Assert the **detail**, not only the status: `ConfigServeAuthInterceptor` uses `Unauthenticated` and an HTTP 401 maps to `Unauthenticated` too, so a status-only assertion passes on a broken build. Record the observed cookie-challenge signature (302 vs 401, and the resulting client-side status) in the plan's execution notes — it is the string the live gate greps for. +- **`MeshGrpcAnonymousMetadataTests`** (new): census as designed above — per-role endpoint sets, `IAllowAnonymous` on each, expected counts (admin-only: 2; driver-only: 2 or 3 with a sync port; fused with all three ports: 5), and the scope assertion. +- **`MeshGrpcDriverOnlyPipelineTests`** (new, T6): a driver-only shape (no `AddOtOpcUaAuth`, no `UseAuthentication`/`UseAuthorization`) serves `NodeCommandService` and `TelemetryStreamService` identically — pins the report's "site nodes unaffected" finding so that adding auth services to driver nodes fails a test instead of a fleet. +- **`MeshGrpcMapConventionTests`** (new, T7): source scan of `Program.cs`. +- **Regression:** `MeshGrpcBoundaryTests`, `ConfigServeListenerTests`, `LocalDbSyncListenerTests`, `TelemetryListenerTests` all stay green untouched (the mapper is behaviour-preserving; the boundary test keeps its own hand-built node pipeline, which is now honestly scoped to "the node side, without auth"). Full project: `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests`. + +**Live gate — docker-dev rig, and it MUST run the falsification control first.** + +The rig runs locally (OrbStack on the Mac); AdminUI `http://localhost:9200` round-robins central-1/central-2 via Traefik, so **both centrals must be rebuilt/recreated together**. LDAP is the shared GLAuth at `10.100.0.35:3893` (users `alice`/`bob`, password `password`; `multi-role` for the OPC UA data plane). + +*Step 0 — falsification control (pre-fix image; this is what makes the gate non-vacuous):* +```bash +# On master (WITHOUT the fix), flip the central pair to production-faithful auth: +OTOPCUA_DISABLE_LOGIN=false docker compose -f docker-dev/docker-compose.yml up -d central-1 central-2 +open http://localhost:9200 # EXPECT a login form — if it auto-authenticates, the flip did not take +curl -s -X POST http://localhost:9200/api/deployments \ + -H "X-Api-Key: docker-dev-deploy-key" -H "Content-Type: application/json" \ + -d '{"CreatedBy":"r3-01-control"}' # 202 Accepted (the deploy API is AllowAnonymous by design) +docker logs otopcua-dev-site-a-1-1 2>&1 | grep -iE "fetch|artifact|401|302|/login|Unauthenticated" | tail +docker logs otopcua-dev-central-1-1 2>&1 | grep -iE "ReportApply|ack|401|302" | tail +docker exec otopcua-dev-sql-1 /opt/mssql-tools18/bin/sqlcmd -C -S localhost -U sa -P 'OtOpcUa!Dev123' \ + -d OtOpcUa -I -Q "SELECT TOP 3 Id, Status, CreatedUtc FROM dbo.Deployment ORDER BY CreatedUtc DESC" +``` +**Required outcome: FAILURE.** Deployment `Status = 4` (TimedOut; `0 Dispatching / 1 AwaitingApplyAcks / 2 Sealed / 3 PartiallyFailed / 4 TimedOut`), site-node logs showing the artifact fetch rejected at the HTTP layer, no `NodeDeploymentState` rows for this revision. A green deploy here means the flip did not take effect and the whole gate proves nothing. *(If `mssql-tools18` is absent in the image, use `/opt/mssql-tools/bin/sqlcmd` without `-C`; `-I` is mandatory for this schema or every statement fails `Msg 1934`.)* + +*Step 1 — the fix:* +```bash +docker compose -f docker-dev/docker-compose.yml build central-1 central-2 site-a-1 site-a-2 site-b-1 site-b-2 +OTOPCUA_DISABLE_LOGIN=false docker compose -f docker-dev/docker-compose.yml up -d +``` +Sign in at `http://localhost:9200` as `alice`/`password` (proves the cookie stack is genuinely live), then repeat the headless deploy above. **Required outcome:** +1. Deployment reaches `Status = 2` (Sealed) — not 4. +2. `SELECT NodeId, RevisionHash, State FROM dbo.NodeDeploymentState` shows **all six** nodes on the new revision. +3. Site-node logs show a successful artifact fetch (SHA-256 verified) and an apply; no `/login`, 302 or 401 on any mesh path. +4. `/hosts` shows fresh driver health for all six nodes — **including central-1 and central-2**. ⚠️ Do **not** accept the `/alerts` live pill as the telemetry evidence: it is aggregate ("live when ≥1 node stream is up"), so it stays green while precisely the two fused-node streams are broken. The per-node rows are the discriminating observation. +5. A driver command from the AdminUI (`/hosts` → Restart driver) reaches a **central-pair** node — the central→own-pair `NodeCommandService` dial that step 0 broke. + +*Step 2 — restore:* `docker compose -f docker-dev/docker-compose.yml up -d central-1 central-2` (no env var ⇒ `DisableLogin` back to `true`); confirm `:9200` auto-authenticates again. Record everything in `archreview/plans/STATUS.md`. + +### Effort + Risk + +**Effort:** S code (one new file, one `Program.cs` edit, one compose line) + M verification (three new test classes; the rig session is the long pole, and it is run twice — once to fail, once to pass). +**Risk:** **low for the fix, medium for the refactor.** The behavioural change is strictly additive: five endpoints stop being subject to a policy that was never intended to gate them, and nothing else on the host loses protection (the census's scope assertion pins that). The real risks are (a) the map extraction touching the boot path — mitigated by landing it as a behaviour-preserving commit with the listener/boundary suites green **before** any convention is added, and (b) the empty-prefix route group changing gRPC routing for LocalDbSync — mitigated by the census (metadata) plus a routing assertion in T3, with the fallback position being a cross-repo `ZB.MOM.WW.LocalDb.Replication` change to mark its own endpoints anonymous. The one thing that must not be skipped is the step-0 falsification control: a fixed build and an unflipped rig are indistinguishable at step 1. + +--- + +## Task breakdown + +Branch: one branch `fix/archreview-r3-01-mesh-grpc-allowanonymous` off master. Ordering is strictly TDD: the refactor lands first with its scaffold green, then each guard is written to FAIL against the un-fixed convention and made to pass. T4–T7 are independent of each other once T3 lands. + +Verification bar: every task has a deterministic guard; T4/T5 additionally carry falsifiability controls; T10 is the merge-blocking live gate and is only meaningful if its step-0 control **fails** first. + +--- + +### T1 — Extract `MapOtOpcUaMeshGrpcServices` (behaviour-preserving) + census scaffold + +**Classification:** MEDIUM-RISK (boot-path refactor; no behaviour change) +**Estimated implement time:** 10 min +**Parallelizable with:** T8, T9; nothing else (every later task builds on the mapper) +**Files:** `src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/MeshGrpcEndpoints.cs` (new), `src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs`, `tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/MeshGrpcAnonymousMetadataTests.cs` (new) + +1. Write the scaffold census: build a `WebApplication` per role shape (admin-only / driver-only / fused-with-all-three-ports), call `MapOtOpcUaMeshGrpcServices`, enumerate `((IEndpointRouteBuilder)app).DataSources.SelectMany(d => d.Endpoints)`, and assert the **expected endpoint counts and service prefixes** per shape (no metadata assertion yet). Run `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests --filter "FullyQualifiedName~MeshGrpcAnonymousMetadataTests"` → **FAIL** (method does not exist). +2. Implement `MeshGrpcEndpoints` as a pure move of `Program.cs:743-784` — same gates, same order, **every existing comment carried over**. Replace the block in `Program.cs` with the single call. +3. Filter → **PASS**. Then the regression net: `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests --filter "FullyQualifiedName~MeshGrpcBoundaryTests|FullyQualifiedName~ConfigServeListenerTests|FullyQualifiedName~LocalDbSyncListenerTests|FullyQualifiedName~TelemetryListenerTests"` → **PASS unchanged**. +4. Commit: `refactor(host): extract MapOtOpcUaMeshGrpcServices (one authority for the five mesh maps)`. + +### T2 — Census asserts `IAllowAnonymous`; `.AllowAnonymous()` on the four gRPC maps + +**Classification:** HIGH-RISK (the Critical's fix; changes endpoint authorization metadata) +**Estimated implement time:** 5 min +**Parallelizable with:** T8, T9; after T1 +**Files:** `MeshGrpcEndpoints.cs`, `MeshGrpcAnonymousMetadataTests.cs` + +1. Extend the census: every mesh endpoint must carry `IAllowAnonymous` metadata; add the **guard-of-the-guard** (the census must have found ≥1 endpoint in each shape, with the exact expected count) and the **scope** assertion (only the five known service prefixes are anonymised). Filter → **FAIL** for all five endpoints. +2. Implement `.AllowAnonymous()` on `DeploymentArtifactGrpcService`, `CentralCommandGrpcService`, `TelemetryStreamGrpcService`, `NodeCommandGrpcService`, with the S14 rationale in the method's XML doc. +3. Filter → **four PASS, LocalDbSync still FAIL** (expected — T3 owns it). Commit: `fix(host): mesh gRPC endpoints opt out of the AdminUI cookie FallbackPolicy (archreview 03/S14)`. + +### T3 — LocalDbSync anonymous group (the non-chainable map) + +**Classification:** medium-risk (route-group/gRPC interaction on a mapped endpoint) +**Estimated implement time:** 5 min +**Parallelizable with:** T8, T9; after T2 +**Files:** `MeshGrpcEndpoints.cs`, `MeshGrpcAnonymousMetadataTests.cs` + +1. Add the assertion that the `localdb_sync.v1.LocalDbSync` endpoints carry `IAllowAnonymous` **and** that their route patterns are unchanged by the group (assert the raw pattern text still begins `/localdb_sync.v1.LocalDbSync/`). Filter → **FAIL**. +2. Implement `app.MapGroup(string.Empty).AllowAnonymous().MapZbLocalDbSync()`, with the comment recording *why* (the library's map returns `IEndpointRouteBuilder`, verified against 0.2.1 metadata, and marks nothing anonymous itself). +3. Filter → **PASS**; `LocalDbSyncListenerTests` → **PASS unchanged**. Commit: `fix(host): LocalDb sync endpoint opts out of the cookie fallback via an empty-prefix anon group`. + +### T4 — `MeshGrpcAuthStackTests`: a PSK call succeeds through the REAL auth stack + +**Classification:** HIGH-RISK verification (this is the guard that would have caught S14) +**Estimated implement time:** ~15 min (fixture plumbing) — the deliberately-larger task +**Parallelizable with:** T6, T7, T8, T9; after T3 +**Files:** `tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/MeshGrpcAuthStackTests.cs` (new) + +1. Fixture: `WebApplication` with `EnvironmentName = "Production"`, an InMemory `OtOpcUaConfigDbContext` (DataProtection's `PersistKeysToDbContext` dependency), `AddOtOpcUaAuth(config)` with `Security:Auth:DisableLogin=false`, `AddGrpc` with the production interceptors, `UseAuthentication()`/`UseAuthorization()`, `MapOtOpcUaMeshGrpcServices`, h2c Kestrel on a free port. Assert the resolved cookie-scheme handler is `CookieAuthenticationHandler` (not `AutoLoginAuthenticationHandler`). +2. Positive tests (run against **HEAD before T2** to see them FAIL, then against the fixed build): correctly-keyed `DeploymentArtifactService` fetch, `CentralCommandService.ReportApply`, and `NodeCommandService.Execute` each **succeed**. +3. Negative control: one extra gRPC service mapped **without** `AllowAnonymous` in the same fixture — a correctly-keyed call to it is refused by the cookie challenge. (If it succeeds, the fixture's auth stack is inert and the class proves nothing.) +4. Filter → **PASS**. Commit: `test(host): mesh gRPC boundary under the production auth stack (FallbackPolicy live)`. + +### T5 — Status fidelity: a no-PSK call surfaces the INTERCEPTOR, not a cookie challenge + +**Classification:** high-risk verification (the vacuity trap) +**Estimated implement time:** 5 min +**Parallelizable with:** T6, T7, T8, T9; after T4 +**Files:** `MeshGrpcAuthStackTests.cs` + +1. Add: a no-bearer call to each of the three services fails with the interceptor's **detail string** — `"Config-serve authentication failed."`, `"Telemetry stream authentication failed."`, `"Node command authentication failed."` — and a wrong-bearer call likewise. ⚠️ Assert the detail, **not** the status alone: `ConfigServeAuthInterceptor` throws `Unauthenticated` and `Grpc.Net.Client` maps HTTP 401 to `Unauthenticated`, so a status-only assertion is green on a broken build. +2. Record the observed cookie-challenge signature (302 to `/login` vs 401, and the client-side status/detail it produces) in a code comment and in this plan's execution notes — the live gate greps for it. +3. Filter → **PASS**. Commit: `test(host): no-PSK mesh calls report the interceptor's verdict, not a cookie 401/redirect`. + +### T6 — Driver-only pipeline control (pins "site nodes unaffected") + +**Classification:** standard (test-only) +**Estimated implement time:** 5 min +**Parallelizable with:** T4, T5, T7, T8, T9; after T3 +**Files:** `tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/MeshGrpcDriverOnlyPipelineTests.cs` (new) + +1. Build the driver-only shape (no `AddOtOpcUaAuth`, no `UseAuthentication`/`UseAuthorization`) and assert `NodeCommandService`/`TelemetryStreamService` behave identically to T4's fused shape — correctly-keyed call succeeds, no-key call gets the interceptor's status. Also assert the app has **no** `IAuthorizationPolicyProvider` fallback registered, which is *why* it is unaffected — a future "register auth everywhere" change then fails here rather than in a site outage. +2. Filter → **PASS**. Commit: `test(host): driver-only nodes carry no authorization middleware (03/S14 exemption pinned)`. + +### T7 — `Program.cs` source-scan convention guard + +**Classification:** standard (test-only) +**Estimated implement time:** 5 min +**Parallelizable with:** T4, T5, T6, T8, T9; after T1 +**Files:** `tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/MeshGrpcMapConventionTests.cs` (new) + +1. Following `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/FlushLoggingConventionTests.cs:28-45` (solution-root walk + `File.ReadAllText`): assert `Program.cs` contains **zero** `MapGrpcService<` and zero `MapZbLocalDbSync(` occurrences, and exactly one `MapOtOpcUaMeshGrpcServices(` call. Guard-of-the-guard: the located file must be non-empty and must contain `MapOtOpcUaMeshGrpcServices`, so a bad path cannot pass the test by finding nothing. +2. Filter → **PASS** (it would have FAILED at HEAD before T1 — note that in the test's remarks). Commit: `test(host): a mesh gRPC map added outside MeshGrpcEndpoints fails the build`. + +### T8 — docker-dev: make the production-faithful auth mode a one-variable flip + +**Classification:** standard (rig config; default behaviour unchanged) +**Estimated implement time:** 5 min +**Parallelizable with:** everything +**Files:** `docker-dev/docker-compose.yml` + +1. `Security__Auth__DisableLogin: "${OTOPCUA_DISABLE_LOGIN:-true}"` on central-1 (`:385`) and central-2 (`:511`); header-comment paragraph naming the flip, the LDAP users (`alice`/`bob`, password `password`, shared GLAuth `10.100.0.35:3893`), and **why** it exists (theme 2: every mesh live gate before R3-01 ran with the blocking middleware neutralized). +2. Verify `docker compose -f docker-dev/docker-compose.yml config` resolves to `"true"` with no env var set (byte-identical default) and to `"false"` with `OTOPCUA_DISABLE_LOGIN=false`. Commit: `chore(docker-dev): parameterise Security__Auth__DisableLogin on the central pair`. + +### T9 — Docs + +**Classification:** standard (docs) +**Estimated implement time:** 5 min +**Parallelizable with:** everything +**Files:** `docs/security.md`, `docs/Configuration.md`, `docs/Telemetry.md` + +Add "Mesh gRPC ingress is `AllowAnonymous` by design" to `docs/security.md`: what the cookie `FallbackPolicy` covers, why interceptors cannot gate what the middleware already rejected, the fail-closed property that makes anonymous safe, the affected-map table (incl. the fused-vs-driver-only distinction), and the two standing guards. One cross-reference line each in `docs/Configuration.md` §`MeshTransport` / §`ConfigSource`-`ConfigServe` and `docs/Telemetry.md`. Commit: `docs: mesh gRPC ingress is AllowAnonymous; the PSK interceptors are the gate (R3-01)`. + +### T10 — LIVE GATE: docker-dev with `DisableLogin=false` — operator/rig task + +**Classification:** HIGH-RISK verification gate (merge-blocking) +**Estimated implement time:** ~45 min rig session (two passes) +**Parallelizable with:** nothing (needs the built branch **and** a master build for the control) +**Files:** none (rig session; outcomes recorded in `archreview/plans/STATUS.md`) + +1. **Step 0 — falsification control on the PRE-FIX build**, per Finding 1's recipe: `OTOPCUA_DISABLE_LOGIN=false` on the central pair, headless deploy, **expect `Deployment.Status = 4` (TimedOut)** with the fetch/ack rejections in the node logs. A green deploy here voids the entire gate — stop and find out why the flip did not take. +2. **Step 1 — the fixed build**: rebuild all six nodes, sign in as `alice`, headless deploy ⇒ `Status = 2` (Sealed), `NodeDeploymentState` on the new revision for **all six** nodes, no `/login`/302/401 on any mesh path, `/hosts` fresh for central-1 **and** central-2 (not the aggregate `/alerts` pill — it cannot see the fused-node streams failing), and an AdminUI driver restart reaching a central-pair node. +3. **Step 2 — restore** `DisableLogin` to `true` (no env var) and confirm `:9200` auto-authenticates. +4. Record both passes — including the exact log signature the blocked calls produced — in `STATUS.md` and in this plan's execution notes. + +--- + +### Sequencing summary + +``` +T1 → T2 → T3 (fix, sequential — same two files) +T3 → T4 → T5 (real-auth-stack guard + status fidelity) +T3 → T6 (driver-only control) +T1 → T7 (source scan) +T8, T9 anytime +T10 last — and its step-0 control runs on master BEFORE the branch is merged +``` + +Overall effort: **~half a dev-day code+tests, plus the two-pass rig session.** The Critical itself is five lines; everything else in this plan exists because the five lines were invisible to a seven-phase live-gate program. diff --git a/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md.tasks.json b/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md.tasks.json new file mode 100644 index 00000000..35776954 --- /dev/null +++ b/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md.tasks.json @@ -0,0 +1,85 @@ +{ + "planPath": "archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md", + "lastUpdated": "2026-08-20", + "tasks": [ + { + "id": "T1", + "subject": "Extract MapOtOpcUaMeshGrpcServices from Program.cs:743-784 (behaviour-preserving) + endpoint census scaffold (03/S14)", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T2", + "subject": "Census asserts IAllowAnonymous + scope/count guard-of-the-guard; .AllowAnonymous() on the four MapGrpcService maps", + "status": "pending", + "blockedBy": [ + "T1" + ] + }, + { + "id": "T3", + "subject": "LocalDb sync endpoint anonymised via an empty-prefix MapGroup (MapZbLocalDbSync returns IEndpointRouteBuilder, not chainable)", + "status": "pending", + "blockedBy": [ + "T2" + ] + }, + { + "id": "T4", + "subject": "MeshGrpcAuthStackTests: PSK-bearing calls succeed through the REAL AddOtOpcUaAuth + UseAuthentication/UseAuthorization stack, with an un-anonymised negative control", + "status": "pending", + "blockedBy": [ + "T3" + ] + }, + { + "id": "T5", + "subject": "Status fidelity: a no-PSK call reports the interceptor's DETAIL string, not a cookie 401/302 (ConfigServe Unauthenticated collision makes status-only assertions vacuous)", + "status": "pending", + "blockedBy": [ + "T4" + ] + }, + { + "id": "T6", + "subject": "MeshGrpcDriverOnlyPipelineTests: driver-only nodes carry no authorization middleware — pins the 03/S14 exemption", + "status": "pending", + "blockedBy": [ + "T3" + ] + }, + { + "id": "T7", + "subject": "MeshGrpcMapConventionTests: Program.cs source scan — zero direct MapGrpcService/MapZbLocalDbSync sites, exactly one mapper call", + "status": "pending", + "blockedBy": [ + "T1" + ] + }, + { + "id": "T8", + "subject": "docker-dev: Security__Auth__DisableLogin parameterised as ${OTOPCUA_DISABLE_LOGIN:-true} on central-1/central-2 + header docs", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T9", + "subject": "docs/security.md \"Mesh gRPC ingress is AllowAnonymous by design\" + cross-refs in docs/Configuration.md and docs/Telemetry.md", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T10", + "subject": "LIVE GATE: docker-dev with DisableLogin=false — step-0 falsification control MUST fail on the pre-fix build (Deployment.Status=4), then the fixed build seals Applied on all six nodes; restore DisableLogin=true", + "status": "pending", + "blockedBy": [ + "T3", + "T4", + "T5", + "T6", + "T7", + "T8" + ] + } + ] +} diff --git a/archreview/plans/R3-04-one-role-authority-plan.md b/archreview/plans/R3-04-one-role-authority-plan.md new file mode 100644 index 00000000..be92c942 --- /dev/null +++ b/archreview/plans/R3-04-one-role-authority-plan.md @@ -0,0 +1,458 @@ +# Design + Implementation Plan — R3-04 One Role Authority + +- **Source reports:** `archreview/01-core-composition.md` §S-12 (`:198-210`) + §C-9 (`:264-278`) · `archreview/03-server-runtime.md` §C5 (`:131-133`) (2026-08-19 round-3 re-review at `12b0035a`) +- **Prior designs consumed:** none — both findings are **new in round 3**. The nearest ancestor is Gitea **#532** (`RoleParser.ResolveEffectiveRoles` + the `AddOtOpcUaCluster` PostConfigure), whose remediation created the second authority these findings are about; the field incident it closed is `docs/deployments/wonder-app-vd03-makino-z-34184.md`. +- **Plan verified against tree at:** master `7c77a8c7` (clean; code identical to `12b0035a`) — every anchor re-opened and re-checked 2026-08-20. +- **Scope:** OVERALL prioritized action item **#4** — "One role authority: `ResolveEffectiveRoles` normalizes + validates; `Program.cs` and all four validators consume it; boot guard asserting Akka and Host role views agree" (01 S-12/C-9, 03 C5). Effort Small / Impact Medium-High. +- **Cross-cutting requirement (OVERALL theme 4, "One rule stated twice diverges"):** the deliverable is not "fix two call sites" — it is that **the role list is derived in exactly one place and every consumer inherits it**, enforced by a guard test so the rule cannot be re-stated. The authoring-vs-deploy-gate arc (#535→#559) is the named precedent: one authority, enforced in the service, mirrored by the gate. Concrete pick here: a `EffectiveRoles` value type produced by one factory, consumed by `Program.cs`, the four `Cluster` validators, the Akka options PostConfigure and every gossip-side predicate; plus `RoleAuthorityGuardTests`, a source-scanning guard modelled on `CommandDisposalConventionsTests` / `StatusCodeParityTests`, that fails when any production file outside the authority reads `OTOPCUA_ROLES`, `Cluster:Roles`, or a bare `"admin"`/`"driver"` role literal. +- **Cross-cutting requirement (OVERALL theme 2, "verification that cannot see its own blind spot"):** the docker-dev rig **sets both role sources, consistently, on all six nodes** (`docker-compose.yml:298/352-354`, `:448/485-487`, `:564/606-607`, `:686/716-717`, `:782/803-804`, `:853/874-875`). Neither finding can reproduce on the rig as configured. The live gate below therefore **changes the rig** (site-b-2 becomes `Cluster:Roles`-only) so the divergence becomes observable — the same lesson 03/S14 taught about `DisableLogin=true`. + +## Verification summary + +Both findings **CONFIRMED at `7c77a8c7`** — neither stale, neither partially fixed. Every report anchor is **exact**; no drift. Anchor status: + +| Finding | Report anchor | Status at `7c77a8c7` | +|---|---|---| +| 01/S-12 = 03/C5 | `Program.cs:61-63` (`RoleParser.Parse(Environment.GetEnvironmentVariable("OTOPCUA_ROLES"))` → `hasAdmin`/`hasDriver`) | **Exact.** `:61` resolve, `:62` `roles.Contains("admin")`, `:63` `roles.Contains("driver")` — **bare string literals, not `RoleParser.Admin`/`.Driver`**, a detail neither report names. The env-only array reaches **four** further decisions the reports do not enumerate: the per-role appsettings overlay filename (`:80`), the health-check registration (`:564` `AddOtOpcUaHealth(hasAdmin)`), the three dedicated gRPC listener ports (`:624-626`), and the boot banner (`:789-790`). | +| 01/S-12 = 03/C5 | `RoleParser.cs:115-125` / `:115-119` (`ResolveEffectiveRoles`) | **Exact.** Two-arg overload `:115-119` (`configured is { Count: > 0 }` ⇒ verbatim copy; else `Parse(envRoles)`), one-arg convenience `:124-125`, `RolesEnvVar` const `:128`, `Parse` `:133-152`. | +| 01/S-12 = 03/C5 | `Cluster/ServiceCollectionExtensions.cs` PostConfigure | **Exact**, now pinpointed: `:51` — `options => options.Roles = RoleParser.ResolveEffectiveRoles(options.Roles)`, applied inside `AddOtOpcUaCluster` (`:34-51`). The gossiped list is handed to Akka at `:267` (`Roles = options.Roles`). | +| 01/S-12 = 03/C5 | `RoleParser.cs:96-103` ("the exact knob the wonder-app-vd03 field fix hand-added") | **Exact** — the `` paragraph is `:94-103`. | +| 01/C-9 | `SplitTopologyTransportValidator.cs:69-78` (the one validator that normalizes) | **Exact.** Comment `:69-72`; `configured` read `:73`; `ResolveEffectiveRoles` `:74`; **`normalized = Array.ConvertAll(effective, r => r.Trim().ToLowerInvariant())` `:76`**; classification `:78`, `:89-90`. | +| 01/C-9 | `MeshTransportOptionsValidator.cs:76-79` | **Exact** — `ResolveEffectiveRoles(GetSection("Cluster:Roles").Get())` `:76-77`; case-sensitive `Array.IndexOf(roles, RoleParser.Driver)` `:78`, `.Admin` `:79`. | +| 01/C-9 | `TelemetryOptions.cs:124-126` (`TelemetryOptionsValidator`, class at `:88`) | **Exact** — resolve `:124-125`, case-sensitive `IndexOf` `:126`. Note the sibling `TelemetryDialOptionsValidator` (`:160`) reads **no** roles at all. | +| 01/C-9 | `ConfigSourceOptionsValidator.cs:101-104` | **Exact** — resolve `:101-102`, case-sensitive `IndexOf` `:103-104`. | +| 01/C-9 | "the four Cluster fail-fast validators" | **Correct as a count, but the brief's fourth candidate is wrong.** `AkkaClusterOptionsValidator` (`AkkaClusterOptionsValidator.cs:37`) reads **no roles** — it validates self-first seed ordering only. The four role-reading validators are `SplitTopologyTransportValidator`, `MeshTransportOptionsValidator`, `TelemetryOptionsValidator`, `ConfigSourceOptionsValidator`; all four are registered in `Cluster/ServiceCollectionExtensions.cs:56/63/70/80`. | +| 01/C-9 | "only the env fallback is lowercased/validated by `Parse`" | **Exact and load-bearing.** `Parse` `:137-141` lower-cases + trims + dedupes and `:143-149` throws `ArgumentException` on an unknown role. The configured path (`:117`) does **none** of that: `Cluster:Roles: ["Drivr"]` is accepted silently, gossiped verbatim, and matches nothing. | + +**The blast radius is materially larger than either finding states, and the plan is sized to the larger one.** 01/C-9 describes the case hazard as "skips every driver-scoped boot-time rule". That is true and it is the *smaller* half. The same verbatim spelling is **gossiped to Akka** (`ServiceCollectionExtensions.cs:267`), and six production predicates match gossiped roles **ordinally** against the lowercase constant: `RedundancyStateActor.cs:146/193` (Primary election), `ManualFailoverService.cs:125`, `ClusterNodeAddressReconcilerActor.cs:185`, `DriverHostActor.cs:1612` (the R2-04 primary-gate member count), `ScriptedAlarmHostActor.cs:520`, and `Fleet.razor:149`. A pair configured `Cluster:Roles: ["Driver"]` therefore **elects no Primary at all** (`SelectOldestUpMemberOfRole(members, "driver")` matches nothing ⇒ every node Detached ⇒ the R2-04 data-plane gate denies every write, the alarm S&F queue never drains, ServiceLevel never rises) while `/hosts` cheerfully renders the role chip `Driver`. Akka's own `ClusterSingletonOptions.Role` matching (`ControlPlane/ServiceCollectionExtensions.cs:52` `AdminRole = "admin"`, `:247` `?? RoleParser.Driver`) is likewise ordinal and outside this repo's control — which is why the fix must normalize the **gossiped** plain-role strings, not only the local predicates. + +Supporting facts verified: `RoleParser.IsClusterRole` is already case-**insensitive** (`:37-40`, Gitea #532) while `ClusterIdFromRole` (`:53`) deliberately preserves the operator's suffix spelling, with an XML `` (`:44-52`) explicitly warning that normalizing one side "would silently rename the Akka role of a running deployment"; every in-repo ClusterId comparison is `OrdinalIgnoreCase` (`ConfigDbApiKeyStore.cs:144-152`, `DeploymentArtifact.ParseDataPlaneAuthority`, `AclScopeRef`); `ClusterRoleInfo` builds `_localRoles` as an `OrdinalIgnoreCase` HashSet (`ClusterRoleInfo.cs:45`) so `HasRole` is already case-insensitive but `LocalRoles` still exposes the verbatim strings; none of the four shipped `appsettings*.json` overlays carries a `Cluster` section (keys are `Serilog` + `Security` only), so the "the role overlay may not set `Cluster:Roles`" invariant this plan states costs zero migration; `Install-Services.ps1:177-182` writes **only** `OTOPCUA_ROLES` to the Windows service environment (`$normalisedRoles` is trimmed + sorted + deduped but **not** lower-cased) and `docs/deployments/wonder-app-vd03-makino-z-34184.md:46-60` records the field fix that hand-added `Cluster__Roles__0/1` beside it — so **prod runs both sources and `OTOPCUA_ROLES` must keep working**; the repo has an established source-scanning convention-test idiom (`CommandDisposalConventionsTests.cs:54-66`, walking up from `AppContext.BaseDirectory` to the `ZB.MOM.WW.OtOpcUa.slnx` root) and an established reflection-census idiom (`StatusCodeParityTests`, `PageAuthorizationGuardTests`, `DeferredSinkForwardingReflectionTests`); `EnvVarScope` + the `OTOPCUA_ROLES` xUnit collection (`tests/Core/…Cluster.Tests/EnvVarScope.cs:26-37`) already serialize every test class that touches the process env var — new tests must join that collection. + +### Role-reading census (every production site, `7c77a8c7`) + +"Source" is what the site actually reads. "Normalizes?" is whether **that site** trims/case-folds before deciding. `env` = `OTOPCUA_ROLES` via `RoleParser.Parse`; `cfg⇒env` = `ResolveEffectiveRoles` (configured-first, env fallback); `bound` = `AkkaClusterOptions.Roles` (post-`AddOtOpcUaCluster` PostConfigure, i.e. `cfg⇒env` verbatim); `gossip` = `Member.Roles` off live `Cluster.State`. + +| # | Site | Source | Normalizes? | What it decides | +|---|---|---|---|---| +| 1 | `Host/Program.cs:61` | **env only** | lower+validate (via `Parse`) | the whole host role view | +| 2 | `Host/Program.cs:62` `roles.Contains("admin")` | env only | n/a (bare literal, ordinal) | `hasAdmin` | +| 3 | `Host/Program.cs:63` `roles.Contains("driver")` | env only | n/a (bare literal, ordinal) | `hasDriver` | +| 4 | `Host/Program.cs:80` `roleSuffix` | env only | no (ordinal sort of already-lowered) | which `appsettings.{role}.json` overlay loads | +| 5 | `Host/Program.cs:158/172/374/414/435/477/482/520/586/590/596/730/747/755/780` | env only (via 2/3) | n/a | **every** conditional registration + endpoint map | +| 6 | `Host/Program.cs:564` `AddOtOpcUaHealth(hasAdmin)` | env only | n/a | the ConfigDb health check | +| 7 | `Host/Program.cs:624-626` | env only | n/a | LocalDb-sync / ConfigServe / Telemetry listener ports | +| 8 | `Host/Program.cs:789-790` banner | env only | n/a | the operator-visible boot line | +| 9 | `Cluster/ServiceCollectionExtensions.cs:51` PostConfigure | **cfg⇒env** | **no** | `AkkaClusterOptions.Roles` — the authority for everything below | +| 10 | `Cluster/ServiceCollectionExtensions.cs:267` `Roles = options.Roles` | bound | no | **what Akka gossips** | +| 11 | `Cluster/SplitTopologyTransportValidator.cs:73-76,89-90` | cfg⇒env | **yes** (trim + lower, `:76`) | split-topology transport fail-fast | +| 12 | `Cluster/MeshTransportOptionsValidator.cs:76-79` | cfg⇒env | **no** | GrpcPsk / CentralGrpcEndpoints fail-fast | +| 13 | `Cluster/TelemetryOptions.cs:124-126` (`TelemetryOptionsValidator`) | cfg⇒env | **no** | Telemetry ApiKey / listen-port fail-fast | +| 14 | `Cluster/ConfigSourceOptionsValidator.cs:101-104` | cfg⇒env | **no** | driver-only ⇒ `FetchAndCache` fail-fast | +| 15 | `Cluster/ClusterRoleInfo.cs:45` `_localRoles` | bound | **yes** for lookups (`OrdinalIgnoreCase` set); **no** trim; `LocalRoles` exposes verbatim | `HasRole`, `/fleet` + `/hosts` role chips | +| 16 | `Cluster/ClusterRoleInfo.cs:52` `ClusterRolesIn` | bound | prefix case-insensitive; suffix verbatim | `ClusterId` / `ClusterRole` identity | +| 17 | `Cluster/ClusterRoleInfo.cs:116/123/142/150` | **gossip** | no | fleet-wide role→member index | +| 18 | `ControlPlane/ServiceCollectionExtensions.cs:24,52` `AdminRole = "admin"` | literal ⇒ Akka | n/a (Akka matches **ordinally** vs gossip) | which node hosts the 5 admin singletons | +| 19 | `ControlPlane/ServiceCollectionExtensions.cs:246-247` | bound | prefix case-insensitive; falls back to `RoleParser.Driver` | redundancy-singleton cluster scope | +| 20 | `ControlPlane/Redundancy/RedundancyStateActor.cs:146,193` | **gossip** | **no** (`Roles.Contains("driver")`) | **Primary election** | +| 21 | `ControlPlane/Redundancy/ManualFailoverService.cs:125` | **gossip** | **no** | manual failover target set | +| 22 | `ControlPlane/Fleet/ClusterNodeAddressReconcilerActor.cs:185` | **gossip** | **no** | `ClusterNode` row reconciliation | +| 23 | `Runtime/Drivers/DriverHostActor.cs:1612` | **gossip** | **no** | R2-04 primary-gate driver member count | +| 24 | `Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs:520` | **gossip** | **no** | scripted-alarm emit gate member count | +| 25 | `Runtime/ServiceCollectionExtensions.cs:323` `GetService()` | bound (via 15/16) | inherits 15 | runtime actor cluster identity | +| 26 | `ControlPlane/ServiceCollectionExtensions.cs:175` `roleInfo.ClusterId/.ClusterRole` | bound (via 16) | inherits 16 | reconciler own-cluster scoping | +| 27 | `Host/OpcUa/ConfigDbApiKeyStore.cs:83` `ResolveClusterId` | bound | prefix case-insensitive; compared `OrdinalIgnoreCase` (`:150`) | which cluster's ACL grants this node honours | +| 28 | `Host/Program.cs:427` / `:494` | bound | inherits 27 / 19 | ACL cluster filter; redundancy-singleton scope | +| 29 | `Host/Health/HealthEndpoints.cs:82` `RolePreference = [Admin, Driver]` | literals ⇒ shared Health lib vs gossip | no | `/health/active` node preference | +| 30 | `AdminUI/Components/Pages/Fleet.razor:149,159` `MembersWithRole("driver")` | **gossip** | no (bare literal) | the `/fleet` "Akka drivers" column | +| 31 | `AdminUI/Components/Pages/Hosts.razor:480,487` | **gossip** | display only (`OrdinalIgnoreCase` sort) | the `/hosts` role + leader-role chips | +| — | `scripts/install/Install-Services.ps1:177-182` | writes **env only** | trim + sort + dedupe, **not** lower | the prod Windows-service role knob | +| — | `docker-dev/docker-compose.yml` ×6 nodes | writes **both**, consistently | n/a | the rig — and the reason neither finding reproduces there | + +**Census read:** 31 production sites. **8 read the env var only** (all in `Program.cs`, rows 1-8 — the whole of 01/S-12). **6 read `cfg⇒env`** and only **one of them normalizes** (rows 11-14 — the whole of 01/C-9, plus the PostConfigure at row 9 and the Akka handoff at row 10 which normalize nothing). **11 read the bound options**, mostly through `ClusterRoleInfo`, which is case-insensitive for lookups but verbatim for exposure. **6 match live gossip ordinally against a lowercase constant** (rows 17, 20-24, 30) — the half neither finding names, and the half that turns a miscased `Cluster:Roles` into a no-Primary outage. **2 deployment surfaces** set roles: prod (env only) and the rig (both). + +**How the two knobs relate today, exactly.** There is **no** mapping of `OTOPCUA_ROLES` into `Cluster:Roles` in configuration — no `AddEnvironmentVariables` prefix trick, no alias. The two are read independently: `Program.cs:61` reads the env var directly, and `AddOtOpcUaCluster:51` post-configures the *bound* `Cluster:Roles` with `ResolveEffectiveRoles`, which falls back to the env var only when the bound array is empty. So the **documented precedence — `Cluster:Roles` wins outright when non-empty, else `OTOPCUA_ROLES`** (`RoleParser.cs:104-109`, `docs/Configuration.md:126`) — governs **only the Akka side**. The Host side has no precedence at all: it sees the env var and nothing else. The two views coincide on every node that sets both consistently (all six docker-dev nodes, and wonder-app-vd03 after its field fix) and diverge on exactly the shape #532 made first-class: `Cluster:Roles` set, `OTOPCUA_ROLES` unset. What operators are told: `docs/Configuration.md:29` ("the roles themselves come from the `OTOPCUA_ROLES` env var"), `:124` (`Roles` … "when empty, falls back to `OTOPCUA_ROLES`"), `:126` (the two-knobs note, which **already tells operators to prefer `Cluster:Roles`** — i.e. the docs currently recommend the broken configuration), `:449` (the env-var table), `docs/ServiceHosting.md:19-27` (role gating is "`Program.cs` reads `OTOPCUA_ROLES`"), `docs/Redundancy.md:211-231` (per-node identity example uses `OTOPCUA_ROLES=admin,driver`), `docs/v2/Cluster.md:68` ("these two should stay in sync" — stated as an operator obligation, which is the whole defect). `infra/` says nothing about roles. + +### Open design choices — resolved in this plan + +1. **Precedence** (keep two sources + reconcile · vs · deprecate `OTOPCUA_ROLES` · vs · deprecate `Cluster:Roles`): resolved to **keep both, one documented precedence — `Cluster:Roles` non-empty wins outright, else `OTOPCUA_ROLES`, else empty**, i.e. `ResolveEffectiveRoles`'s existing rule applied to *both* sides instead of one. Deprecating `OTOPCUA_ROLES` is rejected outright: `Install-Services.ps1:182` writes it to the Windows service key, it is the only role knob on every non-field-fixed production install (memory `reference_servecli_wonder_app_vd03`: 23 env vars on the service key), and it names the appsettings overlay — a deprecation would break every existing deployment for a cosmetic gain. Deprecating `Cluster:Roles` is rejected because it is what Akka binds and what #532's own remediation made first-class. A **Warning** (not an error) when both are set and their normalized sets differ is added instead (choice 5). +2. **Case normalization scope** (normalize the whole gossiped list · vs · normalize only local predicates · vs · a two-layer value type): resolved to **two layers in one type**. `EffectiveRoles` carries a **`Gossiped` list** (what Akka receives) in which *plain* roles are lower-cased but a **cluster role keeps its ClusterId suffix verbatim** (`Cluster-SITE-A` ⇒ `cluster-SITE-A`), and exposes **case-insensitive predicates** (`HasAdmin`/`HasDriver`/`Has`) for every decision. Rationale: lower-casing a plain role changes the gossiped string **only for a node that is already broken** (a node spelled `driver` today is byte-identical after the change), while lower-casing a ClusterId suffix would rename `cluster-MAIN`/`cluster-SITE-A` on a running fleet — precisely what `RoleParser.ClusterIdFromRole`'s `` (`:44-52`) warns against, and a rolling upgrade of a pair would gossip two different role names mid-window and split it. Normalizing predicates *only* is insufficient because Akka's own `ClusterSingletonOptions.Role` match (rows 18/19) is ordinal and not ours to change. +3. **Structural identity vs a runtime assert** (the OVERALL action item's "boot guard asserting Akka and Host role views agree"): resolved to **make them the same object**, and the runtime assert is then **not written**. `Program.cs` resolves `EffectiveRoles` once and registers it as a singleton; `AddOtOpcUaCluster` sets `options.Roles = effectiveRoles.Gossiped` from that instance; the four validators take the instance by constructor injection. There is then no second derivation to disagree with, so an assert comparing two views would compare an object to itself — a test that can never fail is worse than no test, because it reads as coverage. What *is* kept is the one genuinely non-redundant residual check (choice 4). Note also that a literal "Akka gossip vs Host view" assert is not implementable where it would be wanted: gossiped roles do not exist until the `ActorSystem` is built, and resolving anything ActorSystem-shaped during composition is the Phase-6 boot-`StackOverflow` trap (CLAUDE.md; `ControlPlane/ServiceCollectionExtensions.cs:239-248`). +4. **The residual divergence surface — the config-layer ordering trap.** `Program.cs` must resolve roles **before** it adds the per-role overlay (`:80` needs the role names to pick the filename), so the authority reads the *pre-overlay* configuration (base `appsettings.json` + env + args). If a role overlay — or a `${secret:}` expansion — ever set `Cluster:Roles`, the post-overlay bind would differ from the instance every consumer holds. Resolved by **stating the invariant** ("`Cluster:Roles` may not be set in a role overlay") and enforcing it with a **narrow** fail-fast added to the existing `AkkaClusterOptionsValidator`: if the bound `Roles` (post-overlay) normalizes to a different set than the registered `EffectiveRoles`, fail host start naming both. This is 5 lines, is *not* the redundant assert of choice 3 (it compares two genuinely different config snapshots), and is a no-op today — none of the four shipped overlays carries a `Cluster` section. +5. **Unknown / miscased role handling** (silent · warn · fail): resolved to **fail host start on an unknown role from either source**, matching what `Parse` already does for the env path (`:143-149`) and closing the configured path's total absence of validation. The message follows the house validator style (`MeshTransportOptionsValidator.cs:49-55`): name the key, the offending value, the source it came from, and the allowed set. A *miscased but known* role does **not** fail — it normalizes silently, because failing it would brick every node an operator authored as `Driver` at exactly the moment they upgrade. Separately, a **Warning** fires when both sources are set and their normalized sets differ (the `Cluster:Roles`-wins case an operator almost certainly did not intend). +6. **Gossip-side predicates** (leave them ordinal · vs · one case-insensitive helper): resolved to a **shared `ClusterRoles.Has(IEnumerable gossipedRoles, string role)` helper** (`OrdinalIgnoreCase`) adopted at all six ordinal gossip sites (rows 20-24, 30). With choice 2 in place a *local* node can no longer gossip a miscased plain role, so this is **defence in depth for a peer running an older build** — precisely the rolling-upgrade window choice 2 is designed to survive. Cheap, and it removes the last bare `"driver"` literals from production code so the guard test (choice 7) can be strict. +7. **Guard style** (reflection census · vs · source scan · vs · Roslyn analyzer): resolved to a **source scan**, because the thing being guarded is a *textual* read (`Environment.GetEnvironmentVariable("OTOPCUA_ROLES")`, `GetSection("Cluster:Roles")`, `"driver"`) that reflection cannot see — the same limitation `StatusCodeParityTests`' own XML doc calls out ("an inline literal is invisible to this guard"). The repo already has the idiom and the root-walk helper (`CommandDisposalConventionsTests.cs:54-66`). A Roslyn analyzer was considered and rejected as disproportionate for two forbidden strings. +8. **Rig fidelity** (leave docker-dev alone · vs · make one node `Cluster:Roles`-only): resolved to **change site-b-2 to `Cluster:Roles`-only** (drop its `OTOPCUA_ROLES`). site-b is already the repo's designated A/B *control* node for the bootstrap guard, so it is the established place to pin a configuration variant, and site-b-2 is a driver-only node whose failure mode (deploy times out) is exactly what 01/S-12 predicts. Before the fix that node must **fail** the gate; after it, pass. Without this the gate is theatre — see theme 2. + +--- + +## Finding 1 — 01/S-12 = 03/C5: two role authorities can wire a node inconsistently (Medium, two independent reviewers) + +### Restatement + +`Program.cs:61-63` derives `hasAdmin`/`hasDriver` — the switches for the entire conditional host wiring, ~15 registration blocks, the health check, three gRPC listener ports and the appsettings overlay — **solely from `OTOPCUA_ROLES`**. The Akka cluster roles resolve configured-first through `RoleParser.ResolveEffectiveRoles` (`RoleParser.cs:115-119`), applied to the bound options by `AddOtOpcUaCluster`'s PostConfigure (`Cluster/ServiceCollectionExtensions.cs:51`). A node configured **only** through `Cluster:Roles` — the shape #532's own remediation made first-class, and the shape `docs/Configuration.md:126` currently *recommends* — joins the mesh advertising `driver` (and `cluster-*`), passes `SplitTopologyTransportValidator` and `ConfigSourceOptionsValidator` (which read effective roles), becomes an expected-ack member of every deployment via `ConfigPublishCoordinator`'s enabled-`ClusterNode`-rows set, and **times out every deployment forever** because no driver stack was registered: no `WithOtOpcUaRuntimeActors`, no LocalDb, no OPC UA endpoint, no ConfigDb, and the deny-all `NullOpcUaUserAuthenticator`. It can even be the oldest role-carrying member for singleton placement without running the singleton manager. This is the exact inverse of the #532 incident, and nothing cross-checks the two sources. + +### Verification + +Confirmed exact (see summary table). Precisely, at `7c77a8c7`: + +- `Program.cs:61` — `var roles = RoleParser.Parse(Environment.GetEnvironmentVariable("OTOPCUA_ROLES"));`, with the comment "Roles drive the entire conditional wiring below" one line above. Nothing else in `Program.cs` reads `Cluster:Roles`; the file's only other role reads are the *bound* `AkkaClusterOptions` at `:427` and `:494`, both for cluster **identity** (ClusterId / singleton scope), never for wiring. +- `:62-63` — `roles.Contains("admin")` / `roles.Contains("driver")` against **bare literals**, not `RoleParser.Admin`/`.Driver`. (A second, independent brittleness: these two literals are invisible to any constant-based census.) +- `:80` — the overlay filename is `string.Join('-', roles.OrderBy(...))`, so a `Cluster:Roles`-only node also loads **no role overlay** — a quiet second consequence neither report names. +- `Cluster/ServiceCollectionExtensions.cs:34-51` — the PostConfigure. `:267` hands `options.Roles` to Akka's `ClusterOptions`. +- Deploy-path consequence re-verified against CLAUDE.md's Phase-1 note: `ConfigPublishCoordinator` sources its expected-ack set from **enabled `ClusterNode` rows**, and `Enabled = 0` is rejected by `DraftValidator.ValidateClusterTopology`; so a mis-wired node cannot even be excluded without the `MaintenanceMode` hatch. "Times out every deploy" is fleet-scoped, not node-scoped. +- Rig blindness re-verified: all six docker-dev nodes set `OTOPCUA_ROLES` **and** `Cluster__Roles__*` to the same values (`:298`/`:352-354`, `:448`/`:485-487`, `:564`/`:606-607`, `:686`/`:716-717`, `:782`/`:803-804`, `:853`/`:874-875`). + +### Root cause + +`Program.cs`'s role read predates the cluster options entirely and was never revisited when #532 introduced the fallback. #532 closed the env-only→**no** Akka roles direction by teaching the *Akka* side about the env var; it did not teach the *Host* side about `Cluster:Roles`, so the fix created a second authority rather than collapsing to one. The structural reason it stayed hidden is ordering: `Program.cs` needs roles at line 61, before the host is built and before any options bind, so reaching for `Environment.GetEnvironmentVariable` is the path of least resistance — and the configuration object that could answer the question (`builder.Configuration`, available from line 66) is created five lines later. + +### Proposed design + +One authority, one object, consumed everywhere. + +**The type (`src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs`, new):** + +```csharp +/// The node's effective cluster roles — the SINGLE authority for "what is this node". +/// Resolved once, from one documented precedence, and consumed by BOTH the Host's conditional +/// wiring and the Akka role list, so the two cannot disagree (archreview 01/S-12 = 03/C5). +/// +/// Precedence: Cluster:Roles when non-empty, else OTOPCUA_ROLES, +/// else empty. Unchanged from — what changed is +/// that Program.cs now obeys it too, instead of reading the environment directly. +/// Normalization is two-layer, on purpose. lower-cases plain +/// roles (Akka matches gossiped role names ORDINALLY, and every consumer compares against a +/// lowercase constant) but keeps a cluster role's ClusterId suffix VERBATIM — lower-casing that +/// would rename cluster-SITE-A on a running fleet, and a rolling pair upgrade would gossip +/// two different role names mid-window. Lower-casing a plain role changes the wire only for a +/// node that is already broken. +/// +public sealed class EffectiveRoles +{ + public static EffectiveRoles Resolve(IConfiguration configuration); // the one factory + public static EffectiveRoles Resolve(IReadOnlyList? configured, string? envRoles); + + public IReadOnlyList Gossiped { get; } // what AkkaClusterOptions.Roles becomes + public RoleSource Source { get; } // Configured | Environment | None + public bool HasAdmin { get; } + public bool HasDriver { get; } + public bool HasDev { get; } + public bool Has(string role); // OrdinalIgnoreCase + public string? ClusterRole { get; } // verbatim suffix + public string? ClusterId { get; } +} +``` + +- `Resolve` **validates both sources**: every entry must be `admin`/`driver`/`dev` (case-insensitive after trim) or a well-formed `cluster-`; otherwise it throws with the house message shape — `"Cluster:Roles[1] = 'Drivr' is not a known role. Allowed: admin, driver, dev, or 'cluster-'."` — naming the key that supplied it. +- `RoleParser.ResolveEffectiveRoles` is **retained** as a thin delegation (`=> EffectiveRoles.Resolve(configured, env).Gossiped.ToArray()`) so nothing outside this plan's touched files breaks, and its XML doc points at the new authority. `RoleParser.Parse` stays as the env-string parser `Resolve` uses. + +**`Program.cs` (`:61-63`):** + +```csharp +// The SINGLE role authority: Cluster:Roles first, OTOPCUA_ROLES as the fallback (archreview 01/S-12). +// Resolved off builder.Configuration BEFORE the per-role overlay is added, because the overlay's +// FILENAME depends on the answer — which is why a role overlay may not itself set Cluster:Roles +// (AkkaClusterOptionsValidator fails start if one ever does). +var builder = WebApplication.CreateBuilder(args); +var effectiveRoles = EffectiveRoles.Resolve(builder.Configuration); +var hasAdmin = effectiveRoles.HasAdmin; +var hasDriver = effectiveRoles.HasDriver; +builder.Services.AddSingleton(effectiveRoles); +``` + +`WebApplication.CreateBuilder(args)` moves **above** the role read (it is currently at `:66`). `:80`'s `roleSuffix` uses `effectiveRoles.Gossiped`; `:789-790`'s banner gains `source={Source}` so the boot line states *which knob won* — the single cheapest operator-visible artifact of this change. + +**`AddOtOpcUaCluster` (`Cluster/ServiceCollectionExtensions.cs:34-51`):** the PostConfigure consumes the registered instance rather than re-deriving — + +```csharp +services.TryAddSingleton(sp => EffectiveRoles.Resolve(sp.GetRequiredService())); +services.AddOptions().PostConfigure( + (options, roles) => options.Roles = [.. roles.Gossiped]); +``` + +`TryAddSingleton` keeps `AddOtOpcUaCluster` usable from harnesses that never ran `Program.cs`. **This is the structural identity**: `AkkaClusterOptions.Roles` is now *by construction* the same list `hasDriver` was computed from — there is no second derivation, hence no assert (resolved choice 3). + +**Residual guard (`AkkaClusterOptionsValidator`, existing, `ValidateOnStart`):** if the bound `Roles` (post-overlay) normalizes to a different **set** than the injected `EffectiveRoles.Gossiped`, fail with `"Cluster:Roles was changed by a configuration layer applied after role resolution (host view: [driver], bound: [admin,driver]). A per-role appsettings overlay may not set Cluster:Roles."` No-op today (choice 4). + +**Alternatives considered:** +- *One-line fix: `Program.cs:61` calls `RoleParser.ResolveEffectiveRoles(builder.Configuration.GetSection("Cluster:Roles").Get())`* — the fix both reports propose. Rejected as the whole answer: it closes S-12 and leaves C-9, leaves the two bare `"admin"`/`"driver"` literals, leaves the configured path unvalidated, and — critically — leaves **two derivations of the same rule** (Program's and the PostConfigure's), which is the exact disease theme 4 names. It is, however, the correct *first commit* (T3 below) and is sequenced as such. +- *Deprecate `OTOPCUA_ROLES` with a warning* — rejected (choice 1). +- *Map `OTOPCUA_ROLES` into configuration as `Cluster:Roles` via a custom `IConfigurationSource`* — attractive (it would make one key, not two), but rejected: the env var is comma-separated while `Cluster:Roles` is an indexed array, so the mapping needs a bespoke provider whose ordering against `AddEnvironmentVariables`/`AddCommandLine` is subtle, and it would silently change what `Cluster:Roles` reads as in every diagnostic dump. A value type is simpler and states the precedence in code where a reader will find it. +- *A runtime boot guard comparing Akka gossip to the Host view* — rejected (choice 3), replaced by the narrower config-layer guard. + +### Implementation steps + +1. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs` (new) — the type, `Resolve` ×2, normalization, validation, `RoleSource` enum. +2. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs` — `ResolveEffectiveRoles` ×2 delegate to `EffectiveRoles`; XML docs re-point at the authority; `Parse` unchanged. +3. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs` — `TryAddSingleton`; PostConfigure at `:51` consumes it. +4. `src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs` — `:61-63` rewrite (builder first), `:80` overlay, `:789-790` banner + `source=`; register the singleton. +5. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs` — the post-overlay set-equality guard. + +### Tests + +- `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs` (new; joins the `OTOPCUA_ROLES` collection via `EnvVarScope`): precedence matrix (configured-only / env-only / both-agree / both-differ / neither); `Gossiped` normalization (`" Driver "` ⇒ `driver`; `Cluster-SITE-A` ⇒ `cluster-SITE-A` — **suffix case preserved**, the falsification control for choice 2); `HasDriver` true for every casing; unknown role from **either** source throws naming the key; `Source` reports the winner. +- `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs` (existing, 5 cases) — must stay green unchanged; it is the #532 regression net. +- `tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ClusterRolesOnlyBootTests.cs` (new, on the `DriverOnlyNoConfigDbBootTests` direct-host-build pattern — **not** `WebApplicationFactory`, per `TwoNodeClusterHarness.cs:48`): build the host with `Cluster:Roles = ["driver"]` and **`OTOPCUA_ROLES` unset**, assert the driver stack is present (`ILocalDb` singleton resolvable, `IOpcUaUserAuthenticator` is `ApiKeyOpcUaUserAuthenticator` not `NullOpcUaUserAuthenticator`, the runtime actors registered) and that `IOptions.Value.Roles` equals `["driver"]`. **Negative control:** the same test against the pre-fix build must FAIL — record that it does. +- `AkkaClusterOptionsValidatorTests` — one case: a post-role-resolution config layer setting `Cluster:Roles` fails start; the no-overlay case passes. + +### Effort + Risk + +**Effort:** S. **Risk:** **medium-high — this changes what every node wires and what it gossips.** The change is behaviour-preserving for every node that sets both sources consistently (all six rig nodes, every current prod install) and behaviour-*correcting* for the two broken shapes. The residual risks are (a) the `WebApplication.CreateBuilder` move ahead of the role read — mechanical but it reorders the first ten lines of the composition root, and (b) a node whose `Cluster:Roles` and `OTOPCUA_ROLES` genuinely differ today will now wire from `Cluster:Roles`, which is a real behaviour change the Warning in choice 5 is there to announce. Land T1-T3 behind the full `Cluster.Tests` + `Host.Tests` suites, and do not merge without the live gate (T13) — a role-wiring regression is a total node outage and the rig is the only place it shows. + +--- + +## Finding 2 — 01/C-9: role normalization is stated in one validator of four (Medium) + +### Restatement + +`SplitTopologyTransportValidator` normalizes roles (trim + `ToLowerInvariant`, `:76`) and its comment names the exact hazard: "the `Cluster:Roles` bind path is verbatim (no lowercasing)… so 'Driver' / 'Admin' cannot slip past the ordinal checks" (`:69-72`). Its three siblings do the opposite: `MeshTransportOptionsValidator.cs:78-79`, `TelemetryOptions.cs:126`, and `ConfigSourceOptionsValidator.cs:103-104` all run case-sensitive `Array.IndexOf(roles, RoleParser.Driver)` over `ResolveEffectiveRoles`, which returns configured roles **verbatim** (`RoleParser.cs:117` — only the env fallback is lowercased/validated by `Parse`). A node configured `Cluster:Roles: ["Driver"]` silently skips every driver-scoped boot-time rule (GrpcPsk-required, CentralGrpcEndpoints-required, FetchAndCache-required) and fails later at runtime with precisely the absence-class failures these validators exist to fail fast on. Nothing validates unknown or miscased plain role names on the configured path at all. + +### Verification + +Confirmed exact (see summary table), and **the report under-states the consequence**. The same verbatim string is gossiped (`Cluster/ServiceCollectionExtensions.cs:267`), and six production predicates match gossip ordinally against the lowercase constant — census rows 17, 20-24, 30. Concretely, a pair configured `Cluster:Roles: ["Driver", "cluster-SITE-A"]`: + +- `RedundancyStateActor.SelectOldestUpMemberOfRole(members, "driver")` (`:146`) matches **no member** ⇒ no Primary is ever elected ⇒ `IsDriverPrimary` false on both nodes ⇒ (R2-04) every operator write is denied `"not primary"`, alarm acks are dropped, the alerts fan-out never publishes, and the LocalDb alarm store-and-forward queue **never drains** (`HistorianDrainState.NotPrimary` on both). `ServiceLevel` never rises. +- `DriverHostActor.cs:1612` / `ScriptedAlarmHostActor.cs:520` count **0** driver members, so the R2-04 primary gate takes its single-node "allow" branch on a genuine 2-node pair — the exact dual-primary window R2-04 exists to close, silently re-opened by a spelling. +- `ClusterNodeAddressReconcilerActor.cs:185` sees no driver members ⇒ Error-logs `EnabledRowNotInCluster` for every row, forever. +- `/hosts` (`Hosts.razor:487`) renders the chip `Driver` and `/fleet` (`Fleet.razor:149`) shows zero Akka drivers — an operator sees a healthy-looking node in a fleet that reports none. + +Also verified: `AkkaClusterOptionsValidator` — the fourth validator the task brief guessed at — reads no roles at all; the four are as listed above. And `TelemetryDialOptionsValidator` (`TelemetryOptions.cs:160`) reads none, so the central/dial side is unaffected. + +### Root cause + +Two derivations of one rule, at two different times. `SplitTopologyTransportValidator` landed with Phase 6 (`344b0d33`) and normalized because its author reasoned about the two role sources explicitly; the other three copied the `ResolveEffectiveRoles` + `Array.IndexOf` shape without the normalization, which was the *newer* and less obvious half. Nothing structural connects the four — each re-derives "is this a driver node" from a raw array — so the correct spelling in one file is invisible to the next author. The deeper cause is the same as Finding 1's: normalization lives at the consumers instead of at the authority. + +### Proposed design + +**Delete the derivation from all four validators; inject the authority.** Each validator's ctor takes `EffectiveRoles` instead of (or alongside) `IConfiguration`, and the four role lines collapse to `roles.HasDriver` / `roles.HasAdmin`. `SplitTopologyTransportValidator` additionally uses `roles.ClusterRole` / `roles.ClusterId` in place of its private `FindIndex`/`ClusterIdFromRole` pair — its message keeps the operator's original spelling because `ClusterId` preserves the suffix verbatim by design (choice 2). Its `:59-72` comment block, which is now the *history* of a rule the authority states, shrinks to a one-line pointer. + +**Gossip-side defence in depth (choice 6).** Add to `RoleParser`: + +```csharp +/// True when a GOSSIPED role list carries , compared +/// case-insensitively. Akka compares role names ordinally and a PEER may be running an older +/// build that gossips a verbatim 'Driver' (archreview 01/C-9), so every predicate over +/// Member.Roles goes through here rather than List.Contains. +public static bool GossipedRolesHave(IEnumerable? gossipedRoles, string role); +``` + +Adopted at `RedundancyStateActor.cs:193`, `ManualFailoverService.cs:125`, `ClusterNodeAddressReconcilerActor.cs:185`, `DriverHostActor.cs:1612`, `ScriptedAlarmHostActor.cs:520`, `Fleet.razor:149`, and `SelectOldestUpMemberOfRole` (`RedundancyStateActor.cs:146`). `ClusterRoleInfo`'s gossip index (`:116/123/142/150`) becomes an `OrdinalIgnoreCase`-keyed dictionary. + +**The guard that stops it recurring (choice 7).** New `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs`, scanning `src/**/*.cs` + `src/**/*.razor` from the solution root (the `CommandDisposalConventionsTests.cs:54-66` root-walk): + +- **No file except `EffectiveRoles.cs` and `RoleParser.cs` contains `OTOPCUA_ROLES`** (outside a comment or XML doc). +- **No file except `EffectiveRoles.cs` contains `"Cluster:Roles"`** as a configuration key (outside a comment / an error-message string literal — the scan strips `//` and `///` lines and allows occurrences inside a string that also contains a space, which is how the validators' operator messages read). +- **No file except `RoleParser.cs` declares or compares a bare `"admin"` / `"driver"` / `"dev"` role literal** — the constants exist for this. This is the check that catches `Program.cs:62-63` and `Fleet.razor:149` today. +- Each rule reports **every** offending `file:line`, not the first, and the XML doc states the escape hatch (add the file to the small, named allow-list *with a reason*) so the guard is maintainable rather than deleted at the first inconvenience. + +**Alternatives considered:** +- *Normalize inside `ResolveEffectiveRoles` and leave the four validators alone* — the fix 01/C-9 itself proposes. Rejected as insufficient on its own: it fixes the four validators (they inherit) but does nothing for the six ordinal **gossip** predicates, which are the half with the outage-grade consequence, and it leaves four re-derivations in place for the next rule to diverge on. It is, however, subsumed — the authority *does* normalize; the validators are converted anyway so the derivation stops existing. +- *Make every gossip predicate case-insensitive and skip normalizing the gossiped list* — rejected: Akka's own singleton `Role` matching (rows 18/19) is ordinal and not ours to intercept, so a miscased node would still fail to host its singletons. +- *A Roslyn analyzer forbidding the literals* — rejected as disproportionate (choice 7); the repo's analyzer project exists but the payload here is two strings. +- *`StringComparer.OrdinalIgnoreCase` on the raw arrays at each of the four validators* — rejected: that is the rule stated four more times, which is the finding. + +### Implementation steps + +1. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs` — ctor takes `EffectiveRoles`; `:59-90` collapses; comment shrinks to a pointer. +2. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/MeshTransportOptionsValidator.cs` — same, `:71-79`. +3. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs` (`TelemetryOptionsValidator`) — same, `:119-126`. +4. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs` — same, `:95-104`. +5. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs` — `GossipedRolesHave`. +6. `RedundancyStateActor.cs:146,193` · `ManualFailoverService.cs:125` · `ClusterNodeAddressReconcilerActor.cs:185` · `DriverHostActor.cs:1612` · `ScriptedAlarmHostActor.cs:520` · `AdminUI/Components/Pages/Fleet.razor:149,159` · `ClusterRoleInfo.cs:116-150` — adopt it. +7. `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs:56/63/70/80` — the four `AddValidatedOptions` registrations resolve the new ctor dependency (no signature change at the extension level; the validators are DI-constructed). + +### Tests + +- `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleCaseVariantValidatorTests.cs` (new): a `[Theory]` over `"Driver"`, `" driver "`, `"DRIVER"`, `"Admin"`, `" cluster-SITE-A "` × **all four validators**, asserting each fires the *same* failure it fires for the canonical spelling. **The load-bearing case is the failure**, not the pass: each variant must produce a `ValidateOptionsResult.Fail` naming the same key, because a validator that never runs its driver branch also "passes". Add the mirrored canonical-spelling case as the control. +- `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs` (new, choice 7) — and it must be **shown to fail on the pre-fix tree** (`Program.cs:62-63` and `Fleet.razor:149` are live violations today); record the pre-fix offender list in the commit body. +- `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/GossipedRoleMatchTests.cs` (new): `GossipedRolesHave(["Driver"], "driver")` true; `(["cluster-SITE-A"], "cluster-site-a")` true; null/empty false. +- `tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests` — one `RedundancyStateActor` case where the member gossips `"Driver"` and a Primary is still elected (the outage this finding actually causes; a pure-unit test over the selection helper is sufficient and deterministic). +- Existing suites stay green unchanged: `SplitTopologyTransportValidatorTests` (16 cases incl. the two `OTOPCUA_ROLES`-source cases at `:238`/`:258`), `MeshTransportOptionsValidatorTests` (incl. `:308`/`:329`), `ConfigSourceOptionsValidatorTests`, `RoleParserTests`, `AkkaClusterOptionsRoleFallbackTests`. Their construction changes from `new XValidator(config)` to `new XValidator(EffectiveRoles.Resolve(config))` — mechanical, and it keeps them testing the validator rather than the resolution. + +### Effort + Risk + +**Effort:** S-M (the four validators are ~4 lines each; the gossip-predicate sweep is 8 files; the guard test is the bulk). **Risk:** low-medium. The validators' behaviour is strictly widened (they now fire on inputs they used to skip), which can only turn a silent misconfiguration into a loud one — but that means **a node an operator authored as `Cluster:Roles: ["Driver"]` will now refuse to start** if it lacks its driver-scoped keys, where before it booted and misbehaved. That is the intent, and it is exactly the kind of change the live gate must exercise before merge. The gossip-side helper is behaviour-preserving for every correctly-spelled fleet. + +--- + +## Docs to update + +| File | Change | +|---|---| +| `docs/Configuration.md:29` | "The roles themselves come from the `OTOPCUA_ROLES` env var" ⇒ "…from `Cluster:Roles`, falling back to `OTOPCUA_ROLES`" (the overlay is now chosen from the effective roles). | +| `docs/Configuration.md:124` (`Cluster:Roles` row) | Add: values are trimmed and (for plain roles) lower-cased; a cluster role's ClusterId keeps its spelling; an unknown role **fails host start**. | +| `docs/Configuration.md:126` (the two-knobs note) | **Rewrite.** It currently says the two knobs drive *different* things ("`OTOPCUA_ROLES` drives all of `Program.cs`'s conditional wiring; `Cluster:Roles` becomes the gossiped Akka roles") — that description is the defect. Replace with: one authority, one precedence, both consumers; the boot banner names the winning source; setting both to different values logs a Warning; a role overlay may not set `Cluster:Roles`. Keep the #532 history as a short parenthetical. | +| `docs/Configuration.md:449` (env-var table) | `OTOPCUA_ROLES` row: read by `EffectiveRoles.Resolve` only, as the fallback when `Cluster:Roles` is empty. | +| `docs/ServiceHosting.md:19-27` | "`Program.cs` reads `OTOPCUA_ROLES`" ⇒ resolves `EffectiveRoles`; the role table and the "Single-node dev / Production" line get the precedence sentence. | +| `docs/Redundancy.md:211-231` | The per-node identity example shows `OTOPCUA_ROLES=admin,driver`; add the `Cluster__Roles__0/1` equivalent and state that either works, both together must agree. | +| `docs/v2/Cluster.md:68` | Delete "these two should stay in sync" — the operator obligation this plan removes. | +| `scripts/install/Install-Services.ps1` | Doc-comment only: `-Roles` writes `OTOPCUA_ROLES`, which is now the documented fallback rather than the primary knob; no behaviour change (choice 1 — do not break it). | +| `CLAUDE.md` | One paragraph under the cluster/redundancy material: the role authority, the precedence, the guard test, and the "a role overlay may not set `Cluster:Roles`" invariant. | + +--- + +## Task breakdown + +Branch: one branch `fix/archreview-r3-04-one-role-authority` off master. Ordering is genuinely sequential for T1→T4 (each consumes the previous type), then the sweep and guards fan out. Verification bar: every task has a deterministic unit guard; T3, T4 and T9 additionally carry falsification controls (a test that must FAIL on the pre-fix tree); T13 is the merge-blocking live gate. Follow TDD: write the failing test, see it FAIL, implement, see it PASS. + +--- + +### T1 — `EffectiveRoles` value type: one factory, one precedence, two-layer normalization, both-source validation + +**Classification:** HIGH-RISK (defines the node's identity; every downstream decision inherits it) +**Estimated implement time:** 10 min +**Parallelizable with:** T8 (guard-test scaffolding only); NOT T2-T7 +**Files:** `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs` (new), `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs`, `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs` (new) + +1. Write `EffectiveRolesTests` per Finding 1's Tests list — precedence matrix, `Gossiped` normalization **including the `cluster-SITE-A` suffix-preservation control**, `Has*` case-insensitivity, unknown-role-throws from **both** sources, `Source`. Join the `OTOPCUA_ROLES` xUnit collection (`EnvVarScope`). +2. `dotnet test tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests --filter "FullyQualifiedName~EffectiveRolesTests"` → **FAIL** (no such type). +3. Implement `EffectiveRoles` + `RoleSource`; make `RoleParser.ResolveEffectiveRoles` (both overloads) delegate to it; leave `Parse` as the env-string parser. +4. Filter → **PASS**; then the full project: `dotnet test tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests` → **PASS** (`RoleParserTests` + `AkkaClusterOptionsRoleFallbackTests` are the #532 regression net and must not need edits). +5. Commit: `feat(cluster): EffectiveRoles — one role authority with one precedence + two-layer normalization (archreview 01/S-12, 01/C-9)`. + +### T2 — `AddOtOpcUaCluster` post-configures from the registered instance (structural identity) + +**Classification:** high-risk (changes what the node gossips) +**Estimated implement time:** 5 min +**Parallelizable with:** T8; after T1 +**Files:** `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs`, `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs` + +1. Add a case: with `Cluster:Roles = ["Driver", "Cluster-SITE-A"]`, the bound `AkkaClusterOptions.Roles` is `["driver", "cluster-SITE-A"]` (plain lowered, suffix preserved) and is **reference-equal in content** to the registered `EffectiveRoles.Gossiped`. Run → **FAIL**. +2. Implement: `TryAddSingleton` + `PostConfigure` replacing `:51`. +3. Filter + full `Cluster.Tests` → **PASS**. Commit: `fix(cluster): AkkaClusterOptions.Roles is the EffectiveRoles instance, not a second derivation`. + +### T3 — `Program.cs` consumes the authority (closes 01/S-12 = 03/C5) + +**Classification:** HIGH-RISK (every conditional registration in the composition root) +**Estimated implement time:** 10 min +**Parallelizable with:** nothing; after T2 +**Files:** `src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs` + +1. Regression gate BEFORE editing: `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests` → record green. +2. Implement: move `WebApplication.CreateBuilder(args)` above the role read; `EffectiveRoles.Resolve(builder.Configuration)`; `hasAdmin`/`hasDriver` from it; register the singleton; `:80` overlay from `Gossiped`; `:789-790` banner gains `source=`. +3. `dotnet build ZB.MOM.WW.OtOpcUa.slnx` → **PASS**; `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests` → **PASS unchanged**. +4. Commit: `fix(host): Program.cs derives roles from the one authority, not OTOPCUA_ROLES alone (archreview 01/S-12 = 03/C5)`. + +### T4 — The four role-reading validators consume the authority (closes 01/C-9) + +**Classification:** medium-risk (fail-fast rules now fire on inputs they used to skip) +**Estimated implement time:** 10 min +**Parallelizable with:** T5, T6, T7; after T2 +**Files:** `SplitTopologyTransportValidator.cs`, `MeshTransportOptionsValidator.cs`, `TelemetryOptions.cs`, `ConfigSourceOptionsValidator.cs`, `Cluster/ServiceCollectionExtensions.cs`, `tests/Core/…Cluster.Tests/RoleCaseVariantValidatorTests.cs` (new) + the four existing validator suites + +1. Write `RoleCaseVariantValidatorTests` (Theory: 5 spellings × 4 validators, each asserting the **same Fail** as the canonical spelling, plus the canonical control). Run → **FAIL for three of the four** (SplitTopology already normalizes — that asymmetry *is* the finding, and it makes the first run a built-in falsification control). +2. Implement: each validator's ctor takes `EffectiveRoles`; the `ResolveEffectiveRoles` + `Array.IndexOf` pairs collapse to `roles.HasDriver` / `roles.HasAdmin`; `SplitTopologyTransportValidator` uses `roles.ClusterRole`/`.ClusterId`; update the four existing test suites' construction. +3. Filter → **PASS**; full `Cluster.Tests` → **PASS**. Commit: `fix(cluster): all four role-scoped validators read the authority — 'Driver' no longer skips every driver rule (archreview 01/C-9)`. + +### T5 — Gossip-side predicates: `GossipedRolesHave` at the six ordinal sites + +**Classification:** medium-risk (Primary election + the R2-04 gate's member count) +**Estimated implement time:** 10 min +**Parallelizable with:** T4, T6, T7; after T1 +**Files:** `RoleParser.cs`, `RedundancyStateActor.cs`, `ManualFailoverService.cs`, `ClusterNodeAddressReconcilerActor.cs`, `DriverHostActor.cs`, `ScriptedAlarmHostActor.cs`, `ClusterRoleInfo.cs`, `AdminUI/Components/Pages/Fleet.razor`, `tests/Core/…Cluster.Tests/GossipedRoleMatchTests.cs` (new), a `RedundancyStateActor` selection test + +1. Tests: `GossipedRolesHave` truth table; and a `RedundancyStateActor` selection case where the member gossips `"Driver"` and a Primary **is** elected. Run → **FAIL**. +2. Implement the helper and adopt it at all seven sites (incl. `SelectOldestUpMemberOfRole`); `ClusterRoleInfo`'s gossip index becomes `OrdinalIgnoreCase`-keyed. +3. Filters → **PASS**; run `Cluster.Tests`, `ControlPlane.Tests`, `Runtime.Tests`. Commit: `fix(cluster): gossiped-role predicates compare case-insensitively — a miscased peer no longer un-elects the Primary`. + +### T6 — Both-sources-disagree Warning + `source=` in the boot banner + +**Classification:** standard (observability) +**Estimated implement time:** 5 min +**Parallelizable with:** T4, T5, T7; after T3 +**Files:** `EffectiveRoles.cs`, `Program.cs`, `EffectiveRolesTests.cs` + +1. Test: both sources set to *different* normalized sets ⇒ the resolve reports a diagnostic the caller logs (return it on the type — do **not** log from a static factory; `Program.cs` logs it after Serilog is configured). Run → **FAIL**. +2. Implement + emit at boot. Filter → **PASS**. Commit: `feat(host): warn when Cluster:Roles and OTOPCUA_ROLES disagree; boot banner names the winning source`. + +### T7 — Post-overlay config-layer guard in `AkkaClusterOptionsValidator` + +**Classification:** standard (narrow fail-fast; no-op today) +**Estimated implement time:** 5 min +**Parallelizable with:** T4, T5, T6; after T2 +**Files:** `AkkaClusterOptionsValidator.cs`, `tests/Core/…Cluster.Tests/AkkaClusterOptionsValidatorTests.cs` + +1. Test: a configuration layer applied after role resolution changes `Cluster:Roles` ⇒ `Fail` naming both views; the ordinary case ⇒ `Success`. Run → **FAIL**. +2. Implement (set-equality against the injected `EffectiveRoles.Gossiped`). Filter → **PASS**. Commit: `fix(cluster): fail start when a late config layer changes Cluster:Roles (role overlays may not set it)`. + +### T8 — `RoleAuthorityGuardTests` — the rule cannot be stated twice again + +**Classification:** high-risk verification (this is the deliverable of theme 4, not a nicety) +**Estimated implement time:** 15 min (the one deliberately-larger task — source scan + allow-list + comment stripping) +**Parallelizable with:** T1-T7 for scaffolding; the assertions only go green after T3+T4+T5 +**Files:** `tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs` (new) + +1. Write the three rules per Finding 2 (no `OTOPCUA_ROLES` outside `EffectiveRoles.cs`/`RoleParser.cs`; no `"Cluster:Roles"` key outside `EffectiveRoles.cs`; no bare `"admin"`/`"driver"`/`"dev"` role literal outside `RoleParser.cs`), scanning `src/**/*.cs` + `src/**/*.razor` from the `.slnx` root, stripping `//`/`///` lines, reporting **every** offender as `file:line`, with a named allow-list each entry of which carries a reason. +2. Run on the pre-T3 tree → **FAIL**, listing at minimum `Program.cs:61,62,63` and `Fleet.razor:149,159`. **Paste that list into the commit body** — it is the proof the guard can fail. +3. After T3-T5, run → **PASS**. Commit: `test(cluster): guard that role reads live only in the authority (archreview theme 4)`. + +### T9 — `Cluster:Roles`-only node gets the driver stack (integration) + +**Classification:** high-risk verification (the exact shape 01/S-12 describes) +**Estimated implement time:** 15 min (harness plumbing) +**Parallelizable with:** T5-T8; after T3 +**Files:** `tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ClusterRolesOnlyBootTests.cs` (new) + +1. Build the host on the `DriverOnlyNoConfigDbBootTests` direct-host-build pattern with `Cluster:Roles = ["driver"]` and `OTOPCUA_ROLES` **unset** (via `EnvVarScope(null)`); assert the driver stack is registered (`ILocalDb`, `ApiKeyOpcUaUserAuthenticator` not the Null one, runtime actors) and `AkkaClusterOptions.Roles == ["driver"]`. +2. **Negative control:** `git stash` T3's change (or run against master) → the test must **FAIL** with the Null authenticator / missing `ILocalDb`. Record it. +3. `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests --filter "FullyQualifiedName~ClusterRolesOnlyBootTests"` → **PASS**. Commit: `test(integration): a Cluster:Roles-only node wires the driver stack`. + +### T10 — Docs sweep + +**Classification:** standard (docs) +**Estimated implement time:** 15 min +**Parallelizable with:** all +**Files:** per the "Docs to update" table + +Rewrite `docs/Configuration.md:126` (the note that currently documents the defect as design), plus the seven other rows. Commit: `docs: one role authority — precedence, normalization, and the overlay invariant (R3-04)`. + +### T11 — docker-dev rig: make the divergence observable + +**Classification:** standard (rig config) — **but it is what makes T13 non-vacuous** +**Estimated implement time:** 5 min +**Parallelizable with:** T10; before T13 +**Files:** `docker-dev/docker-compose.yml`, `docker-dev/README.md` + +1. Drop `OTOPCUA_ROLES` from **site-b-2** (keep its `Cluster__Roles__0/1`), and add a comment naming this as the `Cluster:Roles`-only pin for archreview 01/S-12 — site-b is already the rig's A/B control node. Leave the other five nodes as they are (they are the "both sources agree" control). +2. Add a `docker-dev/README.md` row saying so, and that the node is **expected to fail deploys on a pre-R3-04 build**. +3. Commit: `chore(docker-dev): site-b-2 is Cluster:Roles-only — the rig can now see the two-authority split`. + +### T12 — Optional: a miscased pin + +**Classification:** standard (rig config) +**Estimated implement time:** 5 min +**Parallelizable with:** T11; before T13 +**Files:** `docker-dev/docker-compose.yml` + +Spell **site-a-2**'s `Cluster__Roles__0` as `Driver` (capital D), keeping `cluster-SITE-A` verbatim, as the standing 01/C-9 pin. Expected post-fix: site-a still elects exactly one Primary and `/hosts` shows a lowercase `driver` chip on both nodes. Pre-fix: site-a elects none. Commit: `chore(docker-dev): site-a-2 pins a miscased Cluster:Roles entry (archreview 01/C-9)`. + +### T13 — LIVE GATE: docker-dev six-node rig — operator/rig task + +**Classification:** HIGH-RISK verification gate (merge-blocking) +**Estimated implement time:** ~40 min rig session +**Parallelizable with:** nothing (needs the built branch + T11/T12) +**Files:** none (rig session; findings recorded in `archreview/plans/STATUS.md`) + +**How roles are set on the rig today:** every node sets `OTOPCUA_ROLES` **and** `Cluster__Roles__*` to the same values — central-1/2 `admin,driver,cluster-MAIN` (`:298`/`:352-354`, `:448`/`:485-487`), site-a-1/2 `driver,cluster-SITE-A` (`:564`/`:606-607`, `:686`/`:716-717`), site-b-1/2 `driver,cluster-SITE-B` (`:782`/`:803-804`, `:853`/`:874-875`). T11/T12 make two of them divergent on purpose. Login is disabled on the rig, so drive the AdminUI directly at `http://localhost:9200` (round-robins central-1/central-2 — **rebuild both**). + +1. **Pre-fix baseline (on master, with T11/T12's compose applied).** `docker compose -f docker-dev/docker-compose.yml up -d --build` → confirm the failure is real: site-b-2's log shows the boot banner with **no** roles (`roles=[]`), `/hosts` shows it gossiping `driver` while `/fleet` counts it, and a fleet deploy **seals `TimedOut`** naming site-b-2 as silent. site-a shows **no Primary** (both nodes' `Server_ServiceLevel` low; `/hosts` shows no `driver` leader role). Record both — without this step the gate proves nothing. +2. **Build the branch:** `docker compose -f docker-dev/docker-compose.yml up -d --build` on all six. +3. **Role parity, per node:** `docker logs otopcua-dev--1 | grep "starting with roles"` → each node's banner names the same role set its compose entry declares, with `source=Configured`, and the two divergent nodes report the *config* roles (site-b-2 `driver,cluster-SITE-B`; site-a-2 `driver,cluster-SITE-A` — **lower-cased**). Cross-check against `/hosts`, which renders the **gossiped** `m.Roles` per member (`Hosts.razor:487`): the chips must match the banners exactly, on all six. That comparison — banner (Host view) vs `/hosts` chips (Akka view) — is the live form of the "two views agree" guard. +4. **site-a Primary election (01/C-9):** `/hosts` shows exactly one `driver` leader role in the SITE-A pair; Client.CLI `read` of `Server_ServiceLevel` on `opc.tcp://localhost:4842` vs `:4843` shows the 250/240 split. Pre-fix this leg showed neither. +5. **Deploy seals Applied (01/S-12):** deploy a config from `/fleet`; it must seal **`Applied`** with an ack from **all six** nodes including site-b-2 (pre-fix: `TimedOut`). Confirm site-b-2 actually serves — Client.CLI `browse -u opc.tcp://localhost:4845` returns the deployed tree. +6. **Fail-fast still fails (T4 regression):** temporarily set site-b-1's `Cluster__Roles__0` to `Drivr`, `docker compose up -d site-b-1` → the container must **exit** with the unknown-role message naming `Cluster:Roles`. Revert. +7. Record every leg in `archreview/plans/STATUS.md` with the pre-fix baseline beside the post-fix result. + +--- + +### Sequencing summary + +``` +T1 → T2 → T3 → T9 (the authority, then the Host, then its integration proof) + ↘ T4, T5, T6, T7 (validators / gossip predicates / warning / overlay guard — parallel after T2) +T8 scaffolded early; green only after T3+T4+T5 +T10 anytime +T11, T12 (rig config) before T13; T13 last, after everything is merge-ready +``` + +Overall effort: **~1 dev-day code+unit, plus the rig session.** The three HIGH-RISK slices (T1 identity type, T3 composition root, T8 the guard) each land behind their full project suites before anything stacks on them; T13 is merge-blocking and is only meaningful because T11/T12 gave the rig a node that can actually exhibit the defect. diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json new file mode 100644 index 00000000..52d0a593 --- /dev/null +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -0,0 +1,112 @@ +{ + "planPath": "archreview/plans/R3-04-one-role-authority-plan.md", + "lastUpdated": "2026-08-20", + "tasks": [ + { + "id": "T1", + "subject": "EffectiveRoles value type: one factory, one precedence (Cluster:Roles > OTOPCUA_ROLES), two-layer normalization, both-source validation (01/S-12, 01/C-9)", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T2", + "subject": "AddOtOpcUaCluster post-configures AkkaClusterOptions.Roles from the registered EffectiveRoles instance (structural identity, no second derivation)", + "status": "pending", + "blockedBy": [ + "T1" + ] + }, + { + "id": "T3", + "subject": "Program.cs derives hasAdmin/hasDriver/roleSuffix/banner from the authority, not OTOPCUA_ROLES alone (01/S-12 = 03/C5)", + "status": "pending", + "blockedBy": [ + "T2" + ] + }, + { + "id": "T4", + "subject": "All four role-scoped validators consume EffectiveRoles: SplitTopology/MeshTransport/Telemetry/ConfigSource (01/C-9)", + "status": "pending", + "blockedBy": [ + "T2" + ] + }, + { + "id": "T5", + "subject": "RoleParser.GossipedRolesHave adopted at the six ordinal gossip predicates + ClusterRoleInfo index (Primary election, R2-04 member count, /fleet)", + "status": "pending", + "blockedBy": [ + "T1" + ] + }, + { + "id": "T6", + "subject": "Warning when Cluster:Roles and OTOPCUA_ROLES disagree; boot banner names the winning source", + "status": "pending", + "blockedBy": [ + "T3" + ] + }, + { + "id": "T7", + "subject": "AkkaClusterOptionsValidator fails start when a late config layer changes Cluster:Roles (role overlays may not set it)", + "status": "pending", + "blockedBy": [ + "T2" + ] + }, + { + "id": "T8", + "subject": "RoleAuthorityGuardTests: source scan proving no production file outside the authority reads OTOPCUA_ROLES, Cluster:Roles, or a bare role literal", + "status": "pending", + "blockedBy": [ + "T3", + "T4", + "T5" + ] + }, + { + "id": "T9", + "subject": "ClusterRolesOnlyBootTests: a Cluster:Roles-only node wires the driver stack (with a pre-fix negative control)", + "status": "pending", + "blockedBy": [ + "T3" + ] + }, + { + "id": "T10", + "subject": "Docs sweep: Configuration.md roles section (rewrite the two-knobs note), ServiceHosting.md, Redundancy.md, v2/Cluster.md, install script comment, CLAUDE.md", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T11", + "subject": "docker-dev: site-b-2 becomes Cluster:Roles-only so the rig can exhibit the two-authority split (theme 2 rig-fidelity)", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T12", + "subject": "docker-dev: site-a-2 pins a miscased Cluster__Roles__0 = 'Driver' as the standing 01/C-9 pin", + "status": "pending", + "blockedBy": [] + }, + { + "id": "T13", + "subject": "LIVE GATE: docker-dev six-node rig — pre-fix baseline, per-node banner vs /hosts gossip parity, site-a Primary election, deploy seals Applied incl. site-b-2, unknown-role fail-fast", + "status": "pending", + "blockedBy": [ + "T3", + "T4", + "T5", + "T6", + "T7", + "T8", + "T9", + "T11", + "T12" + ] + } + ] +}