From 8ee36d26bf037942471fd4c540c8e4bf8a2c8f5a Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 8 Sep 2021 14:58:39 -0700 Subject: [PATCH] Move handshakers to core configuration system (#27195) * Move handshakers to core configuration * Automated change: Fix sanity tests * fix memory ordering * Update http_connect_handshaker.h * slightly simpler code * clangfmt * review feedback * review feedback Co-authored-by: ctiller --- BUILD | 35 +++++- CMakeLists.txt | 68 +++++++++++ build_autogenerated.yaml | 112 +++++++++++++++++- .../client_channel/client_channel_plugin.cc | 9 +- .../client_channel/http_connect_handshaker.cc | 10 +- .../client_channel/http_connect_handshaker.h | 12 +- .../chttp2/client/chttp2_connector.cc | 9 +- .../transport/chttp2/server/chttp2_server.cc | 6 +- src/core/lib/channel/handshaker_factory.h | 12 +- src/core/lib/channel/handshaker_registry.cc | 87 +++----------- src/core/lib/channel/handshaker_registry.h | 41 +++++-- src/core/lib/config/core_configuration.cc | 9 +- src/core/lib/config/core_configuration.h | 36 +++++- .../lib/http/httpcli_security_connector.cc | 4 +- .../security/transport/security_handshaker.cc | 8 +- .../security/transport/security_handshaker.h | 3 +- src/core/lib/surface/init.cc | 4 - src/core/lib/surface/init.h | 1 - src/core/lib/surface/init_secure.cc | 2 - src/core/lib/surface/init_unsecure.cc | 6 - .../plugin_registry/grpc_plugin_registry.cc | 10 +- .../grpc_unsecure_plugin_registry.cc | 6 +- .../readahead_handshaker_server_ssl.cc | 12 +- 23 files changed, 364 insertions(+), 138 deletions(-) diff --git a/BUILD b/BUILD index dfc5eac40d2..88d27497bd3 100644 --- a/BUILD +++ b/BUILD @@ -801,6 +801,7 @@ grpc_cc_library( ], deps = [ "gpr_platform", + "handshaker_registry", ], ) @@ -1209,6 +1210,32 @@ grpc_cc_library( ], ) +grpc_cc_library( + name = "handshaker_factory", + language = "c++", + public_hdrs = [ + "src/core/lib/channel/handshaker_factory.h", + ], + deps = [ + "gpr_base", + ], +) + +grpc_cc_library( + name = "handshaker_registry", + srcs = [ + "src/core/lib/channel/handshaker_registry.cc", + ], + language = "c++", + public_hdrs = [ + "src/core/lib/channel/handshaker_registry.h", + ], + deps = [ + "gpr_base", + "handshaker_factory", + ], +) + grpc_cc_library( name = "grpc_base_c", srcs = [ @@ -1224,7 +1251,6 @@ grpc_cc_library( "src/core/lib/channel/channelz_registry.cc", "src/core/lib/channel/connected_channel.cc", "src/core/lib/channel/handshaker.cc", - "src/core/lib/channel/handshaker_registry.cc", "src/core/lib/channel/status_util.cc", "src/core/lib/compression/compression.cc", "src/core/lib/compression/compression_args.cc", @@ -1394,8 +1420,6 @@ grpc_cc_library( "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", "src/core/lib/channel/handshaker.h", - "src/core/lib/channel/handshaker_factory.h", - "src/core/lib/channel/handshaker_registry.h", "src/core/lib/channel/status_util.h", "src/core/lib/compression/algorithm_metadata.h", "src/core/lib/compression/compression_args.h", @@ -1678,6 +1702,7 @@ grpc_cc_library( language = "c++", visibility = ["@grpc:client_channel"], deps = [ + "config", "debug_location", "gpr_base", "grpc_base_c", @@ -1685,6 +1710,7 @@ grpc_cc_library( "grpc_deadline_filter", "grpc_health_upb", "grpc_trace", + "handshaker_registry", "orphanable", "ref_counted", "ref_counted_ptr", @@ -2668,6 +2694,7 @@ grpc_cc_library( visibility = ["@grpc:public"], deps = [ "alts_util", + "config", "gpr_base", "grpc_base", "grpc_base_c", @@ -2972,6 +2999,7 @@ grpc_cc_library( ], language = "c++", deps = [ + "config", "gpr_base", "grpc_base_c", "grpc_client_channel", @@ -3025,6 +3053,7 @@ grpc_cc_library( ], language = "c++", deps = [ + "config", "gpr_base", "grpc_base_c", "grpc_codegen", diff --git a/CMakeLists.txt b/CMakeLists.txt index 002754c1dda..76e1c358856 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10419,7 +10419,64 @@ endif() if(gRPC_BUILD_TESTS) add_executable(core_configuration_test + src/core/ext/upb-generated/google/api/annotations.upb.c + src/core/ext/upb-generated/google/api/expr/v1alpha1/checked.upb.c + src/core/ext/upb-generated/google/api/expr/v1alpha1/syntax.upb.c + src/core/ext/upb-generated/google/api/http.upb.c + src/core/ext/upb-generated/google/protobuf/any.upb.c + src/core/ext/upb-generated/google/protobuf/duration.upb.c + src/core/ext/upb-generated/google/protobuf/empty.upb.c + src/core/ext/upb-generated/google/protobuf/struct.upb.c + src/core/ext/upb-generated/google/protobuf/timestamp.upb.c + src/core/ext/upb-generated/google/protobuf/wrappers.upb.c + src/core/ext/upb-generated/google/rpc/status.upb.c + src/core/lib/channel/handshaker_registry.cc src/core/lib/config/core_configuration.cc + src/core/lib/gpr/alloc.cc + src/core/lib/gpr/atm.cc + src/core/lib/gpr/cpu_iphone.cc + src/core/lib/gpr/cpu_linux.cc + src/core/lib/gpr/cpu_posix.cc + src/core/lib/gpr/cpu_windows.cc + src/core/lib/gpr/env_linux.cc + src/core/lib/gpr/env_posix.cc + src/core/lib/gpr/env_windows.cc + src/core/lib/gpr/log.cc + src/core/lib/gpr/log_android.cc + src/core/lib/gpr/log_linux.cc + src/core/lib/gpr/log_posix.cc + src/core/lib/gpr/log_windows.cc + src/core/lib/gpr/murmur_hash.cc + src/core/lib/gpr/string.cc + src/core/lib/gpr/string_posix.cc + src/core/lib/gpr/string_util_windows.cc + src/core/lib/gpr/string_windows.cc + src/core/lib/gpr/sync.cc + src/core/lib/gpr/sync_abseil.cc + src/core/lib/gpr/sync_posix.cc + src/core/lib/gpr/sync_windows.cc + src/core/lib/gpr/time.cc + src/core/lib/gpr/time_posix.cc + src/core/lib/gpr/time_precise.cc + src/core/lib/gpr/time_windows.cc + src/core/lib/gpr/tmpfile_msys.cc + src/core/lib/gpr/tmpfile_posix.cc + src/core/lib/gpr/tmpfile_windows.cc + src/core/lib/gpr/wrap_memcpy.cc + src/core/lib/gprpp/arena.cc + src/core/lib/gprpp/examine_stack.cc + src/core/lib/gprpp/fork.cc + src/core/lib/gprpp/global_config_env.cc + src/core/lib/gprpp/host_port.cc + src/core/lib/gprpp/mpscq.cc + src/core/lib/gprpp/stat_posix.cc + src/core/lib/gprpp/stat_windows.cc + src/core/lib/gprpp/status_helper.cc + src/core/lib/gprpp/thd_posix.cc + src/core/lib/gprpp/thd_windows.cc + src/core/lib/gprpp/time_util.cc + src/core/lib/profiling/basic_timers.cc + src/core/lib/profiling/stap_timers.cc test/core/config/core_configuration_test.cc third_party/googletest/googletest/src/gtest-all.cc third_party/googletest/googlemock/src/gmock-all.cc @@ -10447,6 +10504,17 @@ target_include_directories(core_configuration_test target_link_libraries(core_configuration_test ${_gRPC_PROTOBUF_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} + absl::base + absl::core_headers + absl::memory + absl::status + absl::cord + absl::str_format + absl::strings + absl::synchronization + absl::time + absl::optional + upb ) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index ad74c610388..46b742113ce 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -5333,11 +5333,121 @@ targets: build: test language: c++ headers: + - src/core/ext/upb-generated/google/api/annotations.upb.h + - src/core/ext/upb-generated/google/api/expr/v1alpha1/checked.upb.h + - src/core/ext/upb-generated/google/api/expr/v1alpha1/syntax.upb.h + - src/core/ext/upb-generated/google/api/http.upb.h + - src/core/ext/upb-generated/google/protobuf/any.upb.h + - src/core/ext/upb-generated/google/protobuf/duration.upb.h + - src/core/ext/upb-generated/google/protobuf/empty.upb.h + - src/core/ext/upb-generated/google/protobuf/struct.upb.h + - src/core/ext/upb-generated/google/protobuf/timestamp.upb.h + - src/core/ext/upb-generated/google/protobuf/wrappers.upb.h + - src/core/ext/upb-generated/google/rpc/status.upb.h + - src/core/lib/channel/handshaker_factory.h + - src/core/lib/channel/handshaker_registry.h - src/core/lib/config/core_configuration.h + - src/core/lib/gpr/alloc.h + - src/core/lib/gpr/env.h + - src/core/lib/gpr/murmur_hash.h + - src/core/lib/gpr/spinlock.h + - src/core/lib/gpr/string.h + - src/core/lib/gpr/string_windows.h + - src/core/lib/gpr/time_precise.h + - src/core/lib/gpr/tls.h + - src/core/lib/gpr/tmpfile.h + - src/core/lib/gpr/useful.h + - src/core/lib/gprpp/arena.h + - src/core/lib/gprpp/construct_destruct.h + - src/core/lib/gprpp/debug_location.h + - src/core/lib/gprpp/examine_stack.h + - src/core/lib/gprpp/fork.h + - src/core/lib/gprpp/global_config.h + - src/core/lib/gprpp/global_config_custom.h + - src/core/lib/gprpp/global_config_env.h + - src/core/lib/gprpp/global_config_generic.h + - src/core/lib/gprpp/host_port.h + - src/core/lib/gprpp/manual_constructor.h + - src/core/lib/gprpp/memory.h + - src/core/lib/gprpp/mpscq.h + - src/core/lib/gprpp/stat.h + - src/core/lib/gprpp/status_helper.h + - src/core/lib/gprpp/sync.h + - src/core/lib/gprpp/thd.h + - src/core/lib/gprpp/time_util.h + - src/core/lib/profiling/timers.h src: + - src/core/ext/upb-generated/google/api/annotations.upb.c + - src/core/ext/upb-generated/google/api/expr/v1alpha1/checked.upb.c + - src/core/ext/upb-generated/google/api/expr/v1alpha1/syntax.upb.c + - src/core/ext/upb-generated/google/api/http.upb.c + - src/core/ext/upb-generated/google/protobuf/any.upb.c + - src/core/ext/upb-generated/google/protobuf/duration.upb.c + - src/core/ext/upb-generated/google/protobuf/empty.upb.c + - src/core/ext/upb-generated/google/protobuf/struct.upb.c + - src/core/ext/upb-generated/google/protobuf/timestamp.upb.c + - src/core/ext/upb-generated/google/protobuf/wrappers.upb.c + - src/core/ext/upb-generated/google/rpc/status.upb.c + - src/core/lib/channel/handshaker_registry.cc - src/core/lib/config/core_configuration.cc + - src/core/lib/gpr/alloc.cc + - src/core/lib/gpr/atm.cc + - src/core/lib/gpr/cpu_iphone.cc + - src/core/lib/gpr/cpu_linux.cc + - src/core/lib/gpr/cpu_posix.cc + - src/core/lib/gpr/cpu_windows.cc + - src/core/lib/gpr/env_linux.cc + - src/core/lib/gpr/env_posix.cc + - src/core/lib/gpr/env_windows.cc + - src/core/lib/gpr/log.cc + - src/core/lib/gpr/log_android.cc + - src/core/lib/gpr/log_linux.cc + - src/core/lib/gpr/log_posix.cc + - src/core/lib/gpr/log_windows.cc + - src/core/lib/gpr/murmur_hash.cc + - src/core/lib/gpr/string.cc + - src/core/lib/gpr/string_posix.cc + - src/core/lib/gpr/string_util_windows.cc + - src/core/lib/gpr/string_windows.cc + - src/core/lib/gpr/sync.cc + - src/core/lib/gpr/sync_abseil.cc + - src/core/lib/gpr/sync_posix.cc + - src/core/lib/gpr/sync_windows.cc + - src/core/lib/gpr/time.cc + - src/core/lib/gpr/time_posix.cc + - src/core/lib/gpr/time_precise.cc + - src/core/lib/gpr/time_windows.cc + - src/core/lib/gpr/tmpfile_msys.cc + - src/core/lib/gpr/tmpfile_posix.cc + - src/core/lib/gpr/tmpfile_windows.cc + - src/core/lib/gpr/wrap_memcpy.cc + - src/core/lib/gprpp/arena.cc + - src/core/lib/gprpp/examine_stack.cc + - src/core/lib/gprpp/fork.cc + - src/core/lib/gprpp/global_config_env.cc + - src/core/lib/gprpp/host_port.cc + - src/core/lib/gprpp/mpscq.cc + - src/core/lib/gprpp/stat_posix.cc + - src/core/lib/gprpp/stat_windows.cc + - src/core/lib/gprpp/status_helper.cc + - src/core/lib/gprpp/thd_posix.cc + - src/core/lib/gprpp/thd_windows.cc + - src/core/lib/gprpp/time_util.cc + - src/core/lib/profiling/basic_timers.cc + - src/core/lib/profiling/stap_timers.cc - test/core/config/core_configuration_test.cc - deps: [] + deps: + - absl/base:base + - absl/base:core_headers + - absl/memory:memory + - absl/status:status + - absl/strings:cord + - absl/strings:str_format + - absl/strings:strings + - absl/synchronization:synchronization + - absl/time:time + - absl/types:optional + - upb uses_polling: false - name: delegating_channel_test gtest: true diff --git a/src/core/ext/filters/client_channel/client_channel_plugin.cc b/src/core/ext/filters/client_channel/client_channel_plugin.cc index 1d33d25b491..8bd41f2c722 100644 --- a/src/core/ext/filters/client_channel/client_channel_plugin.cc +++ b/src/core/ext/filters/client_channel/client_channel_plugin.cc @@ -58,7 +58,6 @@ void grpc_client_channel_init(void) { GRPC_CLIENT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, append_filter, const_cast( &grpc_core::ClientChannel::kFilterVtable)); - grpc_http_connect_register_handshaker_factory(); grpc_client_channel_global_init_backup_polling(); } @@ -71,3 +70,11 @@ void grpc_client_channel_shutdown(void) { grpc_core::LoadBalancingPolicyRegistry::Builder::ShutdownRegistry(); grpc_core::ServiceConfigParser::Shutdown(); } + +namespace grpc_core { + +void BuildClientChannelConfiguration(CoreConfiguration::Builder* builder) { + RegisterHttpConnectHandshaker(builder); +} + +} // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/http_connect_handshaker.cc b/src/core/ext/filters/client_channel/http_connect_handshaker.cc index 3dff8249d08..1cc25dff9a0 100644 --- a/src/core/ext/filters/client_channel/http_connect_handshaker.cc +++ b/src/core/ext/filters/client_channel/http_connect_handshaker.cc @@ -32,7 +32,9 @@ #include "src/core/ext/filters/client_channel/client_channel.h" #include "src/core/ext/filters/client_channel/resolver_registry.h" #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/channel/handshaker.h" #include "src/core/lib/channel/handshaker_registry.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/sync.h" #include "src/core/lib/http/format_request.h" @@ -382,10 +384,10 @@ class HttpConnectHandshakerFactory : public HandshakerFactory { } // namespace -} // namespace grpc_core - -void grpc_http_connect_register_handshaker_factory() { - grpc_core::HandshakerRegistry::RegisterHandshakerFactory( +void RegisterHttpConnectHandshaker(CoreConfiguration::Builder* builder) { + builder->handshaker_registry()->RegisterHandshakerFactory( true /* at_start */, grpc_core::HANDSHAKER_CLIENT, absl::make_unique()); } + +} // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/http_connect_handshaker.h b/src/core/ext/filters/client_channel/http_connect_handshaker.h index 26c31f2a0ab..31d25c255b5 100644 --- a/src/core/ext/filters/client_channel/http_connect_handshaker.h +++ b/src/core/ext/filters/client_channel/http_connect_handshaker.h @@ -19,6 +19,10 @@ #ifndef GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_HTTP_CONNECT_HANDSHAKER_H #define GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_HTTP_CONNECT_HANDSHAKER_H +#include + +#include "src/core/lib/config/core_configuration.h" + /// Channel arg indicating the server in HTTP CONNECT request (string). /// The presence of this arg triggers the use of HTTP CONNECT. #define GRPC_ARG_HTTP_CONNECT_SERVER "grpc.http_connect_server" @@ -28,7 +32,11 @@ /// separated by colons. #define GRPC_ARG_HTTP_CONNECT_HEADERS "grpc.http_connect_headers" -/// Registers handshaker factory. -void grpc_http_connect_register_handshaker_factory(); +namespace grpc_core { + +// Register the HTTP Connect handshaker into the configuration builder. +void RegisterHttpConnectHandshaker(CoreConfiguration::Builder* builder); + +} // namespace grpc_core #endif /* GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_HTTP_CONNECT_HANDSHAKER_H */ diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.cc b/src/core/ext/transport/chttp2/client/chttp2_connector.cc index 9e0ebc45c4d..96dd4eb7bf2 100644 --- a/src/core/ext/transport/chttp2/client/chttp2_connector.cc +++ b/src/core/ext/transport/chttp2/client/chttp2_connector.cc @@ -28,13 +28,12 @@ #include #include "src/core/ext/filters/client_channel/connector.h" -#include "src/core/ext/filters/client_channel/http_connect_handshaker.h" #include "src/core/ext/filters/client_channel/subchannel.h" #include "src/core/ext/transport/chttp2/transport/chttp2_transport.h" #include "src/core/lib/address_utils/sockaddr_utils.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/handshaker.h" -#include "src/core/lib/channel/handshaker_registry.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/iomgr/tcp_client.h" #include "src/core/lib/slice/slice_internal.h" @@ -134,9 +133,9 @@ void Chttp2Connector::Connected(void* arg, grpc_error_handle error) { void Chttp2Connector::StartHandshakeLocked() { handshake_mgr_ = MakeRefCounted(); - HandshakerRegistry::AddHandshakers(HANDSHAKER_CLIENT, args_.channel_args, - args_.interested_parties, - handshake_mgr_.get()); + CoreConfiguration::Get().handshaker_registry().AddHandshakers( + HANDSHAKER_CLIENT, args_.channel_args, args_.interested_parties, + handshake_mgr_.get()); grpc_endpoint_add_to_pollset_set(endpoint_, args_.interested_parties); handshake_mgr_->DoHandshake(endpoint_, args_.channel_args, args_.deadline, nullptr /* acceptor */, OnHandshakeDone, this); diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.cc b/src/core/ext/transport/chttp2/server/chttp2_server.cc index ed5f2146244..7919203b58f 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.cc +++ b/src/core/ext/transport/chttp2/server/chttp2_server.cc @@ -42,7 +42,7 @@ #include "src/core/lib/address_utils/sockaddr_utils.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/handshaker.h" -#include "src/core/lib/channel/handshaker_registry.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/endpoint.h" @@ -320,8 +320,8 @@ Chttp2ServerListener::ActiveConnection::HandshakingState::HandshakingState( interested_parties_(grpc_pollset_set_create()), channel_resource_user_(channel_resource_user) { grpc_pollset_set_add_pollset(interested_parties_, accepting_pollset_); - HandshakerRegistry::AddHandshakers(HANDSHAKER_SERVER, args, - interested_parties_, handshake_mgr_.get()); + CoreConfiguration::Get().handshaker_registry().AddHandshakers( + HANDSHAKER_SERVER, args, interested_parties_, handshake_mgr_.get()); } Chttp2ServerListener::ActiveConnection::HandshakingState::~HandshakingState() { diff --git a/src/core/lib/channel/handshaker_factory.h b/src/core/lib/channel/handshaker_factory.h index 520e5e4b258..0d55a6d79ac 100644 --- a/src/core/lib/channel/handshaker_factory.h +++ b/src/core/lib/channel/handshaker_factory.h @@ -23,12 +23,20 @@ #include -#include "src/core/lib/channel/handshaker.h" - // A handshaker factory is used to create handshakers. +// TODO(ctiller): grpc_pollset_set and HandshakeManager are forward declared in +// this file. grpc_pollset_set ought to be eliminated when EventEngine lands IO +// support. At the same time, we ought to be able to include handshake_manager.h +// here and eliminate the HandshakeManager dependency - we cannot right now +// because HandshakeManager names too many iomgr types. + +typedef struct grpc_pollset_set grpc_pollset_set; + namespace grpc_core { +class HandshakeManager; + class HandshakerFactory { public: virtual void AddHandshakers(const grpc_channel_args* args, diff --git a/src/core/lib/channel/handshaker_registry.cc b/src/core/lib/channel/handshaker_registry.cc index e8fadb14208..d9d7bc9db64 100644 --- a/src/core/lib/channel/handshaker_registry.cc +++ b/src/core/lib/channel/handshaker_registry.cc @@ -20,88 +20,31 @@ #include "src/core/lib/channel/handshaker_registry.h" -#include - -#include - -#include "absl/container/inlined_vector.h" - -#include - -#include "src/core/lib/gpr/alloc.h" -#include "src/core/lib/gprpp/memory.h" - -// -// grpc_handshaker_factory_list -// - namespace grpc_core { -namespace { - -class HandshakerFactoryList { - public: - void Register(bool at_start, std::unique_ptr factory); - void AddHandshakers(const grpc_channel_args* args, - grpc_pollset_set* interested_parties, - HandshakeManager* handshake_mgr); - - private: - absl::InlinedVector, 2> factories_; -}; - -HandshakerFactoryList* g_handshaker_factory_lists = nullptr; - -} // namespace - -void HandshakerFactoryList::Register( - bool at_start, std::unique_ptr factory) { - factories_.push_back(std::move(factory)); - if (at_start) { - auto* end = &factories_[factories_.size() - 1]; - std::rotate(&factories_[0], end, end + 1); - } +void HandshakerRegistry::Builder::RegisterHandshakerFactory( + bool at_start, HandshakerType handshaker_type, + std::unique_ptr factory) { + auto& vec = factories_[handshaker_type]; + auto where = at_start ? vec.begin() : vec.end(); + vec.insert(where, std::move(factory)); } -void HandshakerFactoryList::AddHandshakers(const grpc_channel_args* args, - grpc_pollset_set* interested_parties, - HandshakeManager* handshake_mgr) { - for (size_t idx = 0; idx < factories_.size(); ++idx) { - auto& handshaker_factory = factories_[idx]; - handshaker_factory->AddHandshakers(args, interested_parties, handshake_mgr); +HandshakerRegistry HandshakerRegistry::Builder::Build() { + HandshakerRegistry out; + for (size_t i = 0; i < NUM_HANDSHAKER_TYPES; i++) { + out.factories_[i] = std::move(factories_[i]); } -} - -// -// plugin -// - -void HandshakerRegistry::Init() { - GPR_ASSERT(g_handshaker_factory_lists == nullptr); - g_handshaker_factory_lists = new HandshakerFactoryList[NUM_HANDSHAKER_TYPES]; -} - -void HandshakerRegistry::Shutdown() { - GPR_ASSERT(g_handshaker_factory_lists != nullptr); - delete[] g_handshaker_factory_lists; - g_handshaker_factory_lists = nullptr; -} - -void HandshakerRegistry::RegisterHandshakerFactory( - bool at_start, HandshakerType handshaker_type, - std::unique_ptr factory) { - GPR_ASSERT(g_handshaker_factory_lists != nullptr); - auto& factory_list = g_handshaker_factory_lists[handshaker_type]; - factory_list.Register(at_start, std::move(factory)); + return out; } void HandshakerRegistry::AddHandshakers(HandshakerType handshaker_type, const grpc_channel_args* args, grpc_pollset_set* interested_parties, - HandshakeManager* handshake_mgr) { - GPR_ASSERT(g_handshaker_factory_lists != nullptr); - auto& factory_list = g_handshaker_factory_lists[handshaker_type]; - factory_list.AddHandshakers(args, interested_parties, handshake_mgr); + HandshakeManager* handshake_mgr) const { + for (const auto& factory : factories_[handshaker_type]) { + factory->AddHandshakers(args, interested_parties, handshake_mgr); + } } } // namespace grpc_core diff --git a/src/core/lib/channel/handshaker_registry.h b/src/core/lib/channel/handshaker_registry.h index 4a02efc3f24..ccd61412c56 100644 --- a/src/core/lib/channel/handshaker_registry.h +++ b/src/core/lib/channel/handshaker_registry.h @@ -21,6 +21,9 @@ #include +#include +#include + #include #include "src/core/lib/channel/handshaker_factory.h" @@ -35,18 +38,32 @@ typedef enum { class HandshakerRegistry { public: - /// Registers a new handshaker factory. Takes ownership. - /// If \a at_start is true, the new handshaker will be at the beginning of - /// the list. Otherwise, it will be added to the end. - static void RegisterHandshakerFactory( - bool at_start, HandshakerType handshaker_type, - std::unique_ptr factory); - static void AddHandshakers(HandshakerType handshaker_type, - const grpc_channel_args* args, - grpc_pollset_set* interested_parties, - HandshakeManager* handshake_mgr); - static void Init(); - static void Shutdown(); + class Builder { + public: + /// Registers a new handshaker factory. Takes ownership. + /// If \a at_start is true, the new handshaker will be at the beginning of + /// the list. Otherwise, it will be added to the end. + void RegisterHandshakerFactory(bool at_start, + HandshakerType handshaker_type, + std::unique_ptr factory); + + HandshakerRegistry Build(); + + private: + std::vector> + factories_[NUM_HANDSHAKER_TYPES]; + }; + + void AddHandshakers(HandshakerType handshaker_type, + const grpc_channel_args* args, + grpc_pollset_set* interested_parties, + HandshakeManager* handshake_mgr) const; + + private: + HandshakerRegistry() = default; + + std::vector> + factories_[NUM_HANDSHAKER_TYPES]; }; } // namespace grpc_core diff --git a/src/core/lib/config/core_configuration.cc b/src/core/lib/config/core_configuration.cc index 09f8c08f5ae..61f29e1436d 100644 --- a/src/core/lib/config/core_configuration.cc +++ b/src/core/lib/config/core_configuration.cc @@ -23,10 +23,11 @@ std::atomic CoreConfiguration::config_{nullptr}; CoreConfiguration::Builder::Builder() = default; CoreConfiguration* CoreConfiguration::Builder::Build() { - return new CoreConfiguration; + return new CoreConfiguration(this); } -CoreConfiguration::CoreConfiguration() = default; +CoreConfiguration::CoreConfiguration(Builder* builder) + : handshaker_registry_(builder->handshaker_registry_.Build()) {} const CoreConfiguration& CoreConfiguration::BuildNewAndMaybeSet() { // Construct builder, pass it up to code that knows about build configuration @@ -38,8 +39,8 @@ const CoreConfiguration& CoreConfiguration::BuildNewAndMaybeSet() { // here, in which case we drop the work we did and use the one that got set // first CoreConfiguration* expected = nullptr; - if (!config_.compare_exchange_strong(expected, p, - std::memory_order_release)) { + if (!config_.compare_exchange_strong(expected, p, std::memory_order_acq_rel, + std::memory_order_acquire)) { delete p; return *expected; } diff --git a/src/core/lib/config/core_configuration.h b/src/core/lib/config/core_configuration.h index 4927899102d..db8530b4b7c 100644 --- a/src/core/lib/config/core_configuration.h +++ b/src/core/lib/config/core_configuration.h @@ -19,6 +19,8 @@ #include +#include "src/core/lib/channel/handshaker_registry.h" + namespace grpc_core { // Global singleton that stores library configuration - factories, etc... @@ -32,9 +34,15 @@ class CoreConfiguration { // their configuration and assemble the published CoreConfiguration. class Builder { public: + HandshakerRegistry::Builder* handshaker_registry() { + return &handshaker_registry_; + } + private: friend class CoreConfiguration; + HandshakerRegistry::Builder handshaker_registry_; + Builder(); CoreConfiguration* Build(); }; @@ -50,14 +58,38 @@ class CoreConfiguration { return BuildNewAndMaybeSet(); } + // Build a special core configuration. + // Requires no concurrent Get() be called. + // Doesn't call the regular BuildCoreConfiguration function, instead calls + // `build`. + // BuildFunc is a callable that takes a Builder* and returns void. + // We use a template instead of std::function to avoid + // including std::function in this widely used header, and to ensure no code + // is generated in programs that do not use this function. + // This is sometimes useful for testing. + template + static void BuildSpecialConfiguration(BuildFunc build) { + // Build bespoke configuration + Builder builder; + build(&builder); + CoreConfiguration* p = builder.Build(); + // Swap in final configuration, deleting anything that was already present. + delete config_.exchange(p, std::memory_order_release); + } + // Drop the core configuration. Users must ensure no other threads are // accessing the configuration. + // Clears any dynamically registered builders. static void Reset(); // Accessors + const HandshakerRegistry& handshaker_registry() const { + return handshaker_registry_; + } + private: - CoreConfiguration(); + explicit CoreConfiguration(Builder* builder); // Create a new CoreConfiguration, and either set it or throw it away. // We allow multiple CoreConfiguration's to be created in parallel. @@ -65,6 +97,8 @@ class CoreConfiguration { // The configuration static std::atomic config_; + + HandshakerRegistry handshaker_registry_; }; extern void BuildCoreConfiguration(CoreConfiguration::Builder* builder); diff --git a/src/core/lib/http/httpcli_security_connector.cc b/src/core/lib/http/httpcli_security_connector.cc index 877b0861411..ed8e624e2f4 100644 --- a/src/core/lib/http/httpcli_security_connector.cc +++ b/src/core/lib/http/httpcli_security_connector.cc @@ -28,7 +28,7 @@ #include #include "src/core/lib/channel/channel_args.h" -#include "src/core/lib/channel/handshaker_registry.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/http/httpcli.h" @@ -205,7 +205,7 @@ static void ssl_handshake(void* arg, grpc_endpoint* tcp, const char* host, grpc_arg channel_arg = grpc_security_connector_to_arg(sc.get()); grpc_channel_args args = {1, &channel_arg}; c->handshake_mgr = grpc_core::MakeRefCounted(); - grpc_core::HandshakerRegistry::AddHandshakers( + grpc_core::CoreConfiguration::Get().handshaker_registry().AddHandshakers( grpc_core::HANDSHAKER_CLIENT, &args, /*interested_parties=*/nullptr, c->handshake_mgr.get()); c->handshake_mgr->DoHandshake(tcp, /*channel_args=*/nullptr, deadline, diff --git a/src/core/lib/security/transport/security_handshaker.cc b/src/core/lib/security/transport/security_handshaker.cc index 932fa1845b7..d062e90fdc4 100644 --- a/src/core/lib/security/transport/security_handshaker.cc +++ b/src/core/lib/security/transport/security_handshaker.cc @@ -32,7 +32,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/channelz.h" #include "src/core/lib/channel/handshaker.h" -#include "src/core/lib/channel/handshaker_registry.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/security/context/security_context.h" #include "src/core/lib/security/transport/secure_endpoint.h" @@ -594,11 +594,11 @@ RefCountedPtr SecurityHandshakerCreate( } } -void SecurityRegisterHandshakerFactories() { - HandshakerRegistry::RegisterHandshakerFactory( +void SecurityRegisterHandshakerFactories(CoreConfiguration::Builder* builder) { + builder->handshaker_registry()->RegisterHandshakerFactory( false /* at_start */, HANDSHAKER_CLIENT, absl::make_unique()); - HandshakerRegistry::RegisterHandshakerFactory( + builder->handshaker_registry()->RegisterHandshakerFactory( false /* at_start */, HANDSHAKER_SERVER, absl::make_unique()); } diff --git a/src/core/lib/security/transport/security_handshaker.h b/src/core/lib/security/transport/security_handshaker.h index a9e1fe83d45..8f4ab8b2eb4 100644 --- a/src/core/lib/security/transport/security_handshaker.h +++ b/src/core/lib/security/transport/security_handshaker.h @@ -22,6 +22,7 @@ #include #include "src/core/lib/channel/handshaker.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/security/security_connector/security_connector.h" namespace grpc_core { @@ -32,7 +33,7 @@ RefCountedPtr SecurityHandshakerCreate( const grpc_channel_args* args); /// Registers security handshaker factories. -void SecurityRegisterHandshakerFactories(); +void SecurityRegisterHandshakerFactories(CoreConfiguration::Builder*); } // namespace grpc_core diff --git a/src/core/lib/surface/init.cc b/src/core/lib/surface/init.cc index 5830b8c9d44..961de6469c8 100644 --- a/src/core/lib/surface/init.cc +++ b/src/core/lib/surface/init.cc @@ -32,7 +32,6 @@ #include "src/core/lib/channel/channel_stack.h" #include "src/core/lib/channel/channelz_registry.h" #include "src/core/lib/channel/connected_channel.h" -#include "src/core/lib/channel/handshaker_registry.h" #include "src/core/lib/debug/stats.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/fork.h" @@ -145,8 +144,6 @@ void grpc_init(void) { grpc_core::ExecCtx::GlobalInit(); grpc_iomgr_init(); gpr_timers_global_init(); - grpc_core::HandshakerRegistry::Init(); - grpc_security_init(); for (int i = 0; i < g_number_of_plugins; i++) { if (g_all_of_the_plugins[i].init != nullptr) { g_all_of_the_plugins[i].init(); @@ -183,7 +180,6 @@ void grpc_shutdown_internal_locked(void) gpr_timers_global_destroy(); grpc_tracer_shutdown(); grpc_mdctx_global_shutdown(); - grpc_core::HandshakerRegistry::Shutdown(); grpc_slice_intern_shutdown(); grpc_core::channelz::ChannelzRegistry::Shutdown(); grpc_stats_shutdown(); diff --git a/src/core/lib/surface/init.h b/src/core/lib/surface/init.h index 9e8bedcb519..8982e28aae7 100644 --- a/src/core/lib/surface/init.h +++ b/src/core/lib/surface/init.h @@ -30,7 +30,6 @@ class EventEngine; void grpc_register_security_filters(void); void grpc_security_pre_init(void); -void grpc_security_init(void); void grpc_maybe_wait_for_async_shutdown(void); #endif /* GRPC_CORE_LIB_SURFACE_INIT_H */ diff --git a/src/core/lib/surface/init_secure.cc b/src/core/lib/surface/init_secure.cc index 609f868a0c8..0db0ebcbe26 100644 --- a/src/core/lib/surface/init_secure.cc +++ b/src/core/lib/surface/init_secure.cc @@ -98,5 +98,3 @@ void grpc_register_security_filters(void) { maybe_prepend_sdk_server_authz_filter, nullptr); } - -void grpc_security_init() { grpc_core::SecurityRegisterHandshakerFactories(); } diff --git a/src/core/lib/surface/init_unsecure.cc b/src/core/lib/surface/init_unsecure.cc index 7b86d50dcf7..c9e1688d42a 100644 --- a/src/core/lib/surface/init_unsecure.cc +++ b/src/core/lib/surface/init_unsecure.cc @@ -25,9 +25,3 @@ void grpc_security_pre_init(void) {} void grpc_register_security_filters(void) {} - -void grpc_security_init(void) { - gpr_log(GPR_DEBUG, - "Using insecure gRPC build. Security handshakers will not be invoked " - "even if secure credentials are used."); -} diff --git a/src/core/plugin_registry/grpc_plugin_registry.cc b/src/core/plugin_registry/grpc_plugin_registry.cc index c6a99c707dd..6a790f33fd9 100644 --- a/src/core/plugin_registry/grpc_plugin_registry.cc +++ b/src/core/plugin_registry/grpc_plugin_registry.cc @@ -165,8 +165,12 @@ void grpc_register_built_in_plugins(void) { namespace grpc_core { -void BuildCoreConfiguration(CoreConfiguration::Builder*) { - // TODO(ctiller): Incrementally call out to plugins as we require them to register things with builder. -} +extern void BuildClientChannelConfiguration(CoreConfiguration::Builder* builder); +extern void SecurityRegisterHandshakerFactories(CoreConfiguration::Builder* builder); +void BuildCoreConfiguration(CoreConfiguration::Builder* builder) { + BuildClientChannelConfiguration(builder); + SecurityRegisterHandshakerFactories(builder); } + +} // namespace grpc_core diff --git a/src/core/plugin_registry/grpc_unsecure_plugin_registry.cc b/src/core/plugin_registry/grpc_unsecure_plugin_registry.cc index e4b7c0a1824..86edc86d5e9 100644 --- a/src/core/plugin_registry/grpc_unsecure_plugin_registry.cc +++ b/src/core/plugin_registry/grpc_unsecure_plugin_registry.cc @@ -118,8 +118,10 @@ void grpc_register_built_in_plugins(void) { namespace grpc_core { -void BuildCoreConfiguration(CoreConfiguration::Builder*) { - // TODO(ctiller): Incrementally call out to plugins as we require them to register things with builder. +extern void BuildClientChannelConfiguration(CoreConfiguration::Builder* builder); + +void BuildCoreConfiguration(CoreConfiguration::Builder* builder) { + BuildClientChannelConfiguration(builder); } } diff --git a/test/core/handshake/readahead_handshaker_server_ssl.cc b/test/core/handshake/readahead_handshaker_server_ssl.cc index 66eec9ecd8e..120490e7e03 100644 --- a/test/core/handshake/readahead_handshaker_server_ssl.cc +++ b/test/core/handshake/readahead_handshaker_server_ssl.cc @@ -33,6 +33,7 @@ #include "src/core/lib/channel/handshaker_factory.h" #include "src/core/lib/channel/handshaker_registry.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/iomgr/load_file.h" #include "src/core/lib/security/transport/security_handshaker.h" #include "test/core/handshake/server_ssl_common.h" @@ -76,10 +77,15 @@ class ReadAheadHandshakerFactory : public HandshakerFactory { } // namespace grpc_core int main(int /*argc*/, char* /*argv*/[]) { + grpc_core::CoreConfiguration::BuildSpecialConfiguration( + [](grpc_core::CoreConfiguration::Builder* builder) { + BuildCoreConfiguration(builder); + builder->handshaker_registry()->RegisterHandshakerFactory( + true /* at_start */, grpc_core::HANDSHAKER_SERVER, + absl::make_unique()); + }); + grpc_init(); - grpc_core::HandshakerRegistry::RegisterHandshakerFactory( - true /* at_start */, grpc_core::HANDSHAKER_SERVER, - absl::make_unique()); const char* full_alpn_list[] = {"grpc-exp", "h2"}; GPR_ASSERT(server_ssl_test(full_alpn_list, 2, "grpc-exp")); CleanupSslLibrary();