diff --git a/Cargo.lock b/Cargo.lock index 006d8dd374..b9bd444e6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4326,6 +4326,7 @@ dependencies = [ "oauth2", "openshell-bootstrap", "openshell-core", + "openshell-otel", "openshell-crypto", "openshell-policy", "openshell-providers", diff --git a/crates/openshell-cli/Cargo.toml b/crates/openshell-cli/Cargo.toml index a6cb1fc895..7fcdef8bf0 100644 --- a/crates/openshell-cli/Cargo.toml +++ b/crates/openshell-cli/Cargo.toml @@ -18,6 +18,7 @@ path = "src/main.rs" openshell-crypto = { workspace = true } openshell-bootstrap = { path = "../openshell-bootstrap" } openshell-core = { path = "../openshell-core", default-features = false } +openshell-otel = { path = "../openshell-otel" } openshell-policy = { path = "../openshell-policy" } openshell-providers = { path = "../openshell-providers" } openshell-sdk = { path = "../openshell-sdk" } diff --git a/crates/openshell-cli/src/tls.rs b/crates/openshell-cli/src/tls.rs index f96cbe01f4..920cab9488 100644 --- a/crates/openshell-cli/src/tls.rs +++ b/crates/openshell-cli/src/tls.rs @@ -5,6 +5,7 @@ use miette::{IntoDiagnostic, Result, WrapErr}; use openshell_core::auth::EdgeAuthInterceptor; use openshell_core::net::set_tcp_nodelay_best_effort; use openshell_core::proto::open_shell_client::OpenShellClient; +use openshell_otel::EnvTraceContextInterceptor; use rustls::{ RootCertStore, client::danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}, @@ -25,7 +26,30 @@ use tracing::debug; use url::{Host, Url}; /// Concrete gRPC client type used by all commands. -pub type GrpcClient = OpenShellClient>; +pub type GrpcClient = OpenShellClient>; + +/// Interceptor stack applied to gateway requests from clients built by +/// [`grpc_client`]. +/// +/// Runs authentication header injection first, then passive W3C trace-context +/// forwarding so a CI pipeline's `TRACEPARENT` extends into the gateway's +/// spans. tonic allows a single interceptor per channel, so the two concerns +/// are composed here. +#[derive(Clone)] +pub struct GatewayInterceptor { + auth: EdgeAuthInterceptor, + trace: EnvTraceContextInterceptor, +} + +impl tonic::service::Interceptor for GatewayInterceptor { + fn call( + &mut self, + request: tonic::Request<()>, + ) -> std::result::Result, tonic::Status> { + let request = self.auth.call(request)?; + self.trace.call(request) + } +} #[derive(Clone, Debug, Default)] pub struct TlsOptions { @@ -451,17 +475,22 @@ pub async fn build_channel(server: &str, tls: &TlsOptions) -> Result { /// Build a gRPC [`OpenShellClient`]. /// -/// When `tls.edge_token` is set, the returned client is wrapped with an -/// interceptor that injects authentication headers on every request. -/// Otherwise, standard mTLS is used (interceptor is a no-op). +/// The returned client carries a [`GatewayInterceptor`] that injects +/// authentication headers (when a token is set) and passively forwards the +/// `TRACEPARENT`/`TRACESTATE` trace context from the environment on every +/// request. With no token and no trace context, the interceptor is a no-op. pub async fn grpc_client(server: &str, tls: &TlsOptions) -> Result { let channel = build_channel(server, tls).await?; let interceptor = interceptor_from_tls(tls)?; Ok(OpenShellClient::with_interceptor(channel, interceptor)) } -fn interceptor_from_tls(tls: &TlsOptions) -> Result { - EdgeAuthInterceptor::new(tls.oidc_token.as_deref(), tls.edge_token.as_deref()) +fn interceptor_from_tls(tls: &TlsOptions) -> Result { + let auth = EdgeAuthInterceptor::new(tls.oidc_token.as_deref(), tls.edge_token.as_deref())?; + Ok(GatewayInterceptor { + auth, + trace: EnvTraceContextInterceptor::from_env(), + }) } /// Gateway replica that owns the sandbox named in `response`, when it says. @@ -510,10 +539,60 @@ pub fn retry_unrouted(replica: Option<&AsciiMetadataValue>, status: &tonic::Stat #[cfg(test)] mod tests { - use super::{owner_replica, retry_unrouted, routed_to, tls_server_name, with_owner_hint}; + use super::{ + TlsOptions, interceptor_from_tls, owner_replica, retry_unrouted, routed_to, + tls_server_name, with_owner_hint, + }; + use crate::TEST_ENV_LOCK; use openshell_core::replica_routing::{ OWNER_REPLICA_HEADER, OWNER_REQUEST_HEADER, ROUTE_REPLICA_HEADER, }; + use tonic::service::Interceptor as _; + + #[test] + fn gateway_interceptor_composes_auth_and_trace_forwarding() { + let _guard = TEST_ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"; + temp_env::with_vars( + [ + ("TRACEPARENT", Some(traceparent)), + ("TRACESTATE", Some("vendor=value")), + ], + || { + let tls = TlsOptions { + oidc_token: Some("test-token".to_string()), + ..TlsOptions::new(None, None, None) + }; + let mut interceptor = interceptor_from_tls(&tls).unwrap(); + + let request = interceptor.call(tonic::Request::new(())).unwrap(); + + assert_eq!( + request + .metadata() + .get("authorization") + .and_then(|value| value.to_str().ok()), + Some("Bearer test-token") + ); + assert_eq!( + request + .metadata() + .get("traceparent") + .and_then(|value| value.to_str().ok()), + Some(traceparent) + ); + assert_eq!( + request + .metadata() + .get("tracestate") + .and_then(|value| value.to_str().ok()), + Some("vendor=value") + ); + }, + ); + } #[test] fn owner_hint_is_requested_explicitly() { diff --git a/crates/openshell-otel/src/lib.rs b/crates/openshell-otel/src/lib.rs index b10c300833..02843b4c93 100644 --- a/crates/openshell-otel/src/lib.rs +++ b/crates/openshell-otel/src/lib.rs @@ -21,9 +21,9 @@ pub use grpc::{ }; pub use ocsf::{OcsfOtlp, OcsfOtlpLayer}; pub use propagation::{ - HeaderMapExtractor, MetadataMapInjector, TRACEPARENT_ENV, TRACESTATE_ENV, - TraceContextInterceptor, current_trace_context_carrier, current_trace_context_environment, - set_parent_from_environment, trace_context_environment, + EnvTraceContextInterceptor, HeaderMapExtractor, MetadataMapInjector, TRACEPARENT_ENV, + TRACESTATE_ENV, TraceContextInterceptor, current_trace_context_carrier, + current_trace_context_environment, set_parent_from_environment, trace_context_environment, }; use opentelemetry::KeyValue; diff --git a/crates/openshell-otel/src/propagation.rs b/crates/openshell-otel/src/propagation.rs index 764e343146..895bc04113 100644 --- a/crates/openshell-otel/src/propagation.rs +++ b/crates/openshell-otel/src/propagation.rs @@ -6,6 +6,7 @@ use std::collections::BTreeMap; use http::HeaderMap; +use opentelemetry::Context; use opentelemetry::propagation::{Extractor, Injector, TextMapPropagator}; use opentelemetry_sdk::propagation::TraceContextPropagator; use tracing_opentelemetry::OpenTelemetrySpanExt as _; @@ -112,18 +113,18 @@ pub fn current_trace_context_environment() -> Vec<(&'static str, String)> { /// Parent `span` under the trace context in `TRACEPARENT` and `TRACESTATE`. /// +/// Reads the environment the same way [`EnvTraceContextInterceptor`] does: +/// both spellings are accepted, whitespace is trimmed, and blank values are +/// treated as unset. +/// /// Leaves `span` unchanged when the environment carries no valid context. pub fn set_parent_from_environment(span: &tracing::Span) { let carrier = [ - ("traceparent", TRACEPARENT_ENV), - ("tracestate", TRACESTATE_ENV), + ("traceparent", read_env(TRACEPARENT_ENV)), + ("tracestate", read_env(TRACESTATE_ENV)), ] .into_iter() - .filter_map(|(field, name)| { - std::env::var(name) - .ok() - .map(|value| (field.to_string(), value)) - }) + .filter_map(|(field, value)| value.map(|value| (field.to_string(), value))) .collect::>(); set_parent_from_carrier(span, &carrier); } @@ -167,9 +168,124 @@ impl tonic::service::Interceptor for TraceContextInterceptor { } } +/// Reads W3C trace-context propagation fields from process environment +/// variables (`TRACEPARENT` and `TRACESTATE`). +/// +/// CI systems (GitHub Actions, GitLab CI, Jenkins OpenTelemetry plugins) +/// export these variables to propagate the pipeline trace to child processes. +/// Both upper-case and lower-case spellings are accepted; blank values are +/// treated as unset. +#[derive(Debug, Clone, Default)] +struct EnvTraceContext { + traceparent: Option, + tracestate: Option, +} + +impl EnvTraceContext { + fn from_env() -> Self { + Self { + traceparent: read_env(TRACEPARENT_ENV), + tracestate: read_env(TRACESTATE_ENV), + } + } +} + +/// Read one environment variable, trimming whitespace and discarding blanks. +fn read_one(name: &str) -> Option { + std::env::var(name) + .ok() + .map(|value| value.trim().to_string()) + .filter(|value| !value.is_empty()) +} + +/// Read an environment variable by its upper-case name, falling back to the +/// lower-case spelling, trimming whitespace and discarding blank values. +/// +/// Each spelling is normalized before the fallback, so a blank upper-case +/// value never shadows a valid lower-case one. +fn read_env(upper: &str) -> Option { + read_one(upper).or_else(|| read_one(&upper.to_ascii_lowercase())) +} + +impl Extractor for EnvTraceContext { + fn get(&self, key: &str) -> Option<&str> { + match key { + "traceparent" => self.traceparent.as_deref(), + "tracestate" => self.tracestate.as_deref(), + _ => None, + } + } + + fn keys(&self) -> Vec<&str> { + let mut keys = Vec::new(); + if self.traceparent.is_some() { + keys.push("traceparent"); + } + if self.tracestate.is_some() { + keys.push("tracestate"); + } + keys + } +} + +/// Passively forwards a W3C trace context captured from the process +/// environment onto outbound tonic requests. +/// +/// Unlike [`TraceContextInterceptor`], this does not require an active +/// OpenTelemetry span or an `SdkTracerProvider`; it reads `TRACEPARENT` +/// (and optionally `TRACESTATE`) once at construction and injects them as +/// gRPC metadata. This lets short-lived clients (e.g. the CLI on a CI runner) +/// extend a pipeline trace to the gateway without any collector configuration. +/// +/// When `TRACEPARENT` is unset or invalid the captured context carries no +/// valid span, so injection is a no-op. +#[derive(Debug, Clone)] +pub struct EnvTraceContextInterceptor { + context: Context, +} + +impl EnvTraceContextInterceptor { + /// Capture the W3C trace context from the process environment. + #[must_use] + pub fn from_env() -> Self { + let env = EnvTraceContext::from_env(); + let context = context_from(&env); + if env.traceparent.is_some() && !span_context_is_valid(&context) { + tracing::debug!("TRACEPARENT is set but invalid; not forwarding trace context"); + } + Self { context } + } +} + +/// Extract a [`Context`] from an [`Extractor`] using a fresh base context, so +/// the result depends only on the extractor and not on any ambient context. +fn context_from(extractor: &impl Extractor) -> Context { + TraceContextPropagator::new().extract_with_context(&Context::new(), extractor) +} + +fn span_context_is_valid(context: &Context) -> bool { + use opentelemetry::trace::TraceContextExt as _; + + context.span().span_context().is_valid() +} + +impl tonic::service::Interceptor for EnvTraceContextInterceptor { + fn call( + &mut self, + mut request: tonic::Request<()>, + ) -> Result, tonic::Status> { + TraceContextPropagator::new().inject_context( + &self.context, + &mut MetadataMapInjector::new(request.metadata_mut()), + ); + Ok(request) + } +} + #[cfg(test)] mod tests { use super::*; + use tonic::service::Interceptor as _; fn env_test_exporter() -> ( opentelemetry_sdk::trace::SdkTracerProvider, @@ -320,4 +436,124 @@ mod tests { Some("value") ); } + + fn interceptor_with(extractor: EnvTraceContext) -> EnvTraceContextInterceptor { + EnvTraceContextInterceptor { + context: context_from(&extractor), + } + } + + fn snapshot_env(names: &[&'static str]) -> Vec<(&'static str, Option)> { + names + .iter() + .map(|name| (*name, std::env::var(name).ok())) + .collect() + } + + #[allow(unsafe_code)] + fn restore_env(saved: &[(&'static str, Option)]) { + for (name, value) in saved { + unsafe { + match value { + Some(value) => std::env::set_var(name, value), + None => std::env::remove_var(name), + } + } + } + } + + #[test] + #[allow(unsafe_code)] + fn env_trace_context_from_env_reads_both_spellings_and_skips_blanks() { + let _tracing_lock = crate::test_lock(); + let saved = snapshot_env(&[TRACEPARENT_ENV, "traceparent", TRACESTATE_ENV, "tracestate"]); + let traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"; + + // Upper-case spelling with surrounding whitespace. + unsafe { + std::env::set_var(TRACEPARENT_ENV, format!(" {traceparent} ")); + std::env::set_var(TRACESTATE_ENV, " vendor=value "); + } + let padded = EnvTraceContext::from_env(); + assert_eq!(padded.traceparent.as_deref(), Some(traceparent)); + assert_eq!(padded.tracestate.as_deref(), Some("vendor=value")); + + // Lower-case-only spelling. + unsafe { + std::env::remove_var(TRACEPARENT_ENV); + std::env::remove_var(TRACESTATE_ENV); + std::env::set_var("traceparent", traceparent); + std::env::set_var("tracestate", "vendor=value"); + } + let lower = EnvTraceContext::from_env(); + assert_eq!(lower.traceparent.as_deref(), Some(traceparent)); + assert_eq!(lower.tracestate.as_deref(), Some("vendor=value")); + + // A blank upper-case value must not shadow a valid lower-case one. + unsafe { + std::env::set_var(TRACEPARENT_ENV, " "); + std::env::remove_var("tracestate"); + } + let shadowed = EnvTraceContext::from_env(); + assert_eq!(shadowed.traceparent.as_deref(), Some(traceparent)); + assert_eq!(shadowed.tracestate, None); + + // Blank values in both spellings are treated as unset. + unsafe { + std::env::set_var("traceparent", ""); + } + let blank = EnvTraceContext::from_env(); + assert_eq!(blank.traceparent, None); + + restore_env(&saved); + } + + #[test] + fn env_interceptor_forwards_valid_traceparent() { + let traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"; + let mut interceptor = interceptor_with(EnvTraceContext { + traceparent: Some(traceparent.to_string()), + tracestate: Some("vendor=value".to_string()), + }); + + let request = interceptor.call(tonic::Request::new(())).unwrap(); + + assert_eq!( + request + .metadata() + .get("traceparent") + .and_then(|value| value.to_str().ok()), + Some(traceparent) + ); + assert_eq!( + request + .metadata() + .get("tracestate") + .and_then(|value| value.to_str().ok()), + Some("vendor=value") + ); + } + + #[test] + fn env_interceptor_is_noop_without_traceparent() { + let mut interceptor = interceptor_with(EnvTraceContext::default()); + + let request = interceptor.call(tonic::Request::new(())).unwrap(); + + assert!(request.metadata().get("traceparent").is_none()); + assert!(request.metadata().get("tracestate").is_none()); + } + + #[test] + fn env_interceptor_is_noop_for_invalid_traceparent() { + let mut interceptor = interceptor_with(EnvTraceContext { + traceparent: Some("not-a-valid-traceparent".to_string()), + tracestate: Some("vendor=value".to_string()), + }); + + let request = interceptor.call(tonic::Request::new(())).unwrap(); + + assert!(request.metadata().get("traceparent").is_none()); + assert!(request.metadata().get("tracestate").is_none()); + } } diff --git a/docs/observability/traces.mdx b/docs/observability/traces.mdx index ef8bd6f5f8..16f8af0347 100644 --- a/docs/observability/traces.mdx +++ b/docs/observability/traces.mdx @@ -69,6 +69,8 @@ Give each installation that reports to the same collector its own name, includin The gateway continues a valid W3C `traceparent` sent by the caller, so a client's trace can include the gateway's spans for its requests. Without one, the gateway starts a new trace. +The CLI forwards `TRACEPARENT` and `TRACESTATE` from its environment on gateway requests from its standard commands. A CI system that exports these variables (GitHub Actions, GitLab CI, Jenkins OpenTelemetry plugins) therefore parents the gateway's spans for the commands it runs, with no collector or provider configuration on the CI runner. + The gateway also passes its trace context to compute drivers, so driver spans appear in the same trace as the gateway request that caused them. Built-in drivers export their spans through the gateway's process. Managed external drivers receive the trace endpoint, gateway name, and compute driver from the gateway. Drivers that you run yourself own their telemetry configuration. ## Next Steps