From a6c0779bd5dc2edaddf7b53f5782cc28a1235e30 Mon Sep 17 00:00:00 2001 From: Lorem Dev Date: Fri, 14 Aug 2026 11:01:38 +0200 Subject: [PATCH] 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" ],