Conversation
lucastemb
marked this pull request as ready for review
June 4, 2026 17:52
dd-octo-sts
Bot
added
area/io
![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: dc7d606ddd
ℹ️ 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".
| let endpoints = config.build_routable_endpoints(live_config.clone())?; | ||
| let mut client_builder = HttpClient::builder() | ||
| .with_request_timeout(config.request_timeout()) | ||
| .with_tls_handshake_timeout(config.tls_handshake_timeout()) |
There was a problem hiding this comment.
When proxy_https is configured, this timeout only reaches the inner connector; HttpClientBuilder::build then wraps it in hyper_http_proxy::ProxyConnector, whose CONNECT path performs the destination TLS handshake itself after opening the tunnel. In that common forwarder-proxy setup, a stalled TLS handshake to the intake still ignores tls_handshake_timeout and waits for the broader request timeout, even though the config is now classified as fully supported.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Worth a follow up, since it would involve making a PR against the hyper-http-proxy fork.
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (35)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 ( |
jszwedko
reviewed
Jun 4, 2026
Binary Size Analysis (Agent Data Plane)Baseline: b8dd422 · Comparison: 43275bd · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
jszwedko
reviewed
Jun 4, 2026
| /// | ||
| /// Defaults to 10 seconds. If the TLS handshake does not complete within this duration after the | ||
| /// TCP connection is established, the connection attempt fails with a timeout error. | ||
| #[serde(default = "default_tls_handshake_timeout_secs", rename = "tls_handshake_timeout")] |
Collaborator
There was a problem hiding this comment.
Will this parse durations correctly? The config coming from the Core Agent will be a String "Go duration" like 10s.
Contributor
Author
There was a problem hiding this comment.
Good call out. I tried to base it off of some of the other config keys in ForwarderConfiguration, but I think the same logic does not apply since things like forwarder_timeout are based off just pure seconds. As a result, I also changed the name to tls_handshake_timeout to reflect this
jszwedko
reviewed
Jun 4, 2026
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
Collaborator
There was a problem hiding this comment.
Did you mean to delete these tests?
Contributor
Author
There was a problem hiding this comment.
I did because configure_tls_alpn_for_http_protocol was no longer a standalone function after refactoring, but I should've probably modified the tests instead of deleting altogether.
jszwedko
reviewed
Jun 4, 2026
jszwedko
reviewed
Jun 4, 2026
| @@ -378,27 +382,51 @@ impl Service<Uri> for HttpsCapableConnector { | |||
| } | |||
|
|
|||
| fn call(&mut self, dst: Uri) -> Self::Future { | |||
Collaborator
jszwedko
reviewed
Jun 4, 2026
jszwedko
reviewed
Jun 4, 2026
jszwedko
reviewed
Jun 4, 2026
This comment has been minimized.
This comment has been minimized.
jszwedko
reviewed
Jun 5, 2026
jszwedko
left a comment
jszwedko
left a comment
Collaborator
There was a problem hiding this comment.
A few more comments, and I'd still like more review of the changes to conn.rs.
| /// | ||
| /// Defaults to 10 seconds. If the TLS handshake does not complete within this duration after the | ||
| /// TCP connection is established, the connection attempt fails with a timeout error. | ||
| #[serde(default = "default_tls_handshake_timeout", rename = "tls_handshake_timeout")] |
Collaborator
There was a problem hiding this comment.
Suggested change
| #[serde(default = "default_tls_handshake_timeout", rename = "tls_handshake_timeout")] | |
| #[serde(default = "default_tls_handshake_timeout"] |
I don't think we need the rename now that the field name matches.
Comment on lines
+176
to
+177
| /// Defaults to 10 seconds. | ||
| /// |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// Defaults to 10 seconds. | |
| /// |
There is no default for this function that I can see 🤔
| use std::sync::Arc; | ||
|
|
||
| use tower::Service as _; | ||
|
|
Collaborator
There was a problem hiding this comment.
Did you mean to remove this? I think it is actually needed 🤔
| http-body-util = { workspace = true } | ||
| proptest = { workspace = true } | ||
| rand_distr = { workspace = true } | ||
| saluki-metrics = { workspace = true, features = ["test"] } |
Collaborator
There was a problem hiding this comment.
Is this needed? I'm not seeing saluki-metrics used in saluki-io.
4 tasks
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
## 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>
dd-octo-sts Bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
## 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
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
2 participants
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.
Summary
Added workaround to allow for
tls_handshake_timeoutconfig option.tokio-rustlsprovides no built-in timeout, so handshakes could be potentially infinite. The idea here is to replacehyper-rustls'sHttpsConnectorwith direct ownership of the TLS layer so that we can wrap it in a timeout with the time specified through the config option.Change Type
How did you test this PR?
Unit tests
References