feat: configure TLS with environment variables. #2465

jvanz · 2024-12-23T20:34:06Z

Updates the opentelemetry-otlp crate to allow users to configure TLS using environment variables. Removing the need to crating the TLS config object and defining it with the with_tls_config method. In the same way other OTLP libraries does (e.g. go lang).

Partially fixes #774

Merge requirement checklist

CONTRIBUTING guidelines followed
Unit tests added/updated (if applicable)
Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
Changes in public API reviewed (if applicable)

codecov · 2024-12-24T16:15:38Z

Codecov Report

Attention: Patch coverage is 5.36913% with 141 lines in your changes missing coverage. Please review.

Project coverage is 77.4%. Comparing base (68af3bb) to head (07e62dd).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-otlp/src/exporter/tonic/mod.rs	0.0%	121 Missing ⚠️
opentelemetry-otlp/src/exporter/mod.rs	28.5%	20 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2465     +/-   ##
=======================================
- Coverage   77.9%   77.4%   -0.5%     
=======================================
  Files        123     123             
  Lines      22944   23168    +224     
=======================================
+ Hits       17880   17948     +68     
- Misses      5064    5220    +156

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jvanz · 2025-01-20T19:51:33Z

@TommyCpp , how can we move this forward? 😄

Updates the opentelemetry-otlp crate to allow users to configure TLS using environment variables. Removing the need to crating the TLS config object and defining it with the `with_tls_config` method. In the same way other OTLP libraries does (e.g. go lang). Signed-off-by: José Guilherme Vanz <[email protected]>

TommyCpp · 2025-01-20T20:35:40Z

👀 Will take a look today

opentelemetry-otlp/CHANGELOG.md

opentelemetry-otlp/src/exporter/mod.rs

Fix typo in the changelog. Signed-off-by: José Guilherme Vanz <[email protected]>

Add missing TLS configuration directives. Signed-off-by: José Guilherme Vanz <[email protected]>

opentelemetry-otlp/src/metric.rs

Add comments formatting adding missing periods at the end. Signed-off-by: José Guilherme Vanz <[email protected]>

cijothomas · 2025-01-27T18:42:47Z

opentelemetry-otlp/tests/smoke.rs

@@ -136,3 +151,150 @@ async fn smoke_tracer() {
    let first_event = first_span.events.first().unwrap();
    assert_eq!("my-test-event", first_event.name);
 }
+
+#[tokio::test(flavor = "multi_thread")]


we now have automated integration tests. Could these tests be added there? It does not have anything validating the TLS code paths..
https://github.com/open-telemetry/opentelemetry-rust/tree/main/opentelemetry-otlp/tests/integration_test/tests

+1., this needs to be moved to integrations tests.

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/gen_test_cert.sh we can steal some scripts from here to generate the needed artifacts.

cijothomas · 2025-01-27T20:16:31Z

opentelemetry-otlp/src/exporter/tonic/mod.rs

+        {
+            let insecure = config.insecure.unwrap_or_else(|| {
+                env::var(signal_insecure_var)
+                    .or_else(|_| env::var(OTEL_EXPORTER_OTLP_INSECURE))


haven't done a full look, but wondering what is the priority order when signal specific ENV variables and this is present?

Looking at the code (specifically resolve_tls_config), the priority order seems to be:
ClientTlsConfig -> signal_specific_env -> otlp_generic_env

But need @jvanz to confirm :)

lalitb · 2025-01-27T23:59:56Z

opentelemetry-otlp/src/exporter/mod.rs

@@ -27,6 +27,18 @@ pub const OTEL_EXPORTER_OTLP_HEADERS: &str = "OTEL_EXPORTER_OTLP_HEADERS";
 pub const OTEL_EXPORTER_OTLP_PROTOCOL: &str = "OTEL_EXPORTER_OTLP_PROTOCOL";
 /// Compression algorithm to use, defaults to none.
 pub const OTEL_EXPORTER_OTLP_COMPRESSION: &str = "OTEL_EXPORTER_OTLP_COMPRESSION";
+/// Certificate file to validate the OTLP server connection.
+#[cfg(feature = "tls")]


tls feature flag is specific to grpc-tonic. Just to be clear -this PR is only adding TLS support for grpc, and not http?

lalitb · 2025-01-28T00:13:41Z

opentelemetry-otlp/src/exporter/mod.rs

@@ -217,6 +268,34 @@ impl<B: HasExportConfig> WithExportConfig for B {
        self.export_config().endpoint = exporter_config.endpoint;
        self.export_config().protocol = exporter_config.protocol;
        self.export_config().timeout = exporter_config.timeout;
+        #[cfg(feature = "tls")]
+        {
+            self.export_config().insecure = Some(true);


As per the specs, the connection should be secure by default.
https://github.com/open-telemetry/opentelemetry-specification/blob/3b037be082915f29766a87c01141c72fb0fb5f1b/specification/protocol/exporter.md

lalitb · 2025-01-28T00:17:39Z

opentelemetry-otlp/src/exporter/tonic/mod.rs

-                .map_err(crate::Error::from)?,
-            None => endpoint,
+        {
+            let insecure = config.insecure.unwrap_or_else(|| {


Please correct me - there would always be a default value associated with the insecure, which means else part will never execute?

jvanz force-pushed the tls-envvars branch 3 times, most recently from 50894e9 to 5031002 Compare December 24, 2024 16:12

jvanz force-pushed the tls-envvars branch 3 times, most recently from 9458a03 to 093a1ce Compare December 31, 2024 14:46

jvanz marked this pull request as ready for review December 31, 2024 14:50

jvanz requested a review from a team as a code owner December 31, 2024 14:50

jvanz force-pushed the tls-envvars branch from 9771ae3 to b2c6437 Compare January 20, 2025 19:50

jvanz force-pushed the tls-envvars branch from b2c6437 to a569202 Compare January 20, 2025 20:07

TommyCpp self-assigned this Jan 20, 2025

TommyCpp reviewed Jan 21, 2025

View reviewed changes

opentelemetry-otlp/CHANGELOG.md Outdated Show resolved Hide resolved

opentelemetry-otlp/src/exporter/mod.rs Show resolved Hide resolved

TommyCpp removed their assignment Jan 21, 2025

jvanz added 2 commits January 22, 2025 17:05

fix: typo in the changelog.

47ef0e6

Fix typo in the changelog. Signed-off-by: José Guilherme Vanz <[email protected]>

fix: add missing TLS configuration directives.

07e62dd

Add missing TLS configuration directives. Signed-off-by: José Guilherme Vanz <[email protected]>

jvanz requested a review from TommyCpp January 22, 2025 20:10

ThomsonTan reviewed Jan 22, 2025

View reviewed changes

opentelemetry-otlp/src/metric.rs Outdated Show resolved Hide resolved

fix: add missing periods at the end of comments.

ed43079

Add comments formatting adding missing periods at the end. Signed-off-by: José Guilherme Vanz <[email protected]>

jvanz requested a review from ThomsonTan January 27, 2025 18:11

cijothomas reviewed Jan 27, 2025

View reviewed changes

cijothomas requested a review from lalitb January 27, 2025 21:07

lalitb approved these changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: configure TLS with environment variables. #2465

feat: configure TLS with environment variables. #2465

jvanz commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading

jvanz commented Jan 20, 2025

TommyCpp commented Jan 20, 2025

cijothomas Jan 27, 2025

lalitb Jan 28, 2025

cijothomas Jan 28, 2025

cijothomas Jan 27, 2025

lalitb Jan 28, 2025

lalitb Jan 27, 2025

lalitb Jan 28, 2025

lalitb Jan 28, 2025

feat: configure TLS with environment variables. #2465

Are you sure you want to change the base?

feat: configure TLS with environment variables. #2465

Conversation

jvanz commented Dec 23, 2024 • edited Loading

Merge requirement checklist

codecov bot commented Dec 24, 2024 • edited Loading

Codecov Report

jvanz commented Jan 20, 2025

TommyCpp commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvanz commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading