# 0018. UI backend: the read and approval server implementation

Status: Accepted, amended five times · Date: 2026-10-25 · Design section: CLI and UI, roadmap item 5

Decided: One `Server` type implements all three handler interfaces over a single uncached client; settled the four approval refusals; fixed `loadBundle` resolving the TalosCluster's own namespace instead of the backend's, the same bug ADR 0011 had already fixed on the controller side; granted the `nodes` get/list `findKubernetesNode` had always read without it; added a chart-vs-`config/` RBAC drift test after the chart's own copy of this `ClusterRole` was found silently narrower than `config/rbac/ichor-ui/role.yaml`; extended that drift test to the controller's `manager-role` and `manager-secrets-role`, which found and fixed missing `talosoperations`, Argo CD `applications` and metrics-cert Secret grants

## Context

ADR 0016 made `ClusterService`, `MachineService` and `NodeService` exist as
compiling Go interfaces (`ichorv1alpha1connect.*Handler`) and committed the
generated code under `gen/go/`. Nothing implemented them. This unit is that
implementation, registered on `cmd/ichor-ui`: nine RPCs across the three
services, the Talos-reading half of `NodeService`, secret masking for
`GetEffectiveConfig`, a resumable log stream, graceful termination, and a
UI-backend-only RBAC `ClusterRole` that stays out of the controller's
`manager-role`.

Versions consulted: `connectrpc.com/connect` v1.21.0 and
`github.com/siderolabs/talos/pkg/machinery` v1.14.3, both already pinned in
`go.mod`. Everything below was checked against the vendored module source
in the Go module cache (`go.yaml.in/yaml/v4`'s actual decode behavior,
machinery's `config.Provider.RedactSecrets`, the COSI resource shapes under
`pkg/machinery/resources/{hardware,block,network,perf}`) rather than taken
on faith from documentation; no WebFetch against talos.dev was needed since
the module source answered every question this unit ran into. Context7 and
Serena were unavailable for this session, as noted by the task; `rg` and
direct reads of the module cache stood in for both.

## Decision

### One `Server` type, three handler interfaces

`internal/uiapi.Server` (`server.go`) embeds `client.Client` and implements
`ClusterServiceHandler`, `MachineServiceHandler` and `NodeServiceHandler` on
the same type. All three read the same two CRDs (`TalosCluster`,
`TalosMachine`) through the same client; splitting them into three Go types
would only have produced three copies of the same `Get`/`List` plumbing.
`cmd/ichor-ui/main.go` constructs one `Server` and registers all three
generated handlers on one `http.ServeMux`.

The Kubernetes client is a plain, uncached `client.New(cfg, ...)`, not a
`ctrl.Manager`'s cached/informer-backed one: this backend is a stateless,
request-driven reader, not a reconciler, so there is no watch loop to
justify an informer cache, and avoiding one keeps the RBAC `ClusterRole`
honest about what it actually needs (`get`/`list`, never `watch`).

### Approval's four refusals (`machine_service.go`)

`ApproveMachine` refuses, in order, before it writes anything:

1. **Already approved** -- `CodeFailedPrecondition`, no mutation at all.
   Re-approval does not try to detect "did anything actually change"; it
   refuses outright, so "approving an already-approved machine causes no
   reconfiguration" holds by construction rather than by a diff that could
   itself be wrong.
2. **Weak identity** -- re-applies ADR 0011's rule (UUID, serial and at
   least one permanent MAC all present) to the `TalosMachine`'s
   already-recorded `spec.identity`, via
   `ichorv1alpha1.TalosMachineIdentity.IsStrongIdentity` (`identity.go`
   itself was later deleted once the rule moved there; see ADR 0033).
   This is a deliberate second *check*, not a second *rule*:
   `internal/controller`'s own call site converts its own
   `HardwareIdentity` to the CRD type and calls the same method, and
   discovery refusing to auto-register a weak identity does not
   guarantee every `TalosMachine` this server ever sees went through
   discovery (a future registration path, or a hand-written fixture, might
   not have).
3. **`MACHINE_ROLE_UNSPECIFIED`** -- `CodeInvalidArgument`. The wire enum's
   zero value is refused rather than stored as an empty
   `TalosMachineSpec.spec.role`: `pkg/render.BuildConfig`'s call to
   machinery's `machine.ParseType` does not itself reject an empty role (an
   empty role resolves to a worker silently, machinery's own choice, not
   this package's to rely on changing), so the check is explicit here and
   again in `GetEffectiveConfig` rather than assumed downstream.

`RejectMachine` refuses to touch an already-approved machine
(`CodeFailedPrecondition` -- "reject" has no meaning post-approval; removal
is a different, later operation) and, for a pending machine, deliberately
mutates nothing and deletes nothing. Deleting it was the real risk design
wanted guarded against: ADR 0011's "same machine returning" branch only
short-circuits for an object that still exists under its own name, so
deleting a rejected machine would let the next discovery pass recreate it
as fresh and pending, making the rejection meaningless the moment discovery
runs again. Leaving the object alone, with its `Approved: false` untouched,
is the whole fix.

**Gap to report**: the proto has no "rejected" state distinct from
"pending." `RejectMachineResponse` carries only an `AuditRecord`; the
`Machine` message's `approved bool` cannot represent "explicitly turned
away" versus "never looked at," so a rejected machine looks identical to a
freshly discovered one in `ListMachines`/`GetMachine`. Recording that
durably (an annotation, or a proto field) is a contract decision, left to
the design per the task's instruction not to add fields speculatively.

### Audit (`AuditRecord`)

`actor` is the fixed string `"ichor-ui"` (`machine_service.go`'s `actor`
constant), not a per-caller identity: there is no authentication yet
(OIDC/impersonation is roadmap phase 9), so anything else would be
fabricated. `AuditRecord` is returned to the caller and nothing more --
not also written as a Kubernetes `Event` or any other durable log. Adding
an `EventRecorder` would mean wiring a `client-go` broadcaster this backend
has no other use for, purely to duplicate what the response already states;
skipped under ponytail's "does this need to exist."

### Secrets masking is machinery's job, not a path denylist

`effectiveconfig.go` does not classify any field path as secret by hand.
`pkg/render.BuildConfig` (new, exported from the previous `Render`'s body;
see below) returns the merged `config.Provider` before encoding, and
`effectiveConfigValues` renders it twice -- once plain, once through
`Provider.RedactSecrets(secretMask)` -- then flattens both to
path-\>JSON-value maps (`leafPaths`/`walkLeaves`) and calls any leaf where
the two differ `secret: true`, taking the **redacted** copy's value for the
wire. A leaf is never read from the plain tree once it is known to differ.

This was the deliberate choice over a `pkg/fieldmeta`-style prefix table:
machinery's own `config/types/v1alpha1/v1alpha1_redact_test.go` shows
`RedactSecrets` already covers tokens, CA keys (never certs -- a cert is
public by definition and stays unredacted), the cluster bootstrap/discovery
secret, aescbc/secretbox encryption secrets, registry auth (username stays,
password/token/client-key don't), wireguard private keys, VIP cloud API
tokens and disk-encryption passphrases. Re-deriving that list by hand would
duplicate machinery's own authority on its config shape and silently fall
behind every time machinery adds a secret-bearing field; diffing against
machinery's own redaction cannot fall behind it.

Two things found empirically while building the leak test, both now
load-bearing comments in the code:

- `Provider.RedactSecrets` does not always honor the replacement string it
  is given: the cluster bootstrap token field is hardcoded to `"***"`
  regardless of the caller's argument (confirmed by reading
  `v1alpha1_redact_test.go`, which asserts `"***"` for that one field while
  every other redacted field uses the test's own `"**.***"`). `secretMask`
  is therefore fixed at the literal `"***"` in this package -- changing it
  would make that one field's mask silently diverge from every other
  masked field's.
- A `[]byte`-typed field (a CA key) decodes back from YAML as a plain Go
  `string`, and the *value itself* is already base64 text in the YAML
  machinery wrote, not the raw PEM text -- this is machinery's own
  rendering choice for `[]byte` fields, observed rather than documented.
  `GetEffectiveConfig`'s wire value for such a field is therefore base64,
  which is still safe (nothing of the real key survives a correct
  redaction) but not literally the string `"***"`; the test
  (`effectiveconfig_test.go`) checks both the raw and base64-encoded forms
  of the two CA keys for exactly this reason.

### Origin, without a second renderer

A leaf's `ConfigOrigin` is the last (bottom-up, so highest-priority)
applicable `TalosConfigPatch` whose **own** documents -- rendered in
isolation via `render.DocumentFromPatch` (newly exported for this reason)
plus `container.New(doc).EncodeBytes(...)`, never diffed against the merged
tree -- set that exact leaf path. A leaf no patch's own document sets is
bucketed as `{layer: CONFIG_LAYER_SYSTEM, source: "system"}`. This needed
no second render pass over the whole config (the earlier draft of this
design called `render.Render` once per patch-prefix and diffed consecutive
outputs; dropped once `DocumentFromPatch` made rendering each patch's own,
isolated contribution directly possible) -- `patchOrigins` runs once over
the already-known list of applicable patches.

Simplification recorded here rather than tested further: everything not
claimed by a patch is bucketed as `CONFIG_LAYER_SYSTEM`/`"system"`, with no
split between "Ichor's own system-layer generation" (the control-plane
taint) and "whatever machinery's `generate.NewInput().Config()` fills by
default" (cluster name, default install image, the discovery identity
document). The design's own `ConfigOrigin.source` comment allows both
`"system"` and `"cluster"` as non-patch sources without defining the line
between them; splitting it further wasn't something this unit had a
concrete screen requirement to resolve against.

### `pkg/render` changes: `BuildConfig` and `DocumentFromPatch` exported

`pkg/render/render.go` gained two exports, both pure refactors of code that
already existed, not new rendering logic:

- `BuildConfig` is `Render`'s existing body, stopping before
  `EncodeBytes`/hashing and returning the `coreconfig.Provider` itself.
  `Render` now calls `BuildConfig` then encodes -- identical behavior,
  confirmed by `pkg/render`'s existing test suite passing unchanged.
- `DocumentFromPatch` (was `documentFromPatch`) is the same per-document
  kind switch, now exported so `patchOrigins` can convert one patch's
  documents without reimplementing the kind dispatch.

This package is one `pkg/render` other agents are also working in: the
task flagged that a fix rejecting an empty `TalosMachineSpec.spec.role`
(machinery's `machine.ParseType("")` otherwise resolves to a worker
silently) might already be landing elsewhere. It is not present in this
worktree's `pkg/render/render.go` as of this change, and this unit's own
refactor does not touch `machine.ParseType` or add any such check inside
`pkg/render` itself -- `ApproveMachine` and `GetEffectiveConfig` each guard
against an empty/unspecified role on their own side instead (see above and
`node_service.go`), independent of whatever lands in `pkg/render` later, so
neither relies on a check that might not be there yet.

### Resume tokens (`resumetoken.go`)

A per-machine `{time, ordinal}` checkpoint, JSON-then-base64-encoded,
exactly matching ADR 0016's "last-seen log timestamp plus a monotonic
ordinal to break ties within the same timestamp, joined and base64-encoded
as one token covering every machine in the request." Opaque to the client,
as specified -- nothing but this package ever decodes it.

`StreamNodeLogs` streams Talos's `Dmesg` API (`talosclient_machinery.go`):
the request carries `repeated ObjectRef machines` and a resume token, no
service/container selector, so a whole-machine kernel log is the only log
source the proto's shape can mean (a per-container log, which would need an
`id`, is a gap to report if the UI ever needs it). `LogLine.Time` is when
this backend *observed* the line, not a timestamp parsed out of it: Dmesg's
lines carry a boot-relative monotonic offset, not wall-clock time, in the
vendored module's resource shape (no controller source ships with
`pkg/machinery` to check the exact format against), so the backend's own
clock is the only stable ordering key available without guessing at an
unverified text format.

**Known limit, recorded rather than hidden**: resuming after a backend
drain re-opens a fresh `Dmesg` stream on the new pod; Talos's own `tail`
flag replays some recent backlog, but each replayed line gets a *new*
`LogLine.Time` from the new pod's clock, not the original observation time.
A resumed stream therefore dedupes on "have I sent a line with this
observed time or later" within one backend process, not truly exactly
across a drain -- a small window of possible gap or re-delivery right at
the reconnect point is accepted rather than solved, since Dmesg offers
nothing stable to key an exact resume on. This is explicitly why logs don't
get the same "exact resumable" treatment architecture.md's "streams that
resume from a token" reserves for a `resourceVersion`-keyed watch.

### `NodeTalosClient`: the read-only Talos surface (`talosclient.go`, `talosclient_machinery.go`)

A four-method interface (`Hardware`, `Disks`, `NetworkInterfaces`,
`ResourceUsage`, plus `StreamLogs`) returning the wire types
(`gen/go/ichor/v1alpha1.Hardware` etc.) directly -- no parallel domain
struct, since nothing else in this package needs a Talos-shaped value that
isn't about to be put straight on a response. `NewMachineryNodeTalosClient`
dials maintenance mode (insecure, unauthenticated) when the machine isn't
approved yet -- no CA trust exists for it -- or mints a `role.Reader`
client certificate from the bundle otherwise ("readers who can access
read-only APIs that do not expose secrets," `pkg/machinery/role`'s own doc
comment), matching "Trust": the UI backend never holds an admin identity.

Per field, mapped from COSI resources confirmed in the vendored module
(`hardware.SystemInformation/Processor/MemoryModule`, `block.Disk`,
`network.LinkStatus/AddressStatus`, `perf.CPU/Memory`):

- `Disk.by_id_path` comes from scanning `block.Disk.Symlinks` for a
  `/by-id/` path, since the resource carries no dedicated field for it.
- `Disk.role` and `NetworkInterface.alias` are always
  `DISK_ROLE_UNSPECIFIED`/empty: disk-role assignment and
  `LinkAliasConfig` don't exist on the CRD yet (the same gap ADR 0016
  already noted for `ApproveMachineRequest`), so there is no source of
  truth to fill them from. Both will populate automatically once that CRD
  work lands, with no change needed here.
- **Unverified shortcut**: `ResourceUsage.cpu_cores_used` is derived from
  `perf.CPU.CPUTotal.Idle` as `(100-idle)/100 * cores`, assuming
  `CPUTotal`'s fields are already a percentage for the latest sample
  window. The vendored module ships only the resource's shape, not the
  controller that populates it, so this was never checked against a real
  Talos node's live values. Upgrade by sampling `perf.CPU` twice and
  computing a rate once this is checked against a live cluster.

This file (like `internal/controller`'s own `*_machinery.go` files) has no
direct unit test: it is a thin adapter over a real gRPC client, and no
Talos call may reach a real node from a test. `NodeTalosClient`'s consuming
logic in `node_service.go` is tested through a hand-written fake instead.

### RBAC: kept out of `manager-role` with a build tag, not a path change

`internal/uiapi/rbac_markers.go` carries every `+kubebuilder:rbac` marker
for this package's own `ClusterRole`, behind `//go:build ichorui_rbac` --
a tag `go build`/`go test`/`go vet` never set, and the file has nothing but
a package clause and comments, so excluding it changes nothing about what
the package does or how it's tested. `make manifests` (`paths="./..."`, no
special tags) therefore never discovers these markers and never merges them
into the controller's `manager-role`. A new, purely additive Makefile
target, `manifests-ichor-ui`, passes `--load-build-tags=ichorui_rbac` and
scopes `paths="./internal/uiapi/..."`, writing `config/rbac/ichor-ui/role.yaml`
under its own `roleName=ichor-ui-role`.

This was chosen over narrowing `manifests`'s own `paths="./..."` argument
(which would also have worked, since `./cmd/ichor-ui` and `./cmd` can be
told apart by whether the glob is recursive) because other agents are
concurrently editing this Makefile and were told not to expect its existing
targets rewritten; the build-tag approach needed zero changes to any
existing line, confirmed by running `make manifests` before and after and
diffing `config/`: empty.

The resulting `ClusterRole` (`get`/`list` on `talosclusters` and
`talosconfigpatches`, `get`/`list`/`update` -- never `create`, `patch` or
`delete` -- on `talosmachines`, `get` on `secrets`) is deliberately narrower
than the controller's: no `watch` (no cache/informer, see above), no
`events`, no `finalizers`, no `status` subresource (approval writes
`spec` through the main resource endpoint via `client.Update`, not
`Patch`, so `update` is the only verb ever exercised). `secrets: get`
mirrors a tradeoff ADR 0011 already accepted for the controller for the
same structural reason: `TalosCluster.spec.secretsRef` can name a Secret in
any namespace, so nothing narrower than cluster scope can express "the
Secret this particular `TalosCluster` names," short of restricting
`TalosCluster` to one namespace -- a design decision neither ADR made
unilaterally.

`config/rbac/ichor-ui/` (`service_account.yaml`, `role.yaml` (generated),
`role_binding.yaml`, `kustomization.yaml`) is a standalone kustomize base,
deliberately **not** added as a resource of `config/rbac/kustomization.yaml`
or `config/default/kustomization.yaml`: `ichor-ui` has no Deployment or
Service manifest yet anywhere in `config/` (that is a later work unit, per
ADR 0017's own scope boundary), so wiring RBAC into the controller's install
path would deploy permissions for a workload that doesn't exist there yet.

### `cmd/ichor-ui/main.go`: wiring and graceful termination

`main.go` gained a Kubernetes client (`ctrl.GetConfigOrDie()` +
`client.New`, the scheme built the same way `cmd/main.go` builds its own:
`clientgoscheme.AddToScheme` + `ichorv1alpha1.AddToScheme`), a `draining`
channel, a `uiapi.Server`, and the three generated handlers registered on
the existing mux alongside `spaHandler`. Nothing in `webfs_dev.go`,
`webfs_embed.go` or the SPA-fallback logic changed.

Graceful termination: `draining` is closed exactly once, at the same point
shutdown already began (`<-ctx.Done()`), **before** `srv.Shutdown` is
called. `StreamNodeLogs`'s own `select` (`done` vs. `s.Draining`) notices on
its next scheduling and returns `CodeUnavailable` with a message asking the
client to reconnect with its latest resume token -- "graceful termination
that asks clients to reconnect" (architecture.md). Any already-finished
unary call is unaffected; `srv.Shutdown`'s own timeout is what the draining
signal exists to make unnecessary for an in-flight stream specifically,
rather than making that stream wait out the whole shutdown window.

## Consequences

- Nine RPCs across three services are implemented and tested against
  envtest (`internal/uiapi/*_test.go`, 20 Ginkgo specs plus plain-Go tests
  for `strongIdentity` and the resume token). `make build test lint` passes
  unchanged for the rest of the repository.
- The masking guarantee rests on `config.Provider.RedactSecrets`, not on
  this package's own judgment of which fields are secret. If machinery ever
  stops redacting a field it used to, this package's output changes with
  it automatically (for better, if machinery widens its own redaction, or
  worse, if it narrows it) -- a dependency worth re-confirming on any
  `pkg/machinery` version bump, the same way a version-contract bump
  already gets scrutiny per principle 4.
- `pkg/render/render.go` now exports `BuildConfig` and `DocumentFromPatch`;
  both are refactors of existing code, confirmed behavior-identical by the
  package's own pre-existing test suite passing unchanged.
- `config/rbac/ichor-ui/` exists but is not deployed by any kustomization
  yet; wiring it in is the job of whichever work unit adds `ichor-ui`'s
  Deployment/Service manifests.
- Gaps reported rather than silently worked around, two of which the
  amendment below closes: no proto-level "rejected" state for a
  `TalosMachine` (closed), and `StreamNodeLogs` having no way to select a
  specific service/container log (closed). Still open: `AuditRecord.actor`
  is a fixed placeholder pending OIDC (roadmap phase 9);
  `ResourceUsage.cpu_cores_used`'s formula is unverified against a live
  node.

## A build-tag file is not type-checked by default, and two gaps that let it drift silently

Found auditing this ADR's own mechanism, not while building it: putting
`rbac_markers.go`'s content behind `//go:build ichorui_rbac` correctly keeps
its markers out of `manager-role` (the stated goal), but it has a side
effect nobody had named yet -- `go build ./...`, `go test ./...`, `go vet
./...` and golangci-lint's default run **never compile that file at all**,
since none of them ever sets that tag:

```console
$ go list -f '{{.GoFiles}}' ./internal/uiapi/
[bundle.go cluster_service.go ... ]      # rbac_markers.go absent
$ go list -tags ichorui_rbac -f '{{.GoFiles}}' ./internal/uiapi/
[... rbac_markers.go]                    # present only with the tag
```

A syntax or type error in that file would pass every check in the repo
silently. `rbac_markers.go` is comment-only today so this never bit, but the
next file anyone puts behind a build tag for the same "keep it out of a
default scan" reason will hit this the first time it has an actual mistake
in it. Fixed in `make vet`, the one place now: a second `go vet -tags
ichorui_rbac ./internal/uiapi/...` pass after the untagged one, so the file
is compiled on every `make test`/CI run without the tag ever reaching
`manifests`, `build`, or anything that produces a binary. One place, not
two: golangci-lint's own build-tag config was considered and rejected so
that only `make vet`'s recipe needs to know this tag exists.

Two related gaps closed in the same pass, both CI-level rather than
`internal/uiapi`-level:

- **`manifests-ichor-ui` was defined but never called from anywhere** --
  not `manifests`, not CI. `config/rbac/ichor-ui/role.yaml` could silently
  fall behind `rbac_markers.go`'s actual markers indefinitely; nothing
  would ever regenerate it except a developer remembering the target's
  name. `manifests` now depends on `manifests-ichor-ui`, so one `make
  manifests` run (or the CI job below) regenerates both `ClusterRole`s
  together.
- **No CI step compared generated output against what's committed, for
  anything** -- `config/crd/bases`, `config/rbac/*` (including
  `ichor-ui/role.yaml` once wired in above), and `pkg/typedconfig`'s
  `zz_generated.*.go` could all drift from their source silently; only
  `dist/chart/chart_test.go`'s chart-vs-`config/` check existed, one layer
  short of source-vs-`config/`. Added as `manifests-drift`, a job in
  `.github/workflows/test.yml` run in parallel with `test`: `make manifests
  generate fmt` (`fmt`, not just `generate` -- see its own comment: raw
  `controller-gen`/mirrorgen output isn't always import-sorted the way
  `go fmt` leaves it, and skipping it produced a false positive against
  already-correct, already-committed files during this change), then `git
  diff --exit-code`, failing with a message naming the fix
  (`make manifests generate fmt`, then commit). Verified by hand: adding an
  unregenerated field to `TalosClusterSpec` and running the same two
  commands fails with exactly that diff and message; reverting the field
  makes it pass again.

## Amendment (same date): approval tri-state and service-selectable logs

Two gaps this ADR originally reported back were handed back as real work:
`RejectMachine`'s own design (never deleting the machine) left no way to
record *that* it had been rejected, and `StreamNodeLogs` could only ever
mean the kernel log. Both proto messages change; both changes are additive
or a pre-release breaking change the design explicitly authorized (v1alpha1,
not yet shipped to any real client).

### `TalosMachineSpec.Approved` (bool) becomes `ApprovalState` (tri-state)

A bool cannot represent pending/approved/rejected without a second field
claiming the same truth, which is exactly the anti-pattern to avoid: two
fields that can both answer "is this approved," with nothing stopping them
from disagreeing. `api/v1alpha1/talosmachine_types.go` removes `Approved
bool` and adds `ApprovalState string` (`pending`/`approved`/`rejected`,
enum-validated, defaulting to `pending`), plus an `IsApproved()` method so
every caller asks the same one question the same one way rather than
comparing the string literal directly. `machine.proto`'s `Machine.approved`
(field 8) is replaced the same way: `reserved 8; reserved "approved";` plus
a new `MachineApprovalState approval_state = 11` enum field, mirroring the
CRD's three states exactly.

This is a real breaking change to both the CRD and the wire type, pre-
authorized for exactly this reason (pre-release, nothing depends on the old
shape yet). `make proto-breaking` confirms it as the one and only breaking
finding (`Previously present field "8" with name "approved" on message
"Machine" was deleted`) -- the new `approval_state` field and
`StreamNodeLogsRequest.service` (below) are both pure additions and `buf
breaking` reports nothing for either, exactly as expected.

Every caller of the old `Spec.Approved`/`Machine.approved` had to move to
the new shape: `internal/controller` (`talosmachine_controller.go`'s
approval gate, `discovery_test.go`'s assertion), `internal/webhook/v1alpha1`
(the quorum-counting delete guard and its fixtures), `internal/cli`
(`init_run.go`'s first-control-plane approval and its test), and this
package's own `convert.go`/`machine_service.go`/`node_service.go` and their
tests. Every one of these is the same one-line mechanical swap
(`m.Spec.Approved` -> `m.Spec.IsApproved()`, or setting `ApprovalState`
instead of `Approved`); none changes behavior.

### Reversibility: rejecting is not permanent

`RejectMachine` sets `ApprovalState` to `rejected` (never deletes the
object, for the same reason as before -- deleting it would let discovery
recreate it as fresh and pending, making the rejection meaningless the
moment discovery runs again; `internal/uiapi/discovery_integration_test.go`
drives the real, exported `controller.DiscoveryReconciler.Discover` against
a rejected fixture to prove this directly rather than trusting the
reasoning). `ApproveMachine`'s only refusal is an *already-approved*
machine; a *rejected* one is not approved, so approving it succeeds and
moves it straight to `approved` -- this is the entire mechanism by which a
rejection is undone, and it needed no new RPC or field.

Decision: **reversible**. The alternative -- a rejection that can never be
undone -- turns a misclick into a permanent dead end for that piece of
hardware, with no UI gesture this phase defines for "undo." Ichor's own
design leans the other way throughout (drift policy, lock re-acquisition,
`forceRemove`, reuse after reset): operator mistakes get a way back, not a
one-way door. The cost is that "rejected" means "not currently wanted,"
not "permanently barred" -- acceptable, since nothing in the design asks
for the stronger guarantee, and a stronger one (a reject that really can
never be undone) is easy to add later as an annotation or a second state if
a real incident ever calls for it.

`RejectMachine` refuses to touch an already-*approved* machine (removal is
the correct operation there, a different and later one); rejecting an
already-*rejected* machine is allowed and simply records the newest reason.

### `StreamNodeLogsRequest.service`: resolving architecture.md's open question

Added as an optional `string service = 3`: empty means Talos's kernel log
(`Dmesg`, the only source this RPC could reach before), any other value is
a Talos service name (`kubelet`, `etcd`, `trustd`, `apid`, `machined`, ...),
read through Talos's `Logs` API. `NodeTalosClient.StreamLogs` gained the
same parameter; `talosclient_machinery.go` branches on it, using
`constants.SystemContainerdNamespace` with `common.ContainerDriver_CONTAINERD`
for the non-empty case -- confirmed, not guessed, by reading
`cmd/talosctl/cmd/talos/{logs,root,image}.go` in the vendored
`github.com/siderolabs/talos` source (not just `pkg/machinery`): that is
exactly the namespace/driver `talosctl logs <service>` resolves to with no
flags given, and the RPC's `id` parameter is the service name verbatim.

**Architecture.md's open question -- "Can `os:reader` read logs, or does
the UI backend need more?" -- is answered: yes, for both RPCs this unit
uses.** `internal/app/machined/pkg/system/services/machined.go` in the same
vendored source carries the authoritative per-method role table (not
`pkg/machinery`, which ships no server-side policy at all), and both
entries read:

```go
"/machine.MachineService/Dmesg": role.MakeSet(role.Admin, role.Operator, role.Reader),
"/machine.MachineService/Logs":  role.MakeSet(role.Admin, role.Operator, role.Reader),
```

`role.Reader` is in both sets. The UI backend's read-only identity
(`role.MakeSet(role.Reader)`, `talosclient_machinery.go`) needs nothing
more for either RPC. This is recorded here, not left as a runtime surprise:
if a future Talos version narrows either table, `StreamLogs`'s error
wrapping states plainly that it was using "this backend's read-only
(role.Reader) Talos identity" when the call failed, so a `PermissionDenied`
reads as a Talos policy change, not a mystery.

## Amendment (2026-10-26): `loadBundle` was resolving the wrong namespace

Found live, not in review: `internal/uiapi/bundle.go`'s `loadBundle` read
`cluster.Namespace` to find the secrets bundle Secret, instead of this
backend's own namespace. ADR 0011 settled this exact question for
`internal/controller`'s own `loadBundle` -- a `TalosCluster` may live in
any namespace the reader watches, but Kubernetes RBAC cannot express "the
Secret this `TalosCluster` names, wherever it happens to live" at
anything narrower than cluster-wide `Secret` read, so the Secret is
resolved in the reader's own fixed namespace instead, and
`internal/uiapi/rbac_markers.go`'s own marker already only grants
`get`/`update`/`patch` on `ichor-webhook-server-cert`/
`ichor-metrics-server-cert` plus namespace-scoped `list`/`watch`/`create`
in `ichor-system` -- never a cluster-wide `Secret` read. `internal/
uiapi`'s copy of `loadBundle` never got the matching fix when ADR 0011
landed on the controller side: it read `cluster.Namespace` directly,
which is wrong for the identical reason and was invisible in every
existing test because every fixture happened to put the `TalosCluster`
and the Secret in the same namespace (`testNamespace`, "default").

A `TalosCluster` outside the backend's own namespace would have made
`GetEffectiveConfig` fail with `Forbidden` (RBAC permits reads only in
`ichor-system`); one living inside it only worked by coincidence, not
because the code resolved the right namespace on purpose.

Fixed to match `internal/controller`'s own shape exactly:
`Server.ControllerNamespace` (set from `POD_NAMESPACE` in `cmd/ichor-ui/
main.go`, the same env var `cmd/main.go` already reads for the
controller) is what `loadBundle` resolves against now, never `cluster.
Namespace`; an empty `ControllerNamespace` fails loudly
("controller namespace is empty (is POD_NAMESPACE set?)") rather than
risking a broader read than intended, the same discipline `internal/
controller/bundle.go` already has. `internal/uiapi/bundle_test.go` is new
and mirrors `internal/controller/bundle_test.go` directly: resolves the
Secret in the given namespace even when the `TalosCluster`'s differs,
fails explicitly when the namespace is empty, names the namespace it
looked in when the Secret is missing. `effectiveconfig_test.go` gained a
fourth test creating the `TalosCluster` and the Secret in two different,
real namespaces end to end through `Server.GetEffectiveConfig` itself
(not just the `loadBundle` unit), to show the whole RPC path, not only
the helper, resolves correctly now.

## Amendment (2026-10-26): the `ClusterRole` had no `nodes` rule, so every `GetNode` was `Forbidden`

Found by a security review, not in use: `node_service.go`'s
`findKubernetesNode` -- `GetNode`'s lookup of the `corev1.Node` behind a
`TalosMachine`, by `Get` on `Spec.Hostname` or, failing that, a `List` over
every `Node` matched against the Talos endpoint address -- was there from
this ADR's first version, but `rbac_markers.go` never carried a matching
`+kubebuilder:rbac` marker for `nodes`. Nothing caught it: envtest does not
enforce RBAC, so every existing test against a fake client passed
regardless of what the generated `ClusterRole` actually said; only a real
deployment's `Get`/`List` would have returned `Forbidden`, surfaced to the
UI as `CodeInternal` (`GetNode`'s own error wrapping). Fixed by adding

```go
// +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list
```

to `rbac_markers.go` and regenerating (`make manifests`, which calls
`manifests-ichor-ui`): `config/rbac/ichor-ui/role.yaml` and
`dist/chart/templates/rbac/ichor-ui-role.yaml` both gained the rule, in the
same `ClusterRole` as `talosclusters`/`talosconfigpatches`/`talosmachines`
-- never `namespace=`, since a `Node` is cluster-scoped in the Kubernetes
API itself and has no namespace a `Role` could qualify into. This is the
one marker in the file where that is not a choice: every other ClusterRole
rule here could, in principle, have been narrowed to a Role if some other
design decision had fixed its resource to one namespace (`secrets`'s own
`namespace=ichor-system` is exactly that, chosen once `loadBundle` stopped
reading `TalosCluster.spec.secretsRef`'s namespace -- see ADR 0011, which
also records this same shape regressing into a cluster-wide grant twice
before). `nodes` has no such escape; `get;list` only, matching exactly the
two calls `findKubernetesNode` makes and nothing wider (no `watch`, since
this package keeps no informer cache, consistent with the rest of this
ADR's "RBAC: kept out of `manager-role`" section above).

`dist/chart/chart_test.go`'s `checkNoClusterWideSecretsRead` guards only
rules naming `secrets`; it does not, and should not, fire on this `nodes`
rule, and `go test ./dist/chart/...` confirms it doesn't.

**No new mechanism added to catch a missing marker itself.** envtest's own
non-enforcement of RBAC is a real gap -- a marker can be deleted or never
written, and every test in this package still passes -- but closing it
would mean either standing up a real API server with RBAC turned on (a
different, heavier test setup change than this fix, and one that would
affect every other `internal/uiapi` test, not just this file) or hand-
writing a second list of "every client-go type this package touches" to
diff against `rbac_markers.go`'s markers, which is exactly the kind of
parallel bookkeeping that drifts the same way the markers themselves just
did. Left unfixed and reported here instead.

## Amendment (2026-10-26): the chart's `ClusterRole` had silently fallen behind `config/`, and nothing would have caught it a third time

The previous amendment's own report named this exact risk: "dist/chart's
templates and config/'s manifests have no automatic drift check tying them
together... a future hand-edit of either alone would go unnoticed by any
test." It was not hypothetical. `dist/chart/templates/rbac/ichor-ui-role.yaml`
had `talosclusters` and `talosconfigpatches` sharing one `apiGroups` entry
with only `get`/`list`, while `config/rbac/ichor-ui/role.yaml` (regenerated
from `rbac_markers.go` when `PatchService`, ADR 0035, added
`create`/`update`/`delete` on `talosconfigpatches`) had moved on. A Helm
install granted none of the three: every `TalosConfigPatch` create, update
or delete `PatchService` makes came back `Forbidden`, surfaced to the UI as
`CodeInternal`. The chart's hand-written copy had not tracked the generated
one since `PatchService` landed.

Fixed in the chart (`dist/chart/templates/rbac/ichor-ui-role.yaml`): split
`talosconfigpatches` back into its own `apiGroups` entry carrying
`create`/`delete`/`get`/`list`/`update`, matching `config/rbac/ichor-ui/
role.yaml` exactly. The comment explaining why the `secrets` rule lives in
its own file (`ichor-ui-secrets-role.yaml`) instead was left untouched --
that split is deliberate packaging, not drift, and stays.

**The fix is a test, not a sync.** `config/rbac/ichor-ui/role.yaml` is a
generated manifest; `dist/chart/templates/rbac/ichor-ui-role.yaml` is a Helm
template carrying `{{ .Release }}` substitutions and its own grouping of
`apiGroups` entries -- there is no byte-identical form to propagate the way
`checkCRDsMatchConfig` already propagates `config/crd/bases` into the
chart's CRD templates. Hand-mirroring was the original design (this ADR's
"RBAC" section above) and it had now drifted twice (the `nodes` rule in the
previous amendment, `talosconfigpatches`'s verbs here) with no CI signal
either time. The decision is not to give up on keeping the chart in sync by
hand -- a generator can't produce Helm template syntax from a plain
manifest -- but to make a drift between them fail loudly instead of
shipping silently. `dist/chart/chart_test.go` gained
`checkUIBackendRBACMatchesConfig`: it loads `config/rbac/ichor-ui/role.yaml`
from disk, renders the chart the same way every other check in that file
already does, and compares both the `ClusterRole`'s and the namespaced
`Role`'s rules as a set of (apiGroup, resource, verb) triples -- not YAML
text, so grouping resources or verbs into one `apiGroups` entry versus
several never causes a false failure. The namespaced `Role` is matched by
rule content, not by name: `config/`'s copy names it `ichor-ui-role` (same
name as the `ClusterRole`, disambiguated only by kind), while the chart
gives it its own fixed name, `ichor-ui-secrets-role`, for the packaging
reason that file's own comment already explains; nothing about that name
difference is itself drift, so the test does not fail on it.

Proven to actually fail, not just reasoned to: removing `update` from the
chart's `talosconfigpatches` verbs and running `go test ./dist/chart/...
-run TestChartInvariants -count=1` produced

```text
chart_test.go:119: helm template ichor .: ClusterRole ichor-ui-role is
missing ichor.maesh.dev/talosconfigpatches:update (present in
../../config/rbac/ichor-ui/role.yaml); add it to
dist/chart/templates/rbac/ichor-ui-role.yaml
```

for both the `ichor` and `something-else` release-name subtests, naming the
exact missing grant and which file to fix. Restoring the verb made the
suite pass again. `checkNoClusterWideSecretsRead` and
`checkUIBackendRBACIsSeparate` both still pass unchanged.

**The same check, extended to the controller's `manager-role`.** The drift
check had only looked at `ichor-ui-role`, so the same drift in the
controller's own RBAC went unseen: `config/rbac/role.yaml` (controller-gen
output) carried `talosoperations`, `talosoperations/status` and the Argo CD
`applications` rules, and the chart's `manager-role.yaml` had none of them
(since `a328ca6`, which added `TalosOperation`). A Helm install would have
been Forbidden on `TalosOperation` (no upgrade could run) and unable to read
`Application` state (the CNI handover could never finish).

`checkControllerRBACMatchesConfig` reuses `checkRBACRulesMatch` unchanged and
compares `config/rbac/role.yaml`'s `ClusterRole manager-role` with the
chart's `manager-role`, and its namespaced `Role manager-role` with the
chart's `manager-secrets-role` (the chart splits secrets out; chart names are
release-templated, so they are matched by suffix). Not compared, on purpose:
`leader-election-role`, `metrics-auth-role` and `metrics-reader` (static
kubebuilder scaffolds, not generated from markers) and the per-CRD
admin/editor/viewer roles (end-user roles, not the controller's identity).

The shared triple now also carries `resourceNames`; without it a name-scoped
grant for one Secret hid a missing one for another. That immediately found a
fourth gap: `manager-secrets-role` lacked `get/patch/update` on
`ichor-metrics-server-cert`, which `cmd/main.go`'s cert rotator needs. Fixed
in the chart with the other three.

Proven both ways: before the chart fix the check failed naming every missing
`talosoperations`, `talosoperations/status` and `applications` verb (and the
three metrics-cert verbs); after it the suite passes, and removing
`applications:watch` made it fail with `ClusterRole manager-role is missing
argoproj.io/applications:watch (present in ../../config/rbac/role.yaml); add
it to dist/chart/templates/rbac/manager-role.yaml`.

## Amendment: RBAC proven by an enforcing test

`GetEffectiveConfig` failed live with `cannot list resource "talosstorages"`: `render.ResolveSystemInputs`
lists `TalosStorage` as the ServiceAccount (ADR 0050) and `rbac_markers.go` had no rule for it. Added
`talosstorages` `list` (no `get`, no `watch`: one non-cached List per call). An audit of every `Get`/`List` in
`internal/uiapi` and the render inputs it calls (`ResolveSystemInputs`, `ResolveRegistryAuth`, `secretsbundle.Load`)
found no other gap: Secrets are `get` in the controller namespace, `argocd-cm` and Applications are already granted.
`internal/uiapi/rbac_enforced_test.go` now loads `config/rbac/ichor-ui/role.yaml` into envtest (whose apiserver
enforces RBAC) and runs `GetEffectiveConfig` as a client impersonating `system:serviceaccount:ichor-system:ichor-ui`,
so a read the generated role does not grant fails the suite with Forbidden. A new render input needs a marker, and
a handler added to that spec is covered.

Amendment 2026-11-05 (ADR 0130): no reason is asked for on approve or reject. `reason` on `RejectMachineRequest`, `ApproveMachineRequest` and `AuditRecord`'s typed use is deprecated and never read; a rejection records the actor and the time only, and a rejected machine that is rejected again simply records the newest decision.