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" ],