From df128ae2c94ac1c0066340dc160265560b1c25e0 Mon Sep 17 00:00:00 2001
From: bryn <bryn@apollographql.com>
Date: Fri, 4 Nov 2022 15:43:31 +0000
Subject: [PATCH] Fixes #2036 Otel default endpoint incorrectly uses https.
 https://github.com/open-telemetry/opentelemetry-rust/issues/908

default endpoint is now configured to use http rather than https.
In addition, tls domain will only be set if the endpoint scheme is https.
If the endpoint port is 443 a warning is displayed if TLS has not been configured.
---
 NEXT_CHANGELOG.md                           | 13 +++++++++
 apollo-router/src/plugins/telemetry/otlp.rs | 32 +++++++++++++++++----
 2 files changed, 40 insertions(+), 5 deletions(-)

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<T: From<HttpExporterBuilder> + From<TonicExporterBuilder>>(
         &self,
     ) -> Result<T, BoxError> {
-        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<TlsConfig> {
         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();