feat(tls): add tls_handshake_timeout config option - #2303
Conversation
Owns the TLS handshake step of outbound HTTPS connections directly instead of delegating to hyper_rustls's fused connect-and-handshake future, so a timeout can be scoped to just the handshake rather than the whole connect+handshake duration. Wires this through as a new tls_handshake_timeout Datadog config option, defaulting to 10s.
dd-octo-sts
Bot
added
area/io
Binary Size Analysis (Agent Data Plane)Baseline: 76206c6 · Comparison: b75e898 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The shared Agent key accepts 0 to disable the TLS handshake deadline, but the new connector turns it into an immediate Tokio timeout. Deployments carrying that valid override would lose all new direct HTTPS intake connections.
📊 Validated against 4 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit cc34cf1 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc34cf17fa
ℹ️ 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".
| .with_request_timeout(config.request_timeout()) | ||
| .with_max_idle_conns_per_host(config.max_idle_connections_per_host()) | ||
| .with_min_tls_version(config.min_tls_version()) | ||
| .with_tls_handshake_timeout(config.tls_handshake_timeout()) |
There was a problem hiding this comment.
When an HTTPS intake is reached through a configured proxy, hyper_http_proxy::ProxyConnector calls this connector only for the proxy transport and performs the destination TLS handshake itself after CONNECT. Consequently, the configured timeout here never bounds the intake handshake (for an HTTPS proxy it only bounds the proxy-side handshake), so a stalled destination handshake lasts until the overall request timeout instead of tls_handshake_timeout; the timeout must also wrap the proxy connector's post-CONNECT TLS step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Sonnet 5] This is a known, pre-existing limitation, not something introduced by this PR — hyper_http_proxy::ProxyConnector performs its own TLS handshake after CONNECT for HTTPS-through-proxy, independent of this connector. tls_handshake_timeout can't bound that step today. Leaving this out of scope for this PR; it's already called out as a known limitation in the PR description.
I'll follow up to add support there.
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
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 ( |
The shared Agent key allows 0 to disable the TLS handshake deadline, but tokio::time::timeout fires immediately on a zero duration, so a valid override to disable the deadline caused every direct HTTPS intake connection to fail its handshake.
Reject unsupported URI schemes explicitly instead of silently falling back to plaintext, strip IPv6 brackets before constructing the TLS server name, and correct ALPN documentation/test for HTTP/1.1-only mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| @@ -115,7 +115,9 @@ mod tests { | |||
| #[test] | |||
| fn incompatible_non_default() { | |||
There was a problem hiding this comment.
The tests in this file needed to be updated now that tls_handshake_timeout is a supported option.
HttpProtocol::Http1 previously advertised "http/1.1" via ALPN, a behavior change from main's hyper-rustls-based connector, which leaves ALPN unset in HTTP/1.1-only mode. Restore that behavior since it wasn't an intentional part of this PR.
|
|
||
| #[test] | ||
| fn auto_protocol_advertises_h2_and_http1_alpn() { | ||
| let tls_config = configure_tls_alpn_for_http_protocol(empty_tls_config(), HttpProtocol::Auto); |
There was a problem hiding this comment.
Previously these tests, strangely, seemed to actually only be testing test code (the previously existing configure_tls_alpn_for_http_protocol was gated with cfg(test)).
Exercises HttpClient end-to-end against a real TCP+TLS server that accepts the connection but never completes the handshake, following the existing send_request_to_tls_server pattern in this file.
check-docs rejects "e.g." per our style guide (Google.Latin).
lucastemb
left a comment
lucastemb
left a comment
There was a problem hiding this comment.
Non-blocking PR comment. Looks good 👍
saluki-io is a source-agnostic library and shouldn't reference the Datadog Agent.
4d40f35
into
main
## Human Summary Adds support for `tls_handlshake_timeout` using [newly documented method in tokio-rustls](rustls/tokio-rustls#187). This is roughly follows #178 but based on latest main. It doesn't handle configuring the connect timeout when using an HTTPS proxy. That requires changes to hyper-http-proxy so I'll follow up with that. Closes: #178 ## AI Summary TLS handshakes to the Datadog intake have no built-in timeout independent of the overall request timeout: `hyper_rustls`'s connector fuses the transport connect and the TLS handshake into a single opaque future, so a stalled handshake (e.g. a peer that accepts the TCP connection but never completes the TLS negotiation) is only bounded by `forwarder_timeout`, which is meant to bound the whole request, not just the handshake. This adds a `tls_handshake_timeout` config option by having the HTTP client connector own the TLS layer directly, so it can time out just the handshake step and still distinguish that failure mode from a slow request. This picks up the intent of #1819, an older PR for the same issue, rewritten against the current typed configuration system rather than resurrected via rebase. ```mermaid sequenceDiagram participant Before as Before (hyper_rustls::HttpsConnector) participant After as After (owned TLS layer) Note over Before: connect + handshake fused into one future Before->>Before: TCP connect Before->>Before: TLS handshake Note over Before: only forwarder_timeout bounds both steps combined Note over After: connect and handshake are separate steps After->>After: TCP connect (connect_timeout) After->>After: TLS handshake (tls_handshake_timeout, new) Note over After: a stalled handshake times out on its own,<br/>without racing the whole request ``` ## Test plan - [x] Added `tls_handshake_timeout` to the Datadog config schema overlay (`support: full`) and wired it through the typed config system (`DatadogTranslator`, `SalukiConfiguration`) and the legacy `ForwarderConfiguration` facet-based config, both consumed by the HTTP client builder. - [x] Added/updated unit tests in `saluki-io`'s `conn.rs` for the new connector split (ALPN protocol selection, including an explicit `http/1.1` ALPN advertisement for `HttpProtocol::Http1` to avoid an ALPN regression from the previous implicit behavior). - [x] Existing `config_smoke::smoke_test` in `saluki-components` (`ForwarderConfiguration`) exercises the new field's default/deserialization against the config registry. - [x] Updated classifier unit tests in `datadog-agent-config` that previously used `tls_handshake_timeout` as an example unsupported/incompatible key, substituting other still-unsupported keys since this key is now fully supported. ## Known limitation Connections made through `proxy_https` bypass this connector and aren't covered by `tls_handshake_timeout` (flagged on the original PR). Left out of scope here; can be addressed separately if needed. Co-authored-by: jesse.szwedko <jesse.szwedko@datadoghq.com> 4d40f35
2 participants
Human Summary
Adds support for
tls_handlshake_timeoutusing newly documented method in tokio-rustls. This is roughly follows #178 but based on latest main.It doesn't handle configuring the connect timeout when using an HTTPS proxy. That requires changes to hyper-http-proxy so I'll follow up with that.
Closes: #178
AI Summary
TLS handshakes to the Datadog intake have no built-in timeout independent of the overall request timeout:
hyper_rustls's connector fuses the transport connect and the TLS handshake into a single opaque future, so a stalled handshake (e.g. a peer that accepts the TCP connection but never completes the TLS negotiation) is only bounded byforwarder_timeout, which is meant to bound the whole request, not just the handshake. This adds atls_handshake_timeoutconfig option by having the HTTP client connector own the TLS layer directly, so it can time out just the handshake step and still distinguish that failure mode from a slow request. This picks up the intent of #1819, an older PR for the same issue, rewritten against the current typed configuration system rather than resurrected via rebase.sequenceDiagram participant Before as Before (hyper_rustls::HttpsConnector) participant After as After (owned TLS layer) Note over Before: connect + handshake fused into one future Before->>Before: TCP connect Before->>Before: TLS handshake Note over Before: only forwarder_timeout bounds both steps combined Note over After: connect and handshake are separate steps After->>After: TCP connect (connect_timeout) After->>After: TLS handshake (tls_handshake_timeout, new) Note over After: a stalled handshake times out on its own,<br/>without racing the whole requestTest plan
tls_handshake_timeoutto the Datadog config schema overlay (support: full) and wired it through the typed config system (DatadogTranslator,SalukiConfiguration) and the legacyForwarderConfigurationfacet-based config, both consumed by the HTTP client builder.saluki-io'sconn.rsfor the new connector split (ALPN protocol selection, including an explicithttp/1.1ALPN advertisement forHttpProtocol::Http1to avoid an ALPN regression from the previous implicit behavior).config_smoke::smoke_testinsaluki-components(ForwarderConfiguration) exercises the new field's default/deserialization against the config registry.datadog-agent-configthat previously usedtls_handshake_timeoutas an example unsupported/incompatible key, substituting other still-unsupported keys since this key is now fully supported.Known limitation
Connections made through
proxy_httpsbypass this connector and aren't covered bytls_handshake_timeout(flagged on the original PR). Left out of scope here; can be addressed separately if needed.