fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

broker: take the AMQP URI from an env var (uriEnv), not clear text in fleetd.yaml #151

New Issue
Closed
opened 2026-08-23 13:16:18 +02:00 by ltms · 2 comments
ltms commented 2026-08-23 13:16:18 +02:00
Owner
Copy Link
Copy Source

Problem

The broker: block takes the AMQP connection as a literal URI:

public record Broker(String uri, Integer prefetch) {
    public boolean isConfigured() { return uri != null && !uri.isBlank(); }
}

An AMQP URI carries its credentials inline — amqp://user:password@host:5672/vhost. The config loader does no environment expansion (I grepped dev/ltms/fleet/config/ for System.getenv and there are no hits), so the only way to configure a broker today is to write the password, in clear text, into fleetd.yaml on every host that runs a daemon.

fleetd.yaml is gitignored, so this does not reach the repository. It is still the one credential in the system that cannot come from the shared secret store, and it has to be copied by hand to each fleet host.

Why now

We are about to stand up a LavinMQ instance serving all the fleets. That turns the durable reply inbox on, which is a real improvement — replies currently live in InMemoryReplyInbox and are lost on restart. But it also means writing that password onto at least two hosts as things stand, and doing it before this is fixed means going back and cleaning up afterwards.

Suggested fix

Add uriEnv beside uri, following the pattern this codebase already uses in three places — auth.tokenEnv, Profile.tokenEnv, and Profile.gitTokenEnv — where config names an environment variable and the daemon reads the value from it.

broker:
  uriEnv: LAVINMQ_URI      # preferred: value comes from the secret store
  prefetch: 20

Points worth deciding rather than assuming:

  • Precedence. If both uri and uriEnv are set, which wins? I would take uriEnv and log that the literal was ignored, so migrating a host is a one-line addition rather than a swap.
  • isConfigured() must account for both. Right now it only inspects uri. If uriEnv names a variable that is unset or empty, the daemon should fall back to the in-memory inbox and say so loudly at startup — a silently disabled durable inbox is the silent-default shape that has bitten this project repeatedly.
  • Startup secret report. Fleetd.requiredSecretEnvVars already derives which env vars the loaded config needs and reports each as set or missing at boot. A configured uriEnv belongs in that list, so a missing broker credential is visible at startup rather than at first reply.
  • Never log the resolved URI. It contains the password. Log the variable name and whether it resolved, never the value — the same discipline as the existing startup secret lines.

Tests

  • uriEnv set and the variable present ⇒ AMQP inbox selected
  • uriEnv set and the variable missing or blank ⇒ in-memory inbox, with a warning
  • both uri and uriEnv set ⇒ the documented winner, deterministically
  • the resolved value never appears in any log line
  • a configured uriEnv shows up in the startup required-secrets report

Note that a test on the config record alone would not prove the daemon actually honours it — Fleetd selects the inbox at line ~378. Cover the selection, not just the parse.

## Problem The `broker:` block takes the AMQP connection as a literal URI: ```java public record Broker(String uri, Integer prefetch) { public boolean isConfigured() { return uri != null && !uri.isBlank(); } } ``` An AMQP URI carries its credentials inline — `amqp://user:password@host:5672/vhost`. The config loader does **no** environment expansion (I grepped `dev/ltms/fleet/config/` for `System.getenv` and there are no hits), so the only way to configure a broker today is to write the password, in clear text, into `fleetd.yaml` on every host that runs a daemon. `fleetd.yaml` is gitignored, so this does not reach the repository. It is still the one credential in the system that cannot come from the shared secret store, and it has to be copied by hand to each fleet host. ## Why now We are about to stand up a LavinMQ instance serving all the fleets. That turns the durable reply inbox on, which is a real improvement — replies currently live in `InMemoryReplyInbox` and are lost on restart. But it also means writing that password onto at least two hosts as things stand, and doing it before this is fixed means going back and cleaning up afterwards. ## Suggested fix Add `uriEnv` beside `uri`, following the pattern this codebase already uses in three places — `auth.tokenEnv`, `Profile.tokenEnv`, and `Profile.gitTokenEnv` — where config names an environment variable and the daemon reads the value from it. ```yaml broker: uriEnv: LAVINMQ_URI # preferred: value comes from the secret store prefetch: 20 ``` Points worth deciding rather than assuming: - **Precedence.** If both `uri` and `uriEnv` are set, which wins? I would take `uriEnv` and log that the literal was ignored, so migrating a host is a one-line addition rather than a swap. - **`isConfigured()` must account for both.** Right now it only inspects `uri`. If `uriEnv` names a variable that is unset or empty, the daemon should fall back to the in-memory inbox and **say so loudly at startup** — a silently disabled durable inbox is the [silent-default](https://git.ltms.dev/fleet/fleetd/issues) shape that has bitten this project repeatedly. - **Startup secret report.** `Fleetd.requiredSecretEnvVars` already derives which env vars the loaded config needs and reports each as set or missing at boot. A configured `uriEnv` belongs in that list, so a missing broker credential is visible at startup rather than at first reply. - **Never log the resolved URI.** It contains the password. Log the variable name and whether it resolved, never the value — the same discipline as the existing startup secret lines. ## Tests - `uriEnv` set and the variable present ⇒ AMQP inbox selected - `uriEnv` set and the variable missing or blank ⇒ in-memory inbox, with a warning - both `uri` and `uriEnv` set ⇒ the documented winner, deterministically - the resolved value never appears in any log line - a configured `uriEnv` shows up in the startup required-secrets report Note that a test on the config record alone would not prove the daemon actually honours it — `Fleetd` selects the inbox at line ~378. Cover the selection, not just the parse.
ltms commented 2026-08-23 13:30:56 +02:00
Author
Owner
Copy Link
Copy Source

Verified against LavinMQ, not assumed

Before designing a shared broker I checked whether the durable inbox actually works on the broker we intend to deploy. It had never been run against one.

AmqpReplyInboxContractTest provisions RabbitMQ through Testcontainers, and the CI path sets AMQP_URI at a RabbitMQ service container. So every existing guarantee about the AMQP inbox — eventual visibility, ack removal, msgId dedup, explicit ownership, and cross-restart redelivery — was measured on RabbitMQ only. Our target is LavinMQ. AMQP 0-9-1 compatibility is close but not identical, and nothing had confirmed it.

I ran a LavinMQ 2.9.1 container and pointed the existing suite at it with AMQP_URI:

AMQP_URI=amqp://guest:guest@localhost:5673 mvn test -Pcontract -Dtest=AmqpReplyInboxContractTest
  Tests run: 8, Failures: 0, Errors: 0, Skipped: 0

All eight pass, including the cross-restart durability case that is the entire reason Stage 2 exists. So the AMQP inbox is safe to run on LavinMQ.

Per-vhost isolation also verified

AmqpReplyInbox names queues agent.<target>.inbox with no fleet or host component, so two daemons sharing one broker share one flat namespace and rely on herdr terminal ids never colliding. I proposed a vhost per fleet to remove that dependency, and then checked it holds:

vhost /         14 queues
vhost fleet01    7 queues
vhost mac        7 queues

The suite passes in full against amqp://…/mac and amqp://…/fleet01 separately, and each vhost keeps its own queue set. So one instance can serve every fleet with real isolation, and the existing uri field already carries the vhost — no code change needed for that part.

Recommended shape

  • one LavinMQ instance, reachable by every fleetd
  • one vhost per fleet (mac, fleet01, …), created up front with permissions for that fleet's user
  • credentials supplied via uriEnv from the secret store, which is what this issue is about

One risk this does not clear

AmqpReplyInbox documents at lines 42-44 that on ownership the broker pushes the whole queue into the in-memory held map, so x-max-length and per-message TTL never fire and an undrained lead grows the JVM heap without limit. The contract suite does not exercise that path, and durable delivery makes it more likely rather than less, because messages now survive restarts instead of vanishing. Worth its own ticket before this is switched on for a busy fleet.

## Verified against LavinMQ, not assumed Before designing a shared broker I checked whether the durable inbox actually works on the broker we intend to deploy. It had never been run against one. `AmqpReplyInboxContractTest` provisions **RabbitMQ** through Testcontainers, and the CI path sets `AMQP_URI` at a RabbitMQ service container. So every existing guarantee about the AMQP inbox — eventual visibility, ack removal, msgId dedup, explicit ownership, and cross-restart redelivery — was measured on RabbitMQ only. Our target is LavinMQ. AMQP 0-9-1 compatibility is close but not identical, and nothing had confirmed it. I ran a LavinMQ **2.9.1** container and pointed the existing suite at it with `AMQP_URI`: ``` AMQP_URI=amqp://guest:guest@localhost:5673 mvn test -Pcontract -Dtest=AmqpReplyInboxContractTest Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 ``` All eight pass, including the cross-restart durability case that is the entire reason Stage 2 exists. So the AMQP inbox is safe to run on LavinMQ. ## Per-vhost isolation also verified `AmqpReplyInbox` names queues `agent.<target>.inbox` with no fleet or host component, so two daemons sharing one broker share one flat namespace and rely on herdr terminal ids never colliding. I proposed a vhost per fleet to remove that dependency, and then checked it holds: ``` vhost / 14 queues vhost fleet01 7 queues vhost mac 7 queues ``` The suite passes in full against `amqp://…/mac` and `amqp://…/fleet01` separately, and each vhost keeps its own queue set. So one instance can serve every fleet with real isolation, and the existing `uri` field already carries the vhost — no code change needed for that part. ## Recommended shape - one LavinMQ instance, reachable by every fleetd - one vhost per fleet (`mac`, `fleet01`, …), created up front with permissions for that fleet's user - credentials supplied via `uriEnv` from the secret store, which is what this issue is about ## One risk this does not clear `AmqpReplyInbox` documents at lines 42-44 that on ownership the broker pushes the whole queue into the in-memory `held` map, so `x-max-length` and per-message TTL never fire and an undrained lead grows the JVM heap without limit. The contract suite does not exercise that path, and durable delivery makes it more likely rather than less, because messages now survive restarts instead of vanishing. Worth its own ticket before this is switched on for a busy fleet.
ltms commented 2026-08-23 14:04:03 +02:00
Author
Owner
Copy Link
Copy Source

Shipped and live on both hosts. Merged as 4644359 (worker branch worker/cb635-broker-urienv, PR #153). 908 tests green on the Mac and on fleet01.

broker.uriEnv: names an env var that holds the AMQP URI. uriEnv wins over uri whenever it is set, and it never falls back to a stale literal uri — an unset or blank variable means "not configured", so the daemon uses the in-memory inbox instead of quietly returning to clear text. Both set logs broker.uri is ignored. The resolved URI is never logged; only the variable name. requiredSecretEnvVars now reports it, so startup prints startup secret LAVINMQ_URI: set (broker uriEnv).

Live proof:

  • fleet01 — broker: { uriEnv: LAVINMQ_URI }; startup log reads reply inbox: AMQP broker (durable) via env var LAVINMQ_URI (prefetch=32), and the broker reports one connection user=fleet-fleet01 vhost=/fleet01 name=bridged-reply-inbox.
  • Mac — bridged.yaml now contains no amqp:// string at all; the password left the config file.

Also added: scripts/redeploy-bridged.sh --check reports whether the named variable resolves in a login shell (3f4ac2b). It reads the name out of bridged.yaml, so renaming the key cannot make the check lie, and it never prints the value. Documented in wiki 11 Features → Broker password out of the config.

Shipped and live on both hosts. Merged as `4644359` (worker branch `worker/cb635-broker-urienv`, PR #153). 908 tests green on the Mac and on fleet01. `broker.uriEnv:` names an env var that holds the AMQP URI. `uriEnv` wins over `uri` whenever it is set, and it never falls back to a stale literal `uri` — an unset or blank variable means "not configured", so the daemon uses the in-memory inbox instead of quietly returning to clear text. Both set logs `broker.uri is ignored`. The resolved URI is never logged; only the variable name. `requiredSecretEnvVars` now reports it, so startup prints `startup secret LAVINMQ_URI: set (broker uriEnv)`. Live proof: - **fleet01** — `broker: { uriEnv: LAVINMQ_URI }`; startup log reads `reply inbox: AMQP broker (durable) via env var LAVINMQ_URI (prefetch=32)`, and the broker reports one connection `user=fleet-fleet01 vhost=/fleet01 name=bridged-reply-inbox`. - **Mac** — `bridged.yaml` now contains **no** `amqp://` string at all; the password left the config file. Also added: `scripts/redeploy-bridged.sh --check` reports whether the named variable resolves in a login shell (`3f4ac2b`). It reads the name out of `bridged.yaml`, so renaming the key cannot make the check lie, and it never prints the value. Documented in wiki *11 Features* → *Broker password out of the config*.
ltms closed this issue 2026-08-23 14:04:15 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#151
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?