replace `RequestFilter` with Tower's upstream impl #842

hawkw · 2021-01-13T19:44:37Z

Tower provides a tower::filter utility for conditionally rejecting
some requests and accepting others. Rather than using tower's filter,
we have a similar linkerd_stack::request_filter, due to two main
issues with the filtering API in tower 0.3 and earlier:

tower::filter's predicates returned futures, which we don't need
--- all our filtering predicates are purely sunchronous
tower::filter didn't allow filters to return any error of their
choosing --- they had to return a specific filter-rejected error.
This doesn't work with other code where we downcast error types,
like switch/fallback.

However, Tower 0.4 includes a rewritten Filter API that no longer
has these issues (tower-rs/tower#508). Therefore, we can switch to
using the upstream implementation. This branch replaces RequestFilter
with tower::filter::Filter.

It was necessary to add an impl of NewService for Filter. This
required an upstream change to expose access to the wrapped service,
so that its new_service method can be called when it's a
NewService, and to expose access to the Predicate. Therefore, this
PR depends on tower-rs/tower#521 and tower-rs/tower#522, and introduces
a Git dependency. The git dep can be removed when Tower 0.4.3 is
released.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2021-01-13T21:34:32Z

Tower 0.4.3 has been published upstream so I'm removing the draft tag.

olix0r · 2021-01-14T01:10:00Z

Cargo.toml

@@ -66,4 +66,4 @@ debug = false
 [patch.crates-io]
 webpki = { git = "https://github.com/linkerd/webpki", branch = "cert-dns-names-0.21", rev = "b2c3bb3" }
 tonic = { git = "https://github.com/hyperium/tonic" }
-tonic-build = { git = "https://github.com/hyperium/tonic" }
+tonic-build = { git = "https://github.com/hyperium/tonic" }


Why this change?

linkerd/stack/Cargo.toml

Co-authored-by: Oliver Gould <ver@buoyant.io>

This release improves diagnostics about the proxy's failfast state: * Warnings are now emitted when the failfast state is entered; * The "max concurrency exhausted" gRPC message has been changed to more-clearly indicate a failfast state error; and * Failfast recovery has been made more robust, ensuring that a service can recover indepenently of new requests being received. Furthermore, metric labeling has been improved: * TCP server metrics are now annotated with the original `target_addr`; * The `tls` label is now set to true for inbound TLS connections that lack a client ID. This is mostly helpful to clarify inbound metrics on the `identity` controller; * Outbound `tls` metrics could be reported incorrectly when a proxy was configured to not use identity. This has been corrected. Finally, socket-level errors now include a _client_ or _server_ prefix to indicate which side of the proxy encountered the error. --- * stack: remove `map_response` (linkerd/linkerd2-proxy#835) * replace `RequestFilter` with Tower's upstream impl (linkerd/linkerd2-proxy#842) * tracing: fix incorrect field format when logging in JSON (linkerd/linkerd2-proxy#845) * replace `FutureService` with Tower's upstream impl (linkerd/linkerd2-proxy#839) * integration: improve tracing in tests (linkerd/linkerd2-proxy#846) * service-profiles: Prevent Duration coercion panics (linkerd/linkerd2-proxy#844) * inbound: Separate HTTP server logic from protocol detection (linkerd/linkerd2-proxy#843) * Correct gRPC 'max-concurrency exhausted' error messages (linkerd/linkerd2-proxy#847) * Update tonic to v0.4 (linkerd/linkerd2-proxy#849) * failfast: Improve diagnostic logging (linkerd/linkerd2-proxy#848) * Update the base docker image (linkerd/linkerd2-proxy#850) * stack: Implement Clone for ResultService (linkerd/linkerd2-proxy#851) * Ensure services in failfast can become ready (linkerd/linkerd2-proxy#858) * tests: replace string matching on metrics with parsing (linkerd/linkerd2-proxy#859) * Decouple tls::accept from TcpStream (linkerd/linkerd2-proxy#853) * metrics: Handle NoPeerIdFromRemote properly (linkerd/linkerd2-proxy#857) * metrics: Reorder metrics labels (linkerd/linkerd2-proxy#856) * Rename tls::accept to tls::server (linkerd/linkerd2-proxy#854) * Annotate socket-level errors with a scope (linkerd/linkerd2-proxy#852) * test: reduce repetition in metrics tests (linkerd/linkerd2-proxy#860) * tls: Disambiguate client and server identities (linkerd/linkerd2-proxy#855) * Update to tower v0.4.4 (linkerd/linkerd2-proxy#864) * Update cargo dependencies (linkerd/linkerd2-proxy#865) * metrics: add `target_addr` label for accepted transport metrics (linkerd/linkerd2-proxy#861) * outbound: Strip endpoint identity when disabled (linkerd/linkerd2-proxy#862) --- The opaque-ports test has been updated to reflect proxy metrics changes.

This release improves diagnostics about the proxy's failfast state: * Warnings are now emitted when the failfast state is entered; * The "max concurrency exhausted" gRPC message has been changed to more-clearly indicate a failfast state error; and * Failfast recovery has been made more robust, ensuring that a service can recover indepenently of new requests being received. Furthermore, metric labeling has been improved: * TCP server metrics are now annotated with the original `target_addr`; * The `tls` label is now set to true for inbound TLS connections that lack a client ID. This is mostly helpful to clarify inbound metrics on the `identity` controller; * Outbound `tls` metrics could be reported incorrectly when a proxy was configured to not use identity. This has been corrected. Finally, socket-level errors now include a _client_ or _server_ prefix to indicate which side of the proxy encountered the error. --- * stack: remove `map_response` (linkerd/linkerd2-proxy#835) * replace `RequestFilter` with Tower's upstream impl (linkerd/linkerd2-proxy#842) * tracing: fix incorrect field format when logging in JSON (linkerd/linkerd2-proxy#845) * replace `FutureService` with Tower's upstream impl (linkerd/linkerd2-proxy#839) * integration: improve tracing in tests (linkerd/linkerd2-proxy#846) * service-profiles: Prevent Duration coercion panics (linkerd/linkerd2-proxy#844) * inbound: Separate HTTP server logic from protocol detection (linkerd/linkerd2-proxy#843) * Correct gRPC 'max-concurrency exhausted' error messages (linkerd/linkerd2-proxy#847) * Update tonic to v0.4 (linkerd/linkerd2-proxy#849) * failfast: Improve diagnostic logging (linkerd/linkerd2-proxy#848) * Update the base docker image (linkerd/linkerd2-proxy#850) * stack: Implement Clone for ResultService (linkerd/linkerd2-proxy#851) * Ensure services in failfast can become ready (linkerd/linkerd2-proxy#858) * tests: replace string matching on metrics with parsing (linkerd/linkerd2-proxy#859) * Decouple tls::accept from TcpStream (linkerd/linkerd2-proxy#853) * metrics: Handle NoPeerIdFromRemote properly (linkerd/linkerd2-proxy#857) * metrics: Reorder metrics labels (linkerd/linkerd2-proxy#856) * Rename tls::accept to tls::server (linkerd/linkerd2-proxy#854) * Annotate socket-level errors with a scope (linkerd/linkerd2-proxy#852) * test: reduce repetition in metrics tests (linkerd/linkerd2-proxy#860) * tls: Disambiguate client and server identities (linkerd/linkerd2-proxy#855) * Update to tower v0.4.4 (linkerd/linkerd2-proxy#864) * Update cargo dependencies (linkerd/linkerd2-proxy#865) * metrics: add `target_addr` label for accepted transport metrics (linkerd/linkerd2-proxy#861) * outbound: Strip endpoint identity when disabled (linkerd/linkerd2-proxy#862) --- The opaque-ports test has been updated to reflect proxy metrics changes. Signed-off-by: Jijeesh <jijeesh.ka@gmail.com>

hawkw added 4 commits January 11, 2021 13:40

wip

8357007

well this *almost* works...

3d52c97

Merge branch 'main' into eliza/upstream-tower-filter

8fb4a64

lol Filter::new` takes args in the opposite order

eb25122

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw requested a review from olix0r January 13, 2021 19:44

hawkw self-assigned this Jan 13, 2021

hawkw added 3 commits January 13, 2021 11:51

Merge branch 'main' into eliza/upstream-tower-filter

30a15ee

de-break lockfile

385df21

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

remove patch

8c68277

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw marked this pull request as ready for review January 13, 2021 21:34

hawkw requested a review from a team January 13, 2021 21:34

olix0r reviewed Jan 14, 2021

View reviewed changes

olix0r approved these changes Jan 14, 2021

View reviewed changes

linkerd/stack/Cargo.toml Outdated Show resolved Hide resolved

hawkw and others added 2 commits January 13, 2021 17:17

Update linkerd/stack/Cargo.toml

bead12f

Co-authored-by: Oliver Gould <ver@buoyant.io>

put back whitespace

634d809

olix0r merged commit bfe233b into main Jan 14, 2021

olix0r deleted the eliza/upstream-tower-filter branch January 14, 2021 16:12

olix0r mentioned this pull request Jan 21, 2021

proxy: v2.129.0 linkerd/linkerd2#5581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace `RequestFilter` with Tower's upstream impl #842

replace `RequestFilter` with Tower's upstream impl #842

hawkw commented Jan 13, 2021

hawkw commented Jan 13, 2021

olix0r Jan 14, 2021 •

edited

Loading

replace RequestFilter with Tower's upstream impl #842

replace RequestFilter with Tower's upstream impl #842

Conversation

hawkw commented Jan 13, 2021

hawkw commented Jan 13, 2021

olix0r Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

replace `RequestFilter` with Tower's upstream impl #842

replace `RequestFilter` with Tower's upstream impl #842

olix0r Jan 14, 2021 •

edited

Loading