Conversation
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 👍 / 👎.
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 ( |
Binary Size Analysis (Agent Data Plane)Baseline: b8dd422 · Comparison: 43275bd · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
| /// | ||
| /// 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")] |
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.
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
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Did you mean to delete these tests?
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.
| @@ -378,27 +382,51 @@ impl Service<Uri> for HttpsCapableConnector { | |||
| } | |||
|
|
|||
| fn call(&mut self, dst: Uri) -> Self::Future { | |||
This comment has been minimized.
This comment has been minimized.
jszwedko
left a comment
jszwedko
left a comment
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")] |
There was a problem hiding this comment.
| #[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.
| /// Defaults to 10 seconds. | ||
| /// |
There was a problem hiding this comment.
| /// Defaults to 10 seconds. | |
| /// |
There is no default for this function that I can see 🤔
| use std::sync::Arc; | ||
|
|
||
| use tower::Service as _; | ||
|
|
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"] } |
There was a problem hiding this comment.
Is this needed? I'm not seeing saluki-metrics used in saluki-io.
2 participants
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