dns resolvers: add All lookup mode by junr03 · Pull Request #18464 · envoyproxy/envoy

junr03 · 2021-10-05T22:28:16Z

Commit Message: dns resolvers - add All lookup mode
Additional Description: this change will enable callback targets to access both address families for a given hostname.
Risk Level: low - new behavior guarded by config option
Testing: added UT
Docs Changes: updated
Release Notes: updated

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only · 2021-10-05T22:28:23Z

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

🐱

Caused by: #18464 was opened by junr03.

see: more, trace.

junr03 · 2021-10-05T22:29:25Z

source/common/network/dns_impl.cc

@@ -104,16 +104,24 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
    // ARES_ECONNREFUSED. If the PendingResolution has not been cancelled that means that the


The c-ares based resolver changed a bit more because it was not originally written to accumulate responses from multiple c-ares callbacks.

I refactored for ease of understanding but open to comments to simplify further!

junr03 · 2021-10-05T22:30:51Z

source/common/network/dns_impl.cc

+                    dns_name_);
+
+    if (!pending_response_.address_list_.empty()) {
+      ASSERT(dns_lookup_family_ == DnsLookupFamily::All && !dual_resolution_);


Here and below I considered a failure in the second resolution an overall success if the first resolution was successful, and hence fired the callback with the existing addresses and a success state. Up to discussing if this should be the behavior.

yeah IMO a resolve success of either v4 or v6 should be considered success.
cc @DavidSchinazi @RyanTheOptimist for informed opinions :-)

+1, success of v4 OR v6 should mean overall success since we have usable addresses

Signed-off-by: Jose Nino <jnino@lyft.com>

alyssawilk

Nice!

alyssawilk · 2021-10-06T15:45:04Z

source/common/network/dns_impl.cc

+                    dns_name_);
+
+    if (!pending_response_.address_list_.empty()) {
+      ASSERT(dns_lookup_family_ == DnsLookupFamily::All && !dual_resolution_);


yeah IMO a resolve success of either v4 or v6 should be considered success.
cc @DavidSchinazi @RyanTheOptimist for informed opinions :-)

alyssawilk · 2021-10-06T15:47:29Z

source/common/network/dns_impl.cc

  }
-  if (!fallback_if_failed_) {
+
+  if (!dual_resolution_) {


hm, are we holding all forward progress on the dns resolver returning v4 and v6 addreses? That might work for a first pass but I believe that the chrome folks have told me it's a bad idea in general for latency / Quality of Experience reasons. If they confirm, I think we need a TODO to handle ipv4 and ipv6 asynchronously.

Yeah I was thinking about this as well from two different angles:

Should both c-ares function calls happen concurrently?

This applies both for c-ares and for apple: should we stream results as they come in to the callback target.

(1) is easy and I could do that in this PR. (2) is a bit harder, and I wouldn't want to do it in this PR.

Personally I would do (1) and not (2) for now.

@alyssawilk @mattklein123 re:(1) do we have a preference for all three modes (All,Auto,V4Preferred) to do the calls concurrently, vs only All doing it concurrently and Auto/V4Preferred only do the second call if the first one fails/returns no addresses?

Hm, less sure on that - I'll defer to Matt and David.
Totally fine with not doing 2 in this PR but I think we should either comment or TODO that it has latency implications and should probably be fixed at some point.

@alyssawilk @mattklein123 I settled on doing both lookups concurrrently for All and leaving Auto and V4Preferred with sequential lookups. I did not want to block this PR on making the necessary changes to enable concurrent lookups for Auto/V4Preferred given that the existing behavior is already sequential, and concurrent lookup for All did not need bigger changes.

I am already part of the way on the TODO, so will put that up as a separate PR.

Agree that waiting for both means users are waiting unnecessarily. But tackling in a separate PR with TODO here sounds good

SG. Do we have a TODO on not waiting for both?

decided to create an issue #18572

This definitely seems like a good way to handle this for now. While suboptimal because it block progres until both return, Chrome does exactly this today. Solving #18572 (later) is a great chance to make envoy mobile better than Chrome!

docs/root/version_history/current.rst

alyssawilk · 2021-10-06T15:55:39Z

oh,I had one other question which is if we're doing the scoping to 5 as disussed on slack, if we should do it here, or where we get the resolution? I have a TODO in my code for where we scope it, but no strong opinion

I'll send mine out in draft form today so you can TAL

junr03 · 2021-10-06T21:41:25Z

I had one other question which is if we're doing the scoping to 5 as disussed on slack, if we should do it here, or where we get the resolution?

My preference is for the callback target to decide what it does with all the addresses that the resolver gives it. That was static dns cluster takes everything, logical takes one, dns cache with happy eyeballs does it thing, etc.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2021-10-07T22:34:39Z

@alyssawilk @mattklein123 updated!

DavidSchinazi · 2021-10-07T23:59:00Z

source/common/network/apple_dns_impl.cc

+    return pending_response_.v4_responses_;
+  case DnsLookupFamily::All:
+    ASSERT(pending_response_.all_responses_.empty());
+    pending_response_.all_responses_.insert(pending_response_.all_responses_.end(),


If I'm understanding this correctly, it's adding v4 addresses before v6 addresses. In general I'd suggest v6 first but you can get fancier as per the happy eyeballs spec

yep, that's the case. But the callback receiver can do what they want with the list, and is where I would expect the ordering complexity to happen; so I am not too opinionated with order here.

I think we should follow the RFC 8305 (the happy eyeballs spec) as David suggest, and I agree with Jose that we should probably do that in HappyEyeballsConnectionImpl. I'd be happy to take a whack at that.

+1 to do this outside.

DavidSchinazi · 2021-10-08T00:00:46Z

source/common/network/dns_impl.cc

+                    dns_name_);
+
+    if (!pending_response_.address_list_.empty()) {
+      ASSERT(dns_lookup_family_ == DnsLookupFamily::All && !dual_resolution_);


+1, success of v4 OR v6 should mean overall success since we have usable addresses

DavidSchinazi · 2021-10-08T00:02:59Z

source/common/network/dns_impl.cc

  }
-  if (!fallback_if_failed_) {
+
+  if (!dual_resolution_) {


Agree that waiting for both means users are waiting unnecessarily. But tackling in a separate PR with TODO here sounds good

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2021-10-08T18:46:59Z

I see the coverage failure. Writing a test for the 1 uncovered line

Signed-off-by: Jose Nino <jnino@lyft.com>

alyssawilk · 2021-10-14T12:48:00Z

(needs main merge)

mattklein123

Awesome, thanks for working on this. A few comments from a read through, thanks.

/wait

mattklein123 · 2021-10-14T16:35:16Z

source/common/network/apple_dns_impl.cc

+    return pending_response_.v4_responses_;
+  case DnsLookupFamily::All:
+    ASSERT(pending_response_.all_responses_.empty());
+    pending_response_.all_responses_.insert(pending_response_.all_responses_.end(),


+1 to do this outside.

mattklein123 · 2021-10-14T16:38:58Z

source/common/network/dns_impl.h

+    // or Auto, perform a second resolution if the first one fails. If dns_lookup_family_ is All
+    // perform the second resolution whether the first one fails or succeeds.


Is this comment out of date? It conflicts with the one above about concurrent? The behavior is that both are concurrent, but we don't raise the final callback until both are done? Or is that not correct? (From further reading I'm pretty sure it's correct but I would do a 2nd comment pass as the logic is actually pretty tricky and I would be concerned we might be missing some corner cases.

mattklein123 · 2021-10-14T16:41:52Z

source/common/network/dns_impl.cc


 void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, int timeouts,
                                                                   ares_addrinfo* addrinfo) {
+  // Purely for checking for invalid state. This will change is Auto and V4Preferred are changed to


Suggested change

// Purely for checking for invalid state. This will change is Auto and V4Preferred are changed to

// Purely for checking for invalid state. This will change if Auto and V4Preferred are changed to

?

mattklein123 · 2021-10-14T16:49:21Z

source/common/network/dns_impl.cc

+    // The only way the resolver would have addresses here is if it is configured to resolve All
+    // families, and the first resolution was successful. In that case, we might as well return
+    // the addresses that resolved.
+    ASSERT(dns_lookup_family_ == DnsLookupFamily::All && !dual_resolution_);


nit: this seems a bit fragile. Should you change status to ARES_SUCCESS in this case? Do we have test coverage for this case?

mattklein123 · 2021-10-14T16:50:34Z

source/common/network/dns_impl.cc

+    if (pending_response_.address_list_.empty()) {
+      pending_response_.status_ = ResolutionStatus::Failure;


Is this true? Couldn't the list actually be empty? Or is this due to the new concurrent behavior and the 2nd one failing? Maybe this is related to my comment above about switching status? Can you add more comments?

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2021-10-19T18:21:20Z

@mattklein123 Thanks for the comments! I needed to rearrange a piece of code because there was not way to get coverage i.e., I couldn't force c-ares to behave in a way to get us on that path. At first glance I think I covered the new arrangement, but will make sure to double check.

At the very least I will add more comments in order to leave everything a bit more clear. I will update when I am back 10/26.

cc @alyssawilk

junr03 · 2021-10-19T18:21:30Z

/wait

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only · 2021-10-27T18:02:51Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18464 was synchronize by junr03.

see: more, trace.

junr03 · 2021-10-27T18:07:56Z

Alright, @mattklein123 when I came back to this with fresh eyes I realized that constructing the pending response as a failure and only changing to success when the c-ares callback gave us an ARES_SUCCESS made the code simpler. All the cases I can think about are covered with the existing and added test cases.

Let me know if this is better!

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

The DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback is a lot more complicated than I would like and pretty hard to follow, so if you see any further opportunities to make that easier to understand that would be a nice follow up. Otherwise LGTM after staring at this for a while. Thanks for adding!

junr03 · 2021-10-27T21:22:28Z

The DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback is a lot more complicated than I would like and pretty hard to follow

Yeah it has never been the easiest to follow. I think this PR is a good first step by taking out finishResolve. I can take another refactoring pass in a follow up.

* main: (221 commits) deps: Bump `protobuf` -> 3.19.0 (envoyproxy#18471) tooling: auto-assign dependency shephards (envoyproxy#18794) clang-tidy: Return from diff fun if empty diff (envoyproxy#18815) repokitteh: Block PRs pending deps approval (envoyproxy#18814) deps: Bump `org_llvm_llvm` -> 12.0.1, `com_github_wavm_wavm` -> 9ffd3e2 (envoyproxy#18747) dns resolvers: add All lookup mode (envoyproxy#18464) doc: fix link formatting for TLS session_timeout (envoyproxy#18790) ext_authz: Set response flag and code details to UAEX when denied (envoyproxy#18740) socket options: add support for directly creating ipv4/ipv6 pairs (envoyproxy#18769) ecds: make onConfigUpdate generic over filter type (envoyproxy#18061) bazel: update CMake instructions in EXTERNAL_DEPS.md (envoyproxy#18799) upstream: fix typo in comment (envoyproxy#18798) runtime: removing envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits (envoyproxy#18696) bazel: Add CC=clang to clang configuration (envoyproxy#18732) fix error request id in the dubbbo local reply (envoyproxy#18741) event: assert the case of both read and closed event registered (envoyproxy#18265) tcp proxy connect tunneling: improved testing (envoyproxy#18784) deps: Bump `protoc-gen-validate` -> 0.6.2 (envoyproxy#18742) deps: Bump `rules_pkg` -> ad57589 (envoyproxy#18746) bazel: copy .bazelversion for envoy filter examples (envoyproxy#18730) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Jose Nino added 2 commits October 5, 2021 14:39

dns resolvers: add All lookup mode

c0b7ccf

Signed-off-by: Jose Nino <jnino@lyft.com>

update

51a3419

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot added the api label Oct 5, 2021

repokitteh-read-only bot assigned adisuissa Oct 5, 2021

junr03 assigned alyssawilk and unassigned adisuissa Oct 5, 2021

junr03 commented Oct 5, 2021

View reviewed changes

Jose Nino added 2 commits October 5, 2021 15:31

update

2272329

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'main' into dns-all-resolution-mode

386c2cb

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 force-pushed the dns-all-resolution-mode branch from 2344225 to 386c2cb Compare October 5, 2021 22:33

fix docs

8e29fbe

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 mentioned this pull request Oct 5, 2021

dns resolvers: add more details to debug logging #18413

Closed

alyssawilk assigned DavidSchinazi Oct 6, 2021

alyssawilk reviewed Oct 6, 2021

View reviewed changes

Jose Nino added 4 commits October 6, 2021 14:45

release notes

9472b9e

Signed-off-by: Jose Nino <jnino@lyft.com>

only all concurrent

23917d0

Signed-off-by: Jose Nino <jnino@lyft.com>

todo

d41d879

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'main' into dns-all-resolution-mode

b51e69c

Signed-off-by: Jose Nino <jnino@lyft.com>

DavidSchinazi reviewed Oct 8, 2021

View reviewed changes

Jose Nino added 4 commits October 8, 2021 09:03

fix from silly merge

4b99dc5

Signed-off-by: Jose Nino <jnino@lyft.com>

fix from silly merge

66f204a

Signed-off-by: Jose Nino <jnino@lyft.com>

green

db288e6

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

a11ff20

Signed-off-by: Jose Nino <jnino@lyft.com>

rejigger for coverage

8411c1c

Signed-off-by: Jose Nino <jnino@lyft.com>

alyssawilk previously approved these changes Oct 13, 2021

View reviewed changes

alyssawilk assigned mattklein123 Oct 13, 2021

mattklein123 requested changes Oct 14, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 14, 2021

Jose Nino added 3 commits October 19, 2021 13:10

Merge branch 'main' into dns-all-resolution-mode

a8aa2ad

Signed-off-by: Jose Nino <jnino@lyft.com>

spelling

368cee4

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

52d1a88

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 dismissed stale reviews from alyssawilk and RyanTheOptimist via 52d1a88 October 19, 2021 18:20

junr03 requested a review from yanavlasov as a code owner October 19, 2021 18:20

repokitteh-read-only bot added api and removed waiting labels Oct 19, 2021

repokitteh-read-only bot added the waiting label Oct 19, 2021

Jose Nino added 2 commits October 27, 2021 11:00

update

a718d06

Signed-off-by: Jose Nino <jnino@lyft.com>

update

8b7ad1a

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting label Oct 27, 2021

Jose Nino added 2 commits October 27, 2021 11:21

alpha

4c2ecd6

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

fc45363

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 approved these changes Oct 27, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Oct 27, 2021

mattklein123 merged commit 036ae75 into main Oct 28, 2021

mattklein123 deleted the dns-all-resolution-mode branch October 28, 2021 15:59

moderation mentioned this pull request Nov 15, 2021

upstream: Implement Happy Eyeballs address sorting #18906

Merged

		@@ -104,16 +104,24 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i
		// ARES_ECONNREFUSED. If the PendingResolution has not been cancelled that means that the

		// or Auto, perform a second resolution if the first one fails. If dns_lookup_family_ is All
		// perform the second resolution whether the first one fails or succeeds.

	// Purely for checking for invalid state. This will change is Auto and V4Preferred are changed to
	// Purely for checking for invalid state. This will change if Auto and V4Preferred are changed to

		if (pending_response_.address_list_.empty()) {
		pending_response_.status_ = ResolutionStatus::Failure;

Conversation

junr03 commented Oct 5, 2021

Uh oh!

repokitteh-read-only bot commented Oct 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk commented Oct 6, 2021

Uh oh!

junr03 commented Oct 6, 2021

Uh oh!

junr03 commented Oct 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Oct 8, 2021

Uh oh!

alyssawilk commented Oct 14, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Oct 19, 2021

Uh oh!

junr03 Oct 7, 2021 •

edited

Loading