From 10bb9208996db0c75b0b2bfd8fc948d3e2351c88 Mon Sep 17 00:00:00 2001 From: Scott Robinson Date: Thu, 30 Jul 2026 19:25:46 +0000 Subject: [PATCH] feat(cli): add healthcheck, foreground supervision, and configurable 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 (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 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. --- .github/workflows/integration.yml | 8 +- README.md | 28 +- crates/app/src/cmd_healthcheck.rs | 404 +++++++++++++++++++++++ crates/app/src/cmd_init.rs | 24 +- crates/app/src/cmd_serve.rs | 39 ++- crates/app/src/cmd_stop.rs | 24 +- crates/app/src/init_helpers.rs | 133 +++++++- crates/app/src/lib.rs | 13 +- crates/server/src/serve.rs | 51 +-- devtools/run-tests | 21 +- docs/manuals/01-architecture-guide.md | 18 +- docs/manuals/05-admin-guide.md | 29 ++ docs/manuals/11-deployment-guide.md | 42 ++- tests/test_cli_container_readiness.py | 444 ++++++++++++++++++++++++++ 14 files changed, 1208 insertions(+), 70 deletions(-) create mode 100644 crates/app/src/cmd_healthcheck.rs create mode 100644 tests/test_cli_container_readiness.py diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 0d3f8676..d4188f61 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -47,7 +47,9 @@ jobs: - name: Start ExtendDB run: | - ./target/release/extenddb serve --config extenddb.toml --foreground & + # --write-pid-file so devtools/run-tests can restart the server with + # 'extenddb stop' when it needs to apply a config change. + ./target/release/extenddb serve --config extenddb.toml --foreground --write-pid-file & for i in $(seq 1 30); do if curl -sk https://127.0.0.1:18443/health | grep -q healthy; then echo "Server ready" @@ -114,7 +116,9 @@ jobs: - name: Start ExtendDB run: | - ./target/release/extenddb serve --config extenddb.toml --foreground & + # --write-pid-file so devtools/run-tests can restart the server with + # 'extenddb stop' when it needs to apply a config change. + ./target/release/extenddb serve --config extenddb.toml --foreground --write-pid-file & for i in $(seq 1 30); do if curl -sk https://127.0.0.1:18443/health | grep -q healthy; then echo "Server ready" diff --git a/README.md b/README.md index c58b29a0..088388f0 100755 --- a/README.md +++ b/README.md @@ -112,7 +112,32 @@ By default `extenddb serve` daemonizes itself, which doesn't play well with cont extenddb serve --config extenddb.toml --foreground ``` -Use this with Docker, Kubernetes, `systemd Type=simple`, runit, s6, or any other supervisor that captures stdout/stderr. `extenddb status` and `extenddb stop` continue to work as in daemon mode, since the PID file is still written. +Use this with Docker, Kubernetes, `systemd Type=simple`, runit, s6, or any other supervisor that captures stdout/stderr. + +In `--foreground` mode extenddb writes no PID file and never creates `run_dir`, so the container can run with a read-only root filesystem. Two consequences: + +- `extenddb status` still works (it probes the port) but reports the PID as unknown. +- `extenddb stop` cannot signal the process — there is no PID file to read. Stop it through your supervisor, which delivers `SIGTERM` and triggers the same graceful shutdown. + +Pass `--write-pid-file` to opt back in when you do want `stop` and `status` to work against a foreground server, for example when running it from a shell rather than under a supervisor. The file goes to the usual `run_dir` path, so neither command needs extra arguments, and `run_dir` has to be writable. + +For a container `HEALTHCHECK`, use the `healthcheck` subcommand rather than `curl`; it needs no shell or extra binaries, so it works on a `distroless`/`scratch` image and accepts the self-signed certificate: + +```bash +extenddb healthcheck --config extenddb.toml # exit 0 healthy, 1 not +extenddb healthcheck --endpoint https://127.0.0.1:18443 # explicit target +``` + +This is a liveness check, which is what a container `HEALTHCHECK` wants: `/health` does not query the storage backend, so it reports healthy even if PostgreSQL becomes unreachable after startup. That is deliberate, since a liveness probe that failed on a database outage would restart every replica at once. A backend that is unreachable at startup does stop the server from listening, so that case is caught. There is no separate readiness endpoint yet. + +To make the generated self-signed certificate valid for the name clients use — an in-cluster service DNS name, for example — pass `--tls-san` to `init` (repeatable): + +```bash +extenddb init --catalog-db extenddb_catalog \ + --tls-san extenddb.default.svc.cluster.local --tls-san extenddb.example.com +``` + +`init` never regenerates an existing certificate, so if one is already present it verifies that it already covers every requested `--tls-san` and fails with an explicit error if it does not, rather than silently dropping the name. ## Monitoring @@ -142,6 +167,7 @@ extenddb serve --config extenddb.toml --foreground # Start in foreground extenddb init --catalog-db NAME # Initialize deployment extenddb stop --config extenddb.toml # Graceful shutdown extenddb status --config extenddb.toml # Check if running +extenddb healthcheck --config extenddb.toml # Probe /health (exit 0 healthy, 1 not) extenddb verify --config extenddb.toml # Validate deployment extenddb migrate --config extenddb.toml # Apply schema migrations extenddb destroy --config extenddb.toml # Tear down deployment diff --git a/crates/app/src/cmd_healthcheck.rs b/crates/app/src/cmd_healthcheck.rs new file mode 100644 index 00000000..48cf0d19 --- /dev/null +++ b/crates/app/src/cmd_healthcheck.rs @@ -0,0 +1,404 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! `extenddb healthcheck` — probe the local `/health` endpoint over HTTPS. +//! +//! Intended for a container `HEALTHCHECK`: it sends an HTTPS `GET /health` over +//! loopback and exits 0 or 1. It needs no shell or `curl`, so it also works on +//! a minimal `distroless`/`scratch` image. +//! +//! This checks liveness: exit 0 means the process is listening, completing TLS, +//! and serving HTTP. That is the signal a container `HEALTHCHECK` or a Kubernetes +//! liveness probe wants, since its job is to restart a wedged process. A liveness +//! probe should deliberately not fail on a backend outage. If it did, a shared +//! database briefly going away would restart every replica at once and make the +//! outage worse. +//! +//! What this does not give you is readiness. `/health` is a static handler that +//! never queries the storage backend, so a replica whose backend has gone away +//! still reports healthy and keeps receiving traffic. A backend that is +//! unreachable at startup does stop the server from listening at all, so that +//! case is caught. Closing the gap properly means adding a separate readiness +//! endpoint backed by a cheap cached round-trip to the storage layer and +//! pointing readiness probes at that, rather than making `/health` query the +//! backend and losing its value as a liveness signal. + +use std::io::{Read, Write}; +use std::net::{TcpStream, ToSocketAddrs}; +use std::sync::Arc; +use std::time::Duration; + +use clap::Args; + +use extenddb_config as config; + +/// Port used when neither `--endpoint` nor the config file supplies one. +const DEFAULT_PORT: u16 = 18443; + +/// Bound on connect, read, and write. `TcpStream::connect` has no timeout of its +/// own, so without this an unreachable address hangs for the OS default of +/// roughly two minutes, far longer than any health-check interval. +const TIMEOUT: Duration = Duration::from_secs(3); + +#[derive(Args)] +pub struct HealthcheckArgs { + /// Path to the config file, used to find the port when --endpoint is not given + #[arg(short, long, default_value = "extenddb.toml")] + config: String, + + /// Override the port to probe (defaults to the port in the config file) + #[arg(short, long)] + port: Option, + + /// Endpoint to probe, e.g. https://127.0.0.1:18443. Overrides --port and + /// the config. + #[arg(long)] + endpoint: Option, +} + +/// Host and port to probe. +#[derive(Debug, PartialEq, Eq)] +struct Target { + host: String, + port: u16, +} + +impl Target { + /// `host:port`, bracketing a bare IPv6 literal so the result parses as an + /// address. + fn authority(&self) -> String { + if self.host.contains(':') { + format!("[{}]:{}", self.host, self.port) + } else { + format!("{}:{}", self.host, self.port) + } + } +} + +/// Run the health check. Returns `Ok(())` when healthy, `Err` otherwise. +pub fn run(args: &HealthcheckArgs) -> anyhow::Result<()> { + let target = resolve_target(args)?; + let status = probe(&target)?; + if (200..300).contains(&status) { + Ok(()) + } else { + anyhow::bail!("/health returned HTTP {status}") + } +} + +/// Decide what to probe: `--endpoint` if given, otherwise the address the +/// configured `bind_addr` implies, on `--port` or the configured port. +fn resolve_target(args: &HealthcheckArgs) -> anyhow::Result { + if let Some(ep) = &args.endpoint { + return parse_endpoint(ep); + } + // A config file that exists but cannot be parsed is worth reporting rather + // than papering over by falling back to defaults. + let (host, config_port) = if std::path::Path::new(&args.config).exists() { + let cfg = config::load(&args.config) + .map_err(|e| anyhow::anyhow!("Failed to load config '{}': {e}", args.config))?; + (probe_host(&cfg.server.bind_addr), cfg.server.port) + } else { + ("127.0.0.1".to_owned(), DEFAULT_PORT) + }; + Ok(Target { + host, + port: args.port.unwrap_or(config_port), + }) +} + +/// The address to probe for a server bound to `bind_addr`. +/// +/// A wildcard bind is not itself connectable, so it maps to the loopback address +/// of the same family. Any other bind address is probed as configured, which +/// matters for an IPv6-only deployment: assuming `127.0.0.1` reports a healthy +/// server bound to `::1` as unhealthy. +fn probe_host(bind_addr: &str) -> String { + match bind_addr.trim() { + "" | "0.0.0.0" => "127.0.0.1".to_owned(), + "::" | "[::]" | "::0" => "::1".to_owned(), + other => other + .trim_start_matches('[') + .trim_end_matches(']') + .to_owned(), + } +} + +/// Parse `[scheme://]host[:port][/path]` into a [`Target`]. +/// +/// The scheme and path are accepted and then ignored. The probe always speaks +/// TLS, because the server has no plaintext mode, and always requests +/// `/health`. +fn parse_endpoint(endpoint: &str) -> anyhow::Result { + let ep = endpoint.trim(); + let rest = ep + .strip_prefix("https://") + .or_else(|| ep.strip_prefix("http://")) + .unwrap_or(ep); + // Keep only the authority, dropping any path or query. + let authority = rest + .split(['/', '?']) + .next() + .unwrap_or("") + .trim_end_matches('.'); + if authority.is_empty() { + anyhow::bail!("--endpoint '{endpoint}' has no host"); + } + + // Bracketed IPv6 literal: [::1] or [::1]:18443. + if let Some(after_bracket) = authority.strip_prefix('[') { + let (host, tail) = after_bracket + .split_once(']') + .ok_or_else(|| anyhow::anyhow!("--endpoint '{endpoint}' has an unclosed '['"))?; + return Ok(Target { + host: host.to_owned(), + port: parse_port(tail, endpoint)?, + }); + } + + // An unbracketed IPv6 literal has more than one colon and cannot carry a + // port, so treat the whole thing as the host. + if authority.matches(':').count() > 1 { + return Ok(Target { + host: authority.to_owned(), + port: DEFAULT_PORT, + }); + } + + match authority.split_once(':') { + Some((host, port)) => Ok(Target { + host: host.to_owned(), + port: parse_port(&format!(":{port}"), endpoint)?, + }), + None => Ok(Target { + host: authority.to_owned(), + port: DEFAULT_PORT, + }), + } +} + +/// Parse a `":"` suffix, falling back to the default port when there is +/// no suffix at all. +fn parse_port(suffix: &str, endpoint: &str) -> anyhow::Result { + match suffix.strip_prefix(':') { + None if suffix.is_empty() => Ok(DEFAULT_PORT), + None => anyhow::bail!("--endpoint '{endpoint}' has trailing garbage after the host"), + Some(p) => p + .parse() + .map_err(|e| anyhow::anyhow!("--endpoint '{endpoint}' has an invalid port '{p}': {e}")), + } +} + +/// Make an HTTPS `GET /health` request. Returns the HTTP status code. +fn probe(target: &Target) -> anyhow::Result { + // rustls 0.23 requires an explicit CryptoProvider. Installing it is + // idempotent, so ignore the error if one is already installed. + let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + + let tls_config = rustls::ClientConfig::builder() + .dangerous() + .with_custom_certificate_verifier(Arc::new(AcceptAnyServerCert)) + .with_no_client_auth(); + + let server_name = rustls::pki_types::ServerName::try_from(target.host.clone()) + .map_err(|e| anyhow::anyhow!("Invalid server name '{}': {e}", target.host))?; + + let mut conn = rustls::ClientConnection::new(Arc::new(tls_config), server_name) + .map_err(|e| anyhow::anyhow!("TLS setup failed: {e}"))?; + + let mut tcp = connect(target)?; + // Do not ignore these: the bounded probe is the whole point of the command. + // A server that completes the TCP connect and then wedges the TLS handshake + // is exactly what a liveness check exists to catch, and without these + // timeouts it would hang instead. + tcp.set_read_timeout(Some(TIMEOUT)) + .map_err(|e| anyhow::anyhow!("Failed to set read timeout: {e}"))?; + tcp.set_write_timeout(Some(TIMEOUT)) + .map_err(|e| anyhow::anyhow!("Failed to set write timeout: {e}"))?; + + let authority = target.authority(); + let request = format!("GET /health HTTP/1.1\r\nHost: {authority}\r\nConnection: close\r\n\r\n"); + + let mut tls = rustls::Stream::new(&mut conn, &mut tcp); + tls.write_all(request.as_bytes()) + .map_err(|e| anyhow::anyhow!("Failed to send request: {e}"))?; + + let mut response = String::new(); + match tls.read_to_string(&mut response) { + Ok(_) => {} + // We asked for `Connection: close`, so the server closing the + // connection after the response is expected. + Err(ref e) if e.kind() == std::io::ErrorKind::UnexpectedEof => {} + Err(e) => return Err(anyhow::anyhow!("Read error: {e}")), + } + + let status_line = response.lines().next().unwrap_or(""); + status_line + .split_whitespace() + .nth(1) + .and_then(|s| s.parse().ok()) + .ok_or_else(|| anyhow::anyhow!("No HTTP status in response")) +} + +/// Connect with a bounded timeout, trying every resolved address. +/// +/// Trying all of them matters for a name like `localhost` that resolves to both +/// `::1` and `127.0.0.1`, since the server may be bound to only one of them. +fn connect(target: &Target) -> anyhow::Result { + let authority = target.authority(); + let addrs: Vec<_> = authority + .to_socket_addrs() + .map_err(|e| anyhow::anyhow!("Cannot resolve {authority}: {e}"))? + .collect(); + if addrs.is_empty() { + anyhow::bail!("Cannot resolve {authority}: no addresses"); + } + let mut last_err = None; + for addr in addrs { + match TcpStream::connect_timeout(&addr, TIMEOUT) { + Ok(stream) => return Ok(stream), + Err(e) => last_err = Some(e), + } + } + Err(anyhow::anyhow!( + "Cannot connect to {authority}: {}", + last_err.expect("non-empty address list yields an error") + )) +} + +/// A rustls verifier that accepts any server certificate. +/// +/// The health check makes no trust decision. All it reports is whether the +/// server answered, and all it reads from the response is the HTTP status line. +/// The default deployment serves a self-signed certificate, so validating it +/// would mean distributing a trust anchor to every probe for no benefit. +/// +/// This applies to `--endpoint` as well as to the loopback default, so a +/// non-loopback endpoint could be answered by an impostor. The worst outcome is +/// a wrong health verdict, but only point `--endpoint` at a server you +/// control. +#[derive(Debug)] +struct AcceptAnyServerCert; + +impl rustls::client::danger::ServerCertVerifier for AcceptAnyServerCert { + fn verify_server_cert( + &self, + _end_entity: &rustls::pki_types::CertificateDer<'_>, + _intermediates: &[rustls::pki_types::CertificateDer<'_>], + _server_name: &rustls::pki_types::ServerName<'_>, + _ocsp_response: &[u8], + _now: rustls::pki_types::UnixTime, + ) -> Result { + Ok(rustls::client::danger::ServerCertVerified::assertion()) + } + + fn verify_tls12_signature( + &self, + _message: &[u8], + _cert: &rustls::pki_types::CertificateDer<'_>, + _dss: &rustls::DigitallySignedStruct, + ) -> Result { + Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) + } + + fn verify_tls13_signature( + &self, + _message: &[u8], + _cert: &rustls::pki_types::CertificateDer<'_>, + _dss: &rustls::DigitallySignedStruct, + ) -> Result { + Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) + } + + fn supported_verify_schemes(&self) -> Vec { + rustls::crypto::aws_lc_rs::default_provider() + .signature_verification_algorithms + .supported_schemes() + } +} + +#[cfg(test)] +mod tests { + use super::{DEFAULT_PORT, Target, parse_endpoint}; + + fn target(host: &str, port: u16) -> Target { + Target { + host: host.to_owned(), + port, + } + } + + #[test] + fn parses_scheme_host_and_port() { + assert_eq!( + parse_endpoint("https://127.0.0.1:18443").unwrap(), + target("127.0.0.1", 18443) + ); + // A plaintext scheme is accepted and ignored, since the probe always + // uses TLS. + assert_eq!( + parse_endpoint("http://127.0.0.1:9000").unwrap(), + target("127.0.0.1", 9000) + ); + assert_eq!( + parse_endpoint(" https://extenddb.svc:8443/ ").unwrap(), + target("extenddb.svc", 8443) + ); + } + + #[test] + fn defaults_the_port_when_absent() { + assert_eq!( + parse_endpoint("https://localhost").unwrap(), + target("localhost", DEFAULT_PORT) + ); + assert_eq!( + parse_endpoint("localhost").unwrap(), + target("localhost", DEFAULT_PORT) + ); + } + + #[test] + fn ignores_a_path() { + assert_eq!( + parse_endpoint("https://127.0.0.1:18443/health").unwrap(), + target("127.0.0.1", 18443) + ); + } + + #[test] + fn parses_ipv6_literals() { + assert_eq!( + parse_endpoint("https://[::1]:18443").unwrap(), + target("::1", 18443) + ); + assert_eq!( + parse_endpoint("https://[::1]").unwrap(), + target("::1", DEFAULT_PORT) + ); + // Unbracketed IPv6 is treated as a bare host on the default port. + assert_eq!(parse_endpoint("::1").unwrap(), target("::1", DEFAULT_PORT)); + } + + #[test] + fn maps_bind_addr_to_a_probe_host() { + use super::probe_host; + // Wildcard binds map to loopback of the same family. + assert_eq!(probe_host("0.0.0.0"), "127.0.0.1"); + assert_eq!(probe_host(""), "127.0.0.1"); + assert_eq!(probe_host("::"), "::1"); + assert_eq!(probe_host("[::]"), "::1"); + // Anything else is probed as configured. + assert_eq!(probe_host("127.0.0.1"), "127.0.0.1"); + assert_eq!(probe_host("::1"), "::1"); + assert_eq!(probe_host("[::1]"), "::1"); + assert_eq!(probe_host("10.0.0.5"), "10.0.0.5"); + } + + #[test] + fn rejects_malformed_endpoints() { + assert!(parse_endpoint("https://").is_err()); + assert!(parse_endpoint("https://127.0.0.1:notaport").is_err()); + assert!(parse_endpoint("https://[::1:18443").is_err()); + } +} diff --git a/crates/app/src/cmd_init.rs b/crates/app/src/cmd_init.rs index 9e5ac10a..6482a0cd 100755 --- a/crates/app/src/cmd_init.rs +++ b/crates/app/src/cmd_init.rs @@ -60,6 +60,12 @@ pub struct InitArgs { #[arg(long)] bind_addr: Option, + /// Additional Subject Alternative Name for the self-signed certificate, + /// repeatable. Added to the default localhost/127.0.0.1/bind-addr list so + /// the cert is also valid for names like an in-cluster service DNS name. + #[arg(long = "tls-san")] + tls_san: Vec, + /// Overwrite existing config file (default: --no-overwrite, exit 255 if exists) #[arg(long, overrides_with = "no_overwrite")] overwrite: bool, @@ -139,6 +145,16 @@ pub async fn run(args: InitArgs) -> anyhow::Result { // Collect CLI args for backend-specific parsing let cli_args: Vec = std::env::args().collect(); + // Extract bind_addr from CLI args + let bind_addr = + extract_arg(&cli_args, "--bind-addr").unwrap_or_else(|| "127.0.0.1".to_string()); + + // Generate the self-signed TLS certificate if it isn't already present, + // covering the bind address plus any --tls-san values so it matches the URLs + // clients use. This runs before any database work so that an unusable + // --tls-san fails before we create users or databases. + generate_tls_cert_if_needed(&bind_addr, &args.tls_san)?; + // Create bootstrapper via registry (no hardcoded match!) let bootstrapper = extenddb_storage::bootstrapper::create_bootstrapper(&args.config, &cli_args) .await @@ -233,14 +249,6 @@ pub async fn run(args: InitArgs) -> anyhow::Result { ); } - // Extract bind_addr from CLI args - let bind_addr = - extract_arg(&cli_args, "--bind-addr").unwrap_or_else(|| "127.0.0.1".to_string()); - - // Generate self-signed TLS certificate if not already present. - // Include the server bind address as a SAN so the cert matches the URL. - generate_tls_cert_if_needed(&bind_addr)?; - // AI-1: Discover rendered docs directory for the config file. let docs_dir = discover_docs_dir(); if let Some(ref d) = docs_dir { diff --git a/crates/app/src/cmd_serve.rs b/crates/app/src/cmd_serve.rs index 714d2db1..9704e8f5 100755 --- a/crates/app/src/cmd_serve.rs +++ b/crates/app/src/cmd_serve.rs @@ -31,6 +31,18 @@ pub struct ServeArgs { /// them. #[arg(long, alias = "no-daemon")] foreground: bool, + + /// Write a PID file in --foreground mode. + /// + /// Foreground mode normally writes none, because the supervisor owns the + /// process and a read-only root filesystem then needs no run directory. + /// Pass this when you want `extenddb status` and `extenddb stop` to work + /// against a foreground server, for example when running it from a shell. + /// It goes to the same `run_dir` path daemon mode uses, so `stop` and + /// `status` find it with no extra arguments. Ignored in daemon mode, which + /// always writes one. + #[arg(long)] + write_pid_file: bool, } /// Bind the listening socket, daemonize, then start the tokio runtime. @@ -111,11 +123,18 @@ pub fn run(args: &ServeArgs, build: BuildInfo) -> anyhow::Result<()> { println!("{banner_line2}"); } - // D-3: Write PID file so `extenddb status` can report the daemon PID. + // D-3: A PID file lets `extenddb status` and `extenddb stop` find the + // process. Daemon mode always writes one. Foreground mode writes one only on + // request, so by default it needs no run directory at all and the container + // can use a read-only root filesystem. let run_dir = config::expand_tilde(&app_config.server.run_dir); - std::fs::create_dir_all(&run_dir) - .map_err(|e| anyhow::anyhow!("Failed to create run directory {run_dir}: {e}"))?; - let pid_file = pid_file_path(&run_dir, port); + let pid_file = if args.foreground && !args.write_pid_file { + None + } else { + std::fs::create_dir_all(&run_dir) + .map_err(|e| anyhow::anyhow!("Failed to create run directory {run_dir}: {e}"))?; + Some(pid_file_path(&run_dir, port)) + }; // P57 Bug 7 fix: Use execute() instead of start() so the parent can // verify the daemon child is healthy before exiting. start() exits the @@ -123,17 +142,19 @@ pub fn run(args: &ServeArgs, build: BuildInfo) -> anyhow::Result<()> { // // When --foreground is set, skip daemonization entirely so the process // can be supervised by Docker, Kubernetes, systemd Type=simple, etc. - // The PID file is still written below by `start_server`, and graceful - // shutdown on SIGINT/SIGTERM still works. + // Graceful shutdown on SIGINT/SIGTERM still works. if !args.foreground { - let daemon = Daemonize::new().pid_file(&pid_file); + let pid_file = pid_file + .as_ref() + .expect("daemon mode always has a PID file path"); + let daemon = Daemonize::new().pid_file(pid_file); match daemon.execute() { daemonize::Outcome::Parent(Ok(_)) => { // Parent process: wait for the PID file to appear (written by // the grandchild after the double-fork), then verify the daemon // is still alive. This catches crashes during early startup // (bad config, missing tables, TLS cert errors). - return verify_daemon_started(&pid_file, &bind_addr); + return verify_daemon_started(pid_file, &bind_addr); } daemonize::Outcome::Parent(Err(e)) => { return Err(anyhow::anyhow!("Failed to daemonize: {e}")); @@ -160,7 +181,7 @@ pub fn run(args: &ServeArgs, build: BuildInfo) -> anyhow::Result<()> { .enable_all() .build()? .block_on(extenddb_server::serve( - ServeParams::new(app_config, std_listener, run_dir, build).with_log_target( + ServeParams::new(app_config, std_listener, pid_file, build).with_log_target( if args.foreground { LogTarget::Stderr } else { diff --git a/crates/app/src/cmd_stop.rs b/crates/app/src/cmd_stop.rs index 9308420f..3cf9f72f 100755 --- a/crates/app/src/cmd_stop.rs +++ b/crates/app/src/cmd_stop.rs @@ -48,12 +48,24 @@ pub fn run(args: &StopArgs) { let pid_str = match std::fs::read_to_string(&pid_file) { Ok(s) => s.trim().to_owned(), Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - eprintln!( - "No extenddb server is running on port {port} (PID file {} not found).\n\ - Start one with: extenddb serve --config {}", - pid_file.display(), - args.config, - ); + // `serve --foreground` writes no PID file, so a missing file does + // not mean nothing is running. Probe the port before saying so. + if std::net::TcpStream::connect(("127.0.0.1", port)).is_ok() { + eprintln!( + "A server is listening on port {port} but wrote no PID file ({}).\n\ + It was most likely started with `extenddb serve --foreground`, which \ + leaves process supervision to the container runtime, systemd, or your \ + shell. Stop it there (or send it SIGTERM directly).", + pid_file.display(), + ); + } else { + eprintln!( + "No extenddb server is running on port {port} (PID file {} not found).\n\ + Start one with: extenddb serve --config {}", + pid_file.display(), + args.config, + ); + } std::process::exit(1); } Err(e) => { diff --git a/crates/app/src/init_helpers.rs b/crates/app/src/init_helpers.rs index 13ad0cce..a936a929 100755 --- a/crates/app/src/init_helpers.rs +++ b/crates/app/src/init_helpers.rs @@ -4,12 +4,34 @@ //! Helpers for `extenddb init`: TLS certificate generation and config file creation. /// Generate a self-signed TLS certificate and key if they don't already exist. -pub fn generate_tls_cert_if_needed(bind_addr: &str) -> anyhow::Result<()> { +/// +/// `extra_sans` are additional Subject Alternative Names, such as an in-cluster +/// service DNS name. They are appended to the default list of `localhost`, +/// `127.0.0.1`, and the bind address so the certificate is valid for the names +/// clients use. +/// +/// An existing certificate is never regenerated, because rotating the key pair +/// under a live deployment would be a surprise. That means `--tls-san` cannot +/// take effect on a later run, so instead of exiting successfully having dropped +/// the requested names, we check that the existing certificate already covers +/// them and fail with an actionable error if it does not. +pub fn generate_tls_cert_if_needed(bind_addr: &str, extra_sans: &[String]) -> anyhow::Result<()> { let tls_dir = extenddb_config::expand_tilde("~/.extenddb/tls"); let cert_path = format!("{tls_dir}/cert.pem"); let key_path = format!("{tls_dir}/key.pem"); + // Requested SANs, trimmed, with blanks and duplicates removed. + let requested = normalize_sans(extra_sans); + + // Validate every requested name before either path uses it, so a name we + // could not verify later is rejected on the first run rather than only on + // the next one. + for san in &requested { + coverage_probe(san)?; + } + if std::path::Path::new(&cert_path).exists() && std::path::Path::new(&key_path).exists() { + ensure_cert_covers_sans(&cert_path, &key_path, &requested)?; println!("--- TLS certificate already exists, skipping generation."); return Ok(()); } @@ -24,6 +46,12 @@ pub fn generate_tls_cert_if_needed(bind_addr: &str) -> anyhow::Result<()> { if bind_addr != "localhost" && bind_addr != "127.0.0.1" && bind_addr != "0.0.0.0" { sans.push(bind_addr.to_owned()); } + // Append the `--tls-san` values the defaults don't already cover. + for san in &requested { + if !sans.iter().any(|s| s.eq_ignore_ascii_case(san)) { + sans.push(san.clone()); + } + } let sans_display = sans.join(", "); let mut params = rcgen::CertificateParams::new(sans) @@ -61,6 +89,109 @@ pub fn generate_tls_cert_if_needed(bind_addr: &str) -> anyhow::Result<()> { Ok(()) } +/// Trim `--tls-san` values, drop blanks, and remove duplicates. +/// +/// DNS names are case-insensitive, so `Foo.example.com` and `foo.example.com` +/// are the same name and only the first spelling given is kept. +fn normalize_sans(sans: &[String]) -> Vec { + let mut out: Vec = Vec::new(); + for san in sans { + let san = san.trim(); + if !san.is_empty() && !out.iter().any(|s: &String| s.eq_ignore_ascii_case(san)) { + out.push(san.to_owned()); + } + } + out +} + +/// Label substituted for the `*` when testing whether a certificate covers a +/// wildcard SAN. Any single label works; this one is chosen to be implausible +/// as a real hostname. +const WILDCARD_PROBE_LABEL: &str = "extenddb-tls-san-probe"; + +/// The server name used to test whether a certificate covers `san`. +/// +/// A wildcard such as `*.svc.cluster.local` is a legitimate certificate entry +/// but not a legitimate server name, so it cannot be verified directly. Verify a +/// synthetic single-label substitution instead: a certificate is valid for +/// `