Skip to content

Commit c87d202

Browse files
olix0rolix0r
authored
refactor(http): consolidate HTTP protocol detection (#3720)
Linkerd's HTTP protocol detection logic is spread across a few crates: the linkerd-detect crate is generic over the actual protocol detection logic, and the linkerd-proxy-http crate provides an implementation. There are no other implemetations of the Detect interface. This leads to gnarly type signatures in the form `Result<Option<http::Variant>, DetectTimeoutError>`: simultaneously verbose and not particularly informative (what does the None case mean exactly). This commit introduces a new crate, `linkerd-http-detect`, consolidating this logic and removes the prior implementations. The admin, inbound, and outbound stacks are updated to use these new types. This work is done in anticipation of introducing metrics that report HTTP detection behavior. There are no functional changes.
1 parent 114ee8d commit c87d202

File tree

16 files changed

+445
-480
lines changed

16 files changed

+445
-480
lines changed

‎Cargo.lock

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1350,7 +1350,6 @@ dependencies = [
13501350
"ipnet",
13511351
"linkerd-addr",
13521352
"linkerd-conditional",
1353-
"linkerd-detect",
13541353
"linkerd-dns",
13551354
"linkerd-duplex",
13561355
"linkerd-errno",
@@ -1578,21 +1577,6 @@ dependencies = [
15781577
name = "linkerd-conditional"
15791578
version = "0.1.0"
15801579

1581-
[[package]]
1582-
name = "linkerd-detect"
1583-
version = "0.1.0"
1584-
dependencies = [
1585-
"async-trait",
1586-
"bytes",
1587-
"linkerd-error",
1588-
"linkerd-io",
1589-
"linkerd-stack",
1590-
"thiserror 2.0.11",
1591-
"tokio",
1592-
"tower",
1593-
"tracing",
1594-
]
1595-
15961580
[[package]]
15971581
name = "linkerd-distribute"
15981582
version = "0.1.0"
@@ -1730,6 +1714,23 @@ dependencies = [
17301714
"tracing",
17311715
]
17321716

1717+
[[package]]
1718+
name = "linkerd-http-detect"
1719+
version = "0.1.0"
1720+
dependencies = [
1721+
"bytes",
1722+
"httparse",
1723+
"linkerd-error",
1724+
"linkerd-http-variant",
1725+
"linkerd-io",
1726+
"linkerd-stack",
1727+
"linkerd-tracing",
1728+
"thiserror 2.0.11",
1729+
"tokio",
1730+
"tokio-test",
1731+
"tracing",
1732+
]
1733+
17331734
[[package]]
17341735
name = "linkerd-http-executor"
17351736
version = "0.1.0"
@@ -2265,11 +2266,11 @@ dependencies = [
22652266
"httparse",
22662267
"hyper",
22672268
"hyper-balance",
2268-
"linkerd-detect",
22692269
"linkerd-duplex",
22702270
"linkerd-error",
22712271
"linkerd-http-box",
22722272
"linkerd-http-classify",
2273+
"linkerd-http-detect",
22732274
"linkerd-http-executor",
22742275
"linkerd-http-h2",
22752276
"linkerd-http-insert",

‎Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ members = [
1616
"linkerd/app",
1717
"linkerd/conditional",
1818
"linkerd/distribute",
19-
"linkerd/detect",
2019
"linkerd/dns/name",
2120
"linkerd/dns",
2221
"linkerd/duplex",
@@ -28,6 +27,7 @@ members = [
2827
"linkerd/http/body-compat",
2928
"linkerd/http/box",
3029
"linkerd/http/classify",
30+
"linkerd/http/detect",
3131
"linkerd/http/executor",
3232
"linkerd/http/h2",
3333
"linkerd/http/insert",

‎linkerd/app/admin/src/stack.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use linkerd_app_core::{
22
classify,
33
config::ServerConfig,
4-
detect, drain, errors, identity,
4+
drain, errors, identity,
55
metrics::{self, FmtMetrics},
66
proxy::http,
77
serve,
@@ -136,19 +136,19 @@ impl Config {
136136
}))
137137
.push_filter(
138138
|(http, tcp): (
139-
Result<Option<http::Variant>, detect::DetectTimeoutError<_>>,
139+
http::Detection,
140140
Tcp,
141141
)| {
142142
match http {
143-
Ok(Some(version)) => Ok(Http { version, tcp }),
143+
http::Detection::Http(version) => Ok(Http { version, tcp }),
144144
// If detection timed out, we can make an educated guess at the proper
145145
// behavior:
146146
// - If the connection was meshed, it was most likely transported over
147147
// HTTP/2.
148148
// - If the connection was unmeshed, it was mostly likely HTTP/1.
149149
// - If we received some unexpected SNI, the client is mostly likely
150150
// confused/stale.
151-
Err(_timeout) => {
151+
http::Detection::ReadTimeout(_timeout) => {
152152
let version = match tcp.tls {
153153
tls::ConditionalServerTls::None(_) => http::Variant::Http1,
154154
tls::ConditionalServerTls::Some(tls::ServerTls::Established {
@@ -166,7 +166,7 @@ impl Config {
166166
}
167167
// If the connection failed HTTP detection, check if we detected TLS for
168168
// another target. This might indicate that the client is confused/stale.
169-
Ok(None) => match tcp.tls {
169+
http::Detection::Empty | http::Detection::NotHttp => match tcp.tls {
170170
tls::ConditionalServerTls::Some(tls::ServerTls::Passthru { sni }) => {
171171
Err(UnexpectedSni(sni, tcp.client).into())
172172
}
@@ -177,8 +177,8 @@ impl Config {
177177
)
178178
.arc_new_tcp()
179179
.lift_new_with_target()
180-
.push(detect::NewDetectService::layer(svc::stack::CloneParam::from(
181-
detect::Config::<http::DetectHttp>::from_timeout(DETECT_TIMEOUT),
180+
.push(http::NewDetect::layer(svc::stack::CloneParam::from(
181+
http::DetectParams { read_timeout: DETECT_TIMEOUT }
182182
)))
183183
.push(transport::metrics::NewServer::layer(metrics.proxy.transport))
184184
.push_map_target(move |(tls, addrs): (tls::ConditionalServerTls, B::Addrs)| {

‎linkerd/app/core/Cargo.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ pin-project = "1"
3434
linkerd-addr = { path = "../../addr" }
3535
linkerd-conditional = { path = "../../conditional" }
3636
linkerd-dns = { path = "../../dns" }
37-
linkerd-detect = { path = "../../detect" }
3837
linkerd-duplex = { path = "../../duplex" }
3938
linkerd-errno = { path = "../../errno" }
4039
linkerd-error = { path = "../../error" }

‎linkerd/app/core/src/config.rs

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
pub use crate::exp_backoff::ExponentialBackoff;
22
use crate::{
3-
proxy::http::{self, h1, h2},
4-
svc::{queue, CloneParam, ExtractParam, Param},
3+
proxy::http::{h1, h2},
4+
svc::{queue, ExtractParam, Param},
55
transport::{DualListenAddr, Keepalive, ListenAddr, UserTimeout},
66
};
77
use std::time::Duration;
@@ -59,14 +59,6 @@ impl<T> ExtractParam<queue::Timeout, T> for QueueConfig {
5959
}
6060
}
6161

62-
// === impl ProxyConfig ===
63-
64-
impl ProxyConfig {
65-
pub fn detect_http(&self) -> CloneParam<linkerd_detect::Config<http::DetectHttp>> {
66-
linkerd_detect::Config::from_timeout(self.detect_protocol_timeout).into()
67-
}
68-
}
69-
7062
// === impl ServerConfig ===
7163

7264
impl Param<DualListenAddr> for ServerConfig {

‎linkerd/app/core/src/lib.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ pub use drain;
3232
pub use ipnet::{IpNet, Ipv4Net, Ipv6Net};
3333
pub use linkerd_addr::{self as addr, Addr, AddrMatch, IpMatch, NameAddr, NameMatch};
3434
pub use linkerd_conditional::Conditional;
35-
pub use linkerd_detect as detect;
3635
pub use linkerd_dns;
3736
pub use linkerd_error::{cause_ref, is_caused_by, Error, Infallible, Recover, Result};
3837
pub use linkerd_exp_backoff as exp_backoff;

‎linkerd/app/inbound/src/detect.rs

Lines changed: 45 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use crate::{
33
Inbound,
44
};
55
use linkerd_app_core::{
6-
detect, identity, io,
6+
identity, io,
77
metrics::ServerLabel,
88
proxy::http,
99
svc, tls,
@@ -48,9 +48,6 @@ struct Detect {
4848
tls: Tls,
4949
}
5050

51-
#[derive(Copy, Clone, Debug)]
52-
struct ConfigureHttpDetect;
53-
5451
#[derive(Clone)]
5552
struct TlsParams {
5653
timeout: tls::server::Timeout,
@@ -111,42 +108,58 @@ impl Inbound<svc::ArcNewTcp<Http, io::BoxedIo>> {
111108
.push_switch(
112109
|(detected, Detect { tls, .. })| -> Result<_, Infallible> {
113110
match detected {
114-
Ok(Some(http)) => Ok(svc::Either::A(Http { http, tls })),
115-
Ok(None) => Ok(svc::Either::B(tls)),
111+
http::Detection::Http(http) => Ok(svc::Either::A(Http { http, tls })),
112+
http::Detection::Empty | http::Detection::NotHttp => {
113+
Ok(svc::Either::B(tls))
114+
}
116115
// When HTTP detection fails, forward the connection to the application as
117116
// an opaque TCP stream.
118-
Err(timeout) => match tls.policy.protocol() {
119-
Protocol::Http1 { .. } => {
120-
// If the protocol was hinted to be HTTP/1.1 but detection
121-
// failed, we'll usually be handling HTTP/1, but we may actually
122-
// be handling HTTP/2 via protocol upgrade. Our options are:
123-
// handle the connection as HTTP/1, assuming it will be rare for
124-
// a proxy to initiate TLS, etc and not send the 16B of
125-
// connection header; or we can handle it as opaque--but there's
126-
// no chance the server will be able to handle the H2 protocol
127-
// upgrade. So, it seems best to assume it's HTTP/1 and let the
128-
// proxy handle the protocol error if we're in an edge case.
129-
info!(%timeout, "Handling connection as HTTP/1 due to policy");
130-
Ok(svc::Either::A(Http {
131-
http: http::Variant::Http1,
132-
tls,
133-
}))
117+
http::Detection::ReadTimeout(timeout) => {
118+
match tls.policy.protocol() {
119+
Protocol::Http1 { .. } => {
120+
// If the protocol was hinted to be HTTP/1.1 but detection
121+
// failed, we'll usually be handling HTTP/1, but we may actually
122+
// be handling HTTP/2 via protocol upgrade. Our options are:
123+
// handle the connection as HTTP/1, assuming it will be rare for
124+
// a proxy to initiate TLS, etc and not send the 16B of
125+
// connection header; or we can handle it as opaque--but there's
126+
// no chance the server will be able to handle the H2 protocol
127+
// upgrade. So, it seems best to assume it's HTTP/1 and let the
128+
// proxy handle the protocol error if we're in an edge case.
129+
info!(
130+
?timeout,
131+
"Handling connection as HTTP/1 due to policy"
132+
);
133+
Ok(svc::Either::A(Http {
134+
http: http::Variant::Http1,
135+
tls,
136+
}))
137+
}
138+
// Otherwise, the protocol hint must have
139+
// been `Detect` or the protocol was updated
140+
// after detection was initiated, otherwise
141+
// we would have avoided detection below.
142+
// Continue handling the connection as if it
143+
// were opaque.
144+
_ => {
145+
info!(
146+
?timeout,
147+
"Handling connection as opaque due to policy"
148+
);
149+
Ok(svc::Either::B(tls))
150+
}
134151
}
135-
// Otherwise, the protocol hint must have been `Detect` or the
136-
// protocol was updated after detection was initiated, otherwise we
137-
// would have avoided detection below. Continue handling the
138-
// connection as if it were opaque.
139-
_ => {
140-
info!(%timeout, "Handling connection as opaque");
141-
Ok(svc::Either::B(tls))
142-
}
143-
},
152+
}
144153
}
145154
},
146155
forward.into_inner(),
147156
)
148157
.lift_new_with_target()
149-
.push(detect::NewDetectService::layer(ConfigureHttpDetect))
158+
.push(http::NewDetect::layer(|Detect { timeout, .. }: &Detect| {
159+
http::DetectParams {
160+
read_timeout: *timeout,
161+
}
162+
}))
150163
.arc_new_tcp();
151164

152165
http.push_on_service(svc::MapTargetLayer::new(io::BoxedIo::new))
@@ -332,14 +345,6 @@ impl svc::Param<tls::ConditionalServerTls> for Tls {
332345
}
333346
}
334347

335-
// === impl ConfigureHttpDetect ===
336-
337-
impl svc::ExtractParam<detect::Config<http::DetectHttp>, Detect> for ConfigureHttpDetect {
338-
fn extract_param(&self, detect: &Detect) -> detect::Config<http::DetectHttp> {
339-
detect::Config::from_timeout(detect.timeout)
340-
}
341-
}
342-
343348
// === impl Http ===
344349

345350
impl svc::Param<http::Variant> for Http {

‎linkerd/app/outbound/src/ingress.rs

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::{http, opaq, policy, Discovery, Outbound, ParentRef};
22
use linkerd_app_core::{
3-
detect, errors, io, profiles,
3+
errors, io, profiles,
44
proxy::{
55
api_resolve::{ConcreteAddr, Metadata},
66
core::Resolve,
@@ -274,8 +274,6 @@ impl<N> Outbound<N> {
274274
NSvc::Future: Send,
275275
{
276276
self.map_stack(|config, rt, inner| {
277-
let detect_http = config.proxy.detect_http();
278-
279277
// Route requests with destinations that can be discovered via the
280278
// `l5d-dst-override` header through the (load balanced) logical
281279
// stack. Route requests without the header through the endpoint
@@ -292,10 +290,12 @@ impl<N> Outbound<N> {
292290
.push_on_service(
293291
svc::layers()
294292
.push(http::BoxRequest::layer())
295-
.push(http::strip_header::request::layer(DST_OVERRIDE_HEADER))
293+
.push(http::strip_header::request::layer(DST_OVERRIDE_HEADER)),
296294
)
297295
.lift_new()
298-
.push(svc::NewOneshotRoute::layer_via(|t: &Http<T>| SelectTarget(t.clone())))
296+
.push(svc::NewOneshotRoute::layer_via(|t: &Http<T>| {
297+
SelectTarget(t.clone())
298+
}))
299299
.check_new_service::<Http<T>, http::Request<_>>();
300300

301301
// HTTP detection is **always** performed. If detection fails, then we
@@ -307,26 +307,33 @@ impl<N> Outbound<N> {
307307
let h2 = config.proxy.server.http2.clone();
308308
let drain = rt.drain.clone();
309309
move |http: &Http<T>| http::ServerParams {
310-
version: http.version,
311-
http2: h2.clone(),
312-
drain: drain.clone()
310+
version: http.version,
311+
http2: h2.clone(),
312+
drain: drain.clone(),
313313
}
314314
}))
315315
.check_new_service::<Http<T>, I>()
316316
.push_switch(
317-
|(detected, target): (detect::Result<http::Variant>, T)| -> Result<_, Infallible> {
318-
if let Some(version) = detect::allow_timeout(detected) {
319-
return Ok(svc::Either::A(Http {
320-
version,
321-
parent: target,
322-
}));
317+
|(detected, parent): (http::Detection, T)| -> Result<_, Infallible> {
318+
match detected {
319+
http::Detection::Http(version) => {
320+
return Ok(svc::Either::A(Http { version, parent }));
321+
}
322+
http::Detection::ReadTimeout(timeout) => {
323+
tracing::info!("Continuing after timeout: {timeout:?}");
324+
}
325+
_ => {}
323326
}
324-
Ok(svc::Either::B(target))
327+
Ok(svc::Either::B(parent))
325328
},
326329
fallback,
327330
)
328331
.lift_new_with_target()
329-
.push(detect::NewDetectService::layer(detect_http))
332+
.push(http::NewDetect::layer(svc::CloneParam::from(
333+
http::DetectParams {
334+
read_timeout: config.proxy.detect_protocol_timeout,
335+
},
336+
)))
330337
.arc_new_tcp()
331338
})
332339
}

0 commit comments

Comments
 (0)