From 5e054bf11e58049d988746cb76238f8099ac1faa Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 4 Sep 2018 13:01:34 -0700 Subject: [PATCH 1/3] 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 926afeec84f..a238d5f989f 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; } From c9e300d5b044351c6e09221b09ba49e53d211643 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 5 Sep 2018 17:08:01 -0700 Subject: [PATCH 2/3] Add channel arg control for user agent --- include/grpc/impl/codegen/grpc_types.h | 2 ++ .../ext/filters/http/server/http_server_filter.cc | 15 +++++++++++++-- .../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 b5353c1dea8..9097984a96b 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 a238d5f989f..53cd059aa80 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 f44ddca3b10..e1bce603fa7 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 fc07681535f..e835f557383 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"); From 16f53ff583c23b3b2a260ba0935fd0c30b479acf Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 5 Sep 2018 18:16:13 -0700 Subject: [PATCH 3/3] Surface user agent by default --- include/grpc/impl/codegen/grpc_types.h | 3 ++- .../ext/filters/http/server/http_server_filter.cc | 2 +- .../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 9097984a96b..5f3b96f40b5 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 53cd059aa80..1b3426b1200 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 e1bce603fa7..f44ddca3b10 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 e835f557383..fc07681535f 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");