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
proxy: Do not fallback when the control plane returns NoEndpoints #2880
Comments
The Destination API's // `no_endpoints{exists: false}` indicates that the service does not exist
// and the client MAY try an alternate service discovery method (e.g. DNS).
//
// `no_endpoints(exists: true)` indicates that the service does exist and
// the client MUST NOT fall back to an alternate service discovery method.
NoEndpoints no_endpoints = 3; Based on my understanding of this API, we should fall back on receipt of a |
When the Destination service returns a `NoEndpoints` response, a field `exists` is set if the destination _does_ exist in service discovery but has no endpoints. The API spec states that: > `no_endpoints{exists: false}` indicates that the service does not exist > and the client MAY try an alternate service discovery method (e.g. DNS). > > `no_endpoints(exists: true)` indicates that the service does exist and > the client MUST NOT fall back to an alternate service discovery > method. When the DNS fallback behaviour was removed from the proxy in #259, this field was overlooked, and the proxy was changed to fall back to original destination routing _any_ time the control plane indicates that no endpoints exist for a destination. This is incorrect, as the proxy should always treat the destination service as authoritative. Additionally, this means that requests to endpoints which are known to not exist will construct an unnecessary client service, and eventually fail with a 502 error when the upstream client connection to the non-existant endpoint ultimately fails. This branch changes the `control::destination::resolution::Daemon` future to only send `Update::NoEndpoints` (which indicates that the load balancer should fall back) to the load balancer when the Destination service response has `exists: false`, or on `InvalidArgument` errors, and _not_ when a `no_endpoints{exists: true}` response is recieved. These requests will now fail fast with a 503 error rather than attempting an ultimately futile fallback request. Previously, one of the discovery tests expected the incorrect behaviour. I've changed this test to now expect that destinations known to not exist do _not_ fall back, and renamed it from `outbound_falls_back_to_orig_dst_when_destination_has_no_endpoints` to `outbound_fails_fast_when_destination_has_no_endpoints`. I've also confirmed that the updated test fails against master and passes after this change. Fixes linkerd/linkerd2#2880 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
I understand that's in the API, but I'm not convinced that policy makes sense going forward... If you resolve a service and then it is later deleted, it seems appropriate for the proxy to eagerly fail these requests. Is there a use case when we would want to forward requests for non-existent services? |
I believe that the intention behind It seems possible that the |
@hawkw not exactly. |
@olix0r ah, yeah, that's an important distinction, thanks for pointing that out. |
When the DNS fallback behavior was removed from the proxy in #259, the proxy was changed to fall back to original destination routing _any_ time the control plane indicates that no endpoints exist for a destination. This is incorrect, as the proxy should always treat the destination service as authoritative. Additionally, this means that requests to endpoints which are known to not exist will construct an unnecessary client service, and eventually fail with a 502 error when the upstream client connection to the non-existent endpoint ultimately fails. Instead, the proxy should only fall back when the destination is outside the search suffixes or the Destination service returns an `InvalidArgument` response. This is a much larger change than #263. In particular, the fallback behavior has been moved from when the the actual client service is called to when the client service is _constructed_, as we do not expect to recieve an `InvalidArgument` response on a Destination query that has previously recieved updates. This was implemented by changing the `Resolve` trait to return a `Future` whose `Item` type is a `Resolution`, rather than returning a `Resolution`. When the Destination query returns `InvalidArgument`, the future will fail. The `proxy::http::fallback` middleware has been changed so that rather than making both the primary and fallback service and falling back on a per-request basis, it tries to make the primary service, and falls back if the primary `MakeService` future fails. Although this is a large change, I think the resulting code is simpler and more elegant than the previous approach. Previously, one of the discovery tests expected the incorrect behavior. I've changed this test to now expect that destinations known to not exist do _not_ fall back, and renamed it from `outbound_falls_back_to_orig_dst_when_destination_has_no_endpoints` to `outbound_fails_fast_when_destination_has_no_endpoints`. I've also confirmed that the updated test fails against master and passes after this change. Fixes linkerd/linkerd2#2880 Closes #263 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
When the DNS fallback behavior was removed from the proxy in #259, the proxy was changed to fall back to original destination routing _any_ time the control plane indicates that no endpoints exist for a destination. This is incorrect, as the proxy should always treat the destination service as authoritative. Additionally, this means that requests to endpoints which are known to not exist will construct an unnecessary client service, and eventually fail with a 502 error when the upstream client connection to the non-existent endpoint ultimately fails. Instead, the proxy should only fall back when the destination is outside the search suffixes or the Destination service returns an `InvalidArgument` response. This is a much larger change than #263. In particular, the fallback behavior has been moved from when the the actual client service is called to when the client service is _constructed_, as we do not expect to recieve an `InvalidArgument` response on a Destination query that has previously recieved updates. This was implemented by changing the `Resolve` trait to return a `Future` whose `Item` type is a `Resolution`, rather than returning a `Resolution`. When the Destination query returns `InvalidArgument`, the future will fail. The `proxy::http::fallback` middleware has been changed so that rather than making both the primary and fallback service and falling back on a per-request basis, it tries to make the primary service, and falls back if the primary `MakeService` future fails. Although this is a large change, I think the resulting code is simpler and more elegant than the previous approach. Previously, one of the discovery tests expected the incorrect behavior. I've changed this test to now expect that destinations known to not exist do _not_ fall back, and renamed it from `outbound_falls_back_to_orig_dst_when_destination_has_no_endpoints` to `outbound_fails_fast_when_destination_has_no_endpoints`. I've also confirmed that the updated test fails against master and passes after this change. Fixes linkerd/linkerd2#2880 Closes #263 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
We must assume that the control plane is authoritative (in order to enforce identity, policy, etc). When we initially scoped the fallback logic, it seemed to make sense that the the proxy should use fallback routing when the control plane reports no endpoints. In practice, it seems better for the proxy to rely on its load balancer as long as the control plane does not return a IllegalArgument error.
The text was updated successfully, but these errors were encountered: