Skip to content
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

refactor(http/retry): outline ForwardCompatibleBody<B> #3614

Merged
merged 2 commits into from
Feb 13, 2025

Conversation

cratelyn
Copy link
Collaborator

in #3559 (4b53081), we introduced a backported Frame<T> type, and a
ForwardCompatibleBody<B> type that allows us to interact with a
http_body::Body circa 0.4.6 in terms of frame-based interfaces that
match those of the 1.0 interface.

see linkerd/linkerd2#8733 for more information on upgrading hyper.

in #3559, we narrowly added this as an internal submodule of the
linkerd-http-retry library. these facilities however, would have
utility in other places such as linkerd-app-core.

this commit pulls these compatibility shims out into a
linkerd-http-body-compat library so that they can be imported and
reused elsewhere.

in #3559 (4b53081), we introduced a backported `Frame<T>` type, and a
`ForwardCompatibleBody<B>` type that allows us to interact with a
`http_body::Body` circa 0.4.6 in terms of frame-based interfaces that
match those of the 1.0 interface.

see linkerd/linkerd2#8733 for more information on upgrading hyper.

in #3559, we narrowly added this as an internal submodule of the
`linkerd-http-retry` library. these facilities however, would have
utility in other places such as `linkerd-app-core`.

this commit pulls these compatibility shims out into a
`linkerd-http-body-compat` library so that they can be imported and
reused elsewhere.

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

nb: marking this as ready for auto-merge, pending approval...

@cratelyn cratelyn merged commit faa42fd into main Feb 13, 2025
16 checks passed
@cratelyn cratelyn deleted the kate/http-retry.outline-http-body-compat branch February 13, 2025 13:47
cratelyn added a commit that referenced this pull request Feb 13, 2025
`linkerd-app-core` includes an error recovery body middleware. this middleware will gracefully catch and report errors encountered when polling an inner body, and via an `R`-typed recovery strategy provided by the caller, will attempt to map the error to a gRPC status code denoting an error.

before we upgrade to hyper 1.0 in service of linkerd/linkerd2#8733, we add some test coverage to ensure that we preserve the behavior of this middleware.

see:
* linkerd/linkerd2#8733
* #3614.

for historical context on this tower layer, see:
* #222
* #1246
* #1282

---

* refactor(http/retry): outline `ForwardCompatibleBody<B>`

in #3559 (4b53081), we introduced a backported `Frame<T>` type, and a
`ForwardCompatibleBody<B>` type that allows us to interact with a
`http_body::Body` circa 0.4.6 in terms of frame-based interfaces that
match those of the 1.0 interface.

see linkerd/linkerd2#8733 for more information on upgrading hyper.

in #3559, we narrowly added this as an internal submodule of the
`linkerd-http-retry` library. these facilities however, would have
utility in other places such as `linkerd-app-core`.

this commit pulls these compatibility shims out into a
`linkerd-http-body-compat` library so that they can be imported and
reused elsewhere.

Signed-off-by: katelyn martin <[email protected]>

* nit(http/body-compat): tidy `combinators` imports

Signed-off-by: katelyn martin <[email protected]>

* refactor(app/core): hoist `errors::code_header` helper

Signed-off-by: katelyn martin <[email protected]>

* refactor(app/core): `l5d-*` constants are headers

these are header values. `http::HeaderName` has a const fn constructor,
so let's use that.

Signed-off-by: katelyn martin <[email protected]>

* refactor(app/core): grpc constants are headers

Signed-off-by: katelyn martin <[email protected]>

* refactor(app/core): hoist `l5d-` and `grpc-` constants

Signed-off-by: katelyn martin <[email protected]>

* refactor(app/core): outline `ResponseBody` middleware

we'll add a few tests for this middleware shortly.

this commit moves this middleware out into its own submodule.

Signed-off-by: katelyn martin <[email protected]>

* refactor(app/core): encapsulate `ResponseBody` enum

for other body middleware, we hide inner enum variants and their
constituent members by using the "inner" pattern.

this commit tweaks `ResponseBody` to follow suit, such that it now holds
an `Inner`, but does not expose its passthrough and rescue variants to
callers.

Signed-off-by: katelyn martin <[email protected]>

* docs(app/core): document `ResponseBody<R, B>`

this adds a small documentation comment describing what this type does.

Signed-off-by: katelyn martin <[email protected]>

* refactor(app/core): a unit test suite for rescue body

this commit introduces a test suite for our error recovery middleware.

this body middleware provides a mechanism to "rescue" errors, gracefully
mapping an error encountered when polling a gRPC body into e.g. trailers
with a gRPC status code.

before we upgrade this middleware in service of linkerd/linkerd2#8733,
we add some test coverage to ensure that we preserve this middleware.

Signed-off-by: katelyn martin <[email protected]>

---------

Signed-off-by: katelyn martin <[email protected]>
cratelyn added a commit that referenced this pull request Feb 14, 2025
this commit makes some minor alterations to our error recovery body
middleware. see linkerd/linkerd2#8733 for more
information.

this commit removes an `assert!` statement from the implementation of
`<Response<R, B> as Body>::poll_data()`. see the documentation of
`Body::poll_frame()`:

> Once the end of the stream is reached, implementations should
> continue to return [`Poll::Ready(None)`].

hyperium/http-body@1090bff#diff-33aabe8c2aaa7614022addf244245e09bbff576a67a9ae3c6938c8a868201d36R60-R61

to do this, this commit introduces a distinct terminal state
`Inner::Rescued` to represent when the underlying `B`-typed body has
yielded an error and been rescued. once in this state the body will
yield no more data frames, instead yielding a collection of trailers
describing the mid-stream error that was encountered by the underlying
body.

the call to `R::rescue` is also moved down into the helper function fka
`grpc_trailers()`. this helps the function follow the grain of our
"state machine" a little more directly.

see #3615, #3614, and #3611 for pretext to this change.
cratelyn added a commit that referenced this pull request Feb 14, 2025
this commit makes some minor alterations to our error recovery body
middleware. see linkerd/linkerd2#8733 for more
information.

this commit removes an `assert!` statement from the implementation of
`<Response<R, B> as Body>::poll_data()`. see the documentation of
`Body::poll_frame()`:

> Once the end of the stream is reached, implementations should
> continue to return [`Poll::Ready(None)`].

hyperium/http-body@1090bff#diff-33aabe8c2aaa7614022addf244245e09bbff576a67a9ae3c6938c8a868201d36R60-R61

to do this, this commit introduces a distinct terminal state
`Inner::Rescued` to represent when the underlying `B`-typed body has
yielded an error and been rescued. once in this state the body will
yield no more data frames, instead yielding a collection of trailers
describing the mid-stream error that was encountered by the underlying
body.

the call to `R::rescue` is also moved down into the helper function fka
`grpc_trailers()`. this helps the function follow the grain of our
"state machine" a little more directly.

see #3615, #3614, and #3611 for pretext to this change.

Signed-off-by: katelyn martin <[email protected]>
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