refactor(http/upgrade): remove HttpConnect extension (#3779)

this branch is motivated by [review feedback](#3504 (comment)) from #3504. see
linkerd/linkerd2#8733 for more information on upgrading `hyper`. there,
we asked:

> I wonder if we should be a little more defensive about cloning [`HttpConnect`]. What does cloning it mean? When handling a CONNECT request, we can't clone the request, really. (Technically, we can't clone the body, but practically, it means we can't clone the request). Can we easily track whether this was accidentally cloned (i.e. with a custom Clone impl or Arc or some such) and validate at runtime (i.e., in proxy::http::h1) that everything is copacetic?

`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.

then, this branch 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.

this also 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.

reviewers are encouraged to examine this branch commit-by-commit; because
of the sensitivity of this change, this refactor is performed in small,
methodical changes.

for posterity, i've run the linkerd/linkerd2 test suite against this branch, as of
linkerd/linkerd2@57dd7f4.

---

* 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 <kate@buoyant.io>

* 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 <kate@buoyant.io>

* 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 <kate@buoyant.io>

* 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 <kate@buoyant.io>

* nit(proxy/http): import `Version`

Signed-off-by: katelyn martin <kate@buoyant.io>

* 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 <kate@buoyant.io>

* 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 <kate@buoyant.io>

---------

Signed-off-by: katelyn martin <kate@buoyant.io>

Author: cratelyn

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<Option<OnUpgrade>>,
     client: TryLock<Option<OnUpgrade>>,

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,36 +165,32 @@ where
 }
 
 /// Checks responses to determine if they are successful HTTP upgrades.
-fn is_upgrade<B>(res: &http::Response<B>) -> bool {
-    #[inline]
-    fn is_connect_success<B>(res: &http::Response<B>) -> bool {
-        res.extensions().get::<HttpConnect>().is_some() && res.status().is_success()
-    }
-
-    // Upgrades were introduced in HTTP/1.1
-    if res.version() != http::Version::HTTP_11 {
-        if is_connect_success(res) {
-            tracing::warn!(
-                "A successful response to a CONNECT request had an incorrect HTTP version \
-                (expected HTTP/1.1, got {:?})",
-                res.version()
-            );
+fn is_upgrade<B>(rsp: &http::Response<B>, is_http_connect: bool) -> bool {
+    use http::Version;
+
+    match rsp.version() {
+        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,
+            // Just a regular HTTP response...
+            _ => 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 \
+                    (expected HTTP/1.1, got {:?})",
+                    version
+                );
+            }
+            false
         }
-        return false;
     }
-
-    // 101 Switching Protocols
-    if res.status() == http::StatusCode::SWITCHING_PROTOCOLS {
-        return true;
-    }
-
-    // CONNECT requests are complete if status code is 2xx.
-    if is_connect_success(res) {
-        return true;
-    }
-
-    // Just a regular HTTP response...
-    false
 }
 
 /// Returns if the request target is in `absolute-form`.