From 72780c64ac5825d07a02d41d4ecfdce55174c6c3 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Fri, 14 Aug 2026 09:41:30 +0200 Subject: [PATCH 1/4] feat: bind Doppel's own variables into every template A template could see only what its own mock declared, so the things every mock wants -- which proxy answered, which version answered, who called -- had to be extracted by hand in each one, and two of them could not be extracted at all. Nine system variables now, bound into every render: `proxy_name`, `mock_name`, `doppel_version`, `request_id`, `method`, `path`, `host`, `peer_ip`, `real_ip`. `real_ip` is the chain a proxy in front writes: `X-Real-IP`, then the *leftmost* `X-Forwarded-For` entry -- the original client rather than the hop next to us, across field lines, because a chain split over several is legal and some proxies emit it -- then the connection's own address, so the variable always has a value and no template needs `| default('')`. `peer_ip` is that address on its own, and the pair is deliberate: one is a claim anybody can make and the other is a fact, and a mock that decides something needs to know which it is reading. They are reserved. Bound *after* the operator's extractions, so a mock that extracts into `proxy_name` finds Doppel's value rather than its own -- the alternative is a template whose meaning depends on which mock rendered it. The extraction still costs a header read, so `startup_advisories` names the mock and the names it shadowed. The list of names lives in `doppel-core`, which is the crate both the binding and the validation can see. `server.external_url` may now be a template over them, 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. Opt-in for a reason -- `Host` is the caller's claim, so a deployment that builds a redirect out of it is choosing to, and the documentation says so in a warning rather than a footnote. A template that fails to render, or renders to something that is not a url, means no rewriting for that request rather than a failed one: a cosmetic improvement to a `Location` is not worth a 500. Every variable name in `main.example.yaml` and the documentation is `snake_case` now, matching the system ones. Jinja accepts either, and an operator's own names are untouched -- but two conventions in one context read as two sources. The reference configuration also stopped extracting `X-Request-ID` into `request_id`: it is a system variable, always present and minted when the client sends none, so the extraction was exactly the wasted work the new advisory warns about. `mock4` keeps the strict-rendering lesson on `X-Trace-Id`, which is genuinely the operator's, and renders `proxy_name` and `doppel_version` beside it. Seventeen unit tests, three on the templated address, one end-to-end through the binary over a real socket -- the `real_ip` chain needs a peer to fall back to, and only a connection has one. Mutation-checked twice: nothing binding the system variables fails the end-to-end test with `undefined variable proxy_name`, and ignoring `X-Real-IP` fails the chain test. --- CHANGES.md | 16 ++ crates/doppel-cli/src/commands/serve.rs | 3 +- crates/doppel-cli/tests/mocks.rs | 41 +++- crates/doppel-cli/tests/proxying.rs | 94 ++++++++ crates/doppel-core/src/config/url.rs | 80 ++++++- crates/doppel-core/src/lib.rs | 1 + crates/doppel-core/src/runtime.rs | 8 +- crates/doppel-core/src/template.rs | 55 +++++ crates/doppel-core/src/validate/advisory.rs | 75 ++++++ crates/doppel-core/src/validate/mock.rs | 6 +- crates/doppel-proxy/src/rewrite.rs | 37 +++ crates/doppel-proxy/src/server.rs | 89 +++++++- crates/doppel-proxy/src/upstream.rs | 72 ++++++ crates/doppel-render/src/extract.rs | 11 + crates/doppel-render/src/lib.rs | 2 + crates/doppel-render/src/render.rs | 4 +- crates/doppel-render/src/system.rs | 216 ++++++++++++++++++ crates/doppel-store-postgres/tests/load.rs | 2 +- docs/usage/configuration.md | 2 +- docs/usage/mocks.md | 68 +++++- docs/usage/proxying.md | 30 +++ frontend/e2e/tests/syntax.spec.ts | 8 +- frontend/e2e/tests/validation.spec.ts | 2 +- frontend/src/components/KeyValueRows.tsx | 2 +- .../src/components/__tests__/grammars.test.ts | 4 +- main.example.yaml | 47 ++-- 26 files changed, 898 insertions(+), 77 deletions(-) create mode 100644 crates/doppel-core/src/template.rs create mode 100644 crates/doppel-render/src/system.rs diff --git a/CHANGES.md b/CHANGES.md index 9687a10..5575892 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,22 @@ release promotes it to a version heading; the `bump-version` skill does that. ## Development +### 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. + +### 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/crates/doppel-cli/src/commands/serve.rs b/crates/doppel-cli/src/commands/serve.rs index 1d6d2e8..0e9b1ce 100644 --- a/crates/doppel-cli/src/commands/serve.rs +++ b/crates/doppel-cli/src/commands/serve.rs @@ -262,8 +262,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/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/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/docs/usage/configuration.md b/docs/usage/configuration.md index a57ac60..a905331 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 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/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/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 From a6c0779bd5dc2edaddf7b53f5782cc28a1235e30 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Fri, 14 Aug 2026 11:01:38 +0200 Subject: [PATCH 2/4] feat: take the Sentry DSN from the environment A DSN carries the key that authorises sending events, which makes it the one Sentry setting that is a credential -- and until now the only way to give Doppel one was to write it into a document the admin API returns and the store keeps. `DOPPEL_SENTRY_DSN` provides it instead, or replaces what `sentry.dsn` says. The environment wins, the same way `DOPPEL_ADMIN_TOKENS` wins over `admin.tokens`: that is how a deployment overrides a document it may not be able to edit. Which source won goes in the startup line beside the redacted DSN, because "sentry reporting enabled" does not answer the question an operator has when it is the wrong DSN. An empty variable counts as unset and leaves `sentry.dsn` in force. Deliberately that direction: `DOPPEL_SENTRY_DSN=${SENTRY_DSN}` with nothing behind `SENTRY_DSN` is a compose file that means nothing by it, and silently switching error reporting off is the worse reading of it. Turning it off is still `dsn: ""`, or no section. Doppel reads its own name rather than the conventional `SENTRY_DSN`: a variable sitting in the environment for the service beside this one should not make this one start reporting to it. Read at startup and not merged into `Config`, like the other two environment values -- the revision is a hash of the document, so folding the environment into it would make two instances reading one stored document disagree about the revision. Validation stays where it was, at the client: the crate that knows what a DSN must parse as is also the one that can redact it in the error. Five unit tests on the precedence and two through the built binary, which is the only place the variable is actually read from a process. Mutation-checked by ignoring the environment: three tests fail, including the end-to-end one. --- CHANGES.md | 2 + README.md | 3 + crates/doppel-cli/src/commands/serve.rs | 12 +- crates/doppel-cli/tests/logging.rs | 68 ++++++++++++ crates/doppel-core/src/config/env.rs | 36 +++++- crates/doppel-core/src/config/mod.rs | 5 +- crates/doppel-core/src/config/server.rs | 5 + crates/doppel-telemetry/src/sentry.rs | 140 ++++++++++++++++++++---- docs/usage/configuration.md | 5 + docs/usage/observability.md | 31 ++++++ docs/usage/parameters.md | 5 + doppel-config.schema.json | 2 +- 12 files changed, 289 insertions(+), 25 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 5575892..b1b30af 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -16,6 +16,8 @@ release promotes it to a version heading; the `bump-version` skill does that. 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 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 0e9b1ce..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 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-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-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 a905331..000b32e 100644 --- a/docs/usage/configuration.md +++ b/docs/usage/configuration.md @@ -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/observability.md b/docs/usage/observability.md index edf7c46..3489d90 100644 --- a/docs/usage/observability.md +++ b/docs/usage/observability.md @@ -146,6 +146,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/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" ], From 52a008eb0bda5167da14350ee4ecb534fa40021f Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Fri, 14 Aug 2026 11:37:52 +0200 Subject: [PATCH 3/4] docs: say which credentials the environment can carry, and which builds report `DOCKERHUB.md` knew about `DOPPEL_ADMIN_TOKENS` and not about `DOPPEL_SENTRY_DSN`, which is the file's own failure mode: it is published somewhere this repository cannot see, and nothing fails when it falls behind. Found by the step added to `pre-release-check` for exactly that, on its first release. The section is about both credentials now, with the empty-variable rule stated -- a variable that failed to interpolate leaves a configured DSN in force rather than silently turning error reporting off. Writing that turned up a second thing, and this one was nearly a false claim I made myself: nothing that ships enables the `sentry` feature. Not the image, not the released binaries. A DSN given to either warns at startup and is not reported, so a reader following the new section would have set a variable that does nothing. Both pages say so now, next to the flag that changes it. --- DOCKERHUB.md | 18 +++++++++++++++--- docs/usage/observability.md | 4 ++++ 2 files changed, 19 insertions(+), 3 deletions(-) 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/docs/usage/observability.md b/docs/usage/observability.md index 3489d90..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" From b0399b66fde9379cdc51abee47342ccb3742a385 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Fri, 14 Aug 2026 11:38:16 +0200 Subject: [PATCH 4/4] chore: release 1.1.0 --- CHANGES.md | 2 ++ Cargo.lock | 14 +++++++------- Cargo.toml | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b1b30af..8bf7070 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,8 @@ 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`, 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"