From 7df0e11755d6eb8a91cf7b60eefda9021b357287 Mon Sep 17 00:00:00 2001 From: Yijie Ma Date: Mon, 8 May 2023 11:12:29 -0700 Subject: [PATCH] [EventEngine] Change TXT lookup result type to std::vector (#33030) One TXT lookup query can return multiple TXT records (see the following example). `EventEngine::DNSResolver` should return all of them to let the caller (e.g. `event_engine_client_channel_resolver`) decide which one they would use. ``` $ dig TXT wikipedia.org ; <<>> DiG 9.18.12-1+build1-Debian <<>> TXT wikipedia.org ;; global options: +cmd ;; Got answer: ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 49626 ;; flags: qr rd ra; QUERY: 1, ANSWER: 3, AUTHORITY: 0, ADDITIONAL: 1 ;; OPT PSEUDOSECTION: ; EDNS: version: 0, flags:; udp: 512 ;; QUESTION SECTION: ;wikipedia.org. IN TXT ;; ANSWER SECTION: wikipedia.org. 600 IN TXT "google-site-verification=AMHkgs-4ViEvIJf5znZle-BSE2EPNFqM1nDJGRyn2qk" wikipedia.org. 600 IN TXT "yandex-verification: 35c08d23099dc863" wikipedia.org. 600 IN TXT "v=spf1 include:wikimedia.org ~all" ``` Note that this change also deviates us from the iomgr's DNSResolver API which uses std::string as the result type. --- include/grpc/event_engine/event_engine.h | 2 +- .../event_engine_client_channel_resolver.cc | 23 +++++++++++++++---- .../thready_event_engine.cc | 2 +- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/include/grpc/event_engine/event_engine.h b/include/grpc/event_engine/event_engine.h index f9c9dadfd76..1cfe1d3e346 100644 --- a/include/grpc/event_engine/event_engine.h +++ b/include/grpc/event_engine/event_engine.h @@ -354,7 +354,7 @@ class EventEngine : public std::enable_shared_from_this { absl::AnyInvocable>)>; /// Called with the result of a TXT record lookup using LookupTXTCallback = - absl::AnyInvocable)>; + absl::AnyInvocable>)>; virtual ~DNSResolver() = default; diff --git a/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc b/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc index 78bb58438bb..33ef066eda4 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/event_engine/event_engine_client_channel_resolver.cc @@ -31,6 +31,7 @@ #include "absl/container/flat_hash_set.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/strip.h" #include "absl/types/optional.h" @@ -128,7 +129,7 @@ class EventEngineClientChannelDNSResolver : public PollingResolver { void OnBalancerHostnamesResolved( std::string authority, absl::StatusOr> addresses); - void OnTXTResolved(absl::StatusOr service_config); + void OnTXTResolved(absl::StatusOr> service_config); // Returns a Result if resolution is complete. // callers must release the lock and call OnRequestComplete if a Result is // returned. This is because OnRequestComplete may Orphan the resolver, @@ -251,7 +252,7 @@ EventEngineClientChannelDNSResolver::EventEngineDNSRequestWrapper:: resolver_.get(), resolver_->name_to_resolve().c_str()); txt_handle_ = event_engine_resolver_->LookupTXT( [self = Ref(DEBUG_LOCATION, "OnTXTResolved")]( - absl::StatusOr service_config) { + absl::StatusOr> service_config) { self->OnTXTResolved(std::move(service_config)); }, absl::StrCat("_grpc_config.", resolver_->name_to_resolve()), @@ -388,7 +389,7 @@ void EventEngineClientChannelDNSResolver::EventEngineDNSRequestWrapper:: } void EventEngineClientChannelDNSResolver::EventEngineDNSRequestWrapper:: - OnTXTResolved(absl::StatusOr service_config) { + OnTXTResolved(absl::StatusOr> service_config) { ValidationErrors::ScopedField field(&errors_, "txt lookup"); absl::optional result; { @@ -400,7 +401,21 @@ void EventEngineClientChannelDNSResolver::EventEngineDNSRequestWrapper:: errors_.AddError(service_config.status().message()); service_config_json_ = service_config.status(); } else { - service_config_json_ = absl::StrCat("grpc_config=", *service_config); + constexpr char kServiceConfigAttributePrefix[] = "grpc_config="; + auto result = std::find_if(service_config->begin(), service_config->end(), + [&](absl::string_view s) { + return absl::StartsWith( + s, kServiceConfigAttributePrefix); + }); + if (result != service_config->end()) { + service_config_json_ = + result->substr(sizeof(kServiceConfigAttributePrefix)); + } else { + service_config_json_ = absl::UnavailableError(absl::StrCat( + "failed to find attribute prefix: ", kServiceConfigAttributePrefix, + " in TXT records")); + errors_.AddError(service_config_json_.status().message()); + } } result = OnResolvedLocked(); } diff --git a/test/core/event_engine/thready_event_engine/thready_event_engine.cc b/test/core/event_engine/thready_event_engine/thready_event_engine.cc index 47f77e650b3..3b613ce4c38 100644 --- a/test/core/event_engine/thready_event_engine/thready_event_engine.cc +++ b/test/core/event_engine/thready_event_engine/thready_event_engine.cc @@ -144,7 +144,7 @@ ThreadyEventEngine::ThreadyDNSResolver::LookupTXT(LookupTXTCallback on_resolve, Duration timeout) { return impl_->LookupTXT( [this, on_resolve = std::move(on_resolve)]( - absl::StatusOr record) mutable { + absl::StatusOr> record) mutable { return engine_->Asynchronously([on_resolve = std::move(on_resolve), record = std::move(record)]() mutable { on_resolve(std::move(record));