From 50fca7d404ae4d2926fe0dfc8077344e9fb38d88 Mon Sep 17 00:00:00 2001 From: b7r6 Date: Thu, 22 Oct 2026 17:30:44 +0000 Subject: [PATCH 1/6] Export telemetry over TLS to an https OTLP collector An `https://` collector endpoint stopped NativeLink at startup: `opentelemetry-otlp` was built without any of its TLS features and refuses to build a tonic exporter for HTTPS without one. The only collector a process could export to was one reached in plaintext, so a worker outside the cluster could not send its telemetry to the cluster's collector. Enable `tls-ring` on `opentelemetry-otlp` in nativelink-util. NativeLink already uses ring as its rustls provider, so this adds no crypto backend. An https endpoint, in `OTEL_EXPORTER_OTLP_ENDPOINT` or `NL_OTEL_ENDPOINT`, now gets server-authenticated TLS against the system trust store (tonic's bare `ClientTlsConfig::new()` trusts nothing, so the roots are enabled explicitly; a plaintext endpoint gets no TLS configuration at all, so a host without a certificate bundle still exports). Read the OpenTelemetry specification's certificate variables, which the Rust SDK does not: `OTEL_EXPORTER_OTLP_CERTIFICATE` names a private CA and is then the only CA trusted; `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` and `OTEL_EXPORTER_OTLP_CLIENT_KEY` name the identity for mTLS and have to be set together. The per-signal forms win over the general one. A file that cannot be read, or half an identity, stops the process at startup with an InvalidArgument naming the variable, which is also what a malformed `NL_OTEL_ENDPOINT` now does instead of panicking. Fixes TraceMachina/nativelink#2983 Co-Authored-By: Claude Fable 5.1 --- nativelink-util/Cargo.toml | 1 + nativelink-util/src/telemetry.rs | 386 ++++++++++++++++-- nativelink-util/tests/telemetry_test.rs | 4 +- .../docs/operate/deploy-bare-metal.mdx | 2 +- .../content/docs/operate/observability.mdx | 31 +- .../content/docs/reference/cli-and-env.mdx | 15 +- 6 files changed, 392 insertions(+), 47 deletions(-) diff --git a/nativelink-util/Cargo.toml b/nativelink-util/Cargo.toml index 5ac55bca41b..0163b726dfc 100644 --- a/nativelink-util/Cargo.toml +++ b/nativelink-util/Cargo.toml @@ -34,6 +34,7 @@ opentelemetry-otlp = { version = "0.32.0", default-features = false, features = "grpc-tonic", "logs", "metrics", + "tls-ring", "trace", "zstd-tonic", ] } diff --git a/nativelink-util/src/telemetry.rs b/nativelink-util/src/telemetry.rs index 05a9525ef5a..7a8ad3cb876 100644 --- a/nativelink-util/src/telemetry.rs +++ b/nativelink-util/src/telemetry.rs @@ -21,7 +21,7 @@ use base64::Engine; use base64::prelude::BASE64_STANDARD_NO_PAD; use ginepro::LoadBalancedChannel; use hyper::http::Response; -use nativelink_error::{Code, ResultExt, make_err}; +use nativelink_error::{Code, Error, ResultExt, make_err}; use nativelink_proto::build::bazel::remote::execution::v2::RequestMetadata; use opentelemetry::propagation::{Injector, TextMapCompositePropagator, TextMapPropagator}; use opentelemetry::trace::{TraceContextExt, Tracer, TracerProvider}; @@ -38,6 +38,7 @@ use opentelemetry_sdk::propagation::{BaggagePropagator, TraceContextPropagator}; use opentelemetry_sdk::trace::SdkTracerProvider; use opentelemetry_semantic_conventions::attribute::ENDUSER_ID; use prost::Message; +use tonic::transport::{Certificate, ClientTlsConfig, Identity}; use tracing::debug; use tracing::metadata::LevelFilter; use tracing_opentelemetry::{MetricsLayer, layer}; @@ -110,7 +111,7 @@ fn tracing_stdout_layer() -> impl Layer { /// /// Returns `Err` if logging was already initialized or if the exporters can't /// be initialized. -pub async fn init_tracing() -> Result<(), nativelink_error::Error> { +pub async fn init_tracing() -> Result<(), Error> { static INITIALIZED: OnceLock<()> = OnceLock::new(); if INITIALIZED.get().is_some() { @@ -131,13 +132,16 @@ pub async fn init_tracing() -> Result<(), nativelink_error::Error> { init_propagator(); - let maybe_channel = maybe_load_balanced_channel().await; + let maybe_channel = maybe_load_balanced_channel().await?; // Logs let mut log_exporter_builder = LogExporter::builder().with_tonic(); if let Some(channel) = maybe_channel.clone() { log_exporter_builder = log_exporter_builder.with_channel(channel.into()); } + if let Some(tls_config) = exporter_tls_config("LOGS")? { + log_exporter_builder = log_exporter_builder.with_tls_config(tls_config); + } let otlp_log_layer = OpenTelemetryTracingBridge::new( &SdkLoggerProvider::builder() .with_resource(resource.clone()) @@ -157,6 +161,9 @@ pub async fn init_tracing() -> Result<(), nativelink_error::Error> { if let Some(channel) = maybe_channel.clone() { span_exporter_builder = span_exporter_builder.with_channel(channel.into()); } + if let Some(tls_config) = exporter_tls_config("TRACES")? { + span_exporter_builder = span_exporter_builder.with_tls_config(tls_config); + } let otlp_trace_layer = layer() .with_tracer( SdkTracerProvider::builder() @@ -178,6 +185,9 @@ pub async fn init_tracing() -> Result<(), nativelink_error::Error> { if let Some(channel) = maybe_channel { metric_exporter_builder = metric_exporter_builder.with_channel(channel.into()); } + if let Some(tls_config) = exporter_tls_config("METRICS")? { + metric_exporter_builder = metric_exporter_builder.with_tls_config(tls_config); + } let meter_provider = SdkMeterProvider::builder() .with_resource(resource) .with_periodic_exporter( @@ -209,38 +219,160 @@ pub async fn init_tracing() -> Result<(), nativelink_error::Error> { pub const NL_OTEL_ENDPOINT: &str = "NL_OTEL_ENDPOINT"; /// Creates a load-balanced gRPC channel for the endpoint configured via -/// [`NL_OTEL_ENDPOINT`], or returns `None` if the variable isn't set. +/// [`NL_OTEL_ENDPOINT`], or returns `None` if the variable isn't set. An +/// `https://` endpoint gets TLS, with the material the OpenTelemetry +/// certificate variables name (see [`otlp_tls_config`]) or the system trust +/// store without them. /// /// Public so that integration tests can verify that exporters behave the /// same with and without the load-balanced channel. -pub async fn maybe_load_balanced_channel() -> Option { - match env::var(NL_OTEL_ENDPOINT) { - Ok(endpoint) => { - let url = Url::parse(endpoint.as_str()) - .map_err(|e| { - make_err!(Code::Internal, "Unable to parse endpoint {endpoint}: {e:?}") - }) - .unwrap(); - - let host = url - .host() - .err_tip(|| format!("Unable to get host from endpoint {endpoint}")) - .unwrap(); - let port = url - .port() - .err_tip(|| format!("Unable to get port from endpoint {endpoint}")) - .unwrap(); - - Some( - LoadBalancedChannel::builder((host.to_string(), port)) - .channel() - .await - .map_err(|e| make_err!(Code::Internal, "Invalid hostname '{endpoint}': {e}")) - .unwrap(), +/// +/// # Errors +/// +/// Returns `Err` if the endpoint is not a URL with a host and an explicit +/// port, if its hostname cannot be resolved, or if the certificate variables +/// name files that cannot be read. +pub async fn maybe_load_balanced_channel() -> Result, Error> { + let Ok(endpoint) = env::var(NL_OTEL_ENDPOINT) else { + return Ok(None); + }; + let url = Url::parse(endpoint.as_str()) + .map_err(|e| { + make_err!( + Code::InvalidArgument, + "Unable to parse endpoint {endpoint}: {e:?}" ) + }) + .err_tip(|| format!("In {NL_OTEL_ENDPOINT}"))?; + + let host = url + .host() + .err_tip(|| format!("Unable to get host from endpoint {endpoint}"))?; + let port = url + .port() + .err_tip(|| format!("Unable to get port from endpoint {endpoint}"))?; + + let mut builder = LoadBalancedChannel::builder((host.to_string(), port)); + if url.scheme() == "https" { + let tls_config = otlp_tls_config(None)?.unwrap_or_else(system_roots_tls_config); + builder = builder.with_tls(tls_config); + } + builder + .channel() + .await + .map_err(|e| make_err!(Code::Internal, "Invalid hostname '{endpoint}': {e}")) + .map(Some) +} + +/// The OpenTelemetry specification's variables for the OTLP exporters' TLS +/// material, as the suffix after `OTEL_EXPORTER_OTLP_`: the CA that signs +/// the collector's certificate, and the certificate and key the exporter +/// presents when the collector asks for one. `opentelemetry-otlp` 0.32 does +/// not read these itself, so they are read here. +const OTLP_CERTIFICATE: &str = "CERTIFICATE"; +const OTLP_CLIENT_CERTIFICATE: &str = "CLIENT_CERTIFICATE"; +const OTLP_CLIENT_KEY: &str = "CLIENT_KEY"; + +/// The default OTLP endpoint of the gRPC exporters, as `opentelemetry-otlp` +/// resolves it when no endpoint variable is set. +const OTLP_GRPC_ENDPOINT_DEFAULT: &str = "http://localhost:4317"; + +/// Looks an OTLP variable up the way the specification orders them: the +/// per-signal form (`OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE`) wins over the +/// general one (`OTEL_EXPORTER_OTLP_CERTIFICATE`), and an empty value counts +/// as unset. Returns the name that was found with its value, for messages. +fn otlp_env_var(signal: Option<&str>, suffix: &str) -> Option<(String, String)> { + signal + .map(|signal| format!("OTEL_EXPORTER_OTLP_{signal}_{suffix}")) + .into_iter() + .chain(core::iter::once(format!("OTEL_EXPORTER_OTLP_{suffix}"))) + .find_map(|name| { + env::var(&name) + .ok() + .filter(|value| !value.is_empty()) + .map(|value| (name, value)) + }) +} + +/// Whether the exporter for `signal` will talk to an `https://` endpoint, +/// resolved the way `opentelemetry-otlp` resolves it. +fn otlp_endpoint_is_https(signal: &str) -> bool { + otlp_env_var(Some(signal), "ENDPOINT") + .map_or_else( + || OTLP_GRPC_ENDPOINT_DEFAULT.to_string(), + |(_, value)| value, + ) + .starts_with("https://") +} + +/// A TLS configuration that trusts the system's certificate store: what an +/// `https://` collector gets when no certificate variable names a CA. +/// tonic's bare `ClientTlsConfig::new()` trusts nothing at all. +fn system_roots_tls_config() -> ClientTlsConfig { + ClientTlsConfig::new().with_enabled_roots() +} + +fn read_pem((name, path): &(String, String)) -> Result, Error> { + std::fs::read(path).map_err(|e| { + make_err!( + Code::InvalidArgument, + "Could not read {path}, which {name} names: {e}" + ) + }) +} + +/// The TLS configuration the OpenTelemetry certificate variables describe +/// for `signal` (`LOGS`, `TRACES` or `METRICS`; `None` reads only the +/// general variables), or `None` when none of them is set. +/// +/// `OTEL_EXPORTER_OTLP_CERTIFICATE` names the CA to verify the collector +/// with, and then only that CA is trusted; without it the system trust store +/// is. `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` and +/// `OTEL_EXPORTER_OTLP_CLIENT_KEY` name the identity to present for mTLS +/// and have to be set together. Each has a per-signal form that wins over +/// the general one. The files are read here, once, at startup. +/// +/// # Errors +/// +/// Returns `Err` if a named file cannot be read, or if only one of the +/// client certificate and key is set. +pub fn otlp_tls_config(signal: Option<&str>) -> Result, Error> { + let ca = otlp_env_var(signal, OTLP_CERTIFICATE); + let client_certificate = otlp_env_var(signal, OTLP_CLIENT_CERTIFICATE); + let client_key = otlp_env_var(signal, OTLP_CLIENT_KEY); + if ca.is_none() && client_certificate.is_none() && client_key.is_none() { + return Ok(None); + } + let mut config = match &ca { + Some(ca) => ClientTlsConfig::new().ca_certificate(Certificate::from_pem(read_pem(ca)?)), + None => system_roots_tls_config(), + }; + match (&client_certificate, &client_key) { + (Some(certificate), Some(key)) => { + config = config.identity(Identity::from_pem(read_pem(certificate)?, read_pem(key)?)); + } + (None, None) => {} + (Some((name, _)), None) | (None, Some((name, _))) => { + return Err(make_err!( + Code::InvalidArgument, + "{name} is set without its pair: a client certificate and its key have to be \ + set together, as OTEL_EXPORTER_OTLP_{OTLP_CLIENT_CERTIFICATE} and \ + OTEL_EXPORTER_OTLP_{OTLP_CLIENT_KEY} or their per-signal forms" + )); } - Err(_) => None, } + Ok(Some(config)) +} + +/// The TLS configuration for the exporter of `signal` when it builds its +/// own channel: the certificate variables' when any is set, the system +/// trust store for an `https://` endpoint without them, and nothing for a +/// plaintext endpoint. Loading the system store is deferred to the moment +/// it is needed because it fails on a host without a certificate bundle, +/// which a plaintext deployment need not have. +fn exporter_tls_config(signal: &str) -> Result, Error> { + Ok(otlp_tls_config(Some(signal))? + .or_else(|| otlp_endpoint_is_https(signal).then(system_roots_tls_config))) } /// This is the header that bazel sends when using the `--remote_header` flag. /// TODO(palfrey): Bazel supports other headers, and we should optimize their usage. @@ -505,7 +637,10 @@ mod tests { // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. unsafe { env::remove_var(NL_OTEL_ENDPOINT) }; assert!( - maybe_load_balanced_channel().await.is_none(), + maybe_load_balanced_channel() + .await + .expect("an unset endpoint is not an error") + .is_none(), "Expected None when {NL_OTEL_ENDPOINT} is unset" ); } @@ -526,11 +661,200 @@ mod tests { // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. unsafe { env::remove_var(NL_OTEL_ENDPOINT) }; assert!( - result.is_some(), + result.expect("a valid endpoint builds").is_some(), "Expected Some(channel) when {NL_OTEL_ENDPOINT} points to a valid URL" ); } + /// Clears every OTLP variable a TLS test may set, before and after it. + fn clear_otlp_tls_env() { + for signal in ["", "LOGS_", "TRACES_", "METRICS_"] { + for suffix in [ + "ENDPOINT", + "CERTIFICATE", + "CLIENT_CERTIFICATE", + "CLIENT_KEY", + ] { + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::remove_var(format!("OTEL_EXPORTER_OTLP_{signal}{suffix}")) }; + } + } + } + + /// Writes dummy PEM bytes to a file in `dir`; tonic only stores them + /// until a connection is made, so the content need not parse. + fn pem_file(dir: &tempfile::TempDir, name: &str) -> String { + let path = dir.path().join(name); + std::fs::write(&path, format!("-----BEGIN {name}-----\n")).unwrap(); + path.to_str().unwrap().to_string() + } + + // The exporter used to refuse an https endpoint outright ("uses HTTPS + // but no TLS feature is enabled"); this fails without `tls-ring` on + // `opentelemetry-otlp`. + #[serial(env)] + #[nativelink_test("crate")] + async fn https_endpoint_builds_an_exporter() { + clear_otlp_tls_env(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", "https://localhost:4317") }; + let mut builder = SpanExporter::builder().with_tonic(); + if let Some(tls_config) = exporter_tls_config("TRACES").unwrap() { + builder = builder.with_tls_config(tls_config); + } + let result = builder.with_protocol(Protocol::Grpc).build(); + clear_otlp_tls_env(); + assert!( + result.is_ok(), + "An https endpoint must build an exporter: {:?}", + result.err() + ); + } + + #[serial(env)] + #[nativelink_test("crate")] + async fn tls_config_absent_without_variables() { + clear_otlp_tls_env(); + assert!(otlp_tls_config(Some("TRACES")).unwrap().is_none()); + assert!(otlp_tls_config(None).unwrap().is_none()); + // A plaintext endpoint gets no TLS configuration at all, so a host + // without a certificate store still exports. + assert!(exporter_tls_config("TRACES").unwrap().is_none()); + // An empty value is unset, as the specification says. + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CERTIFICATE", "") }; + let result = otlp_tls_config(Some("TRACES")); + clear_otlp_tls_env(); + assert!(result.unwrap().is_none()); + } + + #[serial(env)] + #[nativelink_test("crate")] + async fn tls_config_present_with_ca_certificate() { + clear_otlp_tls_env(); + let dir = tempfile::tempdir().unwrap(); + let ca = pem_file(&dir, "ca"); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CERTIFICATE", &ca) }; + let general = otlp_tls_config(None); + let per_signal = otlp_tls_config(Some("METRICS")); + // The variables apply to a plaintext endpoint too; the scheme is + // the exporter's to judge. + let exporter = exporter_tls_config("METRICS"); + clear_otlp_tls_env(); + assert!(general.unwrap().is_some()); + assert!(per_signal.unwrap().is_some()); + assert!(exporter.unwrap().is_some()); + } + + #[serial(env)] + #[nativelink_test("crate")] + async fn tls_config_with_client_identity() { + clear_otlp_tls_env(); + let dir = tempfile::tempdir().unwrap(); + let certificate = pem_file(&dir, "client"); + let key = pem_file(&dir, "key"); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE", &certificate) }; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CLIENT_KEY", &key) }; + let result = otlp_tls_config(Some("LOGS")); + clear_otlp_tls_env(); + assert!(result.unwrap().is_some()); + } + + #[serial(env)] + #[nativelink_test("crate")] + async fn per_signal_variable_wins_over_the_general_one() { + clear_otlp_tls_env(); + let dir = tempfile::tempdir().unwrap(); + let ca = pem_file(&dir, "ca"); + let missing = dir.path().join("missing").to_str().unwrap().to_string(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CERTIFICATE", &missing) }; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE", &ca) }; + let traces = otlp_tls_config(Some("TRACES")); + let metrics = otlp_tls_config(Some("METRICS")); + let general = otlp_tls_config(None); + clear_otlp_tls_env(); + assert!(traces.unwrap().is_some(), "TRACES names a readable file"); + let metrics_err = metrics.expect_err("METRICS falls back to the general, missing file"); + assert_eq!(metrics_err.code, Code::InvalidArgument); + assert!( + metrics_err + .to_string() + .contains("OTEL_EXPORTER_OTLP_CERTIFICATE"), + "{metrics_err}" + ); + assert_eq!( + general.expect_err("the general file is missing").code, + Code::InvalidArgument + ); + } + + #[serial(env)] + #[nativelink_test("crate")] + async fn client_certificate_without_key_is_rejected() { + clear_otlp_tls_env(); + let dir = tempfile::tempdir().unwrap(); + let certificate = pem_file(&dir, "client"); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE", &certificate) }; + let without_key = otlp_tls_config(Some("LOGS")); + clear_otlp_tls_env(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY", &certificate) }; + let without_certificate = otlp_tls_config(Some("LOGS")); + clear_otlp_tls_env(); + for result in [without_key, without_certificate] { + let err = result.expect_err("half an identity is an error"); + assert_eq!(err.code, Code::InvalidArgument); + assert!( + err.to_string().contains("OTEL_EXPORTER_OTLP_CLIENT_KEY"), + "{err}" + ); + } + } + + #[serial(env)] + #[nativelink_test("crate")] + async fn missing_certificate_file_is_rejected() { + clear_otlp_tls_env(); + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("missing.pem").to_str().unwrap().to_string(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CERTIFICATE", &missing) }; + let result = otlp_tls_config(Some("METRICS")); + clear_otlp_tls_env(); + let err = result.expect_err("a missing file is an error"); + assert_eq!(err.code, Code::InvalidArgument); + assert!(err.to_string().contains("missing.pem"), "{err}"); + assert!( + err.to_string().contains("OTEL_EXPORTER_OTLP_CERTIFICATE"), + "{err}" + ); + } + + #[serial(env)] + #[nativelink_test("crate")] + async fn https_endpoint_without_variables_trusts_the_system_store() { + clear_otlp_tls_env(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_LOGS_ENDPOINT", "https://collector:4317") }; + let logs = exporter_tls_config("LOGS"); + let traces = exporter_tls_config("TRACES"); + clear_otlp_tls_env(); + assert!( + logs.unwrap().is_some(), + "an https endpoint gets a TLS config" + ); + assert!( + traces.unwrap().is_none(), + "the default endpoint is plaintext" + ); + } + #[test] fn inject_context_writes_baggage_into_request_metadata() { use opentelemetry::baggage::BaggageExt; diff --git a/nativelink-util/tests/telemetry_test.rs b/nativelink-util/tests/telemetry_test.rs index 821dc101222..843eb558e76 100644 --- a/nativelink-util/tests/telemetry_test.rs +++ b/nativelink-util/tests/telemetry_test.rs @@ -437,7 +437,7 @@ async fn metrics_are_tracked_without_otel_endpoint() -> Result<(), Box Result<(), Box The value is parsed as a URL and the host and port are pulled out - individually, and each step `unwrap`s. A value with no scheme, or with no - explicit port, yields `None` for the port and **panics the process during - startup**, before it binds a socket. + individually. A value with no scheme, or with no explicit port, **stops the + process during startup** with an error, before it binds a socket, rather + than falling back to a default. `http://otel-collector:4317` is correct. `otel-collector:4317` and - `http://otel-collector` both crash the binary. + `http://otel-collector` both stop the binary. @@ -85,12 +85,28 @@ a cluster, because it re-resolves the collector's DNS as pods move. Either way the transport is OTLP over gRPC: the binary is built with only the gRPC exporter, so `OTEL_EXPORTER_OTLP_PROTOCOL` cannot switch it to HTTP. +An `https://` endpoint, in either variable, gets TLS. Without further +configuration the collector's certificate is checked against the host's +system trust store, which is what you want for a collector behind a +public certificate. For a collector with a private CA, set +`OTEL_EXPORTER_OTLP_CERTIFICATE` to the PEM file of that CA and only it is +trusted. For a collector that asks the client for a certificate (mTLS), set +`OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` and `OTEL_EXPORTER_OTLP_CLIENT_KEY` +together. These are the OpenTelemetry specification's own variable names, +so a worker outside the cluster (a macOS worker, say) reaches the cluster's +collector with the same configuration any other OTLP client would use. The +per-signal forms (`OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE` and so on) win over +the general one, as the specification says. + The rest of the surface is small enough to state in full: | Variable | Default | What it does here | | --- | --- | --- | -| `NL_OTEL_ENDPOINT` | unset | One shared gRPC channel for logs, traces and metrics. Panics on a malformed value. | -| `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4317` | Used only when `NL_OTEL_ENDPOINT` is unset. | +| `NL_OTEL_ENDPOINT` | unset | One shared gRPC channel for logs, traces and metrics. Stops the process on a malformed value. An `https://` value gets TLS. | +| `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4317` | Used only when `NL_OTEL_ENDPOINT` is unset. An `https://` value gets TLS. | +| `OTEL_EXPORTER_OTLP_CERTIFICATE` | unset | Path to the PEM CA that signs the collector's certificate. Set, it is the only CA trusted; unset, the system trust store is. | +| `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` | unset | Path to the PEM certificate the exporter presents to the collector for mTLS. Requires `OTEL_EXPORTER_OTLP_CLIENT_KEY`; one without the other stops the process. | +| `OTEL_EXPORTER_OTLP_CLIENT_KEY` | unset | Path to the PEM private key for that certificate. | | `OTEL_EXPORTER_OTLP_COMPRESSION` | unset | `gzip` or `zstd` on the gRPC exporters; the shipped Kubernetes manifests set `zstd`. | | `OTEL_METRIC_EXPORT_INTERVAL` | `60000` (ms) | How often metrics flush. NativeLink sets no interval, so the SDK default of one minute applies. This is the first hop in the [reaction-time budget](/operate/autoscaling-reference#the-reaction-time-budget); lower it to 10000 before you scale on anything. | | `RUST_LOG` | `info` | Applies to the stdout layer **and** to all three OTLP layers. `hyper`, `tonic`, `h2`, `reqwest` and `tower` are forced off afterwards and cannot be re-enabled. | @@ -460,7 +476,8 @@ not your pipeline. | Symptom | Cause | Fix | | --- | --- | --- | -| Process panics at startup with a URL error | `NL_OTEL_ENDPOINT` has no scheme or no explicit port | Use `http://host:4317` | +| Process stops at startup with a URL error | `NL_OTEL_ENDPOINT` has no scheme or no explicit port | Use `http://host:4317` | +| Process stops at startup naming an `OTEL_EXPORTER_OTLP_*` variable | A certificate variable names a file the process cannot read, or a client certificate is set without its key | Fix the path, or set `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` and `OTEL_EXPORTER_OTLP_CLIENT_KEY` together | | No metrics anywhere, process healthy | Nothing is pushing; check `otelcol_receiver_accepted_metric_points` on `:8888` | Verify the endpoint and that it is set on the process you are looking for | | Metrics appear a minute late | `OTEL_METRIC_EXPORT_INTERVAL` defaults to 60 s | Set `10000` | | `nativelink_execution_*` present, `nativelink_cache_*` missing | No store is wrapped | [Turn on cache metrics](#turn-on-cache-metrics) | diff --git a/web/apps/docs/content/docs/reference/cli-and-env.mdx b/web/apps/docs/content/docs/reference/cli-and-env.mdx index 482d5437f9c..ff5b2b25abd 100644 --- a/web/apps/docs/content/docs/reference/cli-and-env.mdx +++ b/web/apps/docs/content/docs/reference/cli-and-env.mdx @@ -50,19 +50,22 @@ They are the only variables that change the binary's own behaviour. | --- | --- | --- | | `NL_LOG` | `pretty` | Format of the stdout log layer. `compact` and `json` are recognised; **any other value silently falls back to `pretty`**, including a typo like `Json`. | | `RUST_LOG` | `info` | Log filter, parsed by `tracing_subscriber`'s `EnvFilter` in lossy mode; an unparseable directive is dropped rather than fatal. Applies to the stdout layer *and* to the OTLP log, trace and metric layers. `hyper`, `tonic`, `h2`, `reqwest` and `tower` are forced off afterwards and cannot be re-enabled from here. | -| `NL_OTEL_ENDPOINT` | not set | When set, all three OTLP exporters share one client-side load-balanced gRPC channel pointed at this endpoint. When unset, each exporter is built with the SDK's own tonic defaults instead. | -| `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4317` | Standard OpenTelemetry SDK variable. Applies only when `NL_OTEL_ENDPOINT` is unset; an explicit channel wins over the SDK's endpoint resolution. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_ENDPOINT` variables behave the same way. | +| `NL_OTEL_ENDPOINT` | not set | When set, all three OTLP exporters share one client-side load-balanced gRPC channel pointed at this endpoint. When unset, each exporter is built with the SDK's own tonic defaults instead. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. | +| `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4317` | Standard OpenTelemetry SDK variable. Applies only when `NL_OTEL_ENDPOINT` is unset; an explicit channel wins over the SDK's endpoint resolution. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_ENDPOINT` variables behave the same way. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. | +| `OTEL_EXPORTER_OTLP_CERTIFICATE` | not set | Standard OpenTelemetry variable, read by NativeLink itself because the Rust SDK does not: the path to a PEM file holding the CA that signs the collector's certificate. Set, it is the only CA trusted; unset, the host's system trust store is. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_CERTIFICATE` forms win over it. An empty value counts as unset; a path that cannot be read stops the process at startup. | +| `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` | not set | The path to the PEM certificate the exporter presents when the collector asks for one (mTLS). Has to be set together with `OTEL_EXPORTER_OTLP_CLIENT_KEY`; one without the other stops the process at startup. Signal-specific forms as above. | +| `OTEL_EXPORTER_OTLP_CLIENT_KEY` | not set | The path to the PEM private key for `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE`. Signal-specific forms as above. | | `OTEL_METRIC_EXPORT_INTERVAL` | `60000` (ms) | How often the periodic metric reader flushes to the collector. NativeLink sets no interval of its own, so the SDK default of one minute applies unless you override it here. This is the first hop in the [reaction-time budget](/operate/autoscaling-reference). | The endpoint is parsed as a URL and then the host and port are pulled out - individually, and each step `unwrap`s. A value with no scheme, or with no - explicit port, yields `None` for the port and **panics the process during - startup** rather than falling back to a default. + individually. A value with no scheme, or with no explicit port, **stops the + process during startup** with an error rather than falling back to a + default. `http://otel-collector:4317` is correct. `otel-collector:4317` and - `http://otel-collector` both crash the binary before it binds a socket. + `http://otel-collector` both stop the binary before it binds a socket. From 3be5a0eed2e00b2700f30340e4c05b9d6df4f409 Mon Sep 17 00:00:00 2001 From: b7r6 Date: Thu, 22 Oct 2026 18:22:05 +0000 Subject: [PATCH 2/6] Give a signal with its own certificate variable its own NL_OTEL_ENDPOINT channel The shared channel `NL_OTEL_ENDPOINT` builds was made with the general certificate variables only, and an exporter handed a channel ignores its own `with_tls_config`, so `OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE` and the other per-signal variables worked with `OTEL_EXPORTER_OTLP_ENDPOINT` but not with `NL_OTEL_ENDPOINT`: the handshake failed with an unknown CA. Keep the one shared channel as the common case, and when any per-signal certificate variable is set build one channel per signal with that signal's resolution of the variables. `maybe_load_balanced_channel` stays as the general-variables channel for the integration tests. Co-Authored-By: Claude Fable 5.1 --- nativelink-util/src/telemetry.rs | 186 ++++++++++++++++-- .../content/docs/operate/observability.mdx | 2 +- .../content/docs/reference/cli-and-env.mdx | 2 +- 3 files changed, 177 insertions(+), 13 deletions(-) diff --git a/nativelink-util/src/telemetry.rs b/nativelink-util/src/telemetry.rs index 7a8ad3cb876..213ecf0786b 100644 --- a/nativelink-util/src/telemetry.rs +++ b/nativelink-util/src/telemetry.rs @@ -132,11 +132,11 @@ pub async fn init_tracing() -> Result<(), Error> { init_propagator(); - let maybe_channel = maybe_load_balanced_channel().await?; + let channels = load_balanced_channels().await?; // Logs let mut log_exporter_builder = LogExporter::builder().with_tonic(); - if let Some(channel) = maybe_channel.clone() { + if let Some(channel) = channels.logs { log_exporter_builder = log_exporter_builder.with_channel(channel.into()); } if let Some(tls_config) = exporter_tls_config("LOGS")? { @@ -158,7 +158,7 @@ pub async fn init_tracing() -> Result<(), Error> { // Traces let mut span_exporter_builder = SpanExporter::builder().with_tonic(); - if let Some(channel) = maybe_channel.clone() { + if let Some(channel) = channels.traces { span_exporter_builder = span_exporter_builder.with_channel(channel.into()); } if let Some(tls_config) = exporter_tls_config("TRACES")? { @@ -182,7 +182,7 @@ pub async fn init_tracing() -> Result<(), Error> { // Metrics let mut metric_exporter_builder = MetricExporter::builder().with_tonic(); - if let Some(channel) = maybe_channel { + if let Some(channel) = channels.metrics { metric_exporter_builder = metric_exporter_builder.with_channel(channel.into()); } if let Some(tls_config) = exporter_tls_config("METRICS")? { @@ -218,21 +218,94 @@ pub async fn init_tracing() -> Result<(), Error> { /// Environment variable pointing OTLP exporters at a load-balanced endpoint. pub const NL_OTEL_ENDPOINT: &str = "NL_OTEL_ENDPOINT"; +/// The OTLP signals, as the OpenTelemetry specification spells them in the +/// per-signal variables (`OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE`). +const OTLP_SIGNALS: [&str; 3] = ["LOGS", "TRACES", "METRICS"]; + +/// The load-balanced channel each exporter sends on when +/// [`NL_OTEL_ENDPOINT`] is set, and `None` for each when it is not. +/// +/// The common case is one channel shared by all three. A channel carries +/// its TLS configuration, and an exporter given a channel ignores its own +/// `with_tls_config`, so a signal with a certificate variable of its own +/// gets a channel of its own, built with that signal's material. +#[derive(Debug, Default)] +pub struct SignalChannels { + pub logs: Option, + pub traces: Option, + pub metrics: Option, +} + +/// Whether any per-signal certificate variable is set and non-empty, which +/// is when the signals need channels of their own. +fn any_per_signal_tls_variable_set() -> bool { + OTLP_SIGNALS.iter().any(|signal| { + [OTLP_CERTIFICATE, OTLP_CLIENT_CERTIFICATE, OTLP_CLIENT_KEY] + .iter() + .any(|suffix| { + env::var(format!("OTEL_EXPORTER_OTLP_{signal}_{suffix}")) + .is_ok_and(|value| !value.is_empty()) + }) + }) +} + +/// Builds the channels the exporters send on when [`NL_OTEL_ENDPOINT`] is +/// set: one shared channel with the general certificate variables, or, when +/// any per-signal certificate variable is set, one per signal with that +/// signal's resolution of the variables. Without the endpoint variable +/// every field is `None` and each exporter builds its own connection. +/// +/// # Errors +/// +/// As [`load_balanced_channel_for`]. +pub async fn load_balanced_channels() -> Result { + if env::var(NL_OTEL_ENDPOINT).is_err() { + return Ok(SignalChannels::default()); + } + if !any_per_signal_tls_variable_set() { + let shared = load_balanced_channel_for(None).await?; + return Ok(SignalChannels { + logs: shared.clone(), + traces: shared.clone(), + metrics: shared, + }); + } + Ok(SignalChannels { + logs: load_balanced_channel_for(Some("LOGS")).await?, + traces: load_balanced_channel_for(Some("TRACES")).await?, + metrics: load_balanced_channel_for(Some("METRICS")).await?, + }) +} + +/// Creates a load-balanced gRPC channel for the endpoint configured via +/// [`NL_OTEL_ENDPOINT`] with the general certificate variables, or returns +/// `None` if the variable isn't set. This is [`load_balanced_channel_for`] +/// with no signal, kept for the integration tests that verify exporters +/// behave the same with and without the load-balanced channel; +/// [`init_tracing`] goes through [`load_balanced_channels`]. +/// +/// # Errors +/// +/// As [`load_balanced_channel_for`]. +pub async fn maybe_load_balanced_channel() -> Result, Error> { + load_balanced_channel_for(None).await +} + /// Creates a load-balanced gRPC channel for the endpoint configured via /// [`NL_OTEL_ENDPOINT`], or returns `None` if the variable isn't set. An /// `https://` endpoint gets TLS, with the material the OpenTelemetry -/// certificate variables name (see [`otlp_tls_config`]) or the system trust -/// store without them. -/// -/// Public so that integration tests can verify that exporters behave the -/// same with and without the load-balanced channel. +/// certificate variables name for `signal` (see [`otlp_tls_config`]) or the +/// system trust store without them; a plaintext endpoint gets no TLS +/// configuration at all. /// /// # Errors /// /// Returns `Err` if the endpoint is not a URL with a host and an explicit /// port, if its hostname cannot be resolved, or if the certificate variables /// name files that cannot be read. -pub async fn maybe_load_balanced_channel() -> Result, Error> { +pub async fn load_balanced_channel_for( + signal: Option<&str>, +) -> Result, Error> { let Ok(endpoint) = env::var(NL_OTEL_ENDPOINT) else { return Ok(None); }; @@ -254,7 +327,7 @@ pub async fn maybe_load_balanced_channel() -> Result let mut builder = LoadBalancedChannel::builder((host.to_string(), port)); if url.scheme() == "https" { - let tls_config = otlp_tls_config(None)?.unwrap_or_else(system_roots_tls_config); + let tls_config = otlp_tls_config(signal)?.unwrap_or_else(system_roots_tls_config); builder = builder.with_tls(tls_config); } builder @@ -836,6 +909,97 @@ mod tests { ); } + // A per-signal certificate variable has to reach the channel the + // exporter is handed: an exporter given a channel ignores its own TLS + // configuration, so a shared channel built from the general variables + // alone failed the handshake with "unknown CA" for a signal whose CA + // is set per signal. + #[serial(env)] + #[nativelink_test("crate")] + async fn per_signal_certificate_reaches_the_load_balanced_channel() { + clear_otlp_tls_env(); + let dir = tempfile::tempdir().unwrap(); + let ca = pem_file(&dir, "ca"); + let missing = dir.path().join("missing").to_str().unwrap().to_string(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var(NL_OTEL_ENDPOINT, "https://localhost:4317") }; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CERTIFICATE", &missing) }; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE", &ca) }; + assert!(any_per_signal_tls_variable_set()); + let traces = load_balanced_channel_for(Some("TRACES")).await; + let metrics = load_balanced_channel_for(Some("METRICS")).await; + let all = load_balanced_channels().await; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::remove_var(NL_OTEL_ENDPOINT) }; + clear_otlp_tls_env(); + + // METRICS falls back to the general variable, whose file is + // missing; that error precedes any name resolution, so it holds + // with or without DNS, and so does the whole-set build that + // includes METRICS. + for result in [metrics, all.map(|channels| channels.metrics)] { + let err = result.expect_err("the general CA file is missing"); + assert_eq!(err.code, Code::InvalidArgument); + assert!( + err.to_string().contains("OTEL_EXPORTER_OTLP_CERTIFICATE"), + "{err}" + ); + } + // TRACES reads its own file; building the channel then resolves + // the host, which needs DNS. + if dns_configured() { + assert!( + traces.expect("TRACES names a readable CA").is_some(), + "Expected a channel for TRACES when {NL_OTEL_ENDPOINT} is set" + ); + } else { + eprintln!( + "Skipping the TRACES channel assertion: no DNS configuration \ + available (e.g. sandboxed Nix build)" + ); + } + } + + // Without a per-signal variable the three signals share one channel, + // and without the endpoint variable they get none. + #[serial(env)] + #[nativelink_test("crate")] + async fn signals_share_one_channel_without_per_signal_variables() { + clear_otlp_tls_env(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::remove_var(NL_OTEL_ENDPOINT) }; + assert!(!any_per_signal_tls_variable_set()); + let channels = load_balanced_channels() + .await + .expect("an unset endpoint is not an error"); + assert!(channels.logs.is_none() && channels.traces.is_none() && channels.metrics.is_none()); + + // An empty per-signal value is unset, so it does not split the + // channel either. + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_METRICS_CLIENT_KEY", "") }; + let split = any_per_signal_tls_variable_set(); + clear_otlp_tls_env(); + assert!(!split); + + if !dns_configured() { + eprintln!( + "Skipping the shared channel assertion: no DNS configuration \ + available (e.g. sandboxed Nix build)" + ); + return; + } + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var(NL_OTEL_ENDPOINT, "http://localhost:4317") }; + let channels = load_balanced_channels().await; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::remove_var(NL_OTEL_ENDPOINT) }; + let channels = channels.expect("a valid endpoint builds"); + assert!(channels.logs.is_some() && channels.traces.is_some() && channels.metrics.is_some()); + } + #[serial(env)] #[nativelink_test("crate")] async fn https_endpoint_without_variables_trusts_the_system_store() { diff --git a/web/apps/docs/content/docs/operate/observability.mdx b/web/apps/docs/content/docs/operate/observability.mdx index 38d1ed1bee3..c7c1ceb3dcc 100644 --- a/web/apps/docs/content/docs/operate/observability.mdx +++ b/web/apps/docs/content/docs/operate/observability.mdx @@ -102,7 +102,7 @@ The rest of the surface is small enough to state in full: | Variable | Default | What it does here | | --- | --- | --- | -| `NL_OTEL_ENDPOINT` | unset | One shared gRPC channel for logs, traces and metrics. Stops the process on a malformed value. An `https://` value gets TLS. | +| `NL_OTEL_ENDPOINT` | unset | One shared gRPC channel for logs, traces and metrics. Stops the process on a malformed value. An `https://` value gets TLS; a per-signal certificate variable gives that signal a channel of its own, built with its material. | | `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4317` | Used only when `NL_OTEL_ENDPOINT` is unset. An `https://` value gets TLS. | | `OTEL_EXPORTER_OTLP_CERTIFICATE` | unset | Path to the PEM CA that signs the collector's certificate. Set, it is the only CA trusted; unset, the system trust store is. | | `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` | unset | Path to the PEM certificate the exporter presents to the collector for mTLS. Requires `OTEL_EXPORTER_OTLP_CLIENT_KEY`; one without the other stops the process. | diff --git a/web/apps/docs/content/docs/reference/cli-and-env.mdx b/web/apps/docs/content/docs/reference/cli-and-env.mdx index ff5b2b25abd..d4a26f9a1d4 100644 --- a/web/apps/docs/content/docs/reference/cli-and-env.mdx +++ b/web/apps/docs/content/docs/reference/cli-and-env.mdx @@ -50,7 +50,7 @@ They are the only variables that change the binary's own behaviour. | --- | --- | --- | | `NL_LOG` | `pretty` | Format of the stdout log layer. `compact` and `json` are recognised; **any other value silently falls back to `pretty`**, including a typo like `Json`. | | `RUST_LOG` | `info` | Log filter, parsed by `tracing_subscriber`'s `EnvFilter` in lossy mode; an unparseable directive is dropped rather than fatal. Applies to the stdout layer *and* to the OTLP log, trace and metric layers. `hyper`, `tonic`, `h2`, `reqwest` and `tower` are forced off afterwards and cannot be re-enabled from here. | -| `NL_OTEL_ENDPOINT` | not set | When set, all three OTLP exporters share one client-side load-balanced gRPC channel pointed at this endpoint. When unset, each exporter is built with the SDK's own tonic defaults instead. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. | +| `NL_OTEL_ENDPOINT` | not set | When set, all three OTLP exporters share one client-side load-balanced gRPC channel pointed at this endpoint. When unset, each exporter is built with the SDK's own tonic defaults instead. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. A per-signal certificate variable gives that signal a channel of its own, built with its material, instead of the shared one. | | `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4317` | Standard OpenTelemetry SDK variable. Applies only when `NL_OTEL_ENDPOINT` is unset; an explicit channel wins over the SDK's endpoint resolution. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_ENDPOINT` variables behave the same way. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. | | `OTEL_EXPORTER_OTLP_CERTIFICATE` | not set | Standard OpenTelemetry variable, read by NativeLink itself because the Rust SDK does not: the path to a PEM file holding the CA that signs the collector's certificate. Set, it is the only CA trusted; unset, the host's system trust store is. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_CERTIFICATE` forms win over it. An empty value counts as unset; a path that cannot be read stops the process at startup. | | `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` | not set | The path to the PEM certificate the exporter presents when the collector asks for one (mTLS). Has to be set together with `OTEL_EXPORTER_OTLP_CLIENT_KEY`; one without the other stops the process at startup. Signal-specific forms as above. | From 2d0222da5e73223fddafcf9419f05efe7eceeaa7 Mon Sep 17 00:00:00 2001 From: b7r6 Date: Thu, 22 Oct 2026 18:48:32 +0000 Subject: [PATCH 3/6] Build no TLS for a plaintext OTLP signal and accept an uppercase https scheme A plaintext signal got a TLS configuration whenever a certificate variable was set, and the exporter still hands that to tonic, which loads the trust roots as it builds the connector and fails on a host without a certificate bundle: a deployment sending metrics over TLS with an identity and logs in plaintext died building the log exporter. A plaintext endpoint now gets no configuration whatever the variables say; only an https endpoint gets one. The scheme check was a case-sensitive `starts_with("https://")`, while the SDK accepts `HTTPS://` too, so such an endpoint started with an empty trust store. Parse the value as a URL and compare the scheme; a value that does not parse counts as plaintext and is left for the exporter to reject. Co-Authored-By: Claude Fable 5.1 --- nativelink-util/src/telemetry.rs | 101 +++++++++++++++++++++++++------ 1 file changed, 84 insertions(+), 17 deletions(-) diff --git a/nativelink-util/src/telemetry.rs b/nativelink-util/src/telemetry.rs index 213ecf0786b..13ed7eb6ec3 100644 --- a/nativelink-util/src/telemetry.rs +++ b/nativelink-util/src/telemetry.rs @@ -368,14 +368,17 @@ fn otlp_env_var(signal: Option<&str>, suffix: &str) -> Option<(String, String)> } /// Whether the exporter for `signal` will talk to an `https://` endpoint, -/// resolved the way `opentelemetry-otlp` resolves it. +/// resolved the way `opentelemetry-otlp` resolves it. The scheme is +/// compared case-insensitively, as the SDK accepts `HTTPS://` too; a value +/// that does not parse as a URL counts as plaintext and is left for the +/// exporter to reject. (The `NL_OTEL_ENDPOINT` path parses its value with +/// `Url` as well, so it judges the scheme the same way.) fn otlp_endpoint_is_https(signal: &str) -> bool { - otlp_env_var(Some(signal), "ENDPOINT") - .map_or_else( - || OTLP_GRPC_ENDPOINT_DEFAULT.to_string(), - |(_, value)| value, - ) - .starts_with("https://") + let endpoint = otlp_env_var(Some(signal), "ENDPOINT").map_or_else( + || OTLP_GRPC_ENDPOINT_DEFAULT.to_string(), + |(_, value)| value, + ); + Url::parse(&endpoint).is_ok_and(|url| url.scheme() == "https") } /// A TLS configuration that trusts the system's certificate store: what an @@ -438,14 +441,20 @@ pub fn otlp_tls_config(signal: Option<&str>) -> Result, } /// The TLS configuration for the exporter of `signal` when it builds its -/// own channel: the certificate variables' when any is set, the system -/// trust store for an `https://` endpoint without them, and nothing for a -/// plaintext endpoint. Loading the system store is deferred to the moment -/// it is needed because it fails on a host without a certificate bundle, -/// which a plaintext deployment need not have. +/// own channel: nothing for a plaintext endpoint, whatever the certificate +/// variables say, and for an `https://` endpoint the variables' material or +/// the system trust store without them. A plaintext signal must get no +/// configuration at all: the exporter would still hand it to tonic, which +/// loads the trust roots as it builds the connector and fails on a host +/// without a certificate bundle, so a deployment that sends one signal +/// over TLS and another in plaintext would die building the plaintext one. fn exporter_tls_config(signal: &str) -> Result, Error> { - Ok(otlp_tls_config(Some(signal))? - .or_else(|| otlp_endpoint_is_https(signal).then(system_roots_tls_config))) + if !otlp_endpoint_is_https(signal) { + return Ok(None); + } + Ok(Some( + otlp_tls_config(Some(signal))?.unwrap_or_else(system_roots_tls_config), + )) } /// This is the header that bazel sends when using the `--remote_header` flag. /// TODO(palfrey): Bazel supports other headers, and we should optimize their usage. @@ -811,13 +820,71 @@ mod tests { unsafe { env::set_var("OTEL_EXPORTER_OTLP_CERTIFICATE", &ca) }; let general = otlp_tls_config(None); let per_signal = otlp_tls_config(Some("METRICS")); - // The variables apply to a plaintext endpoint too; the scheme is - // the exporter's to judge. + // The endpoint is the plaintext default, so the exporter gets no + // TLS configuration however the variables are set. let exporter = exporter_tls_config("METRICS"); clear_otlp_tls_env(); assert!(general.unwrap().is_some()); assert!(per_signal.unwrap().is_some()); - assert!(exporter.unwrap().is_some()); + assert!(exporter.unwrap().is_none()); + } + + // A deployment that sends metrics over TLS with an identity and logs in + // plaintext: the plaintext signal must get no TLS configuration, or the + // exporter loads the trust roots for it and a host without a + // certificate bundle dies building the log exporter. + #[serial(env)] + #[nativelink_test("crate")] + async fn plaintext_signal_gets_no_tls_config_whatever_the_variables() { + clear_otlp_tls_env(); + let dir = tempfile::tempdir().unwrap(); + let certificate = pem_file(&dir, "client"); + let key = pem_file(&dir, "key"); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE", &certificate) }; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CLIENT_KEY", &key) }; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { + env::set_var( + "OTEL_EXPORTER_OTLP_METRICS_ENDPOINT", + "https://collector:4317", + ); + }; + let logs = exporter_tls_config("LOGS"); + let metrics = exporter_tls_config("METRICS"); + clear_otlp_tls_env(); + assert!( + logs.unwrap().is_none(), + "the plaintext default endpoint gets no TLS configuration" + ); + assert!( + metrics.unwrap().is_some(), + "the https endpoint gets the variables' configuration" + ); + } + + // The SDK accepts an uppercase scheme; an `HTTPS://` endpoint has to + // get its trust roots too, or the process starts with an empty store. + #[serial(env)] + #[nativelink_test("crate")] + async fn uppercase_https_scheme_gets_the_default_roots() { + clear_otlp_tls_env(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { + env::set_var( + "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", + "HTTPS://collector:4317", + ); + }; + let traces = exporter_tls_config("TRACES"); + // A value that is not a URL is not https; the exporter rejects it. + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", "collector:4317:https") }; + let unparseable = exporter_tls_config("TRACES"); + clear_otlp_tls_env(); + assert!(traces.unwrap().is_some(), "HTTPS:// is an https endpoint"); + assert!(unparseable.unwrap().is_none()); } #[serial(env)] From 8c9f95f47c1160abc9c46818b6e4f91bdd6951d6 Mon Sep 17 00:00:00 2001 From: b7r6 Date: Thu, 22 Oct 2026 18:53:10 +0000 Subject: [PATCH 4/6] Spell unparsable the way the typos hook wants Co-Authored-By: Claude Fable 5.1 --- nativelink-util/src/telemetry.rs | 4 ++-- web/apps/docs/content/docs/reference/cli-and-env.mdx | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/nativelink-util/src/telemetry.rs b/nativelink-util/src/telemetry.rs index 13ed7eb6ec3..f00915aeb0e 100644 --- a/nativelink-util/src/telemetry.rs +++ b/nativelink-util/src/telemetry.rs @@ -881,10 +881,10 @@ mod tests { // A value that is not a URL is not https; the exporter rejects it. // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. unsafe { env::set_var("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", "collector:4317:https") }; - let unparseable = exporter_tls_config("TRACES"); + let unparsable = exporter_tls_config("TRACES"); clear_otlp_tls_env(); assert!(traces.unwrap().is_some(), "HTTPS:// is an https endpoint"); - assert!(unparseable.unwrap().is_none()); + assert!(unparsable.unwrap().is_none()); } #[serial(env)] diff --git a/web/apps/docs/content/docs/reference/cli-and-env.mdx b/web/apps/docs/content/docs/reference/cli-and-env.mdx index d4a26f9a1d4..cede4210de3 100644 --- a/web/apps/docs/content/docs/reference/cli-and-env.mdx +++ b/web/apps/docs/content/docs/reference/cli-and-env.mdx @@ -49,7 +49,7 @@ They are the only variables that change the binary's own behaviour. | Variable | Default | Effect | | --- | --- | --- | | `NL_LOG` | `pretty` | Format of the stdout log layer. `compact` and `json` are recognised; **any other value silently falls back to `pretty`**, including a typo like `Json`. | -| `RUST_LOG` | `info` | Log filter, parsed by `tracing_subscriber`'s `EnvFilter` in lossy mode; an unparseable directive is dropped rather than fatal. Applies to the stdout layer *and* to the OTLP log, trace and metric layers. `hyper`, `tonic`, `h2`, `reqwest` and `tower` are forced off afterwards and cannot be re-enabled from here. | +| `RUST_LOG` | `info` | Log filter, parsed by `tracing_subscriber`'s `EnvFilter` in lossy mode; an unparsable directive is dropped rather than fatal. Applies to the stdout layer *and* to the OTLP log, trace and metric layers. `hyper`, `tonic`, `h2`, `reqwest` and `tower` are forced off afterwards and cannot be re-enabled from here. | | `NL_OTEL_ENDPOINT` | not set | When set, all three OTLP exporters share one client-side load-balanced gRPC channel pointed at this endpoint. When unset, each exporter is built with the SDK's own tonic defaults instead. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. A per-signal certificate variable gives that signal a channel of its own, built with its material, instead of the shared one. | | `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4317` | Standard OpenTelemetry SDK variable. Applies only when `NL_OTEL_ENDPOINT` is unset; an explicit channel wins over the SDK's endpoint resolution. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_ENDPOINT` variables behave the same way. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. | | `OTEL_EXPORTER_OTLP_CERTIFICATE` | not set | Standard OpenTelemetry variable, read by NativeLink itself because the Rust SDK does not: the path to a PEM file holding the CA that signs the collector's certificate. Set, it is the only CA trusted; unset, the host's system trust store is. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_CERTIFICATE` forms win over it. An empty value counts as unset; a path that cannot be read stops the process at startup. | From dc9f055eb79d31239ce66555675d4d98801b37ec Mon Sep 17 00:00:00 2001 From: b7r6 Date: Thu, 22 Oct 2026 18:56:50 +0000 Subject: [PATCH 5/6] Say a directive that cannot be parsed, which both spell checkers accept Co-Authored-By: Claude Fable 5.1 --- web/apps/docs/content/docs/reference/cli-and-env.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/apps/docs/content/docs/reference/cli-and-env.mdx b/web/apps/docs/content/docs/reference/cli-and-env.mdx index cede4210de3..5b9a08a7aee 100644 --- a/web/apps/docs/content/docs/reference/cli-and-env.mdx +++ b/web/apps/docs/content/docs/reference/cli-and-env.mdx @@ -49,7 +49,7 @@ They are the only variables that change the binary's own behaviour. | Variable | Default | Effect | | --- | --- | --- | | `NL_LOG` | `pretty` | Format of the stdout log layer. `compact` and `json` are recognised; **any other value silently falls back to `pretty`**, including a typo like `Json`. | -| `RUST_LOG` | `info` | Log filter, parsed by `tracing_subscriber`'s `EnvFilter` in lossy mode; an unparsable directive is dropped rather than fatal. Applies to the stdout layer *and* to the OTLP log, trace and metric layers. `hyper`, `tonic`, `h2`, `reqwest` and `tower` are forced off afterwards and cannot be re-enabled from here. | +| `RUST_LOG` | `info` | Log filter, parsed by `tracing_subscriber`'s `EnvFilter` in lossy mode; a directive that cannot be parsed is dropped rather than fatal. Applies to the stdout layer *and* to the OTLP log, trace and metric layers. `hyper`, `tonic`, `h2`, `reqwest` and `tower` are forced off afterwards and cannot be re-enabled from here. | | `NL_OTEL_ENDPOINT` | not set | When set, all three OTLP exporters share one client-side load-balanced gRPC channel pointed at this endpoint. When unset, each exporter is built with the SDK's own tonic defaults instead. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. A per-signal certificate variable gives that signal a channel of its own, built with its material, instead of the shared one. | | `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4317` | Standard OpenTelemetry SDK variable. Applies only when `NL_OTEL_ENDPOINT` is unset; an explicit channel wins over the SDK's endpoint resolution. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_ENDPOINT` variables behave the same way. An `https://` endpoint gets TLS, with the certificate variables below or the system trust store. | | `OTEL_EXPORTER_OTLP_CERTIFICATE` | not set | Standard OpenTelemetry variable, read by NativeLink itself because the Rust SDK does not: the path to a PEM file holding the CA that signs the collector's certificate. Set, it is the only CA trusted; unset, the host's system trust store is. The signal-specific `OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_CERTIFICATE` forms win over it. An empty value counts as unset; a path that cannot be read stops the process at startup. | From 6c2194dada57d5bc8b10f3b8ca668bf7c5b638e9 Mon Sep 17 00:00:00 2001 From: b7r6 Date: Thu, 22 Oct 2026 18:59:56 +0000 Subject: [PATCH 6/6] Skip the SDK endpoint's TLS configuration for an exporter given the NL_OTEL_ENDPOINT channel An exporter handed the load-balanced channel sends on it and ignores the SDK's endpoint, but the builder still read that endpoint's certificate variables, so a stale OTEL_EXPORTER_OTLP_ENDPOINT pointing at an https collector with a missing CA file stopped a process that was going to connect in plaintext elsewhere. The transport is now chosen once per signal: the channel, or else the signal's own TLS configuration. Co-Authored-By: Claude Fable 5.1 --- nativelink-util/src/telemetry.rs | 107 +++++++++++++++++++++++++------ 1 file changed, 86 insertions(+), 21 deletions(-) diff --git a/nativelink-util/src/telemetry.rs b/nativelink-util/src/telemetry.rs index f00915aeb0e..fe60a29ccf8 100644 --- a/nativelink-util/src/telemetry.rs +++ b/nativelink-util/src/telemetry.rs @@ -135,13 +135,8 @@ pub async fn init_tracing() -> Result<(), Error> { let channels = load_balanced_channels().await?; // Logs - let mut log_exporter_builder = LogExporter::builder().with_tonic(); - if let Some(channel) = channels.logs { - log_exporter_builder = log_exporter_builder.with_channel(channel.into()); - } - if let Some(tls_config) = exporter_tls_config("LOGS")? { - log_exporter_builder = log_exporter_builder.with_tls_config(tls_config); - } + let log_exporter_builder = + with_transport(LogExporter::builder().with_tonic(), channels.logs, "LOGS")?; let otlp_log_layer = OpenTelemetryTracingBridge::new( &SdkLoggerProvider::builder() .with_resource(resource.clone()) @@ -157,13 +152,11 @@ pub async fn init_tracing() -> Result<(), Error> { .with_filter(otlp_filter()); // Traces - let mut span_exporter_builder = SpanExporter::builder().with_tonic(); - if let Some(channel) = channels.traces { - span_exporter_builder = span_exporter_builder.with_channel(channel.into()); - } - if let Some(tls_config) = exporter_tls_config("TRACES")? { - span_exporter_builder = span_exporter_builder.with_tls_config(tls_config); - } + let span_exporter_builder = with_transport( + SpanExporter::builder().with_tonic(), + channels.traces, + "TRACES", + )?; let otlp_trace_layer = layer() .with_tracer( SdkTracerProvider::builder() @@ -181,13 +174,11 @@ pub async fn init_tracing() -> Result<(), Error> { .with_filter(otlp_filter()); // Metrics - let mut metric_exporter_builder = MetricExporter::builder().with_tonic(); - if let Some(channel) = channels.metrics { - metric_exporter_builder = metric_exporter_builder.with_channel(channel.into()); - } - if let Some(tls_config) = exporter_tls_config("METRICS")? { - metric_exporter_builder = metric_exporter_builder.with_tls_config(tls_config); - } + let metric_exporter_builder = with_transport( + MetricExporter::builder().with_tonic(), + channels.metrics, + "METRICS", + )?; let meter_provider = SdkMeterProvider::builder() .with_resource(resource) .with_periodic_exporter( @@ -456,6 +447,27 @@ fn exporter_tls_config(signal: &str) -> Result, Error> { otlp_tls_config(Some(signal))?.unwrap_or_else(system_roots_tls_config), )) } + +/// Gives the exporter builder for `signal` its transport: the channel +/// [`NL_OTEL_ENDPOINT`] supplied, which carries its own TLS configuration, +/// or else the TLS configuration the signal's own endpoint calls for. With +/// a channel the SDK's endpoint is ignored, so it is not examined either: +/// an `OTEL_EXPORTER_OTLP_ENDPOINT` left pointing at an `https://` +/// collector, with a certificate file that is no longer there, must not +/// stop a process that is not going to connect to it. +fn with_transport( + builder: B, + channel: Option, + signal: &str, +) -> Result { + if let Some(channel) = channel { + return Ok(builder.with_channel(channel.into())); + } + Ok(match exporter_tls_config(signal)? { + Some(tls_config) => builder.with_tls_config(tls_config), + None => builder, + }) +} /// This is the header that bazel sends when using the `--remote_header` flag. /// TODO(palfrey): Bazel supports other headers, and we should optimize their usage. const BAZEL_REQUESTMETADATA_HEADER: &str = "build.bazel.remote.execution.v2.requestmetadata-bin"; @@ -1086,6 +1098,59 @@ mod tests { ); } + // An exporter handed the NL_OTEL_ENDPOINT channel sends on that channel, + // so the SDK endpoint it would otherwise use, and that endpoint's + // certificate variables, are not examined: a stale https endpoint with + // a missing CA file used to stop startup although nothing was going to + // connect to it. + #[serial(env)] + #[nativelink_test("crate")] + async fn supplied_channel_skips_the_sdk_endpoints_tls_configuration() { + clear_otlp_tls_env(); + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("missing.pem").to_str().unwrap().to_string(); + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", "https://unused.example:4317") }; + // SAFETY: `#[serial(env)]` serializes all env-var writes across tests. + unsafe { env::set_var("OTEL_EXPORTER_OTLP_CERTIFICATE", &missing) }; + // Without a channel the exporter would connect to that endpoint, so + // its missing CA file is an error. + let own_connection = with_transport(SpanExporter::builder().with_tonic(), None, "TRACES"); + let channel = if dns_configured() { + LoadBalancedChannel::builder(("localhost", 4317)) + .channel() + .await + .ok() + } else { + eprintln!( + "Skipping the supplied-channel assertion: no DNS configuration \ + available (e.g. sandboxed Nix build)" + ); + None + }; + let supplied = channel.map(|channel| { + with_transport( + SpanExporter::builder().with_tonic(), + Some(channel), + "TRACES", + ) + }); + clear_otlp_tls_env(); + assert_eq!( + own_connection + .expect_err("the missing CA file is an error for an exporter that would use it") + .code, + Code::InvalidArgument + ); + if let Some(supplied) = supplied { + assert!( + supplied.is_ok(), + "a supplied channel must not read the ignored endpoint's variables: {:?}", + supplied.err() + ); + } + } + #[test] fn inject_context_writes_baggage_into_request_metadata() { use opentelemetry::baggage::BaggageExt;