Skip to content

Commit

Permalink
Safer endpoint Uri construction (#1553)
Browse files Browse the repository at this point in the history
Make endpoint Uri construction less fragile by stripping out duplicate slashes.

Fixes #997

The functionality to append the signal path to a user-supplied tracing endpoint is already in place. However, it does so in a way that is likely to break for any user who's passing the string value of a Uri or Url, which will have a trailing slash appended to them. This attempts to fix that issue.

There were other possible changes discussed but ever implemented/merged in #1056. This PR attempts to keep it simple by just changing existing behavior to not break in a common use case.
  • Loading branch information
jaymell authored Feb 18, 2024
1 parent 1be83bb commit 4c22cf6
Showing 1 changed file with 28 additions and 5 deletions.
33 changes: 28 additions & 5 deletions opentelemetry-otlp/src/exporter/http/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,15 @@ impl OtlpHttpClient {
}
}

fn build_endpoint_uri(endpoint: &str, path: &str) -> Result<Uri, crate::Error> {
let path = if endpoint.ends_with('/') && path.starts_with('/') {
path.strip_prefix('/').unwrap()
} else {
path
};
format!("{endpoint}{path}").parse().map_err(From::from)
}

// see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#endpoint-urls-for-otlphttp
fn resolve_endpoint(
signal_endpoint_var: &str,
Expand All @@ -287,15 +296,13 @@ fn resolve_endpoint(
// if signal env var is not set, then we check if the OTEL_EXPORTER_OTLP_ENDPOINT is set
if let Some(endpoint) = env::var(OTEL_EXPORTER_OTLP_ENDPOINT)
.ok()
.and_then(|s| format!("{s}{signal_endpoint_path}").parse().ok())
.and_then(|s| build_endpoint_uri(&s, signal_endpoint_path).ok())
{
return Ok(endpoint);
}

// if neither works, we use the one provided in pipeline. If user never provide one, we will use the default one
format!("{provided_or_default_endpoint}{signal_endpoint_path}")
.parse()
.map_err(From::from)
// if neither works, we use the one provided in pipeline. If user never provides one, we will use the default one
build_endpoint_uri(provided_or_default_endpoint, signal_endpoint_path)
}

#[allow(clippy::mutable_key_type)] // http headers are not mutated
Expand All @@ -313,6 +320,8 @@ mod tests {
use crate::exporter::tests::run_env_test;
use crate::{OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT};

use super::build_endpoint_uri;

#[test]
fn test_append_signal_path_to_generic_env() {
run_env_test(
Expand Down Expand Up @@ -374,6 +383,20 @@ mod tests {
});
}

#[test]
fn test_build_endpoint_uri() {
let uri = build_endpoint_uri("https://example.com", "/v1/traces").unwrap();
assert_eq!(uri, "https://example.com/v1/traces");

// Should be no duplicate slahes:
let uri = build_endpoint_uri("https://example.com/", "/v1/traces").unwrap();
assert_eq!(uri, "https://example.com/v1/traces");

// Append paths properly:
let uri = build_endpoint_uri("https://example.com/additional/path/", "/v1/traces").unwrap();
assert_eq!(uri, "https://example.com/additional/path/v1/traces");
}

#[test]
fn test_invalid_uri_in_signal_env_falls_back_to_generic_env() {
run_env_test(
Expand Down

0 comments on commit 4c22cf6

Please sign in to comment.