ext proc: fix upstream extproc grpc client stats scope

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.<cluster_name>.`` for an upstream filter. The
+server scope is now used, so the Google gRPC client stats are emitted with the expected
+``grpc.<google_grpc_stat_prefix>.`` prefix.

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<Stats::Scope> 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<FilterConfig>(
-      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.<google_grpc_stat_prefix>.' as the prefix.
+      // To avoid unexpected additional prefixes like 'http.<connection_manager>' or
+      // 'cluster.<cluster_name>', the server scope here is used.
+      auto client = createExternalProcessorClient(context.clusterManager().grpcAsyncClientManager(),
+                                                  context.scope());
       callbacks.addStreamFilter(
           Http::StreamFilterSharedPtr{std::make_shared<Filter>(filter_config, std::move(client))});
     };

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",
     ],

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.<cluster_name>.' 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<Server::Configuration::MockServerFactoryContext> 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<Grpc::RawAsyncClientSharedPtr> {
+            client_scope = &scope;
+            return absl::InternalError("no client for this test");
+          }));
+
+  testing::NiceMock<Http::MockStreamDecoderFilterCallbacks> 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