From c1f9199f9ce8758849276cafd14b365145cb4ce1 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 18 Mar 2024 12:26:29 -0700 Subject: [PATCH] [channel] canonify target and set channel arg in only one place (#36134) Channel target will now be reported in canonified form (e.g., in channelz). Closes #36134 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36134 from markdroth:channel_target 22e49e73f82b5ac5dcb35ceb4b048946d9de11b3 PiperOrigin-RevId: 616911536 --- BUILD | 1 + .../transport/binder/client/channel_create.cc | 26 +++++-------------- .../binder/client/channel_create_impl.cc | 11 ++++---- .../binder/client/channel_create_impl.h | 3 ++- .../client/chaotic_good_connector.cc | 4 --- .../chttp2/client/chttp2_connector.cc | 7 +---- src/core/lib/surface/channel_create.cc | 9 +++++++ src/php/tests/unit_tests/CallInvokerTest.php | 2 +- .../channelz/channelz_servicer_test.py | 2 +- 9 files changed, 27 insertions(+), 38 deletions(-) diff --git a/BUILD b/BUILD index 747f134371c..2c3bb0b7d9e 100644 --- a/BUILD +++ b/BUILD @@ -1790,6 +1790,7 @@ grpc_cc_library( "channel", "channel_arg_names", "channelz", + "config", "gpr", "grpc_base", "grpc_public_hdrs", diff --git a/src/core/ext/transport/binder/client/channel_create.cc b/src/core/ext/transport/binder/client/channel_create.cc index b22176e090d..efae0104b28 100644 --- a/src/core/ext/transport/binder/client/channel_create.cc +++ b/src/core/ext/transport/binder/client/channel_create.cc @@ -121,33 +121,21 @@ std::shared_ptr CreateCustomBinderChannel( grpc_binder::TryEstablishConnectionWithUri(static_cast(jni_env_void), context, uri, connection_id); - grpc_channel_args channel_args; - args.SetChannelArgs(&channel_args); + grpc_binder::GetSecurityPolicySetting()->Set(connection_id, security_policy); // Set server URI to a URI that contains connection id. The URI will be used // by subchannel connector to obtain correct endpoint binder from // `EndpointBinderPool`. - grpc_channel_args* new_args; - { - string server_uri = "binder:" + connection_id; - grpc_arg server_uri_arg = - grpc_channel_arg_string_create(const_cast(GRPC_ARG_SERVER_URI), - const_cast(server_uri.c_str())); - const char* to_remove[] = {GRPC_ARG_SERVER_URI}; - new_args = grpc_channel_args_copy_and_add_and_remove( - &channel_args, to_remove, 1, &server_uri_arg, 1); - } + string server_uri = "binder:" + connection_id; - grpc_binder::GetSecurityPolicySetting()->Set(connection_id, security_policy); + grpc_channel_args channel_args; + args.SetChannelArgs(&channel_args); - auto channel = CreateChannelInternal( - "", grpc::internal::CreateClientBinderChannelImpl(new_args), + return CreateChannelInternal( + "", + grpc::internal::CreateClientBinderChannelImpl(server_uri, &channel_args), std::vector< std::unique_ptr>()); - - grpc_channel_args_destroy(new_args); - - return channel; } bool InitializeBinderChannelJavaClass(void* jni_env_void) { diff --git a/src/core/ext/transport/binder/client/channel_create_impl.cc b/src/core/ext/transport/binder/client/channel_create_impl.cc index 8e86d722f03..d9d5208f9a9 100644 --- a/src/core/ext/transport/binder/client/channel_create_impl.cc +++ b/src/core/ext/transport/binder/client/channel_create_impl.cc @@ -65,7 +65,8 @@ grpc_channel* CreateDirectBinderChannelImplForTesting( return channel->release()->c_ptr(); } -grpc_channel* CreateClientBinderChannelImpl(const grpc_channel_args* args) { +grpc_channel* CreateClientBinderChannelImpl(std::string target, + const grpc_channel_args* args) { grpc_core::ExecCtx exec_ctx; gpr_once_init(&g_factory_once, FactoryInit); @@ -75,14 +76,12 @@ grpc_channel* CreateClientBinderChannelImpl(const grpc_channel_args* args) { .PreconditionChannelArgs(args) .SetObject(g_factory); - auto channel = - grpc_core::ChannelCreate("binder_channel_target_placeholder", - channel_args, GRPC_CLIENT_CHANNEL, nullptr); + auto channel = grpc_core::ChannelCreate(target, channel_args, + GRPC_CLIENT_CHANNEL, nullptr); if (!channel.ok()) { return grpc_lame_client_channel_create( - "binder_channel_target_placeholder", - static_cast(channel.status().code()), + target.c_str(), static_cast(channel.status().code()), "Failed to create binder channel"); } diff --git a/src/core/ext/transport/binder/client/channel_create_impl.h b/src/core/ext/transport/binder/client/channel_create_impl.h index b88d39c2d27..7f00a5e1e09 100644 --- a/src/core/ext/transport/binder/client/channel_create_impl.h +++ b/src/core/ext/transport/binder/client/channel_create_impl.h @@ -34,7 +34,8 @@ grpc_channel* CreateDirectBinderChannelImplForTesting( security_policy); // Creates a GRPC_CLIENT_CHANNEL channel -grpc_channel* CreateClientBinderChannelImpl(const grpc_channel_args* args); +grpc_channel* CreateClientBinderChannelImpl(std::string target, + const grpc_channel_args* args); } // namespace internal } // namespace grpc diff --git a/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc b/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc index 4fc5cc9dc56..47f8af13760 100644 --- a/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc +++ b/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc @@ -370,15 +370,11 @@ grpc_channel* grpc_chaotic_good_channel_create(const char* target, grpc_channel* channel = nullptr; grpc_error_handle error; // Create channel. - std::string canonical_target = grpc_core::CoreConfiguration::Get() - .resolver_registry() - .AddDefaultPrefixIfNeeded(target); auto r = grpc_core::ChannelCreate( target, grpc_core::CoreConfiguration::Get() .channel_args_preconditioning() .PreconditionChannelArgs(args) - .Set(GRPC_ARG_SERVER_URI, canonical_target) .SetObject( grpc_core::NoDestructSingleton< grpc_core::chaotic_good::ChaoticGoodChannelFactory>::Get()), diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.cc b/src/core/ext/transport/chttp2/client/chttp2_connector.cc index 17edffae52a..93940b5091e 100644 --- a/src/core/ext/transport/chttp2/client/chttp2_connector.cc +++ b/src/core/ext/transport/chttp2/client/chttp2_connector.cc @@ -314,12 +314,7 @@ absl::StatusOr> CreateChannel(const char* target, gpr_log(GPR_ERROR, "cannot create channel with NULL target name"); return absl::InvalidArgumentError("channel target is NULL"); } - // Add channel arg containing the server URI. - std::string canonical_target = - CoreConfiguration::Get().resolver_registry().AddDefaultPrefixIfNeeded( - target); - return ChannelCreate(target, args.Set(GRPC_ARG_SERVER_URI, canonical_target), - GRPC_CLIENT_CHANNEL, nullptr); + return ChannelCreate(target, args, GRPC_CLIENT_CHANNEL, nullptr); } } // namespace diff --git a/src/core/lib/surface/channel_create.cc b/src/core/lib/surface/channel_create.cc index c98da2824b9..7c67e20652c 100644 --- a/src/core/lib/surface/channel_create.cc +++ b/src/core/lib/surface/channel_create.cc @@ -22,6 +22,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/channelz.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/debug/stats.h" #include "src/core/lib/debug/stats_data.h" #include "src/core/lib/surface/channel.h" @@ -34,6 +35,14 @@ absl::StatusOr> ChannelCreate( std::string target, ChannelArgs args, grpc_channel_stack_type channel_stack_type, Transport* optional_transport) { global_stats().IncrementClientChannelsCreated(); + // For client channels, canonify target string and add channel arg. + // Note: We don't do this for direct channels or lame channels. + if (channel_stack_type == GRPC_CLIENT_CHANNEL) { + target = + CoreConfiguration::Get().resolver_registry().AddDefaultPrefixIfNeeded( + target); + args = args.Set(GRPC_ARG_SERVER_URI, target); + } // Set default authority if needed. if (!args.GetString(GRPC_ARG_DEFAULT_AUTHORITY).has_value()) { auto ssl_override = args.GetString(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG); diff --git a/src/php/tests/unit_tests/CallInvokerTest.php b/src/php/tests/unit_tests/CallInvokerTest.php index 52af5b1b72f..5fa22858f45 100644 --- a/src/php/tests/unit_tests/CallInvokerTest.php +++ b/src/php/tests/unit_tests/CallInvokerTest.php @@ -194,7 +194,7 @@ class CallInvokerTest extends \PHPUnit\Framework\TestCase $stub = new \Grpc\BaseStub('localhost:50051', ['credentials' => \Grpc\ChannelCredentials::createInsecure(), 'grpc_call_invoker' => $call_invoker]); - $this->assertEquals($call_invoker->getChannel()->getTarget(), 'localhost:50050'); + $this->assertEquals($call_invoker->getChannel()->getTarget(), 'dns:///localhost:50050'); $call_invoker->getChannel()->close(); } diff --git a/src/python/grpcio_tests/tests_aio/channelz/channelz_servicer_test.py b/src/python/grpcio_tests/tests_aio/channelz/channelz_servicer_test.py index 12b8fd41632..07cfe67427c 100644 --- a/src/python/grpcio_tests/tests_aio/channelz/channelz_servicer_test.py +++ b/src/python/grpcio_tests/tests_aio/channelz/channelz_servicer_test.py @@ -94,7 +94,7 @@ class _ChannelServerPair: channelz_pb2.GetTopChannelsRequest(start_channel_id=0) ) for channel in resp.channel: - if channel.data.target == self.address: + if channel.data.target == "dns:///" + self.address: self.channel_ref_id = channel.ref.channel_id resp = await channelz_stub.GetServers(