From 6e3938c829b00d4a6db542f3ebadef2e57819e10 Mon Sep 17 00:00:00 2001 From: Nathan Herring Date: Thu, 9 Aug 2018 11:11:37 -0500 Subject: This replaces mutually-exclusive flags for a single selector flag which defines the call credentials to composite over any channel credentials. It is the call credentials version of #16204. Fixes issue #16205. --- test/cpp/util/cli_credentials.cc | 69 ++++++++++++++++++++++++++++++++++------ test/cpp/util/cli_credentials.h | 3 ++ 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/test/cpp/util/cli_credentials.cc b/test/cpp/util/cli_credentials.cc index acf4ef8ef1..0a922617bb 100644 --- a/test/cpp/util/cli_credentials.cc +++ b/test/cpp/util/cli_credentials.cc @@ -28,7 +28,8 @@ DEFINE_bool(use_auth, false, "--channel_creds_type=gdc."); DEFINE_string( access_token, "", - "The access token that will be sent to the server to authenticate RPCs."); + "The access token that will be sent to the server to authenticate RPCs. " + "Deprecated. Use --call_creds=access_token=."); DEFINE_string( ssl_target, "", "If not empty, treat the server host name as this for ssl/tls certificate " @@ -37,10 +38,34 @@ DEFINE_string( channel_creds_type, "", "The channel creds type: insecure, ssl, gdc (Google Default Credentials) " "or alts."); +DEFINE_string( + call_creds, "", + "Call credentials to use: none (default), or access_token=. If " + "provided, the call creds are composited on top of channel creds."); namespace grpc { namespace testing { +namespace { + +const char ACCESS_TOKEN_PREFIX[] = "access_token="; +constexpr int ACCESS_TOKEN_PREFIX_LEN = + sizeof(ACCESS_TOKEN_PREFIX) / sizeof(*ACCESS_TOKEN_PREFIX) - 1; + +bool IsAccessToken(const grpc::string& auth) { + return auth.length() > ACCESS_TOKEN_PREFIX_LEN && + auth.compare(0, ACCESS_TOKEN_PREFIX_LEN, ACCESS_TOKEN_PREFIX) == 0; +} + +grpc::string AccessToken(const grpc::string& auth) { + if (!IsAccessToken(auth)) { + return ""; + } + return grpc::string(auth, ACCESS_TOKEN_PREFIX_LEN); +} + +} // namespace + grpc::string CliCredentials::GetDefaultChannelCredsType() const { // Compatibility logic for --enable_ssl. if (FLAGS_enable_ssl) { @@ -59,6 +84,16 @@ grpc::string CliCredentials::GetDefaultChannelCredsType() const { return "insecure"; } +grpc::string CliCredentials::GetDefaultCallCreds() const { + if (!FLAGS_access_token.empty()) { + fprintf(stderr, + "warning: --access_token is deprecated. Use " + "--call_creds=access_token=.\n"); + return grpc::string("access_token=") + FLAGS_access_token; + } + return "none"; +} + std::shared_ptr CliCredentials::GetChannelCredentials() const { if (FLAGS_channel_creds_type.compare("insecure") == 0) { @@ -80,18 +115,30 @@ CliCredentials::GetChannelCredentials() const { std::shared_ptr CliCredentials::GetCallCredentials() const { - if (!FLAGS_access_token.empty()) { - if (FLAGS_use_auth) { - fprintf(stderr, - "warning: use_auth is ignored when access_token is provided."); - } - return grpc::AccessTokenCredentials(FLAGS_access_token); + if (IsAccessToken(FLAGS_call_creds)) { + return grpc::AccessTokenCredentials(AccessToken(FLAGS_call_creds)); } + if (FLAGS_call_creds.compare("none") != 0) { + // Nothing to do; creds, if any, are baked into the channel. + return std::shared_ptr(); + } + fprintf(stderr, + "--call_creds=%s invalid; must be none " + "or access_token=.\n", + FLAGS_call_creds.c_str()); return std::shared_ptr(); } std::shared_ptr CliCredentials::GetCredentials() const { + if (FLAGS_call_creds.empty()) { + FLAGS_call_creds = GetDefaultCallCreds(); + } else if (!FLAGS_access_token.empty() && !IsAccessToken(FLAGS_call_creds)) { + fprintf(stderr, + "warning: ignoring --access_token because --call_creds " + "already set to %s.\n", + FLAGS_call_creds.c_str()); + } if (FLAGS_channel_creds_type.empty()) { FLAGS_channel_creds_type = GetDefaultChannelCredsType(); } else if (FLAGS_enable_ssl && FLAGS_channel_creds_type.compare("ssl") != 0) { @@ -106,7 +153,7 @@ std::shared_ptr CliCredentials::GetCredentials() FLAGS_channel_creds_type.c_str()); } // Legacy transport upgrade logic for insecure requests. - if (!FLAGS_access_token.empty() && + if (IsAccessToken(FLAGS_call_creds) && FLAGS_channel_creds_type.compare("insecure") == 0) { fprintf(stderr, "warning: --channel_creds_type=insecure upgraded to ssl because " @@ -126,10 +173,14 @@ const grpc::string CliCredentials::GetCredentialUsage() const { return " --enable_ssl ; Set whether to use ssl (deprecated)\n" " --use_auth ; Set whether to create default google" " credentials\n" + " ; (deprecated)\n" " --access_token ; Set the access token in metadata," " overrides --use_auth\n" + " ; (deprecated)\n" " --ssl_target ; Set server host for ssl validation\n" - " --channel_creds_type ; Set to insecure, ssl, gdc, or alts\n"; + " --channel_creds_type ; Set to insecure, ssl, gdc, or alts\n" + " --call_creds ; Set to none, or" + " access_token=\n"; } const grpc::string CliCredentials::GetSslTargetNameOverride() const { diff --git a/test/cpp/util/cli_credentials.h b/test/cpp/util/cli_credentials.h index 4636d3ca14..472c7abef2 100644 --- a/test/cpp/util/cli_credentials.h +++ b/test/cpp/util/cli_credentials.h @@ -36,6 +36,9 @@ class CliCredentials { // Returns the appropriate channel_creds_type value for the set of legacy // flag arguments. virtual grpc::string GetDefaultChannelCredsType() const; + // Returns the appropriate call_creds value for the set of legacy flag + // arguments. + virtual grpc::string GetDefaultCallCreds() const; // Returns the base transport channel credentials. Child classes can override // to support additional channel_creds_types unknown to this base class. virtual std::shared_ptr GetChannelCredentials() -- cgit v1.2.3 From 7269fd47eb12f9383535c657b855286cd4570d9f Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 30 Aug 2018 11:37:34 -0700 Subject: Fix ipv6 address parsing issue --- src/core/ext/filters/client_channel/parse_address.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/ext/filters/client_channel/parse_address.cc b/src/core/ext/filters/client_channel/parse_address.cc index b3900114ad..5eeea1b148 100644 --- a/src/core/ext/filters/client_channel/parse_address.cc +++ b/src/core/ext/filters/client_channel/parse_address.cc @@ -128,6 +128,13 @@ bool grpc_parse_ipv6_hostport(const char* hostport, grpc_resolved_address* addr, char host_without_scope[GRPC_INET6_ADDRSTRLEN]; size_t host_without_scope_len = static_cast(host_end - host); uint32_t sin6_scope_id = 0; + if (host_without_scope_len > GRPC_INET6_ADDRSTRLEN) { + gpr_log(GPR_ERROR, + "invalid ipv6 address length %d. Length cannot be greater than " + "GRPC_INET6_ADDRSTRLEN i.e %d)", + host_without_scope_len, GRPC_INET6_ADDRSTRLEN); + goto done; + } strncpy(host_without_scope, host, host_without_scope_len); host_without_scope[host_without_scope_len] = '\0'; if (grpc_inet_pton(GRPC_AF_INET6, host_without_scope, &in6->sin6_addr) == -- cgit v1.2.3 From 0a7363ffcbbb578a7dc94f2bcffbe7b8ba78e1d2 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 30 Aug 2018 13:13:48 -0700 Subject: Add a test to parse invalid addresses --- src/core/ext/filters/client_channel/parse_address.cc | 2 +- test/core/client_channel/parse_address_test.cc | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/core/ext/filters/client_channel/parse_address.cc b/src/core/ext/filters/client_channel/parse_address.cc index 5eeea1b148..c1dc0bc2ce 100644 --- a/src/core/ext/filters/client_channel/parse_address.cc +++ b/src/core/ext/filters/client_channel/parse_address.cc @@ -125,7 +125,7 @@ bool grpc_parse_ipv6_hostport(const char* hostport, grpc_resolved_address* addr, char* host_end = static_cast(gpr_memrchr(host, '%', strlen(host))); if (host_end != nullptr) { GPR_ASSERT(host_end >= host); - char host_without_scope[GRPC_INET6_ADDRSTRLEN]; + char host_without_scope[GRPC_INET6_ADDRSTRLEN + 1]; size_t host_without_scope_len = static_cast(host_end - host); uint32_t sin6_scope_id = 0; if (host_without_scope_len > GRPC_INET6_ADDRSTRLEN) { diff --git a/test/core/client_channel/parse_address_test.cc b/test/core/client_channel/parse_address_test.cc index ae157fbb8b..798600293b 100644 --- a/test/core/client_channel/parse_address_test.cc +++ b/test/core/client_channel/parse_address_test.cc @@ -91,6 +91,14 @@ static void test_grpc_parse_ipv6(const char* uri_text, const char* host, grpc_uri_destroy(uri); } +/* Test parsing invalid ipv6 addresses (valid uri_text but invalid ipv6 addr) */ +static void test_grpc_parse_ipv6_invalid(const char* uri_text) { + grpc_core::ExecCtx exec_ctx; + grpc_uri* uri = grpc_uri_parse(uri_text, 0); + grpc_resolved_address addr; + GPR_ASSERT(!grpc_parse_ipv6(uri, &addr)); +} + int main(int argc, char** argv) { grpc_test_init(argc, argv); grpc_init(); @@ -100,5 +108,10 @@ int main(int argc, char** argv) { test_grpc_parse_ipv6("ipv6:[2001:db8::1]:12345", "2001:db8::1", 12345, 0); test_grpc_parse_ipv6("ipv6:[2001:db8::1%252]:12345", "2001:db8::1", 12345, 2); - grpc_shutdown(); + /* Address length greater than GRPC_INET6_ADDRSTRLEN */ + test_grpc_parse_ipv6_invalid( + "ipv6:WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW45%" + "v6:45%x$1*"); + + test_grpc_parse grpc_shutdown(); } -- cgit v1.2.3 From 50419a04ae9c1012a4eb6e159fdb225175e99bf6 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 30 Aug 2018 13:23:27 -0700 Subject: fix format specifier for size_t and and a typo in test --- src/core/ext/filters/client_channel/parse_address.cc | 2 +- test/core/client_channel/parse_address_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/ext/filters/client_channel/parse_address.cc b/src/core/ext/filters/client_channel/parse_address.cc index c1dc0bc2ce..7e726dd562 100644 --- a/src/core/ext/filters/client_channel/parse_address.cc +++ b/src/core/ext/filters/client_channel/parse_address.cc @@ -130,7 +130,7 @@ bool grpc_parse_ipv6_hostport(const char* hostport, grpc_resolved_address* addr, uint32_t sin6_scope_id = 0; if (host_without_scope_len > GRPC_INET6_ADDRSTRLEN) { gpr_log(GPR_ERROR, - "invalid ipv6 address length %d. Length cannot be greater than " + "invalid ipv6 address length %zu. Length cannot be greater than " "GRPC_INET6_ADDRSTRLEN i.e %d)", host_without_scope_len, GRPC_INET6_ADDRSTRLEN); goto done; diff --git a/test/core/client_channel/parse_address_test.cc b/test/core/client_channel/parse_address_test.cc index 798600293b..d51c6178f8 100644 --- a/test/core/client_channel/parse_address_test.cc +++ b/test/core/client_channel/parse_address_test.cc @@ -113,5 +113,5 @@ int main(int argc, char** argv) { "ipv6:WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW45%" "v6:45%x$1*"); - test_grpc_parse grpc_shutdown(); + grpc_shutdown(); } -- cgit v1.2.3 From 5e054bf11e58049d988746cb76238f8099ac1faa Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 4 Sep 2018 13:01:34 -0700 Subject: Stop unconditionally surfacing user agent to server --- src/core/ext/filters/http/server/http_server_filter.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/ext/filters/http/server/http_server_filter.cc b/src/core/ext/filters/http/server/http_server_filter.cc index 926afeec84..a238d5f989 100644 --- a/src/core/ext/filters/http/server/http_server_filter.cc +++ b/src/core/ext/filters/http/server/http_server_filter.cc @@ -262,6 +262,10 @@ static grpc_error* hs_filter_incoming_metadata(grpc_call_element* elem, GRPC_ERROR_STR_KEY, grpc_slice_from_static_string(":authority"))); } + if (b->idx.named.user_agent != nullptr) { + grpc_metadata_batch_remove(b, b->idx.named.user_agent); + } + return error; } -- cgit v1.2.3 From c9e300d5b044351c6e09221b09ba49e53d211643 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 5 Sep 2018 17:08:01 -0700 Subject: Add channel arg control for user agent --- include/grpc/impl/codegen/grpc_types.h | 2 ++ src/core/ext/filters/http/server/http_server_filter.cc | 15 +++++++++++++-- test/core/end2end/tests/workaround_cronet_compression.cc | 9 +++++++++ test/cpp/end2end/end2end_test.cc | 13 ++++++++++--- 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index b5353c1dea..9097984a96 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -342,6 +342,8 @@ typedef struct { "grpc.disable_client_authority_filter" /** If set to zero, disables use of http proxies. Enabled by default. */ #define GRPC_ARG_ENABLE_HTTP_PROXY "grpc.enable_http_proxy" +/** If set to non zero, surfaces the user agent string to the server. */ +#define GRPC_ARG_SURFACE_USER_AGENT "grpc.surface_user_agent" /** \} */ /** Result of a grpc call. If the caller satisfies the prerequisites of a diff --git a/src/core/ext/filters/http/server/http_server_filter.cc b/src/core/ext/filters/http/server/http_server_filter.cc index a238d5f989..53cd059aa8 100644 --- a/src/core/ext/filters/http/server/http_server_filter.cc +++ b/src/core/ext/filters/http/server/http_server_filter.cc @@ -23,6 +23,7 @@ #include #include #include +#include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gprpp/manual_constructor.h" #include "src/core/lib/profiling/timers.h" #include "src/core/lib/slice/b64.h" @@ -66,6 +67,10 @@ struct call_data { grpc_closure* original_recv_trailing_metadata_ready; }; +struct channel_data { + bool surface_user_agent; +}; + } // namespace static grpc_error* hs_filter_outgoing_metadata(grpc_call_element* elem, @@ -262,7 +267,8 @@ static grpc_error* hs_filter_incoming_metadata(grpc_call_element* elem, GRPC_ERROR_STR_KEY, grpc_slice_from_static_string(":authority"))); } - if (b->idx.named.user_agent != nullptr) { + channel_data* chand = static_cast(elem->channel_data); + if (!chand->surface_user_agent && b->idx.named.user_agent != nullptr) { grpc_metadata_batch_remove(b, b->idx.named.user_agent); } @@ -434,7 +440,12 @@ static void hs_destroy_call_elem(grpc_call_element* elem, /* Constructor for channel_data */ static grpc_error* hs_init_channel_elem(grpc_channel_element* elem, grpc_channel_element_args* args) { + channel_data* chand = static_cast(elem->channel_data); GPR_ASSERT(!args->is_last); + chand->surface_user_agent = grpc_channel_arg_get_bool( + grpc_channel_args_find(args->channel_args, + const_cast(GRPC_ARG_SURFACE_USER_AGENT)), + false); return GRPC_ERROR_NONE; } @@ -448,7 +459,7 @@ const grpc_channel_filter grpc_http_server_filter = { hs_init_call_elem, grpc_call_stack_ignore_set_pollset_or_pollset_set, hs_destroy_call_elem, - 0, + sizeof(channel_data), hs_init_channel_elem, hs_destroy_channel_elem, grpc_channel_next_get_info, diff --git a/test/core/end2end/tests/workaround_cronet_compression.cc b/test/core/end2end/tests/workaround_cronet_compression.cc index f44ddca3b1..e1bce603fa 100644 --- a/test/core/end2end/tests/workaround_cronet_compression.cc +++ b/test/core/end2end/tests/workaround_cronet_compression.cc @@ -149,6 +149,15 @@ static void request_with_payload_template( arg.value.string = user_agent_override; client_args = grpc_channel_args_copy_and_add(client_args_old, &arg, 1); grpc_channel_args_destroy(client_args_old); + // force grpc lib to pass the user agent back up to server. + grpc_channel_args* server_args_old = server_args; + grpc_arg server_arg; + server_arg.key = const_cast(GRPC_ARG_SURFACE_USER_AGENT); + server_arg.type = GRPC_ARG_INTEGER; + server_arg.value.integer = 1; + server_args = + grpc_channel_args_copy_and_add(server_args_old, &server_arg, 1); + grpc_channel_args_destroy(server_args_old); } f = begin_test(config, test_name, client_args, server_args); diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index fc07681535..e835f55738 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -244,15 +244,17 @@ class End2endTest : public ::testing::TestWithParam { BuildAndStartServer(processor); } - void RestartServer(const std::shared_ptr& processor) { + void RestartServer(const std::shared_ptr& processor, + bool surface_user_agent = false) { if (is_server_started_) { server_->Shutdown(); - BuildAndStartServer(processor); + BuildAndStartServer(processor, surface_user_agent); } } void BuildAndStartServer( - const std::shared_ptr& processor) { + const std::shared_ptr& processor, + bool surface_user_agent = false) { ServerBuilder builder; ConfigureServerBuilder(&builder); auto server_creds = GetCredentialsProvider()->GetServerCredentials( @@ -268,6 +270,9 @@ class End2endTest : public ::testing::TestWithParam { builder.SetSyncServerOption(ServerBuilder::SyncServerOption::NUM_CQS, 4); builder.SetSyncServerOption( ServerBuilder::SyncServerOption::CQ_TIMEOUT_MSEC, 10); + if (surface_user_agent) { + builder.AddChannelArgument(GRPC_ARG_SURFACE_USER_AGENT, 1); + } server_ = builder.BuildAndStart(); is_server_started_ = true; @@ -664,6 +669,8 @@ TEST_P(End2endTest, SimpleRpcWithCustomUserAgentPrefix) { } user_agent_prefix_ = "custom_prefix"; ResetStub(); + RestartServer(std::shared_ptr(), true); + ResetChannel(); EchoRequest request; EchoResponse response; request.set_message("Hello hello hello hello"); -- cgit v1.2.3 From 16f53ff583c23b3b2a260ba0935fd0c30b479acf Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 5 Sep 2018 18:16:13 -0700 Subject: Surface user agent by default --- include/grpc/impl/codegen/grpc_types.h | 3 ++- src/core/ext/filters/http/server/http_server_filter.cc | 2 +- test/core/end2end/tests/workaround_cronet_compression.cc | 9 --------- test/cpp/end2end/end2end_test.cc | 13 +++---------- 4 files changed, 6 insertions(+), 21 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 9097984a96..5f3b96f40b 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -342,7 +342,8 @@ typedef struct { "grpc.disable_client_authority_filter" /** If set to zero, disables use of http proxies. Enabled by default. */ #define GRPC_ARG_ENABLE_HTTP_PROXY "grpc.enable_http_proxy" -/** If set to non zero, surfaces the user agent string to the server. */ +/** If set to non zero, surfaces the user agent string to the server. User + agent is surfaced by default. */ #define GRPC_ARG_SURFACE_USER_AGENT "grpc.surface_user_agent" /** \} */ diff --git a/src/core/ext/filters/http/server/http_server_filter.cc b/src/core/ext/filters/http/server/http_server_filter.cc index 53cd059aa8..1b3426b120 100644 --- a/src/core/ext/filters/http/server/http_server_filter.cc +++ b/src/core/ext/filters/http/server/http_server_filter.cc @@ -445,7 +445,7 @@ static grpc_error* hs_init_channel_elem(grpc_channel_element* elem, chand->surface_user_agent = grpc_channel_arg_get_bool( grpc_channel_args_find(args->channel_args, const_cast(GRPC_ARG_SURFACE_USER_AGENT)), - false); + true); return GRPC_ERROR_NONE; } diff --git a/test/core/end2end/tests/workaround_cronet_compression.cc b/test/core/end2end/tests/workaround_cronet_compression.cc index e1bce603fa..f44ddca3b1 100644 --- a/test/core/end2end/tests/workaround_cronet_compression.cc +++ b/test/core/end2end/tests/workaround_cronet_compression.cc @@ -149,15 +149,6 @@ static void request_with_payload_template( arg.value.string = user_agent_override; client_args = grpc_channel_args_copy_and_add(client_args_old, &arg, 1); grpc_channel_args_destroy(client_args_old); - // force grpc lib to pass the user agent back up to server. - grpc_channel_args* server_args_old = server_args; - grpc_arg server_arg; - server_arg.key = const_cast(GRPC_ARG_SURFACE_USER_AGENT); - server_arg.type = GRPC_ARG_INTEGER; - server_arg.value.integer = 1; - server_args = - grpc_channel_args_copy_and_add(server_args_old, &server_arg, 1); - grpc_channel_args_destroy(server_args_old); } f = begin_test(config, test_name, client_args, server_args); diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index e835f55738..fc07681535 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -244,17 +244,15 @@ class End2endTest : public ::testing::TestWithParam { BuildAndStartServer(processor); } - void RestartServer(const std::shared_ptr& processor, - bool surface_user_agent = false) { + void RestartServer(const std::shared_ptr& processor) { if (is_server_started_) { server_->Shutdown(); - BuildAndStartServer(processor, surface_user_agent); + BuildAndStartServer(processor); } } void BuildAndStartServer( - const std::shared_ptr& processor, - bool surface_user_agent = false) { + const std::shared_ptr& processor) { ServerBuilder builder; ConfigureServerBuilder(&builder); auto server_creds = GetCredentialsProvider()->GetServerCredentials( @@ -270,9 +268,6 @@ class End2endTest : public ::testing::TestWithParam { builder.SetSyncServerOption(ServerBuilder::SyncServerOption::NUM_CQS, 4); builder.SetSyncServerOption( ServerBuilder::SyncServerOption::CQ_TIMEOUT_MSEC, 10); - if (surface_user_agent) { - builder.AddChannelArgument(GRPC_ARG_SURFACE_USER_AGENT, 1); - } server_ = builder.BuildAndStart(); is_server_started_ = true; @@ -669,8 +664,6 @@ TEST_P(End2endTest, SimpleRpcWithCustomUserAgentPrefix) { } user_agent_prefix_ = "custom_prefix"; ResetStub(); - RestartServer(std::shared_ptr(), true); - ResetChannel(); EchoRequest request; EchoResponse response; request.set_message("Hello hello hello hello"); -- cgit v1.2.3 From 3c1a3d9f1b4c8ac573c0234f71534fd74cf2ec4b Mon Sep 17 00:00:00 2001 From: Juanli Shen Date: Thu, 6 Sep 2018 15:06:17 -0700 Subject: Fix subchannel key comparison if forcing creation --- src/core/ext/filters/client_channel/subchannel_index.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/ext/filters/client_channel/subchannel_index.cc b/src/core/ext/filters/client_channel/subchannel_index.cc index cb02b1a748..4e155a2830 100644 --- a/src/core/ext/filters/client_channel/subchannel_index.cc +++ b/src/core/ext/filters/client_channel/subchannel_index.cc @@ -73,7 +73,8 @@ static grpc_subchannel_key* subchannel_key_copy(grpc_subchannel_key* k) { int grpc_subchannel_key_compare(const grpc_subchannel_key* a, const grpc_subchannel_key* b) { - if (g_force_creation) return false; + // To pretend the keys are different, return a non-zero value. + if (g_force_creation) return 1; int c = GPR_ICMP(a->args.filter_count, b->args.filter_count); if (c != 0) return c; if (a->args.filter_count > 0) { -- cgit v1.2.3 From bf53d1c8803b2b43bfb1756ce1c7f85326342418 Mon Sep 17 00:00:00 2001 From: Juanli Shen Date: Fri, 7 Sep 2018 14:21:41 -0700 Subject: Fix subchannel key comparison --- src/core/ext/filters/client_channel/subchannel_index.cc | 6 +++--- src/core/ext/filters/client_channel/subchannel_index.h | 5 +---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/core/ext/filters/client_channel/subchannel_index.cc b/src/core/ext/filters/client_channel/subchannel_index.cc index 4e155a2830..f2b6c24e8e 100644 --- a/src/core/ext/filters/client_channel/subchannel_index.cc +++ b/src/core/ext/filters/client_channel/subchannel_index.cc @@ -42,7 +42,7 @@ struct grpc_subchannel_key { grpc_subchannel_args args; }; -static bool g_force_creation = false; +static gpr_atm g_force_creation = false; static grpc_subchannel_key* create_key( const grpc_subchannel_args* args, @@ -74,7 +74,7 @@ static grpc_subchannel_key* subchannel_key_copy(grpc_subchannel_key* k) { int grpc_subchannel_key_compare(const grpc_subchannel_key* a, const grpc_subchannel_key* b) { // To pretend the keys are different, return a non-zero value. - if (g_force_creation) return 1; + if (GPR_UNLIKELY(gpr_atm_no_barrier_load(&g_force_creation))) return 1; int c = GPR_ICMP(a->args.filter_count, b->args.filter_count); if (c != 0) return c; if (a->args.filter_count > 0) { @@ -251,5 +251,5 @@ void grpc_subchannel_index_unregister(grpc_subchannel_key* key, } void grpc_subchannel_index_test_only_set_force_creation(bool force_creation) { - g_force_creation = force_creation; + gpr_atm_no_barrier_store(&g_force_creation, force_creation); } diff --git a/src/core/ext/filters/client_channel/subchannel_index.h b/src/core/ext/filters/client_channel/subchannel_index.h index a7dae9d47d..c135613d26 100644 --- a/src/core/ext/filters/client_channel/subchannel_index.h +++ b/src/core/ext/filters/client_channel/subchannel_index.h @@ -65,13 +65,10 @@ void grpc_subchannel_index_ref(void); void grpc_subchannel_index_unref(void); /** \em TEST ONLY. - * If \a force_creation is true, all key comparisons will be false, resulting in + * If \a force_creation is true, all keys are regarded different, resulting in * new subchannels always being created. Otherwise, the keys will be compared as * usual. * - * This function is *not* threadsafe on purpose: it should *only* be used in - * test code. - * * Tests using this function \em MUST run tests with and without \a * force_creation set. */ void grpc_subchannel_index_test_only_set_force_creation(bool force_creation); -- cgit v1.2.3 From 8cf33aa8fabfd9eeeb9e89f0796121d94cde7e34 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Sun, 9 Sep 2018 13:33:39 -0700 Subject: Fix asan bug --- test/core/client_channel/parse_address_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/core/client_channel/parse_address_test.cc b/test/core/client_channel/parse_address_test.cc index d51c6178f8..004549fa62 100644 --- a/test/core/client_channel/parse_address_test.cc +++ b/test/core/client_channel/parse_address_test.cc @@ -97,6 +97,7 @@ static void test_grpc_parse_ipv6_invalid(const char* uri_text) { grpc_uri* uri = grpc_uri_parse(uri_text, 0); grpc_resolved_address addr; GPR_ASSERT(!grpc_parse_ipv6(uri, &addr)); + grpc_uri_destroy(uri); } int main(int argc, char** argv) { -- cgit v1.2.3 From 2ff5be8c08c75a2c2c0152694788aa1e5ed3d50d Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 10 Sep 2018 10:14:05 -0700 Subject: reveiwer comments --- .../ext/filters/client_channel/client_channel.cc | 128 +++++++++++---------- src/core/lib/channel/connected_channel.cc | 6 +- 2 files changed, 68 insertions(+), 66 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 4691aa1c97..388736b60a 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -936,7 +936,7 @@ typedef struct client_channel_call_data { // state needed to support channelz interception of recv trailing metadata. grpc_closure recv_trailing_metadata_ready_channelz; grpc_closure* original_recv_trailing_metadata; - grpc_metadata_batch* recv_trailing_metadata_batch; + grpc_metadata_batch* recv_trailing_metadata; grpc_polling_entity* pollent; bool pollent_added_to_interested_parties; @@ -999,7 +999,7 @@ static void start_internal_recv_trailing_metadata(grpc_call_element* elem); static void on_complete(void* arg, grpc_error* error); static void start_retriable_subchannel_batches(void* arg, grpc_error* ignored); static void start_pick_locked(void* arg, grpc_error* ignored); -static void maybe_intercept_metadata_for_channelz( +static void maybe_intercept_recv_trailing_metadata_for_channelz( grpc_call_element* elem, grpc_transport_stream_op_batch* batch); // @@ -1299,7 +1299,7 @@ static void pending_batches_resume(grpc_call_element* elem) { pending_batch* pending = &calld->pending_batches[i]; grpc_transport_stream_op_batch* batch = pending->batch; if (batch != nullptr) { - maybe_intercept_metadata_for_channelz(elem, batch); + maybe_intercept_recv_trailing_metadata_for_channelz(elem, batch); batch->handler_private.extra_arg = calld->subchannel_call; GRPC_CLOSURE_INIT(&batch->handler_private.closure, resume_pending_batch_in_call_combiner, batch, @@ -2589,6 +2589,69 @@ static void start_retriable_subchannel_batches(void* arg, grpc_error* ignored) { closures.RunClosures(calld->call_combiner); } +// +// Channelz +// + +static void recv_trailing_metadata_ready_channelz(void* arg, + grpc_error* error) { + grpc_call_element* elem = static_cast(arg); + channel_data* chand = static_cast(elem->channel_data); + call_data* calld = static_cast(elem->call_data); + if (grpc_client_channel_trace.enabled()) { + gpr_log(GPR_INFO, + "chand=%p calld=%p: got recv_trailing_metadata_ready_channelz, " + "error=%s", + chand, calld, grpc_error_string(error)); + } + GPR_ASSERT(calld->recv_trailing_metadata != nullptr); + grpc_status_code status = GRPC_STATUS_OK; + grpc_metadata_batch* md_batch = calld->recv_trailing_metadata; + get_call_status(elem, md_batch, GRPC_ERROR_REF(error), &status, nullptr); + grpc_core::channelz::SubchannelNode* channelz_subchannel = + calld->pick.connected_subchannel->channelz_subchannel(); + GPR_ASSERT(channelz_subchannel != nullptr); + if (status == GRPC_STATUS_OK) { + channelz_subchannel->RecordCallSucceeded(); + } else { + channelz_subchannel->RecordCallFailed(); + } + calld->recv_trailing_metadata = nullptr; + GRPC_CLOSURE_RUN(calld->original_recv_trailing_metadata, error); +} + +// If channelz is enabled, intercept recv_trailing so that we may check the +// status and associate it to a subchannel. +// Returns true if callback was intercepted, false otherwise. +static void maybe_intercept_recv_trailing_metadata_for_channelz( + grpc_call_element* elem, grpc_transport_stream_op_batch* batch) { + call_data* calld = static_cast(elem->call_data); + // only intercept payloads with recv trailing. + if (!batch->recv_trailing_metadata) { + return; + } + // only add interceptor is channelz is enabled. + if (calld->pick.connected_subchannel->channelz_subchannel() == nullptr) { + return; + } + if (grpc_client_channel_trace.enabled()) { + gpr_log(GPR_INFO, + "calld=%p batch=%p: intercepting recv trailing for channelz", calld, + batch); + } + GRPC_CLOSURE_INIT(&calld->recv_trailing_metadata_ready_channelz, + recv_trailing_metadata_ready_channelz, elem, + grpc_schedule_on_exec_ctx); + // save some state needed for the interception callback. + GPR_ASSERT(calld->recv_trailing_metadata == nullptr); + calld->recv_trailing_metadata = + batch->payload->recv_trailing_metadata.recv_trailing_metadata; + calld->original_recv_trailing_metadata = + batch->payload->recv_trailing_metadata.recv_trailing_metadata_ready; + batch->payload->recv_trailing_metadata.recv_trailing_metadata_ready = + &calld->recv_trailing_metadata_ready_channelz; +} + // // LB pick // @@ -2669,65 +2732,6 @@ static void pick_done(void* arg, grpc_error* error) { } } -static void recv_trailing_metadata_ready_channelz(void* arg, - grpc_error* error) { - grpc_call_element* elem = static_cast(arg); - channel_data* chand = static_cast(elem->channel_data); - call_data* calld = static_cast(elem->call_data); - if (grpc_client_channel_trace.enabled()) { - gpr_log(GPR_INFO, - "chand=%p calld=%p: got recv_trailing_metadata_ready_channelz, " - "error=%s", - chand, calld, grpc_error_string(error)); - } - GPR_ASSERT(calld->recv_trailing_metadata_batch != nullptr); - grpc_status_code status = GRPC_STATUS_OK; - grpc_metadata_batch* md_batch = calld->recv_trailing_metadata_batch; - get_call_status(elem, md_batch, GRPC_ERROR_REF(error), &status, nullptr); - grpc_core::channelz::SubchannelNode* channelz_subchannel = - calld->pick.connected_subchannel->channelz_subchannel(); - GPR_ASSERT(channelz_subchannel != nullptr); - if (status == GRPC_STATUS_OK) { - channelz_subchannel->RecordCallSucceeded(); - } else { - channelz_subchannel->RecordCallFailed(); - } - calld->recv_trailing_metadata_batch = nullptr; - GRPC_CLOSURE_RUN(calld->original_recv_trailing_metadata, error); -} - -// If channelz is enabled, intercept recv_trailing so that we may check the -// status and associate it to a subchannel. -// Returns true if callback was intercepted, false otherwise. -static void maybe_intercept_metadata_for_channelz( - grpc_call_element* elem, grpc_transport_stream_op_batch* batch) { - call_data* calld = static_cast(elem->call_data); - // only intercept payloads with recv trailing. - if (!batch->recv_trailing_metadata) { - return; - } - // only add interceptor is channelz is enabled. - if (calld->pick.connected_subchannel->channelz_subchannel() == nullptr) { - return; - } - if (grpc_client_channel_trace.enabled()) { - gpr_log(GPR_INFO, - "calld=%p batch=%p: intercepting recv trailing for channelz", calld, - batch); - } - GRPC_CLOSURE_INIT(&calld->recv_trailing_metadata_ready_channelz, - recv_trailing_metadata_ready_channelz, elem, - grpc_schedule_on_exec_ctx); - // save some state needed for the interception callback. - GPR_ASSERT(calld->recv_trailing_metadata_batch == nullptr); - calld->recv_trailing_metadata_batch = - batch->payload->recv_trailing_metadata.recv_trailing_metadata; - calld->original_recv_trailing_metadata = - batch->payload->recv_trailing_metadata.recv_trailing_metadata_ready; - batch->payload->recv_trailing_metadata.recv_trailing_metadata_ready = - &calld->recv_trailing_metadata_ready_channelz; -} - static void maybe_add_call_to_channel_interested_parties_locked( grpc_call_element* elem) { channel_data* chand = static_cast(elem->channel_data); diff --git a/src/core/lib/channel/connected_channel.cc b/src/core/lib/channel/connected_channel.cc index 4a4f0e49d0..e2ea334ded 100644 --- a/src/core/lib/channel/connected_channel.cc +++ b/src/core/lib/channel/connected_channel.cc @@ -126,13 +126,11 @@ static void con_start_transport_stream_op_batch( // closure for each one. callback_state* state = static_cast(gpr_malloc(sizeof(*state))); - intercept_callback(calld, state, true, - "connected_on_complete (cancel_stream)", + intercept_callback(calld, state, true, "on_complete (cancel_stream)", &batch->on_complete); } else if (batch->on_complete != nullptr) { callback_state* state = get_state_for_batch(calld, batch); - intercept_callback(calld, state, false, "connected_on_complete", - &batch->on_complete); + intercept_callback(calld, state, false, "on_complete", &batch->on_complete); } grpc_transport_perform_stream_op( chand->transport, TRANSPORT_STREAM_FROM_CALL_DATA(calld), batch); -- cgit v1.2.3