Repository navigation
client/server timeout examples, doc pass hinting towards timeouts. - #187
Conversation
ctz
left a comment
There was a problem hiding this comment.
I would perhaps consider folding the timeout-having examples into the others? As this kind of stuff tends to end up being the starting point for some developments, and it's easy to remove timeouts that are counterproductive than know that they are good practice in the first place?
Rewrote the history to do that. |
* Avoid the `rustls::` prefix for `RootCertStore` and `ClientConfig` * Consistently use the `tokio_rustls::rustls` re-export.
Demonstrates using tokio-rustls and externally applying timeouts to TCP connect, TLS handshake, reads/writes on a TLS stream, and graceful shutdown.
* Avoid the `rustls::` prefix for `ServerConfig`. * Consistently use the `tokio_rustls::rustls` re-export.
Always echo data read from the client back to it, removing the -e/--echo_mode flag and the fixed HTTP 200 response mode. This keeps the example focused on demonstrating tokio-rustls rather than showing two ways to respond. IMO a fixed HTTP response that doesn't take into account any details of the request isn't especially useful.
Adds a -l/--lazy flag that performs the TLS handshake with LazyConfigAcceptor instead of TlsAcceptor, demonstrating access to the ClientHello (e.g. for SNI-based config selection) before completing the handshake with a chosen ServerConfig.
Demonstrates using tokio-rustls to accept & handle a client request (both directly, and lazily), while applying timeouts to the TLS handshake, individual reads/writes on the established stream, and the graceful TLS shutdown (close_notify). In the lazy path a single Instant deadline (via timeout_at) spans both the wait for the ClientHello and the rest of the handshake, and take_io() is used to answer a stalled client in plaintext.
This comment was marked as outdated.
This comment was marked as outdated.
|
@cpu I just saw this PR. All good, though personally, outside of strict rustls context, I think one would need only the TLS handshake timeout from rustls. For the raw socket timeouts it's better to use |
## 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>
## 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
Here's an attempt at exploring an alternative to #182 that adds client and server examples demonstrating thorough use of timeouts applied external to the core crate. This is mostly straight forward (though some extra care is needed w/ the lazy acceptor and understanding the dual await points).
In addition to the updated examples I sprinkled the main API surfaces with rustdoc comments that hinted at the need for considering timeouts and offered some advice.
I was skeptical initially, but I think this might be a better route forwards compared to #182. If folks agree I'll pair it with an example in hyper-rustls, and then close 182 and associated bits.