dns cache: add DNS query timeout option #17207

mattklein123 · 2021-06-30T23:03:27Z

Risk Level: Low
Testing: New UT
Docs Changes: Added
Release Notes: Added
Platform Specific Features: N/A

Signed-off-by: Matt Klein <[email protected]>

repokitteh-read-only · 2021-06-30T23:03:34Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17207 was opened by mattklein123.

see: more, trace.

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

alyssawilk

Looks good overall! Few thoughts below

alyssawilk · 2021-07-01T13:45:06Z

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

+  // The timeout used for DNS queries. This timeout is independent of any timeout and retry policy
+  // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque.
+  // Setting this timeout will ensure that queries succeed or fail within the specified time frame
+  // and are then retried using the standard refresh rates. Defaults to 5s if not set.


Can we make it a bit more clear if the original request is canceled or if we retry in parallel like hedging? I recall (possibly erroneously) that was the latency-improving timeout folks often use on mobile of "if DNS does not return in this time use stale result" which implies to me 5s happens frequently enough we don't want to cancel. Alternately if platform implementation details would result in the OS getting the results for the original request that's Ok too but I'm not sure how to regression test that. cc @RyanTheOptimist @DavidSchinazi for thoughts as this is more their area than mine.

Yeah this is a good call out. The current implementation will actually cancel and not hedge. Part of the impetus for this change is we think the iOS/apple resolver is occasionally getting wedged, so we are going to accompany this with some changes to that resolver to hopefully make it better handle timeouts (cc @junr03). One thing we might want to do here is actually modify the cancel() API to take a reason parameter, so that the impl can do different things if it's a normal cancellation vs. a timeout. I think in the case of timeout we might want to have the impls tare down the DNS connections and make new ones. Let me do that in this change.

What does "hedge" mean in this context?

I'm not sure about 5s being the right value. I think that's probably too short for a client network stack which need to operate in congested lossy networks. I could look at some telemetry from Chrome around DNS resolution times if that would help? (Of course, I don't know much about the forward proxy deployments so maybe that's the right value for them)

FWIW, the way the stale DNS racing works in Chrome with QUIC is as follows. First host resolution is attempted. If no "Fresh" (presumably cached) result is available but a stale result is (this is all synchronous and does not involve waiting for network events) then the QUIC connection is started using the stale DNS entry. At this point we are running the QUIC handshake in parallel with the DNS request. We only use the resulting QUIC connection to send requests on if the DNS comes back and matches the IP connected to.

To do this, Chrome's resolver basically has two APIs. One for getting a fresh address (which will be async if the address is not cached) and another for getting a stale address (which is always synchronous but may return nothing).

Does that help?

I could look at some telemetry from Chrome around DNS resolution times if that would help?

Yeah that would be super useful. My feeling though is this is an OK default for server and we can tune it for envoy-mobile config.

Does that help?

Yes definitely. I think this is what we need to move towards. cc @junr03

Yeah I'm good with the default being server side as long as we make sure that @goaway and @junr03 and co know to tweak their defaults when they pick up the change.

oh and @RyanTheOptimist hedging is an option in router config where you can configure a retry in parallel - 2 requests to 2 upstreams, and cancel the slow one once the first response headers coming back. My point was I was ok with 5s for mobile if we were kicking off a second attempt not canceling the first but I'm also Ok with just upping the default on mobile.

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

Signed-off-by: Matt Klein <[email protected]>

mattklein123 · 2021-07-01T19:12:45Z

@alyssawilk @junr03 I think this is ready for another pass. The tidy error is not real and I'm not exactly sure how to squelch it.

alyssawilk · 2021-07-07T14:46:00Z

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

+  // The timeout used for DNS queries. This timeout is independent of any timeout and retry policy
+  // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque.
+  // Setting this timeout will ensure that queries succeed or fail within the specified time frame
+  // and are then retried using the standard refresh rates. Defaults to 5s if not set.


Yeah I'm good with the default being server side as long as we make sure that @goaway and @junr03 and co know to tweak their defaults when they pick up the change.

alyssawilk · 2021-07-07T14:47:50Z

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

+  // The timeout used for DNS queries. This timeout is independent of any timeout and retry policy
+  // used by the underlying DNS implementation (e.g., c-areas and Apple DNS) which are opaque.
+  // Setting this timeout will ensure that queries succeed or fail within the specified time frame
+  // and are then retried using the standard refresh rates. Defaults to 5s if not set.


oh and @RyanTheOptimist hedging is an option in router config where you can configure a retry in parallel - 2 requests to 2 upstreams, and cancel the slow one once the first response headers coming back. My point was I was ok with 5s for mobile if we were kicking off a second attempt not canceling the first but I'm also Ok with just upping the default on mobile.

alyssawilk · 2021-07-07T14:50:11Z

source/common/network/apple_dns_impl.cc

@@ -265,7 +265,9 @@ AppleDnsResolverImpl::PendingResolution::~PendingResolution() {
  }
 }

-void AppleDnsResolverImpl::PendingResolution::cancel() {
+void AppleDnsResolverImpl::PendingResolution::cancel(Network::ActiveDnsQuery::CancelReason) {
+  // TODO(mattklein123): If cancel reason is timeout, do something more aggressive about destroying


I'd be inclined to remove the argument if it's not used anywhere, unless you actually plan on following up in the not too distant future.

My intention here was to follow up with at least the Apple DNS provider (and maybe c-ares) to blow away the channels if we have a timeout, to make it more likely we can work around dead connections. cc @junr03 wdyt?

@mattklein123 For the Apple implementation post #17226 there is nothing to blow away as each PendingResolution is getting its own socket (and registered file event with the event loop).

On the c-ares implementation, where the connection is shared, the distinction would be useful. A timeout based cancelation could be treated the same as a c-ares error and used to blow up the existing channel.

OK, let's keep it, and we can make the c-ares change for Android, which I think would be useful IMO.

Description: Includes three relevant commits: envoyproxy/envoy#17226 envoyproxy/envoy#17274 envoyproxy/envoy#17207 Signed-off-by: Jose Nino <[email protected]>

Description: Includes three relevant commits: envoyproxy/envoy#17226 envoyproxy/envoy#17274 envoyproxy/envoy#17207 Signed-off-by: Jose Nino <[email protected]> Signed-off-by: Rafal Augustyniak <[email protected]>

Signed-off-by: Matt Klein <[email protected]>

Description: Includes three relevant commits: #17226 #17274 #17207 Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>

dns cache: add DNS query timeout option

baef43e

Signed-off-by: Matt Klein <[email protected]>

mattklein123 requested a review from alyssawilk as a code owner June 30, 2021 23:03

repokitteh-read-only bot added the api label Jun 30, 2021

repokitteh-read-only bot assigned markdroth Jun 30, 2021

mattklein123 assigned junr03 and alyssawilk and unassigned markdroth Jun 30, 2021

mattklein123 commented Jun 30, 2021

View reviewed changes

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc Show resolved Hide resolved

alyssawilk reviewed Jul 1, 2021

View reviewed changes

alyssawilk added the waiting label Jul 1, 2021

mattklein123 added 3 commits July 1, 2021 17:12

Merge remote-tracking branch 'origin/main' into dns_timeout

71de6be

Signed-off-by: Matt Klein <[email protected]>

comments

e822fdf

Signed-off-by: Matt Klein <[email protected]>

Merge remote-tracking branch 'origin/main' into dns_timeout

7a2708a

Signed-off-by: Matt Klein <[email protected]>

repokitteh-read-only bot removed the waiting label Jul 1, 2021

alyssawilk approved these changes Jul 7, 2021

View reviewed changes

mattklein123 merged commit ea9ef5f into main Jul 7, 2021

mattklein123 deleted the dns_timeout branch July 7, 2021 21:00

junr03 mentioned this pull request Jul 12, 2021

envoy: 3876d7c0f envoyproxy/envoy-mobile#1580

Merged

goaway pushed a commit to envoyproxy/envoy-mobile that referenced this pull request Jul 12, 2021

envoy: 3876d7c0f (#1580)

9df58ea

Description: Includes three relevant commits: envoyproxy/envoy#17226 envoyproxy/envoy#17274 envoyproxy/envoy#17207 Signed-off-by: Jose Nino <[email protected]>

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

dns cache: add DNS query timeout option (envoyproxy#17207)

012f005

Signed-off-by: Matt Klein <[email protected]>

jpsim pushed a commit that referenced this pull request Nov 28, 2022

envoy: 3876d7c (#1580)

65e5088

Description: Includes three relevant commits: #17226 #17274 #17207 Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

envoy: 3876d7c (#1580)

b34ed59

Description: Includes three relevant commits: #17226 #17274 #17207 Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns cache: add DNS query timeout option #17207

dns cache: add DNS query timeout option #17207

mattklein123 commented Jun 30, 2021

repokitteh-read-only bot commented Jun 30, 2021

alyssawilk left a comment

alyssawilk Jul 1, 2021

mattklein123 Jul 1, 2021 •

edited

Loading

RyanTheOptimist Jul 1, 2021

mattklein123 Jul 1, 2021

alyssawilk Jul 7, 2021

alyssawilk Jul 7, 2021

mattklein123 commented Jul 1, 2021

alyssawilk Jul 7, 2021

alyssawilk Jul 7, 2021

alyssawilk Jul 7, 2021

mattklein123 Jul 7, 2021

junr03 Jul 7, 2021 •

edited

Loading

mattklein123 Jul 7, 2021

dns cache: add DNS query timeout option #17207

dns cache: add DNS query timeout option #17207

Conversation

mattklein123 commented Jun 30, 2021

repokitteh-read-only bot commented Jun 30, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 Jul 1, 2021 •

edited

Loading

junr03 Jul 7, 2021 •

edited

Loading