diff --git a/CHANGES.md b/CHANGES.md index 9687a10..8bf7070 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,26 @@ release promotes it to a version heading; the `bump-version` skill does that. ## Development +## 1.1.0 -- 2026-08-14 + +### Added + +- Nine system variables in every mock template: `proxy_name`, `mock_name`, + `doppel_version`, `request_id`, `method`, `path`, `host`, `peer_ip`, `real_ip`. +- `real_ip` reads `X-Real-IP`, then the leftmost `X-Forwarded-For`, then the + connection's own address. `peer_ip` is always the connection's. +- `server.external_url` may be a template over those variables, rendered per + request: `http://{{ host }}/` answers each client with its own address. +- `DOPPEL_SENTRY_DSN` provides the Sentry DSN, or overrides `sentry.dsn`: a DSN is + a credential, and the environment is where a deployment keeps one. + +### Changed + +- Variable names in `main.example.yaml` and the documentation are `snake_case`. + Your own names are unaffected; Jinja accepts either. +- A mock that extracts into a system variable's name is named at startup: the + system value wins, so the extraction is read and thrown away. + ## 1.0.0 -- 2026-08-13 ### Added diff --git a/Cargo.lock b/Cargo.lock index d39b0c5..e23639a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -657,7 +657,7 @@ dependencies = [ [[package]] name = "doppel-admin" -version = "1.0.0" +version = "1.1.0" dependencies = [ "async-trait", "axum", @@ -678,7 +678,7 @@ dependencies = [ [[package]] name = "doppel-cli" -version = "1.0.0" +version = "1.1.0" dependencies = [ "anyhow", "clap", @@ -699,7 +699,7 @@ dependencies = [ [[package]] name = "doppel-core" -version = "1.0.0" +version = "1.1.0" dependencies = [ "arc-swap", "async-trait", @@ -722,7 +722,7 @@ dependencies = [ [[package]] name = "doppel-proxy" -version = "1.0.0" +version = "1.1.0" dependencies = [ "axum", "bytes", @@ -741,7 +741,7 @@ dependencies = [ [[package]] name = "doppel-render" -version = "1.0.0" +version = "1.1.0" dependencies = [ "doppel-core", "minijinja", @@ -750,7 +750,7 @@ dependencies = [ [[package]] name = "doppel-store-postgres" -version = "1.0.0" +version = "1.1.0" dependencies = [ "async-trait", "doppel-core", @@ -765,7 +765,7 @@ dependencies = [ [[package]] name = "doppel-telemetry" -version = "1.0.0" +version = "1.1.0" dependencies = [ "doppel-core", "sentry", diff --git a/Cargo.toml b/Cargo.toml index 70cec73..6cd8175 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["crates/*"] resolver = "3" [workspace.package] -version = "1.0.0" +version = "1.1.0" edition = "2024" rust-version = "1.94" license = "Apache-2.0" diff --git a/DOCKERHUB.md b/DOCKERHUB.md index 2381df1..23c5a2b 100644 --- a/DOCKERHUB.md +++ b/DOCKERHUB.md @@ -178,17 +178,29 @@ docker run --rm -p 58080:8080 -p 58081:8081 \ Without it a rewritten redirect names port 8080, which is not published, and the client follows it nowhere. Doppel logs the address it settled on at startup. -## Tokens +## Credentials -`DOPPEL_ADMIN_TOKENS` keeps secrets out of the configuration file that gets -mounted in: +Two of them, and neither has to go into the configuration file that gets mounted +in: ```bash docker run --rm \ -e DOPPEL_ADMIN_TOKENS='{"ci":{"token":"...","group":"admin"}}' \ + -e DOPPEL_SENTRY_DSN='https://key@sentry.example.com/1' \ ... ``` +Both override what the document says. `DOPPEL_SENTRY_DSN` also means a +`sentry` section is not needed at all -- and an empty value counts as unset, so a +variable that failed to interpolate leaves a configured DSN in force rather than +silently turning error reporting off. + +Sentry reporting needs a build with the `sentry` cargo feature, and neither the +published image nor the released binaries carry it -- it is off by default so a +default build does not ship a TLS stack and an HTTP client it never uses. A DSN +given to a build without it warns at startup and is not reported; nothing else +changes. Build your own with `--features sentry` if you need it. + ## Reloading `doppel config reload` talks to a control socket inside the container. Two ways diff --git a/README.md b/README.md index de638da..e408b17 100644 --- a/README.md +++ b/README.md @@ -148,6 +148,9 @@ full list. upstream's redirects. Needed behind a port mapping or an ingress; otherwise `server.host:server.port` is used. +`DOPPEL_ADMIN_TOKENS` and `DOPPEL_SENTRY_DSN` keep the two credentials out of the +configuration file. Both override what the document says. + ## A minimal configuration ```yaml diff --git a/crates/doppel-cli/src/commands/serve.rs b/crates/doppel-cli/src/commands/serve.rs index 1d6d2e8..da98a70 100644 --- a/crates/doppel-cli/src/commands/serve.rs +++ b/crates/doppel-cli/src/commands/serve.rs @@ -45,8 +45,16 @@ pub async fn serve(store: Arc, config: Config) -> Result<(), Cl // // Bound to a name, not `_`: the guard flushes and stops reporting when // dropped, and `let _ = ...` would drop it here. - let _sentry = doppel_telemetry::sentry::init(config.sentry.as_ref()) - .map_err(|err| CliError::Failed(err.to_string()))?; + // + // `DOPPEL_SENTRY_DSN` is read here rather than merged into the document, for + // the reason the admin tokens are not: the revision is a hash of the + // configuration's content, and folding the environment into it would make two + // instances reading one stored document disagree about the revision. + let _sentry = doppel_telemetry::sentry::init( + config.sentry.as_ref(), + doppel_core::config::sentry_dsn_from_env().as_deref(), + ) + .map_err(|err| CliError::Failed(err.to_string()))?; // Checked before anything binds, and a bad value fails startup. A // malformed token variable that was logged and skipped would leave an @@ -262,8 +270,7 @@ pub async fn serve(store: Arc, config: Config) -> Result<(), Cl }); let proxy_task = tokio::spawn(serve_proxy( - ProxyState::new(Arc::clone(&holder)) - .with_external_url(external_url.map(doppel_core::config::ExternalUrl::into_url)), + ProxyState::new(Arc::clone(&holder)).with_external_url(external_url), listener, async { let _ = proxy_rx.await; diff --git a/crates/doppel-cli/tests/logging.rs b/crates/doppel-cli/tests/logging.rs index 0723965..63f0c28 100644 --- a/crates/doppel-cli/tests/logging.rs +++ b/crates/doppel-cli/tests/logging.rs @@ -180,3 +180,71 @@ fn a_dsn_without_the_feature_is_reported_rather_than_ignored() { ); assert!(!both.contains("s3cr3tsentrykey"), "{both}"); } + +/// The DSN from the environment, through the built binary. +/// +/// The unit tests decide precedence; this is the half only a process can answer: +/// that the variable is read at all, that the startup line says which source won, +/// and that the key does not travel any further from the environment than it does +/// from the document. +#[test] +fn a_dsn_from_the_environment_is_read_and_its_source_named() { + let up = upstream(); + // No `sentry` section in the document at all -- the case a deployment that + // provisions credentials through the environment actually has. + let server = Server::start_with_env(up.port, config, &[("DOPPEL_SENTRY_DSN", SENTRY_DSN)]); + server.get("/anything"); + + send_sigterm(server.pid()); + let (_status, stdout, stderr) = + wait_after_signal(server.into_child(), "SIGTERM", SIGNAL_WAIT_DEADLINE); + + let both = format!("{stdout}{stderr}"); + assert!( + !both.contains("s3cr3tsentrykey"), + "the key leaked into the logs: {both}" + ); + // Named the way an operator would grep for it, whether or not this build can + // report: with the feature the line says reporting is enabled, without it that + // nothing will be reported, and both carry the source. + assert!( + both.contains("DOPPEL_SENTRY_DSN"), + "startup must say which source the dsn came from: {both}" + ); + assert!( + both.contains("sentry.invalid"), + "the host survives redaction, which is what makes the line useful: {both}" + ); +} + +/// An empty variable leaves the document's DSN alone. +/// +/// The direction matters: `DOPPEL_SENTRY_DSN=${SENTRY_DSN}` with nothing behind +/// `SENTRY_DSN` is a compose file that means nothing by it, and silently turning +/// error reporting off is the worse reading of that. +#[test] +fn an_empty_variable_does_not_disable_a_configured_dsn() { + let up = upstream(); + let server = Server::start_with_env( + up.port, + |ports, socket, templates| { + config(ports, socket, templates).replace( + "proxies:", + &format!("sentry:\n dsn: \"{SENTRY_DSN}\"\nproxies:"), + ) + }, + &[("DOPPEL_SENTRY_DSN", "")], + ); + server.get("/anything"); + + send_sigterm(server.pid()); + let (_status, stdout, stderr) = + wait_after_signal(server.into_child(), "SIGTERM", SIGNAL_WAIT_DEADLINE); + + let both = format!("{stdout}{stderr}"); + assert!( + both.contains("sentry.dsn"), + "the document's dsn must still be the one in force: {both}" + ); + assert!(!both.contains("s3cr3tsentrykey"), "{both}"); +} diff --git a/crates/doppel-cli/tests/mocks.rs b/crates/doppel-cli/tests/mocks.rs index 11fb261..e3af5d8 100644 --- a/crates/doppel-cli/tests/mocks.rs +++ b/crates/doppel-cli/tests/mocks.rs @@ -86,7 +86,7 @@ fn mock2_renders_from_the_request_body_and_query() { assert_eq!(parsed["description"], "first"); assert_eq!( parsed["items"], 3, - "`resourceItems | length` must count the array" + "`resource_items | length` must count the array" ); } @@ -112,13 +112,13 @@ fn the_literal_42_path_is_mocked_rather_than_proxied() { #[test] fn mock4_binds_a_path_capture_into_the_body_and_headers() { let (_up, server) = server(); - // `mock4` extracts `requestId` from `X-Request-ID` and renders it into a - // response header. Rendering is strict, so that request header is not - // optional -- the reference config says as much where the mock is defined. + // `mock4` extracts `trace_id` from `X-Trace-Id` and renders it into a response + // header. Rendering is strict, so that request header is not optional -- the + // reference config says as much where the mock is defined. let url = format!("http://127.0.0.1:{}/api/v1/resource/7/", server.port()); let response = reqwest::blocking::Client::new() .get(url) - .header("x-request-id", "abc-123") + .header("x-trace-id", "abc-123") .send() .unwrap(); assert_eq!(response.status().as_u16(), 200); @@ -127,11 +127,36 @@ fn mock4_binds_a_path_capture_into_the_body_and_headers() { "7", "the capture must render into the response header too" ); + assert_eq!( + response.headers().get("x-trace-id").unwrap(), + "abc-123", + "the extracted header must render back out" + ); + // A system variable Doppel binds itself, with no extraction anywhere in the + // reference configuration: the id it echoes is the one the client sent. + let echoed = response + .headers() + .get("x-request-id") + .and_then(|value| value.to_str().ok()) + .unwrap_or_default() + .to_owned(); let parsed: serde_json::Value = serde_json::from_str(&response.text().unwrap()).unwrap(); assert_eq!( parsed["id"], "7", - "the resourceId capture must reach the body" + "the resource_id capture must reach the body" + ); + // `proxy_name` and `doppel_version`, rendered from nothing the configuration + // declared. + let served_by = parsed["served_by"].as_str().unwrap_or_default(); + assert!( + served_by.starts_with("proxy1 "), + "the proxy's own name must render, got {served_by:?}" + ); + assert!( + served_by.ends_with(env!("CARGO_PKG_VERSION")), + "the running version must render, got {served_by:?}" ); + assert!(!echoed.is_empty(), "a request id must always be echoed"); } #[test] @@ -146,7 +171,7 @@ fn mock4_without_the_header_it_extracts_fails_loudly() { let parsed: serde_json::Value = serde_json::from_str(&body).unwrap(); assert_eq!(parsed["code"], "TEMPLATE_RENDER_ERROR"); assert!( - parsed["message"].as_str().unwrap().contains("requestId"), + parsed["message"].as_str().unwrap().contains("trace_id"), "the message must name the variable, got {}", parsed["message"] ); @@ -166,7 +191,7 @@ fn mock6_renders_a_template_file() { server.write_template( "proxy1", "put.json.j2", - r#"{"updated": "{{ resourceId }}", "name": "{{ resourceName }}"}"#, + r#"{"updated": "{{ resource_id }}", "name": "{{ resource_name }}"}"#, ); let (status, body) = server.request( "PUT", diff --git a/crates/doppel-cli/tests/proxying.rs b/crates/doppel-cli/tests/proxying.rs index 74a4e92..83b822c 100644 --- a/crates/doppel-cli/tests/proxying.rs +++ b/crates/doppel-cli/tests/proxying.rs @@ -85,3 +85,97 @@ fn a_body_that_names_the_upstream_comes_back_naming_doppel() { "a different host must survive: {body}" ); } + +/// The system variables, rendered by a mock in a running binary. +/// +/// `real_ip` is the one worth driving through a socket rather than a unit test: +/// the chain is `X-Real-IP`, then the leftmost `X-Forwarded-For`, then the peer, +/// and only a real connection has a peer to fall back to. +#[test] +fn a_mock_renders_the_system_variables() { + let up = upstream(); + let server = Server::start_with(up.port, |ports, socket, templates| { + format!( + r#" +server: + host: "127.0.0.1" + port: {proxy} +admin: + host: "127.0.0.1" + port: {admin} + tokens: [] + access: {{}} + upload: + limit: 1Mi +control: + socket: {socket} +templates: + dir: {templates} +proxies: + - name: p1 + type: http + url: "http://127.0.0.1:{upstream}/" + mocks: + - name: who + request: + method: GET + url: ^/who$ + response: + status: 200 + json: '{{"proxy": "{{{{ proxy_name }}}}", "mock": "{{{{ mock_name }}}}", "version": "{{{{ doppel_version }}}}", "real_ip": "{{{{ real_ip }}}}", "peer_ip": "{{{{ peer_ip }}}}", "method": "{{{{ method }}}}", "path": "{{{{ path }}}}", "host": "{{{{ host }}}}", "request_id": "{{{{ request_id }}}}"}}' +"#, + proxy = ports.server, + admin = ports.admin, + upstream = ports.upstream, + socket = socket.display(), + templates = templates.display(), + ) + }); + + // Nothing in front of this, so `real_ip` falls back to the peer -- and the + // peer is loopback, because that is where the test client is. + let (status, body) = server.get("/who"); + assert_eq!(status, 200, "{body}"); + let plain: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!(plain["proxy"], "p1"); + assert_eq!(plain["mock"], "who"); + assert_eq!(plain["version"], env!("CARGO_PKG_VERSION")); + assert_eq!(plain["method"], "GET"); + assert_eq!(plain["path"], "/who"); + assert_eq!(plain["peer_ip"], "127.0.0.1"); + assert_eq!(plain["real_ip"], "127.0.0.1", "with no header, the peer"); + assert!( + !plain["request_id"].as_str().unwrap().is_empty(), + "a request id is always bound, minted when the client sends none" + ); + assert!( + plain["host"].as_str().unwrap().starts_with("127.0.0.1:"), + "host is what the client asked for: {}", + plain["host"] + ); + + // With a proxy in front, `real_ip` is what that proxy says. + let url = format!("http://127.0.0.1:{}/who", server.port()); + let response = reqwest::blocking::Client::new() + .get(&url) + .header("x-real-ip", "203.0.113.7") + .header("x-forwarded-for", "198.51.100.1, 10.0.0.8") + .send() + .unwrap(); + let claimed: serde_json::Value = serde_json::from_str(&response.text().unwrap()).unwrap(); + assert_eq!(claimed["real_ip"], "203.0.113.7", "X-Real-IP comes first"); + assert_eq!( + claimed["peer_ip"], "127.0.0.1", + "and the peer is still the socket's own, which nobody can claim" + ); + + // Without X-Real-IP, the leftmost X-Forwarded-For entry: the original client + // rather than the hop next to us. + let forwarded = reqwest::blocking::Client::new() + .get(&url) + .header("x-forwarded-for", "198.51.100.1, 10.0.0.8") + .send() + .unwrap(); + let chained: serde_json::Value = serde_json::from_str(&forwarded.text().unwrap()).unwrap(); + assert_eq!(chained["real_ip"], "198.51.100.1"); +} diff --git a/crates/doppel-core/src/config/env.rs b/crates/doppel-core/src/config/env.rs index 8be9e2c..7c9da30 100644 --- a/crates/doppel-core/src/config/env.rs +++ b/crates/doppel-core/src/config/env.rs @@ -1,5 +1,5 @@ -//! Configuration supplied by the environment: the admin tokens, and the -//! external url. +//! Configuration supplied by the environment: the admin tokens, the external +//! url, and the Sentry DSN. //! //! A deployment that provisions its secrets through the environment should //! not have to write them into the configuration document to use them. These @@ -36,6 +36,38 @@ pub struct EnvExternalUrlError { pub reason: UrlError, } +/// The variable that provides `sentry.dsn`, or replaces it. +pub const SENTRY_DSN_VAR: &str = "DOPPEL_SENTRY_DSN"; + +/// `DOPPEL_SENTRY_DSN`, or `None` when it is unset or empty. +/// +/// A DSN carries the key that authorises sending events, which is why this +/// exists: it is the one Sentry setting that is a credential, and a deployment +/// that provisions credentials through the environment should not have to write +/// this one into a document the admin API returns and the store keeps. +/// +/// Not validated here. A malformed value fails startup where the client is +/// built, which is the one place that knows what a DSN has to parse as -- and it +/// reports the value with its credential redacted, which this module has no way +/// to do. +/// +/// Empty counts as unset, like the two variables above, and that direction is +/// deliberate: `DOPPEL_SENTRY_DSN=${SENTRY_DSN}` with nothing behind `SENTRY_DSN` +/// leaves a configured DSN in force rather than silently turning reporting off. +/// Turning it off is `dsn: ""` in the document, or no `sentry` section at all. +/// +/// Doppel reads its own name and not the conventional `SENTRY_DSN`. A variable +/// that is in the environment for the service beside this one should not make +/// this one start reporting to it. +#[must_use] +pub fn sentry_dsn_from_env() -> Option { + match std::env::var(SENTRY_DSN_VAR) { + Ok(raw) if raw.trim().is_empty() => None, + Ok(raw) => Some(raw.trim().to_owned()), + Err(_) => None, + } +} + /// `DOPPEL_EXTERNAL_URL`, or `None` when it is unset or empty. /// /// Empty counts as unset, like the token variable: `DOPPEL_EXTERNAL_URL=${HOST}` diff --git a/crates/doppel-core/src/config/mod.rs b/crates/doppel-core/src/config/mod.rs index d49e735..922fa21 100644 --- a/crates/doppel-core/src/config/mod.rs +++ b/crates/doppel-core/src/config/mod.rs @@ -30,7 +30,10 @@ pub use admin::{ UploadConfig, }; pub use duration::{Seconds, SecondsError, TimeoutError, TimeoutSeconds}; -pub use env::{EnvExternalUrlError, EnvTokens, EnvTokensError, external_url_from_env}; +pub use env::{ + EnvExternalUrlError, EnvTokens, EnvTokensError, SENTRY_DSN_VAR, external_url_from_env, + sentry_dsn_from_env, +}; pub use header::{HeaderName, HeaderNameError, HeaderValue, HeaderValueError}; pub use mock::{MockConfig, MockProxyOverride, MockRequest, MockResponse}; pub use name::{MAX_PROXY, Name, NameError, ProxyName}; diff --git a/crates/doppel-core/src/config/server.rs b/crates/doppel-core/src/config/server.rs index 11d9e6e..26cb3ba 100644 --- a/crates/doppel-core/src/config/server.rs +++ b/crates/doppel-core/src/config/server.rs @@ -188,6 +188,11 @@ impl Default for TemplatesConfig { pub struct SentryConfig { /// The Sentry DSN to report to. Empty disables reporting, so a deployment /// can blank it without removing the section. + /// + /// `DOPPEL_SENTRY_DSN` overrides it. A DSN carries the key that authorises + /// sending events, so a deployment that keeps credentials in the environment + /// can leave this field out entirely; an empty variable counts as unset and + /// leaves this value in force. #[schema(examples("https://key@o0.ingest.sentry.io/0"))] pub dsn: String, } diff --git a/crates/doppel-core/src/config/url.rs b/crates/doppel-core/src/config/url.rs index a558d60..1a9d21b 100644 --- a/crates/doppel-core/src/config/url.rs +++ b/crates/doppel-core/src/config/url.rs @@ -137,34 +137,90 @@ impl utoipa::ToSchema for UpstreamUrl {} /// Held to the same rules as an upstream base, and used the same way: a path is /// a prefix, so `https://gw.example.com/doppel/` is a Doppel reached under a /// prefix and rewritten locations keep it. +/// +/// It may also be a template over the system variables, rendered per request -- +/// `http://{{ host }}/` answers each client with the address it asked for, and +/// `https://{{ proxy_name }}.gw.example.com/` gives each proxy its own name +/// behind a wildcard. That is opt-in for a reason: `host` is a claim by the +/// caller, so a deployment that builds a redirect out of it is choosing to. #[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct ExternalUrl(reqwest::Url); +pub enum ExternalUrl { + /// A url, parsed and checked when the configuration was read. + Fixed(reqwest::Url), + /// A template, kept as text: what it renders to depends on the request, so + /// the rules a fixed url is held to are checked on the result instead. A + /// rendered value that does not parse means no rewriting for that request -- + /// not a failed one. + Template(String), +} + +/// Whether a configured value is a template rather than a url. +/// +/// `{{` is the only marker looked for. A path may legally contain a brace, and +/// neither Jinja's statement (`{%`) nor its comment (`{#`) form makes sense in an +/// address, so the test stays on the one form this feature is for. +#[must_use] +pub fn is_template(value: &str) -> bool { + value.contains("{{") +} impl ExternalUrl { /// Check a string and keep it parsed, or say why not. pub fn parse(value: &str) -> Result { - parse_base(value).map(Self) + if is_template(value) { + return Self::parse_template(value); + } + parse_base(value).map(Self::Fixed) + } + + /// A template, checked as far as one can be before it renders. + /// + /// The scheme has to be there literally: everything after it may be + /// substituted, but a value that does not begin `http://` or `https://` + /// cannot become a usable url however it renders, and catching that at + /// startup beats finding out on the first redirect. + fn parse_template(value: &str) -> Result { + let scheme = value.split("://").next().unwrap_or_default(); + if !matches!(scheme, "http" | "https") { + return Err(UrlError::BadScheme(scheme.to_owned())); + } + if value.contains('?') || value.contains('#') { + return Err(UrlError::HasQueryOrFragment); + } + Ok(Self::Template(value.to_owned())) } + /// The url, when this is one rather than a template. #[must_use] - pub fn into_url(self) -> reqwest::Url { - self.0 + pub fn as_url(&self) -> Option<&reqwest::Url> { + match self { + Self::Fixed(url) => Some(url), + Self::Template(_) => None, + } } + /// The template, when this is one. #[must_use] - pub fn as_url(&self) -> &reqwest::Url { - &self.0 + pub fn template(&self) -> Option<&str> { + match self { + Self::Fixed(_) => None, + Self::Template(text) => Some(text), + } } + /// What the operator wrote: a normalised url, or the template verbatim. #[must_use] pub fn as_str(&self) -> &str { - self.0.as_str() + match self { + Self::Fixed(url) => url.as_str(), + Self::Template(text) => text, + } } } impl fmt::Display for ExternalUrl { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.write_str(self.0.as_str()) + f.write_str(self.as_str()) } } @@ -177,10 +233,12 @@ impl FromStr for ExternalUrl { } impl Serialize for ExternalUrl { - /// The parsed form, for the reason `UpstreamUrl` serializes that way: the - /// revision is computed over what this writes. + /// What the operator wrote. A fixed url comes back normalised, for the reason + /// `UpstreamUrl` does -- the revision is computed over this -- and a template + /// comes back verbatim, because there is nothing to normalise and rewriting it + /// would change the revision of a document nobody edited. fn serialize(&self, s: S) -> Result { - s.serialize_str(self.0.as_str()) + s.serialize_str(self.as_str()) } } diff --git a/crates/doppel-core/src/lib.rs b/crates/doppel-core/src/lib.rs index 36b4aa1..81e9bcc 100644 --- a/crates/doppel-core/src/lib.rs +++ b/crates/doppel-core/src/lib.rs @@ -10,6 +10,7 @@ pub mod redact; pub mod reload; pub mod runtime; pub mod store; +pub mod template; pub mod validate; pub use config::{Config, ConfigError}; diff --git a/crates/doppel-core/src/runtime.rs b/crates/doppel-core/src/runtime.rs index 9a4c8bf..0d27acd 100644 --- a/crates/doppel-core/src/runtime.rs +++ b/crates/doppel-core/src/runtime.rs @@ -678,10 +678,12 @@ proxies: let rt = compile_reference(); let proxy1 = rt.proxy_by_name("proxy1").unwrap(); - // mock4 declares `requestId: X-Request-ID`. + // mock4 declares `trace_id: X-Trace-Id`. Deliberately not a system + // variable's name: the reference configuration stopped extracting one + // when Doppel began binding them itself. assert_eq!( mock(proxy1, "mock4").header_vars, - vec![("requestId".to_owned(), "x-request-id".to_owned())] + vec![("trace_id".to_owned(), "x-trace-id".to_owned())] ); } @@ -696,7 +698,7 @@ proxies: crate::config::Selector::parse(".filter").unwrap() ))); assert!(mock(proxy1, "mock2").body_vars.contains(&( - "resourceItems".to_owned(), + "resource_items".to_owned(), crate::config::Selector::parse(".content.items").unwrap() ))); } diff --git a/crates/doppel-core/src/template.rs b/crates/doppel-core/src/template.rs new file mode 100644 index 0000000..9af8924 --- /dev/null +++ b/crates/doppel-core/src/template.rs @@ -0,0 +1,55 @@ +//! The template variable names Doppel binds itself. +//! +//! The list lives here rather than beside the code that binds it, because two +//! places need it and they sit on opposite sides of the dependency direction: +//! `doppel-render` binds these into a context, and validation here has to be able +//! to say when a mock declares one of them. `doppel-core` depends on neither, so +//! the shared fact -- the names -- lives in the crate both can see. + +/// Every name Doppel binds itself, sorted. +/// +/// Reserved: an extraction of the same name is overwritten by the system value, +/// so a template always means what the documentation says it means. Sorted +/// because `startup_advisories` reports them in this order and a test asserts the +/// list against what is actually bound. +pub const RESERVED: &[&str] = &[ + "doppel_version", + "host", + "method", + "mock_name", + "path", + "peer_ip", + "proxy_name", + "real_ip", + "request_id", +]; + +/// Whether a name is one of Doppel's own. +#[must_use] +pub fn is_reserved(name: &str) -> bool { + RESERVED.contains(&name) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_list_is_sorted_and_has_no_duplicates() { + // `startup_advisories` reports in this order, and a sorted list is also + // how a reader checks whether a name is on it. + let mut sorted = RESERVED.to_vec(); + sorted.sort_unstable(); + sorted.dedup(); + assert_eq!(sorted, RESERVED); + } + + #[test] + fn a_name_that_is_not_ours_is_not_reserved() { + assert!(is_reserved("proxy_name")); + assert!(!is_reserved("resource_id")); + // Case matters: a name is reserved as written, and `Proxy_Name` is a + // different variable in Jinja. + assert!(!is_reserved("Proxy_Name")); + } +} diff --git a/crates/doppel-core/src/validate/advisory.rs b/crates/doppel-core/src/validate/advisory.rs index ae8999d..0103384 100644 --- a/crates/doppel-core/src/validate/advisory.rs +++ b/crates/doppel-core/src/validate/advisory.rs @@ -89,6 +89,41 @@ pub fn startup_advisories(config: &Config) -> Vec { } } + // A mock that extracts something Doppel already binds. The extraction still + // happens -- a header read, a selector walk -- and its result is then + // overwritten by the system value, so the work is wasted and the template does + // not mean what its author thinks. + for proxy in &config.proxies { + for mock in &proxy.mocks { + let declared = mock + .request + .headers + .keys() + .chain(mock.request.query.keys()) + .chain(mock.request.body.keys()); + let mut shadowed: Vec<&str> = declared + .map(String::as_str) + .filter(|name| crate::template::is_reserved(name)) + .collect(); + shadowed.sort_unstable(); + shadowed.dedup(); + if !shadowed.is_empty() { + out.push(format!( + "mock `{}` of proxy `{}` extracts {} into a name Doppel binds \ + itself; the system value wins, so the extraction is read and \ + thrown away. See the system variables in the documentation", + mock.name, + proxy.name, + shadowed + .iter() + .map(|name| format!("`{name}`")) + .collect::>() + .join(", ") + )); + } + } + } + for proxy in &config.proxies { if proxy.url.has_credentials() { out.push(format!( @@ -235,6 +270,46 @@ proxies: ); } + /// A mock that extracts into a name Doppel binds itself. The extraction is + /// read and then overwritten, so the work is wasted and the template does not + /// mean what its author thinks. + #[test] + fn a_mock_that_shadows_a_system_variable_is_named() { + let text = raw(8080, 8081).replace( + " url: \"https://example.com/\"", + " url: \"https://example.com/\"\n mocks:\n - name: m1\n \ + request:\n method: GET\n url: ^/x$\n headers:\n \ + proxy_name: X-Whatever\n trace_id: X-Trace-Id\n response:\n \ + status: 200\n body: \"ok\"", + ); + let notes = startup_advisories(&load_from_str(&text).unwrap()); + + let note = notes + .iter() + .find(|note| note.contains("binds itself")) + .unwrap_or_else(|| panic!("{notes:?}")); + assert!(note.contains("`m1`"), "{note}"); + assert!(note.contains("`proxy_name`"), "{note}"); + // And only the reserved one: `trace_id` is the operator's own name and + // nothing is wrong with it. + assert!(!note.contains("trace_id"), "{note}"); + } + + #[test] + fn a_mock_that_extracts_its_own_names_says_nothing() { + let text = raw(8080, 8081).replace( + " url: \"https://example.com/\"", + " url: \"https://example.com/\"\n mocks:\n - name: m1\n \ + request:\n method: GET\n url: ^/x$\n headers:\n \ + trace_id: X-Trace-Id\n response:\n status: 200\n body: \"ok\"", + ); + let notes = startup_advisories(&load_from_str(&text).unwrap()); + assert!( + !notes.iter().any(|note| note.contains("binds itself")), + "{notes:?}" + ); + } + #[test] fn a_privileged_port_is_still_a_legal_configuration() { // The point of an advisory rather than a rule: running on port 80 diff --git a/crates/doppel-core/src/validate/mock.rs b/crates/doppel-core/src/validate/mock.rs index feb4088..73c6416 100644 --- a/crates/doppel-core/src/validate/mock.rs +++ b/crates/doppel-core/src/validate/mock.rs @@ -131,7 +131,7 @@ proxies: method: GET url: /api/(?P\d+)/ headers: - requestId: X-Request-ID + request_id: X-Request-ID query: filter: .filter body: @@ -216,7 +216,7 @@ proxies: #[test] fn v19_capture_groups_must_not_collide_with_declared_variables() { let text = good().replace( - " requestId: X-Request-ID", + " request_id: X-Request-ID", " id: X-Request-ID", ); assert_violation( @@ -310,7 +310,7 @@ proxies: #[test] fn a_declared_header_source_that_is_not_a_header_name_fails_at_load() { // This was V24, now the value type of the `headers` map. - let text = good().replace("requestId: X-Request-ID", "requestId: \"X Request ID\""); + let text = good().replace("request_id: X-Request-ID", "request_id: \"X Request ID\""); let err = load_from_str(&text).unwrap_err().to_string(); assert!(err.contains("not a valid header name"), "{err}"); } diff --git a/crates/doppel-proxy/src/rewrite.rs b/crates/doppel-proxy/src/rewrite.rs index 4980712..65de4d0 100644 --- a/crates/doppel-proxy/src/rewrite.rs +++ b/crates/doppel-proxy/src/rewrite.rs @@ -26,6 +26,8 @@ use axum::body::Body; use axum::http::{HeaderMap, HeaderValue, header}; use bytes::Bytes; +use doppel_core::config::ExternalUrl; +use doppel_render::{Renderer, SystemVars}; use futures_util::{StreamExt, TryStreamExt}; /// The content types worth searching for a URL. @@ -45,6 +47,41 @@ const TEXTUAL: &[&str] = &[ "application/ld+json", ]; +/// The address to rewrite to for this request. +/// +/// A fixed url is itself. A template is rendered against the system variables +/// and parsed, which is where `http://{{ host }}/` becomes the address this +/// client asked for. +/// +/// A template that renders to something unusable means no rewriting for this +/// request rather than a failed one: the rewrite is there to keep a client from +/// wandering off, and answering `500` because a `Location` could not be improved +/// is a worse outcome than relaying the upstream's own. Logged at debug, because +/// it is per request and a broken template would otherwise print once per +/// redirect for as long as it takes somebody to notice. +#[must_use] +pub fn resolve_external( + configured: Option<&ExternalUrl>, + system: &SystemVars, +) -> Option { + match configured? { + ExternalUrl::Fixed(url) => Some(url.clone()), + ExternalUrl::Template(template) => { + let rendered = Renderer::new() + .render_str(template, &system.as_variables()) + .inspect_err(|err| { + tracing::debug!(template, %err, "external_url did not render"); + }) + .ok()?; + reqwest::Url::parse(&rendered) + .inspect_err(|err| { + tracing::debug!(template, rendered, %err, "external_url is not a url"); + }) + .ok() + } + } +} + /// Whether a body with these headers is one to rewrite. #[must_use] pub fn is_rewritable(headers: &HeaderMap) -> bool { diff --git a/crates/doppel-proxy/src/server.rs b/crates/doppel-proxy/src/server.rs index 4805dfb..6317630 100644 --- a/crates/doppel-proxy/src/server.rs +++ b/crates/doppel-proxy/src/server.rs @@ -22,14 +22,16 @@ pub struct ProxyState { pub holder: Arc, pub sampler: Arc, /// Where clients reach this Doppel, from `server.external_url` or - /// `DOPPEL_EXTERNAL_URL`, when the deployment says. + /// `DOPPEL_EXTERNAL_URL`, when the deployment says. A url or a template over + /// the system variables -- `crate::rewrite::resolve_external` decides which, + /// per request, because a template's answer depends on the request. /// /// Resolved once at startup rather than read from the running configuration, /// like the rest of `server`: the listeners are bound before the first /// reload, and a reload that changes that section reports it as unapplied. /// Behind an `Arc` because this struct is cloned per request and a `Url` is /// a `String` behind the scenes. - pub external_url: Option>, + pub external_url: Option>, } impl ProxyState { @@ -44,7 +46,7 @@ impl ProxyState { /// The same state, told where clients reach this Doppel. #[must_use] - pub fn with_external_url(mut self, external: Option) -> Self { + pub fn with_external_url(mut self, external: Option) -> Self { self.external_url = external.map(Arc::new); self } @@ -90,6 +92,14 @@ async fn handle( let method = request.method().clone(); let path = request.uri().path().to_owned(); + // Everything Doppel itself contributes to a template, built once here: a + // mock's response renders against it, and so does `server.external_url` when + // that is a template -- which is why it is built before the proxy resolves + // rather than inside the mock branch. `proxy_name` and `mock_name` are filled + // in as they become known; a request that resolves to nothing renders nothing + // and keeps the empty ones. + let mut system = system_vars(&request, &request_id, &method, &path, peer); + let proxy = match resolve(&runtime, request.headers()) { Ok(proxy) => proxy, Err(err) => { @@ -135,6 +145,8 @@ async fn handle( } }; + system.proxy_name = proxy.name.clone(); + // Mock matching comes before fault injection, so `replace` means what it // says: the share of matching requests a mock answers. Deciding the faults // first made it the share of whatever survived the loss roll instead -- @@ -227,8 +239,16 @@ async fn handle( // Rendered first, then padded: the delay is a target for the whole // response, so whatever producing it cost comes out of the wait. let rendering = std::time::Instant::now(); - let outcome = - serve_mock(&runtime.config.templates.dir, proxy, mock, vars, request).await; + system.mock_name = mock.name.clone(); + let outcome = serve_mock( + &runtime.config.templates.dir, + proxy, + mock, + vars, + &system, + request, + ) + .await; let (response, error_code) = match outcome { Ok(response) => (response, None), Err(err) => (error_response(&err), Some(err.code.as_str())), @@ -329,7 +349,7 @@ async fn handle( Some(peer.ip()), &runtime.resolve_headers, &request_id, - state.external_url.as_deref(), + crate::rewrite::resolve_external(state.external_url.as_deref(), &system).as_ref(), ) .await; let latency_ms = pad_to_target(faults.latency, attempt.elapsed(), &proxy.name).await; @@ -490,6 +510,52 @@ pub fn request_id(headers: &HeaderMap) -> String { .unwrap_or_else(|| format!("{:032x}", rand::random::())) } +/// The system variables for this request. +/// +/// `real_ip` is a claim and `peer_ip` is not, which is why both are here: a mock +/// that reports who called it wants the first, and one that decides anything +/// wants to know it is reading the second. `X-Real-IP` is read the way a proxy in +/// front writes it -- see `SystemVars::resolve_real_ip` for the order. +fn system_vars( + request: &Request, + request_id: &str, + method: &axum::http::Method, + path: &str, + peer: SocketAddr, +) -> doppel_render::SystemVars { + let headers = request.headers(); + let forwarded_for: Vec<&str> = headers + .get_all("x-forwarded-for") + .iter() + .filter_map(|value| value.to_str().ok()) + .collect(); + let real_ip = doppel_render::SystemVars::resolve_real_ip( + headers + .get("x-real-ip") + .and_then(|value| value.to_str().ok()), + &forwarded_for, + Some(peer.ip()), + ); + + doppel_render::SystemVars { + // Filled in by the caller as they become known: a request that resolves + // to no proxy still renders an error, and an empty name is the honest + // answer there. + proxy_name: String::new(), + mock_name: String::new(), + request_id: request_id.to_owned(), + method: method.as_str().to_owned(), + path: path.to_owned(), + host: headers + .get(axum::http::header::HOST) + .and_then(|value| value.to_str().ok()) + .unwrap_or_default() + .to_owned(), + peer_ip: peer.ip().to_string(), + real_ip, + } +} + /// Set `X-Request-ID` on every response the client receives, not just a /// successfully forwarded one -- otherwise the id logged for a rejected or /// dropped request would never reach the client that could quote it back. @@ -514,6 +580,7 @@ async fn serve_mock( proxy: &CompiledProxy, mock: &CompiledMock, mut vars: Variables, + system: &doppel_render::SystemVars, request: Request, ) -> Result { crate::mock::bind_headers(mock, request.headers(), &mut vars); @@ -539,6 +606,12 @@ async fn serve_mock( crate::mock::bind_body(mock, &root, &mut vars); } + // Last, so a mock that extracts something called `proxy_name` finds the + // system value in its template rather than its own. `startup_advisories` + // names any mock that does this, because the extraction still costs a header + // read while its result goes unused. + system.bind(&mut vars); + let renderer = Renderer::new(); let body: Vec = match &mock.body { MockBody::None => Vec::new(), @@ -2002,11 +2075,11 @@ proxies: - name: m1 request: method: GET - url: /widgets/(?P[0-9]+)/ + url: /widgets/(?P[0-9]+)/ response: status: 200 headers: - X-Resource-ID: "{{ resourceId }}" + X-Resource-ID: "{{ resource_id }}" "#; let response = send(state(&config_with(extra), vec![0.0]), get("/widgets/42/")).await; assert_eq!(response.status(), StatusCode::OK); diff --git a/crates/doppel-proxy/src/upstream.rs b/crates/doppel-proxy/src/upstream.rs index 10f0336..fb97cd1 100644 --- a/crates/doppel-proxy/src/upstream.rs +++ b/crates/doppel-proxy/src/upstream.rs @@ -1581,4 +1581,76 @@ mod tests { "caller-chosen-id" ); } + /// `server.external_url` as a template: the address a client asked for. + /// + /// This is the case the templating exists for. A deployment behind an ingress + /// that serves several hostnames cannot name one address in the configuration, + /// and `{{ host }}` answers each client with its own -- at the price of + /// building a redirect out of a header the caller controls, which is why it is + /// opt-in and says so in the documentation. + #[tokio::test] + async fn an_external_url_template_renders_the_host_the_client_asked_for() { + let base = upstream().await; + let client = reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .build() + .unwrap(); + + let system = doppel_render::SystemVars { + host: "doppel.internal:9000".to_owned(), + proxy_name: "p1".to_owned(), + ..doppel_render::SystemVars::default() + }; + let configured = doppel_core::config::ExternalUrl::parse("http://{{ host }}/").unwrap(); + let external = crate::rewrite::resolve_external(Some(&configured), &system) + .expect("a template over a known host renders"); + + let (response, _) = forward( + &client, + &proxy(&base), + request(Method::GET, "/redirect/self"), + None, + &[], + TEST_REQUEST_ID, + Some(&external), + ) + .await + .unwrap(); + + assert_eq!( + location_of(response).await, + "http://doppel.internal:9000/moved?keep=1#frag" + ); + } + + /// A template that cannot render, or renders to something that is not a url, + /// means no rewriting -- not a failed request. + #[tokio::test] + async fn a_template_that_does_not_render_leaves_the_response_alone() { + let system = doppel_render::SystemVars::default(); + + // `host` is empty when the client sent none, so this renders to + // `http:///`, which is not a url. + let empty_host = doppel_core::config::ExternalUrl::parse("http://{{ host }}/").unwrap(); + assert!(crate::rewrite::resolve_external(Some(&empty_host), &system).is_none()); + + // And a name no system variable has: strict rendering refuses it rather + // than substituting nothing. + let unknown = doppel_core::config::ExternalUrl::parse("http://{{ nowhere }}/").unwrap(); + assert!(crate::rewrite::resolve_external(Some(&unknown), &system).is_none()); + } + + /// A fixed url is still a fixed url, and needs no renderer at all. + #[tokio::test] + async fn a_fixed_external_url_resolves_to_itself() { + let configured = + doppel_core::config::ExternalUrl::parse("https://doppel.example.com/").unwrap(); + let resolved = crate::rewrite::resolve_external( + Some(&configured), + &doppel_render::SystemVars::default(), + ) + .expect("a fixed url resolves"); + + assert_eq!(resolved.as_str(), "https://doppel.example.com/"); + } } diff --git a/crates/doppel-render/src/extract.rs b/crates/doppel-render/src/extract.rs index 7f32667..3f970d9 100644 --- a/crates/doppel-render/src/extract.rs +++ b/crates/doppel-render/src/extract.rs @@ -20,6 +20,17 @@ impl Variables { self.0.insert(name.to_owned(), value); } + /// One bound value, for a caller that needs to read back what it bound. + #[must_use] + pub fn get(&self, name: &str) -> Option<&serde_json::Value> { + self.0.get(name) + } + + /// Every name bound, in order. `BTreeMap`, so the order is the names'. + pub fn names(&self) -> impl Iterator { + self.0.keys().map(String::as_str) + } + /// A minijinja context built from the bound variables, for the renderer /// to pass to `Environment::render_str`/`render`. #[must_use] diff --git a/crates/doppel-render/src/lib.rs b/crates/doppel-render/src/lib.rs index e2215c2..d03698e 100644 --- a/crates/doppel-render/src/lib.rs +++ b/crates/doppel-render/src/lib.rs @@ -7,6 +7,8 @@ pub mod extract; pub mod render; +pub mod system; pub use extract::{Variables, parse_body}; pub use render::Renderer; +pub use system::{RESERVED, SystemVars, VERSION}; diff --git a/crates/doppel-render/src/render.rs b/crates/doppel-render/src/render.rs index 539f902..d334c2b 100644 --- a/crates/doppel-render/src/render.rs +++ b/crates/doppel-render/src/render.rs @@ -264,12 +264,12 @@ mod tests { let vars = Variables::new(); let err = renderer - .render_str("{{ resourceItems | length }}", &vars) + .render_str("{{ resource_items | length }}", &vars) .unwrap_err(); assert_eq!(err.code, ErrorCode::TemplateRenderError); assert!( - err.message.contains("resourceItems"), + err.message.contains("resource_items"), "message did not identify the undefined variable behind the filter: {}", err.message ); diff --git a/crates/doppel-render/src/system.rs b/crates/doppel-render/src/system.rs new file mode 100644 index 0000000..164b818 --- /dev/null +++ b/crates/doppel-render/src/system.rs @@ -0,0 +1,216 @@ +//! The variables Doppel puts in every template context itself. +//! +//! Everything else a template can see is the operator's own: a named capture in +//! a mock's path pattern, a `headers`, `query` or `body` entry. Those describe +//! one mock. These describe the request and the process, are the same in every +//! template, and are worth having without declaring them nine times. +//! +//! **They are reserved.** They are bound *after* the operator's extractions, so +//! a mock that declares `proxy_name` finds the system value in its template +//! rather than its own -- the alternative is a template whose meaning depends on +//! which mock rendered it. `startup_advisories` names any mock that shadows one, +//! because the extraction still happens and still costs a header read while its +//! result goes unused. +//! +//! Named in `snake_case`, which is also the convention this project's own +//! configuration follows for extracted names. Jinja has no trouble with either, +//! but two conventions in one context read as two sources. + +use std::net::IpAddr; + +use crate::extract::Variables; + +/// The version this binary reports, from the workspace's one version number. +pub const VERSION: &str = env!("CARGO_PKG_VERSION"); + +/// Every name Doppel binds itself, from `doppel-core` -- validation needs the +/// same list and sits on the other side of the dependency direction. +pub use doppel_core::template::RESERVED; + +/// What Doppel knows about one request, independent of any mock. +/// +/// Built once per request and used twice: to render a mock's response, and to +/// render `server.external_url` when that is a template. The second is why this +/// lives here rather than beside the mock code -- a redirect is rewritten for a +/// forwarded request too, where there is no mock at all. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct SystemVars { + /// The proxy that resolved, or empty when none did. + pub proxy_name: String, + /// The mock answering, or empty when the request is being forwarded. + pub mock_name: String, + /// The id echoed in `X-Request-ID`, minted when the client sent none. + pub request_id: String, + pub method: String, + /// The request path, without the query string. + pub path: String, + /// The `Host` the client asked for, or empty when it sent none. + /// + /// A claim by the caller, not a fact about this process. It is here because + /// a template that reports what it was asked for needs it, and it is called + /// `host` rather than something reassuring so nobody forgets which it is. + pub host: String, + /// The address the connection came from: the socket's own, and the only one + /// of the two nobody can fake. + pub peer_ip: String, + /// Who the request is *said* to be from: `X-Real-IP`, else the first entry + /// of `X-Forwarded-For`, else `peer_ip`. + pub real_ip: String, +} + +impl SystemVars { + /// `real_ip`, by the order a proxy chain writes it. + /// + /// `X-Real-IP` first because a single proxy in front sets exactly that and + /// nothing else; then the leftmost `X-Forwarded-For`, which is the original + /// client in a chain that appends; then the peer, so the variable always has + /// a value and a template never has to write `| default(...)`. + /// + /// Every field line of `X-Forwarded-For` is considered, not only the first: + /// a chain split across several lines is legal and some proxies emit it, and + /// taking `get()` alone would read the second hop as the client. + #[must_use] + pub fn resolve_real_ip( + real_ip_header: Option<&str>, + forwarded_for: &[&str], + peer: Option, + ) -> String { + if let Some(value) = real_ip_header.map(str::trim).filter(|v| !v.is_empty()) { + return value.to_owned(); + } + for line in forwarded_for { + if let Some(first) = line + .split(',') + .map(str::trim) + .find(|entry| !entry.is_empty()) + { + return first.to_owned(); + } + } + peer.map(|address| address.to_string()).unwrap_or_default() + } + + /// Bind these into `vars`, overwriting anything of the same name. + /// + /// Last, deliberately: see the module comment. + pub fn bind(&self, vars: &mut Variables) { + let pairs: [(&str, &str); 9] = [ + ("proxy_name", &self.proxy_name), + ("mock_name", &self.mock_name), + ("request_id", &self.request_id), + ("method", &self.method), + ("path", &self.path), + ("host", &self.host), + ("peer_ip", &self.peer_ip), + ("real_ip", &self.real_ip), + ("doppel_version", VERSION), + ]; + for (name, value) in pairs { + vars.insert(name, serde_json::Value::String(value.to_owned())); + } + } + + /// These alone, for rendering something that has no mock behind it. + #[must_use] + pub fn as_variables(&self) -> Variables { + let mut vars = Variables::new(); + self.bind(&mut vars); + vars + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn every_reserved_name_is_bound_and_every_bound_name_is_reserved() { + // The two lists are written out separately -- one for validation to read, + // one for the binding -- so this is what keeps them the same list. + let bound = SystemVars::default().as_variables(); + let mut names: Vec = bound.names().map(ToOwned::to_owned).collect(); + names.sort(); + assert_eq!( + names, RESERVED, + "the reserved list and the binding disagree" + ); + } + + #[test] + fn the_version_is_the_one_this_binary_reports() { + let vars = SystemVars::default().as_variables(); + assert_eq!( + vars.get("doppel_version").and_then(|v| v.as_str()), + Some(VERSION) + ); + assert!(VERSION.starts_with(char::is_numeric), "{VERSION}"); + } + + #[test] + fn real_ip_prefers_the_header_a_single_proxy_sets() { + let peer: IpAddr = "10.0.0.9".parse().unwrap(); + assert_eq!( + SystemVars::resolve_real_ip(Some("203.0.113.7"), &["198.51.100.1"], Some(peer)), + "203.0.113.7" + ); + } + + #[test] + fn then_the_leftmost_forwarded_for_entry() { + // The original client in a chain that appends, not the hop next to us. + let peer: IpAddr = "10.0.0.9".parse().unwrap(); + assert_eq!( + SystemVars::resolve_real_ip(None, &["198.51.100.1, 10.0.0.8"], Some(peer)), + "198.51.100.1" + ); + // A chain split across field lines, which is legal and does happen. + assert_eq!( + SystemVars::resolve_real_ip(None, &["198.51.100.1", "10.0.0.8"], Some(peer)), + "198.51.100.1" + ); + } + + #[test] + fn then_the_peer_which_nobody_can_fake() { + let peer: IpAddr = "10.0.0.9".parse().unwrap(); + assert_eq!( + SystemVars::resolve_real_ip(None, &[], Some(peer)), + "10.0.0.9" + ); + // Empty rather than absent, so a template never needs `| default`. + assert_eq!(SystemVars::resolve_real_ip(None, &[], None), ""); + } + + #[test] + fn an_empty_or_blank_header_is_not_an_answer() { + let peer: IpAddr = "10.0.0.9".parse().unwrap(); + assert_eq!( + SystemVars::resolve_real_ip(Some(" "), &[], Some(peer)), + "10.0.0.9" + ); + assert_eq!( + SystemVars::resolve_real_ip(Some(""), &[" , "], Some(peer)), + "10.0.0.9" + ); + } + + #[test] + fn a_system_name_wins_over_an_extraction_of_the_same_name() { + let mut vars = Variables::new(); + vars.insert( + "proxy_name", + serde_json::Value::String("from-a-header".into()), + ); + SystemVars { + proxy_name: "alpha".to_owned(), + ..SystemVars::default() + } + .bind(&mut vars); + + assert_eq!( + vars.get("proxy_name").and_then(|v| v.as_str()), + Some("alpha"), + "a mock must not be able to change what a system variable means" + ); + } +} diff --git a/crates/doppel-store-postgres/tests/load.rs b/crates/doppel-store-postgres/tests/load.rs index 69e993c..e88d772 100644 --- a/crates/doppel-store-postgres/tests/load.rs +++ b/crates/doppel-store-postgres/tests/load.rs @@ -69,7 +69,7 @@ proxies: method: GET url: /widgets/(?P\d+)/ headers: - requestId: X-Request-ID + request_id: X-Request-ID query: filter: .filter body: diff --git a/crates/doppel-telemetry/src/sentry.rs b/crates/doppel-telemetry/src/sentry.rs index b6fe1b8..ff94dd4 100644 --- a/crates/doppel-telemetry/src/sentry.rs +++ b/crates/doppel-telemetry/src/sentry.rs @@ -45,21 +45,56 @@ impl std::fmt::Debug for Sentry { } } -/// The DSN, if the configuration names a non-empty one. +/// Where a DSN came from, for the line that says reporting is on. /// -/// An absent section and a section with an empty or whitespace-only DSN mean -/// the same thing. Treating `dsn: ""` as a value would try to initialise -/// Sentry against nothing and fail startup for what is plainly a way of +/// Worth logging: "sentry reporting enabled" with a redacted DSN does not tell an +/// operator whether the document or the environment won, and that is exactly what +/// they need to know when it is the wrong one. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DsnSource { + Document, + Environment, +} + +impl DsnSource { + const fn as_str(self) -> &'static str { + match self { + Self::Document => "sentry.dsn", + Self::Environment => "DOPPEL_SENTRY_DSN", + } + } +} + +/// The DSN to use, and where it came from. +/// +/// The environment wins. A DSN is a credential, and a deployment that provisions +/// credentials through the environment is overriding a document it may not even +/// be able to edit -- the same precedence `DOPPEL_ADMIN_TOKENS` has over +/// `admin.tokens`. +/// +/// An absent section, a section with an empty or whitespace-only DSN, and an unset +/// variable all mean the same thing. Treating `dsn: ""` as a value would try to +/// initialise Sentry against nothing and fail startup for what is plainly a way of /// writing "off". -fn configured_dsn(config: Option<&SentryConfig>) -> Option<&str> { +fn resolve_dsn<'a>( + config: Option<&'a SentryConfig>, + from_env: Option<&'a str>, +) -> Option<(&'a str, DsnSource)> { + if let Some(dsn) = from_env.map(str::trim).filter(|dsn| !dsn.is_empty()) { + return Some((dsn, DsnSource::Environment)); + } config .map(|sentry| sentry.dsn.trim()) .filter(|dsn| !dsn.is_empty()) + .map(|dsn| (dsn, DsnSource::Document)) } #[cfg(feature = "sentry")] -pub fn init(config: Option<&SentryConfig>) -> Result { - let Some(dsn) = configured_dsn(config) else { +pub fn init( + config: Option<&SentryConfig>, + from_env: Option<&str>, +) -> Result { + let Some((dsn, source)) = resolve_dsn(config, from_env) else { return Ok(Sentry { status: SentryStatus::Disabled, _guard: None, @@ -85,7 +120,11 @@ pub fn init(config: Option<&SentryConfig>) -> Result { options.release = ::sentry::release_name!(); let guard = ::sentry::init(options); - tracing::info!(dsn = %redact_credentials(dsn), "sentry reporting enabled"); + tracing::info!( + dsn = %redact_credentials(dsn), + source = source.as_str(), + "sentry reporting enabled" + ); Ok(Sentry { status: SentryStatus::Enabled, _guard: Some(guard), @@ -93,8 +132,11 @@ pub fn init(config: Option<&SentryConfig>) -> Result { } #[cfg(not(feature = "sentry"))] -pub fn init(config: Option<&SentryConfig>) -> Result { - let status = if let Some(dsn) = configured_dsn(config) { +pub fn init( + config: Option<&SentryConfig>, + from_env: Option<&str>, +) -> Result { + let status = if let Some((dsn, source)) = resolve_dsn(config, from_env) { // Loud rather than silent. The operator asked for reporting and will // not get it; a knob that reads as honoured and is not is the defect // this project already removed once, in `admin.workers`. Not fatal, @@ -102,8 +144,9 @@ pub fn init(config: Option<&SentryConfig>) -> Result { // turn an observability gap into an outage. tracing::warn!( dsn = %redact_credentials(dsn), - "sentry.dsn is configured but this binary was built without the `sentry` feature; \ - nothing will be reported" + source = source.as_str(), + "a sentry dsn is configured but this binary was built without the `sentry` \ + feature; nothing will be reported" ); SentryStatus::Unsupported } else { @@ -124,25 +167,81 @@ mod tests { #[test] fn no_sentry_section_is_disabled_and_not_an_error() { - assert_eq!(init(None).unwrap().status, SentryStatus::Disabled); + assert_eq!(init(None, None).unwrap().status, SentryStatus::Disabled); } #[test] fn an_empty_dsn_is_the_documented_way_to_turn_it_off() { for dsn in ["", " ", "\t"] { assert_eq!( - init(Some(&config(dsn))).unwrap().status, + init(Some(&config(dsn)), None).unwrap().status, SentryStatus::Disabled, "{dsn:?} should read as off" ); } } + #[test] + fn the_environment_provides_a_dsn_when_the_document_has_none() { + // The case this exists for: a deployment that provisions credentials + // through the environment and a configuration that names none. + let (dsn, source) = resolve_dsn(None, Some("https://key@sentry.example.com/1")) + .expect("the variable is a dsn"); + + assert_eq!(dsn, "https://key@sentry.example.com/1"); + assert_eq!(source, DsnSource::Environment); + } + + #[test] + fn the_environment_wins_over_the_document() { + // The same precedence `DOPPEL_ADMIN_TOKENS` has: a deployment overriding + // a document it may not be able to edit. + let document = config("https://from-document@sentry.example.com/1"); + let (dsn, source) = resolve_dsn( + Some(&document), + Some("https://from-environment@sentry.example.com/2"), + ) + .expect("a dsn is resolved"); + + assert_eq!(dsn, "https://from-environment@sentry.example.com/2"); + assert_eq!(source, DsnSource::Environment); + } + + #[test] + fn an_empty_variable_leaves_the_document_in_force() { + // Deliberately this direction. `DOPPEL_SENTRY_DSN=${SENTRY_DSN}` with + // nothing behind `SENTRY_DSN` is a compose file that means nothing by it, + // and silently turning error reporting off is the worse reading. + let document = config("https://key@sentry.example.com/1"); + for empty in ["", " "] { + let (dsn, source) = + resolve_dsn(Some(&document), Some(empty)).expect("the document still names one"); + + assert_eq!(dsn, "https://key@sentry.example.com/1", "{empty:?}"); + assert_eq!(source, DsnSource::Document, "{empty:?}"); + } + } + + #[test] + fn a_dsn_from_the_environment_is_trimmed_like_one_from_the_document() { + let (dsn, _) = resolve_dsn(None, Some(" https://key@sentry.example.com/1 ")) + .expect("whitespace is not part of a dsn"); + assert_eq!(dsn, "https://key@sentry.example.com/1"); + } + + #[test] + fn the_source_is_named_the_way_an_operator_would_look_for_it() { + // It goes in a log line, so it has to be the thing they would grep for + // rather than a word this module invented. + assert_eq!(DsnSource::Document.as_str(), "sentry.dsn"); + assert_eq!(DsnSource::Environment.as_str(), "DOPPEL_SENTRY_DSN"); + } + #[test] fn the_debug_of_the_guard_carries_no_dsn() { // `serve` may log its state, and a `Debug` that printed the DSN would // put the key in the log the first time anyone did. - let sentry = init(Some(&config("https://key@sentry.example.com/1"))).unwrap(); + let sentry = init(Some(&config("https://key@sentry.example.com/1")), None).unwrap(); let rendered = format!("{sentry:?}"); assert!(!rendered.contains("key"), "{rendered}"); assert!(!rendered.contains("sentry.example.com"), "{rendered}"); @@ -154,7 +253,7 @@ mod tests { // Reported rather than accepted: `sentry::init` takes an unparseable // value and hands back a client that drops everything, so the only // signal an operator would get is silence. - let err = init(Some(&config("https://s3cr3tkey@/missing-project"))) + let err = init(Some(&config("https://s3cr3tkey@/missing-project")), None) .expect_err("a DSN with no host must be refused"); let message = err.to_string(); assert!(!message.contains("s3cr3tkey"), "{message}"); @@ -163,8 +262,11 @@ mod tests { #[cfg(feature = "sentry")] #[test] fn a_wholly_unparseable_dsn_is_not_echoed_back() { - let err = init(Some(&config("this is not a dsn but it might be a secret"))) - .expect_err("must be refused"); + let err = init( + Some(&config("this is not a dsn but it might be a secret")), + None, + ) + .expect_err("must be refused"); let message = err.to_string(); assert!(!message.contains("might be a secret"), "{message}"); assert!(message.contains(""), "{message}"); @@ -176,7 +278,7 @@ mod tests { // The distinction is the point: `Disabled` means the operator turned // it off, `Unsupported` means they asked and this build cannot. assert_eq!( - init(Some(&config("https://key@sentry.example.com/1"))) + init(Some(&config("https://key@sentry.example.com/1")), None) .unwrap() .status, SentryStatus::Unsupported diff --git a/docs/usage/configuration.md b/docs/usage/configuration.md index a57ac60..000b32e 100644 --- a/docs/usage/configuration.md +++ b/docs/usage/configuration.md @@ -68,7 +68,7 @@ server: |---|---|---|---| | `host` | IP address | required | Must parse as an IP, not a hostname | | `port` | 1..65535 | required | Must differ from `admin.port` | -| `external_url` | absolute URL | `host:port` | Where clients reach Doppel, for rewriting a redirect or a body. `DOPPEL_EXTERNAL_URL` overrides it. See [Doppel's own address](proxying.md#doppels-own-address) | +| `external_url` | absolute URL, or a template | `host:port` | Where clients reach Doppel, for rewriting a redirect or a body. May be a template over the [system variables](mocks.md#system-variables), e.g. `http://{{ host }}/`. `DOPPEL_EXTERNAL_URL` overrides it. See [Doppel's own address](proxying.md#doppels-own-address) | Worker threads are **not** configured here. They size the tokio runtime, and a database-backed store cannot be opened before that runtime exists -- so the @@ -120,6 +120,11 @@ sentry: Optional. An absent section or an empty DSN disables it. +`DOPPEL_SENTRY_DSN` provides the DSN, or replaces the one written here -- a DSN is +a credential, and the environment is where a deployment usually keeps one. An +empty variable counts as unset and leaves this field in force. See +[Sentry](observability.md#the-dsn-from-the-environment). + ## `admin` The admin listener's address, its tokens, and who may do what. diff --git a/docs/usage/mocks.md b/docs/usage/mocks.md index c53b2a0..21db33f 100644 --- a/docs/usage/mocks.md +++ b/docs/usage/mocks.md @@ -47,21 +47,22 @@ that gets forwarded. ## Variables -Four sources, all optional. +Five sources. Four are yours and optional; the fifth is Doppel's and always +there. **Path captures.** Named groups in the pattern become variables: ```yaml - url: /api/v1/resource/(?P\d+)/ + url: /api/v1/resource/(?P\d+)/ ``` -binds `resourceId`. +binds `resource_id`. **Headers.** A map of variable name to header name: ```yaml headers: - requestId: X-Request-ID + trace_id: X-Trace-Id ``` **Query and body.** A map of variable name to a selector -- a leading dot and @@ -71,7 +72,7 @@ dot-separated keys: query: filter: .filter body: - itemCount: .content.items + item_count: .content.items ``` Selectors address object keys. A selector that lands on an array yields the @@ -81,6 +82,55 @@ not supported. A variable name may not collide with a capture group name; that is rejected at load. +Names are `snake_case` throughout this documentation and in +`main.example.yaml`. Jinja accepts any identifier, so `itemCount` works -- but +Doppel's own variables are `snake_case`, and one convention per context reads as +one source. + +## System variables + +Doppel binds nine variables into every template it renders, whether or not the +mock asked for anything: + +| Variable | Is | +|---|---| +| `proxy_name` | The proxy that resolved. Empty for a request that resolved to none | +| `mock_name` | The mock answering. Empty when the request is being forwarded | +| `doppel_version` | The version of the binary serving the request | +| `request_id` | The id echoed in `X-Request-ID`, minted when the client sent none | +| `method` | The request method | +| `path` | The request path, without the query string | +| `host` | The `Host` the client asked for. Empty when it sent none | +| `peer_ip` | The address the connection came from | +| `real_ip` | Who the request is *said* to be from: `X-Real-IP`, else the leftmost `X-Forwarded-For` entry, else `peer_ip` | + +```yaml + response: + status: 200 + json: '{"served_by": "{{ proxy_name }} {{ doppel_version }}", "caller": "{{ real_ip }}"}' + headers: + X-Request-ID: "{{ request_id }}" +``` + +**`peer_ip` and `real_ip` are not the same claim.** `peer_ip` is the socket's own +address and nobody can fake it. `real_ip` is what a proxy in front says, out of +headers a client can also send -- useful for a mock that reports who called it, +and not something to make a decision on unless you know what sits in front. + +**They are reserved.** They are bound after your extractions, so a mock that +extracts into `proxy_name` finds Doppel's value in its template rather than its +own. The extraction still happens and its result is thrown away, which is why +startup says so: + +``` +mock `m1` of proxy `alpha` extracts `proxy_name` into a name Doppel binds +itself; the system value wins, so the extraction is read and thrown away +``` + +Being always present, they also never need `| default('')` -- an absent one is an +empty string rather than an undefined variable, which is the one place Doppel's +own variables are gentler than yours. + ## Rendering Exactly one of three fields produces the body, or none at all for a status that @@ -101,13 +151,13 @@ An undefined variable is an error, not an empty string. ```yaml headers: - requestId: X-Request-ID + request_id: X-Request-ID response: - json: '{"seen": "{{ requestId }}"}' + json: '{"seen": "{{ request_id }}"}' ``` A request without `X-Request-ID` fails this mock with -`TEMPLATE_RENDER_ERROR`, because `requestId` is undefined. That is deliberate: +`TEMPLATE_RENDER_ERROR`, because `request_id` is undefined. That is deliberate: a mock that silently renders `"seen": ""` because a variable was mistyped is worse than one that refuses. The error message names the expression that failed. @@ -115,7 +165,7 @@ failed. If a variable should be optional, say so: ```yaml - json: '{"seen": "{{ requestId | default('''') }}"}' + json: '{"seen": "{{ request_id | default('''') }}"}' ``` ## Serving some of the time diff --git a/docs/usage/observability.md b/docs/usage/observability.md index edf7c46..ea75d9b 100644 --- a/docs/usage/observability.md +++ b/docs/usage/observability.md @@ -137,6 +137,10 @@ Optional, behind the `sentry` cargo feature and off in a default build: cargo build --release --features sentry ``` +Neither the released binaries nor the published image carry it: a default build +should not ship a TLS stack and an HTTP client it never uses. Reporting to Sentry +means building your own, which is the trade this flag is. + ```yaml sentry: dsn: "https://key@sentry.example.com/1" @@ -146,6 +150,37 @@ An absent section or an empty DSN disables it and is not an error. A malformed DSN fails startup rather than producing a client that silently drops everything -- with the key masked in the message. +### The DSN from the environment + +```bash +DOPPEL_SENTRY_DSN="https://key@sentry.example.com/1" +``` + +A DSN carries the key that authorises sending events, which makes it the one +Sentry setting that is a credential -- and a deployment that provisions +credentials through the environment should not have to write this one into a +document the admin API returns and the store keeps. + +The variable wins over `sentry.dsn`, the same way `DOPPEL_ADMIN_TOKENS` wins over +`admin.tokens`: it is how a deployment overrides a document it may not be able to +edit. Startup says which source it used, so the answer is one line away rather +than a guess: + +```json +{"level":"INFO","fields":{"message":"sentry reporting enabled", + "dsn":"https://@sentry.example.com/1","source":"DOPPEL_SENTRY_DSN"}} +``` + +**An empty variable is not a way to turn reporting off.** It counts as unset and +leaves `sentry.dsn` in force, deliberately: `DOPPEL_SENTRY_DSN=${SENTRY_DSN}` with +nothing behind `SENTRY_DSN` is a compose file that means nothing by it, and +silently disabling error reporting is the worse reading. To turn it off, write +`dsn: ""` or remove the section. + +Doppel reads its own name, not the conventional `SENTRY_DSN`. A variable that is +in the environment for the service beside this one should not make this one start +reporting to it. + A build without the feature that is given a DSN warns at startup and carries on. It does not pretend to report, and it does not refuse to run: reporting is optional by design, so turning a missing integration into an outage would be diff --git a/docs/usage/parameters.md b/docs/usage/parameters.md index ba36571..80e42a6 100644 --- a/docs/usage/parameters.md +++ b/docs/usage/parameters.md @@ -1344,6 +1344,11 @@ sentry: The Sentry DSN to report to. Empty disables reporting, so a deployment can blank it without removing the section. +`DOPPEL_SENTRY_DSN` overrides it. A DSN carries the key that authorises +sending events, so a deployment that keeps credentials in the environment +can leave this field out entirely; an empty variable counts as unset and +leaves this value in force. + | | | |---|---| | Type | string | diff --git a/docs/usage/proxying.md b/docs/usage/proxying.md index bcaa679..575e4da 100644 --- a/docs/usage/proxying.md +++ b/docs/usage/proxying.md @@ -237,6 +237,36 @@ rather than assumed. A path is kept as a prefix: `https://gw.example.com/doppel/` is a Doppel reached under a prefix, and its rewritten locations carry it. +### When one address is not enough + +`external_url` may be a template over the +[system variables](mocks.md#system-variables), rendered per request: + +```yaml +server: + # Whatever this client asked for. + external_url: "http://{{ host }}/" + # Or a name per proxy, behind a wildcard. + external_url: "https://{{ proxy_name }}.gw.example.com/" +``` + +A value containing `{{` is a template; anything else is parsed as a url when the +configuration is read, as before. The scheme has to be literal -- `http://` or +`https://` -- because a value that does not start with one cannot become a usable +url however it renders, and that is worth failing at startup. + +!!! warning "`{{ host }}` is the caller's claim" + `Host` arrives from the client. A deployment that builds a redirect out of it + is choosing to let a caller decide where its own redirects point, which is + fine when something in front validates the host and is not when nothing does. + That is why Doppel does not do this by default: it is one line to opt in, and + the line is where the decision belongs. + +A template that fails to render, or renders to something that is not a url, means +**no rewriting for that request** -- the upstream's own `Location` is relayed +instead. A cosmetic feature is not worth a `500`, and the reason is logged at +debug rather than per redirect at warn. + Set `rewrite_redirects: false` to relay the header byte for byte. That is what a client being tested *for its redirect handling* needs; it is not what a client being tested against a degraded backend needs. diff --git a/doppel-config.schema.json b/doppel-config.schema.json index f8705e4..483402d 100644 --- a/doppel-config.schema.json +++ b/doppel-config.schema.json @@ -747,7 +747,7 @@ "additionalProperties": false, "properties": { "dsn": { - "description": "The Sentry DSN to report to. Empty disables reporting, so a deployment\ncan blank it without removing the section.", + "description": "The Sentry DSN to report to. Empty disables reporting, so a deployment\ncan blank it without removing the section.\n\n`DOPPEL_SENTRY_DSN` overrides it. A DSN carries the key that authorises\nsending events, so a deployment that keeps credentials in the environment\ncan leave this field out entirely; an empty variable counts as unset and\nleaves this value in force.", "examples": [ "https://key@o0.ingest.sentry.io/0" ], diff --git a/frontend/e2e/tests/syntax.spec.ts b/frontend/e2e/tests/syntax.spec.ts index 6e54acf..ad521a3 100644 --- a/frontend/e2e/tests/syntax.spec.ts +++ b/frontend/e2e/tests/syntax.spec.ts @@ -70,7 +70,7 @@ test('a path pattern is coloured, and sits at the height of a control', async ({ await page.getByRole('button', { name: 'Add a mock' }).click() const pattern = page.getByLabel('Path pattern') - await pattern.fill('^/api/v1/resource/(?P\\d+)/$') + await pattern.fill('^/api/v1/resource/(?P\\d+)/$') const colours = await tokenColours(page) // Groups, classes, quantifiers and anchors are four different things, and prism's @@ -103,7 +103,7 @@ test('the whole document is coloured in the YAML editor', async ({ page }) => { test('a Jinja expression is coloured everywhere one is allowed', async ({ page }) => { // Four fields render through Jinja: a mock's text body, its JSON body, a response // header, and a template file. The braces used to be plain in all of them, which made - // `{{ requestId }}` look like part of the text it is not. + // `{{ request_id }}` look like part of the text it is not. await signIn(page) await page.getByRole('button', { name: 'Add a proxy' }).click() await page.locator('summary').filter({ hasText: 'Mocks' }).first().click() @@ -112,7 +112,7 @@ test('a Jinja expression is coloured everywhere one is allowed', async ({ page } // A header value: one line, and a template rather than a string. await page.getByRole('button', { name: 'Add Response headers' }).click() await page.getByLabel('Response headers header name 1').fill('X-Request-ID') - await page.getByLabel('Response headers template 1').fill('rid-{{ requestId }}') + await page.getByLabel('Response headers template 1').fill('rid-{{ request_id }}') const header = await tokenColours(page) expect(header.size).toBeGreaterThan(1) @@ -124,7 +124,7 @@ test('a Jinja expression is coloured everywhere one is allowed', async ({ page } // browser rather than only in a unit test -- an expression inside a string is the // case prism resolves in favour of the string unless it is asked not to. await page.getByLabel('mock-1 response source').selectOption('json') - await page.getByLabel(/mock-1 json/).fill('{"id": "{{ resourceId }}", "n": 1}') + await page.getByLabel(/mock-1 json/).fill('{"id": "{{ resource_id }}", "n": 1}') const both = await paintedRgbAll(page, 'pre .token', 'color') const json = await paintedRgbAll(page, 'pre .token.property', 'color') const jinja = await paintedRgbAll(page, 'pre .token.jinja2 .token.variable', 'color') diff --git a/frontend/e2e/tests/validation.spec.ts b/frontend/e2e/tests/validation.spec.ts index 6ca0384..3d3fd99 100644 --- a/frontend/e2e/tests/validation.spec.ts +++ b/frontend/e2e/tests/validation.spec.ts @@ -141,7 +141,7 @@ test('a path pattern is left to the server, dialect and all', async ({ page }) = await open(page, 'Mocks') await page.getByRole('button', { name: 'Add a mock' }).click() - await page.getByLabel('Path pattern').fill('/api/v1/resource/(?P\\d+)/') + await page.getByLabel('Path pattern').fill('/api/v1/resource/(?P\\d+)/') // No complaint anywhere on the form: the field's own hint still says what a path // pattern is, and nothing has been reported as wrong. await expect(page.getByRole('alert')).toHaveCount(0) diff --git a/frontend/src/components/KeyValueRows.tsx b/frontend/src/components/KeyValueRows.tsx index 2ca57f8..f6f5ab1 100644 --- a/frontend/src/components/KeyValueRows.tsx +++ b/frontend/src/components/KeyValueRows.tsx @@ -55,7 +55,7 @@ export function KeyValueRows({ /** * What the values are written in, when they are not plain strings. * - * A mock's response headers are Jinja templates -- `rid-{{ requestId }}` -- so the + * A mock's response headers are Jinja templates -- `rid-{{ request_id }}` -- so the * value gets a one-line editor with the braces coloured instead of an input. Every * other map here holds a header name or a selector, which are neither templates nor * anything else worth colouring. diff --git a/frontend/src/components/__tests__/grammars.test.ts b/frontend/src/components/__tests__/grammars.test.ts index 4d2519e..fd78af4 100644 --- a/frontend/src/components/__tests__/grammars.test.ts +++ b/frontend/src/components/__tests__/grammars.test.ts @@ -14,7 +14,7 @@ function tokens(html: string): string[] { describe('a template with no host language', () => { it('colours the expression and leaves the text alone', () => { - const html = colour('rid-{{ requestId }}', 'text') + const html = colour('rid-{{ request_id }}', 'text') expect(tokens(html)).toContain('jinja2 jinja2') expect(tokens(html)).toContain('variable') // The text around it is text: `rid-` is not a variable, and neither is a word that @@ -42,7 +42,7 @@ describe('JSON with Jinja in it', () => { // The case that a single top-level jinja token silently missed: prism gives // overlapping greedy patterns to whichever starts first, and the quote starts // before the brace. - const html = colour('{"id": "{{ resourceId }}", "n": 1}', 'json') + const html = colour('{"id": "{{ resource_id }}", "n": 1}', 'json') const classes = tokens(html) expect(classes).toContain('property') expect(classes).toContain('number') diff --git a/main.example.yaml b/main.example.yaml index bbabe62..233400f 100644 --- a/main.example.yaml +++ b/main.example.yaml @@ -101,7 +101,7 @@ proxies: body_limit: 1Mi # bounds the body buffered for mocks that extract from it (mock2, mock6 below); default 1Mi # Order matters. Patterns are matched as unanchored regexes, so a general # pattern placed first shadows every more specific one below it: with - # `/api/v1/resource/` ahead of `/api/v1/resource/(?P\d+)/`, + # `/api/v1/resource/` ahead of `/api/v1/resource/(?P\d+)/`, # the second can never fire, because the first already matches that path. # Specific patterns therefore come first here. Doppel does not detect this # for you -- a shadowed mock is a valid configuration, just a useless one. @@ -121,39 +121,44 @@ proxies: max: 0.3 - name: mock4 # Referencing an extracted header makes that header required: rendering - # is strict, so a request without X-Request-ID fails this mock with + # is strict, so a request without X-Trace-Id fails this mock with # TEMPLATE_RENDER_ERROR rather than rendering an empty string. Use - # `{{ requestId | default('') }}` in the templates below if the header - # should be optional. + # `{{ trace_id | default('') }}` below if the header should be optional. + # + # `request_id` is not extracted here and must not be: it is one of the + # system variables Doppel binds itself, always present, minted when the + # client sent no X-Request-ID. Extracting into that name would be read and + # then overwritten, which is what the startup advisory about shadowing is + # for. request: method: GET - url: /api/v1/resource/(?P\d+)/ + url: /api/v1/resource/(?P\d+)/ headers: - requestId: X-Request-ID + trace_id: X-Trace-Id response: status: 200 - json: '{"message": "Success", "id": "{{ resourceId }}"}' + json: '{"message": "Success", "id": "{{ resource_id }}", "served_by": "{{ proxy_name }} {{ doppel_version }}"}' headers: - X-Resource-ID: "{{ resourceId }}" - X-Request-ID: "{{ requestId }}" + X-Resource-ID: "{{ resource_id }}" + X-Request-ID: "{{ request_id }}" # a system variable + X-Trace-Id: "{{ trace_id }}" - name: mock5 request: method: DELETE - url: /api/v1/resource/(?P\d+)/ - headers: - requestId: X-Request-ID + url: /api/v1/resource/(?P\d+)/ response: status: 204 # 204 forbids a body, so none is declared headers: - X-Resource-ID: "{{ resourceId }}" + X-Resource-ID: "{{ resource_id }}" + X-Request-ID: "{{ request_id }}" - name: mock6 request: method: PUT - url: /api/v1/resource/(?P\d+)/ + url: /api/v1/resource/(?P\d+)/ body: - resourceName: .name - resourceDescription: .description - resourceItems: .content.items + resource_name: .name + resource_description: .description + resource_items: .content.items response: status: 200 template: put.json.j2 @@ -177,15 +182,15 @@ proxies: method: POST url: /api/v1/resource/ body: - resourceName: .name - resourceDescription: .description - resourceItems: .content.items + resource_name: .name + resource_description: .description + resource_items: .content.items query: filter: .filter sort: .sort response: status: 201 - json: '{"message": "Resource created", "name": "{{ resourceName }}", "description": "{{ resourceDescription }}", "items": {{ resourceItems | length }}}' + json: '{"message": "Resource created", "name": "{{ resource_name }}", "description": "{{ resource_description }}", "items": {{ resource_items | length }}}' headers: Content-Type: application/json