diff --git a/changelogs/current/minor_behavior_changes/ext_proc__grpc-client-stats-scope.rst b/changelogs/current/minor_behavior_changes/ext_proc__grpc-client-stats-scope.rst new file mode 100644 index 0000000000000..83af12b153f0f --- /dev/null +++ b/changelogs/current/minor_behavior_changes/ext_proc__grpc-client-stats-scope.rst @@ -0,0 +1,5 @@ +Changed the stats scope that is used to create the gRPC client of the external processing +(``ext_proc``) filter. Previously the filter's own scope was used, so the gRPC client stats +gained an unexpected extra prefix, ``cluster..`` for an upstream filter. The +server scope is now used, so the Google gRPC client stats are emitted with the expected +``grpc..`` prefix. diff --git a/source/extensions/filters/http/ext_proc/config.cc b/source/extensions/filters/http/ext_proc/config.cc index 33830a09933fd..aa8804431232d 100644 --- a/source/extensions/filters/http/ext_proc/config.cc +++ b/source/extensions/filters/http/ext_proc/config.cc @@ -95,21 +95,23 @@ ExternalProcessingFilterConfig::createHttpFilterFactoryFromProtoTyped( PROTOBUF_GET_MS_OR_DEFAULT(proto_config, message_timeout, DefaultMessageTimeoutMs); const uint32_t max_message_timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config, max_message_timeout, DefaultMaxMessageTimeoutMs); - // The scope outlives the filter chain, so the callback below can hold on to it. The extra - // context itself must not be captured: it is a stack temporary at the call site. - OptRef scope = extra_context.scopeOr(context); + Stats::Scope& scope = extra_context.scopeOr(context); absl::Status config_creation_status = absl::OkStatus(); auto filter_config = std::make_shared( - proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, *scope, + proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, scope, extra_context.stats_prefix, extra_context.is_upstream, Envoy::Extensions::Filters::Common::Expr::getBuilder(context), context, config_creation_status); RETURN_IF_NOT_OK_REF(config_creation_status); if (proto_config.has_grpc_service()) { - return [filter_config = std::move(filter_config), &context, - scope](Http::FilterChainFactoryCallbacks& callbacks) { - auto client = - createExternalProcessorClient(context.clusterManager().grpcAsyncClientManager(), *scope); + return [filter_config = std::move(filter_config), + &context](Http::FilterChainFactoryCallbacks& callbacks) { + // The google gRPC client will create a fresh scope for its stats from the input scope and use + // 'grpc..' as the prefix. + // To avoid unexpected additional prefixes like 'http.' or + // 'cluster.', the server scope here is used. + auto client = createExternalProcessorClient(context.clusterManager().grpcAsyncClientManager(), + context.scope()); callbacks.addStreamFilter( Http::StreamFilterSharedPtr{std::make_shared(filter_config, std::move(client))}); }; diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index fba7674480626..2ec7fe9d6c386 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -22,7 +22,9 @@ envoy_extension_cc_test( rbe_pool = "linux_x64_small", tags = ["skip_on_windows"], deps = [ + "//source/common/stats:isolated_store_lib", "//source/extensions/filters/http/ext_proc:config", + "//test/mocks/http:http_mocks", "//test/mocks/server:factory_context_mocks", "//test/test_common:status_utility_lib", ], diff --git a/test/extensions/filters/http/ext_proc/config_test.cc b/test/extensions/filters/http/ext_proc/config_test.cc index 1534d66cf7892..6df73e4ed2894 100644 --- a/test/extensions/filters/http/ext_proc/config_test.cc +++ b/test/extensions/filters/http/ext_proc/config_test.cc @@ -1,9 +1,11 @@ // Changing the default behavior of ext_proc is generally not allowed. While you may add tests, you // generally should not change or remove existing tests. +#include "source/common/stats/isolated_store_impl.h" #include "source/extensions/filters/http/ext_proc/config.h" #include "source/extensions/filters/http/ext_proc/ext_proc.h" +#include "test/mocks/http/mocks.h" #include "test/mocks/server/factory_context.h" #include "test/test_common/status_utility.h" #include "test/test_common/utility.h" @@ -619,6 +621,65 @@ TEST(HttpExtProcConfigTest, PerRouteEmitClientSpanConfig) { EXPECT_FALSE(typed_config.emitClientSpan().value()); } +// The gRPC client is shared across listeners and clusters via a central cache, so it must always +// be created with the server scope. In particular the scope of an upstream filter, which has a +// 'cluster..' prefix, must not leak into the gRPC client stats. +TEST(HttpExtProcConfigTest, GrpcClientIsCreatedWithServerScope) { + std::string yaml = R"EOF( + grpc_service: + google_grpc: + target_uri: ext_proc_server + stat_prefix: google + failure_mode_allow: true + )EOF"; + + ExternalProcessingFilterConfig factory; + ProtobufTypes::MessagePtr proto_config = factory.createEmptyConfigProto(); + TestUtility::loadFromYaml(yaml, *proto_config); + + testing::NiceMock context; + Stats::IsolatedStoreImpl cluster_store; + Stats::ScopeSharedPtr cluster_scope = cluster_store.createScope("cluster.fake_cluster"); + + // Create the filter as an upstream filter, that is with the cluster scope. + Server::Configuration::ExtraFactoryContext extra_context{context.messageValidationVisitor(), + "stats"}; + extra_context.scope = *cluster_scope; + extra_context.is_upstream = true; + + Http::FilterFactoryCb cb = + factory.createHttpFilterFactoryFromProto(*proto_config, context, extra_context).value(); + + Http::StreamFilterSharedPtr filter; + Http::MockFilterChainFactoryCallbacks filter_callback; + EXPECT_CALL(filter_callback, addStreamFilter(_)).WillOnce(testing::SaveArg<0>(&filter)); + cb(filter_callback); + + // Opening the stream is the point at which the client asks for the raw async client with the + // scope that it was created with. Fail the creation to keep the test to the scope check. + Stats::Scope* client_scope = nullptr; + EXPECT_CALL(context.cluster_manager_.async_client_manager_, + getOrCreateRawAsyncClientWithHashKey(_, _, _)) + .WillOnce(testing::Invoke( + [&client_scope](const Grpc::GrpcServiceConfigWithHashKey&, Stats::Scope& scope, + bool) -> absl::StatusOr { + client_scope = &scope; + return absl::InternalError("no client for this test"); + })); + + testing::NiceMock decoder_callbacks; + filter->setDecoderFilterCallbacks(decoder_callbacks); + Http::TestRequestHeaderMapImpl headers{ + {":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + filter->decodeHeaders(headers, true); + + ASSERT_NE(client_scope, nullptr); + EXPECT_EQ(client_scope, &context.scope()); + EXPECT_NE(client_scope, cluster_scope.get()); + + filter->onDestroy(); +} + } // namespace } // namespace ExternalProcessing } // namespace HttpFilters