diff --git a/.env.example b/.env.example index 338b5ac70..dffd54c7e 100644 --- a/.env.example +++ b/.env.example @@ -42,10 +42,15 @@ PROD_MQTT_PORT=1883 # Used by: docker compose PROD_MQTT_BIND= -# Disable internal Mosquitto broker (set true to use external MQTT only) -# Default: false -# Used by: manage.sh + docker compose overrides -DISABLE_MOSQUITTO=false +# Disable the bundled Mosquitto broker. The bundled broker is OPT-IN (#441): +# leave this true and point mqttSources in config.json (or MQTT_BROKER below) +# at your own broker, or set it to false to run the bundled broker. +# true/1/yes/on and false/0/no/off are accepted, trimmed and in any case, by +# manage.sh, docker compose + the container entrypoint and the ingestor +# alike; an unset, empty or unrecognised value means true. +# Default: true +# Used by: manage.sh + docker compose overrides + the container entrypoint +DISABLE_MOSQUITTO=true # --- Staging (HTTP only, no HTTPS) --- # Data directory @@ -80,7 +85,7 @@ MQTT_BROKER= MQTT_TOPIC= # Refuse to start the ingestor when the broker setup cannot ingest anything, -# instead of only logging it. With DISABLE_MOSQUITTO=true and no mqttSources +# instead of only logging it. With the bundled broker off and no mqttSources # (or only loopback ones) the ingestor connects to a broker that is not # running; this turns that into a hard failure at startup. # Accepted truthy values: 1 true TRUE yes YES y Y on ON @@ -96,22 +101,24 @@ MQTT_SOURCE_STRICT=false ENABLE_PPROF=false # Host interface the pprof HOST PORTS are published on, for a container whose -# compose file maps them (docker-compose.staging*.yml). EMPTY (the default) -# publishes them with no host interface, i.e. both IPv4 and IPv6, which is -# what those files have always done; 127.0.0.1 requires an SSH tunnel to -# reach the profiler. Writing 0.0.0.0 here is NOT the same as leaving it -# empty: it publishes IPv4 only. -# Default: (empty - every interface, IPv4 and IPv6) +# compose file maps them (docker-compose.staging*.yml). Empty means the +# default, 127.0.0.1 (#441): /debug/pprof/ is unauthenticated, so reach it +# through an SSH tunnel — +# ssh -L 6060:127.0.0.1:6060 -L 6061:127.0.0.1:6061 you@host +# Set 0.0.0.0 to publish it on every IPv4 interface again (remote access). +# Default: 127.0.0.1 # Used by: docker compose PPROF_BIND= -# Host the pprof listener INSIDE the process binds to. Empty means every -# interface, which is what the server and ingestor have always done. Set it -# to 127.0.0.1 for a process that runs without a published port — a loopback -# listener is NOT reachable through a docker port mapping, so use PPROF_BIND -# for a container instead. +# Host the pprof listener INSIDE the process binds to. Empty means the +# default, 127.0.0.1 (#441). Set 0.0.0.0 (or *) to listen on every interface +# — needed for a process whose pprof port is reached from outside it without +# a compose file that handles this: a bare binary profiled remotely, or a +# `docker run -p ...:6060:6060`, since a loopback listener is NOT reachable +# through a docker port mapping. The staging compose files pin it to 0.0.0.0 +# themselves and narrow the published port with PPROF_BIND instead. # Per-role overrides: PPROF_HOST_SERVER, PPROF_HOST_INGESTOR below. -# Default: +# Default: 127.0.0.1 # Used by: the server + the ingestor PPROF_HOST= diff --git a/DEPLOY.md b/DEPLOY.md index bb01624de..d14a3afdf 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -41,10 +41,10 @@ Settings can be overridden via environment variables: | Variable | Default | Description | |----------|---------|-------------| | `DISABLE_CADDY` | `false` | Skip internal Caddy (set `true` behind a reverse proxy) | -| `DISABLE_MOSQUITTO` | `true` in `docker-compose.staging.yml`; `false` elsewhere | Skip internal MQTT broker. **Which values count depends on the launcher.** `./manage.sh` resolves it with `is_true()` (`1`/`true`/`TRUE`/`yes`/`YES`/`y`/`Y`/`on`/`ON`) and then runs `docker-compose.no-mosquitto.yml`, which hard-codes `DISABLE_MOSQUITTO=true` in the container — so any of those values gives an **external** broker on a manage.sh-managed host. A bare `docker compose up` against `docker-compose.yml` passes the value straight through, and the entrypoint disables the broker only on the exact `true`, so there `yes` leaves the bundled broker **running**. `./manage.sh broker-check` reports the mode this script will actually select, and warns when the value falls in that gap. Default flipped to `true` for the staging deploy in v3.7+ because a standalone `mqtt-broker` container owns MQTT on that host — see "Standalone MQTT broker (staging)" below. | +| `DISABLE_MOSQUITTO` | `true` | Skip the bundled MQTT broker. The bundled broker is **opt-in** (#441): set `false` to run it. `./manage.sh`, the container entrypoint and the ingestor read the value the same way — `true`/`1`/`yes`/`on` and `false`/`0`/`no`/`off`, trimmed, any case; unset, empty or anything else means `true` (and an unrecognised value is reported). `./manage.sh broker-check` prints the effective mode. | | `PROD_MQTT_BIND` | *(empty — every interface, IPv4 and IPv6)* | Host interface the bundled broker's `1883` is published on. Empty renders the mapping with no host interface, which is the unchanged dual-stack publish; `127.0.0.1` makes the plaintext broker reachable from the host only. An explicit `0.0.0.0` is **IPv4 only** — not the same as empty. | -| `PPROF_BIND` | *(empty — every interface, IPv4 and IPv6)* | Host interface the staging pprof ports `6060`/`6061` are published on. Empty is the unchanged dual-stack publish; `127.0.0.1` requires an SSH tunnel to profile. An explicit `0.0.0.0` is IPv4 only. | -| `PPROF_HOST` | *(every interface)* | Host the in-process pprof listener binds. **Not** a substitute for `PPROF_BIND`: a loopback listener inside a container is unreachable through a docker port mapping. | +| `PPROF_BIND` | `127.0.0.1` | Host interface the staging pprof ports `6060`/`6061` are published on (#441). Profile through an SSH tunnel (`ssh -L 6060:127.0.0.1:6060 -L 6061:127.0.0.1:6061 you@host`), or set `0.0.0.0` for remote access over IPv4. | +| `PPROF_HOST` | `127.0.0.1` | Host the in-process pprof listener binds (#441); `0.0.0.0` or `*` for every interface. **Not** a substitute for `PPROF_BIND`: a loopback listener inside a container is unreachable through a docker port mapping, which is why the staging compose files pin it to `0.0.0.0`. | | `PPROF_PORT_SERVER` / `PPROF_PORT_INGESTOR` | `6060` / `6061` | Per-process pprof ports; they must stay distinct. Override the shared `PPROF_PORT`. | | `MQTT_SOURCE_STRICT` | `false` | Make the ingestor refuse to start when its broker setup cannot ingest, instead of only logging it. | | `MQTT_BROKER` / `MQTT_TOPIC` | *(empty — `config.json` decides)* | External broker URL and topic for the ingestor; `MQTT_BROKER` replaces the `mqttSources` in `config.json`. | @@ -53,7 +53,12 @@ Settings can be overridden via environment variables: `PPROF_HOST*` / `PPROF_PORT*` settings are read by the server and the ingestor, not by docker compose, so every app compose file forwards them into the container through its `environment:` block with an empty default. Setting one -in `.env` therefore does reach the process; leaving it unset is inert. +in `.env` therefore does reach the process; leaving it unset is inert. The +staging compose files pin `PPROF_HOST=0.0.0.0` instead, because they publish +the pprof ports. + +Upgrading past #441 changes two defaults — the bundled broker and the pprof +bind — see [docs/deployment.md, "Upgrading past #441"](docs/deployment.md#upgrading-past-441--bundled-broker-opt-in-pprof-on-loopback). One caveat: `MQTT_BROKER` **overrides** `mqttSources` in `config.json`. Before the compose files forwarded it, a value in `.env` was ignored by the container; @@ -309,8 +314,8 @@ not in `/tmp`, which is ephemeral by definition. ## Broker preflight (`./manage.sh broker-check`) -`DISABLE_MOSQUITTO=true` turns the bundled broker off; it does not configure -a replacement. With no `mqttSources` the ingestor falls back to its built-in +The bundled broker is opt-in, and turning it off (the default, +`DISABLE_MOSQUITTO=true`) does not configure a replacement. With no `mqttSources` the ingestor falls back to its built-in `mqtt://localhost:1883` — the bundled broker's own address, where nothing is then listening — and ingests nothing while the container still reports healthy. diff --git a/README.md b/README.md index 8d6b92879..08d98c467 100644 --- a/README.md +++ b/README.md @@ -99,13 +99,14 @@ docker run -d --name corescope-meshview \ --restart=unless-stopped \ -p 127.0.0.1:8080:80 \ -p 127.0.0.1:1883:1883 \ + -e DISABLE_MOSQUITTO=false \ --mount "type=bind,source=$(pwd)/data,target=/app/data" \ corescope-meshview:local ``` Open [http://localhost:8080](http://localhost:8080). The included Caddy proxy forwards HTTP requests to the Go server. The database and configuration persist in `./data`; packets appear once an observer publishes to the local MQTT broker. -This example exposes HTTP and MQTT on the Docker host's loopback interface. A publisher on that host can connect to `mqtt://localhost:1883`. To collect from an external broker, replace `mqttSources` with that broker's connection settings and restart the container. You can then disable the included broker with `-e DISABLE_MOSQUITTO=true` and omit the MQTT port mapping. +This example exposes HTTP and MQTT on the Docker host's loopback interface. The included broker is opt-in, which is what `-e DISABLE_MOSQUITTO=false` does; a publisher on that host can then connect to `mqtt://localhost:1883`. To collect from an external broker, replace `mqttSources` with that broker's connection settings, drop `-e DISABLE_MOSQUITTO=false` and the MQTT port mapping, and recreate the container. ```bash docker logs -f corescope-meshview diff --git a/cmd/ingestor/broker_mode.go b/cmd/ingestor/broker_mode.go index 4d0e46fb6..26ef30ebc 100644 --- a/cmd/ingestor/broker_mode.go +++ b/cmd/ingestor/broker_mode.go @@ -2,30 +2,25 @@ package main // Broker-mode diagnostics (#372). // -// CoreScope ships a bundled mosquitto that an operator can turn off with -// DISABLE_MOSQUITTO=true to point the ingestor at their own broker. Two -// things could go wrong silently in that arrangement: +// CoreScope ships a bundled mosquitto. It is opt-in (#441): it runs only +// when DISABLE_MOSQUITTO is a recognised false value, and otherwise the +// ingestor is expected to reach the operator's own broker. Two things could +// go wrong silently in that arrangement: // // 1. Nothing validates the external source. LoadConfig falls back to // mqtt://localhost:1883 when no mqttSources are configured, which is // the bundled broker's address — with the bundled broker off, the // ingestor then connects to nothing and ingest stops with no explanation // beyond a connection error in the log. -// 2. The value of DISABLE_MOSQUITTO is read differently by different -// launchers. docker/entrypoint-go.sh — the only reader inside this -// container — disables mosquitto on exactly "true". manage.sh's -// is_true() also accepts 1/TRUE/yes/y/on, but it resolves the value -// itself: dc_prod/dc_staging swap in docker-compose.no-mosquitto.yml, -// which hard-codes DISABLE_MOSQUITTO=true in the container. So a -// container that can see "yes" here was started WITHOUT manage.sh, by -// a bare `docker compose up` against docker-compose.yml, and the -// bundled broker is running even though the operator asked for an -// external one. +// 2. A DISABLE_MOSQUITTO value nobody recognises. docker/entrypoint-go.sh, +// manage.sh and this file read the value with one table +// (test-fixtures/disable-mosquitto-cases.json), so they agree on every +// value, but a typo silently takes the default. // // This file resolves the mode the way the entrypoint does and reports both // hazards as diagnostics. They are diagnostics, not refusals: an install -// that merely updates must behave exactly as before. MQTT_SOURCE_STRICT -// turns the errors into a refusal to start for operators who want that. +// that merely updates must keep starting. MQTT_SOURCE_STRICT turns the +// errors into a refusal to start for operators who want that. // // Every broker URL reaches a diagnostic through brokerurl.Mask, and hosts // are read out of the masked form, so no password, user name or query token @@ -61,32 +56,40 @@ type BrokerMode struct { // decided exactly as docker/entrypoint-go.sh decides it. Bundled bool // Raw is the DISABLE_MOSQUITTO value exactly as the environment carries - // it — untrimmed, because the entrypoint compares it untrimmed. + // it, for diagnostics. Raw string - // Ambiguous is true when Raw is neither the exact "true" the entrypoint - // requires nor a recognised false value. Such a value leaves the - // bundled broker running in this container even though the operator - // who typed it probably meant to disable it. - Ambiguous bool + // Unrecognised is true when Raw is neither a true nor a false spelling, + // so the default (bundled broker off) applied. + Unrecognised bool } // ResolveBrokerMode reads DISABLE_MOSQUITTO through getenv (os.Getenv in -// production, a map in tests). -// -// The value is compared RAW, not trimmed: docker/entrypoint-go.sh does -// `[ "${DISABLE_MOSQUITTO:-false}" = "true" ]`, so " true" is not "true" to -// it and the bundled broker keeps running. Trimming here would report the -// opposite mode to the one this container is actually in. Such a value can -// only reach the container through the process environment — manage.sh's -// own .env reader and docker compose's dotenv parser both trim — which is -// exactly the case Ambiguous exists to flag. +// production, a map in tests). Trimmed of POSIX whitespace and compared +// case-insensitively, true/1/yes/on disable the bundled broker and +// false/0/no/off enable it; unset, empty and anything else take the +// default, disabled. docker/entrypoint-go.sh and manage.sh's +// parse_disable_mosquitto apply the same table. func ResolveBrokerMode(getenv func(string) string) BrokerMode { raw := getenv("DISABLE_MOSQUITTO") - mode := BrokerMode{Raw: raw, Bundled: raw != "true"} - mode.Ambiguous = mode.Bundled && !isClearlyFalse(raw) + mode := BrokerMode{Raw: raw} + switch strings.ToLower(strings.Trim(raw, " \t\n\v\f\r")) { + case "false", "0", "no", "off": + mode.Bundled = true + case "true", "1", "yes", "on", "": + default: + mode.Unrecognised = true + } return mode } +// setting renders the DISABLE_MOSQUITTO value a diagnostic is about. +func (m BrokerMode) setting() string { + if m.Raw == "" { + return "DISABLE_MOSQUITTO unset (default true)" + } + return fmt.Sprintf("DISABLE_MOSQUITTO=%q", m.Raw) +} + // StrictBrokerSources reports whether MQTT_SOURCE_STRICT asks for broker // errors to stop the process instead of being logged. It is a new setting // with no legacy spelling to preserve, so it accepts the same truthy values @@ -95,7 +98,8 @@ func StrictBrokerSources(getenv func(string) string) bool { return isShellTruthy(strings.TrimSpace(getenv("MQTT_SOURCE_STRICT"))) } -// isShellTruthy mirrors manage.sh's is_true(). +// isShellTruthy is the truthy set .env.example documents for +// MQTT_SOURCE_STRICT. func isShellTruthy(v string) bool { switch v { case "1", "true", "TRUE", "yes", "YES", "y", "Y", "on", "ON": @@ -104,18 +108,6 @@ func isShellTruthy(v string) bool { return false } -// isClearlyFalse reports whether a value unambiguously means "keep the -// bundled broker". Anything outside both this set and the exact "true" is -// ambiguous: the entrypoint keeps the bundled broker, but the operator who -// typed it may well have meant the opposite. -func isClearlyFalse(v string) bool { - switch v { - case "", "false", "FALSE", "False", "0", "no", "NO", "n", "N", "off", "OFF": - return true - } - return false -} - // brokerSchemes are the schemes paho reaches a broker with, after // Config.ResolvedSources maps mqtt:// and mqtts:// onto tcp:// and ssl://. var brokerSchemes = map[string]bool{ @@ -131,14 +123,12 @@ var brokerSchemes = map[string]bool{ func CheckBrokerSources(mode BrokerMode, sources []MQTTSource, sourcesDefaulted bool) []BrokerDiagnostic { var diags []BrokerDiagnostic - if mode.Ambiguous { + if mode.Unrecognised { diags = append(diags, BrokerDiagnostic{ Severity: SeverityWarn, - Message: fmt.Sprintf("DISABLE_MOSQUITTO=%q is neither the exact %q the container entrypoint requires nor a recognised false value, "+ - "so the bundled broker WILL start in this container. A container that sees this value was started by a bare "+ - "`docker compose up` against docker-compose.yml: ./manage.sh resolves the value itself and swaps in "+ - "docker-compose.no-mosquitto.yml, which passes the exact %q instead. Set it to %q or %q so every launcher reads it the same way.", - mode.Raw, "true", "true", "true", "false"), + Message: fmt.Sprintf("DISABLE_MOSQUITTO=%q is not a recognised value (true/1/yes/on or false/0/no/off), "+ + "so the default applies and the bundled broker is off. Set it to %q to run the bundled broker, or to %q.", + mode.Raw, "false", "true"), }) } @@ -170,23 +160,25 @@ func CheckBrokerSources(mode BrokerMode, sources []MQTTSource, sourcesDefaulted case sourcesDefaulted: diags = append(diags, BrokerDiagnostic{ Severity: SeverityError, - Message: "DISABLE_MOSQUITTO=true selects an external MQTT broker, but no mqttSources are configured: " + + Message: mode.setting() + " selects an external MQTT broker, but no mqttSources are configured: " + "the ingestor fell back to its built-in mqtt://localhost:1883, which is the bundled broker's address and " + - "nothing is listening there. Configure mqttSources in config.json (or set MQTT_BROKER) to the external broker.", + "nothing is listening there. Configure mqttSources in config.json (or set MQTT_BROKER) to the external broker, " + + "or set DISABLE_MOSQUITTO=false to run the bundled broker (it is opt-in).", }) case usable == 0: // Every source was malformed; the errors above already say so. case loopback == usable: diags = append(diags, BrokerDiagnostic{ Severity: SeverityError, - Message: fmt.Sprintf("DISABLE_MOSQUITTO=true disables the bundled broker, but all %d configured mqtt source(s) point at loopback (%s): "+ - "nothing is listening there. Point them at the external broker's address.", usable, maskedBrokerList(sources)), + Message: fmt.Sprintf("%s disables the bundled broker, but all %d configured mqtt source(s) point at loopback (%s): "+ + "nothing is listening there. Point them at the external broker's address, or set DISABLE_MOSQUITTO=false to run the bundled broker.", + mode.setting(), usable, maskedBrokerList(sources)), }) case loopback > 0: diags = append(diags, BrokerDiagnostic{ Severity: SeverityWarn, - Message: fmt.Sprintf("DISABLE_MOSQUITTO=true disables the bundled broker, so %d of %d mqtt source(s) pointing at loopback cannot connect (%s); "+ - "the remaining source(s) still ingest.", loopback, usable, maskedBrokerList(sources)), + Message: fmt.Sprintf("%s disables the bundled broker, so %d of %d mqtt source(s) pointing at loopback cannot connect (%s); "+ + "the remaining source(s) still ingest.", mode.setting(), loopback, usable, maskedBrokerList(sources)), }) } return diags diff --git a/cmd/ingestor/broker_mode_test.go b/cmd/ingestor/broker_mode_test.go index f33575c67..cea4325fb 100644 --- a/cmd/ingestor/broker_mode_test.go +++ b/cmd/ingestor/broker_mode_test.go @@ -1,8 +1,10 @@ package main import ( + "encoding/json" "fmt" "os" + "path/filepath" "regexp" "strings" "testing" @@ -24,101 +26,104 @@ func diagText(diags []BrokerDiagnostic) string { return b.String() } -func TestResolveBrokerMode_MatchesTheEntrypointSpelling(t *testing.T) { - // docker/entrypoint-go.sh disables the bundled mosquitto on exactly - // `DISABLE_MOSQUITTO=true`. The ingestor must read the same value the - // same way, or it would report a broker mode the container is not in. - for _, tc := range []struct { - value string - wantBundled bool - wantAmbig bool - }{ - {"", true, false}, - {"false", true, false}, - {"FALSE", true, false}, - {"0", true, false}, - {"true", false, false}, - // Review round 2, finding 7: docker/entrypoint-go.sh compares the - // RAW value, so a surrounding space is NOT "true" to it and the - // bundled broker keeps running. Trimming here would report the - // opposite mode to the one the container is in. Such a value can - // only arrive through the process environment — manage.sh's .env - // reader and docker compose's dotenv parser both trim [T]. - {" true ", true, true}, - {" true", true, true}, - {"true ", true, true}, - {"\ttrue", true, true}, - {"TRUE", true, true}, - {"True", true, true}, - {"1", true, true}, - {"yes", true, true}, - {"on", true, true}, - {"y", true, true}, - } { - got := ResolveBrokerMode(brokerEnv(map[string]string{"DISABLE_MOSQUITTO": tc.value})) - if got.Bundled != tc.wantBundled { - t.Errorf("DISABLE_MOSQUITTO=%q: Bundled = %v, want %v", tc.value, got.Bundled, tc.wantBundled) - } - if got.Ambiguous != tc.wantAmbig { - t.Errorf("DISABLE_MOSQUITTO=%q: Ambiguous = %v, want %v", tc.value, got.Ambiguous, tc.wantAmbig) - } - } +// disableMosquittoCase is one row of test-fixtures/disable-mosquitto-cases.json, +// the table docker/entrypoint-go.sh and manage.sh are held to as well +// (test-issue-441-disable-mosquitto.js). A nil Value means unset. +type disableMosquittoCase struct { + Value *string `json:"value"` + Disabled bool `json:"disabled"` + Recognised bool `json:"recognised"` } -func TestBrokerMode_AmbiguousValueIsReportedNotSilentlyResolved(t *testing.T) { - // A value the entrypoint does not accept leaves the BUNDLED broker - // running in this container, while the operator who typed it most - // likely meant the opposite. The issue requires that this cannot - // happen silently. - mode := ResolveBrokerMode(brokerEnv(map[string]string{"DISABLE_MOSQUITTO": "yes"})) - diags := CheckBrokerSources(mode, []MQTTSource{{Name: "ext", Broker: "mqtt://broker.example:1883"}}, false) - if len(diags) == 0 { - t.Fatalf("an ambiguous DISABLE_MOSQUITTO value must produce a diagnostic") +func loadDisableMosquittoCases(t *testing.T) []disableMosquittoCase { + t.Helper() + data, err := os.ReadFile(filepath.Join("..", "..", "test-fixtures", "disable-mosquitto-cases.json")) + if err != nil { + t.Fatalf("read the shared DISABLE_MOSQUITTO table: %v", err) } - joined := diagText(diags) - for _, want := range []string{"DISABLE_MOSQUITTO", "yes", "bundled"} { - if !strings.Contains(joined, want) { - t.Errorf("diagnostic must mention %q; got:\n%s", want, joined) - } + var table struct { + Cases []disableMosquittoCase `json:"cases"` } - if sev := WorstBrokerSeverity(diags); sev != SeverityWarn { - t.Errorf("an ambiguous value is a warning, not an error (it must not block a start); got %q", sev) + if err := json.Unmarshal(data, &table); err != nil { + t.Fatalf("parse the shared DISABLE_MOSQUITTO table: %v", err) + } + if len(table.Cases) == 0 { + t.Fatal("the shared DISABLE_MOSQUITTO table is empty") } + return table.Cases } -// Review round 2, finding 2: manage.sh does NOT leave the bundled broker -// running on an is_true() value. dc_prod/dc_staging swap in -// docker-compose.no-mosquitto.yml, which hard-codes DISABLE_MOSQUITTO=true -// in the container's environment — so a container that can see "yes" at all -// was NOT launched by manage.sh. The diagnostic must therefore not tell the -// operator that manage.sh believes it disabled the broker; it must name the -// launcher that actually delivers this value, a bare docker compose run. -func TestAmbiguousDiagnostic_DoesNotMisattributeTheModeToManageSh(t *testing.T) { - mode := ResolveBrokerMode(brokerEnv(map[string]string{"DISABLE_MOSQUITTO": "yes"})) - diags := CheckBrokerSources(mode, []MQTTSource{{Name: "ext", Broker: "mqtt://broker.example:1883"}}, false) - joined := diagText(diags) - if !strings.Contains(joined, "docker compose") { - t.Errorf("the diagnostic must name the launcher that delivers an untranslated value (a bare `docker compose` run); got:\n%s", joined) - } - // It may mention manage.sh, but only to say it would have selected the - // external compose file — never that it "may believe" the broker was - // disabled while this container still runs it. - for _, forbidden := range []string{"is_true() may read", "may believe", "manage.sh's is_true(), which accepts"} { - if strings.Contains(joined, forbidden) { - t.Errorf("the diagnostic still attributes this mode to manage.sh (%q); got:\n%s", forbidden, joined) +// envFor returns the environment one table row describes. os.Getenv reads +// an unset variable as "", which is what a missing key gives here. +func (c disableMosquittoCase) envFor() map[string]string { + if c.Value == nil { + return map[string]string{} + } + return map[string]string{"DISABLE_MOSQUITTO": *c.Value} +} + +func (c disableMosquittoCase) String() string { + if c.Value == nil { + return "" + } + return fmt.Sprintf("%q", *c.Value) +} + +// Issue #441: the bundled broker is opt-in, and the ingestor reads +// DISABLE_MOSQUITTO exactly as the entrypoint and manage.sh do — trimmed, +// case-insensitive, true/1/yes/on and false/0/no/off, anything else the +// default (true). +func TestResolveBrokerMode_FollowsTheSharedTable(t *testing.T) { + for _, c := range loadDisableMosquittoCases(t) { + got := ResolveBrokerMode(brokerEnv(c.envFor())) + if got.Bundled == c.Disabled { + t.Errorf("DISABLE_MOSQUITTO=%s: Bundled = %v, want %v", c, got.Bundled, !c.Disabled) + } + if got.Unrecognised == c.Recognised { + t.Errorf("DISABLE_MOSQUITTO=%s: Unrecognised = %v, want %v", c, got.Unrecognised, !c.Recognised) } } - if strings.Contains(joined, "manage.sh") && !strings.Contains(joined, "no-mosquitto") { - t.Errorf("if the diagnostic mentions manage.sh it must say it swaps in the no-mosquitto compose file; got:\n%s", joined) +} + +func TestBrokerMode_JunkValueIsReportedAndFallsBackToTheDefault(t *testing.T) { + good := []MQTTSource{{Name: "ext", Broker: "mqtt://broker.example:1883"}} + for _, c := range loadDisableMosquittoCases(t) { + if c.Recognised { + continue + } + mode := ResolveBrokerMode(brokerEnv(c.envFor())) + diags := CheckBrokerSources(mode, good, false) + joined := diagText(diags) + if WorstBrokerSeverity(diags) != SeverityWarn { + t.Errorf("DISABLE_MOSQUITTO=%s is junk: want exactly a warning (it must not block a start); got:\n%s", c, joined) + } + for _, want := range []string{"DISABLE_MOSQUITTO", *c.Value, "not a recognised", "default"} { + if !strings.Contains(joined, want) { + t.Errorf("DISABLE_MOSQUITTO=%s: the warning must mention %q; got:\n%s", c, want, joined) + } + } } } -func TestBrokerMode_UnambiguousValuesAreQuiet(t *testing.T) { - for _, value := range []string{"", "false", "true"} { - mode := ResolveBrokerMode(brokerEnv(map[string]string{"DISABLE_MOSQUITTO": value})) - diags := CheckBrokerSources(mode, []MQTTSource{{Name: "ext", Broker: "mqtt://broker.example:1883"}}, false) - if len(diags) != 0 { - t.Errorf("DISABLE_MOSQUITTO=%q with a usable external source must be quiet; got:\n%s", value, diagText(diags)) +// Before #441 a value such as "yes" was read one way by manage.sh and +// another by the entrypoint, and the ingestor said so. Parsing is now the +// same everywhere, so no recognised value may produce a diagnostic about +// it: with a usable source in the mode the value selects, the check is +// silent. +func TestBrokerMode_RecognisedValuesReportNoMismatch(t *testing.T) { + external := []MQTTSource{{Name: "ext", Broker: "mqtt://broker.example:1883"}} + bundled := []MQTTSource{{Name: "local", Broker: "mqtt://localhost:1883"}} + for _, c := range loadDisableMosquittoCases(t) { + if !c.Recognised { + continue + } + mode := ResolveBrokerMode(brokerEnv(c.envFor())) + sources := external + if !c.Disabled { + sources = bundled + } + if diags := CheckBrokerSources(mode, sources, false); len(diags) != 0 { + t.Errorf("DISABLE_MOSQUITTO=%s with a usable source must be quiet; got:\n%s", c, diagText(diags)) } } } @@ -173,7 +178,7 @@ func TestExternalMode_OneLoopbackAmongRealSourcesIsOnlyAWarning(t *testing.T) { } func TestBundledMode_LoopbackSourcesAreFine(t *testing.T) { - mode := ResolveBrokerMode(brokerEnv(map[string]string{})) + mode := ResolveBrokerMode(brokerEnv(map[string]string{"DISABLE_MOSQUITTO": "false"})) diags := CheckBrokerSources(mode, []MQTTSource{{Name: "local", Broker: "mqtt://localhost:1883"}}, true) if len(diags) != 0 { t.Fatalf("the bundled broker listens on loopback, so this is the normal shape; got:\n%s", diagText(diags)) @@ -372,10 +377,10 @@ func TestReportBrokerSetup_LogsEveryDiagnosticAndOnlyRefusesUnderStrict(t *testi t.Errorf("the fatal message must name the setting that caused it; got %q", msg) } - // Strict with only a warning: still starts. + // Strict with only a warning (a junk DISABLE_MOSQUITTO): still starts. lines = nil fatal, _ = ReportBrokerSetup(brokerEnv(map[string]string{ - "DISABLE_MOSQUITTO": "yes", + "DISABLE_MOSQUITTO": "ture", "MQTT_SOURCE_STRICT": "true", }), []MQTTSource{{Name: "ext", Broker: "mqtt://broker.example:1883"}}, false, logf) if fatal { @@ -411,11 +416,12 @@ func TestIngestorMainReportsBrokerSetupBeforeConnecting(t *testing.T) { } } -// TestEmptyBrokerEnvironmentIsInert is the ingestor half of review round 2, -// finding 3. The compose files now forward MQTT_BROKER, MQTT_TOPIC and -// MQTT_SOURCE_STRICT with an empty default, so those keys are PRESENT and -// empty in the container of an install that never set them. -func TestEmptyBrokerEnvironmentIsInert(t *testing.T) { +// TestEmptyBrokerEnvironmentIsTheExternalDefault covers the keys every +// compose file forwards with an empty default (#372): present and empty is +// the same as unset. Since #441 that is the EXTERNAL-broker default, not +// the bundled one, so an install with no source configured is told so — +// but still starts unless MQTT_SOURCE_STRICT asks otherwise. +func TestEmptyBrokerEnvironmentIsTheExternalDefault(t *testing.T) { getenv := brokerEnv(map[string]string{ "DISABLE_MOSQUITTO": "", "MQTT_BROKER": "", @@ -423,18 +429,20 @@ func TestEmptyBrokerEnvironmentIsInert(t *testing.T) { "MQTT_SOURCE_STRICT": "", }) mode := ResolveBrokerMode(getenv) - if !mode.Bundled || mode.Ambiguous { - t.Errorf("an empty DISABLE_MOSQUITTO must be the quiet bundled default; got %+v", mode) + if mode.Bundled || mode.Unrecognised { + t.Errorf("an empty DISABLE_MOSQUITTO is the recognised external default; got %+v", mode) } if StrictBrokerSources(getenv) { t.Error("an empty MQTT_SOURCE_STRICT must not turn on strict mode") } - diags := CheckBrokerSources(mode, []MQTTSource{{Name: "local", Broker: "mqtt://localhost:1883"}}, true) - if len(diags) != 0 { - t.Errorf("an all-empty broker environment must produce no diagnostic; got:\n%s", diagText(diags)) - } - fatal, msg := ReportBrokerSetup(getenv, []MQTTSource{{Name: "local", Broker: "mqtt://localhost:1883"}}, true, func(string, ...any) {}) + var lines []string + logf := func(format string, args ...any) { lines = append(lines, fmt.Sprintf(format, args...)) } + fatal, msg := ReportBrokerSetup(getenv, []MQTTSource{{Name: "local", Broker: "mqtt://localhost:1883"}}, true, logf) if fatal { - t.Errorf("an all-empty broker environment must never refuse to start; got %q", msg) + t.Errorf("an empty broker environment must not refuse to start without MQTT_SOURCE_STRICT; got %q", msg) + } + joined := strings.Join(lines, "\n") + if !strings.Contains(joined, "no mqttSources are configured") || !strings.Contains(joined, "DISABLE_MOSQUITTO=false") { + t.Errorf("the missing source must be reported, with the opt-in back to the bundled broker; got:\n%s", joined) } } diff --git a/cmd/server/compose_bind_372_test.go b/cmd/server/compose_bind_372_test.go index dfdb36d05..4bced59a9 100644 --- a/cmd/server/compose_bind_372_test.go +++ b/cmd/server/compose_bind_372_test.go @@ -103,10 +103,13 @@ func TestStagingComposes_PprofHostPortsBindExplicitlyAndStayDistinct(t *testing. ports := portsBlockOf(t, compose, "staging-go") seen := map[string]bool{} for _, port := range []string{"6060", "6061"} { - re := bindMapping("PPROF_BIND", port, port) + // #441 moved the default from the conditional, dual-stack + // form to loopback; TestStagingComposes_PprofPublishDefaultsToLoopback + // pins the exact shape. + re := pprofLoopbackMapping(port) if !re.MatchString(ports) { - t.Fatalf("%s: pprof port %s must publish through an explicit, CONDITIONAL bind interface, i.e.\n"+ - " - \"${PPROF_BIND:+${PPROF_BIND}:}%s:%s\"\nports block:\n%s", compose, port, port, port, ports) + t.Fatalf("%s: pprof port %s must publish through the PPROF_BIND bind interface, i.e.\n"+ + " - \"${PPROF_BIND:-127.0.0.1}:%s:%s\"\nports block:\n%s", compose, port, port, port, ports) } if seen[port] { t.Fatalf("%s: port %s published twice", compose, port) @@ -134,12 +137,13 @@ func TestEnvExample_DocumentsEveryNewKnob(t *testing.T) { // otherwise an operator cannot find the setting that keeps or changes // their exposure. for _, knob := range []struct{ key, wantDefault string }{ - // The two publish-bind knobs default to EMPTY, not 0.0.0.0: an - // unqualified docker mapping is dual-stack and 0.0.0.0: is not, so - // an explicit 0.0.0.0 default would silently drop IPv6. + // PROD_MQTT_BIND defaults to EMPTY, not 0.0.0.0: an unqualified + // docker mapping is dual-stack and 0.0.0.0: is not, so an explicit + // 0.0.0.0 default would silently drop IPv6. The pprof knobs default + // to loopback since #441. {"PROD_MQTT_BIND", "(empty - every interface, IPv4 and IPv6)"}, - {"PPROF_BIND", "(empty - every interface, IPv4 and IPv6)"}, - {"PPROF_HOST", ""}, + {"PPROF_BIND", "127.0.0.1"}, + {"PPROF_HOST", "127.0.0.1"}, {"PPROF_PORT_SERVER", "6060"}, {"PPROF_PORT_INGESTOR", "6061"}, {"MQTT_SOURCE_STRICT", "false"}, @@ -273,6 +277,13 @@ func TestComposeBindDefaultsStayDualStack(t *testing.T) { t.Fatalf("read %s: %v", path, err) } for _, m := range anyHostBindDefault.FindAllStringSubmatch(stripYAMLComments(string(data)), -1) { + // The one owner-approved default (#441): the pprof ports are + // published on loopback unless PPROF_BIND widens them. It + // narrows rather than drops part of the old exposure, and is + // pinned by TestStagingComposes_PprofPublishDefaultsToLoopback. + if m[1] == "PPROF_BIND" && m[2] == "127.0.0.1" { + continue + } t.Errorf("%s publishes a port whose host interface defaults to %q via ${%s:-...}; "+ "a defaulted host interface is never equivalent to the old unqualified mapping "+ "(0.0.0.0: is IPv4-only, the unqualified form is dual-stack). Use ${%s:+${%s}:} instead.", diff --git a/cmd/server/compose_optin_441_test.go b/cmd/server/compose_optin_441_test.go new file mode 100644 index 000000000..2f6ce24d5 --- /dev/null +++ b/cmd/server/compose_optin_441_test.go @@ -0,0 +1,151 @@ +package main + +// Shape tests for issue #441, the owner decisions on #431: +// +// 1. The bundled mosquitto is opt-in: every compose file renders +// DISABLE_MOSQUITTO=true when the variable is unset. +// 3. pprof is loopback by default when enabled: the staging files publish +// 6060/6061 on 127.0.0.1 unless PPROF_BIND says otherwise, the +// in-container listener stays reachable by that publish, and the +// files that publish nothing leave the listener on its own loopback +// default. +// +// The helpers (readRepoCompose, stripYAMLComments, extractServiceBlock, +// extractSubBlock, portsBlockOf, readRepoFile, appComposeServices) live in +// staging_compose_broker_test.go and compose_bind_372_test.go. + +import ( + "regexp" + "strings" + "testing" +) + +func envBlockOf(t *testing.T, compose, service string) string { + t.Helper() + block := extractServiceBlock(stripYAMLComments(readRepoCompose(t, compose)), service) + if strings.TrimSpace(block) == "" { + t.Fatalf("%s: no services.%s block", compose, service) + } + env := extractSubBlock(block, "environment", 4) + if strings.TrimSpace(env) == "" { + t.Fatalf("%s: services.%s has no environment: sub-block", compose, service) + } + return env +} + +func TestComposeFiles_BundledBrokerIsOptIn(t *testing.T) { + assign := regexp.MustCompile(`(?m)^\s*-\s*DISABLE_MOSQUITTO=(\S*)\s*$`) + for _, target := range appComposeServices { + t.Run(target.file, func(t *testing.T) { + env := envBlockOf(t, target.file, target.service) + all := assign.FindAllStringSubmatch(env, -1) + if len(all) != 1 { + t.Fatalf("want exactly one DISABLE_MOSQUITTO assignment, got %d\nenvironment block:\n%s", len(all), env) + } + switch got := all[0][1]; got { + case "${DISABLE_MOSQUITTO:-true}": + case "true": + // The no-mosquitto files exist to run without the bundled + // broker; they may pin it. + if !strings.Contains(target.file, "no-mosquitto") { + t.Errorf("%s pins DISABLE_MOSQUITTO=true, leaving no way to opt in to the bundled broker; use ${DISABLE_MOSQUITTO:-true}", target.file) + } + default: + t.Errorf("%s passes DISABLE_MOSQUITTO=%s; the bundled broker is opt-in, so an unset value must render true (${DISABLE_MOSQUITTO:-true})", target.file, got) + } + }) + } +} + +// pprofLoopbackMapping matches a pprof port published on PPROF_BIND with a +// 127.0.0.1 default, e.g. "${PPROF_BIND:-127.0.0.1}:6060:6060". +func pprofLoopbackMapping(port string) *regexp.Regexp { + return regexp.MustCompile(`(?m)^\s*-\s*"\$\{PPROF_BIND:-127\.0\.0\.1\}:` + port + `:` + port + `"\s*$`) +} + +// pprofPublishers are the compose files that publish the pprof ports. +var pprofPublishers = []string{ + "docker-compose.staging.yml", + "docker-compose.staging.no-mosquitto.yml", +} + +func TestStagingComposes_PprofPublishDefaultsToLoopback(t *testing.T) { + for _, compose := range pprofPublishers { + t.Run(compose, func(t *testing.T) { + ports := portsBlockOf(t, compose, "staging-go") + for _, port := range []string{"6060", "6061"} { + if !pprofLoopbackMapping(port).MatchString(ports) { + t.Errorf("%s: pprof port %s must publish on PPROF_BIND, defaulting to loopback:\n"+ + " - \"${PPROF_BIND:-127.0.0.1}:%s:%s\"\nports block:\n%s", compose, port, port, port, ports) + } + } + }) + } +} + +// A loopback listener INSIDE the container is unreachable through a docker +// port mapping: docker's proxy connects to the container's network +// address. Where pprof is published, the in-container listener must +// therefore bind every interface, and PPROF_BIND alone decides who on the +// host can reach it. Without this, the #441 loopback default in pprofbind +// would silently kill the published port even with PPROF_BIND=0.0.0.0. +func TestStagingComposes_InContainerPprofListenerStaysReachable(t *testing.T) { + wildcard := regexp.MustCompile(`(?m)^\s*-\s*PPROF_HOST=0\.0\.0\.0\s*$`) + for _, compose := range pprofPublishers { + t.Run(compose, func(t *testing.T) { + env := envBlockOf(t, compose, "staging-go") + if !wildcard.MatchString(env) { + t.Errorf("%s publishes pprof, so it must pin the in-container listener to every interface (PPROF_HOST=0.0.0.0)\nenvironment block:\n%s", compose, env) + } + }) + } +} + +// The compose files that publish no pprof port must not widen the +// in-container listener: with nothing published, the loopback default is +// the whole point. +func TestNonPublishingComposes_LeavePprofOnLoopback(t *testing.T) { + publishes := map[string]bool{} + for _, f := range pprofPublishers { + publishes[f] = true + } + widened := regexp.MustCompile(`(?m)^\s*-\s*PPROF_HOST(?:_SERVER|_INGESTOR)?=(?:0\.0\.0\.0|\*|::|\[::\]|\$\{PPROF_HOST(?:_SERVER|_INGESTOR)?:-[^}]+\})\s*$`) + for _, target := range appComposeServices { + if publishes[target.file] { + continue + } + t.Run(target.file, func(t *testing.T) { + ports := extractSubBlock(extractServiceBlock(stripYAMLComments(readRepoCompose(t, target.file)), target.service), "ports", 4) + if regexp.MustCompile(`606[01]`).MatchString(ports) { + t.Fatalf("%s now publishes a pprof port; add it to pprofPublishers", target.file) + } + env := envBlockOf(t, target.file, target.service) + if m := widened.FindString(env); m != "" { + t.Errorf("%s publishes no pprof port but widens the in-container listener: %q", target.file, strings.TrimSpace(m)) + } + }) + } +} + +func TestDeploymentDocs_CarryThe441UpgradeNote(t *testing.T) { + full := readRepoFile(t, "docs/deployment.md") + heading := regexp.MustCompile(`(?m)^#{2,4} .*#441.*$`) + loc := heading.FindStringIndex(full) + if loc == nil { + t.Fatalf("docs/deployment.md must carry an upgrade note headed with #441") + } + doc := full[loc[0]:] + if next := regexp.MustCompile(`(?m)^#{2,3} `).FindStringIndex(doc[3:]); next != nil { + doc = doc[:next[0]+3] + } + for _, want := range []string{ + "DISABLE_MOSQUITTO=false", // how to keep the bundled broker + "PPROF_BIND=0.0.0.0", // how to keep remote pprof on a published port + "PPROF_HOST=0.0.0.0", // ... and on a process with no published port + "ssh -L", // the tunnel alternative + } { + if !strings.Contains(doc, want) { + t.Errorf("the #441 upgrade note must name %q; section was:\n%s", want, doc) + } + } +} diff --git a/docker-compose.example.yml b/docker-compose.example.yml index a2277767e..5e20ea959 100644 --- a/docker-compose.example.yml +++ b/docker-compose.example.yml @@ -3,9 +3,9 @@ # # AUDIENCE: third-party operators running a self-hosted CoreScope on # ONE host. Ships with the built-in Mosquitto broker DISABLED by -# default (DISABLE_MOSQUITTO defaults to `false` here, meaning -# mosquitto RUNS in-container — the historical shape); flip to `true` -# if you already have an external MQTT broker to point CoreScope at. +# default (#441: DISABLE_MOSQUITTO defaults to `true`), so point +# mqttSources in config.json (or MQTT_BROKER) at your MQTT broker, or +# set DISABLE_MOSQUITTO=false to run the bundled one in-container. # # For the CoreScope maintainers' internal staging deployment (with a # standalone `mqtt-broker` container on external network @@ -23,7 +23,7 @@ services: - ${DATA_DIR:-./data}:/app/data environment: - DISABLE_CADDY=${DISABLE_CADDY:-true} - - DISABLE_MOSQUITTO=${DISABLE_MOSQUITTO:-false} + - DISABLE_MOSQUITTO=${DISABLE_MOSQUITTO:-true} # In-process settings — read by the server and the ingestor, not by # docker compose. Without these lines a value in .env reaches docker # compose and stops there, so the documented "one line in .env" would diff --git a/docker-compose.staging.no-mosquitto.yml b/docker-compose.staging.no-mosquitto.yml index b932e4eb3..b7937ab24 100644 --- a/docker-compose.staging.no-mosquitto.yml +++ b/docker-compose.staging.no-mosquitto.yml @@ -34,18 +34,20 @@ services: - "host.docker.internal:host-gateway" ports: - "${STAGING_GO_HTTP_PORT:-82}:80" - # PPROF_BIND is the host interface the pprof ports are published on. - # Empty (the shipped default) renders no host interface at all, which - # is the dual-stack publish this file always had; 127.0.0.1 narrows it - # to the host. An explicit 0.0.0.0 would drop IPv6, so do not use it. - - "${PPROF_BIND:+${PPROF_BIND}:}6060:6060" - - "${PPROF_BIND:+${PPROF_BIND}:}6061:6061" + # PPROF_BIND is the host interface the pprof ports are published on, + # 127.0.0.1 unless set (#441); reach them with an SSH tunnel or set + # PPROF_BIND=0.0.0.0 for remote access. See docker-compose.staging.yml. + - "${PPROF_BIND:-127.0.0.1}:6060:6060" + - "${PPROF_BIND:-127.0.0.1}:6061:6061" volumes: - ${STAGING_DATA_DIR:-~/meshcore-staging-data}:/app/data - caddy-data-staging-go:/data/caddy environment: - NODE_ENV=staging - ENABLE_PPROF=true + # Every interface INSIDE the container, so the published pprof ports + # above reach it; PPROF_BIND decides who on the host can (#441). + - PPROF_HOST=0.0.0.0 - DISABLE_MOSQUITTO=true # In-process settings — read by the server and the ingestor, not by # docker compose. Without these lines a value in .env reaches docker @@ -57,7 +59,6 @@ services: - MQTT_BROKER=${MQTT_BROKER:-} - MQTT_TOPIC=${MQTT_TOPIC:-} - MQTT_SOURCE_STRICT=${MQTT_SOURCE_STRICT:-} - - PPROF_HOST=${PPROF_HOST:-} - PPROF_HOST_SERVER=${PPROF_HOST_SERVER:-} - PPROF_HOST_INGESTOR=${PPROF_HOST_INGESTOR:-} - PPROF_PORT=${PPROF_PORT:-} diff --git a/docker-compose.staging.yml b/docker-compose.staging.yml index f33ce697e..e026dadcd 100644 --- a/docker-compose.staging.yml +++ b/docker-compose.staging.yml @@ -50,29 +50,29 @@ services: # # (drop it in-between the :80 mapping and the pprof ports below). # pprof host ports. PPROF_BIND is the host interface they are - # published on. Leave it EMPTY (the shipped default) and each mapping - # renders with no host interface at all — byte for byte what this file - # always had, published on both IPv4 (0.0.0.0) and IPv6 ([::]), so a - # deploy that profiles this host remotely keeps working. An explicit - # PPROF_BIND=0.0.0.0 would NOT be the same: it publishes IPv4 only. - # Set PPROF_BIND=127.0.0.1 to require an SSH tunnel instead. The two - # ports stay distinct: 6060 is the server's listener, 6061 the + # published on, 127.0.0.1 unless set (#441): /debug/pprof/ is + # unauthenticated, so reach it with an SSH tunnel + # (ssh -L 6060:127.0.0.1:6060 -L 6061:127.0.0.1:6061 you@host), or + # set PPROF_BIND=0.0.0.0 to publish it on every IPv4 interface again. + # The two ports stay distinct: 6060 is the server's listener, 6061 the # ingestor's, and collapsing them would profile only whichever # process won the bind. # - # NOTE: narrowing the IN-CONTAINER listener with PPROF_HOST is a - # different setting and is incompatible with publishing these ports — - # docker's proxy reaches the container by its network address, not - # its loopback. Use PPROF_BIND for a published port, PPROF_HOST only - # when the process runs without a port mapping. - - "${PPROF_BIND:+${PPROF_BIND}:}6060:6060" # pprof server - - "${PPROF_BIND:+${PPROF_BIND}:}6061:6061" # pprof ingestor + # The in-container listener is pinned to every interface below + # (PPROF_HOST=0.0.0.0): docker's proxy reaches the container by its + # network address, not its loopback, so a loopback listener in here + # would make these ports dead. PPROF_BIND alone narrows them. + - "${PPROF_BIND:-127.0.0.1}:6060:6060" # pprof server + - "${PPROF_BIND:-127.0.0.1}:6061:6061" # pprof ingestor volumes: - ${STAGING_DATA_DIR:-~/meshcore-staging-data}:/app/data - caddy-data-staging-go:/data/caddy environment: - NODE_ENV=staging - ENABLE_PPROF=true + # Every interface INSIDE the container, so the published pprof ports + # above reach it; PPROF_BIND decides who on the host can (#441). + - PPROF_HOST=0.0.0.0 # Default TRUE: the standalone mqtt-broker container owns MQTT on # the staging VM (semantic flip vs. pre-v3.7). Override to false # only if you intend to run the legacy in-container mosquitto @@ -90,7 +90,6 @@ services: - MQTT_BROKER=${MQTT_BROKER:-} - MQTT_TOPIC=${MQTT_TOPIC:-} - MQTT_SOURCE_STRICT=${MQTT_SOURCE_STRICT:-} - - PPROF_HOST=${PPROF_HOST:-} - PPROF_HOST_SERVER=${PPROF_HOST_SERVER:-} - PPROF_HOST_INGESTOR=${PPROF_HOST_INGESTOR:-} - PPROF_PORT=${PPROF_PORT:-} diff --git a/docker-compose.yml b/docker-compose.yml index 449458071..2dd4bedab 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -28,8 +28,8 @@ services: # IPv4 (0.0.0.0) and IPv6 ([::]). Setting it to 0.0.0.0 would NOT be # the same thing: an explicit IPv4 host publishes IPv4 only. # Set PROD_MQTT_BIND=127.0.0.1 to keep the broker reachable only from - # the host, or set DISABLE_MOSQUITTO=true to use your own broker - # instead (then this mapping has nothing behind it). See + # the host. The bundled broker is opt-in (#441): unless + # DISABLE_MOSQUITTO=false, nothing listens behind this mapping. See # docs/deployment.md, "Migration — pulling the new Compose file". - "${PROD_MQTT_BIND:+${PROD_MQTT_BIND}:}${PROD_MQTT_PORT:-1883}:1883" volumes: @@ -38,7 +38,10 @@ services: - caddy-data:/data/caddy environment: - NODE_ENV=production - - DISABLE_MOSQUITTO=${DISABLE_MOSQUITTO:-false} + # The bundled mosquitto is opt-in (#441): set DISABLE_MOSQUITTO=false + # to run it, otherwise point mqttSources in config.json (or + # MQTT_BROKER) at your own broker. + - DISABLE_MOSQUITTO=${DISABLE_MOSQUITTO:-true} - DISABLE_CADDY=${DISABLE_CADDY:-false} # In-process settings — read by the server and the ingestor, not by # docker compose. Without these lines a value in .env reaches docker diff --git a/docker/entrypoint-go.sh b/docker/entrypoint-go.sh index af629a2aa..a75b89234 100644 --- a/docker/entrypoint-go.sh +++ b/docker/entrypoint-go.sh @@ -38,13 +38,35 @@ if [ -f /app/data/.env ]; then set +a fi +# The bundled mosquitto is opt-in (#441): it runs only when DISABLE_MOSQUITTO +# is a recognised false value. Trimmed and case-insensitive, true/1/yes/on +# disable it and false/0/no/off enable it; unset, empty and anything else +# take the default, true. manage.sh (parse_disable_mosquitto) and the +# ingestor (ResolveBrokerMode) read it with the same table; see +# test-fixtures/disable-mosquitto-cases.json. +DISABLE_MOSQUITTO_NORM=$(printf '%s' "${DISABLE_MOSQUITTO-}" | + sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//' | tr '[:upper:]' '[:lower:]') +case "$DISABLE_MOSQUITTO_NORM" in + true | 1 | yes | on | '') MOSQUITTO_DISABLED=true ;; + false | 0 | no | off) MOSQUITTO_DISABLED=false ;; + *) + echo "[config] DISABLE_MOSQUITTO='${DISABLE_MOSQUITTO}' is not a recognised value (true/1/yes/on or false/0/no/off); using the default, true" + MOSQUITTO_DISABLED=true + ;; +esac +if [ -n "${DISABLE_MOSQUITTO-}" ]; then + MOSQUITTO_REASON="DISABLE_MOSQUITTO=${DISABLE_MOSQUITTO}" +else + MOSQUITTO_REASON="DISABLE_MOSQUITTO unset; the bundled broker is opt-in, set DISABLE_MOSQUITTO=false to run it" +fi + SUPERVISORD_CONF="/etc/supervisor/conf.d/supervisord.conf" -if [ "${DISABLE_MOSQUITTO:-false}" = "true" ] && [ "${DISABLE_CADDY:-false}" = "true" ]; then - echo "[config] internal MQTT broker disabled (DISABLE_MOSQUITTO=true)" +if [ "$MOSQUITTO_DISABLED" = "true" ] && [ "${DISABLE_CADDY:-false}" = "true" ]; then + echo "[config] internal MQTT broker disabled (${MOSQUITTO_REASON})" echo "[config] Caddy reverse proxy disabled (DISABLE_CADDY=true)" SUPERVISORD_CONF="/etc/supervisor/conf.d/supervisord-no-mosquitto-no-caddy.conf" -elif [ "${DISABLE_MOSQUITTO:-false}" = "true" ]; then - echo "[config] internal MQTT broker disabled (DISABLE_MOSQUITTO=true)" +elif [ "$MOSQUITTO_DISABLED" = "true" ]; then + echo "[config] internal MQTT broker disabled (${MOSQUITTO_REASON})" SUPERVISORD_CONF="/etc/supervisor/conf.d/supervisord-no-mosquitto.conf" elif [ "${DISABLE_CADDY:-false}" = "true" ]; then echo "[config] Caddy reverse proxy disabled (DISABLE_CADDY=true)" diff --git a/docs/deployment.md b/docs/deployment.md index 8986a514c..d983fa3ee 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -42,12 +42,14 @@ docker run -d --name corescope \ ghcr.io/kpa-clawbot/corescope:latest ``` -Open `http://localhost` — you'll see an empty dashboard ready to receive packets. +Open `http://localhost` — you'll see an empty dashboard. No `config.json` is required. The server starts with sensible defaults: - HTTP on port 3000 (Caddy proxies port 80 → 3000 internally) -- Internal Mosquitto MQTT broker on port 1883 -- Ingestor connects to `mqtt://localhost:1883` automatically +- **No MQTT broker.** The bundled Mosquitto is opt-in: add + `-e DISABLE_MOSQUITTO=false -p 1883:1883` to run it, or point `mqttSources` + in `config.json` (or `-e MQTT_BROKER=…`) at your own broker +- The ingestor connects to `mqtt://localhost:1883` unless configured otherwise - SQLite database at `/app/data/meshcore.db` ### Full `docker run` Reference (recommended) @@ -76,7 +78,7 @@ docker run -d --name corescope \ | `-v /your/data:/app/data` | Yes | Persistent data: SQLite DB, config.json, theme.json | | `-v /your/Caddyfile:/etc/caddy/Caddyfile:ro` | No | Custom Caddyfile for HTTPS | | `-v /your/caddy-data:/data/caddy` | No | Caddy TLS certificate storage | -| `-e DISABLE_MOSQUITTO=true` | No | Skip the internal Mosquitto broker (use your own) | +| `-e DISABLE_MOSQUITTO=false` | No | Run the bundled Mosquitto broker. It is **opt-in**: unset, the broker does not start and the ingestor needs your own broker in `mqttSources` | | `-e DISABLE_CADDY=true` | No | Skip the built-in Caddy reverse proxy | | `-e MQTT_BROKER=mqtt://host:1883` | No | Override MQTT broker URL | | `-e CORESCOPE_RUN_AS_NONROOT=1` | No | Opt-in: run `corescope-server` and `corescope-ingestor` without root — see [Running the Go services without root](#running-the-go-services-without-root-opt-in) | @@ -293,10 +295,10 @@ docker compose up -d |----------|---------|-------------| | `HTTP_PORT` | `80` | Host port for the web UI | | `DATA_DIR` | `./data` | Host path for persistent data | -| `DISABLE_MOSQUITTO` | `false` | Set `true` to use an external MQTT broker | +| `DISABLE_MOSQUITTO` | `true` | Set `false` to run the bundled MQTT broker (opt-in) | | `DISABLE_CADDY` | `false` | Set `true` to skip the built-in Caddy proxy | | `PROD_MQTT_BIND` | *(empty — IPv4 and IPv6)* | Host interface the bundled broker's `1883` is published on; `127.0.0.1` for host-only | -| `PPROF_BIND` | *(empty — IPv4 and IPv6)* | Host interface the pprof ports are published on (staging compose files) | +| `PPROF_BIND` | `127.0.0.1` | Host interface the pprof ports are published on (staging compose files); `0.0.0.0` for remote access | ### manage.sh (legacy alternative) @@ -333,41 +335,98 @@ Data is preserved in the volume — updates are non-destructive. ## Migration — pulling the new Compose file -This release makes the bundled MQTT broker's exposure and the pprof -listeners' exposure **explicit settings** instead of hard-coded values. +### Upgrading past #441 — bundled broker opt-in, pprof on loopback -**Nothing changes for an existing install.** Every new setting ships with the -default that reproduces the previous behaviour exactly, so a `git pull` + -`docker compose up -d` leaves your deployment on the same interfaces and -ports it was on before. The table below is what you can now change, not what -changed under you. +Two defaults change. Both are deliberate and both are one line to undo. + +**1. The bundled Mosquitto no longer starts unless you ask for it.** +`DISABLE_MOSQUITTO` now defaults to `true` everywhere: `docker-compose.yml`, +`docker-compose.example.yml`, the image's entrypoint (so a bare `docker run` +too), `manage.sh` and its setup wizard (which now defaults to "no"), and +`.env.example`. An install that relies on the bundled broker and never set +the variable — or set it to something that is not a recognised value — will +start **without a broker** and ingest nothing, with a `[broker] error:` line +in the ingestor log. To keep the bundled broker, set: + +```bash +DISABLE_MOSQUITTO=false # in .env, or -e DISABLE_MOSQUITTO=false +``` + +An install that already sets `DISABLE_MOSQUITTO=true`, or `false`, behaves +exactly as before. So does one whose `.env` was copied from an older +`.env.example`, which carried an explicit `DISABLE_MOSQUITTO=false`. + +**2. `DISABLE_MOSQUITTO` is read the same way by every launcher.** The +entrypoint, `manage.sh` and the ingestor accept, trimmed and in any case, +`true`/`1`/`yes`/`on` (broker off) and `false`/`0`/`no`/`off` (broker on). +Unset or empty means the default, `true`. Anything else (`y`, `n`, a typo) +also means `true`, and is reported by the entrypoint, the ingestor and +`./manage.sh broker-check`. Before, a bare `docker compose up` disabled the +broker only on the exact `true` while `manage.sh` also took `yes`/`1`/`on`; +`broker-check` no longer has a mismatch to report and prints the effective +mode instead, e.g. `Broker mode: external (DISABLE_MOSQUITTO=yes, read as +true)`. + +**3. pprof listens on loopback when enabled.** With `ENABLE_PPROF=true` the +server (`6060`) and ingestor (`6061`) listeners bind `127.0.0.1`, and the +staging compose files publish those ports on `127.0.0.1` only. Ports are +unchanged and stay distinct. `/debug/pprof/` is unauthenticated, so the +intended way in is an SSH tunnel: + +```bash +ssh -L 6060:127.0.0.1:6060 -L 6061:127.0.0.1:6061 you@your-host +# then open http://localhost:6060/debug/pprof/ and http://localhost:6061/debug/pprof/ +``` + +To keep remote access instead: + +| Where pprof runs | Setting that restores remote access | +|------------------|-------------------------------------| +| `docker-compose.staging*.yml` (publishes `6060`/`6061`) | `PPROF_BIND=0.0.0.0` in `.env` — IPv4 on every interface. The in-container listener is pinned to every interface by those files, so `PPROF_BIND` alone decides | +| A bare binary, or a `docker run`/compose file that maps the pprof ports itself | `PPROF_HOST=0.0.0.0` (or `*`) — a loopback listener inside a container is unreachable through a port mapping | + +`MQTT_SOURCE_STRICT` is unchanged and stays off by default. + +**A non-container ingestor.** An ingestor started outside the image with +`DISABLE_MOSQUITTO` unset now assumes an external broker, so a loopback-only +`mqttSources` (a broker on the same host) logs a `[broker] error:` line. It +still starts and still connects; set `DISABLE_MOSQUITTO=false` to silence it. + +### The #372 settings + +#372 made the bundled MQTT broker's exposure and the pprof listeners' +exposure **explicit settings** instead of hard-coded values. + +**#372 itself changed nothing for an existing install.** Every setting it +added shipped with the default that reproduced the previous behaviour; the +two defaults #441 moved are marked in the table below. The table is what you +can change, not what changed under you. | Setting | Shipped default | What the default does | Change it to | |---------|-----------------|-----------------------|--------------| | `PROD_MQTT_BIND` | empty | Publishes the bundled broker's `1883` with no host interface, i.e. on both IPv4 (`0.0.0.0`) and IPv6 (`[::]`) — byte for byte the mapping this file always had | `127.0.0.1` to reach the plaintext broker from the host only | -| `PPROF_BIND` | empty | Publishes the staging pprof ports `6060`/`6061` with no host interface, i.e. IPv4 and IPv6 — unchanged from before | `127.0.0.1` to require an SSH tunnel for profiling | -| `PPROF_HOST` | empty | The in-process pprof listener binds every interface — unchanged from before | `127.0.0.1` for a process with **no** published port (see the warning below) | +| `PPROF_BIND` | `127.0.0.1` (since #441) | Publishes the staging pprof ports `6060`/`6061` on the host's loopback only | `0.0.0.0` for remote access over IPv4 | +| `PPROF_HOST` | `127.0.0.1` (since #441; pinned to `0.0.0.0` in the staging compose files) | The in-process pprof listener binds loopback | `0.0.0.0` (or `*`) for a process whose pprof port is reached from outside it (see the warning below) | | `PPROF_PORT_SERVER` / `PPROF_PORT_INGESTOR` | `6060` / `6061` | Same ports as before; they must stay distinct | Any free pair, when `6060`/`6061` are taken | | `MQTT_SOURCE_STRICT` | `false` | A broker setup that cannot ingest is logged, and the ingestor still starts | `true` to refuse to start instead | -| `DISABLE_MOSQUITTO` | `false` (`true` in `docker-compose.staging.yml`) | Unchanged — the bundled broker still runs by default | `true` if you run your own broker (use the exact `true`, so manage.sh and a bare `docker compose` agree) | +| `DISABLE_MOSQUITTO` | `true` (since #441) | The bundled broker does not run | `false` to run the bundled broker | | `MQTT_BROKER` / `MQTT_TOPIC` | empty | Unchanged — `config.json` decides the sources | the external broker's URL, as an alternative to editing `config.json` | -**Leave a bind variable empty, do not write `0.0.0.0` into it.** Docker +**Leave `PROD_MQTT_BIND` empty, do not write `0.0.0.0` into it.** Docker publishes a mapping with no host interface (`"1883:1883"`) on *both* `0.0.0.0` and `[::]`, but a mapping with an explicit IPv4 host (`"0.0.0.0:1883:1883"`) -publishes IPv4 **only**. The shipped defaults are therefore empty, so the -rendered mapping is identical to the one these files had before — run -`docker compose config` on an unset `.env` and no `host_ip` appears at all. -Setting `PROD_MQTT_BIND=0.0.0.0` or `PPROF_BIND=0.0.0.0` would quietly drop -IPv6 reachability of the broker or the profiler. +publishes IPv4 **only**. Its shipped default is therefore empty, so the +rendered mapping is identical to the one this file always had. The same is +why `PPROF_BIND=0.0.0.0` restores remote profiling over IPv4 only. **`PPROF_BIND` and `PPROF_HOST` are not interchangeable.** `PPROF_BIND` is the *host* interface a published container port listens on; `PPROF_HOST` is the interface the listener *inside* the process binds. A loopback listener inside a container is not reachable through a docker port mapping at all, because -docker's proxy connects to the container's network address. Use `PPROF_BIND` -for a containerised deploy and `PPROF_HOST` only for a process that runs -without a port mapping. +docker's proxy connects to the container's network address — which is why +the staging compose files pin `PPROF_HOST=0.0.0.0` and narrow with +`PPROF_BIND`. Use `PPROF_BIND` for a compose file that publishes pprof and +`PPROF_HOST` for anything else. **What you may now see in the log.** Two situations that used to pass silently are reported: @@ -375,26 +434,12 @@ are reported: - `ENABLE_PPROF` set to something other than the exact string `true` (for example `1`, `yes` or `TRUE`) leaves pprof **off**, as it always did, and the process now logs that it did so. -- `DISABLE_MOSQUITTO=true` with no `mqttSources` configured, or with only - loopback ones, logs a `[broker] error:` line — the ingestor is pointed at a - broker that is not running and will ingest nothing. It still starts, so this - changes no behaviour; set `MQTT_SOURCE_STRICT=true` if you would rather the - startup fail. A value of `DISABLE_MOSQUITTO` that is neither `true` nor - `false` logs a `[broker] warn:` line, because what such a value means - depends on how the stack was launched — see the next paragraph. - -**`DISABLE_MOSQUITTO` depends on the launcher, and `broker-check` reports the -one that applies.** `./manage.sh` resolves the value itself with `is_true()` -(`1`, `true`, `TRUE`, `yes`, `y`, `on`, …) and then runs -`docker-compose.no-mosquitto.yml`, which hard-codes `DISABLE_MOSQUITTO=true` -in the container — so on a manage.sh-managed host every one of those values -gives an **external** broker, and `./manage.sh broker-check` validates the -external sources accordingly. A bare `docker compose up` against -`docker-compose.yml` passes the value straight to the entrypoint, which -disables the bundled broker only on the exact string `true`, so `yes` there -leaves the bundled broker **running**. Both `broker-check` and the ingestor's -`[broker] warn:` line name that gap when a value falls into it. Set `true` or -`false` and every launcher agrees. +- With the bundled broker off and no `mqttSources` configured, or with only + loopback ones, the ingestor logs a `[broker] error:` line — it is pointed at + a broker that is not running and will ingest nothing. It still starts; set + `MQTT_SOURCE_STRICT=true` if you would rather the startup fail. A + `DISABLE_MOSQUITTO` value that is not recognised logs a `[broker] warn:` + line (see "Upgrading past #441" above). **Settings in `.env` that a process reads now reach the container.** `MQTT_BROKER`, `MQTT_TOPIC`, `MQTT_SOURCE_STRICT`, `ENABLE_PPROF` and the @@ -446,11 +491,11 @@ CoreScope uses a layered configuration system (highest priority wins): | `MQTT_BROKER` | `mqtt://localhost:1883` | MQTT broker URL (overrides config file) | | `MQTT_TOPIC` | `meshcore/#` | MQTT topic subscription pattern | | `DB_PATH` | `data/meshcore.db` | SQLite database path | -| `DISABLE_MOSQUITTO` | `false` | Skip the internal Mosquitto broker. Only the exact string `true` disables it | +| `DISABLE_MOSQUITTO` | `true` | Skip the bundled Mosquitto broker; `false` runs it (opt-in). `true`/`1`/`yes`/`on` and `false`/`0`/`no`/`off`, trimmed, any case; anything else means `true` | | `DISABLE_CADDY` | `false` | Skip the built-in Caddy reverse proxy | | `MQTT_SOURCE_STRICT` | `false` | Refuse to start when the broker setup cannot ingest, instead of only logging it | | `ENABLE_PPROF` | `false` | Expose `/debug/pprof/`. Only the exact string `true` enables it | -| `PPROF_HOST` | *(every interface)* | Host the in-process pprof listener binds; `127.0.0.1` for loopback only | +| `PPROF_HOST` | `127.0.0.1` | Host the in-process pprof listener binds; `0.0.0.0` or `*` for every interface | | `PPROF_PORT_SERVER` | `6060` | Server pprof port (overrides the shared `PPROF_PORT`) | | `PPROF_PORT_INGESTOR` | `6061` | Ingestor pprof port (overrides the shared `PPROF_PORT`) | @@ -589,16 +634,17 @@ Map tile providers are enabled and configured via the `config.json` file. You ca ## MQTT Setup -CoreScope receives MeshCore packets via MQTT. The container ships with an internal Mosquitto broker — no setup needed for basic use. +CoreScope receives MeshCore packets via MQTT. The container ships with an internal Mosquitto broker, which is **opt-in**: it runs only with `DISABLE_MOSQUITTO=false`. Otherwise configure your own broker (below). -### Internal broker (default) +### Internal broker (opt-in) -The built-in Mosquitto broker listens on port 1883 inside the container. Point your MeshCore gateways at it: +With `DISABLE_MOSQUITTO=false` the built-in Mosquitto broker listens on port 1883 inside the container. Point your MeshCore gateways at it: ```bash -# Expose MQTT port for external gateways +# Run the bundled broker and expose its port for external gateways docker run -d --name corescope \ -p 80:80 -p 1883:1883 \ + -e DISABLE_MOSQUITTO=false \ -v corescope-data:/app/data \ ghcr.io/kpa-clawbot/corescope:latest ``` @@ -607,10 +653,8 @@ docker run -d --name corescope \ To use your own MQTT broker (Mosquitto, EMQX, HiveMQ, etc.): -1. Disable the internal broker: - ```bash - -e DISABLE_MOSQUITTO=true - ``` +1. Leave the internal broker off — the default (`DISABLE_MOSQUITTO` unset or + `true`). 2. Point the ingestor at your broker: ```bash @@ -637,8 +681,7 @@ To use your own MQTT broker (Mosquitto, EMQX, HiveMQ, etc.): ./manage.sh broker-check ``` - `DISABLE_MOSQUITTO=true` turns off the bundled broker but does **not** - configure a replacement. With no `mqttSources`, the ingestor falls back to + With the bundled broker off, nothing configures a replacement for you. With no `mqttSources`, the ingestor falls back to its built-in `mqtt://localhost:1883` — the bundled broker's own address, where nothing is now listening — and ingests nothing while otherwise looking healthy. `broker-check` and the ingestor's own startup log both @@ -1110,16 +1153,16 @@ by the helper. ### Container starts but dashboard is empty This is normal on first start with no MQTT sources configured. The dashboard shows data once packets arrive via MQTT. Either: -- Point a MeshCore gateway at the container's MQTT broker (port 1883) +- Run the bundled broker (`DISABLE_MOSQUITTO=false`, opt-in) and point a MeshCore gateway at it (port 1883) - Configure an external MQTT source in `config.json` ### "no MQTT connections established" in logs The ingestor couldn't connect to any MQTT broker. Check: -1. Is the internal Mosquitto running? (`DISABLE_MOSQUITTO` should be `false`, and only the exact string `true` disables it) +1. If you rely on the internal Mosquitto, is it running? It is opt-in: `DISABLE_MOSQUITTO` must be `false` (or `0`/`no`/`off`) 2. Is the external broker reachable? Test with `mosquitto_sub -h broker -t meshcore/#` 3. Are credentials correct in `config.json`? -4. Run `./manage.sh broker-check`, and look for `[broker]` lines in the ingestor log — with `DISABLE_MOSQUITTO=true` and no external `mqttSources`, the ingestor is pointed at a broker that is not running. +4. Run `./manage.sh broker-check`, and look for `[broker]` lines in the ingestor log — with the bundled broker off and no external `mqttSources`, the ingestor is pointed at a broker that is not running. ### WebSocket disconnects / real-time updates stop diff --git a/internal/pprofbind/pprofbind.go b/internal/pprofbind/pprofbind.go index 0113831b4..52758a7fd 100644 --- a/internal/pprofbind/pprofbind.go +++ b/internal/pprofbind/pprofbind.go @@ -7,11 +7,14 @@ // reports why, so an operator who typed ENABLE_PPROF=1 is told rather than // left wondering. // -// The bind host is a separate, explicit setting. Its default is the empty -// host — ":6060" — which is what the server and the ingestor listened on -// before this package existed, i.e. every interface. Installs that profile -// a remote host therefore keep working untouched; an operator who wants -// loopback only sets PPROF_HOST (or the per-role PPROF_HOST_). +// The bind host is a separate, explicit setting. Its default is loopback, +// 127.0.0.1 (#441): /debug/pprof/ is unauthenticated and can stall a +// process, so it is reached through an SSH tunnel unless an operator widens +// it with PPROF_HOST (or the per-role PPROF_HOST_) — "0.0.0.0" or "*" +// for every interface, the pre-#441 listener. A container whose compose +// file publishes the pprof ports pins PPROF_HOST to every interface itself +// and narrows the published port with PPROF_BIND, because docker's port +// proxy cannot reach a loopback listener inside the container. // // Ports stay distinct per role: the server defaults to 6060 and the // ingestor to 6061. The shared PPROF_PORT predates this package and still @@ -22,7 +25,7 @@ // // PPROF_HOST_ PPROF_PORT_ // PPROF_HOST PPROF_PORT -// "" (every interface) Role.DefaultPort +// DefaultHost Role.DefaultPort package pprofbind import ( @@ -42,6 +45,10 @@ type Role struct { DefaultPort string } +// DefaultHost is where a listener binds when no PPROF_HOST setting names a +// host. +const DefaultHost = "127.0.0.1" + // Server and Ingestor are the two roles. Their default ports differ so both // listeners can run side by side in one container, which is how // docker-compose.staging.yml publishes them. @@ -87,10 +94,11 @@ func (r Role) Resolve(getenv func(string) string) Config { if host != "" { addr = net.JoinHostPort(host, port) } - // A listener on every interface has no single hostname to print, so the - // URL names loopback — the one address that always reaches it. + // A listener on every interface ("" or an unspecified address such as + // 0.0.0.0) has no single hostname to print, so the URL names loopback — + // the one address that always reaches it. display := host - if display == "" { + if ip := net.ParseIP(host); display == "" || (ip != nil && ip.IsUnspecified()) { display = "localhost" } return Config{ @@ -121,19 +129,19 @@ func (r Role) resolvePort(getenv func(string) string) (string, error) { return strconv.Itoa(n), nil } -// resolveHost applies the host precedence. "" and "*" both mean every -// interface, which keeps the pre-#372 default reachable by name. Anything -// that is not a bare host — a CIDR mask, a host:port pair, embedded -// whitespace — is rejected rather than guessed at. +// resolveHost applies the host precedence. "*" means every interface, as +// does an unspecified address such as "0.0.0.0". Anything that is not a +// bare host — a CIDR mask, a host:port pair, embedded whitespace — is +// rejected rather than guessed at. func (r Role) resolveHost(getenv func(string) string) (string, error) { - raw, from := "", "default" + raw, from := DefaultHost, "default" if v := strings.TrimSpace(getenv("PPROF_HOST")); v != "" { raw, from = v, "PPROF_HOST" } if v := strings.TrimSpace(getenv("PPROF_HOST_" + r.EnvSuffix)); v != "" { raw, from = v, "PPROF_HOST_"+r.EnvSuffix } - if raw == "" || raw == "*" { + if raw == "*" { return "", nil } if strings.ContainsAny(raw, " \t/?#@") { diff --git a/internal/pprofbind/pprofbind_test.go b/internal/pprofbind/pprofbind_test.go index 5a3cc500d..fbe83fbe9 100644 --- a/internal/pprofbind/pprofbind_test.go +++ b/internal/pprofbind/pprofbind_test.go @@ -8,30 +8,67 @@ func env(kv map[string]string) func(string) string { return func(k string) string { return kv[k] } } -func TestDefaultsPreserveEveryInterfaceListener(t *testing.T) { - // Pre-#372 behaviour: ENABLE_PPROF=true and nothing else listens on - // ":"+port, i.e. every interface. Existing installs that profile - // remotely depend on it, so the default must not move to loopback. +// Issue #441 (owner decision on #431): with ENABLE_PPROF=true and nothing +// else, each listener binds loopback on its own port. /debug/pprof/ is +// unauthenticated and can stall a process, so reaching it from elsewhere +// is something an operator asks for (PPROF_HOST here, PPROF_BIND for a +// published container port) or gets through an SSH tunnel. +func TestDefaultsBindLoopbackOnly(t *testing.T) { for _, tc := range []struct { role Role wantAddr string }{ - {Server, ":6060"}, - {Ingestor, ":6061"}, + {Server, "127.0.0.1:6060"}, + {Ingestor, "127.0.0.1:6061"}, } { got := tc.role.Resolve(env(map[string]string{"ENABLE_PPROF": "true"})) if !got.Enabled { t.Fatalf("%s: want enabled, got %+v", tc.role.Name, got) } if got.Addr != tc.wantAddr { - t.Errorf("%s: Addr = %q, want %q (every interface, as before #372)", tc.role.Name, got.Addr, tc.wantAddr) + t.Errorf("%s: Addr = %q, want %q (loopback by default since #441)", tc.role.Name, got.Addr, tc.wantAddr) } - if got.Host != "" { - t.Errorf("%s: Host = %q, want %q (empty means every interface)", tc.role.Name, got.Host, "") + if got.Host != "127.0.0.1" { + t.Errorf("%s: Host = %q, want %q", tc.role.Name, got.Host, "127.0.0.1") + } + if got.URL != "http://127.0.0.1:"+got.Port+"/debug/pprof/" { + t.Errorf("%s: URL = %q", tc.role.Name, got.URL) } } } +// The way back to the pre-#441 listener: an explicit wildcard host, shared +// or per role. +func TestWildcardHostRestoresEveryInterface(t *testing.T) { + for _, tc := range []struct { + name string + kv map[string]string + role Role + want string + }{ + {"0.0.0.0", map[string]string{"PPROF_HOST": "0.0.0.0"}, Server, "0.0.0.0:6060"}, + {"star", map[string]string{"PPROF_HOST": "*"}, Ingestor, ":6061"}, + {"ipv6 wildcard", map[string]string{"PPROF_HOST": "::"}, Server, "[::]:6060"}, + {"per role", map[string]string{"PPROF_HOST_INGESTOR": "0.0.0.0"}, Ingestor, "0.0.0.0:6061"}, + } { + t.Run(tc.name, func(t *testing.T) { + kv := map[string]string{"ENABLE_PPROF": "true"} + for k, v := range tc.kv { + kv[k] = v + } + got := tc.role.Resolve(env(kv)) + if !got.Enabled || got.Addr != tc.want { + t.Errorf("want enabled on %q, got %+v", tc.want, got) + } + }) + } + // A per-role override leaves the other role on the loopback default. + other := Server.Resolve(env(map[string]string{"ENABLE_PPROF": "true", "PPROF_HOST_INGESTOR": "0.0.0.0"})) + if other.Addr != "127.0.0.1:6060" { + t.Errorf("PPROF_HOST_INGESTOR must not move the server listener; got %q", other.Addr) + } +} + func TestServerAndIngestorDefaultPortsStayDistinct(t *testing.T) { if Server.DefaultPort == Ingestor.DefaultPort { t.Fatalf("server and ingestor must keep distinct default pprof ports, both are %q", Server.DefaultPort) @@ -87,11 +124,11 @@ func TestSharedPortStillAppliesToBothRoles(t *testing.T) { // PPROF_PORT predates #372 and applied to whichever process read it. // Keep that so an existing install that sets it is unaffected. kv := map[string]string{"ENABLE_PPROF": "true", "PPROF_PORT": "7000"} - if got := Server.Resolve(env(kv)); got.Addr != ":7000" { - t.Errorf("server Addr = %q, want :7000", got.Addr) + if got := Server.Resolve(env(kv)); got.Addr != "127.0.0.1:7000" { + t.Errorf("server Addr = %q, want 127.0.0.1:7000", got.Addr) } - if got := Ingestor.Resolve(env(kv)); got.Addr != ":7000" { - t.Errorf("ingestor Addr = %q, want :7000", got.Addr) + if got := Ingestor.Resolve(env(kv)); got.Addr != "127.0.0.1:7000" { + t.Errorf("ingestor Addr = %q, want 127.0.0.1:7000", got.Addr) } } @@ -101,11 +138,11 @@ func TestPerRolePortOverridesSharedPort(t *testing.T) { "PPROF_PORT": "7000", "PPROF_PORT_SERVER": "7100", } - if got := Server.Resolve(env(kv)); got.Addr != ":7100" { - t.Errorf("server Addr = %q, want :7100 (per-role port wins)", got.Addr) + if got := Server.Resolve(env(kv)); got.Addr != "127.0.0.1:7100" { + t.Errorf("server Addr = %q, want 127.0.0.1:7100 (per-role port wins)", got.Addr) } - if got := Ingestor.Resolve(env(kv)); got.Addr != ":7000" { - t.Errorf("ingestor Addr = %q, want :7000 (shared port still applies)", got.Addr) + if got := Ingestor.Resolve(env(kv)); got.Addr != "127.0.0.1:7000" { + t.Errorf("ingestor Addr = %q, want 127.0.0.1:7000 (shared port still applies)", got.Addr) } } @@ -151,7 +188,7 @@ func TestUnusablePortDisablesWithAReason(t *testing.T) { got := Server.Resolve(env(kv)) if port == "" { // Empty reads as unset: fall through to the default. - if !got.Enabled || got.Addr != ":6060" { + if !got.Enabled || got.Addr != "127.0.0.1:6060" { t.Errorf("PPROF_PORT_SERVER=%q: want the default :6060, got %+v", port, got) } continue @@ -184,10 +221,18 @@ func TestUnusableHostDisablesWithAReason(t *testing.T) { } func TestURLNamesLocalhostWhenBoundEverywhere(t *testing.T) { - got := Server.Resolve(env(map[string]string{"ENABLE_PPROF": "true"})) + got := Server.Resolve(env(map[string]string{"ENABLE_PPROF": "true", "PPROF_HOST": "*"})) if got.URL != "http://localhost:6060/debug/pprof/" { t.Errorf("URL = %q, want http://localhost:6060/debug/pprof/", got.URL) } + // An unspecified address is every interface too (the staging compose + // files pin PPROF_HOST=0.0.0.0), so it names loopback the same way. + for _, host := range []string{"0.0.0.0", "::"} { + got = Server.Resolve(env(map[string]string{"ENABLE_PPROF": "true", "PPROF_HOST": host})) + if got.URL != "http://localhost:6060/debug/pprof/" { + t.Errorf("PPROF_HOST=%s: URL = %q, want http://localhost:6060/debug/pprof/", host, got.URL) + } + } got = Ingestor.Resolve(env(map[string]string{"ENABLE_PPROF": "true", "PPROF_HOST": "::1"})) if got.URL != "http://[::1]:6061/debug/pprof/" { t.Errorf("URL = %q, want http://[::1]:6061/debug/pprof/", got.URL) @@ -219,14 +264,14 @@ func TestEmptyEnvironmentIsInert(t *testing.T) { t.Errorf("%s: an all-empty environment must say nothing; got reason %q", role.Name, got.Reason) } } - // With pprof asked for, the empty host and port settings must still - // resolve to the pre-#372 listener: every interface, the role's own port. + // With pprof asked for, the empty host and port settings must resolve + // to the default listener: loopback (#441), the role's own port. for _, tc := range []struct { role Role wantAddr string }{ - {Server, ":6060"}, - {Ingestor, ":6061"}, + {Server, "127.0.0.1:6060"}, + {Ingestor, "127.0.0.1:6061"}, } { on := map[string]string{"ENABLE_PPROF": "true"} for k, v := range empty { @@ -237,7 +282,7 @@ func TestEmptyEnvironmentIsInert(t *testing.T) { } got := tc.role.Resolve(env(on)) if !got.Enabled || got.Addr != tc.wantAddr { - t.Errorf("%s: empty host/port settings must resolve to %q on every interface; got %+v", tc.role.Name, tc.wantAddr, got) + t.Errorf("%s: empty host/port settings must resolve to %q; got %+v", tc.role.Name, tc.wantAddr, got) } } } diff --git a/manage.sh b/manage.sh index 018a1c320..6aaa2a859 100755 --- a/manage.sh +++ b/manage.sh @@ -68,15 +68,30 @@ err() { printf '%b\n' "${RED}✗${NC} $1"; } info() { printf '%b\n' "${CYAN}→${NC} $1"; } step() { printf '%b\n' "\n${BOLD}[$1/$TOTAL_STEPS] $2${NC}"; } -is_true() { - case "${1:-}" in - 1|true|TRUE|yes|YES|y|Y|on|ON) return 0 ;; - *) return 1 ;; +# DISABLE_MOSQUITTO (#441): the bundled mosquitto is opt-in. Prints "true" +# (bundled broker off), "false" (on) or "invalid". Trimmed and +# case-insensitive: true/1/yes/on and false/0/no/off; unset or empty is the +# default, true. docker/entrypoint-go.sh and the ingestor's +# ResolveBrokerMode read it with the same table +# (test-fixtures/disable-mosquitto-cases.json), and an invalid value falls +# back to the default in all three. +parse_disable_mosquitto() { + local v + v=$(printf '%s' "${1:-}" | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//' | tr '[:upper:]' '[:lower:]') + case "$v" in + true|1|yes|on|"") echo true ;; + false|0|no|off) echo false ;; + *) echo invalid ;; esac } +# True unless DISABLE_MOSQUITTO (or the value given) is a recognised false. +mosquitto_disabled() { + [ "$(parse_disable_mosquitto "${1-${DISABLE_MOSQUITTO:-}}")" != "false" ] +} + dc_prod() { - if is_true "${DISABLE_MOSQUITTO:-false}"; then + if mosquitto_disabled; then $DC -f docker-compose.no-mosquitto.yml "$@" else $DC "$@" @@ -84,7 +99,7 @@ dc_prod() { } dc_staging() { - if is_true "${DISABLE_MOSQUITTO:-false}"; then + if mosquitto_disabled; then $DC -f docker-compose.staging.no-mosquitto.yml -p corescope-staging "$@" else $DC -f "$STAGING_COMPOSE_FILE" -p corescope-staging "$@" @@ -97,12 +112,6 @@ confirm() { [[ $REPLY =~ ^[Yy]$ ]] } -confirm_yes_default() { - read -p " $1 [Y/n] " -n 1 -r - echo - [[ -z "$REPLY" || $REPLY =~ ^[Yy]$ ]] -} - # State tracking — marks completed steps so re-runs skip them mark_done() { echo "$1" >> "$STATE_FILE"; } is_done() { [ -f "$STATE_FILE" ] && grep -qx "$1" "$STATE_FILE" 2>/dev/null; } @@ -370,7 +379,7 @@ preflight_validate_prod_ports() { local http_port="${PROD_HTTP_PORT:-80}" local https_port="${PROD_HTTPS_PORT:-443}" local mqtt_port="" - if ! is_true "${DISABLE_MOSQUITTO:-false}"; then + if ! mosquitto_disabled; then mqtt_port="${PROD_MQTT_PORT:-1883}" fi local failed=0 @@ -437,39 +446,12 @@ mask_broker_text() { -e 's#(://[^[:space:]]*)\?[^[:space:]]*#\1#g' } -# True when the bundled mosquitto will run on a host managed by THIS script. -# -# manage.sh is the authority for its own hosts, not the container entrypoint: -# dc_prod/dc_staging pick the compose file with is_true(), and -# docker-compose.no-mosquitto.yml / docker-compose.staging.no-mosquitto.yml -# hard-code DISABLE_MOSQUITTO=true in the container's environment. So "yes", -# "1", "on" and "TRUE" all end up EXTERNAL here, even though the entrypoint -# on its own only accepts the exact string "true". Reporting the entrypoint's -# reading instead would claim "bundled" for those values and skip the source -# validation that catches a deploy which ingests nothing. +# True when the bundled mosquitto will run. manage.sh, the entrypoint and +# the ingestor all read DISABLE_MOSQUITTO with parse_disable_mosquitto's +# table (#441), so the mode reported here is the mode the container is in +# whichever compose file is used. bundled_broker_enabled() { - ! is_true "${DISABLE_MOSQUITTO:-false}" -} - -# True when DISABLE_MOSQUITTO means one thing to this script and another to a -# bare `docker compose up` against docker-compose.yml. is_true() accepts -# 1/true/TRUE/yes/YES/y/Y/on/ON; the entrypoint accepts only the exact -# "true". A value in that gap is external under manage.sh (which swaps in the -# no-mosquitto compose file) and bundled without it. -disable_mosquitto_is_ambiguous() { - [ "${1:-}" = "true" ] && return 1 - is_true "${1:-}" -} - -# True when DISABLE_MOSQUITTO is set to something NEITHER this script nor the -# container entrypoint recognises — " true" with a stray space, or a typo. -# Both then fall back to keeping the bundled broker, which may be the -# opposite of what was meant. -disable_mosquitto_is_unrecognised() { - case "${1:-}" in - ""|true|false|FALSE|False|0|no|NO|n|N|off|OFF) return 1 ;; - esac - ! is_true "${1:-}" + ! mosquitto_disabled } # Host part of a broker URL, lowercased, with scheme, user-info, port and @@ -571,35 +553,37 @@ broker_check_report() { local cfg="${1:-$PROD_DATA/config.json}" local errors=0 local raw="${DISABLE_MOSQUITTO:-}" - - if disable_mosquitto_is_ambiguous "$raw"; then - warn "DISABLE_MOSQUITTO='${raw}' means different things to different launchers." - echo " This script's is_true() accepts it, so ./manage.sh runs the external-broker" - echo " compose file, which hard-codes DISABLE_MOSQUITTO=true in the container — the" - echo " bundled broker will NOT start, and that is the mode reported below. A bare" - echo " 'docker compose up' against docker-compose.yml passes '${raw}' straight" - echo " through, and the entrypoint disables the bundled broker only on the exact" - echo " value \"true\", so there the bundled broker WOULD start. Set it to \"true\"" - echo " or \"false\" so both read it the same way." - elif disable_mosquitto_is_unrecognised "$raw"; then - warn "DISABLE_MOSQUITTO='${raw}' is neither a recognised true nor a recognised false value." - echo " This script's is_true() rejects it and the container entrypoint accepts only" - echo " the exact value \"true\", so both keep the bundled broker running. If you" - echo " meant to use an external broker, set it to \"true\"." - fi + local parsed setting + parsed=$(parse_disable_mosquitto "$raw") + + case "$parsed" in + invalid) + warn "DISABLE_MOSQUITTO='${raw}' is not a recognised value (true/1/yes/on or false/0/no/off)." + echo " The default applies, so the bundled broker will NOT start. Set it to \"false\"" + echo " to run the bundled broker, or to \"true\" to silence this warning." + setting="DISABLE_MOSQUITTO=${raw}, not recognised, default true" + ;; + *) + if [ -z "$raw" ]; then + setting="DISABLE_MOSQUITTO unset, default true" + else + setting="DISABLE_MOSQUITTO=${raw}, read as ${parsed}" + fi + ;; + esac if bundled_broker_enabled; then - log "Broker mode: bundled mosquitto (DISABLE_MOSQUITTO=${raw:-false})." + log "Broker mode: bundled mosquitto (${setting})." return 0 fi - info "Broker mode: external (DISABLE_MOSQUITTO=${raw}) — the bundled broker will not start." + info "Broker mode: external (${setting}) — the bundled broker will not start." local sources sources=$(broker_sources "$cfg") if [ -z "$sources" ]; then err "No external MQTT source is configured, so nothing will be ingested." - echo " DISABLE_MOSQUITTO=true disables the bundled broker, but the ingestor then" + echo " The bundled broker is off (it is opt-in), but the ingestor then" echo " falls back to its built-in mqtt://localhost:1883 — the bundled broker's own" echo " address — where nothing is listening." echo "" @@ -832,7 +816,7 @@ cmd_setup() { local selected_http="$default_http" local selected_https="$default_https" local selected_mqtt="$default_mqtt" - local selected_disable_mosquitto="${DISABLE_MOSQUITTO:-false}" + local selected_disable_mosquitto="${DISABLE_MOSQUITTO:-true}" local selected_data_dir="${PROD_DATA_DIR:-$HOME/meshcore-data}" local env_http="" @@ -856,7 +840,7 @@ cmd_setup() { fi local has_current_ports=false - if is_true "$selected_disable_mosquitto"; then + if mosquitto_disabled "$selected_disable_mosquitto"; then if is_valid_port "$env_http" && is_valid_port "$env_https"; then has_current_ports=true fi @@ -911,7 +895,8 @@ cmd_setup() { selected_http=$(prompt_for_port "HTTP" "$default_http" "$suggested_http") selected_https=$(prompt_for_port "HTTPS" "$default_https" "$suggested_https") - if confirm_yes_default "Use built-in MQTT broker?"; then + # The bundled broker is opt-in (#441), so Enter keeps it off. + if confirm "Use built-in MQTT broker?"; then selected_disable_mosquitto="false" if is_port_in_use "$default_mqtt"; then warn "Port ${default_mqtt} is in use." @@ -924,7 +909,7 @@ cmd_setup() { selected_mqtt=$(prompt_for_port "MQTT" "$default_mqtt" "$suggested_mqtt") else selected_disable_mosquitto="true" - log "Internal MQTT broker disabled." + log "Internal MQTT broker disabled. Point mqttSources in config.json at your broker." fi fi @@ -1014,7 +999,7 @@ cmd_setup() { echo " Resolved port mapping:" echo " UI HTTP: ${selected_http}" echo " UI HTTPS: ${selected_https}" - if is_true "$selected_disable_mosquitto"; then + if mosquitto_disabled "$selected_disable_mosquitto"; then echo " MQTT: disabled (external broker)" else echo " MQTT: ${selected_mqtt}" @@ -1311,7 +1296,7 @@ cmd_start() { info "Starting staging container (${STAGING_CONTAINER}) on port ${STAGING_GO_HTTP_PORT:-82}..." dc_prod up -d prod dc_staging up -d staging-go - if is_true "${DISABLE_MOSQUITTO:-false}"; then + if mosquitto_disabled; then log "Production started on ports ${PROD_HTTP_PORT:-80}/${PROD_HTTPS_PORT:-443} (MQTT disabled)" log "Staging started on port ${STAGING_GO_HTTP_PORT:-82} (MQTT disabled)" else diff --git a/scripts/test-docker-nonroot.sh b/scripts/test-docker-nonroot.sh index 320af36c7..ecbbbbeb0 100644 --- a/scripts/test-docker-nonroot.sh +++ b/scripts/test-docker-nonroot.sh @@ -197,6 +197,9 @@ DATA_BEFORE=$(tree "$V0") for combo in "none" "DISABLE_MOSQUITTO" "DISABLE_CADDY" "DISABLE_MOSQUITTO DISABLE_CADDY"; do dis=() for d in $combo; do [ "$d" = none ] || dis+=(-e "$d=true"); done + # The bundled broker is opt-in (#441): the combinations that keep it ask + # for it explicitly. + case " $combo " in *" DISABLE_MOSQUITTO "*) ;; *) dis+=(-e DISABLE_MOSQUITTO=false) ;; esac case "$combo" in none) conf=supervisord.conf ;; DISABLE_MOSQUITTO) conf=supervisord-no-mosquitto.conf ;; diff --git a/scripts/test-nonroot-entrypoint.sh b/scripts/test-nonroot-entrypoint.sh index af3d6d0ae..5d72057e7 100644 --- a/scripts/test-nonroot-entrypoint.sh +++ b/scripts/test-nonroot-entrypoint.sh @@ -146,6 +146,9 @@ BEFORE=$(tree "$V") for combo in none DISABLE_MOSQUITTO DISABLE_CADDY "DISABLE_MOSQUITTO DISABLE_CADDY"; do dis=() for d in $combo; do [ "$d" = none ] || dis+=(-e "$d=true"); done + # The bundled broker is opt-in (#441): the combinations that keep it ask + # for it explicitly. + case " $combo " in *" DISABLE_MOSQUITTO "*) ;; *) dis+=(-e DISABLE_MOSQUITTO=false) ;; esac case "$combo" in none) conf=supervisord.conf ;; DISABLE_MOSQUITTO) conf=supervisord-no-mosquitto.conf ;; @@ -328,7 +331,9 @@ layout "$V" start "$V" "${ON[@]}" start "$V" check "off after on, clean volume: exit status" 0 "$RC" -has "off after on, clean volume: shipped config" "$OUT" '^argv: -c /etc/supervisor/conf.d/supervisord.conf$' +# DISABLE_MOSQUITTO is unset here, so the shipped config is the one without +# the bundled broker, which is opt-in (#441). +has "off after on, clean volume: shipped config" "$OUT" '^argv: -c /etc/supervisor/conf.d/supervisord-no-mosquitto.conf$' has "off after on, clean volume: .env sourced" "$OUT" '^env: FROM_ENV_FILE=yes$' has "off after on: says it took back group write and the sticky bit" "$OUT" '^\[nonroot\] the mode is off: removed group write and the sticky bit' check "off after on: data dir no longer group-writable or sticky (fs.protected_regular)" "$H:$G 755" "$(stat_of "$V" .)" diff --git a/test-all.sh b/test-all.sh index 1f510f1aa..20a8b7b4a 100755 --- a/test-all.sh +++ b/test-all.sh @@ -70,6 +70,7 @@ run test-issue-405-observers-listener-leak.js run test-issue-322-comment-guards.js run test-backup-sqlite-safety.js run test-issue-372-broker-check.js +run test-issue-441-disable-mosquitto.js run test-perf-go-runtime.js run test-channel-psk-ux.js run test-channel-sidebar-layout.js diff --git a/test-fixtures/disable-mosquitto-cases.json b/test-fixtures/disable-mosquitto-cases.json new file mode 100644 index 000000000..9909a5f9f --- /dev/null +++ b/test-fixtures/disable-mosquitto-cases.json @@ -0,0 +1,44 @@ +{ + "_comment": "Issue #441: the one DISABLE_MOSQUITTO table. docker/entrypoint-go.sh, cmd/ingestor (ResolveBrokerMode) and manage.sh must all read every value here the same way; test-issue-441-disable-mosquitto.js and cmd/ingestor/broker_mode_test.go both load this file. value null means unset. disabled: the bundled mosquitto does NOT run. recognised: false means the value is junk, falls back to the default (disabled) and is reported.", + "cases": [ + { "value": null, "disabled": true, "recognised": true }, + { "value": "", "disabled": true, "recognised": true }, + { "value": " ", "disabled": true, "recognised": true }, + + { "value": "true", "disabled": true, "recognised": true }, + { "value": "TRUE", "disabled": true, "recognised": true }, + { "value": "True", "disabled": true, "recognised": true }, + { "value": " true ", "disabled": true, "recognised": true }, + { "value": "\ttrue", "disabled": true, "recognised": true }, + { "value": "1", "disabled": true, "recognised": true }, + { "value": "yes", "disabled": true, "recognised": true }, + { "value": "YES", "disabled": true, "recognised": true }, + { "value": "Yes", "disabled": true, "recognised": true }, + { "value": "on", "disabled": true, "recognised": true }, + { "value": "ON", "disabled": true, "recognised": true }, + { "value": " On", "disabled": true, "recognised": true }, + + { "value": "false", "disabled": false, "recognised": true }, + { "value": "FALSE", "disabled": false, "recognised": true }, + { "value": "False", "disabled": false, "recognised": true }, + { "value": " false ", "disabled": false, "recognised": true }, + { "value": "false\t", "disabled": false, "recognised": true }, + { "value": "0", "disabled": false, "recognised": true }, + { "value": "no", "disabled": false, "recognised": true }, + { "value": "NO", "disabled": false, "recognised": true }, + { "value": "No", "disabled": false, "recognised": true }, + { "value": "off", "disabled": false, "recognised": true }, + { "value": "OFF", "disabled": false, "recognised": true }, + { "value": " Off ", "disabled": false, "recognised": true }, + + { "value": "ture", "disabled": true, "recognised": false }, + { "value": "flase", "disabled": true, "recognised": false }, + { "value": "y", "disabled": true, "recognised": false }, + { "value": "n", "disabled": true, "recognised": false }, + { "value": "2", "disabled": true, "recognised": false }, + { "value": "-1", "disabled": true, "recognised": false }, + { "value": "enabled", "disabled": true, "recognised": false }, + { "value": "tr ue", "disabled": true, "recognised": false }, + { "value": "truefalse", "disabled": true, "recognised": false } + ] +} diff --git a/test-issue-372-broker-check.js b/test-issue-372-broker-check.js index 75f02104d..ba82fb66d 100644 --- a/test-issue-372-broker-check.js +++ b/test-issue-372-broker-check.js @@ -65,7 +65,8 @@ const externalWithGoodBroker = JSON.stringify({ }); test('bundled mode with no external config is reported as fine', () => { - const r = brokerCheck({ env: {}, config: null }); + // The bundled broker is opt-in since #441. + const r = brokerCheck({ env: { DISABLE_MOSQUITTO: 'false' }, config: null }); assert.strictEqual(r.status, 0, 'expected exit 0, got ' + r.status + '\n' + r.out); assert.match(r.out, /bundled/i, r.out); assert.doesNotMatch(r.out, /\berror\b/i, r.out); @@ -161,26 +162,12 @@ test('MQTT_BROKER overrides a loopback-only config.json', () => { assert.match(r.out, /env\.example/, r.out); }); -test('a DISABLE_MOSQUITTO value the entrypoint does not accept is reported', () => { - // manage.sh's is_true() takes 'yes'; the container entrypoint only takes - // the exact string 'true'. The two disagree about which broker runs, and - // the report must name the disagreement without failing over it. - const r = brokerCheck({ env: { DISABLE_MOSQUITTO: 'yes' }, config: externalWithGoodBroker }); - assert.match(r.out, /DISABLE_MOSQUITTO/, r.out); - assert.match(r.out, /yes/, r.out); - assert.match(r.out, /docker compose/i, 'the warning must say which launcher reads it differently:\n' + r.out); - assert.strictEqual(r.status, 0, 'an ambiguous value is a warning, not a failure\n' + r.out); -}); - -// Issue #372 review round 2, finding 2: broker-check must report the mode -// THIS SCRIPT selects, not the mode the entrypoint would pick from the raw -// value. dc_prod/dc_staging choose the compose file with is_true(), and -// docker-compose.no-mosquitto.yml hard-codes DISABLE_MOSQUITTO=true in the -// container — so 'yes', '1' and 'on' all end up in EXTERNAL mode on a -// manage.sh-managed host, and a check that says "bundled" there skips the -// source validation that would have caught a dead deploy. +// broker-check must report the mode the container will be in. Since #441 +// manage.sh, the entrypoint and the ingestor read DISABLE_MOSQUITTO with one +// table (test-issue-441-disable-mosquitto.js drives all of it); these cases +// keep the #372 view of it: the mode decides whether sources are validated. const MODE_BY_VALUE = [ - [undefined, 'bundled'], + [undefined, 'external'], ['false', 'bundled'], ['true', 'external'], ['1', 'external'], @@ -188,73 +175,62 @@ const MODE_BY_VALUE = [ ['on', 'external'], ]; -test('broker-check reports the mode manage.sh actually selects, for every DISABLE_MOSQUITTO value', () => { +test('broker-check reports the mode the container will be in, for every DISABLE_MOSQUITTO value', () => { for (const [value, want] of MODE_BY_VALUE) { const env = value === undefined ? {} : { DISABLE_MOSQUITTO: value }; const r = brokerCheck({ env, config: externalWithGoodBroker }); const other = want === 'bundled' ? 'external' : 'bundled'; assert.match(r.out, new RegExp('Broker mode: ' + want), - 'DISABLE_MOSQUITTO=' + value + ' selects ' + want + ' via is_true()/dc_prod:\n' + r.out); + 'DISABLE_MOSQUITTO=' + value + ' selects ' + want + ':\n' + r.out); assert.doesNotMatch(r.out, new RegExp('Broker mode: ' + other), 'DISABLE_MOSQUITTO=' + value + ' must not be reported as ' + other + ':\n' + r.out); } }); -test('an is_true value with no source fails the check, because the container will be external', () => { - // The compose file manage.sh picks for these values hard-codes - // DISABLE_MOSQUITTO=true, so there is no bundled broker to fall back on - // and an unconfigured source means nothing is ingested. - for (const value of ['true', '1', 'yes', 'on']) { - const r = brokerCheck({ env: { DISABLE_MOSQUITTO: value }, config: null }); +test('an external-mode value with no source fails the check', () => { + for (const value of [undefined, 'true', '1', 'yes', 'on']) { + const env = value === undefined ? {} : { DISABLE_MOSQUITTO: value }; + const r = brokerCheck({ env, config: null }); assert.strictEqual(r.status, 1, 'DISABLE_MOSQUITTO=' + value + ' is external, so a missing source must fail; got ' + r.status + '\n' + r.out); assert.match(r.out, /mqttSources/, r.out); } }); -test('a falsey or unset DISABLE_MOSQUITTO needs no external source', () => { - for (const value of [undefined, 'false']) { - const env = value === undefined ? {} : { DISABLE_MOSQUITTO: value }; - const r = brokerCheck({ env, config: null }); +test('a false DISABLE_MOSQUITTO needs no external source', () => { + for (const value of ['false', '0', 'no', 'off']) { + const r = brokerCheck({ env: { DISABLE_MOSQUITTO: value }, config: null }); assert.strictEqual(r.status, 0, 'DISABLE_MOSQUITTO=' + value + ' runs the bundled broker; got ' + r.status + '\n' + r.out); } }); -test('an unambiguous value produces no ambiguity warning', () => { - for (const value of ['true', 'false']) { +test('a recognised value produces no warning about the value', () => { + for (const value of ['true', 'false', 'yes', 'OFF']) { const r = brokerCheck({ env: { DISABLE_MOSQUITTO: value }, config: externalWithGoodBroker }); - assert.doesNotMatch(r.out, /different things|neither a recognised/i, + assert.doesNotMatch(r.out, /different things|not a recognised/i, 'DISABLE_MOSQUITTO=' + value + ':\n' + r.out); } const unset = brokerCheck({ env: {}, config: externalWithGoodBroker }); - assert.doesNotMatch(unset.out, /different things|neither a recognised/i, unset.out); + assert.doesNotMatch(unset.out, /different things|not a recognised/i, unset.out); }); -test('a value neither launcher recognises is reported as keeping the bundled broker', () => { - // A typo falls through is_true() and the entrypoint's exact comparison, so - // the bundled broker runs either way. +test('an unrecognised value is reported and takes the default (external)', () => { const r = brokerCheck({ env: { DISABLE_MOSQUITTO: 'ture' }, config: null }); - assert.match(r.out, /neither a recognised/i, r.out); - assert.match(r.out, /Broker mode: bundled/, r.out); - assert.strictEqual(r.status, 0, r.out); - - // ' true' (stray space) is the same case, but it cannot be delivered - // through .env: manage.sh's reader trims it, and so does docker compose's. - // Through the process environment it survives, and then neither is_true() - // nor the entrypoint's `= "true"` accepts it. - const spaced = brokerCheck({ env: {}, procEnv: { DISABLE_MOSQUITTO: ' true' }, config: null }); - assert.match(spaced.out, /neither a recognised/i, spaced.out); - assert.match(spaced.out, /Broker mode: bundled/, spaced.out); - assert.strictEqual(spaced.status, 0, spaced.out); + assert.match(r.out, /not a recognised/i, r.out); + assert.match(r.out, /Broker mode: external/, r.out); + assert.strictEqual(r.status, 1, 'external with no source:\n' + r.out); }); -test('manage.sh trims a .env value, so a spaced DISABLE_MOSQUITTO from .env still works', () => { - // Documents the asymmetry behind review finding 7: the whitespace a value - // may carry depends on how it was delivered, not on which reader sees it. - const r = brokerCheck({ env: { DISABLE_MOSQUITTO: ' true' }, config: externalWithGoodBroker }); - assert.match(r.out, /Broker mode: external/, r.out); - assert.doesNotMatch(r.out, /neither a recognised/i, r.out); +test('a spaced DISABLE_MOSQUITTO reads the same from .env and from the process environment', () => { + // manage.sh's .env reader trims; since #441 the parser trims as well, so + // the delivery route no longer changes the meaning. + const viaEnv = brokerCheck({ env: { DISABLE_MOSQUITTO: ' true' }, config: externalWithGoodBroker }); + const viaProc = brokerCheck({ env: {}, procEnv: { DISABLE_MOSQUITTO: ' true' }, config: externalWithGoodBroker }); + for (const r of [viaEnv, viaProc]) { + assert.match(r.out, /Broker mode: external/, r.out); + assert.doesNotMatch(r.out, /not a recognised/i, r.out); + } }); test('broker credentials never reach the output', () => { diff --git a/test-issue-441-disable-mosquitto.js b/test-issue-441-disable-mosquitto.js new file mode 100644 index 000000000..a46e3d6ad --- /dev/null +++ b/test-issue-441-disable-mosquitto.js @@ -0,0 +1,251 @@ +'use strict'; +// Issue #441 — the bundled mosquitto is opt-in, and DISABLE_MOSQUITTO means +// the same thing to every launcher. +// +// The three readers — docker/entrypoint-go.sh (inside the image), manage.sh +// (on the host) and the ingestor (cmd/ingestor, tested in Go) — are driven +// here, for the shell two, over the one table in +// test-fixtures/disable-mosquitto-cases.json that the Go test also loads. +// +// The entrypoint is the shipped file with its absolute paths pointed into a +// throwaway directory and a stub supervisord that prints the config it was +// handed. manage.sh runs in a sandbox with a stub `docker` that records its +// argv. No daemon, container, network or real data directory is touched. +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const root = __dirname; +let passed = 0; +let failed = 0; +function test(name, fn) { + try { fn(); passed++; console.log('PASS ' + name); } + catch (err) { failed++; console.error('FAIL ' + name + ': ' + err.message); } +} + +const CASES = JSON.parse(fs.readFileSync(path.join(root, 'test-fixtures', 'disable-mosquitto-cases.json'), 'utf8')).cases; +const show = (v) => (v === null ? '' : JSON.stringify(v)); + +// ─── docker/entrypoint-go.sh ───────────────────────────────────────────── + +// Absolute paths in the entrypoint, and where the harness puts them. Each +// must be present: if one moves, this harness must be updated rather than +// silently running the entrypoint against the real filesystem. +function entrypointSandbox() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cs441-ep-')); + fs.mkdirSync(path.join(dir, 'app', 'data'), { recursive: true }); + const stub = path.join(dir, 'supervisord'); + fs.writeFileSync(stub, '#!/bin/sh\necho "argv: $*"\n'); + fs.chmodSync(stub, 0o755); + const nonroot = path.join(dir, 'nonroot.sh'); + fs.writeFileSync(nonroot, '#!/bin/sh\necho "nonroot helper must not run in this test" >&2\nexit 1\n'); + fs.chmodSync(nonroot, 0o755); + + let src = fs.readFileSync(path.join(root, 'docker', 'entrypoint-go.sh'), 'utf8'); + for (const [from, to] of [ + ['/usr/bin/supervisord', stub], + ['/usr/local/lib/corescope/nonroot.sh', nonroot], + ['/app/', path.join(dir, 'app') + '/'], + ]) { + assert.ok(src.includes(from), 'docker/entrypoint-go.sh no longer mentions ' + from + '; update this harness'); + src = src.split(from).join(to); + } + fs.writeFileSync(path.join(dir, 'entrypoint.sh'), src); + return dir; +} + +function runEntrypoint({ procEnv = {}, dataEnv = null } = {}) { + const dir = entrypointSandbox(); + if (dataEnv !== null) fs.writeFileSync(path.join(dir, 'app', 'data', '.env'), dataEnv); + const res = spawnSync('sh', [path.join(dir, 'entrypoint.sh')], { + cwd: dir, + encoding: 'utf8', + env: { PATH: process.env.PATH, ...procEnv }, + }); + const out = (res.stdout || '') + (res.stderr || ''); + const m = out.match(/^argv: -c \/etc\/supervisor\/conf\.d\/(\S+)$/m); + return { status: res.status, out, conf: m ? m[1] : null }; +} + +function entrypointFor(value, extra = {}) { + const procEnv = { ...extra }; + if (value !== null) procEnv.DISABLE_MOSQUITTO = value; + return runEntrypoint({ procEnv }); +} + +test('entrypoint: every DISABLE_MOSQUITTO value selects the table\'s broker mode', () => { + for (const c of CASES) { + const r = entrypointFor(c.value); + assert.strictEqual(r.status, 0, show(c.value) + ':\n' + r.out); + const want = c.disabled ? 'supervisord-no-mosquitto.conf' : 'supervisord.conf'; + assert.strictEqual(r.conf, want, 'DISABLE_MOSQUITTO=' + show(c.value) + ':\n' + r.out); + if (c.disabled) assert.match(r.out, /internal MQTT broker disabled/, show(c.value) + ':\n' + r.out); + else assert.doesNotMatch(r.out, /internal MQTT broker disabled/, show(c.value) + ':\n' + r.out); + } +}); + +test('entrypoint: a junk value is reported and falls back to the default', () => { + for (const c of CASES) { + const r = entrypointFor(c.value); + if (c.recognised) { + assert.doesNotMatch(r.out, /not a recognised/i, show(c.value) + ' is recognised:\n' + r.out); + } else { + assert.match(r.out, /DISABLE_MOSQUITTO/, r.out); + assert.match(r.out, /not a recognised/i, show(c.value) + ' is junk and must be reported:\n' + r.out); + assert.match(r.out, /default/i, 'the report must say the default applies:\n' + r.out); + } + } +}); + +test('entrypoint: DISABLE_CADDY still combines with the normalised value', () => { + const both = entrypointFor('Yes', { DISABLE_CADDY: 'true' }); + assert.strictEqual(both.conf, 'supervisord-no-mosquitto-no-caddy.conf', both.out); + const unsetBroker = entrypointFor(null, { DISABLE_CADDY: 'true' }); + assert.strictEqual(unsetBroker.conf, 'supervisord-no-mosquitto-no-caddy.conf', unsetBroker.out); + const caddyOnly = entrypointFor('OFF', { DISABLE_CADDY: 'true' }); + assert.strictEqual(caddyOnly.conf, 'supervisord-no-caddy.conf', caddyOnly.out); +}); + +test('entrypoint: the data-volume .env is read with the same rules', () => { + // docker/entrypoint-go.sh sources /app/data/.env after the container + // environment is set, so a value there overrides compose's. + const on = runEntrypoint({ procEnv: { DISABLE_MOSQUITTO: 'false' }, dataEnv: 'DISABLE_MOSQUITTO=YES\n' }); + assert.strictEqual(on.conf, 'supervisord-no-mosquitto.conf', on.out); + const off = runEntrypoint({ procEnv: { DISABLE_MOSQUITTO: 'true' }, dataEnv: 'DISABLE_MOSQUITTO=Off\n' }); + assert.strictEqual(off.conf, 'supervisord.conf', off.out); +}); + +// ─── manage.sh ─────────────────────────────────────────────────────────── + +const externalWithGoodBroker = JSON.stringify({ + mqttSources: [{ name: 'ext', broker: 'mqtt://broker.example:1883', topics: ['meshcore/#'] }], +}); + +// `route` is how the value reaches manage.sh: through .env (its own reader +// trims it) or through the process environment (delivered verbatim). +const manageCache = new Map(); +function manage(args, opts) { + const key = JSON.stringify([args, opts]); + if (!manageCache.has(key)) manageCache.set(key, runManage(args, opts)); + return manageCache.get(key); +} + +function runManage(args, { value, route = 'env', config = externalWithGoodBroker }) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cs441-ms-')); + fs.copyFileSync(path.join(root, 'manage.sh'), path.join(dir, 'manage.sh')); + for (const name of ['docker-compose.yml', 'docker-compose.no-mosquitto.yml']) { + fs.copyFileSync(path.join(root, name), path.join(dir, name)); + } + const data = path.join(dir, 'data'); + fs.mkdirSync(data); + if (config !== null) fs.writeFileSync(path.join(data, 'config.json'), config); + const bin = path.join(dir, 'bin'); + fs.mkdirSync(bin); + const argvLog = path.join(dir, 'docker-argv.log'); + fs.writeFileSync(path.join(bin, 'docker'), '#!/bin/sh\necho "$*" >> "' + argvLog + '"\nexit 0\n'); + fs.chmodSync(path.join(bin, 'docker'), 0o755); + + const envLines = ['PROD_DATA_DIR=' + data]; + const procEnv = {}; + if (value !== null) { + if (route === 'env') envLines.push('DISABLE_MOSQUITTO=' + value); + else procEnv.DISABLE_MOSQUITTO = value; + } + fs.writeFileSync(path.join(dir, '.env'), envLines.join('\n') + '\n'); + + const res = spawnSync('bash', ['manage.sh', ...args], { + cwd: dir, + encoding: 'utf8', + env: { PATH: bin + ':' + process.env.PATH, HOME: dir, TERM: 'dumb', ...procEnv }, + }); + const argv = fs.existsSync(argvLog) ? fs.readFileSync(argvLog, 'utf8') : ''; + return { status: res.status, out: (res.stdout || '') + (res.stderr || ''), argv }; +} + +// .env cannot carry a value whose meaning depends on a leading or trailing +// tab the reader strips, but every case can travel through the process env. +const ROUTES = ['env', 'proc']; + +test('manage.sh: broker-check reports the table\'s mode for every value and route', () => { + for (const c of CASES) { + for (const route of ROUTES) { + const r = manage(['broker-check'], { value: c.value, route }); + const want = c.disabled ? 'external' : 'bundled'; + const other = c.disabled ? 'bundled' : 'external'; + assert.match(r.out, new RegExp('Broker mode: ' + want), show(c.value) + ' via ' + route + ':\n' + r.out); + assert.doesNotMatch(r.out, new RegExp('Broker mode: ' + other), show(c.value) + ' via ' + route + ':\n' + r.out); + } + } +}); + +test('manage.sh: dc_prod picks the compose file the table implies', () => { + for (const c of CASES) { + for (const route of ROUTES) { + const r = manage(['logs', 'prod'], { value: c.value, route }); + const line = r.argv.split('\n').find((l) => /\blogs\b/.test(l)) || ''; + assert.ok(line, 'no `docker compose ... logs` call recorded:\n' + r.argv + '\n' + r.out); + const noMosq = /-f docker-compose\.no-mosquitto\.yml/.test(line); + assert.strictEqual(noMosq, c.disabled, + 'DISABLE_MOSQUITTO=' + show(c.value) + ' via ' + route + ' ran: ' + line); + } + } +}); + +test('manage.sh: no launcher-mismatch warning is left; junk is reported once', () => { + for (const c of CASES) { + for (const route of ROUTES) { + const r = manage(['broker-check'], { value: c.value, route }); + assert.doesNotMatch(r.out, /different things to different launchers|is_true\(\)/, + show(c.value) + ' via ' + route + ': parsing is consistent, so no mismatch may be reported:\n' + r.out); + if (c.recognised) { + assert.doesNotMatch(r.out, /not a recognised/i, show(c.value) + ' via ' + route + ':\n' + r.out); + } else { + assert.match(r.out, /not a recognised/i, show(c.value) + ' via ' + route + ' is junk:\n' + r.out); + } + } + } +}); + +test('manage.sh: broker-check names the effective value it acted on', () => { + const yes = manage(['broker-check'], { value: 'YES', route: 'env' }); + assert.match(yes.out, /Broker mode: external \(DISABLE_MOSQUITTO=YES, read as true\)/, yes.out); + const off = manage(['broker-check'], { value: 'Off', route: 'env' }); + assert.match(off.out, /Broker mode: bundled mosquitto \(DISABLE_MOSQUITTO=Off, read as false\)/, off.out); + const unset = manage(['broker-check'], { value: null }); + assert.match(unset.out, /Broker mode: external \(DISABLE_MOSQUITTO unset, default true\)/, unset.out); + const junk = manage(['broker-check'], { value: 'ture', route: 'env' }); + assert.match(junk.out, /Broker mode: external \(DISABLE_MOSQUITTO=ture, not recognised, default true\)/, junk.out); +}); + +test('manage.sh: an unset DISABLE_MOSQUITTO with no source fails the check (opt-in broker)', () => { + const r = manage(['broker-check'], { value: null, config: null }); + assert.strictEqual(r.status, 1, 'unset now means external, so a missing source must fail:\n' + r.out); + assert.match(r.out, /DISABLE_MOSQUITTO=false/, 'the remediation must name the opt-in:\n' + r.out); + const optedIn = manage(['broker-check'], { value: 'false', config: null }); + assert.strictEqual(optedIn.status, 0, optedIn.out); +}); + +test('manage.sh: the setup wizard defaults to no bundled broker', () => { + const src = fs.readFileSync(path.join(root, 'manage.sh'), 'utf8'); + const prompt = src.match(/^\s*if (confirm\w*) "Use built-in MQTT broker\?"; then$/m); + assert.ok(prompt, 'the setup wizard must still ask about the built-in broker'); + assert.strictEqual(prompt[1], 'confirm', + 'the built-in broker is opt-in, so the prompt must default to No (confirm, not confirm_yes_default)'); + assert.doesNotMatch(src, /DISABLE_MOSQUITTO:-false/, 'no manage.sh default may still be false'); +}); + +// ─── shipped defaults ──────────────────────────────────────────────────── + +test('.env.example ships DISABLE_MOSQUITTO=true with that default documented', () => { + const env = fs.readFileSync(path.join(root, '.env.example'), 'utf8'); + assert.match(env, /^DISABLE_MOSQUITTO=true$/m, env); + const lines = env.split('\n'); + const i = lines.findIndex((l) => l.startsWith('DISABLE_MOSQUITTO=')); + const block = lines.slice(Math.max(0, i - 8), i).join('\n'); + assert.match(block, /^# Default: true$/m, block); +}); + +console.log('\n' + passed + ' passed, ' + failed + ' failed'); +process.exit(failed ? 1 : 0);