From 61be7b0070ef1b6195cc802e540d28c7f0417353 Mon Sep 17 00:00:00 2001 From: Yijie Ma Date: Fri, 18 Oct 2024 17:02:39 -0700 Subject: [PATCH] [EventEngine] [reland] Migrate `chttp2_server` to use EE DNSResolver (#37901) Closes #37901 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/37901 from yijiem:dns-migration-chttp2-server-2 70d29b3b6ccfc8f181b26d82361a5a841f19e6c1 PiperOrigin-RevId: 687466646 --- bazel/experiments.bzl | 5 ++ src/core/BUILD | 5 ++ .../transport/chttp2/server/chttp2_server.cc | 90 ++++++++++++++----- src/core/lib/event_engine/resolved_address.cc | 3 + src/core/lib/event_engine/utils.cc | 16 ++++ src/core/lib/event_engine/utils.h | 7 ++ src/core/lib/experiments/experiments.cc | 27 ++++++ src/core/lib/experiments/experiments.h | 8 ++ src/core/lib/experiments/experiments.yaml | 8 ++ 9 files changed, 145 insertions(+), 24 deletions(-) diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index 635160b0657..11dc93d96cf 100644 --- a/bazel/experiments.bzl +++ b/bazel/experiments.bzl @@ -23,6 +23,7 @@ EXPERIMENT_ENABLES = { "event_engine_application_callbacks": "event_engine_application_callbacks", "event_engine_client": "event_engine_client", "event_engine_dns": "event_engine_dns", + "event_engine_dns_non_client_channel": "event_engine_dns_non_client_channel", "event_engine_listener": "event_engine_listener", "free_large_allocator": "free_large_allocator", "local_connector_secure": "local_connector_secure", @@ -45,6 +46,7 @@ EXPERIMENT_ENABLES = { EXPERIMENT_POLLERS = [ "event_engine_client", "event_engine_dns", + "event_engine_dns_non_client_channel", "event_engine_listener", ] @@ -54,6 +56,7 @@ EXPERIMENTS = { }, "off": { "core_end2end_test": [ + "event_engine_dns_non_client_channel", "local_connector_secure", ], "endpoint_test": [ @@ -103,6 +106,7 @@ EXPERIMENTS = { }, "off": { "core_end2end_test": [ + "event_engine_dns_non_client_channel", "local_connector_secure", ], "endpoint_test": [ @@ -136,6 +140,7 @@ EXPERIMENTS = { }, "off": { "core_end2end_test": [ + "event_engine_dns_non_client_channel", "local_connector_secure", ], "endpoint_test": [ diff --git a/src/core/BUILD b/src/core/BUILD index a6b01f761c4..3792101db63 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -2378,9 +2378,11 @@ grpc_cc_library( hdrs = ["lib/event_engine/utils.h"], external_deps = [ "absl/log:check", + "absl/status:statusor", "absl/strings", ], deps = [ + "notification", "time", "//:event_engine_base_hdrs", "//:gpr_platform", @@ -7658,8 +7660,11 @@ grpc_cc_library( "connection_quota", "error", "error_utils", + "event_engine_common", "event_engine_extensions", "event_engine_query_extensions", + "event_engine_tcp_socket_utils", + "event_engine_utils", "grpc_insecure_credentials", "handshaker_registry", "iomgr_fwd", diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.cc b/src/core/ext/transport/chttp2/server/chttp2_server.cc index 2bc2e1c857c..e4e43d2a126 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.cc +++ b/src/core/ext/transport/chttp2/server/chttp2_server.cc @@ -59,6 +59,9 @@ #include "src/core/lib/event_engine/channel_args_endpoint_config.h" #include "src/core/lib/event_engine/extensions/supports_fd.h" #include "src/core/lib/event_engine/query_extensions.h" +#include "src/core/lib/event_engine/resolved_address_internal.h" +#include "src/core/lib/event_engine/tcp_socket_utils.h" +#include "src/core/lib/event_engine/utils.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/endpoint.h" #include "src/core/lib/iomgr/event_engine_shims/endpoint.h" @@ -113,7 +116,8 @@ using AcceptorPtr = std::unique_ptr; class Chttp2ServerListener : public Server::ListenerInterface { public: - static grpc_error_handle Create(Server* server, grpc_resolved_address* addr, + static grpc_error_handle Create(Server* server, + const EventEngine::ResolvedAddress& addr, const ChannelArgs& args, Chttp2ServerArgsModifier args_modifier, int* port_num); @@ -703,8 +707,9 @@ void Chttp2ServerListener::ActiveConnection::OnDrainGraceTimeExpiry() { // grpc_error_handle Chttp2ServerListener::Create( - Server* server, grpc_resolved_address* addr, const ChannelArgs& args, - Chttp2ServerArgsModifier args_modifier, int* port_num) { + Server* server, const EventEngine::ResolvedAddress& addr, + const ChannelArgs& args, Chttp2ServerArgsModifier args_modifier, + int* port_num) { // Create Chttp2ServerListener. OrphanablePtr listener = MakeOrphanable(server, args, args_modifier, @@ -716,18 +721,24 @@ grpc_error_handle Chttp2ServerListener::Create( &listener->tcp_server_shutdown_complete_, ChannelArgsEndpointConfig(args), OnAccept, listener.get(), &listener->tcp_server_); if (!error.ok()) return error; + // TODO(yijiem): remove this conversion when we remove all + // grpc_resolved_address usages. + grpc_resolved_address iomgr_addr = + grpc_event_engine::experimental::CreateGRPCResolvedAddress(addr); if (listener->config_fetcher_ != nullptr) { - listener->resolved_address_ = *addr; + listener->resolved_address_ = iomgr_addr; // TODO(yashykt): Consider binding so as to be able to return the port // number. } else { - error = grpc_tcp_server_add_port(listener->tcp_server_, addr, port_num); + error = + grpc_tcp_server_add_port(listener->tcp_server_, &iomgr_addr, port_num); if (!error.ok()) return error; } // Create channelz node. if (args.GetBool(GRPC_ARG_ENABLE_CHANNELZ) .value_or(GRPC_ENABLE_CHANNELZ_DEFAULT)) { - auto string_address = grpc_sockaddr_to_uri(addr); + auto string_address = + grpc_event_engine::experimental::ResolvedAddressToString(addr); if (!string_address.ok()) { return GRPC_ERROR_CREATE(string_address.status().ToString()); } @@ -957,37 +968,68 @@ grpc_error_handle Chttp2ServerAddPort(Server* server, const char* addr, args_modifier); } *port_num = -1; - absl::StatusOr> resolved_or; + absl::StatusOr> resolved; + absl::StatusOr> results = + std::vector(); std::vector error_list; std::string parsed_addr = URI::PercentDecode(addr); absl::string_view parsed_addr_unprefixed{parsed_addr}; // Using lambda to avoid use of goto. grpc_error_handle error = [&]() { grpc_error_handle error; + // TODO(ladynana, yijiem): this code does not handle address URIs correctly: + // it's parsing `unix://foo/bar` as path `/foo/bar` when it should be + // parsing it as authority `foo` and path `/bar`. Also add API documentation + // on the valid URIs that grpc_server_add_http2_port accepts. if (absl::ConsumePrefix(&parsed_addr_unprefixed, kUnixUriPrefix)) { - resolved_or = grpc_resolve_unix_domain_address(parsed_addr_unprefixed); + resolved = grpc_resolve_unix_domain_address(parsed_addr_unprefixed); + GRPC_RETURN_IF_ERROR(resolved.status()); } else if (absl::ConsumePrefix(&parsed_addr_unprefixed, kUnixAbstractUriPrefix)) { - resolved_or = + resolved = grpc_resolve_unix_abstract_domain_address(parsed_addr_unprefixed); + GRPC_RETURN_IF_ERROR(resolved.status()); } else if (absl::ConsumePrefix(&parsed_addr_unprefixed, kVSockUriPrefix)) { - resolved_or = grpc_resolve_vsock_address(parsed_addr_unprefixed); + resolved = grpc_resolve_vsock_address(parsed_addr_unprefixed); + GRPC_RETURN_IF_ERROR(resolved.status()); } else { - resolved_or = - GetDNSResolver()->LookupHostnameBlocking(parsed_addr, "https"); + if (IsEventEngineDnsNonClientChannelEnabled()) { + absl::StatusOr> ee_resolver = + args.GetObjectRef()->GetDNSResolver( + EventEngine::DNSResolver::ResolverOptions()); + GRPC_RETURN_IF_ERROR(ee_resolver.status()); + results = grpc_event_engine::experimental::LookupHostnameBlocking( + ee_resolver->get(), parsed_addr, "https"); + } else { + // TODO(yijiem): Remove this after event_engine_dns_non_client_channel + // is fully enabled. + absl::StatusOr> iomgr_results = + GetDNSResolver()->LookupHostnameBlocking(parsed_addr, "https"); + GRPC_RETURN_IF_ERROR(iomgr_results.status()); + for (const auto& addr : *iomgr_results) { + results->push_back( + grpc_event_engine::experimental::CreateResolvedAddress(addr)); + } + } } - if (!resolved_or.ok()) { - return absl_status_to_grpc_error(resolved_or.status()); + if (resolved.ok()) { + for (const auto& addr : *resolved) { + results->push_back( + grpc_event_engine::experimental::CreateResolvedAddress(addr)); + } } + GRPC_RETURN_IF_ERROR(results.status()); // Create a listener for each resolved address. - for (auto& addr : *resolved_or) { + for (EventEngine::ResolvedAddress& addr : *results) { // If address has a wildcard port (0), use the same port as a previous // listener. - if (*port_num != -1 && grpc_sockaddr_get_port(&addr) == 0) { - grpc_sockaddr_set_port(&addr, *port_num); + if (*port_num != -1 && + grpc_event_engine::experimental::ResolvedAddressGetPort(addr) == 0) { + grpc_event_engine::experimental::ResolvedAddressSetPort(addr, + *port_num); } int port_temp = -1; - error = Chttp2ServerListener::Create(server, &addr, args, args_modifier, + error = Chttp2ServerListener::Create(server, addr, args, args_modifier, &port_temp); if (!error.ok()) { error_list.push_back(error); @@ -999,17 +1041,17 @@ grpc_error_handle Chttp2ServerAddPort(Server* server, const char* addr, } } } - if (error_list.size() == resolved_or->size()) { + if (error_list.size() == results->size()) { std::string msg = absl::StrFormat( "No address added out of total %" PRIuPTR " resolved for '%s'", - resolved_or->size(), addr); + results->size(), addr); return GRPC_ERROR_CREATE_REFERENCING(msg.c_str(), error_list.data(), error_list.size()); } else if (!error_list.empty()) { - std::string msg = absl::StrFormat( - "Only %" PRIuPTR " addresses added out of total %" PRIuPTR - " resolved", - resolved_or->size() - error_list.size(), resolved_or->size()); + std::string msg = + absl::StrFormat("Only %" PRIuPTR + " addresses added out of total %" PRIuPTR " resolved", + results->size() - error_list.size(), results->size()); error = GRPC_ERROR_CREATE_REFERENCING(msg.c_str(), error_list.data(), error_list.size()); LOG(INFO) << "WARNING: " << StatusToString(error); diff --git a/src/core/lib/event_engine/resolved_address.cc b/src/core/lib/event_engine/resolved_address.cc index 1a41663bedd..2672923f8c2 100644 --- a/src/core/lib/event_engine/resolved_address.cc +++ b/src/core/lib/event_engine/resolved_address.cc @@ -48,6 +48,9 @@ EventEngine::ResolvedAddress CreateResolvedAddress( grpc_resolved_address CreateGRPCResolvedAddress( const EventEngine::ResolvedAddress& ra) { + static_assert( + GRPC_MAX_SOCKADDR_SIZE == EventEngine::ResolvedAddress::MAX_SIZE_BYTES, + "size should match"); grpc_resolved_address grpc_addr; memset(&grpc_addr, 0, sizeof(grpc_resolved_address)); memcpy(grpc_addr.addr, ra.address(), ra.size()); diff --git a/src/core/lib/event_engine/utils.cc b/src/core/lib/event_engine/utils.cc index f2a202315ce..ad4b44a287e 100644 --- a/src/core/lib/event_engine/utils.cc +++ b/src/core/lib/event_engine/utils.cc @@ -20,6 +20,7 @@ #include #include "absl/strings/str_cat.h" +#include "src/core/util/notification.h" #include "src/core/util/time.h" namespace grpc_event_engine { @@ -38,5 +39,20 @@ grpc_core::Timestamp ToTimestamp(grpc_core::Timestamp now, grpc_core::Duration::Milliseconds(1); } +absl::StatusOr> +LookupHostnameBlocking(EventEngine::DNSResolver* dns_resolver, + absl::string_view name, absl::string_view default_port) { + absl::StatusOr> results; + grpc_core::Notification done; + dns_resolver->LookupHostname( + [&](absl::StatusOr> addresses) { + results = std::move(addresses); + done.Notify(); + }, + name, default_port); + done.WaitForNotification(); + return results; +} + } // namespace experimental } // namespace grpc_event_engine diff --git a/src/core/lib/event_engine/utils.h b/src/core/lib/event_engine/utils.h index 45675ee8d88..0e15d69b495 100644 --- a/src/core/lib/event_engine/utils.h +++ b/src/core/lib/event_engine/utils.h @@ -19,7 +19,10 @@ #include #include +#include +#include "absl/status/statusor.h" +#include "absl/strings/string_view.h" #include "src/core/util/time.h" namespace grpc_event_engine { @@ -36,6 +39,10 @@ std::string HandleToString(const Handle& handle) { grpc_core::Timestamp ToTimestamp(grpc_core::Timestamp now, EventEngine::Duration delta); +absl::StatusOr> +LookupHostnameBlocking(EventEngine::DNSResolver* dns_resolver, + absl::string_view name, absl::string_view default_port); + } // namespace experimental } // namespace grpc_event_engine diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index 4c542dd2310..1e752e0ca9b 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -41,6 +41,11 @@ const char* const additional_constraints_event_engine_client = "{}"; const char* const description_event_engine_dns = "If set, use EventEngine DNSResolver for client channel resolution"; const char* const additional_constraints_event_engine_dns = "{}"; +const char* const description_event_engine_dns_non_client_channel = + "If set, use EventEngine DNSResolver in other places besides client " + "channel."; +const char* const additional_constraints_event_engine_dns_non_client_channel = + "{}"; const char* const description_event_engine_listener = "Use EventEngine listeners instead of iomgr's grpc_tcp_server"; const char* const additional_constraints_event_engine_listener = "{}"; @@ -122,6 +127,10 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_event_engine_client, nullptr, 0, false, true}, {"event_engine_dns", description_event_engine_dns, additional_constraints_event_engine_dns, nullptr, 0, false, false}, + {"event_engine_dns_non_client_channel", + description_event_engine_dns_non_client_channel, + additional_constraints_event_engine_dns_non_client_channel, nullptr, 0, + false, false}, {"event_engine_listener", description_event_engine_listener, additional_constraints_event_engine_listener, nullptr, 0, false, true}, {"free_large_allocator", description_free_large_allocator, @@ -188,6 +197,11 @@ const char* const additional_constraints_event_engine_client = "{}"; const char* const description_event_engine_dns = "If set, use EventEngine DNSResolver for client channel resolution"; const char* const additional_constraints_event_engine_dns = "{}"; +const char* const description_event_engine_dns_non_client_channel = + "If set, use EventEngine DNSResolver in other places besides client " + "channel."; +const char* const additional_constraints_event_engine_dns_non_client_channel = + "{}"; const char* const description_event_engine_listener = "Use EventEngine listeners instead of iomgr's grpc_tcp_server"; const char* const additional_constraints_event_engine_listener = "{}"; @@ -269,6 +283,10 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_event_engine_client, nullptr, 0, true, true}, {"event_engine_dns", description_event_engine_dns, additional_constraints_event_engine_dns, nullptr, 0, true, false}, + {"event_engine_dns_non_client_channel", + description_event_engine_dns_non_client_channel, + additional_constraints_event_engine_dns_non_client_channel, nullptr, 0, + false, false}, {"event_engine_listener", description_event_engine_listener, additional_constraints_event_engine_listener, nullptr, 0, true, true}, {"free_large_allocator", description_free_large_allocator, @@ -335,6 +353,11 @@ const char* const additional_constraints_event_engine_client = "{}"; const char* const description_event_engine_dns = "If set, use EventEngine DNSResolver for client channel resolution"; const char* const additional_constraints_event_engine_dns = "{}"; +const char* const description_event_engine_dns_non_client_channel = + "If set, use EventEngine DNSResolver in other places besides client " + "channel."; +const char* const additional_constraints_event_engine_dns_non_client_channel = + "{}"; const char* const description_event_engine_listener = "Use EventEngine listeners instead of iomgr's grpc_tcp_server"; const char* const additional_constraints_event_engine_listener = "{}"; @@ -416,6 +439,10 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_event_engine_client, nullptr, 0, true, true}, {"event_engine_dns", description_event_engine_dns, additional_constraints_event_engine_dns, nullptr, 0, true, false}, + {"event_engine_dns_non_client_channel", + description_event_engine_dns_non_client_channel, + additional_constraints_event_engine_dns_non_client_channel, nullptr, 0, + false, false}, {"event_engine_listener", description_event_engine_listener, additional_constraints_event_engine_listener, nullptr, 0, true, true}, {"free_large_allocator", description_free_large_allocator, diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index 56b2c65459a..99702c5bbd1 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -65,6 +65,7 @@ inline bool IsClientPrivacyEnabled() { return false; } inline bool IsEventEngineApplicationCallbacksEnabled() { return true; } inline bool IsEventEngineClientEnabled() { return false; } inline bool IsEventEngineDnsEnabled() { return false; } +inline bool IsEventEngineDnsNonClientChannelEnabled() { return false; } inline bool IsEventEngineListenerEnabled() { return false; } inline bool IsFreeLargeAllocatorEnabled() { return false; } inline bool IsLocalConnectorSecureEnabled() { return false; } @@ -98,6 +99,7 @@ inline bool IsEventEngineApplicationCallbacksEnabled() { return true; } inline bool IsEventEngineClientEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_DNS inline bool IsEventEngineDnsEnabled() { return true; } +inline bool IsEventEngineDnsNonClientChannelEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_LISTENER inline bool IsEventEngineListenerEnabled() { return true; } inline bool IsFreeLargeAllocatorEnabled() { return false; } @@ -132,6 +134,7 @@ inline bool IsEventEngineApplicationCallbacksEnabled() { return true; } inline bool IsEventEngineClientEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_DNS inline bool IsEventEngineDnsEnabled() { return true; } +inline bool IsEventEngineDnsNonClientChannelEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_LISTENER inline bool IsEventEngineListenerEnabled() { return true; } inline bool IsFreeLargeAllocatorEnabled() { return false; } @@ -165,6 +168,7 @@ enum ExperimentIds { kExperimentIdEventEngineApplicationCallbacks, kExperimentIdEventEngineClient, kExperimentIdEventEngineDns, + kExperimentIdEventEngineDnsNonClientChannel, kExperimentIdEventEngineListener, kExperimentIdFreeLargeAllocator, kExperimentIdLocalConnectorSecure, @@ -208,6 +212,10 @@ inline bool IsEventEngineClientEnabled() { inline bool IsEventEngineDnsEnabled() { return IsExperimentEnabled(); } +#define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_DNS_NON_CLIENT_CHANNEL +inline bool IsEventEngineDnsNonClientChannelEnabled() { + return IsExperimentEnabled(); +} #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_LISTENER inline bool IsEventEngineListenerEnabled() { return IsExperimentEnabled(); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index d46093ae848..05c3d193b5d 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -77,6 +77,14 @@ test_tags: ["cancel_ares_query_test", "resolver_component_tests_runner_invoker"] allow_in_fuzzing_config: false uses_polling: true +- name: event_engine_dns_non_client_channel + description: + If set, use EventEngine DNSResolver in other places besides client channel. + expiry: 2025/03/01 + owner: yijiem@google.com + test_tags: ["core_end2end_test"] + allow_in_fuzzing_config: false + uses_polling: true - name: event_engine_listener description: Use EventEngine listeners instead of iomgr's grpc_tcp_server expiry: 2025/03/01