Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 18, 2025

Conversation

cratelyn
Copy link
Collaborator

@cratelyn cratelyn commented Mar 18, 2025

this branch is motivated by review feedback 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.

`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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: katelyn martin <[email protected]>
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 <[email protected]>
this commit adds a brief comment noting that upgrades are a concept
specific to http/1.1.

Signed-off-by: katelyn martin <[email protected]>
@cratelyn cratelyn marked this pull request as ready for review March 18, 2025 22:01
@cratelyn cratelyn requested a review from a team as a code owner March 18, 2025 22:01
@cratelyn
Copy link
Collaborator Author

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

@cratelyn cratelyn merged commit 04d86a0 into main Mar 18, 2025
15 checks passed
@cratelyn cratelyn deleted the kate/hyper-1.x-remove-http-connect-extension branch March 18, 2025 22:42
@cratelyn
Copy link
Collaborator Author

for posterity, this is also related to #3540.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants