rewrite `tower::filter` by hawkw · Pull Request #508 · tower-rs/tower

hawkw · 2021-01-06T01:27:58Z

Motivation

It was pointed out that there is currently some overlap between the
try_with Service combinator and tower::filter middleware (see #499 (comment) ).
try_with synchronously maps from a Request ->
Result<DifferentRequest, Error>, while tower::filter
asynchronously maps from a &Request to a Result<(), Error>. The
key differences are: - try_with takes a request by value, and allows
the predicate to return a different request value - try_with also
permits changing the type of the request - try_with is synchronous,
while tower::filter is asynchronous - tower::filter has a
Predicate trait, which can be implemented by more than just functions.
For example, a struct with a HashSet could implement Predicate by
failing requests that match the values in the hashset.

It definitely seems like there's demand for both synchronous and
asynchronous request filtering. However, the APIs we have currently
differ pretty significantly. It would be nice to make them more
consistent with each other.

As an aside, tower::filter does not seem all that widely used.

Meanwhile, linkerd2-proxy defines its own RequestFilter middleware,
using a predicate trait that's essentially in between tower::filter and
ServiceExt::try_with: - it's synchronous, like try_with - it allows
modifying the type of the request, like try_with - it uses a trait for
predicates, rather than a Fn, like tower::filter - it uses a similar
naming scheme to tower::filter ("filtering" rather than "with"/"map").

Solution

This branch rewrites tower::filter to make the following changes:

Predicates are synchronous by default. A separate AsyncFilter type
and an AsyncPredicate trait are available for predicates returning
futures.
Predicates may now return a new Request type, allowing Filter and
AsyncFilter to subsume try_map_request.
Predicates may now return any error type, and errors are now converted
into BoxErrors.

Closes #502

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

hawkw · 2021-01-06T18:10:00Z

+macro_rules! opaque_future {
+    ($(#[$m:meta])* pub type $name:ident<$($param:ident),+> = $actual:ty;) => {
+        #[pin_project::pin_project]
+        $(#[$m])*
+        pub struct $name<$($param),+>(#[pin] pub(crate) $actual);
+
+        impl<$($param),+> std::fmt::Debug for $name<$($param),+> {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                f.debug_tuple(stringify!($name)).field(&format_args!("...")).finish()
+            }
+        }
+
+        impl<$($param),+> std::future::Future for $name<$($param),+>
+        where
+            $actual: std::future::Future,
+        {
+            type Output = <$actual as std::future::Future>::Output;
+            #[inline]
+            fn poll(self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Self::Output> {
+                self.project().0.poll(cx)
+            }
+        }
+    }
+}


The idea behind this thing is that we probably don't want to expose type aliases for futures_util futures in public APIs, for stability reasons. Since type aliases don't hide the aliased type, users could refer to these as the nested chain of futures_util combinator types. Thus, if we change the specific combinator futures, that's potentially a breaking change.

I'll wrap other exposed combinator futures in opaque_future! when this merges.

LucioFranco

LGTM

LucioFranco · 2021-01-06T17:41:37Z


+    /// Conditionally reject requests based on `predicate`.
+    ///
+    /// `predicate` must implement the [`Predicate`] trait.


Suggested change

/// `predicate` must implement the [`Predicate`] trait.

/// - `predicate` must implement the [`Predicate`] trait.

minor nit: feel like this reads a bit better?

i don't think it makes sense to use a bulleted list with only one entry?

LucioFranco · 2021-01-06T17:42:03Z

+
+    /// Conditionally reject requests based on an asynchronous `predicate`.
+    ///
+    /// `predicate` must implement the [`AsyncPredicate`] trait.


Suggested change

/// `predicate` must implement the [`AsyncPredicate`] trait.

/// - `predicate` must implement the [`AsyncPredicate`] trait.

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

For stability reasons, we probably don't want to expose future combinator types from the `futures_util` crate in public APIs. If we were to change which combinators are used to implement these futures, or if `futures_util` made breaking changes, this could cause API breakage. This branch wraps all publicly exposed `futures_util` types in the `opaque_future!` macro added in #508, to wrap them in newtypes that hide their internal details. This way, we can change these futures' internals whenever we like.

* make all combinator futures opaque For stability reasons, we probably don't want to expose future combinator types from the `futures_util` crate in public APIs. If we were to change which combinators are used to implement these futures, or if `futures_util` made breaking changes, this could cause API breakage. This branch wraps all publicly exposed `futures_util` types in the `opaque_future!` macro added in #508, to wrap them in newtypes that hide their internal details. This way, we can change these futures' internals whenever we like. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

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 added 3 commits January 5, 2021 11:06

wip

b3f835a

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

rewrite tower::filter

9e71437

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

doctest fixup

2e7050a

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

hawkw requested review from LucioFranco, carllerche and olix0r January 6, 2021 01:27

feature fixup

b465b01

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

hawkw commented Jan 6, 2021

View reviewed changes

Comment thread tower/Cargo.toml Outdated

fix wrong feature name

00553a8

olix0r reviewed Jan 6, 2021

View reviewed changes

Comment thread tower/src/filter/predicate.rs Outdated

LucioFranco approved these changes Jan 6, 2021

View reviewed changes

hawkw added 2 commits January 6, 2021 12:06

remove error associated type from predicates

b118a90

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

remove bonus newline, misc cleanup

5efc32b

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

hawkw merged commit 2a7d47a into master Jan 6, 2021

hawkw deleted the eliza/new-filter branch January 6, 2021 20:18

hawkw mentioned this pull request Jan 6, 2021

make all combinator futures opaque #509

Merged

hawkw mentioned this pull request Jan 13, 2021

replace RequestFilter with Tower's upstream impl linkerd/linkerd2-proxy#842

Merged

dependabot Bot mentioned this pull request Mar 11, 2021

chore(deps): bump tower-test from 0.3.0 to 0.4.0 CleverCloud/vector#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewrite `tower::filter`#508

rewrite `tower::filter`#508
hawkw merged 7 commits into
masterfrom
eliza/new-filter

hawkw commented Jan 6, 2021 •

edited

Loading

Uh oh!

hawkw Jan 6, 2021

Uh oh!

Uh oh!

Uh oh!

LucioFranco left a comment

Uh oh!

LucioFranco Jan 6, 2021

Uh oh!

hawkw Jan 6, 2021

Uh oh!

LucioFranco Jan 6, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	/// `predicate` must implement the [`Predicate`] trait.
	/// - `predicate` must implement the [`Predicate`] trait.

	/// `predicate` must implement the [`AsyncPredicate`] trait.
	/// - `predicate` must implement the [`AsyncPredicate`] trait.

Conversation

hawkw commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

Uh oh!

hawkw Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LucioFranco left a comment

Choose a reason for hiding this comment

Uh oh!

LucioFranco Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

hawkw Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

LucioFranco Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hawkw commented Jan 6, 2021 •

edited

Loading