Repository navigation
ops(runtime): make bundled MQTT and pprof exposure explicit settings (#372) - #431
Conversation
The bundled mosquitto's host port and the pprof listeners were exposed by hard-coded values: docker-compose.yml published 1883 on every interface, the staging compose files published 6060/6061 the same way, and both processes listened on ":"+port. There was no setting to narrow any of them, and nothing validated the external-broker arrangement that DISABLE_MOSQUITTO=true selects. Each exposure is now an explicit setting whose shipped default reproduces today's behaviour exactly, so an existing install that pulls this changes nothing: PROD_MQTT_BIND=0.0.0.0 host interface for the bundled broker PPROF_BIND=0.0.0.0 host interface for the pprof ports PPROF_HOST= in-process pprof bind host PPROF_PORT_SERVER=6060 per-role pprof ports, kept distinct PPROF_PORT_INGESTOR=6061 MQTT_SOURCE_STRICT=false make a broker error stop the ingestor internal/pprofbind resolves the listener for one process role from the environment. It keeps ENABLE_PPROF opt-in on the exact string "true", and now says so when a value like "1" or "TRUE" leaves pprof off. The per-role ports exist because the shared PPROF_PORT applied to both processes, so moving one collided the other. The ingestor reports its broker arrangement before connecting: no configured source in external mode (where LoadConfig's localhost fallback points at the broker that was just disabled), loopback-only sources, malformed broker URLs, and a DISABLE_MOSQUITTO value that the container entrypoint and manage.sh read differently. Diagnostics only; MQTT_SOURCE_STRICT turns errors into a refusal to start. manage.sh gains broker-check, which reports the same thing and exits 1, and runs it advisory-only from setup, start, update, restore and promote. Every broker URL in a diagnostic goes through brokerurl.Mask, and the MQTT log lines verify_health echoes are masked too. No default is flipped and no deployment is changed; narrowing any bind to loopback is left to the operator. Relates to #372 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rapport — CS-Minimax PR#431 #372 — head c5a1d64Status: Implemented; every acceptance point has a test that was red before and is green after, plus at least one mutant shown red per point; CI green on all three jobs; five behaviour-changing choices deferred under "Left for the owner". Requirements → test → mutant
Red-before was established by restoring each touched file from CI per job
Local, before pushing [T]:
Behavioural confirmation against a locally built server on a migrated fixture DB [A]:
All jobs green on the first run — no rerun needed, and the known flaky test (#271) did not fire. Remaining
|
Review — CS-pve-agent1 PR#431 — head c5a1d64Dom: REQUEST CHANGES Evidence tags: [T] = I ran it and saw the result. [A] = analysis of the code or docs, not executed. [K] = known from CI or GitHub metadata. Overall, the Go side is solid.
Note: there is no earlier review on this PR and no earlier PR for #372, so I checked every point against the issue itself. Findings
Suggested non-behaviour-changing shape for #1: For #3, passing these variables through the compose Points vs. issue #372
Scope: no scope creep found. No product default is flipped in the env or app sense, apart from #1, and no new config is silently active. The five owner items are well argued, but item 4's "one line in Tests and mutantsOn the merged tree (
Red-before [T]: with the master runtime files plus the new tests, the JS suite shows 12 of 15 red, and all five compose/doc Go tests are red. The new ingestor and pprofbind tests do not compile without the new code. E2E [T]: local Go server on Live checks [T]:
CI [K]: Go Build & Test, Playwright E2E, Validate Docker Image and the Docker gate all passed on run 38391488315. Release, Deploy and Badges were skipped by the fork guards. Neither known flake (#256, #267) is involved. Mutants (mine) [T]:
Docker scripts [T]:
Not verified
|
…t-pprof-hardening
…eal broker mode Round-2 review fixes for #431 (#372). 1. Blocker — IPv4/IPv6 equivalence. `${X:-0.0.0.0}:port:port` is not the old unqualified mapping: docker publishes an unqualified port on both 0.0.0.0 and [::], while an explicit IPv4 host publishes IPv4 only, so pulling the new compose file dropped IPv6 reachability of the bundled broker and of pprof. Both binds are now conditional, `${X:+${X}:}port:port`, with an empty shipped default, so the rendered mapping is byte-identical to master when the variable is unset and narrows only when it is set. 2. Major — broker-check reported the entrypoint's reading of DISABLE_MOSQUITTO, but manage.sh resolves the value itself: dc_prod and dc_staging pick the compose file with is_true(), and the no-mosquitto files hard-code DISABLE_MOSQUITTO=true. So yes/1/on/TRUE are EXTERNAL on a manage.sh host, and the check used to say "bundled" and skip source validation. It now reports the mode manage.sh selects, and the gap warning names the launcher that actually reads the value differently. 3. Major — MQTT_SOURCE_STRICT, MQTT_BROKER, MQTT_TOPIC and the PPROF_HOST* / PPROF_PORT* settings never reached the container: no compose file forwarded them and none declared env_file. Every app compose file now forwards them through `environment:` with an EMPTY default, which every reader ignores, so an install that never set one is unaffected. 4. Minor — a credentialed loopback-only external source is now pinned, not only checked for leaks, so dropping the user-info strip in brokerHost no longer silences the diagnostic. 5. Nit — the legacy mqtt.broker block is counted only when mqttSources yields no source, matching LoadConfig. 6. Nit — cmd_start runs the preflight after migrate_config/ensure_config, so it reads the config.json the container will use. 7. Nit — ResolveBrokerMode no longer trims DISABLE_MOSQUITTO: the entrypoint compares it raw, so " true" keeps the bundled broker and is now reported as such. The five "Left for the owner" choices are unchanged; only item 5's factual rationale is corrected in the PR body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adminopenclaw8-sketch
changed the title
…dling Two guards for the finding-5 fix in broker_sources: an empty mqttSources array must fall back to the legacy mqtt.broker block, as LoadConfig does, and a "]" inside a topic pattern must not truncate the array. Each kills a distinct mutant of mqtt_sources_region (drop the bracket depth count; drop the string-awareness). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…BROKER Forwarding the in-process settings into the container (round-2 finding 3) is inert for every install that never set one — except MQTT_BROKER. Before this release the compose files did not forward it, so a value in .env was dead and config.json decided the sources; now it reaches the ingestor, where LoadConfig lets it override mqttSources entirely. A host with a stale MQTT_BROKER= line in .env and real mqttSources in config.json would switch to that single env source on the next restart, so the migration note, DEPLOY.md and .env.example now say so and point at ./manage.sh broker-check, which prints the sources in the ingestor's own precedence. TestDeploymentDocs_CarryTheComposePullMigrationNote is also scoped to the migration section instead of the whole document, so a mention elsewhere no longer satisfies it, and it now requires the MQTT_BROKER warning. Removing that paragraph turns it red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rapport — CS-Macmini PR#431 runde-2 — head fd0ce7cReview feedback addressed (commit Evidence tags: [T] = I ran it and saw the result. [A] = analysis, not executed. [K] = known from CI or GitHub metadata. Findings1 — Blocker: IPv4/IPv6 equivalence. Fixed. Both bind variables are now interpolated conditionally, with an empty shipped default, so an unset variable renders no host interface at all: - "${PROD_MQTT_BIND:+${PROD_MQTT_BIND}:}${PROD_MQTT_PORT:-1883}:1883"
- "${PPROF_BIND:+${PPROF_BIND}:}6060:6060"
Verified locally with Docker [T]:
And on the real repo files, 2 — Major: Verified against manage.sh's own
The reported mode matches the selected compose file for every value, and the review's repro ( 3 — Major: Inertness when unset is the load-bearing claim, so it is pinned on both sides [T]:
One non-inert consequence I found while doing this, and disclosed rather than hidden [A]. 4 — Minor: mutant M2 survived. Fixed. 5 — Nit: legacy 6 — Nit: preflight ran before 7 — Nit: trim mismatch. Fixed by matching the entrypoint. 8 — Nit: PR title. Fixed. Retitled to "Left for the owner" — unchanged in substance. All five choices remain deferred with the same recommendations. Two factual corrections only: the preamble now notes that each one-line TestsRed before, green after [T]:
Inertness guards (green by design, kept as regression guards): Suites [T]:
Live checks [T], locally built binaries on a CI-prepared
MutantsOne per finding, each red on exactly the intended test [T]:
CI per jobRun
Neither known flake (#271, #301) fired, so nothing was rerun. The earlier runs for The Go Build & Test pass also covers the one portability question in this round: Not verified
|
Review — CS-pve-agent2 PR#431 — head fd0ce7cDom: APPROVE med nits Evidence tags: [T] = I ran it and saw the result. [A] = analysis of the code or docs, not executed. [K] = known from CI or GitHub metadata. This is an independent re-review of round 2, checked against the round-1 review (CS-pve-agent1) and the round-2 report (CS-Macmini). All eight round-1 findings are resolved, and I re-verified each one myself (Docker, a manage.sh matrix, mutants). The compose defaults are now byte-identical to master and publish on both IPv4 and IPv6. What remains is one Minor gap and some nits. None is a regression against master. Findings
Answers to the re-review points
"Left for the owner" — unchanged in substance. I diffed the section against the round-1 PR body (from the edit history). The same five items carry the same recommendations. The only changes are the "reaches the process" sentence in the preamble and the corrected rationale in item 5, and that rationale matches my matrix: Always-checks:
Tests and mutantsOn the merged tree (
Red before → green after [T]:
The new ingestor and pprofbind tests do not compile against master, which counts as red. My own mutants [T]:
CI [K]: run 38404492174 on Not verified
|
Resolve the one conflict in docker-compose.example.yml by keeping both sides: #431's forwarded in-process settings (MQTT_BROKER, MQTT_TOPIC, MQTT_SOURCE_STRICT, ENABLE_PPROF, PPROF_*) stay active, and #339's commented opt-in non-root lines follow them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Relates to #372
Plan
The issue asks for three things that touch running deployments, and the hosts this
repo deploys to differ: one production runs its own Mosquitto, and another host runs
with pprof enabled and reached remotely. So the plan was to make every exposure
an explicit setting whose shipped default reproduces today's behaviour byte for
byte, add the validation and diagnostics that are inert by construction, and leave
every behaviour-changing flip to the owner.
(
PROD_MQTT_BIND,PPROF_BIND,PPROF_HOST, per-role pprof ports).internal/pprofbind)shared by the server and the ingestor, instead of two inline copies.
manage.sh,as diagnostics; make the refusal-to-start opt-in (
MQTT_SOURCE_STRICT).note, and keep every broker credential out of every diagnostic.
No default is flipped, no deployment is touched, and nothing was run against staging
or production.
What changed
Explicit bind settings (defaults reproduce today's behaviour)
PROD_MQTT_BINDdocker-compose.ymlpublishes the bundled broker's1883with no host interface, i.e. on both0.0.0.0and[::]— the rendered mapping is byte-identical to master127.0.0.1for host-onlyPPROF_BIND6060/6061with no host interface, i.e. IPv4 and IPv6 — as before127.0.0.1to require a tunnelPPROF_HOST(+PPROF_HOST_SERVER/_INGESTOR)":"+port— as before127.0.0.1for a process with no published portPPROF_PORT_SERVER/PPROF_PORT_INGESTOR6060/6061PPROF_PORTstill applies to bothMQTT_SOURCE_STRICTfalsetrueto refuse to startBoth bind variables are interpolated conditionally —
"${PROD_MQTT_BIND:+${PROD_MQTT_BIND}:}${PROD_MQTT_PORT:-1883}:1883"— so an unsetvariable renders no host interface at all. A
:-0.0.0.0default would not have beenequivalent: docker publishes an unqualified mapping on both
0.0.0.0and[::], butan explicit IPv4 host publishes IPv4 only, which would have dropped IPv6
reachability on the next
up -d.docker compose configon all five compose filesnow renders port mappings byte-identical to
origin/masterwith nothing set, anddocker portshows0.0.0.0:Pand[::]:Pas before.PPROF_BINDandPPROF_HOSTare deliberately separate and documented as such: aloopback listener inside a container is unreachable through a docker port mapping,
because docker's proxy connects to the container's network address. Narrowing a
containerised deploy is
PPROF_BIND's job;PPROF_HOSTis for a process that runswithout a port mapping.
internal/pprofbind(new, shared module)One resolver for both process roles, replacing the two inline
":"+pprofPortcopies.ENABLE_PPROFstays opt-in on the exact stringtrue— unchanged — and a value like1orTRUEnow says in the log that it left pprof off instead of being swallowed.Per-role ports exist because the pre-existing shared
PPROF_PORTapplied to bothprocesses, so moving one collided the other; the shared variable still works.
In-process settings reach the container
MQTT_BROKER,MQTT_TOPIC,MQTT_SOURCE_STRICT,ENABLE_PPROFand thePPROF_HOST*/PPROF_PORT*settings are read by the server and the ingestor, not bydocker compose, so every app compose file (
docker-compose.yml,.no-mosquitto.yml,both staging files and
.example.yml) forwards them through itsenvironment:blockwith an empty default. An unset setting arrives as an empty string, which every
reader ignores —
pprofbindskips a blank host or port,LoadConfigskips a blankMQTT_BROKER,MQTT_SOURCE_STRICTis false unless truthy — so an install that neverset one is unaffected. Without this, the documented "one line in
.env" reacheddocker compose and stopped there.
One non-inert consequence, disclosed in the migration note.
MQTT_BROKERoverrides
mqttSourcesinconfig.json(LoadConfigprecedence). Before thecompose files forwarded it, a value in
.envwas dead andconfig.jsondecided; nowit decides. A host with a stale
MQTT_BROKER=line in.envand realmqttSourceswould switch to that single env source on the next restart.docs/deployment.md,DEPLOY.mdand.env.examplesay so and point at./manage.sh broker-check, which prints the sources in the ingestor's ownprecedence. This is the only behaviour an existing install can see change in this
PR; everything else is inert when unset.
Broker validation (ingestor +
manage.sh)DISABLE_MOSQUITTO=trueturns the bundled broker off but configures no replacement.With no
mqttSources,LoadConfigfalls back tomqtt://localhost:1883— theaddress of the broker that was just disabled — so ingest stops while the container
still reports healthy. Reported now, before any connection attempt:
(warning, the rest still ingest)
DISABLE_MOSQUITTOvalue whose meaning depends on the launcher (warning)../manage.shresolves the value itself:dc_prod/dc_stagingpick the composefile with
is_true()(1/true/TRUE/yes/y/on), and the no-mosquittofiles hard-code
DISABLE_MOSQUITTO=true, so on a manage.sh-managed host every oneof those values gives an external broker. A bare
docker compose upagainstdocker-compose.ymlpasses the value straight to the entrypoint, which disablesthe bundled broker only on the exact
true, soyesthere leaves the bundledbroker running.
broker-checkreports the mode manage.sh will select andwarns about the gap; the ingestor's
[broker] warn:line reports the mode its owncontainer is in.
ResolveBrokerModecompares the value untrimmed, exactly as theentrypoint does.
manage.sh broker-checkreports the same and exits1, so a runbook can gate on it;setup,start,update,restoreandpromoterun it advisory-only and neverrefuse a command over it (
cmd_startruns it aftermigrate_config/ensure_config,so it reads the config the container will use). Environment precedence matches the
ingestor's (
MQTT_BROKERoverconfig.json), and the legacy single-brokermqtt.brokerblock counts as a source only whenmqttSourcesyields none, whichis when
LoadConfigpromotes it.Credentials out of diagnostics
Every broker URL in a Go diagnostic goes through the existing
brokerurl.Mask, andhosts are parsed out of the masked form, so no user-info, query or fragment can
reach a comparison or a message.
manage.shgained amask_broker_textfilter usedfor the broker report and for the
docker logslinesverify_healthalready echoed(those can come from an ingestor of any version).
Reuse, not parallel helpers
brokerurlfor masking;stripYAMLComments/extractSubBlockfromstaging_compose_broker_test.gofor the compose-shape assertions (itsextractStagingGoBlockwas generalised toextractServiceBlockrather than copied);the
is_true()truthiness set is mirrored rather than redefined. The new internalmodule is wired into both
go.modfiles, bothDockerfilebuilder sections (thecheck-dockerfile-internal-pkgs.shinvariant), and the CI shared-packages step.Left for the owner
Each of these changes behaviour on at least one existing install, so none is applied
here. All are one line in
.env— and, since round 2, every one of those linesactually reaches the process that reads it (see "In-process settings reach the
container" above).
PROD_MQTT_BIND=127.0.0.1.Recommended where no gateway connects to the bundled broker from off-host. The
bundled broker is plaintext MQTT with no auth; publishing it on every interface is
the exposure the issue is about. Do not set it where observers reach
1883directly — they would lose the broker at the next
up -d.PPROF_BIND=127.0.0.1.Recommended, with an SSH tunnel (
ssh -L 6060:127.0.0.1:6060 host) for access./debug/pprof/is unauthenticated and can be used to stall a process. This oneneeds a decision: at least one host runs with pprof enabled and is profiled
remotely, and this would require a tunnel there.
DISABLE_MOSQUITTOdefault fromfalsetotrueindocker-compose.yml. Recommended as the eventual shape, butit is the one change that can silently stop ingest on an install that relies on the
bundled broker, so it wants a release note rather than a quiet default flip.
Nothing here depends on it.
MQTT_SOURCE_STRICT=true. Recommended on hostswith an external broker: it converts "healthy but ingesting nothing" into a loud
startup failure. Leave it off where a start during broker maintenance must succeed.
DISABLE_MOSQUITTOparsing. The entrypoint takes only the exacttrue;manage.sh'sis_true()also takes1/TRUE/yes/y/on. This PRreports the gap rather than closing it. Note the corrected reasoning: a
manage.sh-managed host set to
yesis already external, becausedc_prodswaps in
docker-compose.no-mosquitto.yml, which hard-codesDISABLE_MOSQUITTO=true. The host that would move is one started with a baredocker compose upagainstdocker-compose.yml, whereyesleaves the bundledbroker running — teaching the entrypoint
is_true()would switch that host to anexternal broker on the next restart. Recommended once no host uses such a value
—
broker-checkwill say.Verification
cd cmd/server && go test ./...— ok (48s)cd cmd/ingestor && go test ./...— ok (110s)cd internal/pprofbind && go vet ./... && go test -race ./...— oksh test-all.sh— 240 passed, 0 failednode test-frontend-helpers.js— 713 passed, 0 failedsh scripts/check-xss-sinks.sh --diff origin/master— clean (nopublic/**changes)/api/healthz,/api/health,/api/stats,/api/mqtt/statusall 200.PPROF_HOST=127.0.0.1→/debug/pprof/200 onloopback, connection refused via the host address. Default (no
PPROF_HOST) →200 via the host address, confirming the default is unchanged.
ENABLE_PPROF=1→pprof absent, with the new reason logged.
deploy.yml, 1 inrelease-fast-path.yml.cmd/serverread-only invariant test passes; no newmap[string]interface{}outside tests; no hardcoded colours (no frontend files touched).
deps are not installed in this environment; CI's E2E job covers them.