Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion lib/saluki-components/src/common/datadog/config.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::time::Duration;

use facet::Facet;
use saluki_config::GenericConfiguration;
use saluki_config::{DurationString, GenericConfiguration};
use saluki_error::GenericError;
use saluki_io::net::client::http::{HttpProtocol, TlsMinimumVersion};
use serde::Deserialize;
Expand All @@ -25,6 +25,10 @@ const fn default_endpoint_buffer_size() -> usize {
16
}

const fn default_tls_handshake_timeout() -> DurationString {
DurationString::new(Duration::from_secs(10))
}

const fn default_forwarder_connection_reset_interval() -> u64 {
0
}
Expand Down Expand Up @@ -166,6 +170,14 @@ pub struct ForwarderConfiguration {
#[serde(default = "default_request_timeout_secs", rename = "forwarder_timeout")]
request_timeout_secs: u64,

/// TLS handshake timeout.
///
/// 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")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[facet(opaque)]
tls_handshake_timeout: DurationString,

/// Maximum number of pending requests for an individual endpoint.
///
/// Defaults to 16.
Expand Down Expand Up @@ -253,6 +265,11 @@ impl ForwarderConfiguration {
Duration::from_secs(self.request_timeout_secs)
}

/// Returns the TLS handshake timeout.
pub const fn tls_handshake_timeout(&self) -> Duration {
self.tls_handshake_timeout.as_duration()
}

/// Returns the maximum number of pending requests for an individual endpoint.
pub const fn endpoint_buffer_size(&self) -> usize {
self.endpoint_buffer_size
Expand Down Expand Up @@ -474,6 +491,21 @@ mod tests {
assert_eq!(proxies[0].uri().to_string(), PROXY_B_URI);
}

#[tokio::test]
async fn tls_handshake_timeout_accepts_duration_string() {
let config =
forwarder_config_from(config_with(serde_json::json!({ "tls_handshake_timeout": "10s" })), None).await;

assert_eq!(config.tls_handshake_timeout(), Duration::from_secs(10));
}

#[tokio::test]
async fn tls_handshake_timeout_defaults_to_10_seconds() {
let config = forwarder_config_from(base_config(), None).await;

assert_eq!(config.tls_handshake_timeout(), Duration::from_secs(10));
}

#[tokio::test]
async fn forwarder_http_protocol_defaults_to_auto() {
let config = forwarder_config_from(base_config(), None).await;
Expand Down
1 change: 1 addition & 0 deletions lib/saluki-components/src/common/datadog/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ where
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())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor TLS handshake timeout for proxied HTTPS

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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a follow up, since it would involve making a PR against the hyper-http-proxy fork.

.with_min_tls_version(config.min_tls_version())
.with_http_protocol(config.http_protocol())
.with_bytes_sent_counter(telemetry.bytes_sent().clone())
Expand Down
6 changes: 3 additions & 3 deletions lib/saluki-components/src/config_registry/classifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ mod tests {
// tests or choose a different key.
fn incompatible_non_default() {
let c = classifier();
let key = unsupported::TLS_HANDSHAKE_TIMEOUT.yaml_path();
let key = unsupported::AGGREGATOR_BUFFER_SIZE.yaml_path();
let result = c.classify(key, &Value::Number(999.into())).unwrap();
assert!(matches!(result.support_level, SupportLevel::Incompatible(_)));
assert!(!result.is_default);
Expand All @@ -126,8 +126,8 @@ mod tests {
#[test]
fn incompatible_default() {
let c = classifier();
let ann = &unsupported::TLS_HANDSHAKE_TIMEOUT;
let result = c.classify(ann.yaml_path(), &Value::String("".into())).unwrap();
let ann = &unsupported::AGGREGATOR_BUFFER_SIZE;
let result = c.classify(ann.yaml_path(), &Value::Number(100.into())).unwrap();
assert!(matches!(result.support_level, SupportLevel::Incompatible(_)));
assert!(result.is_default);
}
Expand Down
12 changes: 12 additions & 0 deletions lib/saluki-components/src/config_registry/datadog/forwarder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,18 @@ crate::declare_annotations! {
pipeline_affinity: PipelineAffinity::CrossCutting,
};

/// `tls_handshake_timeout`—TLS handshake timeout as a duration string.
TLS_HANDSHAKE_TIMEOUT = SalukiAnnotation {
schema: &schema::TLS_HANDSHAKE_TIMEOUT,
support_level: SupportLevel::Full,
additional_yaml_paths: &[],
env_var_override: None,
used_by: &[structs::FORWARDER_CONFIGURATION],
value_type_override: Some(ValueType::String),
test_json: Some("\"30s\""),
pipeline_affinity: PipelineAffinity::CrossCutting,
};

/// `forwarder_high_prio_buffer_size`—max pending requests per endpoint. Schema Float; field usize.
FORWARDER_HIGH_PRIO_BUFFER_SIZE = SalukiAnnotation {
schema: &schema::FORWARDER_HIGH_PRIO_BUFFER_SIZE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,20 +248,6 @@ crate::declare_annotations! {
pipeline_affinity: PipelineAffinity::CrossCutting,
};

/// `tls_handshake_timeout` - HTTP TLS handshake timeout.
TLS_HANDSHAKE_TIMEOUT = SalukiAnnotation {
schema: &schema::TLS_HANDSHAKE_TIMEOUT,
// Not implemented. Request timeout covers the gap. #178
support_level: SupportLevel::Incompatible(Severity::Medium),
additional_yaml_paths: &[],
env_var_override: None,
used_by: &[],
value_type_override: None,
test_json: None,
// TLS is process-wide.
pipeline_affinity: PipelineAffinity::CrossCutting,
};

/// `aggregator_buffer_size` - aggregator input channel depth.
AGGREGATOR_BUFFER_SIZE = SalukiAnnotation {
schema: &schema::AGGREGATOR_BUFFER_SIZE,
Expand Down
1 change: 1 addition & 0 deletions lib/saluki-io/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,6 @@ tokio-vsock = { workspace = true }
http-body-util = { workspace = true }
proptest = { workspace = true }
rand_distr = { workspace = true }
saluki-metrics = { workspace = true, features = ["test"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? I'm not seeing saluki-metrics used in saluki-io.

tempfile = { workspace = true }
tokio-test = { workspace = true }
13 changes: 13 additions & 0 deletions lib/saluki-io/src/net/client/http/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,19 @@ impl HttpClientBuilder {
self
}

/// Sets the timeout for completing the TLS handshake after a connection is established.
///
/// Defaults to 10 seconds.
///
Comment on lines +176 to +177

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Defaults to 10 seconds.
///

There is no default for this function that I can see 🤔

/// This timeout is independent of the per-request timeout set by [`Self::with_request_timeout`].
/// When a new connection is needed, both timers run concurrently and whichever fires first
/// determines the outcome. Setting a short request timeout does not bound the TLS handshake
/// if the connection pool decides to open a new connection before the request timer starts.
pub fn with_tls_handshake_timeout(mut self, timeout: Duration) -> Self {
self.connector_builder = self.connector_builder.with_tls_handshake_timeout(timeout);
self
}

/// Sets the HTTP protocol selection for client connections.
///
/// Defaults to [`HttpProtocol::Auto`], which automatically negotiates HTTP/2 with HTTP/1.1 fallback.
Expand Down
Loading
Loading