diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 99bbeee91b..2ad45776a9 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -93,6 +93,19 @@ When validating variables, we should use default values for object fields if app By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/2003 +### ([Issue #2036](https://github.com/apollographql/router/issues/2036)) + +Work around for [opentelemetry-rust#908](https://github.com/open-telemetry/opentelemetry-rust/issues/908) +The default URL currently incorrectly uses https causing errors when connecting to a default localhost OTel Collector. + +The router will detect and work around this by explicitly setting the correct endpoint URLs when not specified in config. + +In addition: +* basic TLS defaulting will occur when the endpoint scheme uses `https`. +* a warning will be raised if the endpoint port is 443 but no TLS config is specified. + +By [@bryncooke](https://github.com/bryncooke) in https://github.com/apollographql/router/pull/#2048 + ## 🛠 Maintenance ### Apply tower best practice to inner service cloning ([PR #2030](https://github.com/apollographql/router/pull/2030)) diff --git a/apollo-router/src/plugins/telemetry/otlp.rs b/apollo-router/src/plugins/telemetry/otlp.rs index 174959d695..29801f56d2 100644 --- a/apollo-router/src/plugins/telemetry/otlp.rs +++ b/apollo-router/src/plugins/telemetry/otlp.rs @@ -39,9 +39,19 @@ impl Config { pub(crate) fn exporter + From>( &self, ) -> Result { - let endpoint = match &self.endpoint { - Endpoint::Default(_) => None, - Endpoint::Url(s) => Some(s), + let endpoint = match (self.endpoint.clone(), &self.protocol) { + // # https://github.com/apollographql/router/issues/2036 + // Opentelemetry rust incorrectly defaults to https + // This will override the defaults to that of the spec + // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md + (Endpoint::Default(_), Some(Protocol::Http)) => { + Some(Url::parse("http://localhost:4318").expect("default url is valid")) + } + // Default is GRPC + (Endpoint::Default(_), _) => { + Some(Url::parse("http://localhost:4317").expect("default url is valid")) + } + (Endpoint::Url(s), _) => Some(s), }; match self.protocol.clone().unwrap_or_default() { Protocol::Grpc => { @@ -51,7 +61,7 @@ impl Config { .with_env() .with(&self.timeout, |b, t| b.with_timeout(*t)) .with(&endpoint, |b, e| b.with_endpoint(e.as_str())) - .try_with(&grpc.tls_config.defaulted(endpoint), |b, t| { + .try_with(&grpc.tls_config.defaulted(endpoint.as_ref()), |b, t| { Ok(b.with_tls_config(t.try_into()?)) })? .with(&grpc.metadata, |b, m| b.with_metadata(m.clone())) @@ -139,7 +149,13 @@ impl TlsConfig { pub(crate) fn defaulted(mut self, endpoint: Option<&Url>) -> Option { if let Some(endpoint) = endpoint { if self.domain_name.is_none() { - self.domain_name = endpoint.host_str().map(|s| s.to_string()) + // If the URL contains the https scheme then default the tls config to use the domain from the URL. We know it's TLS. + // If the URL contains no scheme and the port is 443 emit a warning suggesting that they may have forgotten to configure TLS domain. + if endpoint.scheme() == "https" { + self.domain_name = endpoint.host_str().map(|s| s.to_string()) + } else if endpoint.port() == Some(443) && endpoint.scheme() != "http" { + tracing::warn!("telemetry otlp exporter has been configured with port 443 but TLS domain has not been set. This is likely a configuration error") + } } } @@ -297,6 +313,12 @@ mod tests { assert_eq!(tls, None); } + #[test] + fn default_endpoint() { + let tls = TlsConfig::default().defaulted(None); + assert_eq!(tls, None); + } + #[test] fn endpoint_grpc_defaulting_scheme() { let url = Url::parse("https://api.apm.com:433").unwrap();