Repository navigation
Conversation
An `https://` collector endpoint stopped NativeLink at startup: `opentelemetry-otlp` was built without any of its TLS features and refuses to build a tonic exporter for HTTPS without one. The only collector a process could export to was one reached in plaintext, so a worker outside the cluster could not send its telemetry to the cluster's collector. Enable `tls-ring` on `opentelemetry-otlp` in nativelink-util. NativeLink already uses ring as its rustls provider, so this adds no crypto backend. An https endpoint, in `OTEL_EXPORTER_OTLP_ENDPOINT` or `NL_OTEL_ENDPOINT`, now gets server-authenticated TLS against the system trust store (tonic's bare `ClientTlsConfig::new()` trusts nothing, so the roots are enabled explicitly; a plaintext endpoint gets no TLS configuration at all, so a host without a certificate bundle still exports). Read the OpenTelemetry specification's certificate variables, which the Rust SDK does not: `OTEL_EXPORTER_OTLP_CERTIFICATE` names a private CA and is then the only CA trusted; `OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE` and `OTEL_EXPORTER_OTLP_CLIENT_KEY` name the identity for mTLS and have to be set together. The per-signal forms win over the general one. A file that cannot be read, or half an identity, stops the process at startup with an InvalidArgument naming the variable, which is also what a malformed `NL_OTEL_ENDPOINT` now does instead of panicking. Fixes TraceMachina#2983 Co-Authored-By: Claude Fable 5.1 <[email protected]>
…INT channel The shared channel `NL_OTEL_ENDPOINT` builds was made with the general certificate variables only, and an exporter handed a channel ignores its own `with_tls_config`, so `OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE` and the other per-signal variables worked with `OTEL_EXPORTER_OTLP_ENDPOINT` but not with `NL_OTEL_ENDPOINT`: the handshake failed with an unknown CA. Keep the one shared channel as the common case, and when any per-signal certificate variable is set build one channel per signal with that signal's resolution of the variables. `maybe_load_balanced_channel` stays as the general-variables channel for the integration tests. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…s scheme
A plaintext signal got a TLS configuration whenever a certificate variable
was set, and the exporter still hands that to tonic, which loads the trust
roots as it builds the connector and fails on a host without a certificate
bundle: a deployment sending metrics over TLS with an identity and logs in
plaintext died building the log exporter. A plaintext endpoint now gets no
configuration whatever the variables say; only an https endpoint gets one.
The scheme check was a case-sensitive `starts_with("https://")`, while the
SDK accepts `HTTPS://` too, so such an endpoint started with an empty trust
store. Parse the value as a URL and compare the scheme; a value that does
not parse counts as plaintext and is left for the exporter to reject.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
Co-Authored-By: Claude Fable 5.1 <[email protected]>
Co-Authored-By: Claude Fable 5.1 <[email protected]>
…L_OTEL_ENDPOINT channel An exporter handed the load-balanced channel sends on it and ignores the SDK's endpoint, but the builder still read that endpoint's certificate variables, so a stale OTEL_EXPORTER_OTLP_ENDPOINT pointing at an https collector with a missing CA file stopped a process that was going to connect in plaintext elsewhere. The transport is now chosen once per signal: the channel, or else the signal's own TLS configuration. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Owner
Author
|
Baked here; shipped upstream as TraceMachina#3023. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Setting
OTEL_EXPORTER_OTLP_ENDPOINT(orNL_OTEL_ENDPOINT) to anhttps://URL stopped NativeLink before it served anything:opentelemetry-otlpwas built without a TLS feature and refuses to build a tonic exporter for HTTPS without one. The only collector a process could export to was one it reached in plaintext, so a worker outside the cluster could not send telemetry to the cluster's collector.This enables
tls-ringonopentelemetry-otlpinnativelink-util(ring is already the rustls provider, so no new crypto backend) and reads the OpenTelemetry specification's certificate variables, which the Rust SDK does not read itself:https://endpoint gets server-authenticated TLS against the system trust store. tonic's bareClientTlsConfig::new()trusts nothing at all, so the roots are enabled explicitly; a plaintext endpoint gets no TLS configuration, so a host without a certificate bundle still exports.OTEL_EXPORTER_OTLP_CERTIFICATEnames a private CA, and then only that CA is trusted.OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATEandOTEL_EXPORTER_OTLP_CLIENT_KEYname the identity for mTLS and have to be set together.OTEL_EXPORTER_OTLP_TRACES_CERTIFICATEand so on) win over the general one, as the specification says.NL_OTEL_ENDPOINT, the three exporters share one channel built with the general variables, as before. An exporter handed a channel ignores its own TLS configuration, so when any per-signal certificate variable is set each signal gets a channel of its own, built with that signal's resolution of the variables. A plaintextNL_OTEL_ENDPOINTbuilds no TLS configuration either way.OTEL_EXPORTER_OTLP_ENDPOINTpointing at anhttps://collector with a missing CA file no longer stops a process that connects in plaintext throughNL_OTEL_ENDPOINT.InvalidArgumentnaming the variable. A malformedNL_OTEL_ENDPOINTnow does the same instead of panicking.Both docs pages that list the telemetry variables gained rows for the three new ones, say that an https endpoint gets TLS, and say that a per-signal certificate variable gives that signal its own
NL_OTEL_ENDPOINTchannel.Fixes TraceMachina#2983
How was this verified?
bazel-bin/nativelink, built frommainat 283ff77) contains the stringuses HTTPS but no TLS feature is enabledonce; the binary built from this branch does not contain it, so the refusal is compiled out.OTEL_EXPORTER_OTLP_ENDPOINT=https://localhost:4317 nativelink config.json5logsReady, listening on 127.0.0.1:50971, is still up after eight seconds with the port inss -ltn, and shuts down cleanly on SIGTERM. Nothing was listening on 4317, which is the issue's point: the process no longer dies at startup.OTEL_EXPORTER_OTLP_CERTIFICATE=/nonexistent/ca.pemstops it withCould not read /nonexistent/ca.pem, which OTEL_EXPORTER_OTLP_CERTIFICATE names: No such file or directory;OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATEwithout the key stops it with a message naming both variables.telemetry.rs(all#[serial(env)]): building a span exporter for anhttps://endpoint succeeds (this one does not even compile without the feature, sincewith_tls_configis feature-gated, and would fail atbuild()on the old binary); no variables yields no TLS config; a CA file yields one; a client certificate and key yield one; the per-signal variable wins over the general one; a client certificate without its key and a missing file are bothInvalidArgumentnaming the variable; anhttps://per-signal endpoint gets a config while the default plaintext endpoint does not.NL_OTEL_ENDPOINT=https://localhost:4317,OTEL_EXPORTER_OTLP_TRACES_CERTIFICATEat a dummy PEM andOTEL_EXPORTER_OTLP_CERTIFICATEat a missing file, the TRACES channel builds while the METRICS channel, and the whole per-signal set, fail withInvalidArgumentnamingOTEL_EXPORTER_OTLP_CERTIFICATE. The METRICS error precedes name resolution so it holds without DNS; the TRACES success needs DNS and is skipped the waychannel_present_when_valid_endpoint_setskips. A second test checks that without per-signal variables (an empty one included) the signals share one channel, and that without the endpoint variable they get none. Before the fix, the first test's TRACES assertion fails: the shared channel read the general variable and errored on the missing file.OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATEand_CLIENT_KEYat dummy files andOTEL_EXPORTER_OTLP_METRICS_ENDPOINT=https://collector:4317while the general endpoint stays the plaintext default,exporter_tls_config("LOGS")isNoneandexporter_tls_config("METRICS")isSome. Before the fix the LOGS call returned a config, and the exporter handed it to tonic, which loads the trust roots and dies on a host with no CA bundle; the existing CA-file test asserted that wrong behaviour and now assertsNone.OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=HTTPS://collector:4317yields a config fromexporter_tls_config("TRACES")(the SDK accepts the uppercase scheme, and the oldstarts_with("https://")would have left the trust store empty), and a value that does not parse as a URL yields none.OTEL_EXPORTER_OTLP_ENDPOINT=https://unused.example:4317andOTEL_EXPORTER_OTLP_CERTIFICATEat a missing file, choosing the transport for an exporter without a channel isInvalidArgument, and with a load-balanced channel it succeeds (the channel half needs DNS, and skips itself where there is none). Testsupplied_channel_skips_the_sdk_endpoints_tls_configuration.cargo test -p nativelink-util --profile=smol --lib telemetry: 16 passed.cargo test -p nativelink-util --profile=smol --test telemetry_test: 6 passed.bazel test //nativelink-util:unit_test //nativelink-util:integration: 23 of 23 pass, with the rustfmt and clippy aspects.lint:anchorsandlint:snippetspass;valeis clean on the changed pages. Biome does not process.mdx, andtyposis not on this machine's path, so those two ran only as far as CI will run them.ClientTlsConfigto hand it.Cargo.lockandMODULE.bazel.lockdid not change: tonic already carriestls-ringin the graph.Risk
Low on existing deployments. A plaintext endpoint takes exactly the path it took before: no TLS configuration is built, even with the certificate variables set, so nothing new runs on a host without a certificate store, and
NL_OTEL_ENDPOINTstill builds one shared channel unless a per-signal certificate variable is set. The new code runs only when an endpoint ishttps://or a certificate variable is set, both of which were fatal before. The one behavioural change on an existing deployment is that a malformedNL_OTEL_ENDPOINTnow exits with an error instead of a panic; the process stops at startup either way. No new crate is linked, since ring and rustls were already in the binary.AI assistance
An agent (Claude Code) wrote the change and tests under b7r6's direction; b7r6 reviews every line before it goes upstream.
🤖 Generated with Claude Code