feat(cli): add healthcheck, foreground supervision, and configurable cert SANs - #230
feat(cli): add healthcheck, foreground supervision, and configurable cert SANs#230robinnsc wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
4b7c4c1 to
7aa3895
Compare
|
Thanks @robinnsc, this is a well-put-together PR and the description matches what the code does. I built the branch and ran everything live against a real PostgreSQL before writing this, so the findings below are observed, not read. What I verifiedAll claims in the description hold on this machine (release build, real Postgres):
One functional issue worth fixing before mergeWildcard SANs break the idempotent entrypoint. The generation path ( Smaller observations (non-blocking)
The security-adjacent choices are right: skipping cert verification in the probe is correct for a self-signed liveness check and is honestly documented, and using rustls' real name verification for SAN coverage (rather than string matching) handles the IP-vs-DNS distinction properly. Sequencing and the containerization planTwo coordination points, neither a fault of this PR:
Net: fix the wildcard SAN check, rebase once #218 lands, and this is good to go from my side. |
…cert SANs Make the binary supervisable and probeable from a container runtime. First of two changes for container readiness; the migration concurrency guard follows separately. - healthcheck: new subcommand that probes /health over HTTPS and exits 0 or 1, so a Docker HEALTHCHECK needs no shell or curl and works on distroless. It reports liveness, which is what a HEALTHCHECK and a Kubernetes livenessProbe want: /health is a static handler that does not query the backend, and a liveness probe that failed on a database outage would restart every replica at once. There is no readiness endpoint yet; adding one backed by a cached storage-layer round-trip is the follow-up. Connect, read, and write are bounded at 3s (TcpStream::connect has no timeout of its own), every resolved address is tried so a name resolving to both ::1 and 127.0.0.1 works, and --endpoint accepts an optional scheme, port, path, and IPv6 literal. The flagless probe derives its host from the configured bind_addr rather than assuming 127.0.0.1, so an IPv6-bound server is not reported unhealthy, and --port mirrors serve's own override. Read and write timeout failures are propagated, since a bounded probe is the whole point of the command. - serve --foreground: write no PID file and skip the run directory by default, so the container can use a read-only root filesystem. Daemon mode is unchanged. With no PID file to read, `stop` now probes the port and reports that a server is listening under foreign supervision rather than claiming nothing is running; `status` already degrades to an unknown PID. - serve --write-pid-file: opt back into the PID file in foreground mode, for shell use and tooling that wants `stop` and `status` to work. It goes to the same run_dir path daemon mode uses, so neither command needs extra arguments, and run_dir then has to be writable. Ignored in daemon mode, which always writes one. devtools/run-tests restarts the server with `stop` to apply a config change, so the integration workflow passes this flag; without it that restart silently did nothing: `stop` failed, `serve` could not bind, and the health check passed against the process that was never replaced. That path no longer suppresses errors either, so a failed restart fails the run instead of reporting success. - init --tls-san <name> (repeatable): append Subject Alternative Names to the generated self-signed certificate so it is valid for the name clients use, such as an in-cluster service DNS name, not just localhost/127.0.0.1/bind-addr. Values are trimmed and de-duplicated case-insensitively. init never regenerates an existing certificate, so a later --tls-san cannot take effect; rather than exit 0 having dropped the name and leave clients to hit a TLS hostname verification failure, it verifies the existing certificate covers every requested SAN and fails with an actionable error otherwise. Certificate generation moved ahead of all database work so a bad SAN fails before any state is created. The coverage check uses rustls's own `verify_server_name`, so it adds no new dependency. A wildcard such as *.svc.cluster.local is a valid certificate entry but not a valid server name, so coverage is tested by substituting a single label; without that, a wildcard accepted on the first run failed on every later one, which is a crash loop for the idempotent entrypoint. Every requested name is validated before generation, so a malformed wildcard fails on the first run rather than the next. - docs: correct the architecture and deployment guides, which claimed the server always daemonizes and that foreground mode still writes a PID file, and replace the container recipe that waited on that PID file with a foreground entrypoint plus a HEALTHCHECK. - tests: add tests/test_cli_container_readiness.py covering SAN generation, dedup/blank handling, the not-covered failure and the already-covered idempotent case, healthcheck up/down/--endpoint plus prompt failure against an unreachable host, and foreground leaving no PID file or run directory while still exiting on SIGTERM. Runs under an isolated $HOME so the suite no longer overwrites the developer's real ~/.extenddb certificate. - devtools/run-tests: exclude the new file from the main pytest suite and run it in the CLI section instead, alongside test_cli_lifecycle.py. Like those tests it starts and stops its own servers and creates its own databases, so it cannot run in parallel against the shared instance the main suite uses. Rebased onto the post-#218 layout: the CLI now lives in crates/app, so cmd_healthcheck joins it there. ServeParams gains pid_file: Option<PathBuf> in place of run_dir, which it only ever used to derive that path, so serve() no longer writes a PID file unconditionally and needs no notion of a run directory.
7aa3895 to
10bb920
Compare
|
Rebased onto main and addressed those callouts:
Will address those doc gap callouts on those various design docs |
3 participants
What
Makes the
extenddbbinary supervisable and probeable from a container runtime. First of two container-readiness PRs; the migration concurrency guard follows in a separate PR.extenddb healthcheck— new subcommand that sends an HTTPSGET /healthand exits 0 or 1, so a DockerHEALTHCHECKneeds no shell orcurland works on adistroless/scratchbase. Reads the port from the config file, or takes--endpoint https://host:port. Connect, read, and write are bounded at 3sTcpStream::connecthas no timeout of its own), every resolved address is tried so a name resolving to both::1and127.0.0.1works, and--endpointaccepts an optional scheme, port, path, and IPv6 literal.serve --foregroundwrites no PID file and skips the run directory entirely, so the container can use a read-only root filesystem. Daemon mode is unchanged. With no PID file to read,stopnow probes the port and reports that a server is listening under foreign supervision instead of claiming nothing is running;statusalready degraded to reporting an unknown PID.serve --write-pid-file— opts back into the PID file in foreground mode, for shell use and for tooling that wantsstopandstatusto work. It goes to the samerun_dirpath daemon mode uses, so neither command needs extra arguments, andrun_dirthen has to be writable. Ignored in daemon mode, which always writes one.init --tls-san <name>(repeatable) — appends Subject Alternative Names to the generated self-signed certificate so it is valid for the name clients actually use, such as an in-cluster service DNS name, rather than onlylocalhost/127.0.0.1/bind-addr. Values are trimmed and de-duplicated case-insensitively.HEALTHCHECK.devtools/run-tests' CLI section instead, alongsidetest_cli_lifecycle.py; like those tests it starts and stops its own servers and creates its own databases, so it cannot run in parallel against the shared instance the main suite uses. The integration workflow now starts the server with--write-pid-file, becauserun-testsrestarts it viaextenddb stopto apply an import/export config change. That restart also no longer suppresses errors — see Notable below.Implementation Decisions
healthcheckis a liveness probe, deliberately./healthis a static handler that does not query the storage backend, so a replica whose database has gone away still reports healthy. That is the right behaviour for aHEALTHCHECKand a KuberneteslivenessProbe: one that failed on a database outage would restart every replica at once and prolong the outage. A backend that is unreachable at startup does stop the server from listening, so that case is caught. There is no readiness endpoint yet, and the follow-up is to add one backed by a cached storage-layer round-trip rather than to make/healthquery the backend and lose its value as a liveness signal. This is stated in the module docs, the admin guide, and the deployment guide so nobody wires it to areadinessProbeexpecting traffic to drain.initfails rather than silently dropping a--tls-san.initnever regenerates an existing certificate, since rotating the key pair under a live deployment would be a surprise. That means a--tls-sanadded on a later run cannot take effect — and the container story generates the certificate into a persistent volume with an idempotent entrypoint that re-runsiniton every start, so this is the common path, not an edge case. Exiting 0 having dropped the name leaves the operator to discover it as a client-side TLS hostname verification failure. Instead, when a certificate already exists we verify it covers every requested SAN and fail with an actionable error otherwise; an already-covered SAN is accepted, so the idempotent entrypoint still works. Certificate generation also moved ahead of all database work so a bad SAN fails before any users or databases are created.Why
Prerequisite binary changes from the containerization design: the server daemonizes by default (so the container runtime sees PID 1 exit), needs a health probe that works without a shell, and fixes its certificate SANs to localhost/bind-addr, which is wrong for any in-cluster service name.
Testing done
New
tests/test_cli_container_readiness.py, against a real PostgreSQL:--tls-sanadds one and multiple SANs to the generated certificate; blanks are skipped and case-insensitive duplicates appear once.initfails, naming the SAN, when an existing certificate does not cover it, and does not rotate the certificate; an already-covered SAN is accepted.healthcheckexits 0 when the server is up, non-zero before start and afterstop, honours--endpointincluding a trailing path, and fails in under 15s against an unreachable host instead of hanging for the OS connect timeout.serve --foregroundleaves no PID file and no run directory, answershealthcheck, is not killed byextenddb stop(which reports the port is listening), and exits on SIGTERM.Plus 5 unit tests for
--endpointparsing (scheme, path, default port, IPv6, malformed input).Also verified manually that the tests fail if the SAN coverage check is removed, so they are not passing by accident, and that the suite no longer touches the real
~/.extenddb/tlsChecklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
Breaking changes
serve --foregroundno longer writes a PID file or createsrun_dir. Onmainit does both, andextenddb stopworks against a foreground server. Anyone relying on that must add--write-pid-file, which restores the previous behaviour exactly.extenddb statusis unaffected apart from reporting the PID as unknown, since it probes the port. Daemon mode is unchanged.The repo's own tooling was such a consumer:
devtools/run-testsrestarts the server withextenddb stop, so the integration workflow passes the new flag.One newly non-silent failure:
init --tls-san Xagainst an existing certificate that does not coverXnow exits non-zero where it previously exited 0 and ignored the flag. Since--tls-sanis new in this PR, no existing invocation can hit it.By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.