From c7a6ab6134bf5eaf16b2d207f6e6b17efd88ca38 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 13 Jan 2025 00:00:00 +0000 Subject: [PATCH 1/7] docs(http/upgrade): add crate-level docs, rfc link Signed-off-by: katelyn martin --- linkerd/http/upgrade/src/lib.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/linkerd/http/upgrade/src/lib.rs b/linkerd/http/upgrade/src/lib.rs index 851d176294..cf488dca2e 100644 --- a/linkerd/http/upgrade/src/lib.rs +++ b/linkerd/http/upgrade/src/lib.rs @@ -1,4 +1,25 @@ -//! Facilities for HTTP/1 upgrades. +//! Facilities for HTTP/1.1 upgrades. +//! +//! HTTP/1.1 specifies an `Upgrade` header field that may be used in tandem with the `Connection` +//! header field as a simple mechanism to transition from HTTP/1.1 to another protocol. This crate +//! provides [`tower`] middleware that enable upgrades to HTTP/2 for services running within a +//! [`tokio`] runtime. +//! +//! Use [`Service::new()`] to add upgrade support to a [`tower::Service`]. +//! +//! [RFC 9110 § 7.6.1][rfc9110-connection] for more information about the `Connection` header +//! field, [RFC 9110 § 7.8][rfc9110-upgrade] for more information about HTTP/1.1's `Upgrade` +//! header field, and [RFC 9110 § 15.2.2][rfc9110-101] for more information about the +//! `101 (Switching Protocols)` response status code. +//! +//! Note that HTTP/2 does *NOT* provide support for the `Upgrade` header field, per +//! [RFC 9113 § 8.6][rfc9113]. HTTP/2 is a multiplexed protocol, and connection upgrades are +//! thus inapplicable. +//! +//! [rfc9110-connection]: https://www.rfc-editor.org/rfc/rfc9110#name-connection +//! [rfc9110-upgrade]: https://www.rfc-editor.org/rfc/rfc9110#field.upgrade +//! [rfc9110-101]: https://www.rfc-editor.org/rfc/rfc9110#name-101-switching-protocols +//! [rfc9113]: https://www.rfc-editor.org/rfc/rfc9113.html#name-the-upgrade-header-field pub use self::upgrade::Service; From 4aa1ffe0646490d4d8fe218dde1491ecc9fdd308 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 13 Jan 2025 00:00:00 +0000 Subject: [PATCH 2/7] docs(http/upgrade): update link to obsolete rfc rfc 9110 obsoletes the following rfc's: 2818, 7230, 7231, 7232, 7233, 7235, 7538, 7615, and 7694. this updates a comment related to connection upgrade logic, linking to the current rfc, 9110. this information now lives in section 9.3.6, paragraph 12. Signed-off-by: katelyn martin --- linkerd/proxy/http/src/h1.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/linkerd/proxy/http/src/h1.rs b/linkerd/proxy/http/src/h1.rs index 125720434f..becb3730eb 100644 --- a/linkerd/proxy/http/src/h1.rs +++ b/linkerd/proxy/http/src/h1.rs @@ -138,17 +138,20 @@ where Box::pin(rsp_fut.err_into().map_ok(move |mut rsp| { 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 - // https://tools.ietf.org/html/rfc7231#section-4.3.6: + // 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 + // > fields in a 2xx (Successful) response to `CONNECT`. A client MUST ignore any + // > `Content-Length` or `Transfer-Encoding` header fields received in a successful + // > response to `CONNECT`. // - // A client MUST ignore any Content-Length or Transfer-Encoding - // header fields received in a successful response to CONNECT. + // see: https://www.rfc-editor.org/rfc/rfc9110#section-9.3.6-12 if rsp.status().is_success() { rsp.headers_mut().remove(CONTENT_LENGTH); rsp.headers_mut().remove(TRANSFER_ENCODING); From 52e3d4158dfe57787839faf0131ae5762db8c34e Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 13 Jan 2025 00:00:00 +0000 Subject: [PATCH 3/7] nit(http/upgrade): update incorrect comment this function has since been renamed `halves()`. Signed-off-by: katelyn martin --- linkerd/http/upgrade/src/upgrade.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linkerd/http/upgrade/src/upgrade.rs b/linkerd/http/upgrade/src/upgrade.rs index 215e7ef006..dddd8003a4 100644 --- a/linkerd/http/upgrade/src/upgrade.rs +++ b/linkerd/http/upgrade/src/upgrade.rs @@ -29,7 +29,7 @@ pub struct Http11Upgrade { inner: Arc, } -/// A named "tuple" returned by `Http11Upgade::new()` of the two halves of +/// A named "tuple" returned by [`Http11Upgade::halves()`] of the two halves of /// an upgrade. #[derive(Debug)] struct Http11UpgradeHalves { From be9031724b7db0576706e086056a97a35055992b Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 13 Jan 2025 00:00:00 +0000 Subject: [PATCH 4/7] docs(http/upgrade): add comments to `wants_upgrade` this adds a comment additionally clarifying that HTTP/2 does not support upgrades. Signed-off-by: katelyn martin --- linkerd/http/upgrade/src/upgrade.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/linkerd/http/upgrade/src/upgrade.rs b/linkerd/http/upgrade/src/upgrade.rs index dddd8003a4..315a56717f 100644 --- a/linkerd/http/upgrade/src/upgrade.rs +++ b/linkerd/http/upgrade/src/upgrade.rs @@ -257,9 +257,11 @@ fn is_origin_form(uri: &http::uri::Uri) -> bool { uri.scheme().is_none() && uri.path_and_query().is_none() } -/// Checks requests to determine if they want to perform an HTTP upgrade. +/// Returns true if the given [Request][http::Request] is attempting an HTTP/1.1 upgrade. fn wants_upgrade(req: &http::Request) -> bool { - // HTTP upgrades were added in 1.1, not 1.0. + // Upgrades are specific to HTTP/1.1. They are not included in HTTP/1.0, nor are they supported + // in HTTP/2. If this request is associated with any protocol version besides HTTP/1.1, we can + // dismiss it immediately as not being applicable to an upgrade. if req.version() != http::Version::HTTP_11 { return false; } From 1e0401b0c958f396c73b8378bffcef29b9d0be68 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 13 Jan 2025 00:00:00 +0000 Subject: [PATCH 5/7] docs(http/upgrade): document `strip_connection_headers()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit this function performs some important behavior that we MUST implement, as a proxy/intermediary. to help elucidate the mandated behavior expected of us by the HTTP/1 specification, add documentation comments noting the related passages from rfc 9110 § 7.6.1. Signed-off-by: katelyn martin --- linkerd/http/upgrade/src/lib.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/linkerd/http/upgrade/src/lib.rs b/linkerd/http/upgrade/src/lib.rs index cf488dca2e..ae09d3d6fe 100644 --- a/linkerd/http/upgrade/src/lib.rs +++ b/linkerd/http/upgrade/src/lib.rs @@ -26,6 +26,31 @@ pub use self::upgrade::Service; pub mod glue; pub mod upgrade; +/// Removes connection headers from the given [`HeaderMap`][http::HeaderMap]. +/// +/// An HTTP proxy is required to do this, according to [RFC 9110 § 7.6.1 ¶ 5][rfc9110-761-5]: +/// +/// > Intermediaries MUST parse a received Connection header field before a message is forwarded +/// > and, for each connection-option in this field, remove any header or trailer field(s) from the +/// > message with the same name as the connection-option, and then remove the Connection header +/// > field itself (or replace it with the intermediary's own control options for the forwarded +/// > message). +/// +/// This function additionally removes some headers mentioned in +/// [RFC 9110 § 7.6.1 ¶ 7-8.5][rfc9110-761-7] +/// +/// > Furthermore, intermediaries SHOULD remove or replace fields that are known to require removal +/// > before forwarding, whether or not they appear as a connection-option, after applying those +/// > fields' semantics. This includes but is not limited to: +/// > +/// > - `Proxy-Connection` (Appendix C.2.2 of [HTTP/1.1]) +/// > - `Keep-Alive` (Section 19.7.1 of [RFC2068]) +/// > - `TE` (Section 10.1.4) +/// > - `Transfer-Encoding` (Section 6.1 of [HTTP/1.1]) +/// > - `Upgrade` (Section 7.8) +/// +/// [rfc9110-761-5]: https://www.rfc-editor.org/rfc/rfc9110#section-7.6.1-5 +/// [rfc9110-761-7]: https://www.rfc-editor.org/rfc/rfc9110#section-7.6.1-7 pub fn strip_connection_headers(headers: &mut http::HeaderMap) { use http::header; if let Some(val) = headers.remove(header::CONNECTION) { From 993e48ce81d966a1cee5feadc231ef273d743fa9 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 13 Jan 2025 00:00:00 +0000 Subject: [PATCH 6/7] nit(http/upgrade): fix typo in `Http11Upgrade` comment Signed-off-by: katelyn martin --- linkerd/http/upgrade/src/upgrade.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linkerd/http/upgrade/src/upgrade.rs b/linkerd/http/upgrade/src/upgrade.rs index 315a56717f..39a498336e 100644 --- a/linkerd/http/upgrade/src/upgrade.rs +++ b/linkerd/http/upgrade/src/upgrade.rs @@ -22,7 +22,7 @@ use try_lock::TryLock; /// inserted into the `Request::extensions()`. If the HTTP1 client service /// also detects an upgrade, the two `OnUpgrade` futures will be joined /// together with the glue in this type. -// Note: this relies on their only having been 2 Inner clones, so don't +// Note: this relies on there only having been 2 Inner clones, so don't // implement `Clone` for this type. pub struct Http11Upgrade { half: Half, From 317807a15913be7f6b9e2c6901511868776ba056 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 13 Jan 2025 00:00:00 +0000 Subject: [PATCH 7/7] docs(http/upgrade): update incorrect comment this comment is not true. this commit updates it, reflecting the current state of the upgrade body's `Drop` logic. Signed-off-by: katelyn martin --- linkerd/http/upgrade/src/glue.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/linkerd/http/upgrade/src/glue.rs b/linkerd/http/upgrade/src/glue.rs index cfc852ba9e..459099e835 100644 --- a/linkerd/http/upgrade/src/glue.rs +++ b/linkerd/http/upgrade/src/glue.rs @@ -18,8 +18,7 @@ use tracing::debug; #[pin_project(PinnedDrop)] #[derive(Debug)] pub struct UpgradeBody { - /// In UpgradeBody::drop, if this was an HTTP upgrade, the body is taken - /// to be inserted into the Http11Upgrade half. + /// The inner [`Body`] being wrapped. #[pin] body: B, upgrade: Option<(Http11Upgrade, hyper::upgrade::OnUpgrade)>,