-
Notifications
You must be signed in to change notification settings - Fork 271
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
retry: Enforce a cap on max replay buffer size (#1043)
This branch adds a limit to the maximum number of bytes a `ReplayBody` will buffer as a defense-in-depth against unbounded buffering. Currently, the retry policy should ensure that requests whose `content-length` header is over the limit will not be wrapped with `ReplayBody`. However, if there's a bug in the retry policy, or if a request claims to have an acceptable `content-length` but keeps sending data past that length, we might buffer a potentially unbounded amount of data. This limit is intended to _ensure_ we don't buffer data over the limit despite the `content-length`-based retry policy. The limit is checked whenever new data is added to the buffer, and if it is reached, the buffer is discarded and the `ReplayBody` will cease buffering additional data. However, the body will be allowed to continue streaming so that the _initial_ request may still complete --- but attempting to retry it will fail, because the buffer was discarded. This way, in the event of a request that somehow passes the limit, we won't fail that request immediately, but instead will fall back to not retrying it. In order to make this change nicely, the `ReplayBody` has to be constructed by the retry policy, rather than by the `Retry` service. This is because the policy knows what the size limits for retries is. To allow this, I did some refactoring. In particular, I introduced a new trait for retry policies that's an extension of `tower::retry::Policy`, but with the addition of a method that's called to modify the request for a retry. This method returns an `Either`, with `Either::A` indicating that the request should be retried, and `Either::B` indicating that it should not; and in the retry case, the policy may change the request's type. This allows the retry policy to be responsible for wrapping the request in a `ReplayBody`, rather than doing that in the `Retry` service. A nice side-effect of this change is that the `linkerd-retry` crate no longer needs to be HTTP-specific. All the HTTP-specific code now lives in `linkerd_app_core::retry`, with the exception of the `ReplayBody` type, which I put in its own crate (mainly just to allow running its tests without having to compiel all of `app-core`). Co-authored-by: Oliver Gould <ver@buoyant.io>
- Loading branch information
Showing
16 changed files
with
1,252 additions
and
977 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| //! A middleware that boxes HTTP request bodies. | ||
| use crate::BoxBody; | ||
| use linkerd_error::Error; | ||
| use linkerd_stack::{layer, Proxy}; | ||
| use std::task::{Context, Poll}; | ||
|
|
||
| /// Boxes request bodies, erasing the original type. | ||
| /// | ||
| /// This is *very* similar to the [`BoxRequest`] middleware. However, that | ||
| /// middleware is generic over a specific body type that is erased. A given | ||
| /// instance of `EraseRequest` can only erase the type of one particular `Body` | ||
| /// type, while this middleware will erase bodies of *any* type. | ||
| /// | ||
| /// An astute reader may ask, why not simply replace `BoxRequest` with this | ||
| /// middleware, if it is a more flexible superset of the same behavior? The | ||
| /// answer is that in many cases, the use of this more flexible middleware | ||
| /// renders request body types uninferrable. If all `BoxRequest`s in the stack | ||
| /// are replaced with `EraseRequest`, suddenly a great deal of | ||
| /// `check_new_service` and `check_service` checks will require explicit | ||
| /// annotations for the pre-erasure body type. This is not great. | ||
| /// | ||
| /// Instead, this type is implemented separately and should be used only when a | ||
| /// stack must be able to implement `Service<http::Request<B>>` for *multiple | ||
| /// distinct values of `B`*. | ||
| #[derive(Debug)] | ||
| pub struct EraseRequest<S>(S); | ||
|
|
||
| impl<S> EraseRequest<S> { | ||
| pub fn new(inner: S) -> Self { | ||
| Self(inner) | ||
| } | ||
|
|
||
| pub fn layer() -> impl layer::Layer<S, Service = Self> + Clone + Copy { | ||
| layer::mk(Self::new) | ||
| } | ||
| } | ||
|
|
||
| impl<S: Clone> Clone for EraseRequest<S> { | ||
| fn clone(&self) -> Self { | ||
| EraseRequest(self.0.clone()) | ||
| } | ||
| } | ||
|
|
||
| impl<S, B> tower::Service<http::Request<B>> for EraseRequest<S> | ||
| where | ||
| B: http_body::Body + Send + 'static, | ||
| B::Data: Send + 'static, | ||
| B::Error: Into<Error>, | ||
| S: tower::Service<http::Request<BoxBody>>, | ||
| { | ||
| type Response = S::Response; | ||
| type Error = S::Error; | ||
| type Future = S::Future; | ||
|
|
||
| #[inline] | ||
| fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
| self.0.poll_ready(cx) | ||
| } | ||
|
|
||
| #[inline] | ||
| fn call(&mut self, req: http::Request<B>) -> Self::Future { | ||
| self.0.call(req.map(BoxBody::new)) | ||
| } | ||
| } | ||
|
|
||
| impl<S, B, P> Proxy<http::Request<B>, S> for EraseRequest<P> | ||
| where | ||
| B: http_body::Body + Send + 'static, | ||
| B::Data: Send + 'static, | ||
| B::Error: Into<Error>, | ||
| S: tower::Service<P::Request>, | ||
| P: Proxy<http::Request<BoxBody>, S>, | ||
| { | ||
| type Request = P::Request; | ||
| type Response = P::Response; | ||
| type Error = P::Error; | ||
| type Future = P::Future; | ||
|
|
||
| #[inline] | ||
| fn proxy(&self, inner: &mut S, req: http::Request<B>) -> Self::Future { | ||
| self.0.proxy(inner, req.map(BoxBody::new)) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.