feat(otlp): added tls server config option for http and grpc - #2220
feat(otlp): added tls server config option for http and grpc#2220lucastemb wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Binary Size Analysis (Agent Data Plane)Baseline: 28b8637 · Comparison: e4ef0f4 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (3)Experiments configured
Bounds Checks: ✅ Passed (3)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
There was a problem hiding this comment.
The TLS implementation is incomplete for production receiver modes: proxy/relay mode still accepts plaintext, while gRPC TLS neither advertises HTTP/2 nor isolates handshakes from the accept loop. These paths can make configured TLS ineffective or prevent legitimate clients from connecting, so the implementation needs targeted fixes before shipping.
🤖 Datadog Autotest · Commit 93e1a74 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93e1a747d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[derive(Clone, Debug, Default, Deserialize, PartialEq, Eq, Serialize)] | ||
| pub struct TlsConfig { |
There was a problem hiding this comment.
Default omitted TLS fields during deserialization
When a user supplies the normal TLS configuration containing only cert_file and key_file, deserialization fails because every String in TlsConfig is required; the #[serde(default)] attributes on the surrounding protocol structs do not apply inside a present tls object. Add serde defaults to this struct or its fields so optional ca_file and client_ca_file values remain empty and basic server TLS can start.
Useful? React with 👍 / 👎.
| match self.grpc_tls_config { | ||
| Some(tls_config) => { | ||
| let tls_acceptor = TlsAcceptor::from(Arc::new(tls_config)); |
There was a problem hiding this comment.
Advertise h2 on the gRPC TLS listener
When gRPC TLS is enabled, build_server_config leaves alpn_protocols empty and this custom acceptor uses that configuration unchanged. Standard tonic gRPC clients require ALPN negotiation to select h2 and reject the resulting connection with H2NotNegotiated; tonic's built-in server TLS path explicitly adds h2. Add the h2 ALPN protocol before constructing this acceptor.
Useful? React with 👍 / 👎.
| match listener.accept().await { | ||
| Ok((stream, _)) => match acceptor.accept(stream).await { | ||
| Ok(stream) => return Some((Ok::<_, io::Error>(stream), (listener, acceptor))), |
There was a problem hiding this comment.
Avoid serializing gRPC TLS handshakes
When any client opens the TLS-enabled gRPC port and does not complete its handshake, this await blocks the unfold stream before it can accept another socket, with no timeout. A single stalled or malicious connection can therefore prevent every subsequent gRPC client from connecting; accept and process handshakes concurrently, or at minimum enforce a handshake timeout.
Useful? React with 👍 / 👎.
| http_tls_config: None, | ||
| grpc_tls_config: None, |
There was a problem hiding this comment.
Apply TLS configuration in OTLP proxy mode
When OTLP proxy mode is enabled, bin/agent-data-plane/src/cli/run.rs:810-845 constructs OtlpRelayConfiguration rather than the native source, but the relay calls OtlpServerBuilder::new without with_tls_configs. Because these newly added builder defaults are plaintext, the configured HTTP and gRPC TLS keys are silently ignored in proxy mode and both receiver ports remain unencrypted; build and pass the receiver TLS configurations from the relay path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred since future of proxy mode uncertain at the time of writing.
| | `otlp_allow_context_heap_allocs` | Allow heap allocations for OTLP contexts | | | ||
| | `otlp_cached_contexts_limit` | Max cached OTLP metric contexts | | | ||
| | `otlp_cached_tagsets_limit` | Max cached OTLP tagsets | | | ||
| | `otlp_config.receiver.protocols.grpc.tls.ca_file` | OTLP gRPC TLS compatibility certificate authorities | | |
There was a problem hiding this comment.
Is the goal to fully implement the options from the OTEL side? Seems like we're missing quite a bit if so? https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/README.md
There was a problem hiding this comment.
I believe so. I scoped this PR to the options that are present here, but I am confused about what the authoritative and comprehensive list is.
| None => { | ||
| let grpc_incoming = tonic::transport::server::TcpIncoming::from(grpc_listener); | ||
| thread_pool_handle | ||
| .spawn_traced_named("otlp-grpc-server", grpc_server.serve_with_incoming(grpc_incoming)); |
There was a problem hiding this comment.
Nit: can this thread_pool_handle statement be out of the match? Seems identical in both cases?
| .clone_key(); | ||
|
|
||
| // The Collector loads `ca_file` into `tls.Config.RootCAs`. A pure inbound receiver does not consult those roots, | ||
| // but loading and validating the file preserves the Collector's configuration behavior. |
There was a problem hiding this comment.
We don't actually do anything meaningful with the ca_file? What's the point, then? Per the OTEL docs, sounds like we should be using this to validate client certificates?
There was a problem hiding this comment.
Yes. As far as I can tell, the Core Agent behaves the same as well. It's effectively a no-op in both since the receiver is a server accepting inbound connections and that config option is only consulted when its a client validating a server.
Summary
Adds TLS support for both OTLP HTTP and gRPC servers.
Change Type
How did you test this PR?
Unit tests
References