Load balancers fall back to ORIG_DST when no endpoints exist #248

hawkw · 2019-05-08T22:31:06Z

Currently, when no endpoints exist in the load balancer for a
destination, we fail the request. This is because we expect endpoints to
be discovered by both destination service queries and DNS lookups, so
if there are no endpoints for a destination, it is assumed to not exist.

In linkerd/linkerd2#2661, we intend to remove the DNS lookup from the
proxy and instead fall back to routing requests for which no endpoints
exist in the destination service to their SO_ORIGINAL_DST IP address.
This means that the current approach of failing requests when the load
balancer has no endpoints will no longer work.

This branch introduces a generic fallback layer, which composes a
primary and secondary service builder into a new layer. The primary
service can fail requests with an error type that propages the original
request, allowing the fallback middleware to call the fallback service
with the same request. Other errors returned by the primary service are
still propagated upstream.

In contrast to the approach used in #240, this fallback middleware is
generic and not tied directly to a load balancer or a router, and can
be used for other purposes in the future. It relies on the router cache
eviction added in #247 to drain the router when it is not being used,
rather than proactively destroying the router when endpoints are
available for the lb, and re-creating it when they exist again.

A new trait, HasEndpointStatus, is added in order to allow the
discovery lookup to communicate the "no endpoints" state to the
balancer. In addition, we add a new Update::NoEndpoints variant to
proxy::resolve::Update, so that when the control plane sends a no
endpoints update, we switch from the balancer to the no endpoints state
immediately, rather than waiting for all the endpoints to be
individually removed. When the balancer has no endpoints, it fails all
requests with a fallback error, so that the fallback middleware

A future commit will remove the DNS lookups from the discovery module.

Closes #240.

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

src/app/main.rs

hawkw · 2019-05-08T22:58:32Z

src/proxy/http/router.rs

+impl<Req, Rec> Clone for Layer<Req, Rec>
+where
+    Rec: Recognize<Req> + Clone + Send + Sync + 'static,
+{


The previous derived Clone implementation for Layer (and for Stack) required that the request type also be Clone, even though it's a PhantomData; this had to be fixed since the fallback layer requires cloning the fallback service.

src/app/main.rs

src/proxy/http/fallback.rs

kleimkuhler

This looks good. I also found the type Layer<...> in fallback.rs to be unclear but it's not a blocker for me.

When we ready Ready for MakeFallback still needs to be figured out. I think I prefer the way the way it currently is -- applying backpressure when making the service instead of falling back and having the possibility that the fallback service is not ready.

src/app/main.rs

src/proxy/http/fallback.rs

hawkw · 2019-05-14T21:21:18Z

@olix0r and @kleimkuhler, I've made all the changes you requested. I think we still need to agree on what the correct thing to do when the fallback service is not ready is.

@olix0r

Currently, router `layer`s are constructed with a single argument, a type implementing `Recognize`. Then, the entire router stack is built with a `router::Config`. However, in #248, it became necessary to provide the config up front when constructing the `router::layer`, as the layer is used in a fallback layer. Rather than providing a separate type for a preconfigured layer, @olix0r suggested we simply change all router layers to accept the `Config` when they're constructed (see #248 (comment)). This branch changes `router::Layer` to accept the config up front. The `router::Stack` types `make` function now requires no arguments, and the implementation of `Service` for `Stack` can be called with any `T` (as the target is now ignored). Signed-off-by: Eliza Weisman <eliza@buoyant.io>

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

src/proxy/http/balance.rs

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

olix0r

For posterity, here's how I tested this:

deployed strest with a proxy from this branch
confirmed traffic to the dst pod
change the service so that no pods are selected in the service
confirm that traffic continues to the pod despite the service being emptied
add a new client (that can't resolve the service name to any ips) and confirm that it cannot send requests

The `tower-balance` load balancer's `poll_ready` function returns `NotReady` when the load balancer has no ready endpoints. However, our fallback strategy added in #248 requires the load balancer to accept a request when it has no endpoints, so that it can return the error indicating that the fallback router should be used. This branch changes the proxy's `http::balance::Service`'s `poll_ready` implementation to return `Ok(Async::Ready(()))` when it is in the no endpoints state, so that the request will fall back. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

@olix0r

commit b27dfb2d21aa8ca5466ea0edce17d27094ace7c1 Author: Takanori Ishibashi <takanori.1112@gmail.com> Date: Wed May 15 05:58:42 2019 +0900 updaes->updates (#250) Signed-off-by: Takanori Ishibashi <takanori.1112@gmail.com> commit 16441c25a9d423a6ab12b689b830d9ae3798fa00 Author: Eliza Weisman <eliza@buoyant.io> Date: Tue May 14 14:40:03 2019 -0700 Pass router::Config directly to router::Layer (#253) Currently, router `layer`s are constructed with a single argument, a type implementing `Recognize`. Then, the entire router stack is built with a `router::Config`. However, in #248, it became necessary to provide the config up front when constructing the `router::layer`, as the layer is used in a fallback layer. Rather than providing a separate type for a preconfigured layer, @olix0r suggested we simply change all router layers to accept the `Config` when they're constructed (see linkerd/linkerd2-proxy#248 (comment)). This branch changes `router::Layer` to accept the config up front. The `router::Stack` types `make` function now requires no arguments, and the implementation of `Service` for `Stack` can be called with any `T` (as the target is now ignored). Signed-off-by: Eliza Weisman <eliza@buoyant.io> commit b70c68d4504a362eac6a7828039a2e5c7fcd308a Author: Eliza Weisman <eliza@buoyant.io> Date: Wed May 15 13:14:04 2019 -0700 Load balancers fall back to ORIG_DST when no endpoints exist (#248) Currently, when no endpoints exist in the load balancer for a destination, we fail the request. This is because we expect endpoints to be discovered by both destination service queries _and_ DNS lookups, so if there are no endpoints for a destination, it is assumed to not exist. In #2661, we intend to remove the DNS lookup from the proxy and instead fall back to routing requests for which no endpoints exist in the destination service to their SO_ORIGINAL_DST IP address. This means that the current approach of failing requests when the load balancer has no endpoints will no longer work. This branch introduces a generic `fallback` layer, which composes a primary and secondary service builder into a new layer. The primary service can fail requests with an error type that propages the original request, allowing the fallback middleware to call the fallback service with the same request. Other errors returned by the primary service are still propagated upstream. In contrast to the approach used in #240, this fallback middleware is generic and not tied directly to a load balancer or a router, and can be used for other purposes in the future. It relies on the router cache eviction added in #247 to drain the router when it is not being used, rather than proactively destroying the router when endpoints are available for the lb, and re-creating it when they exist again. A new trait, `HasEndpointStatus`, is added in order to allow the discovery lookup to communicate the "no endpoints" state to the balancer. In addition, we add a new `Update::NoEndpoints` variant to `proxy::resolve::Update`, so that when the control plane sends a no endpoints update, we switch from the balancer to the no endpoints state _immediately_, rather than waiting for all the endpoints to be individually removed. When the balancer has no endpoints, it fails all requests with a fallback error, so that the fallback middleware A subsequent PR (#248) will remove the DNS lookups from the discovery module. Closes #240. Signed-off-by: Eliza Weisman <eliza@buoyant.io> commit 6525b0638ad18e74510f3156269e0613f237e2f5 Author: Zahari Dichev <zaharidichev@gmail.com> Date: Wed May 15 23:35:09 2019 +0300 Allow disabling tap by setting an env var (#252) This PR fixes #2811. Now if `LINKERD2_PROXY_TAP_DISABLED` is set, the tap is not served at all. The approach taken is that the `ProxyParts` is changed so the `control_listener` is now an `Option` that will be None if tap is disabled as this control_listener seems to be exclusively used to serve the tap. Feel free to suggest a better approach. Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> commit 91f32db2ea6d74470fd689c713ff87dc7586222d Author: Zahari Dichev <zaharidichev@gmail.com> Date: Thu May 16 00:45:23 2019 +0300 Assert that outbound TLS works before identity is certified (#251) This commit introduces TLS capabilities to the support server as well as tests to ensure that outbound TLS works even when there is no verified certificate for the proxy yet. Fixes #2599 Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> commit 45aadc6b1b28e6daea0c40e694a86ae518887d85 Author: Sean McArthur <sean@buoyant.io> Date: Wed May 15 14:25:39 2019 -0700 Update h2 to v0.1.19 Includes a couple HPACK fixes Signed-off-by: Sean McArthur <sean@buoyant.io> commit 3e0e00c6dfbf5a9155b887cfd594f611edfc135f Author: Oliver Gould <ver@buoyant.io> Date: Thu May 16 08:11:06 2019 -0700 Update mio to 0.6.17 (#257) To pick up tokio-rs/mio#939

@olix0r

commit b27dfb2d21aa8ca5466ea0edce17d27094ace7c1 Author: Takanori Ishibashi <takanori.1112@gmail.com> Date: Wed May 15 05:58:42 2019 +0900 updaes->updates (#250) Signed-off-by: Takanori Ishibashi <takanori.1112@gmail.com> commit 16441c25a9d423a6ab12b689b830d9ae3798fa00 Author: Eliza Weisman <eliza@buoyant.io> Date: Tue May 14 14:40:03 2019 -0700 Pass router::Config directly to router::Layer (#253) Currently, router `layer`s are constructed with a single argument, a type implementing `Recognize`. Then, the entire router stack is built with a `router::Config`. However, in #248, it became necessary to provide the config up front when constructing the `router::layer`, as the layer is used in a fallback layer. Rather than providing a separate type for a preconfigured layer, @olix0r suggested we simply change all router layers to accept the `Config` when they're constructed (see linkerd/linkerd2-proxy#248 (comment)). This branch changes `router::Layer` to accept the config up front. The `router::Stack` types `make` function now requires no arguments, and the implementation of `Service` for `Stack` can be called with any `T` (as the target is now ignored). Signed-off-by: Eliza Weisman <eliza@buoyant.io> commit b70c68d4504a362eac6a7828039a2e5c7fcd308a Author: Eliza Weisman <eliza@buoyant.io> Date: Wed May 15 13:14:04 2019 -0700 Load balancers fall back to ORIG_DST when no endpoints exist (#248) Currently, when no endpoints exist in the load balancer for a destination, we fail the request. This is because we expect endpoints to be discovered by both destination service queries _and_ DNS lookups, so if there are no endpoints for a destination, it is assumed to not exist. In #2661, we intend to remove the DNS lookup from the proxy and instead fall back to routing requests for which no endpoints exist in the destination service to their SO_ORIGINAL_DST IP address. This means that the current approach of failing requests when the load balancer has no endpoints will no longer work. This branch introduces a generic `fallback` layer, which composes a primary and secondary service builder into a new layer. The primary service can fail requests with an error type that propages the original request, allowing the fallback middleware to call the fallback service with the same request. Other errors returned by the primary service are still propagated upstream. In contrast to the approach used in #240, this fallback middleware is generic and not tied directly to a load balancer or a router, and can be used for other purposes in the future. It relies on the router cache eviction added in #247 to drain the router when it is not being used, rather than proactively destroying the router when endpoints are available for the lb, and re-creating it when they exist again. A new trait, `HasEndpointStatus`, is added in order to allow the discovery lookup to communicate the "no endpoints" state to the balancer. In addition, we add a new `Update::NoEndpoints` variant to `proxy::resolve::Update`, so that when the control plane sends a no endpoints update, we switch from the balancer to the no endpoints state _immediately_, rather than waiting for all the endpoints to be individually removed. When the balancer has no endpoints, it fails all requests with a fallback error, so that the fallback middleware A subsequent PR (#248) will remove the DNS lookups from the discovery module. Closes #240. Signed-off-by: Eliza Weisman <eliza@buoyant.io> commit 6525b0638ad18e74510f3156269e0613f237e2f5 Author: Zahari Dichev <zaharidichev@gmail.com> Date: Wed May 15 23:35:09 2019 +0300 Allow disabling tap by setting an env var (#252) This PR fixes #2811. Now if `LINKERD2_PROXY_TAP_DISABLED` is set, the tap is not served at all. The approach taken is that the `ProxyParts` is changed so the `control_listener` is now an `Option` that will be None if tap is disabled as this control_listener seems to be exclusively used to serve the tap. Feel free to suggest a better approach. Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> commit 91f32db2ea6d74470fd689c713ff87dc7586222d Author: Zahari Dichev <zaharidichev@gmail.com> Date: Thu May 16 00:45:23 2019 +0300 Assert that outbound TLS works before identity is certified (#251) This commit introduces TLS capabilities to the support server as well as tests to ensure that outbound TLS works even when there is no verified certificate for the proxy yet. Fixes #2599 Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> commit 45aadc6b1b28e6daea0c40e694a86ae518887d85 Author: Sean McArthur <sean@buoyant.io> Date: Wed May 15 14:25:39 2019 -0700 Update h2 to v0.1.19 Includes a couple HPACK fixes Signed-off-by: Sean McArthur <sean@buoyant.io> commit 3e0e00c6dfbf5a9155b887cfd594f611edfc135f Author: Oliver Gould <ver@buoyant.io> Date: Thu May 16 08:11:06 2019 -0700 Update mio to 0.6.17 (#257) To pick up tokio-rs/mio#939

This branch removes the fall-back-to-DNS behaviour from the proxy's destination module. Instead, when the Destination service returns "no endpoints", the proxy will route the request to its original destination, using the fallback behaviour introduced in #248. In addition, the proxy now no longer reconnects to the destination service for queries which receive a response with the Invalid Argument status code. After linkerd/linkerd2#2664, this status is used by the control plane to indicate names which should not query the destination service. I've added a new test for this. Furthermore (and unlike #245), this branch also removes the caching of service discovery lookups so that they can be shared between resolvers for authorities with the same canonical form. When we limited the number of concurrently in flight destination service queries to 100, reusing them was much more important; now it adds a lot of unnecessary complexity. The rewritten `control::destination` module is now much simpler and (hopefully) easier to understand. The `Resolver` holds a destination service `Client`, which is cloned every time a new `Resolution` is requested. Each active `Resolution` has a corresponding background task that drives the destination service query and sends updates through a channel to the `Resolver`. The background task is necessary so that updates are processed as they are recieved, rather than only when the `Resolution` is polled (i.e. when the load balancer is routing a request). In addition, the channel provides the queueing necessary to transform the control plane's update types (which contain multiple endpoint addresses) into multiple `tower-balance` updates for single endpoints, and to invalidate stale endpoints when the control plane connection is reestablished. When a control plane query is not necessary because the requested authority does not match the proxy's destination suffixes, the background task is not spawned and the `Resolution` remains permanently in the "no endpoints" state; and when an `InvalidArgument` error code is recieved, the background task terminates. A majority of the service discovery tests pass with no changes after this change. The following changes were necessary: * The test `outbound_updates_newer_services` was removed, as it tests that the same destination service query is shared between multiple services with the same authority (which is no longer the case). * The mock destination service was changed to send the `Unavailable` status code when it has run out of expected destinations, rather than `InvalidArgument`, as that code is used to indicate that a destination should never be reconnected. * New tests were added for falling back to ORIG_DST when no endpoints exist, and for not reconnecting after an `InvalidArgument` error. Closes #245. Closes linkerd/linkerd2#2661 Closes linkerd/linkerd2#2728 Signed-off-by: Eliza Weisman eliza@buoyant.io

hawkw requested review from olix0r and kleimkuhler May 8, 2019 22:31

hawkw self-assigned this May 8, 2019

hawkw commented May 8, 2019

View reviewed changes

src/app/main.rs Outdated Show resolved Hide resolved

hawkw commented May 8, 2019

View reviewed changes

hawkw force-pushed the liza/fallback branch from c8607bb to 3f68c26 Compare May 9, 2019 23:52

olix0r reviewed May 13, 2019

View reviewed changes

hawkw force-pushed the liza/fallback branch from 880b7e5 to 123b2b7 Compare May 13, 2019 23:57

kleimkuhler reviewed May 14, 2019

View reviewed changes

src/app/main.rs Outdated Show resolved Hide resolved

src/proxy/http/fallback.rs Outdated Show resolved Hide resolved

hawkw force-pushed the liza/fallback branch from 123b2b7 to 076995b Compare May 14, 2019 21:05

hawkw mentioned this pull request May 14, 2019

Pass router::Config directly to router::Layer #253

Merged

hawkw requested review from olix0r and kleimkuhler and removed request for olix0r May 14, 2019 21:20

hawkw added 13 commits May 14, 2019 14:40

Add a generic HTTP fallback layer

6a4dbc0

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

Add "no endpoints" state to resolve::Discover

bbb72aa

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

Hack destination_set to send NoEndpoints

65f4f64

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

wire up endpoint status *without* breaking stuff

668df8d

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

make balancer fallback-aware

40ffba8

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

fix Clone impls requiring req bodies to be Clone

70fde83

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

actually wire up new fallback stuff

6e812aa

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

make errors compose a little nicer

2101438

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

give travis a longer timeout

6290cbe

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

simplify potentially confusing poll_ready impls

83192f8

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

style

44ceee9

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

make Making less confusing

c7ff05b

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

cleanup

46f9bea

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

hawkw force-pushed the liza/fallback branch from 4c43d82 to 46f9bea Compare May 14, 2019 21:40

hawkw mentioned this pull request May 14, 2019

Simplify destination discovery fallback #245

Closed

olix0r reviewed May 15, 2019

View reviewed changes

src/proxy/http/balance.rs Show resolved Hide resolved

hawkw added 3 commits May 15, 2019 11:16

Handle fallback service backpressure in response future

a025826

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

clean up, add comments

bb2bf7e

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

actually these do need to be separate cases...

c5d9c11

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

hawkw requested a review from olix0r May 15, 2019 18:45

olix0r approved these changes May 15, 2019

View reviewed changes

hawkw merged commit b70c68d into master May 15, 2019

hawkw mentioned this pull request May 15, 2019

Fix load balancer not being ready when it wants to fall back #255

Closed

hawkw mentioned this pull request May 20, 2019

Rewrite the destination client and remove DNS fallback #259

Merged

olix0r deleted the liza/fallback branch August 17, 2019 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load balancers fall back to ORIG_DST when no endpoints exist #248

Load balancers fall back to ORIG_DST when no endpoints exist #248

hawkw commented May 8, 2019 •

edited

Loading

hawkw May 8, 2019

kleimkuhler left a comment

hawkw commented May 14, 2019

olix0r left a comment

Load balancers fall back to ORIG_DST when no endpoints exist #248

Load balancers fall back to ORIG_DST when no endpoints exist #248

Conversation

hawkw commented May 8, 2019 • edited Loading

hawkw May 8, 2019

Choose a reason for hiding this comment

kleimkuhler left a comment

Choose a reason for hiding this comment

hawkw commented May 14, 2019

olix0r left a comment

Choose a reason for hiding this comment

hawkw commented May 8, 2019 •

edited

Loading