diff --git a/AGENTS.md b/AGENTS.md index b09fd65b..893d3020 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,7 +33,7 @@ Sixteen internal packages under `internal/` (plus `internal/testutil/` for share - **`cache/`** — `Cache` interface → `LocalCache` (Ristretto) + `SharedCache` (TBD) + `TieredCache` (singleflight) - **`chconn/`** — `Manager`, the one ClickHouse `driver.Conn` every consumer holds; `Reconfigure` swaps the connection behind it after a settings reload changes the wiring (never dials; the old connection closes after a `query_timeout` grace) - **`chsql/`** — dependency-free ClickHouse SQL helpers shared by `query`/`policy` (avoids an import cycle): `QuoteIdent` (backtick-quote every identifier) + `BindUnsafe` (reject names with a literal `?`) -- **`config/`** — YAML + env var config loading (cleanenv) +- **`config/`** — YAML + env var config loading (cleanenv); strict on both sides (undeclared YAML key, unbound `WH_*` variable) and probes `data_dir` writability — boot is the validator, there is no dry run - **`dedupe/`** — `Deduplicator` interface → `Embedded` (Pebble), wrapped by `Managed` whose open/closed state follows the hot-reloadable `dedupe.enabled` in the settings directory's `config.json` - **`discovery/`** — `SchemaRegistry` that introspects ClickHouse `system.columns` (name/type/nullability plus `default_expression` and 1-based `position`) and `system.tables` (each table's `create_table_query`, kept in-process and never serialized — an external-engine table renders its wiring there unconditionally — endpoint, bucket/host, database, username, S3 access key id; ClickHouse masks the password as `[HIDDEN]` from ~23.9, so the exposure is the topology, not the secret), records the server version, + `Validate()` for ingest payloads + `CanonicalizeTimestamps()` rewriting top-level `DateTime`/`DateTime64` column values to the canonical RFC 3339 UTC wire form pre-publish (Key Design Decision #19) - **`ingest/`** — Ingest worker pipeline (`worker.go`: JetStream input → per-table batch INSERT with DLQ output). The pipeline is **insert-only**. The wire format `EventMessage` (`types.go`) carries `{table_name, scope, received_timestamp, format, columns, row}` and nothing else — `row` is one positional `JSONCompactEachRow` line and `columns` names its slots, the table's **insertable** columns (a `MATERIALIZED`/`ALIAS` column cannot be named in an `INSERT`); the worker batches per (table, column list); the worker accepts whatever table name the envelope carries (table existence was already checked by the HTTP ingest handler, which `404`s an unknown table before publish; the worker doesn't re-validate), then bulk-INSERTs. In the embedded-NATS deployment (the default), the server runs with `DontListen: true` (`internal/mq/embedded.go`), so the only Publishers reachable on the `ingest.>` subjects are in-process Go code — today, only the HTTP `/v1/ingest?table={table}` handler. Non-insert mutations (`DELETE`/`UPDATE`/`TRUNCATE`/…) must go through `POST /v1/ops/query` under the admin role (the same `RequireAdmin` gate as the rest of `/v1/ops/*`), so non-admin callers never reach the proxy. A request with no token (or an invalid one) resolves to the `default_role`, which in a production config is not the admin role (setting them equal is a loudly-warned dev-only setting), so it can't reach this endpoint. Plus `Sweeper` (Active Sweeper for NATS message lifecycle) + `EventMessage`/`BufferConsumerName` types (`types.go`) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61cf0d05..8b53b190 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ### Changed +- **Boot refuses an unbound `WH_*` environment variable and an unusable `data_dir`** (BREAKING; `internal/config/check.go` (new, + tests), `internal/config/{config,persistence}.go`, `cmd/wavehouse/main.go`, `docs/src/integrations/diagram-png.mjs`): the environment half of the strict YAML loader. `config.Load` now errors, naming every offender, on a `WH_*` variable that no `Config` field binds — the two variables read outside the struct, `WH_CONFIG` and `WH_LOG_LEVEL`, are exempt — `WH_DEDUPE_ENABLED=true` left in a compose file from before the settings-directory move, or a misspelling, was set, ignored, and believed. **An existing deployment that still exports a variable this release moved to the settings directory stops booting until it is unset**; the upgrade runbook in `deployment.md` gains that audit. Only the `WH_` prefix is checked, since the environment always carries unrelated names; the one outside source that shares it — Kubernetes service-link variables for a Service named `wh` or `wh-*` — is named in the error with the `enableServiceLinks: false` remediation, and the docs build's opt-out knob is renamed from `WH_SKIP_DIAGRAM_PNG` to `DOCS_SKIP_DIAGRAM_PNG` so an exported one no longer refuses a local boot. Right after `Load`, before ClickHouse or the settings directory are touched, `config.CheckDataDir` probes `data_dir` and refuses boot on any of: an empty or blank value (reachable through `WH_DATA_DIR=`), refused outright since the ancestor walk would otherwise fall back to the working directory and NATS and Pebble state would land under it; a path that exists and is not a directory; a dangling symlink at `data_dir` or any component above it (the walk to the nearest existing ancestor uses `Lstat`, so a failed mount is not skipped over as "does not exist" and passed in an unrelated directory); and a directory the process cannot write to — or, when it does not exist, an unwritable nearest ancestor — probed by creating and removing one temp file. So an unusable `data_dir` refuses boot before schema discovery rather than after it; a permission denial — on the probe, or on reaching the path at all through a parent without search permission — carries the UID-65532 remediation (a bind mount owned by root is the typical cause), and that hint string is now shared with `LogStorageInitError`. `EnvConfig` and `EnvLogLevel` join `EnvSettingsDir` as the exported names for the process-level variables. Boot is the validator for the non-hot-reloadable half — there is no dry-run subcommand, by decision on #530: boot config only takes effect through a restart, so the restart is where it is checked, and the docs say so. Closes #530. + - **The docs site now consumes the *published* `@wavehouse/sdk`, not the workspace one** (`docs/package.json`, `pnpm-workspace.yaml`, `Makefile`, `scripts/classify-paths.sh`). The landing page's live demo streams against a separately-deployed backend on its own release cadence, but took its SDK from the tree — so this release's wire change would have reached the deployed site the moment it merged, while the backend still spoke the old envelope: the panel keeps reporting "live" while every frame is dropped for want of a schema announcement, with no `error` callback ([#568](https://github.com/Wave-RF/WaveHouse/issues/568)). `docs` now pins `^0.1.1` from the registry, which takes `0.1.x` patches and stops short of `0.2.0`, so moving the site onto the new wire is a deliberate bump lined up with tagging the SDK release rather than a side effect of merging. `tests/e2e/sdk` deliberately keeps `workspace:*`. Depending on our own package from the registry also made `minimumReleaseAge` apply to it for the first time, and the exclude list named only `@wave-rf/*` (the plugin scope), so a freshly tagged SDK would have been uninstallable by the docs site for seven days — `@wavehouse/*` is now exempt too. The docs build no longer needs `build-ts`. - **Policy format v2: `policies.json` is role-first, and the two operations are separate permission types** (BREAKING; `internal/policy/policy.go`, `internal/policy/rowfilter.go`, `internal/settings/validate.go`, `internal/query/builder.go`, `internal/api/{ingest,structured_query}.go`, `internal/stream/hub.go`, `clients/ts/src/{types,index}.ts`, `deployments/compose/settings/policies.json`, `docs/src/content/docs/{access-control.mdx,settings-directory.mdx,architecture.md}`, `AGENTS.md`, `tests/e2e/sdk/`): a table entry was keyed `tables..select.`; it is now keyed `tables.
..select`. A role appears once per table and its grant carries two optional blocks, so a role that could both read and write no longer has to be written out twice, and "this role has no insert grant" is a missing block rather than an absence you have to notice in a second map. The blocks are now distinct types rather than one struct whose halves were inert per operation: `select` takes `allow_columns`, `deny_columns`, `filter`, `allowed_aggregations`, `denied_aggregations` and the four `max_*` limits; `insert` takes `allow_columns`, `deny_columns`, `check`. Field names and semantics are unchanged — only the nesting moves — but a field on the wrong side that the old layout *accepted* — the four `max_*` limits and the two aggregation rules — is now a validation error instead of being silently ignored. `filter` under an `insert` grant and `check` under a `select` one are rejected too, but that is not new here — [#541](https://github.com/Wave-RF/WaveHouse/pull/541), also unreleased, added the runtime check; the split types now refuse them one layer earlier, as unknown keys at the strict decode. Upgrading from **0.1.0**, though, none of the eight were enforced: a 0.1.0 policy could carry an insert-side `filter` that resolved into a `WHERE` the insert path never read, as well as an ignored limit. Converting to the role-first layout drops both. Internally `ResolvedPermissions` splits the same way (`.Select` / `.Insert`), and `IsColumnAllowed` takes the side to consult, which is what stops the read allowlist from ever answering a write question or vice versa. **There is no automatic conversion** — the settings files are the source of truth and WaveHouse has no write path back to them — so `policies.json` must be converted by hand; run `wavehouse validate` before restarting. A document still in the old layout is reported as one clear finding naming the table and operation and pointing at [the migration note](https://wavehouse.dev/access-control#migrating-from-the-operation-first-layout), instead of the confusing strict-decode "unknown field" error it would otherwise produce (or, for an empty operation block, silently decoding as a role named `select` with no grants — which fails the undeclared-role check when `roles.json` does not declare a role named `select` — the usual case, since `checkRoleRefs` errors and moves on before reaching the "grant sets neither select nor insert" warning. If such a role *is* declared, you get that warning instead and the document adopts). One shape is refused differently: a grant keyed by a role named after the *other* operation (`tables.t.select.insert`) reads as a different grant under each layout — different role, different operation, or both — so it gets its own error asking you to rename the role rather than the migration pointer. A role named after its *own* operation (`tables.t.select.select`) means the same thing either way and is accepted. diff --git a/cmd/wavehouse/main.go b/cmd/wavehouse/main.go index 99ff57b4..20afec50 100644 --- a/cmd/wavehouse/main.go +++ b/cmd/wavehouse/main.go @@ -150,7 +150,7 @@ func run() int { logger.Info("starting WaveHouse", "version", Version, "build_time", BuildTime, "git_commit", GitCommit) cfgPath := "config.yaml" - if p := os.Getenv("WH_CONFIG"); p != "" { + if p := os.Getenv(config.EnvConfig); p != "" { cfgPath = p } @@ -160,6 +160,16 @@ func run() int { return 1 } + // data_dir must be writable before anything dials out, so the refusal + // (and, for the typical cause — a bind mount owned by root rather than + // UID 65532 — the remediation) lands at the top of the log rather than + // after ClickHouse discovery. NATS and Pebble still fail loud on their + // own if the directory changes underneath us. + if err := config.CheckDataDir(cfg.DataDir); err != nil { + logger.Error("check data_dir", "error", err) + return 1 + } + // Settings directory — the hot-reloadable half of configuration (dedupe, // dlq, query, schema, stream, cors — see settings.TenantConfig). Required: // config.Validate already @@ -195,7 +205,7 @@ func run() int { serviceName := "wavehouse" var level slog.Level - switch strings.ToUpper(strings.TrimSpace(os.Getenv("WH_LOG_LEVEL"))) { + switch strings.ToUpper(strings.TrimSpace(os.Getenv(config.EnvLogLevel))) { case "DEBUG": level = slog.LevelDebug case "WARN": diff --git a/config.yaml b/config.yaml index 51cea3fa..f1300837 100644 --- a/config.yaml +++ b/config.yaml @@ -1,5 +1,7 @@ -# This file is strict: a key the server doesn't declare (a typo, or a tunable -# that moved to the settings directory) refuses to boot and is named. +# This file is strict, and so is the environment: a key the server doesn't +# declare (a typo, or a tunable that moved to the settings directory) refuses +# to boot and is named — whether it arrives as a YAML key here or as a WH_* +# variable no key binds. # Root for embedded state. NATS lives at /nats; Pebble (when dedupe # is enabled) lives at /pebble. In a container, this MUST be on a # host-backed volume — the relative default is for local binary use only. diff --git a/deployments/Dockerfile b/deployments/Dockerfile index a29c7088..bb74ecd7 100644 --- a/deployments/Dockerfile +++ b/deployments/Dockerfile @@ -29,7 +29,7 @@ RUN --mount=type=cache,target=/go/pkg/mod \ # subdirectories itself when NATS/Pebble open their stores — pre-creating # them here would buy nothing and obscures intent. Named-volume copy-up runs # against `/app/data` regardless; bind mounts mask the image dir entirely -# and rely on host-side ownership matching UID 65532. +# and rely on the host directory being writable by UID 65532. # # /app/settings is the settings-directory mount point, pre-created but # deliberately EMPTY: the image ships no settings, same as it ships no diff --git a/deployments/Dockerfile.goreleaser b/deployments/Dockerfile.goreleaser index 3f681482..8d22201c 100644 --- a/deployments/Dockerfile.goreleaser +++ b/deployments/Dockerfile.goreleaser @@ -7,7 +7,7 @@ # Only the parents are pre-created — the binary mkdirs `nats/` and `pebble/` # subdirs itself when NATS/Pebble open their stores. Named-volume copy-up # runs against `/app/data`; bind mounts mask the image dir entirely and -# require host-side ownership matching UID 65532. +# require the host directory to be writable by UID 65532. # Pin to the builder's native arch via $BUILDPLATFORM. Without this, a # cross-arch build (e.g. linux/arm64 from an amd64 builder) would default diff --git a/docs/src/content/docs/architecture.md b/docs/src/content/docs/architecture.md index e52e9a03..72c2e766 100644 --- a/docs/src/content/docs/architecture.md +++ b/docs/src/content/docs/architecture.md @@ -107,7 +107,10 @@ The SSE fan-out, factored out of `api/` so the delivery hot path ([#294](https:/ ### `config/` — Configuration -- **config.go** — Loads *boot* configuration from a YAML file with environment variable overrides (using [cleanenv](https://github.com/ilyakaznacheev/cleanenv)); every key has a `WH_`-prefixed env var. Boot config is only what can't change under a running process — resource sizing, listeners, observability exporters, the settings-directory path, and the secrets (`clickhouse.password`, `auth.jwt_secret`, `auth.operator_key`). Everything tenant-tunable lives in the settings directory (`settings/`). The YAML is strict: `rejectUnknownKeys` refuses to boot naming every key the struct doesn't declare, so a tunable that moved to the settings directory can't be read, ignored, and believed. See [Configuration Reference](/configuration). +- **config.go** — Loads *boot* configuration from a YAML file with environment variable overrides (using [cleanenv](https://github.com/ilyakaznacheev/cleanenv)); every key has a `WH_`-prefixed env var. Boot config is only what can't change under a running process — resource sizing, listeners, observability exporters, the settings-directory path, and the secrets (`clickhouse.password`, `auth.jwt_secret`, `auth.operator_key`). Everything tenant-tunable lives in the settings directory (`settings/`). Both sources are strict: `Load` refuses to boot naming every YAML key the struct doesn't declare (`strict.go`) and every `WH_*` environment variable no field binds (`check.go`), so a tunable that moved to the settings directory can't be read, ignored, and believed. Boot is the validator for this half — there is no dry-run command. See [Configuration Reference](/configuration). +- **check.go** — `rejectUnboundEnv` is the environment half of the strict loader: `unboundEnv` walks the struct's `env` tags (plus the two process-level names, `WH_CONFIG` and `WH_LOG_LEVEL`) against the environment; `CheckDataDir` probes `data_dir` — run by `main` right after `Load`, so an unusable `data_dir` refuses boot before anything dials out. It refuses an empty value (reachable through `WH_DATA_DIR=`) outright rather than probing the working directory; a path that exists and is not a directory; a dangling symlink at `data_dir` or any component above it (the walk to the nearest existing ancestor uses `Lstat`, so a failed mount is not skipped over as "does not exist"); and a directory the process cannot write to — or, when it does not exist, an unwritable nearest ancestor — probed by creating and removing one temp file. A permission denial, on the probe or on reaching the path through a parent without search permission, carries the UID-65532 hint, since a bind mount owned by root is the typical cause. +- **strict.go** — `rejectUnknownKeys`, the YAML half: re-reads the file as a generic tree and walks it against the struct's `yaml` tags, listing every key the struct doesn't declare. cleanenv itself is lenient by design, which is exactly wrong for boot config once keys have moved to the settings directory. +- **persistence.go** — `WarnIfFreshDataDir` logs the startup `WARN` when `data_dir` is missing or empty (on a redeploy, the sign that the volume didn't persist); `LogStorageInitError` attaches the UID-65532 `permissionHint` to a NATS or Pebble open failure that looks like a permission denial — the same hint string `CheckDataDir` uses. ### `dedupe/` — Deduplication (Optional) diff --git a/docs/src/content/docs/configuration.mdx b/docs/src/content/docs/configuration.mdx index 43256a31..791f15ba 100644 --- a/docs/src/content/docs/configuration.mdx +++ b/docs/src/content/docs/configuration.mdx @@ -16,7 +16,10 @@ WaveHouse is configured via a YAML file with environment variable overrides. All 1. If a config file exists at the specified path (default: `config.yaml`), it is loaded first. 2. Environment variables override any values from the YAML file. 3. If no config file exists, all values are read from environment variables. Every key has a default except `settings.dir` (`WH_SETTINGS_DIR`), which must be set either way. -4. The YAML file is **strict**: a key this page doesn't list — a typo, or a tunable that has moved to the settings directory (`dlq.enabled`, `clickhouse.addr`, `stream.*`, a leftover `policy:` or `pipes:` block, …) — refuses to boot and names every offending key, so nothing is read, ignored, and believed. +4. Both sources are **strict**. A YAML key this page doesn't list — a typo, or a tunable that has moved to the settings directory (`dlq.enabled`, `clickhouse.addr`, `stream.*`, a leftover `policy:` or `pipes:` block, …) — refuses to boot and names every offending key, so nothing is read, ignored, and believed. A `WH_*` environment variable that binds to no key on this page (`WH_DEDUPE_ENABLED`, `WH_CH_ADDR`, a misspelling) refuses to boot the same way. Two variables have no YAML key and are exempt because they are not config keys at all but process-level settings `main` reads directly: `WH_CONFIG` (below), which locates the file, and `WH_LOG_LEVEL`. Only the `WH_` prefix is checked, since the environment always carries names that aren't WaveHouse's. One outside source does share the prefix. Kubernetes injects `{SERVICE}_SERVICE_HOST`, `{SERVICE}_PORT`, and similar link variables into every pod in a Service's own namespace, for each Service with a cluster IP that existed before the pod started (a headless Service injects nothing, and a Service in another namespace is harmless). The name is uppercased with `-` mapped to `_`, so a Service named `wh` produces `WH_SERVICE_HOST` and `WH_PORT`, one named `wh-foo` produces `WH_FOO_SERVICE_HOST` and `WH_FOO_PORT`, and either way the pod refuses to boot on its next restart. Set `enableServiceLinks: false` on the pod spec, or name the Service something else. The error says so. +5. Before anything dials out, `data_dir` is probed, and boot refuses on any of these: the value is empty; the path exists but is not a directory; the path, or any component above it, is a dangling symlink (a mount that never came up); the directory exists but the process cannot write to it; the directory is absent and its nearest existing ancestor is not writable, so it could not be created. The probe runs before ClickHouse discovery, so the refusal lands at the top of the log, and a permission denial — on the write probe, or on reaching the path at all through a parent without search permission — carries the UID-65532 remediation, since a bind mount owned by root is the typical cause. + +Boot is the validator for this half of configuration: there is no dry run, and a refused boot with the offending key, variable, or path named in the error is the loud signal. The hot-reloadable half has a dry run — `wavehouse validate` — because it is edited under a running server; boot config only ever takes effect through a restart, so the restart is where it is checked. Set `WH_CONFIG` to change the config file path: diff --git a/docs/src/content/docs/deployment.md b/docs/src/content/docs/deployment.md index 45c01c68..1a478570 100644 --- a/docs/src/content/docs/deployment.md +++ b/docs/src/content/docs/deployment.md @@ -186,7 +186,7 @@ On a first-ever run this is expected. On every subsequent run it should be silen ### Distroless Permission Traps (named volume vs bind mount) -WaveHouse images run as the distroless `nonroot` user (UID 65532). Bind mounts and named volumes interact with this differently, and the distroless image has no shell to `chown` things at runtime — so getting the host side wrong produces a hard-to-read permission error from NATS or Pebble at startup. +WaveHouse images run as the distroless `nonroot` user (UID 65532). Bind mounts and named volumes interact with this differently, and the distroless image has no shell to `chown` things at runtime — so getting the host side of `/app/data` wrong refuses boot in the first lines of the log with a named `data_dir` error and the `chown` remediation attached. `/app/settings` fails differently: the server only reads it, so a mount it cannot read, or that does not validate, is a settings-validation error rather than a `data_dir` one. **Named volumes** (the recommended pattern): @@ -206,12 +206,13 @@ volumes: - /srv/wavehouse:/app/data ``` -Bind mounts do **not** copy-up — Docker exposes the host directory as-is, and the image's pre-created dir is masked entirely. If `/srv/wavehouse` is owned by `root:root` on the host (the default for a freshly `mkdir`'d directory), the binary fails at startup with a permission error from NATS: +Bind mounts do **not** copy-up — Docker exposes the host directory as-is, and the image's pre-created dir is masked entirely. The condition boot enforces is that UID 65532 can **write** to the directory — a freshly `mkdir`'d `root:root` directory at the default mode cannot be, which is the common case, though a root-owned directory with permissive mode bits or an ACL passes. If `/srv/wavehouse` is not writable, the binary refuses to start before it touches the settings directory or ClickHouse — `data_dir` is probed for writability right after the config loads: ```text wrap=false -ERROR mq init failed error="..." path=/app/data/nats - hint="if running in a container with a host bind mount, the host - directory must be owned by UID 65532..." +ERROR check data_dir error="data_dir /app/data is not writable; if running in a + container with a host bind mount, the host directory must be writable by + UID 65532 (the `nonroot` user in the distroless image); the usual fix is + `sudo chown -R 65532:65532 /your/host/path`. ...: permission denied" ``` The fix is one host-side command before first start: @@ -305,6 +306,8 @@ readinessProbe: httpGet: { path: /readyz, port: 8080 } ``` +Don't name WaveHouse's own Service `wh` or `wh-*`. kubelet injects `WH_SERVICE_HOST` and friends into every pod in the namespace started after such a Service exists, WaveHouse's own pods included, and the strict environment check refuses them — not at deploy time, but on the next restart. Any Service in the namespace named that way has the same effect; `enableServiceLinks: false` on the pod spec turns the injection off. See [Configuration → Loading Order](/configuration#loading-order). + Until `startupProbe` succeeds, kubelet doesn't run `livenessProbe` or `readinessProbe` against the pod — so a slow or temporarily-unreachable ClickHouse can't restart-loop the pod via the liveness path. Size `failureThreshold` to your expected worst-case CH boot time; the default 30 × 10s = 5min is generous and works for compose-on-NAS-style deployments where CH and WaveHouse can race during a host reboot. ## Behind a reverse proxy @@ -339,10 +342,11 @@ This affects the streaming surface too, and more quietly. SSE gap-fill (`?since= On the worker side the outcome depends on the DLQ. **With the DLQ enabled for the table**, the message is parked on `dlq.{table}` with `X-DLQ-*` headers and is recoverable by hand — but re-ingest each parked envelope's inner `data` object as a fresh `POST /v1/ingest`; republishing the envelope as-is onto `ingest.{table}` fails the same `format` check and simply re-parks it. **With the DLQ switched off for the table, it is permanently lost**: acked and dropped with an `ERROR` log and a `wavehouse_ingest_poison_total` increment carrying `disposition="dropped"`, unrecoverable from either the ingest stream or the DLQ, because a message that can never insert must not redeliver forever. Draining first is cheaper than a manual replay, and it is the only option at all where the DLQ is off. -Two audits belong **before** the drain, because neither announces itself afterwards: +Three audits belong **before** the drain, because none of them announces itself afterwards: - **`Nullable(T) DEFAULT …` columns now store `NULL` where they took their default.** A positional row has one slot per insertable column and no way to say *absent*, so a key the record omits rides as an explicit `null`. `input_format_null_as_default=1` turns that back into the default for a **non-nullable** column, but ClickHouse stores `NULL` on a nullable one whatever the setting says — only an absent key ever took the default. Following this runbook exactly still changes what lands in those columns, silently. See [the ingest note](/ingest-pipeline#the-journey-of-one-event). - **Policy `check` blocks are now validated against the table.** A `check` naming a column the table lacks, one it computes (`MATERIALIZED`/`ALIAS`), or an `EPHEMERAL` one is a per-record `403` on *every* insert by that role. `wavehouse validate` cannot catch it — it never sees the ClickHouse schema — so audit them against their tables first. See [Access control → Insert checks](/access-control#insert-checks). +- **Every `WH_*` variable the binary does not bind refuses boot.** The old binary ignored a variable it did not read; the new one names every unbound one and exits before it opens the queue, so a pod spec or compose file that still carries one comes back from the upgrade as a container that will not start. Diff the environment against the [Configuration Reference](/configuration) first: a `WH_*` variable that is not in its tables is unbound, and whatever it used to configure now lives in the [settings directory](/settings-directory) or is gone. A Kubernetes Service in the pod's namespace named `wh` or `wh-*` counts too: it injects link variables under the `WH_` prefix (`WH_SERVICE_HOST` and `WH_PORT` for `wh`, `WH_FOO_SERVICE_HOST` and `WH_FOO_PORT` for `wh-foo`), so set `enableServiceLinks: false` on the pod spec. To drain before upgrading: diff --git a/docs/src/content/docs/settings-directory.mdx b/docs/src/content/docs/settings-directory.mdx index 0f267958..10e2046e 100644 --- a/docs/src/content/docs/settings-directory.mdx +++ b/docs/src/content/docs/settings-directory.mdx @@ -19,7 +19,7 @@ Where the directory lives is the one boot-config key involved: `settings.dir` / ## Loading and hot reload -The server validates and adopts the directory at boot — a missing or invalid directory refuses to start, so a typo, a missing mount, or an invalid policy surfaces immediately instead of silently denying every request (run `wavehouse validate` to reproduce the findings, `wavehouse bootstrap` to create a starter directory). +The server validates and adopts the directory at boot — a missing or invalid directory refuses to start, so a typo, a missing mount, or an invalid policy surfaces immediately instead of silently denying every request (run `wavehouse validate` to reproduce the findings, `wavehouse bootstrap` to create a starter directory). Boot config has no equivalent command and needs none: it only takes effect through a restart, and the restart refuses on an undeclared YAML key, an unbound `WH_*` variable, or an unusable `data_dir` — see [Configuration — Loading Order](/configuration#loading-order). A running instance re-validates and re-adopts on any of three triggers, all funneling through one serialized reload path: diff --git a/docs/src/integrations/diagram-png.mjs b/docs/src/integrations/diagram-png.mjs index fc041b79..2ea57d04 100644 --- a/docs/src/integrations/diagram-png.mjs +++ b/docs/src/integrations/diagram-png.mjs @@ -109,8 +109,8 @@ export function diagramPng(options = {}) { name: "wh-diagram-png", hooks: { "astro:build:done": async ({ dir, pages, logger }) => { - if (process.env.WH_SKIP_DIAGRAM_PNG === "1") { - logger.info("skipped (WH_SKIP_DIAGRAM_PNG=1)"); + if (process.env.DOCS_SKIP_DIAGRAM_PNG === "1") { + logger.info("skipped (DOCS_SKIP_DIAGRAM_PNG=1)"); return; } try { diff --git a/internal/config/check.go b/internal/config/check.go new file mode 100644 index 00000000..bf779530 --- /dev/null +++ b/internal/config/check.go @@ -0,0 +1,139 @@ +package config + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "reflect" + "sort" + "strings" +) + +// envPrefix is what every WaveHouse environment variable starts with, and the +// only filter that makes an unbound-name check meaningful: the environment +// always carries names that aren't ours. +const envPrefix = "WH_" + +// processEnv lists the WH_* names the binary reads outside the Config struct +// (cmd/wavehouse), so unboundEnv doesn't flag them. +var processEnv = []string{EnvConfig, EnvLogLevel} + +// rejectUnboundEnv is the environment half of rejectUnknownKeys: an error +// naming every WH_* variable in environ that nothing reads. +func rejectUnboundEnv(environ []string) error { + unbound := unboundEnv(environ) + if len(unbound) == 0 { + return nil + } + return fmt.Errorf("unbound environment variable(s): %s — a typo, or a key that moved to the settings directory (%s); unset it, or rename it to a variable the binary reads (a key the Config struct declares, or %s / %s). On Kubernetes, a Service in the pod's namespace named wh or wh-* injects link variables under the WH_ prefix (WH_SERVICE_HOST and WH_PORT for wh, WH_FOO_SERVICE_HOST and WH_FOO_PORT for wh-foo, …) into every pod started after it: set enableServiceLinks: false on the pod spec, or rename the Service", strings.Join(unbound, ", "), EnvSettingsDir, EnvConfig, EnvLogLevel) +} + +// unboundEnv returns, sorted, every WH_* name in environ (os.Environ() form, +// "KEY=value") that no Config field's env tag binds and the binary doesn't +// read otherwise. Such a name is almost always a typo or a key that moved to +// the settings directory — `WH_DEDUPE_ENABLED=true` left in a compose file +// would otherwise be set, ignored, and believed. Only the WH_ prefix is +// checked, since the environment is shared with whatever launched the process. +func unboundEnv(environ []string) []string { + bound := map[string]bool{} + for _, name := range processEnv { + bound[name] = true + } + collectEnvTags(reflect.TypeFor[Config](), bound) + var out []string + for _, kv := range environ { + name, _, _ := strings.Cut(kv, "=") + if strings.HasPrefix(name, envPrefix) && !bound[name] { + out = append(out, name) + } + } + sort.Strings(out) + return out +} + +// collectEnvTags records every name in every env tag in t, recursing into +// nested structs. It mirrors cleanenv's tag grammar: an env tag is a +// comma-separated list of names. cleanenv's `env-prefix` tag is NOT +// mirrored — Config must not use it, or a prefixed variable that cleanenv +// reads would be refused here; TestUnboundEnv_MatchesCleanenv pins that. +func collectEnvTags(t reflect.Type, into map[string]bool) { + for i := range t.NumField() { + f := t.Field(i) + for _, name := range strings.Split(f.Tag.Get("env"), ",") { + if name != "" { + into[name] = true + } + } + if f.Type.Kind() == reflect.Struct { + collectEnvTags(f.Type, into) + } + } +} + +// CheckDataDir reports whether boot can use dir as data_dir: a directory that +// exists must be writable, and one that doesn't (boot creates it — a first +// run, or a missing mount that WarnIfFreshDataDir calls out) must have a +// writable nearest existing ancestor so that creation can succeed. Run before +// anything dials out, so a data_dir the process cannot write to refuses boot +// before ClickHouse discovery rather than after it; a permission denial — +// on the write probe, or on reaching dir at all through a parent without +// search permission — carries the UID-65532 hint, since a bind mount owned +// by root is the typical cause. Writability is probed by creating and +// removing one temp file: the only portable test that exercises the mount's +// ownership and mode. A blank dir — reachable through `WH_DATA_DIR=` — is +// refused outright: the ancestor walk would otherwise probe the working +// directory and pass, and NATS and Pebble state would land under it. +func CheckDataDir(dir string) error { + if strings.TrimSpace(dir) == "" { + return errors.New("data_dir (WH_DATA_DIR) is required: an empty value would scatter NATS and Pebble state under the working directory") + } + info, err := os.Stat(dir) + switch { + case errors.Is(err, fs.ErrNotExist): + // Boot creates it; check the ancestor below. + case errors.Is(err, fs.ErrPermission): + // A component above dir denies search permission (/srv/data at + // root:root 0700 with data_dir under it): Stat fails before the walk + // and the write probe ever run, so the hint has to attach here. + return fmt.Errorf("data_dir %s is not accessible; %s: %w", dir, permissionHint, err) + case err != nil: + return fmt.Errorf("data_dir %s: %w", dir, err) + case !info.IsDir(): + return fmt.Errorf("data_dir %s is not a directory", dir) + } + + // Walk up to the nearest existing entry with Lstat, so a dangling + // symlink — dir itself, or a component above it — stops the walk rather + // than being skipped over as "does not exist": the probe would otherwise + // pass in an unrelated ancestor and NATS would fail on the symlink later. + target := dir + for { + if _, err := os.Lstat(target); err == nil { + break + } + parent := filepath.Dir(target) + if parent == target { + break + } + target = parent + } + if _, err := os.Stat(target); err != nil { + return fmt.Errorf("data_dir %s: %s is a dangling symlink: %w", dir, target, err) + } + f, err := os.CreateTemp(target, ".wavehouse-datadir-probe-*") + if err != nil { + msg := fmt.Sprintf("data_dir %s is not writable", dir) + if target != dir { + msg = fmt.Sprintf("data_dir %s does not exist and %s is not writable, so it cannot be created", dir, target) + } + if errors.Is(err, fs.ErrPermission) { + msg += "; " + permissionHint + } + return fmt.Errorf("%s: %w", msg, err) + } + name := f.Name() + _ = f.Close() + return os.Remove(name) +} diff --git a/internal/config/check_test.go b/internal/config/check_test.go new file mode 100644 index 00000000..df9951a6 --- /dev/null +++ b/internal/config/check_test.go @@ -0,0 +1,189 @@ +package config + +import ( + "os" + "path/filepath" + "reflect" + "regexp" + "testing" + + "github.com/ilyakaznacheev/cleanenv" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestUnboundEnv(t *testing.T) { + t.Parallel() + environ := []string{ + "WH_SERVER_PORT=9090", // struct tag + "WH_OTEL_LOGS_SAMPLE_RATE=0.5", // nested struct tag + "WH_SETTINGS_DIR=./settings", // struct tag + "WH_CONFIG=/etc/wh.yaml", // process-level, read by main + "WH_LOG_LEVEL=debug", // process-level, read by main + "WH_DEDUPE_ENABLED=true", // moved to the settings directory + "WH_SERVER_PROT=1", // typo + "WH_=x", // bare prefix + "PATH=/usr/bin", // not ours + "WHATEVER=1", // prefix is WH_, not WH + "NOT_WH_FOO=1", + } + assert.Equal(t, []string{"WH_", "WH_DEDUPE_ENABLED", "WH_SERVER_PROT"}, unboundEnv(environ)) + assert.Empty(t, unboundEnv(nil)) +} + +// Every variable cleanenv itself reads must count as bound — a name the +// loader honors but this walk misses would refuse every boot that sets it. +// The oracle is cleanenv's own metadata (GetDescription lists each bound +// name), not collectEnvTags, so a divergence in tag grammar (comma lists, +// env-prefix) shows up here rather than in production. +func TestUnboundEnv_MatchesCleanenv(t *testing.T) { + t.Parallel() + desc, err := cleanenv.GetDescription(&Config{}, nil) + require.NoError(t, err) + names := regexp.MustCompile(`(?m)^ (WH_[A-Z0-9_]+) `).FindAllStringSubmatch(desc, -1) + require.NotEmpty(t, names) + var environ []string + seen := map[string]bool{} + for _, m := range names { + environ = append(environ, m[1]+"=1") + seen[m[1]] = true + } + require.True(t, seen[EnvSettingsDir]) + require.True(t, seen["WH_OTEL_TRACES_SAMPLE_RATE"]) + assert.Empty(t, unboundEnv(environ)) +} + +func TestCollectEnvTags_CommaList(t *testing.T) { + t.Parallel() + type multi struct { + A string `env:"WH_ONE,WH_TWO"` + B string `env:""` + } + got := map[string]bool{} + collectEnvTags(reflect.TypeFor[multi](), got) + assert.Equal(t, map[string]bool{"WH_ONE": true, "WH_TWO": true}, got) +} + +func TestLoad_RejectsUnboundEnv(t *testing.T) { + t.Setenv("WH_DEDUPE_ENABLED", "true") + t.Setenv("WH_CH_ADDR", "localhost:9000") + _, err := Load("nonexistent.yaml") + require.Error(t, err) + assert.Contains(t, err.Error(), "unbound environment variable(s): WH_CH_ADDR, WH_DEDUPE_ENABLED") + assert.Contains(t, err.Error(), EnvSettingsDir) +} + +func TestCheckDataDir(t *testing.T) { + t.Parallel() + t.Run("existing writable directory", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + require.NoError(t, CheckDataDir(dir)) + entries, err := os.ReadDir(dir) + require.NoError(t, err) + assert.Empty(t, entries, "the probe file must be removed") + }) + + t.Run("missing directory under a writable parent", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "data", "nested") + require.NoError(t, CheckDataDir(dir)) + _, err := os.Stat(dir) + assert.True(t, os.IsNotExist(err), "the check must not create the directory") + }) + + t.Run("dangling symlink as data_dir", func(t *testing.T) { + t.Parallel() + root := t.TempDir() + link := filepath.Join(root, "data") + require.NoError(t, os.Symlink(filepath.Join(root, "gone"), link)) + err := CheckDataDir(link) + require.Error(t, err) + assert.Contains(t, err.Error(), "dangling symlink") + assert.Contains(t, err.Error(), link) + }) + + t.Run("dangling symlink above data_dir", func(t *testing.T) { + t.Parallel() + root := t.TempDir() + link := filepath.Join(root, "mnt") + require.NoError(t, os.Symlink(filepath.Join(root, "gone"), link)) + err := CheckDataDir(filepath.Join(link, "data")) + require.Error(t, err) + assert.Contains(t, err.Error(), "dangling symlink") + assert.Contains(t, err.Error(), link) + }) + + t.Run("symlink to a real directory passes", func(t *testing.T) { + t.Parallel() + root := t.TempDir() + target := filepath.Join(root, "real") + require.NoError(t, os.Mkdir(target, 0o700)) + link := filepath.Join(root, "data") + require.NoError(t, os.Symlink(target, link)) + require.NoError(t, CheckDataDir(link)) + require.NoError(t, CheckDataDir(filepath.Join(link, "nested"))) + }) + + t.Run("a file is not a directory", func(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), "data") + require.NoError(t, os.WriteFile(path, []byte("x"), 0o600)) + err := CheckDataDir(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a directory") + }) + + t.Run("unwritable directory carries the UID hint", func(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root writes anywhere") + } + dir := t.TempDir() + require.NoError(t, os.Chmod(dir, 0o500)) //nolint:gosec // G302: an unwritable directory is the point of the test + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) //nolint:gosec // G302: restore so TempDir cleanup works + err := CheckDataDir(dir) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not writable") + assert.Contains(t, err.Error(), "65532") + }) + + t.Run("unsearchable parent carries the UID hint", func(t *testing.T) { + // os.Stat fails with EACCES, not ErrNotExist, so this never reaches + // the write probe: the hint must ride on the Stat arm. + if os.Getuid() == 0 { + t.Skip("root searches anywhere") + } + parent := t.TempDir() + require.NoError(t, os.Chmod(parent, 0o600)) + t.Cleanup(func() { _ = os.Chmod(parent, 0o700) }) //nolint:gosec // G302: restore so TempDir cleanup works + err := CheckDataDir(filepath.Join(parent, "data")) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not accessible") + assert.Contains(t, err.Error(), "65532") + }) + + t.Run("missing directory under an unwritable parent", func(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root writes anywhere") + } + parent := t.TempDir() + require.NoError(t, os.Chmod(parent, 0o500)) //nolint:gosec // G302: an unwritable directory is the point of the test + t.Cleanup(func() { _ = os.Chmod(parent, 0o700) }) //nolint:gosec // G302: restore so TempDir cleanup works + err := CheckDataDir(filepath.Join(parent, "data")) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot be created") + assert.Contains(t, err.Error(), parent) + }) +} + +// A blank data_dir must not pass by probing the working directory — an empty +// WH_DATA_DIR reaches Load as "" and would otherwise scatter NATS and Pebble +// state under the cwd. +func TestCheckDataDir_BlankIsRefused(t *testing.T) { + t.Parallel() + for _, dir := range []string{"", " "} { + err := CheckDataDir(dir) + require.Error(t, err, "%q", dir) + assert.Contains(t, err.Error(), "data_dir (WH_DATA_DIR) is required") + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 8d90dbb2..f02bb5b3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -32,6 +32,16 @@ type Config struct { // so a config test pins the tag to this constant to prevent drift. const EnvSettingsDir = "WH_SETTINGS_DIR" +// EnvConfig names the boot-config file (default: ./config.yaml) and +// EnvLogLevel the minimum log level. Neither is a Config field — the first +// locates the file the struct is read from, the second is read by main +// directly — so they are declared here as the process-level names unboundEnv +// must not flag. +const ( + EnvConfig = "WH_CONFIG" + EnvLogLevel = "WH_LOG_LEVEL" +) + // Settings locates the hot-reloadable settings directory — the four JSON // documents (roles.json, policies.json, pipes.json, config.json) validated by // internal/settings. Boot-tier by necessity: it's the pointer the reload @@ -207,11 +217,17 @@ func (c *Config) Validate() error { } // Load reads config from a YAML file (if it exists) with env var overrides. -// The YAML is strict: a key the Config struct doesn't declare is an error -// naming every such key, so a tunable that moved to the settings directory -// (or a typo) can't be silently ignored. Environment variables can't be -// checked the same way — the environment always carries unrelated names. +// Both sources are strict: a YAML key the Config struct doesn't declare, or +// a WH_* environment variable no field binds, is an error naming every +// offender, so a tunable that moved to the settings directory (or a typo) +// can't be silently ignored. The environment always carries unrelated +// names, so only the WH_ prefix is checked there. Boot is the validator for +// this half of configuration — there is no dry run; a refused boot is the +// loud signal. func Load(path string) (*Config, error) { + if err := rejectUnboundEnv(os.Environ()); err != nil { + return nil, err + } var cfg Config if _, err := os.Stat(path); err == nil { if err := cleanenv.ReadConfig(path, &cfg); err != nil { diff --git a/internal/config/persistence.go b/internal/config/persistence.go index f4e7626c..c1640165 100644 --- a/internal/config/persistence.go +++ b/internal/config/persistence.go @@ -7,6 +7,12 @@ import ( "os" ) +// permissionHint is the remediation for the dominant permission-denied +// signature: a Docker bind mount whose host directory is owned by root +// rather than the distroless `nonroot` user. Shared by the boot-time storage +// error and the data_dir probe (CheckDataDir) that runs right after Load. +const permissionHint = "if running in a container with a host bind mount, the host directory must be writable by UID 65532 (the `nonroot` user in the distroless image); the usual fix is `sudo chown -R 65532:65532 /your/host/path`. Named volumes inherit ownership automatically and don't need this." + // LogStorageInitError emits an error log for a storage-init failure, with a // UID-65532 hint when the failure looks like a permission denial — the // dominant signature of a Docker bind mount whose host directory is owned @@ -18,10 +24,7 @@ import ( func LogStorageInitError(logger *slog.Logger, kind, path string, err error) { fields := []any{"error", err, "path", path} if errors.Is(err, os.ErrPermission) { - fields = append(fields, - "hint", - "if running in a container with a host bind mount, the host directory must be owned by UID 65532 (the `nonroot` user in the distroless image). Try `sudo chown -R 65532:65532 /your/host/path`. Named volumes inherit ownership automatically and don't need this.", - ) + fields = append(fields, "hint", permissionHint) } logger.Error(kind+" init failed", fields...) }