Closed
Conversation
Signed-off-by: Eliza Weisman <[email protected]>
(and, don't rely on a specific error status to know what happened) Signed-off-by: Eliza Weisman <[email protected]>
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 <[email protected]>
Member
|
Per discussion in linkerd/linkerd2#2880, should this be changed to never fallback when a NoEndpoints is returned? |
Contributor
Author
|
@olix0r I'm happy to make that change, but I do wonder about one potential case: suppose that kube-dns sees the creation of a service before the Destination service does, so an app resolves the DNS name and gets back an IP, and sends the request to the proxy. The proxy looks up that name in the Destination service, and gets back a |
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
hawkw
changed the title
exists == false
Contributor
Author
|
@olix0r as we discussed offline, f20846c changes this branch so that we now only fall back on |
olix0r
reviewed
Jun 7, 2019
Signed-off-by: Eliza Weisman <[email protected]>
olix0r
reviewed
Jun 10, 2019
Member
olix0r
left a comment
olix0r
left a comment
There was a problem hiding this comment.
If I understand correctly, we can likely simplify the proxy::resolve and fallback modules quite a bit if we can pull the "NoEndpoints" (unresolveable) error up higher, since it will be received immediately upon trying to discover invalid services.
Contributor
Author
hawkw
added a commit
that referenced
this pull request
Jun 12, 2019
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 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the Destination service returns a
NoEndpointsresponse, a fieldexistsis set if the destination does exist in service discoverybut has no endpoints. The API spec states that:
When the DNS fallback behavior 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-existent endpoint ultimately fails.
This branch changes the
control::destination::resolution::Daemonfuture to only send
Update::NoEndpoints(which indicates that the loadbalancer should fall back) to the load balancer when the Destination
service response has
exists: false, or onInvalidArgumenterrors,and not when a
no_endpoints{exists: true}response is received.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 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_endpointstooutbound_fails_fast_when_destination_has_no_endpoints. I've alsoconfirmed that the updated test fails against master and passes after
this change.
Fixes linkerd/linkerd2#2880
Signed-off-by: Eliza Weisman [email protected]