From e513d3f2939f555b40faff1b1f72b870e42c381e Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 17 Mar 2025 00:00:00 +0000 Subject: [PATCH 1/7] refactor(http/upgrade): remove `HttpConnect` extension `linkerd-http-upgrade` provides a `HttpConnect` type that is intended for use as a response extension. this commit performs a refactor, removing this type. we use this extension in a single piece of tower middleware. typically, these sorts of extensions are intended for e.g. passing state between distinct layers of tower middleware, or otherwise facilitating extensions to the HTTP family of protocols. this extension is only constructed and subsequently referenced within a single file, in the `linkerd_proxy_http::http::h1::Client`. we can perform the same task by using the `is_http_connect` boolean we use to conditionally insert this extension. Signed-off-by: katelyn martin --- linkerd/http/upgrade/src/upgrade.rs | 5 ----- linkerd/proxy/http/src/h1.rs | 23 +++++++---------------- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/linkerd/http/upgrade/src/upgrade.rs b/linkerd/http/upgrade/src/upgrade.rs index 14bbc6ffa1..5540266981 100644 --- a/linkerd/http/upgrade/src/upgrade.rs +++ b/linkerd/http/upgrade/src/upgrade.rs @@ -37,11 +37,6 @@ struct Http11UpgradeHalves { client: Http11Upgrade, } -/// A marker type inserted into Extensions to signal it was an HTTP CONNECT -/// request. -#[derive(Debug)] -pub struct HttpConnect; - struct Inner { server: TryLock>, client: TryLock>, diff --git a/linkerd/proxy/http/src/h1.rs b/linkerd/proxy/http/src/h1.rs index b43dd1874a..30b8b7f87b 100644 --- a/linkerd/proxy/http/src/h1.rs +++ b/linkerd/proxy/http/src/h1.rs @@ -6,10 +6,7 @@ use http::{ }; use linkerd_error::{Error, Result}; use linkerd_http_box::BoxBody; -use linkerd_http_upgrade::{ - glue::HyperConnect, - upgrade::{Http11Upgrade, HttpConnect}, -}; +use linkerd_http_upgrade::{glue::HyperConnect, upgrade::Http11Upgrade}; use linkerd_stack::MakeConnection; use std::{pin::Pin, time::Duration}; use tracing::{debug, trace}; @@ -139,12 +136,6 @@ where Box::pin(async move { let mut rsp = rsp_fut.await?; if is_http_connect { - // Add an extension to indicate that this a response to a CONNECT request. - debug_assert!( - upgrade.is_some(), - "Upgrade extension must be set on CONNECT requests" - ); - rsp.extensions_mut().insert(HttpConnect); // Strip headers that may not be transmitted to the server, per RFC 9110: // // > A server MUST NOT send any `Transfer-Encoding` or `Content-Length` header @@ -159,7 +150,7 @@ where } } - if is_upgrade(&rsp) { + if is_upgrade(&rsp, is_http_connect) { trace!("Client response is HTTP/1.1 upgrade"); if let Some(upgrade) = upgrade { upgrade.insert_half(hyper::upgrade::on(&mut rsp))?; @@ -174,15 +165,15 @@ where } /// Checks responses to determine if they are successful HTTP upgrades. -fn is_upgrade(res: &http::Response) -> bool { +fn is_upgrade(res: &http::Response, is_http_connect: bool) -> bool { #[inline] - fn is_connect_success(res: &http::Response) -> bool { - res.extensions().get::().is_some() && res.status().is_success() + fn is_connect_success(res: &http::Response, is_http_connect: bool) -> bool { + is_http_connect && res.status().is_success() } // Upgrades were introduced in HTTP/1.1 if res.version() != http::Version::HTTP_11 { - if is_connect_success(res) { + if is_connect_success(res, is_http_connect) { tracing::warn!( "A successful response to a CONNECT request had an incorrect HTTP version \ (expected HTTP/1.1, got {:?})", @@ -198,7 +189,7 @@ fn is_upgrade(res: &http::Response) -> bool { } // CONNECT requests are complete if status code is 2xx. - if is_connect_success(res) { + if is_connect_success(res, is_http_connect) { return true; } From 75ede4ccfe2588866d15e79c176ee34da32ed452 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 17 Mar 2025 00:00:00 +0000 Subject: [PATCH 2/7] refactor(proxy/http): fold helper function this removes a helper function for a computation whose amortization is no longer as helpful. now that we are passing `is_http_connect` down into this function, we are no longer inspecting the response's extensions. because of that, the only work to do is to check the status code, which is a very cheap comparison. Signed-off-by: katelyn martin --- linkerd/proxy/http/src/h1.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/linkerd/proxy/http/src/h1.rs b/linkerd/proxy/http/src/h1.rs index 30b8b7f87b..08a0f05c2b 100644 --- a/linkerd/proxy/http/src/h1.rs +++ b/linkerd/proxy/http/src/h1.rs @@ -166,14 +166,9 @@ where /// Checks responses to determine if they are successful HTTP upgrades. fn is_upgrade(res: &http::Response, is_http_connect: bool) -> bool { - #[inline] - fn is_connect_success(res: &http::Response, is_http_connect: bool) -> bool { - is_http_connect && res.status().is_success() - } - // Upgrades were introduced in HTTP/1.1 if res.version() != http::Version::HTTP_11 { - if is_connect_success(res, is_http_connect) { + if is_http_connect && res.status().is_success() { tracing::warn!( "A successful response to a CONNECT request had an incorrect HTTP version \ (expected HTTP/1.1, got {:?})", @@ -189,7 +184,7 @@ fn is_upgrade(res: &http::Response, is_http_connect: bool) -> bool { } // CONNECT requests are complete if status code is 2xx. - if is_connect_success(res, is_http_connect) { + if is_http_connect && res.status().is_success() { return true; } From fe7ee2286ba44158a8395b64d36fde5a1f85c6d8 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 17 Mar 2025 00:00:00 +0000 Subject: [PATCH 3/7] refactor(proxy/http): match on response status this commit refactors a sequence of conditional blocks in a helper function used to identity HTTP/1.1 upgrades. this commit replaces this sequence of conditional blocks with a match statement. Signed-off-by: katelyn martin --- linkerd/proxy/http/src/h1.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/linkerd/proxy/http/src/h1.rs b/linkerd/proxy/http/src/h1.rs index 08a0f05c2b..13a9e95cbf 100644 --- a/linkerd/proxy/http/src/h1.rs +++ b/linkerd/proxy/http/src/h1.rs @@ -178,18 +178,13 @@ fn is_upgrade(res: &http::Response, is_http_connect: bool) -> bool { return false; } - // 101 Switching Protocols - if res.status() == http::StatusCode::SWITCHING_PROTOCOLS { - return true; + match res.status() { + http::StatusCode::SWITCHING_PROTOCOLS => true, + // CONNECT requests are complete if status code is 2xx. + status if is_http_connect && status.is_success() => true, + // Just a regular HTTP response... + _ => false, } - - // CONNECT requests are complete if status code is 2xx. - if is_http_connect && res.status().is_success() { - return true; - } - - // Just a regular HTTP response... - false } /// Returns if the request target is in `absolute-form`. From 061e7f09ea8be1ea1c6b901b54b9fdd0d15b6c2b Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 17 Mar 2025 00:00:00 +0000 Subject: [PATCH 4/7] nit(proxy/http): rename `res` to `rsp` we follow a convention where we tend to name responses `rsp`, not `res` or `resp`. this commit applies that convention to this helper function. Signed-off-by: katelyn martin --- linkerd/proxy/http/src/h1.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/linkerd/proxy/http/src/h1.rs b/linkerd/proxy/http/src/h1.rs index 13a9e95cbf..9cf85d4734 100644 --- a/linkerd/proxy/http/src/h1.rs +++ b/linkerd/proxy/http/src/h1.rs @@ -165,20 +165,20 @@ where } /// Checks responses to determine if they are successful HTTP upgrades. -fn is_upgrade(res: &http::Response, is_http_connect: bool) -> bool { +fn is_upgrade(rsp: &http::Response, is_http_connect: bool) -> bool { // Upgrades were introduced in HTTP/1.1 - if res.version() != http::Version::HTTP_11 { - if is_http_connect && res.status().is_success() { + if rsp.version() != http::Version::HTTP_11 { + if is_http_connect && rsp.status().is_success() { tracing::warn!( "A successful response to a CONNECT request had an incorrect HTTP version \ (expected HTTP/1.1, got {:?})", - res.version() + rsp.version() ); } return false; } - match res.status() { + match rsp.status() { http::StatusCode::SWITCHING_PROTOCOLS => true, // CONNECT requests are complete if status code is 2xx. status if is_http_connect && status.is_success() => true, From 383a9b1b380c30c75aef1935e30dfb6b94ed051b Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 17 Mar 2025 00:00:00 +0000 Subject: [PATCH 5/7] nit(proxy/http): import `Version` Signed-off-by: katelyn martin --- linkerd/proxy/http/src/h1.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/linkerd/proxy/http/src/h1.rs b/linkerd/proxy/http/src/h1.rs index 9cf85d4734..7b4456380b 100644 --- a/linkerd/proxy/http/src/h1.rs +++ b/linkerd/proxy/http/src/h1.rs @@ -166,8 +166,10 @@ where /// Checks responses to determine if they are successful HTTP upgrades. fn is_upgrade(rsp: &http::Response, is_http_connect: bool) -> bool { + use http::Version; + // Upgrades were introduced in HTTP/1.1 - if rsp.version() != http::Version::HTTP_11 { + if rsp.version() != Version::HTTP_11 { if is_http_connect && rsp.status().is_success() { tracing::warn!( "A successful response to a CONNECT request had an incorrect HTTP version \ From 3d3a07d8d5aa42068e3131f55bbd7c3a17b60fe4 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 17 Mar 2025 00:00:00 +0000 Subject: [PATCH 6/7] refactor(proxy/http): match on http version this restates an `if version != HTTP_11 { .. }` conditional block as a match statement. this is a code motion change, none of the inner blocks are changed. Signed-off-by: katelyn martin --- linkerd/proxy/http/src/h1.rs | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/linkerd/proxy/http/src/h1.rs b/linkerd/proxy/http/src/h1.rs index 7b4456380b..740a8cf90e 100644 --- a/linkerd/proxy/http/src/h1.rs +++ b/linkerd/proxy/http/src/h1.rs @@ -168,24 +168,25 @@ where fn is_upgrade(rsp: &http::Response, is_http_connect: bool) -> bool { use http::Version; - // Upgrades were introduced in HTTP/1.1 - if rsp.version() != Version::HTTP_11 { - if is_http_connect && rsp.status().is_success() { - tracing::warn!( - "A successful response to a CONNECT request had an incorrect HTTP version \ - (expected HTTP/1.1, got {:?})", - rsp.version() - ); + match rsp.version() { + // Upgrades were introduced in HTTP/1.1 + Version::HTTP_11 => match rsp.status() { + http::StatusCode::SWITCHING_PROTOCOLS => true, + // CONNECT requests are complete if status code is 2xx. + status if is_http_connect && status.is_success() => true, + // Just a regular HTTP response... + _ => false, + }, + version => { + if is_http_connect && rsp.status().is_success() { + tracing::warn!( + "A successful response to a CONNECT request had an incorrect HTTP version \ + (expected HTTP/1.1, got {:?})", + version + ); + } + false } - return false; - } - - match rsp.status() { - http::StatusCode::SWITCHING_PROTOCOLS => true, - // CONNECT requests are complete if status code is 2xx. - status if is_http_connect && status.is_success() => true, - // Just a regular HTTP response... - _ => false, } } From a9715b2acc9b95f2038d65803878df61b7334051 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 17 Mar 2025 00:00:00 +0000 Subject: [PATCH 7/7] refactor(proxy/http): add comments on http/1.1 this commit adds a brief comment noting that upgrades are a concept specific to http/1.1. Signed-off-by: katelyn martin --- linkerd/proxy/http/src/h1.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/linkerd/proxy/http/src/h1.rs b/linkerd/proxy/http/src/h1.rs index 740a8cf90e..b91a23a24a 100644 --- a/linkerd/proxy/http/src/h1.rs +++ b/linkerd/proxy/http/src/h1.rs @@ -169,8 +169,8 @@ fn is_upgrade(rsp: &http::Response, is_http_connect: bool) -> bool { use http::Version; match rsp.version() { - // Upgrades were introduced in HTTP/1.1 Version::HTTP_11 => match rsp.status() { + // `101 Switching Protocols` indicates an upgrade. http::StatusCode::SWITCHING_PROTOCOLS => true, // CONNECT requests are complete if status code is 2xx. status if is_http_connect && status.is_success() => true, @@ -178,6 +178,9 @@ fn is_upgrade(rsp: &http::Response, is_http_connect: bool) -> bool { _ => false, }, version => { + // Upgrades are specific to HTTP/1.1. They are not included in HTTP/1.0, nor are they + // supported in HTTP/2. If this response is associated with any protocol version + // besides HTTP/1.1, it is not applicable to an upgrade. if is_http_connect && rsp.status().is_success() { tracing::warn!( "A successful response to a CONNECT request had an incorrect HTTP version \