[EventEngine] PosixEventEngine DNS Resolver #32701

yijiem · 2023-03-24T04:42:56Z

This PR implements a c-ares based DNS resolver for EventEngine with the reference from the original grpc_ares_wrapper.h. The PosixEventEngine DNSResolver is implemented on top of that. Tests which use the client channel resolver API (resolver.h) are ported, namely the resolver_component_test.cc and the cancel_ares_query_test.cc. The WindowsEventEngine DNSResolver will use the same EventEngine's grpc_ares_wrapper and will be worked on next.

The resolve_address_test.cc which uses the iomgr DNSResolver API has been ported to EventEngine's dns_test.cc. That leaves only 2 tests which use iomgr's API, notably the dns_resolver_cooldown_test.cc and the goaway_server_test.cc which probably need to be restructured to use EventEngine DNSResolver (for one thing they override the original grpc_ares_wrapper's free functions). I will try to tackle these in the next step.

markdroth

This looks really good! Remaining comments are mostly minor.

Please let me know if you have any questions. Thanks!

src/core/lib/event_engine/posix_engine/posix_engine.cc

src/core/lib/event_engine/ares_resolver.h

src/core/lib/event_engine/ares_resolver.cc

test/core/event_engine/test_suite/tests/dns_test.cc

markdroth · 2023-06-30T20:47:16Z

test/core/event_engine/test_suite/tests/dns_test.cc

+             kHealthCheckRecordName},
+            " ");
+        // TODO(yijiem): build a portable solution for Windows
+        FILE* f = popen(command.c_str(), "r");


I feel like we might still be duplicating some stuff unnecessarily here, but I might be wrong, and I don't have time to really dig in to understand the details. I'm going to defer to @apolcyn for review of the tests here, with particular focus on making sure that we are not needlessly reinventing any wheels.

drfloob

Looks good, I found just a few small things to address.

drfloob · 2023-06-30T18:46:56Z

src/core/lib/event_engine/ares_resolver.cc

+#include "src/core/lib/iomgr/sockaddr.h"
+#ifdef GRPC_POSIX_SOCKET_ARES_EV_DRIVER
+#include "src/core/lib/event_engine/posix_engine/tcp_socket_utils.h"
+#elif defined(GRPC_WINDOWS_SOCKET_ARES_EV_DRIVER)


Nit: maybe add a // TODO(yijiem) here for the next step, so the empty block doesn't look like a mistake.

Actually the #elif here doesn't really make sense. We would include something here for Windows when porting the Windows' version of IsIpv6LoopbackAvailable() from iomgr to EE. But there is no need to reserve it here now.

drfloob · 2023-06-30T18:48:56Z

src/core/lib/event_engine/ares_resolver.cc

+namespace grpc_event_engine {
+namespace experimental {
+
+grpc_core::TraceFlag grpc_trace_ares_resolver(false,


Nit: WDYT about just reusing cares_resolver for the trace flag now that we're landing it in production? Since one implementation will be replaced by the other, and no process will be running both ares versions concurrently, it might make sense to not introduce a separate trace flag.

Makes sense. Done.

drfloob · 2023-06-30T20:57:51Z

src/core/lib/event_engine/ares_resolver.cc

+  // Don't query for TXT records if the target is "localhost"
+  if (absl::EqualsIgnoreCase(host, "localhost")) {
+    event_engine_->Run([callback = std::move(callback)]() mutable {
+      callback(std::vector<std::string>());


Can this just be callback({});?

It's interesting, callback takes a absl::StatusOr which has an explicit default constructor which prohibits list initialization: https://source.corp.google.com/piper///depot/google3/third_party/absl/status/statusor.h;l=216-221?q=file:third_party%2Fabsl%20absl::StatusOr&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental

drfloob · 2023-06-30T21:00:10Z

src/core/lib/event_engine/ares_resolver.cc

+            fd_node_list_.begin(), fd_node_list_.end(),
+            [sock = socks[i]](const auto& node) { return node->as == sock; });
+        if (iter == fd_node_list_.end()) {
+          new_list.emplace_back(std::make_unique<FdNode>(


nit: since this is an std::list<std::unique_ptr<FdNode>>, you don't need emplace here, push_bach is preferred. I think we have linting for that internally, so you may get that same recommendation on import.

Done.

BTW love the push_bach since JSBach is my favorite musician :).

drfloob · 2023-06-30T21:08:20Z

src/core/lib/event_engine/posix_engine/posix_engine.cc

@@ -487,9 +490,62 @@ EventEngine::TaskHandle PosixEventEngine::RunAfterInternal(
  return handle;
 }

+#if GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET_TCP)


Note for the future - something you already know, but worth commenting - that this engine is expected to run on platforms that don't have Ares enabled (Android is the only supported platform I'm aware of anymore if iOS works with Ares, but possibly other platforms will use it well). Before the EE dns functionality can be enabled generally, a basic A/AAAA-only native resolver needs to be implemented as well.

It's probably worth having a TODO for that here or below next to the Crash("unimplemented") bit. If I missed a TODO for this somewhere already, apologies.

Good point! Added a TODO below next to the Crash("unimplemented").

drfloob · 2023-06-30T21:15:13Z

src/core/lib/event_engine/posix_engine/posix_engine.h

@@ -138,14 +140,23 @@ class PosixEventEngine final : public PosixEventEngineWithFdSupport,
 public:
  class PosixDNSResolver : public EventEngine::DNSResolver {
   public:
-    ~PosixDNSResolver() override;
+#if GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET_TCP)


On platforms that don't support ares, this class will have an auto-generated public default constructor. Making a private, no-op default constructor, either in non-ares builds or just always having it, would prevent this from being instantiated on platforms that don't support ares. It's a bit of defensive programming, but I think it conveys the intention a bit better at compile time.

Good point! I explicitly deleted the default constructor which should convey the same meaning here.

HannahShiSFB · 2023-07-03T21:29:45Z

@drfloob @apolcyn @markdroth, continue the discussion on the concerns in using c-ares on iOS:

I'm drafting an implementation with DNSService API (#33233).

As I mentioned earlier, this API doesn't support specifying custom DNS servers but can only use which ever servers configured in the system. I took a look at the iomgr implementation, looks like name server is not actually used.

So I wonder what's the use case of specifying a different dns server in event engine? is it OK for iOS I always use the system DNS?

The other thing that's not supported is using a custom timeout, given the new DNS API removed the deadline parameter, I assume it's OK to rely on system default timeout (5s in my test)?

CC: @sampajano

drfloob · 2023-07-10T22:40:20Z

Sorry for the delay, I just returned from vacation.

So I wonder what's the use case of specifying a different dns server in event engine? is it OK for iOS I always use the system DNS?

It is OK for gRPC on iOS to always use the built-in DNS system. Customizing the DNS authority is an advanced feature which has never been supported on iOS.

The other thing that's not supported is using a custom timeout, given the new DNS API removed the deadline parameter, I assume it's OK to rely on system default timeout (5s in my test)?

I think this is OK too. I'm not aware of any requests for advanced DNS resolution capabilities on iOS, so the default timeout should also be fine. The same is true for Android, to the best of my knowledge.

markdroth · 2023-07-18T17:55:06Z

@drfloob @HannahShiSFB

So I wonder what's the use case of specifying a different dns server in event engine? is it OK for iOS I always use the system DNS?

It is OK for gRPC on iOS to always use the built-in DNS system. Customizing the DNS authority is an advanced feature which has never been supported on iOS.

I'm fine with iOS not supporting custom DNS servers. But we need to make sure that if a user provides a URI with a custom DNS server, we do not just silently ignore the custom DNS server; we need to reject such a URI if we cannot honor it.

I think this means that the EE impl should return a failure when creating the DNSResolver object if the caller passes in a custom DNS server.

drfloob · 2023-07-18T18:13:40Z

@markdroth @HannahShiSFB

I think this means that the EE impl should return a failure when creating the DNSResolver object if the caller passes in a custom DNS server.

I agree. @yijiem can you add a bit to the documentation in your API update here: #33744. Something like "If the caller requests a custom DNS resolver, and the EventEngine implementation does not support it, this must return an error."

markdroth

This looks really great! The only blocking issue is merging the changes from #33744 into this PR.

src/core/lib/event_engine/ares_resolver.cc

src/core/lib/event_engine/posix_engine/posix_engine.cc

markdroth

This looks great!

src/core/lib/event_engine/ares_resolver.cc

drfloob

Excellent work, Yijie!

This PR implements a c-ares based DNS resolver for EventEngine with the reference from the original [grpc_ares_wrapper.h](../blob/master/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h). The PosixEventEngine DNSResolver is implemented on top of that. Tests which use the client channel resolver API ([resolver.h](../blob/master/src/core/lib/resolver/resolver.h#L54)) are ported, namely the [resolver_component_test.cc](../blob/master/test/cpp/naming/resolver_component_test.cc) and the [cancel_ares_query_test.cc](../blob/master/test/cpp/naming/cancel_ares_query_test.cc). The WindowsEventEngine DNSResolver will use the same EventEngine's grpc_ares_wrapper and will be worked on next. The [resolve_address_test.cc](https://github.com/grpc/grpc/blob/master/test/core/iomgr/resolve_address_test.cc) which uses the iomgr [DNSResolver](../blob/master/src/core/lib/iomgr/resolve_address.h#L44) API has been ported to EventEngine's dns_test.cc. That leaves only 2 tests which use iomgr's API, notably the [dns_resolver_cooldown_test.cc](../blob/master/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc) and the [goaway_server_test.cc](../blob/master/test/core/end2end/goaway_server_test.cc) which probably need to be restructured to use EventEngine DNSResolver (for one thing they override the original grpc_ares_wrapper's free functions). I will try to tackle these in the next step.

yijiem added 30 commits February 1, 2023 11:50

typo

16fa787

Merge remote-tracking branch 'upstream/master' into ee_cares

f9bd595

use python3

ebab213

stash

abc292f

for iwyu

44f179e

stash

900515f

stash

85b126c

stash

0fa09f5

stash

9194ffa

compiled??????

eff648d

iwyu

a217049

start testing.... from segmentation fault!

c6d86d4

running to hanging...

9b70b1c

GET www.google.com:80 works

f433a42

store

1fccee6

store

02c3c9d

store

561f526

add ares.(cc|h)

3032bb0

store

ae291a1

ares.(cc|h)

316ca9f

iwyu

7bd7915

store

689d4f9

refcount seems to work

ad542df

start dealing with cancel (orphan)

8ba7b6c

Cancel seems to work??

2d42a20

comment

e0b53fa

start GrpcAresSRVRequest and GrpcAresTXTRequest

3812b3c

rename ares.* to ares_driver.*

6254eab

mv grpc_core::SplitHostPort() logic into GrpcAresRequest

75014f1

ready to implement SRV and TXT request?

4096a2f

yijiem added 2 commits June 30, 2023 11:21

sanitize

62835f1

port_s -> port_string

d1fbaeb

markdroth reviewed Jun 30, 2023

View reviewed changes

drfloob reviewed Jun 30, 2023

View reviewed changes

review

b46f8be

yijiem requested review from drfloob, apolcyn and markdroth July 7, 2023 18:40

sanitize

b7a83c3

markdroth reviewed Jul 18, 2023

View reviewed changes

src/core/lib/event_engine/ares_resolver.cc Outdated Show resolved Hide resolved

src/core/lib/event_engine/posix_engine/posix_engine.cc Outdated Show resolved Hide resolved

yijiem added 3 commits July 19, 2023 20:34

Merge remote-tracking branch 'upstream/master' into ee_cares

838493e

fix test

c800a16

review

8743377

markdroth approved these changes Jul 20, 2023

View reviewed changes

src/core/lib/event_engine/ares_resolver.cc Outdated Show resolved Hide resolved

yijiem added the disposition/Needs Internal Changes label Jul 20, 2023

Merge remote-tracking branch 'upstream/master' into ee_cares

6d43655

drfloob approved these changes Jul 20, 2023

View reviewed changes

yijiem added 5 commits July 20, 2023 10:24

Merge remote-tracking branch 'upstream/master' into ee_cares

81d5690

Merge remote-tracking branch 'upstream/master' into ee_cares

638fd3f

fix

c6ea5a2

Merge remote-tracking branch 'upstream/master' into ee_cares

ee12043

Merge remote-tracking branch 'upstream/master' into ee_cares

64b5bdf

yijiem removed the disposition/Needs Internal Changes label Jul 21, 2023

yijiem merged commit a7bf07e into grpc:master Jul 21, 2023
64 of 65 checks passed

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Jul 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EventEngine] PosixEventEngine DNS Resolver #32701

[EventEngine] PosixEventEngine DNS Resolver #32701

yijiem commented Mar 24, 2023 •

edited

markdroth left a comment

markdroth Jun 30, 2023

drfloob left a comment

drfloob Jun 30, 2023

yijiem Jul 6, 2023

drfloob Jun 30, 2023

yijiem Jul 6, 2023

drfloob Jun 30, 2023

yijiem Jul 6, 2023

drfloob Jun 30, 2023

yijiem Jul 6, 2023

drfloob Jun 30, 2023

yijiem Jul 7, 2023

drfloob Jun 30, 2023

yijiem Jul 7, 2023

HannahShiSFB commented Jul 3, 2023 •

edited

drfloob commented Jul 10, 2023

markdroth commented Jul 18, 2023

drfloob commented Jul 18, 2023

markdroth left a comment

markdroth left a comment

drfloob left a comment

[EventEngine] PosixEventEngine DNS Resolver #32701

[EventEngine] PosixEventEngine DNS Resolver #32701

Conversation

yijiem commented Mar 24, 2023 • edited

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drfloob left a 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

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

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

HannahShiSFB commented Jul 3, 2023 • edited

drfloob commented Jul 10, 2023

markdroth commented Jul 18, 2023

drfloob commented Jul 18, 2023

markdroth left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

drfloob left a comment

Choose a reason for hiding this comment

yijiem commented Mar 24, 2023 •

edited

HannahShiSFB commented Jul 3, 2023 •

edited