From c8a4e4bd30c32e8c4d3fdf19e7ec11ae984d395f Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Mon, 26 Feb 2024 14:17:42 +0100 Subject: [PATCH 1/7] Support URL-encoded values for OTEL_EXPORTER_OTLP_HEADERS --- Cargo.toml | 1 + opentelemetry-otlp/Cargo.toml | 1 + opentelemetry-otlp/src/exporter/http/mod.rs | 2 +- opentelemetry-otlp/src/exporter/mod.rs | 31 ++++++++++++++++---- opentelemetry-otlp/src/exporter/tonic/mod.rs | 2 +- 5 files changed, 30 insertions(+), 7 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 3092889e71..d039abe814 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,3 +66,4 @@ tracing = { version = "0.1", default-features = false } tracing-core = { version = "0.1", default-features = false } tracing-subscriber = { version = "0.3", default-features = false } url = { version = "2.2", default-features = false } +urlencoding = { version = "2.1.3" } diff --git a/opentelemetry-otlp/Cargo.toml b/opentelemetry-otlp/Cargo.toml index 7224392543..b2e06e6108 100644 --- a/opentelemetry-otlp/Cargo.toml +++ b/opentelemetry-otlp/Cargo.toml @@ -42,6 +42,7 @@ reqwest = { workspace = true, optional = true } http = { workspace = true, optional = true } serde = { workspace = true, features = ["derive"], optional = true } thiserror = { workspace = true } +urlencoding = { workspace = true } [dev-dependencies] tokio-stream = { workspace = true, features = ["net"] } diff --git a/opentelemetry-otlp/src/exporter/http/mod.rs b/opentelemetry-otlp/src/exporter/http/mod.rs index 6cd90203a7..389af8f6cd 100644 --- a/opentelemetry-otlp/src/exporter/http/mod.rs +++ b/opentelemetry-otlp/src/exporter/http/mod.rs @@ -310,7 +310,7 @@ fn add_header_from_string(input: &str, headers: &mut HashMap WithExportConfig for B { } #[cfg(any(feature = "grpc-tonic", feature = "http-proto"))] -fn parse_header_string(value: &str) -> impl Iterator { +fn parse_header_string(value: &str) -> impl Iterator { value .split_terminator(',') .map(str::trim) @@ -219,10 +219,17 @@ fn parse_header_string(value: &str) -> impl Iterator { } #[cfg(any(feature = "grpc-tonic", feature = "http-proto"))] -fn parse_header_key_value_string(key_value_string: &str) -> Option<(&str, &str)> { +fn parse_header_key_value_string(key_value_string: &str) -> Option<(&str, String)> { key_value_string .split_once('=') - .map(|(key, value)| (key.trim(), value.trim())) + .map(|(key, value)| { + ( + key.trim(), + urlencoding::decode(value.trim()) + .unwrap_or_default() + .into_owned(), + ) + }) .filter(|(key, value)| !key.is_empty() && !value.is_empty()) } @@ -280,7 +287,10 @@ mod tests { for (input_str, expected_headers) in test_cases { assert_eq!( super::parse_header_string(input_str).collect::>(), - expected_headers, + expected_headers + .into_iter() + .map(|(k, v)| (k, v.to_string())) + .collect::>(), ) } } @@ -290,6 +300,14 @@ mod tests { let test_cases = vec![ // Format: (input_str, expected_header) ("k1=v1", Some(("k1", "v1"))), + ( + "Authentication=Basic AAA", + Some(("Authentication", "Basic AAA")), + ), + ( + "Authentication=Basic%20AAA", + Some(("Authentication", "Basic AAA")), + ), ("", None), ("=v1", None), ("k1=", None), @@ -298,7 +316,10 @@ mod tests { for (input_str, expected_headers) in test_cases { assert_eq!( super::parse_header_key_value_string(input_str), - expected_headers, + match expected_headers { + Some((k, v)) => Some((k, v.to_string())), + None => None, + } ) } } diff --git a/opentelemetry-otlp/src/exporter/tonic/mod.rs b/opentelemetry-otlp/src/exporter/tonic/mod.rs index 6ef5e6f1f6..61e142e743 100644 --- a/opentelemetry-otlp/src/exporter/tonic/mod.rs +++ b/opentelemetry-otlp/src/exporter/tonic/mod.rs @@ -381,7 +381,7 @@ fn parse_headers_from_env(signal_headers_var: &str) -> HeaderMap { .filter_map(|(key, value)| { Some(( HeaderName::from_str(key).ok()?, - HeaderValue::from_str(value).ok()?, + HeaderValue::from_str(&value).ok()?, )) }) .collect::() From fa64fd3d1168d5fa60f59fd655178216a49e6c1c Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Mon, 26 Feb 2024 14:25:18 +0100 Subject: [PATCH 2/7] Add changelog --- opentelemetry-otlp/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/opentelemetry-otlp/CHANGELOG.md b/opentelemetry-otlp/CHANGELOG.md index 39c5282b8f..c8994aa109 100644 --- a/opentelemetry-otlp/CHANGELOG.md +++ b/opentelemetry-otlp/CHANGELOG.md @@ -2,6 +2,10 @@ ## vNext +### Fixed + +- URL encoded values in `OTEL_EXPORTER_OTLP_HEADERS` are now correctly decoded. [#1578](https://github.com/open-telemetry/opentelemetry-rust/pull/1578) + ## v0.15.0 ### Changed From 47f6ab4478c50612ecd18e665b8a51573147abab Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Mon, 26 Feb 2024 14:29:18 +0100 Subject: [PATCH 3/7] Fix linter issue --- opentelemetry-otlp/src/exporter/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/opentelemetry-otlp/src/exporter/mod.rs b/opentelemetry-otlp/src/exporter/mod.rs index 5c2b4612c0..5454910a29 100644 --- a/opentelemetry-otlp/src/exporter/mod.rs +++ b/opentelemetry-otlp/src/exporter/mod.rs @@ -316,10 +316,7 @@ mod tests { for (input_str, expected_headers) in test_cases { assert_eq!( super::parse_header_key_value_string(input_str), - match expected_headers { - Some((k, v)) => Some((k, v.to_string())), - None => None, - } + expected_headers.map(|(k, v)| (k, v.to_string())), ) } } From c03cf10cc20b444aeb01d23a34987a37954bf4ef Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Tue, 27 Feb 2024 11:43:18 +0100 Subject: [PATCH 4/7] Also decode for headers passed via the API --- opentelemetry-otlp/src/exporter/http/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/opentelemetry-otlp/src/exporter/http/mod.rs b/opentelemetry-otlp/src/exporter/http/mod.rs index 389af8f6cd..c3aa5fd4c4 100644 --- a/opentelemetry-otlp/src/exporter/http/mod.rs +++ b/opentelemetry-otlp/src/exporter/http/mod.rs @@ -120,7 +120,12 @@ impl HttpExporterBuilder { pub fn with_headers(mut self, headers: HashMap) -> Self { // headers will be wrapped, so we must do some logic to unwrap first. let mut inst_headers = self.http_config.headers.unwrap_or_default(); - inst_headers.extend(headers); + inst_headers.extend(headers.into_iter().map(|(key, value)| { + ( + key, + urlencoding::decode(&value).unwrap_or_default().into_owned(), + ) + })); self.http_config.headers = Some(inst_headers); self } From c1e984bfd1aaa65462ba88de4f2175b867c80778 Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Wed, 28 Feb 2024 14:30:41 +0100 Subject: [PATCH 5/7] Add dedicated implementation for url decoding --- Cargo.toml | 1 - opentelemetry-otlp/Cargo.toml | 1 - opentelemetry-otlp/src/exporter/http/mod.rs | 11 ++--- opentelemetry-otlp/src/exporter/mod.rs | 52 +++++++++++++++++++-- 4 files changed, 54 insertions(+), 11 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d039abe814..3092889e71 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,4 +66,3 @@ tracing = { version = "0.1", default-features = false } tracing-core = { version = "0.1", default-features = false } tracing-subscriber = { version = "0.3", default-features = false } url = { version = "2.2", default-features = false } -urlencoding = { version = "2.1.3" } diff --git a/opentelemetry-otlp/Cargo.toml b/opentelemetry-otlp/Cargo.toml index b2e06e6108..7224392543 100644 --- a/opentelemetry-otlp/Cargo.toml +++ b/opentelemetry-otlp/Cargo.toml @@ -42,7 +42,6 @@ reqwest = { workspace = true, optional = true } http = { workspace = true, optional = true } serde = { workspace = true, features = ["derive"], optional = true } thiserror = { workspace = true } -urlencoding = { workspace = true } [dev-dependencies] tokio-stream = { workspace = true, features = ["net"] } diff --git a/opentelemetry-otlp/src/exporter/http/mod.rs b/opentelemetry-otlp/src/exporter/http/mod.rs index c3aa5fd4c4..a8b4e3d0c3 100644 --- a/opentelemetry-otlp/src/exporter/http/mod.rs +++ b/opentelemetry-otlp/src/exporter/http/mod.rs @@ -120,12 +120,11 @@ impl HttpExporterBuilder { pub fn with_headers(mut self, headers: HashMap) -> Self { // headers will be wrapped, so we must do some logic to unwrap first. let mut inst_headers = self.http_config.headers.unwrap_or_default(); - inst_headers.extend(headers.into_iter().map(|(key, value)| { - ( - key, - urlencoding::decode(&value).unwrap_or_default().into_owned(), - ) - })); + inst_headers.extend( + headers + .into_iter() + .map(|(key, value)| (key, super::url_decode(&value).unwrap_or(value))), + ); self.http_config.headers = Some(inst_headers); self } diff --git a/opentelemetry-otlp/src/exporter/mod.rs b/opentelemetry-otlp/src/exporter/mod.rs index 5454910a29..57461c3633 100644 --- a/opentelemetry-otlp/src/exporter/mod.rs +++ b/opentelemetry-otlp/src/exporter/mod.rs @@ -210,6 +210,35 @@ impl WithExportConfig for B { } } +fn url_decode(value: &str) -> Option { + let mut result = String::with_capacity(value.len()); + let mut chars_to_decode = Vec::::new(); + let mut all_chars = value.chars(); + + loop { + let ch = all_chars.next(); + + if ch.is_some() && ch.unwrap() == '%' { + chars_to_decode.push( + u8::from_str_radix(&format!("{}{}", all_chars.next()?, all_chars.next()?), 16) + .ok()?, + ); + continue; + } + + if !chars_to_decode.is_empty() { + result.push_str(&std::str::from_utf8(&chars_to_decode).ok()?); + chars_to_decode.clear(); + } + + if let Some(c) = ch { + result.push(c); + } else { + return Some(result); + } + } +} + #[cfg(any(feature = "grpc-tonic", feature = "http-proto"))] fn parse_header_string(value: &str) -> impl Iterator { value @@ -225,9 +254,7 @@ fn parse_header_key_value_string(key_value_string: &str) -> Option<(&str, String .map(|(key, value)| { ( key.trim(), - urlencoding::decode(value.trim()) - .unwrap_or_default() - .into_owned(), + url_decode(value.trim()).unwrap_or(value.to_string()), ) }) .filter(|(key, value)| !key.is_empty() && !value.is_empty()) @@ -274,6 +301,24 @@ mod tests { ); } + #[test] + fn test_url_decode() { + let test_cases = vec![ + // Format: (encoded, expected_decoded) + ("v%201", Some("v 1")), + ("v 1", Some("v 1")), + ("%C3%B6%C3%A0%C2%A7%C3%96abcd%C3%84", Some("öà§ÖabcdÄ")), + ("v%XX1", None), + ]; + + for (encoded, expected_decoded) in test_cases { + assert_eq!( + super::url_decode(encoded), + expected_decoded.map(|v| v.to_string()), + ) + } + } + #[test] fn test_parse_header_string() { let test_cases = vec![ @@ -308,6 +353,7 @@ mod tests { "Authentication=Basic%20AAA", Some(("Authentication", "Basic AAA")), ), + ("k1=%XX", Some(("k1", "%XX"))), ("", None), ("=v1", None), ("k1=", None), From dc5a58724d1461ce8e43575fa0ac941110f2e452 Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Wed, 28 Feb 2024 15:19:39 +0100 Subject: [PATCH 6/7] Linter issue --- opentelemetry-otlp/src/exporter/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-otlp/src/exporter/mod.rs b/opentelemetry-otlp/src/exporter/mod.rs index 57461c3633..dc324a103b 100644 --- a/opentelemetry-otlp/src/exporter/mod.rs +++ b/opentelemetry-otlp/src/exporter/mod.rs @@ -227,7 +227,7 @@ fn url_decode(value: &str) -> Option { } if !chars_to_decode.is_empty() { - result.push_str(&std::str::from_utf8(&chars_to_decode).ok()?); + result.push_str(std::str::from_utf8(&chars_to_decode).ok()?); chars_to_decode.clear(); } From 113f8f39e9a95d15b08cd3dceb25529ab392d6f9 Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Wed, 28 Feb 2024 15:36:46 +0100 Subject: [PATCH 7/7] Linter issues --- opentelemetry-otlp/src/exporter/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/opentelemetry-otlp/src/exporter/mod.rs b/opentelemetry-otlp/src/exporter/mod.rs index dc324a103b..e0beda5319 100644 --- a/opentelemetry-otlp/src/exporter/mod.rs +++ b/opentelemetry-otlp/src/exporter/mod.rs @@ -210,6 +210,7 @@ impl WithExportConfig for B { } } +#[cfg(any(feature = "grpc-tonic", feature = "http-proto"))] fn url_decode(value: &str) -> Option { let mut result = String::with_capacity(value.len()); let mut chars_to_decode = Vec::::new();