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

retry: enforce a cap on max replay buffer size #1043

Merged
merged 17 commits into from
Jun 14, 2021
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jun 10, 2021

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

hawkw added 9 commits June 9, 2021 15:16
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
this shouldn't require summing all the buffers in the buflist :)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested review from olix0r and a team June 10, 2021 18:54
linkerd/retry/src/lib.rs Outdated Show resolved Hide resolved
linkerd/http-box/src/erase_request.rs Show resolved Hide resolved
linkerd/retry/src/lib.rs Outdated Show resolved Hide resolved
@olix0r
Copy link
Member

olix0r commented Jun 14, 2021

Shared some suggestions via #1045

* wip

* inline

* Hang the EraseRequest layer off of BoxRequest
@olix0r olix0r merged commit dea0b09 into main Jun 14, 2021
@olix0r olix0r deleted the eliza/cap-replay-body branch June 14, 2021 21:07
@hawkw
Copy link
Contributor Author

hawkw commented Jun 14, 2021

yay!

olix0r added a commit to linkerd/linkerd2 that referenced this pull request Jun 18, 2021
This release updates a wide variety of the proxy's dependencies. No
user-facing changes are expected.

---

* tap: Restore tap matching tests (linkerd/linkerd2-proxy#1042)
* Add a 'rustfmt' feature to crates that use gRPC (linkerd/linkerd2-proxy#1041)
* tap: return an error when a `PortRange`'s min > max (linkerd/linkerd2-proxy#1044)
* retry: Enforce a cap on max replay buffer size (linkerd/linkerd2-proxy#1043)
* Add codecov integration via cargo-tarpaulin (linkerd/linkerd2-proxy#1047)
* Add a dependabot configuration (linkerd/linkerd2-proxy#1046)
* Fixup dependabot configuration (linkerd/linkerd2-proxy#1049)
* build(deps): bump actions/upload-artifact from 2.2.3 to 2.2.4 (linkerd/linkerd2-proxy#1050)
* build(deps): bump softprops/action-gh-release from 0.1.5 to 1 (linkerd/linkerd2-proxy#1051)
* build(deps): bump libfuzzer-sys in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1052)
* build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 in /linkerd/tls/fuzz (linkerd/linkerd2-proxy#1053)
* build(deps): bump libfuzzer-sys in /linkerd/addr/fuzz (linkerd/linkerd2-proxy#1054)
* build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/proxy/http/fuzz (linkerd/linkerd2-proxy#1056)
* build(deps): bump tracing from 0.1.25 to 0.1.26 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1055)
* build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1058)
* build(deps): bump tracing from 0.1.25 to 0.1.26 in /linkerd/addr/fuzz (linkerd/linkerd2-proxy#1059)
* build(deps): bump libfuzzer-sys in /linkerd/proxy/http/fuzz (linkerd/linkerd2-proxy#1060)
* build(deps): bump tracing in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1061)
* build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1062)
* build(deps): bump tokio in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1063)
* build(deps): bump arbitrary in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1064)
* build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1065)
* build(deps): bump tracing in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1066)
* build(deps): bump hyper in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1067)
* build(deps): bump deflate from 0.7.20 to 0.9.1 (linkerd/linkerd2-proxy#1068)
* build(deps): bump libfuzzer-sys in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1069)
* build(deps): bump tonic-build from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1070)
* build(deps): bump linkerd2-proxy-api from `453ac1e` to `48f13d6` (linkerd/linkerd2-proxy#1071)
* build(deps): bump ipnet from 2.3.0 to 2.3.1 (linkerd/linkerd2-proxy#1072)
* build(deps): bump regex from 1.4.3 to 1.4.6 (linkerd/linkerd2-proxy#1073)
* build(deps): bump arbitrary in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1057)
* Update tonic to v0.4.3 (linkerd/linkerd2-proxy#1080)
* ci: Skip coverage tests on dependencies-only changes (linkerd/linkerd2-proxy#1074)
* build(deps): bump indexmap from 1.6.1 to 1.6.2 (linkerd/linkerd2-proxy#1075)
* build(deps): bump rand from 0.8.3 to 0.8.4 (linkerd/linkerd2-proxy#1077)
* build(deps): bump tokio-test from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1076)
* build(deps): bump pin-project from 1.0.5 to 1.0.7 (linkerd/linkerd2-proxy#1078)
* build(deps): bump arbitrary from 1.0.0 to 1.0.1 (linkerd/linkerd2-proxy#1083)
* outbound: replace `sleep`s in test with `yield_now` (linkerd/linkerd2-proxy#1085)
* build(deps): bump thiserror from 1.0.23 to 1.0.25 (linkerd/linkerd2-proxy#1081)
* build(deps): bump async-stream from 0.3.0 to 0.3.2 (linkerd/linkerd2-proxy#1084)
* build(deps): bump serde_json from 1.0.62 to 1.0.64 (linkerd/linkerd2-proxy#1086)
* build(deps): bump tokio-stream from 0.1.5 to 0.1.6 (linkerd/linkerd2-proxy#1082)
* build(deps): Skip coverage by PR title (linkerd/linkerd2-proxy#1087)
* fuzz: Omit lockfiles from version control (linkerd/linkerd2-proxy#1088)
* build(deps): bump tokio from 1.6.1 to 1.7.0 (linkerd/linkerd2-proxy#1079)
* build(deps): bump mimalloc from 0.1.24 to 0.1.25 (linkerd/linkerd2-proxy#1089)
* build(deps): bump tokio-util from 0.6.5 to 0.6.7 (linkerd/linkerd2-proxy#1090)
* build(deps): bump libc from 0.2.86 to 0.2.97 (linkerd/linkerd2-proxy#1093)
* build(deps): bump http from 0.2.3 to 0.2.4 (linkerd/linkerd2-proxy#1092)
* build(deps): bump rustls from 0.19.0 to 0.19.1 (linkerd/linkerd2-proxy#1094)
* build(deps): bump async-trait from 0.1.42 to 0.1.50 (linkerd/linkerd2-proxy#1095)
* build(deps): bump tracing-subscriber from 0.2.17 to 0.2.18 (linkerd/linkerd2-proxy#1096)
* build(deps): bump http-body from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1097)
* build(deps): bump tracing from 0.1.25 to 0.1.26 (linkerd/linkerd2-proxy#1098)
* build(deps): bump hdrhistogram from 7.2.0 to 7.3.0 (linkerd/linkerd2-proxy#1099)
* build(deps): bump regex from 1.4.6 to 1.5.4 (linkerd/linkerd2-proxy#1100)
* build(deps): bump tower from 0.4.7 to 0.4.8 (linkerd/linkerd2-proxy#1102)
* build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1103)
* build(deps): bump actions/download-artifact from 2.0.9 to 2.0.10 (linkerd/linkerd2-proxy#1104)
* Remove unneeded uses of indexmap (linkerd/linkerd2-proxy#1048)
* build(deps): Update trust-dns to v0.21.0-alpha.1 (linkerd/linkerd2-proxy#1105)
* config: add a `PortSet` type (linkerd/linkerd2-proxy#1106)
* replace `linkerd-concurrency-limit` with Tower (linkerd/linkerd2-proxy#1107)
* build(deps): Update Rust to v1.53.0 (linkerd/linkerd2-proxy#1108)
adleong pushed a commit to linkerd/linkerd2 that referenced this pull request Jun 18, 2021
This release updates a wide variety of the proxy's dependencies. No
user-facing changes are expected.

---

* tap: Restore tap matching tests (linkerd/linkerd2-proxy#1042)
* Add a 'rustfmt' feature to crates that use gRPC (linkerd/linkerd2-proxy#1041)
* tap: return an error when a `PortRange`'s min > max (linkerd/linkerd2-proxy#1044)
* retry: Enforce a cap on max replay buffer size (linkerd/linkerd2-proxy#1043)
* Add codecov integration via cargo-tarpaulin (linkerd/linkerd2-proxy#1047)
* Add a dependabot configuration (linkerd/linkerd2-proxy#1046)
* Fixup dependabot configuration (linkerd/linkerd2-proxy#1049)
* build(deps): bump actions/upload-artifact from 2.2.3 to 2.2.4 (linkerd/linkerd2-proxy#1050)
* build(deps): bump softprops/action-gh-release from 0.1.5 to 1 (linkerd/linkerd2-proxy#1051)
* build(deps): bump libfuzzer-sys in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1052)
* build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 in /linkerd/tls/fuzz (linkerd/linkerd2-proxy#1053)
* build(deps): bump libfuzzer-sys in /linkerd/addr/fuzz (linkerd/linkerd2-proxy#1054)
* build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/proxy/http/fuzz (linkerd/linkerd2-proxy#1056)
* build(deps): bump tracing from 0.1.25 to 0.1.26 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1055)
* build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1058)
* build(deps): bump tracing from 0.1.25 to 0.1.26 in /linkerd/addr/fuzz (linkerd/linkerd2-proxy#1059)
* build(deps): bump libfuzzer-sys in /linkerd/proxy/http/fuzz (linkerd/linkerd2-proxy#1060)
* build(deps): bump tracing in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1061)
* build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1062)
* build(deps): bump tokio in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1063)
* build(deps): bump arbitrary in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1064)
* build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1065)
* build(deps): bump tracing in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1066)
* build(deps): bump hyper in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1067)
* build(deps): bump deflate from 0.7.20 to 0.9.1 (linkerd/linkerd2-proxy#1068)
* build(deps): bump libfuzzer-sys in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1069)
* build(deps): bump tonic-build from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1070)
* build(deps): bump linkerd2-proxy-api from `453ac1e` to `48f13d6` (linkerd/linkerd2-proxy#1071)
* build(deps): bump ipnet from 2.3.0 to 2.3.1 (linkerd/linkerd2-proxy#1072)
* build(deps): bump regex from 1.4.3 to 1.4.6 (linkerd/linkerd2-proxy#1073)
* build(deps): bump arbitrary in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1057)
* Update tonic to v0.4.3 (linkerd/linkerd2-proxy#1080)
* ci: Skip coverage tests on dependencies-only changes (linkerd/linkerd2-proxy#1074)
* build(deps): bump indexmap from 1.6.1 to 1.6.2 (linkerd/linkerd2-proxy#1075)
* build(deps): bump rand from 0.8.3 to 0.8.4 (linkerd/linkerd2-proxy#1077)
* build(deps): bump tokio-test from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1076)
* build(deps): bump pin-project from 1.0.5 to 1.0.7 (linkerd/linkerd2-proxy#1078)
* build(deps): bump arbitrary from 1.0.0 to 1.0.1 (linkerd/linkerd2-proxy#1083)
* outbound: replace `sleep`s in test with `yield_now` (linkerd/linkerd2-proxy#1085)
* build(deps): bump thiserror from 1.0.23 to 1.0.25 (linkerd/linkerd2-proxy#1081)
* build(deps): bump async-stream from 0.3.0 to 0.3.2 (linkerd/linkerd2-proxy#1084)
* build(deps): bump serde_json from 1.0.62 to 1.0.64 (linkerd/linkerd2-proxy#1086)
* build(deps): bump tokio-stream from 0.1.5 to 0.1.6 (linkerd/linkerd2-proxy#1082)
* build(deps): Skip coverage by PR title (linkerd/linkerd2-proxy#1087)
* fuzz: Omit lockfiles from version control (linkerd/linkerd2-proxy#1088)
* build(deps): bump tokio from 1.6.1 to 1.7.0 (linkerd/linkerd2-proxy#1079)
* build(deps): bump mimalloc from 0.1.24 to 0.1.25 (linkerd/linkerd2-proxy#1089)
* build(deps): bump tokio-util from 0.6.5 to 0.6.7 (linkerd/linkerd2-proxy#1090)
* build(deps): bump libc from 0.2.86 to 0.2.97 (linkerd/linkerd2-proxy#1093)
* build(deps): bump http from 0.2.3 to 0.2.4 (linkerd/linkerd2-proxy#1092)
* build(deps): bump rustls from 0.19.0 to 0.19.1 (linkerd/linkerd2-proxy#1094)
* build(deps): bump async-trait from 0.1.42 to 0.1.50 (linkerd/linkerd2-proxy#1095)
* build(deps): bump tracing-subscriber from 0.2.17 to 0.2.18 (linkerd/linkerd2-proxy#1096)
* build(deps): bump http-body from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1097)
* build(deps): bump tracing from 0.1.25 to 0.1.26 (linkerd/linkerd2-proxy#1098)
* build(deps): bump hdrhistogram from 7.2.0 to 7.3.0 (linkerd/linkerd2-proxy#1099)
* build(deps): bump regex from 1.4.6 to 1.5.4 (linkerd/linkerd2-proxy#1100)
* build(deps): bump tower from 0.4.7 to 0.4.8 (linkerd/linkerd2-proxy#1102)
* build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1103)
* build(deps): bump actions/download-artifact from 2.0.9 to 2.0.10 (linkerd/linkerd2-proxy#1104)
* Remove unneeded uses of indexmap (linkerd/linkerd2-proxy#1048)
* build(deps): Update trust-dns to v0.21.0-alpha.1 (linkerd/linkerd2-proxy#1105)
* config: add a `PortSet` type (linkerd/linkerd2-proxy#1106)
* replace `linkerd-concurrency-limit` with Tower (linkerd/linkerd2-proxy#1107)
* build(deps): Update Rust to v1.53.0 (linkerd/linkerd2-proxy#1108)
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