dns: add dns resolver implementation based on Apple APIs by junr03 · Pull Request #13074 · envoyproxy/envoy

junr03 · 2020-09-13T18:12:07Z

Commit Message: adding DNS resolver based on Apple's DNSServiceGetAddrInfo functionality.
Additional Description: This is particularly required in builds of Envoy that run on iOS devices >version 14. Apple added new local network permissions that are triggered with DNS resolution over the local DNS server on WiFi networks. The Apple API is excluded from the new permissions.
Risk Level: high for users of the new resolver, as the resolver does not have the amount of production as the c-ares based implementation. However, this is fully opt-in and only available on Apple platforms.
Testing: unit tests that run on mac ci
Release Notes: added

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

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

junr03 · 2020-09-13T18:24:41Z

@goaway @mattklein123 alright, I have battle tested this a bit with envoy mobile and different network conditions (although need to talk to @sberrevoets about testing on a iOS14 device tomorrow. I also ran the (majority) of dns_impl_test suite and it looks good.

I wanted your eyes on this as early as possible, so I think this is ready for a draft stage review. I have left some POINTS FOR DISCUSSION as inline comments, would like to hear your thoughts.

I'll, obviously, add a test suite once we run through first comments. It will look very close to dns_impl_test. I also will add the runtime usage and selective compilation after first comments.

cc @htuch @Reflejo -- given you all were involved in the original writing of the c-ares implementation, which this borrows from in spirit.

source/common/network/apple_dns_impl.h

mattklein123

Thanks for getting this out so quickly. Flushing out some comments from the first pass. Let me know when you want me to take a look again. Thank you!

/wait

source/common/network/apple_dns_impl.cc

source/common/network/apple_dns_impl.h

source/common/network/apple_dns_impl.cc

junr03 · 2020-09-14T19:26:03Z

@mattklein123 @goaway thanks for the initial pass, will push update ASAP

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

junr03 · 2020-09-14T22:09:47Z

@goaway @mattklein123 updated!

I added plentiful comments. I agree with both of you that there are some non-intuitive aspects of the Apple API. Let me know if I went a little overboard.

source/common/network/apple_dns_impl.cc

mattklein123

Thanks this LGTM at a high level. Dropped a few more comments. Let me know when ready for a full review!

/wait

source/common/network/apple_dns_impl.cc

mattklein123 · 2020-09-15T00:05:56Z

source/common/network/apple_dns_impl.cc

+  // This implementation uses a shared connection for two main reasons:
+  //    1. Efficiency of concurrent resolutions by sharing the same underlying UDS to the DNS
+  //       daemon.
+  //    2. An error on a connection to the daemon is good indication that other connection,
+  //       even if not shared, would not succeed. So it is better to share one connection and
+  //       promptly cancel all outstanding queries, rather than individually wait for all
+  //       connections to error out.
+  // However, using a shared connection brings some complexities detailed in the inline comments
+  // for kDNSServiceFlagsShareConnection in dns_sd.h, and copied (and edited) in this implementation
+  // where relevant.


I don't feel strongly about it, but is this a premature optimization? Should we just do a connection per resolution for now and keep it simple?

I ultimately decided to leave it. I think the reasons in the inline comment are strong enough to leave it. Mostly around "failing fast" in mobile environments were instead of potentially waiting for independent timeouts that may stagger, all outstanding queries will fail.

source/common/network/apple_dns_impl.cc

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

htuch · 2020-09-29T20:15:28Z

source/common/event/dispatcher_impl.cc

+  static bool use_apple_api_for_dns_lookups =
+      Runtime::runtimeFeatureEnabled("envoy.restart_features.use_apple_api_for_dns_lookups");
+  if (use_apple_api_for_dns_lookups) {
+    RELEASE_ASSERT(


Shouldn't we be rejecting the config earlier in a more graceful way? If so, shouldn't these be ASSERTs?

Yes, this should be exceptions that fail during config load.

It all comes down to snapping the runtime once by making it static local state here vs checking earlier in callers of this function.

I guess I could change this function to return nullptr on failure, and have callers deal with the return value?

I believe the DNS resolver is created during config load time in all cases. I think you can just throw an exception here.

I think that's the case for all xDS, assuming this also holds for dynamic forward proxy, SG.

It holds for the dynamic proxy. Let me check in the UDP filter. If that's the case, I'll update with an exception.

@htuch @mattklein123 unfortunately, the UDP DNS filter creates a resolver (underneath its own wrapper) for every filter instance construction:

envoy/source/extensions/filters/udp/dns_filter/dns_filter.cc

Line 229 in 3b5acb2

resolver_ = std::make_unique<DnsFilterResolver>(resolver_callback_, config->resolvers(),

So can't throw an exception as is (and per @htuch the assert is not great either).

The underlying resolver in the UDP DNS filter should definitely be shared, and I can work to untangle. However, I don't want to block this PR too much (we need it to be able to run envoy mobile on ios 14 devices). How do you feel about landing this PR with a warn log in place of the assert. Then I can work on untangling the filter situation, and subsequently upgrading the warn log here into an exception?

I would actually rather you just leave the RELEASE_ASSERT and make sure the error message is good so the OSX user can flip the runtime flag, then come back and clean it up. So just leave a TODO for that? @htuch?

htuch · 2020-09-29T20:23:08Z

source/common/network/apple_dns_impl.cc

+            "error_code={}, hostname={}",
+            dns_name_, flags, flags & kDNSServiceFlagsMoreComing ? "yes" : "no",
+            flags & kDNSServiceFlagsAdd ? "yes" : "no", interface_index, error_code, hostname);
+  ASSERT(interface_index == 0);


How certain are you of some of these ASSERTs on state that comes from the NS libs?

As certain as I can be from what they write in the comments in dns_sd.h 😅

htuch · 2020-09-29T20:24:55Z

source/common/network/apple_dns_impl.cc

@@ -0,0 +1,325 @@
+#include "common/network/apple_dns_impl.h"


Sorry, just got around to looking into the details of this PR now. One question I have; what is the threading model here? Who controls the callbacks, which threads do they land on? Maybe a top-level comment explaining this would help. Thanks.

https://github.com/envoyproxy/envoy/pull/13074/files#diff-4ebdef3732b2b07e175aba41bcaf62abR280

junr03 · 2020-09-29T20:35:54Z

@htuch answered two of your questions, and I'd like to hear your opinion on the third.

htuch · 2020-09-29T20:36:16Z

source/common/network/apple_dns_impl.cc

+      &individual_sd_ref_, kDNSServiceFlagsShareConnection | kDNSServiceFlagsTimeout, 0, protocol,
+      dns_name_.c_str(),
+      /*
+       * About Thread Safety (taken from inline documentation there):


I'd push this to the top of the file. It's one of the most important thing to have in mind when reading the code.

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

mattklein123

LGTM!

mattklein123 · 2020-09-30T03:56:34Z

Oops looks like legit CI failure.

/wait

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

junr03 · 2020-09-30T14:04:24Z

alright, now it's all green. Thanks for the reviews @mattklein123!

Updating to pull in envoyproxy/envoy#13074 Signed-off-by: Michael Rebello <me@michaelrebello.com>

Updating to pull in envoyproxy/envoy#13074 which will use Apple's DNS resolver for iOS in order to allow Envoy Mobile to ship on iOS 14 without triggering the "local area network" modal. Signed-off-by: Michael Rebello <me@michaelrebello.com>

yanjunxiang-google · 2021-08-20T17:22:02Z

test/common/network/BUILD

+        "//test/mocks/runtime:runtime_mocks",
+        "//test/mocks/thread_local:thread_local_mocks",
+    ] + select({
+        "//bazel:apple": ["//source/common/network:dns_lib"],


Should this be: "//bazel:apple": ["//source/common/network:apple_dns_lib"],

Updating to pull in #13074 which will use Apple's DNS resolver for iOS in order to allow Envoy Mobile to ship on iOS 14 without triggering the "local area network" modal. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

Jose Nino added 5 commits September 10, 2020 18:23

earliest rough draft

6b72142

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

Merge branch 'master' into apple-dns

afb5731

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

done

3f6adce

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

additional comments

8e3e44e

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

another error

204f3dd

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

junr03 changed the title ~~Apple dns~~ dns: add dns resolver implementation based on Apple APIs Sep 13, 2020

Jose Nino added 2 commits September 13, 2020 11:13

added by accident

b5ecbdb

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

fmt

6fb5a14

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

junr03 assigned goaway and mattklein123 Sep 13, 2020

goaway reviewed Sep 14, 2020

View reviewed changes

source/common/network/apple_dns_impl.h Outdated Show resolved Hide resolved

mattklein123 requested changes Sep 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 14, 2020

goaway reviewed Sep 14, 2020

View reviewed changes

source/common/network/apple_dns_impl.cc Outdated Show resolved Hide resolved

comments

3ecd18a

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

repokitteh-read-only bot removed the waiting label Sep 14, 2020

htuch reviewed Sep 14, 2020

View reviewed changes

source/common/network/apple_dns_impl.cc Outdated Show resolved Hide resolved

mattklein123 requested changes Sep 15, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 15, 2020

Jose Nino added 3 commits September 18, 2020 14:59

comments

1c972cc

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

Merge branch 'master' into apple-dns

d90ab49

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

WIP tests

09c8946

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

repokitteh-read-only bot removed the waiting label Sep 22, 2020

Jose Nino added 4 commits September 21, 2020 19:55

WIP tests

875458a

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

just run mac

44a1ce6

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

WIP tests

25cc48f

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

less expectations

2797de8

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

htuch reviewed Sep 29, 2020

View reviewed changes

mattklein123 added the waiting label Sep 29, 2020

htuch reviewed Sep 29, 2020

View reviewed changes

verbose assert message

40b40e4

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

junr03 dismissed goaway’s stale review via 40b40e4 September 30, 2020 00:39

repokitteh-read-only bot removed the waiting label Sep 30, 2020

mattklein123 previously approved these changes Sep 30, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 30, 2020

repokitteh-read-only bot added the waiting label Sep 30, 2020

switch

80fb3f7

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

junr03 dismissed mattklein123’s stale review via 80fb3f7 September 30, 2020 05:54

repokitteh-read-only bot added api and removed waiting labels Sep 30, 2020

mattklein123 approved these changes Sep 30, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 30, 2020

mattklein123 merged commit d3ecdef into envoyproxy:master Sep 30, 2020

rebello95 added a commit to envoyproxy/envoy-mobile that referenced this pull request Sep 30, 2020

upstream: update with apple dns resolver

eef91d1

Updating to pull in envoyproxy/envoy#13074 Signed-off-by: Michael Rebello <me@michaelrebello.com>

rebello95 mentioned this pull request Sep 30, 2020

upstream: update with apple dns resolver envoyproxy/envoy-mobile#1123

Merged

junr03 mentioned this pull request Oct 12, 2020

DNS config: validate mutually exclusive options when loading dns config. #13512

Open

yanjunxiang-google reviewed Aug 20, 2021

View reviewed changes

yanjunxiang-google mentioned this pull request Oct 4, 2021

Refactoring Envoy DNS resolution as extension #17479

Merged

Conversation

junr03 commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junr03 commented Sep 13, 2020

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junr03 commented Sep 14, 2020

Uh oh!

junr03 commented Sep 14, 2020

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 29, 2020 • 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!

junr03 commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 30, 2020

Uh oh!

junr03 commented Sep 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

junr03 commented Sep 13, 2020 •

edited

Loading

mattklein123 Sep 29, 2020 •

edited

Loading