From 7dd435c71ba9e7db0fcecd4d895d177cbb33c73e Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 17 Mar 2022 11:23:29 -0700 Subject: [PATCH 1/4] ecds: use top level stats prefix Signed-off-by: Kuat Yessenov --- .../root/configuration/overview/extension.rst | 5 +- docs/root/version_history/current.rst | 1 + envoy/filter/config_provider_manager.h | 5 ++ source/common/filter/config_discovery_impl.cc | 12 ++-- source/common/filter/config_discovery_impl.h | 19 ++++-- source/common/runtime/runtime_features.cc | 1 + .../filter/config_discovery_impl_test.cc | 59 ++++++++++++------- .../extension_discovery_integration_test.cc | 58 +++++++----------- 8 files changed, 90 insertions(+), 70 deletions(-) diff --git a/docs/root/configuration/overview/extension.rst b/docs/root/configuration/overview/extension.rst index 792f384f0f57..9149cefcd265 100644 --- a/docs/root/configuration/overview/extension.rst +++ b/docs/root/configuration/overview/extension.rst @@ -80,8 +80,9 @@ for HTTP filters. Extension config discovery service has a :ref:`statistics ` tree rooted at -*.extension_config_discovery.*. In addition -to the common subscription statistics, it also provides the following: +*.extension_config_discovery.*. For HTTP +filters, the value of ** is *http_filters*. In addition to the +common subscription statistics, it also provides the following: .. csv-table:: :header: Name, Type, Description diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 6162c87eab46..8b852c783057 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -17,6 +17,7 @@ Minor Behavior Changes * config: warning messages for protobuf unknown fields now contain ancestors for easier troubleshooting. * decompressor: decompressor does not duplicate `accept-encoding` header values anymore. This behavioral change can be reverted by setting runtime guard ``envoy.reloadable_features.append_to_accept_content_encoding_only_once`` to false. * dynamic_forward_proxy: if a DNS resolution fails, failing immediately with a specific resolution error, rather than finishing up all local filters and failing to select an upstream host. +* ecds: changed to use ``http_filters`` stat prefix as the metrics root for ECDS subscriptions. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.top_level_ecds_stats`` to false. * ext_authz: added requested server name in ext_authz network filter for auth review. * file: changed disk based files to truncate files which are not being appended to. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.append_or_truncate`` to false. * grpc: flip runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to be default enabled. async grpc client created through getOrCreateRawAsyncClient will be cached by default. diff --git a/envoy/filter/config_provider_manager.h b/envoy/filter/config_provider_manager.h index cd1affc57997..0f7a447d1029 100644 --- a/envoy/filter/config_provider_manager.h +++ b/envoy/filter/config_provider_manager.h @@ -52,6 +52,11 @@ template class FilterConfigProviderManager { virtual FilterConfigProviderPtr createStaticFilterConfigProvider(const FactoryCb& config, const std::string& filter_config_name) PURE; + + /** + * Get the stat prefix for the scope of the filter provider manager. + */ + virtual absl::string_view statPrefix() const PURE; }; } // namespace Filter diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc index aae2fa7be7e4..8b0cc7508dfd 100644 --- a/source/common/filter/config_discovery_impl.cc +++ b/source/common/filter/config_discovery_impl.cc @@ -16,6 +16,8 @@ namespace Envoy { namespace Filter { +constexpr absl::string_view HttpStatPrefix = "http_filter."; + namespace { void validateTypeUrlHelper(const std::string& type_url, const absl::flat_hash_set require_type_urls) { @@ -59,7 +61,7 @@ const std::string& DynamicFilterConfigProviderImplBase::name() { return subscrip FilterConfigSubscription::FilterConfigSubscription( const envoy::config::core::v3::ConfigSource& config_source, const std::string& filter_config_name, - Server::Configuration::ServerFactoryContext& factory_context, const std::string& stat_prefix, + Server::Configuration::ServerFactoryContext& factory_context, absl::string_view stat_prefix, FilterConfigProviderManagerImplBase& filter_config_provider_manager, const std::string& subscription_id) : Config::SubscriptionBase( @@ -67,8 +69,8 @@ FilterConfigSubscription::FilterConfigSubscription( filter_config_name_(filter_config_name), factory_context_(factory_context), init_target_(fmt::format("FilterConfigSubscription init {}", filter_config_name_), [this]() { start(); }), - scope_(factory_context.scope().createScope(stat_prefix + "extension_config_discovery." + - filter_config_name_ + ".")), + scope_(factory_context.scope().createScope( + absl::StrCat(stat_prefix, "extension_config_discovery.", filter_config_name_, "."))), stat_prefix_(stat_prefix), stats_({ALL_EXTENSION_CONFIG_DISCOVERY_STATS(POOL_COUNTER(*scope_))}), filter_config_provider_manager_(filter_config_provider_manager), @@ -178,7 +180,7 @@ void FilterConfigSubscription::incrementConflictCounter() { stats_.config_confli std::shared_ptr FilterConfigProviderManagerImplBase::getSubscription( const envoy::config::core::v3::ConfigSource& config_source, const std::string& name, - Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix) { + Server::Configuration::FactoryContext& factory_context, absl::string_view stat_prefix) { // FilterConfigSubscriptions are unique based on their config source and filter config name // combination. // TODO(https://github.com/envoyproxy/envoy/issues/11967) Hash collision can cause subscription @@ -244,6 +246,8 @@ std::tuple HttpFilterConfigProviderManag return {std::move(message), factory.name()}; } +absl::string_view HttpFilterConfigProviderManagerImpl::statPrefix() const { return HttpStatPrefix; } + ProtobufTypes::MessagePtr HttpFilterConfigProviderManagerImpl::getDefaultConfig( const ProtobufWkt::Any& proto_config, const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, bool last_filter_in_filter_chain, diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index c07ab14a94fc..07f8102535f4 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -74,7 +74,7 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImplBa ProtobufTypes::MessagePtr&& default_config, bool last_filter_in_filter_chain, const std::string& filter_chain_type, - const std::string& stat_prefix) + absl::string_view stat_prefix) : DynamicFilterConfigProviderImplBase(subscription, require_type_urls, last_filter_in_filter_chain, filter_chain_type), stat_prefix_(stat_prefix), factory_context_(factory_context), @@ -192,7 +192,7 @@ class FilterConfigSubscription FilterConfigSubscription(const envoy::config::core::v3::ConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::ServerFactoryContext& factory_context, - const std::string& stat_prefix, + absl::string_view stat_prefix, FilterConfigProviderManagerImplBase& filter_config_provider_manager, const std::string& subscription_id); @@ -277,7 +277,7 @@ class FilterConfigProviderManagerImplBase : Logger::Loggable std::shared_ptr getSubscription(const envoy::config::core::v3::ConfigSource& config_source, const std::string& name, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix); + absl::string_view stat_prefix); void applyLastOrDefaultConfig(std::shared_ptr& subscription, DynamicFilterConfigProviderImplBase& provider, const std::string& filter_config_name); @@ -300,8 +300,13 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, bool last_filter_in_filter_chain, const std::string& filter_chain_type) override { + absl::string_view actual_stat_prefix = + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.top_level_ecds_stats") + ? statPrefix() + : stat_prefix; + auto subscription = getSubscription(config_source.config_source(), filter_config_name, - factory_context, stat_prefix); + factory_context, actual_stat_prefix); // For warming, wait until the subscription receives the first response to indicate readiness. // Otherwise, mark ready immediately and start the subscription on initialization. A default // config is expected in the latter case. @@ -323,7 +328,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa auto provider = std::make_unique>( subscription, require_type_urls, factory_context, std::move(default_config), - last_filter_in_filter_chain, filter_chain_type, stat_prefix); + last_filter_in_filter_chain, filter_chain_type, actual_stat_prefix); // Ensure the subscription starts if it has not already. if (config_source.apply_default_config_without_warming()) { @@ -339,6 +344,8 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa return std::make_unique>(config, filter_config_name); } + virtual absl::string_view statPrefix() const override PURE; + protected: virtual ProtobufTypes::MessagePtr getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name, @@ -355,6 +362,8 @@ class HttpFilterConfigProviderManagerImpl getMessage(const envoy::config::core::v3::TypedExtensionConfig& filter_config, Server::Configuration::ServerFactoryContext& factory_context) const override; + absl::string_view statPrefix() const override; + protected: ProtobufTypes::MessagePtr getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name, diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 13e7f44053ba..4dbf2bd98057 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -60,6 +60,7 @@ RUNTIME_GUARD(envoy_reloadable_features_skip_dispatching_frames_for_closed_conne RUNTIME_GUARD(envoy_reloadable_features_strict_check_on_ipv4_compat); RUNTIME_GUARD(envoy_reloadable_features_support_locality_update_on_eds_cluster_endpoints); RUNTIME_GUARD(envoy_reloadable_features_test_feature_true); +RUNTIME_GUARD(envoy_reloadable_features_top_level_ecds_stats); RUNTIME_GUARD(envoy_reloadable_features_udp_listener_updates_filter_chain_in_place); RUNTIME_GUARD(envoy_reloadable_features_update_expected_rq_timeout_on_retry); RUNTIME_GUARD(envoy_reloadable_features_update_grpc_response_error_tag); diff --git a/test/common/filter/config_discovery_impl_test.cc b/test/common/filter/config_discovery_impl_test.cc index f2b1d17b95e0..8889bc133254 100644 --- a/test/common/filter/config_discovery_impl_test.cc +++ b/test/common/filter/config_discovery_impl_test.cc @@ -110,7 +110,7 @@ config_source: { ads: {} } } return filter_config_provider_manager_->createDynamicFilterConfigProvider( - config_source, name, factory_context_, "xds.", last_filter_config, "http"); + config_source, name, factory_context_, "", last_filter_config, "http"); } void setup(bool warm = true, bool default_configuration = false, bool last_filter_config = true) { @@ -161,8 +161,10 @@ TEST_F(FilterConfigDiscoveryImplTest, Basic) { EXPECT_CALL(init_watcher_, ready()); callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()); EXPECT_NE(absl::nullopt, provider_->config()); - EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); + EXPECT_EQ(1UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); } // 2nd request with same response. Based on hash should not reload config. @@ -180,8 +182,10 @@ TEST_F(FilterConfigDiscoveryImplTest, Basic) { const auto decoded_resources = TestUtility::decodeResources(response); callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()); - EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); + EXPECT_EQ(1UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); } } @@ -190,8 +194,9 @@ TEST_F(FilterConfigDiscoveryImplTest, ConfigFailed) { setup(); EXPECT_CALL(init_watcher_, ready()); callbacks_->onConfigUpdateFailed(Config::ConfigUpdateFailureReason::FetchTimedout, {}); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); + EXPECT_EQ(0UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(1UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); } TEST_F(FilterConfigDiscoveryImplTest, TooManyResources) { @@ -217,7 +222,8 @@ TEST_F(FilterConfigDiscoveryImplTest, TooManyResources) { EXPECT_THROW_WITH_MESSAGE( callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()), EnvoyException, "Unexpected number of resources in ExtensionConfigDS response: 2"); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); } TEST_F(FilterConfigDiscoveryImplTest, WrongName) { @@ -239,7 +245,8 @@ TEST_F(FilterConfigDiscoveryImplTest, WrongName) { EXPECT_THROW_WITH_MESSAGE( callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()), EnvoyException, "Unexpected resource name in ExtensionConfigDS response: bar"); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); } TEST_F(FilterConfigDiscoveryImplTest, Incremental) { @@ -268,16 +275,18 @@ version_info: "1" EXPECT_CALL(init_watcher_, ready()); do_xds_response(true); EXPECT_NE(absl::nullopt, provider_->config()); - EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); + EXPECT_EQ(1UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); // Ensure that we honor resource removals. Protobuf::RepeatedPtrField remove; *remove.Add() = "foo"; callbacks_->onConfigUpdate({}, remove, "1"); EXPECT_EQ(absl::nullopt, provider_->config()); - EXPECT_EQ(2UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); + EXPECT_EQ(2UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); } TEST_F(FilterConfigDiscoveryImplTest, IncrementalWithDefault) { @@ -310,8 +319,9 @@ version_info: "1" EXPECT_CALL(init_watcher_, ready()); do_xds_response(false); EXPECT_NE(absl::nullopt, provider_->config()); - EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); + EXPECT_EQ(1UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); // If we get a removal while a default is configured, we should revert back to the default // instead of clearing out the factory. @@ -319,8 +329,9 @@ version_info: "1" *remove.Add() = "foo"; callbacks_->onConfigUpdate({}, remove, "1"); EXPECT_NE(absl::nullopt, provider_->config()); - EXPECT_EQ(2UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); + EXPECT_EQ(2UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); } TEST_F(FilterConfigDiscoveryImplTest, ApplyWithoutWarming) { @@ -328,8 +339,9 @@ TEST_F(FilterConfigDiscoveryImplTest, ApplyWithoutWarming) { setup(false); EXPECT_EQ("foo", provider_->name()); EXPECT_NE(absl::nullopt, provider_->config()); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); + EXPECT_EQ(0UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); } TEST_F(FilterConfigDiscoveryImplTest, DualProviders) { @@ -354,7 +366,8 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProviders) { callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()); EXPECT_NE(absl::nullopt, provider_->config()); EXPECT_NE(absl::nullopt, provider2->config()); - EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(1UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); } TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) { @@ -380,7 +393,8 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) { EnvoyException, "Error: filter config has type URL test.integration.filters.AddBodyFilterConfig but " "expect envoy.extensions.filters.http.router.v3.Router."); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); } // Raise exception when filter is not the last filter in filter chain, but the filter is terminal @@ -406,7 +420,8 @@ TEST_F(FilterConfigDiscoveryImplTest, TerminalFilterInvalid) { EnvoyException, "Error: terminal filter named foo of type envoy.filters.http.router must be the last filter " "in a http filter chain."); - EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, + scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); } } // namespace diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index b0def4f684b6..850eb17c5543 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -297,8 +297,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", denyPrivateConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -322,8 +321,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { // Update again but keep the connection. { sendXdsResponse("foo", "2", allowAllConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 2); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); @@ -339,8 +337,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", denyPrivateConfig(), true); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -365,8 +362,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { { // Wait until the the TTL for the resource expires, which will trigger a config load to remove // the resource. - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 2); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); @@ -377,8 +373,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { // Reinstate the previous configuration. sendXdsResponse("foo", "1", denyPrivateConfig(), true); // Wait until the new configuration has been applied. - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 3); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 3); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); @@ -394,8 +389,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", allowAllConfig(), true); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -413,8 +407,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { { // Wait until the the TTL for the resource expires, which will trigger a config load to remove // the resource. - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 2); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); @@ -430,8 +423,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithMatcher) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponseWithFullYaml("foo", "1", denyPrivateConfigWithMatcher()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -473,7 +465,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicDefaultMatcher) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", invalidConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_fail", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -510,8 +502,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfig) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", allowAllConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); @@ -529,8 +520,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfig) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - test_server_->waitForCounterEq("http.config_test.extension_config_discovery.foo.config_conflict", - 0); + test_server_->waitForCounterEq("http_filter.extension_config_discovery.foo.config_conflict", 0); } // Validate that a listener update falls back to the default extension configuration @@ -543,8 +533,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfigInvalid) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponseWithFullYaml("foo", "1", denyPrivateConfigWithMatcher()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); @@ -570,8 +559,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfigInvalid) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_conflict", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_conflict", 1); } TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { @@ -582,7 +570,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", invalidConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_fail", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -603,7 +591,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", invalidConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_fail", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -638,8 +626,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { // Update should cause a different response. sendXdsResponse("bar", "1", denyPrivateConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.bar.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.bar.config_reload", 1); { auto response = codec_client_->makeHeaderOnlyRequest(request_headers); ASSERT_TRUE(response->waitForEndStream()); @@ -660,7 +647,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); // Update should not cause a different response. sendXdsResponse("bar", "1", invalidConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.bar.config_fail", 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.bar.config_fail", 1); Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); @@ -678,8 +665,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameName) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("baz", "1", denyPrivateConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.baz.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.baz.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -718,7 +704,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFilterNotAtEndOfFilte EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", terminalFilterConfig(), false, false); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_fail", 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_fail", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -740,8 +726,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReloadBoth) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", denyPrivateConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 1); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -776,8 +761,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReloadBoth) { // Update ECDS but keep the connection. { sendXdsResponse("foo", "2", allowAllConfig()); - test_server_->waitForCounterGe("http.config_test.extension_config_discovery.foo.config_reload", - 2); + test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); From 225dcc9e38470002c84f894351de050fe3365c06 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 17 Mar 2022 11:25:33 -0700 Subject: [PATCH 2/4] typo Signed-off-by: Kuat Yessenov --- docs/root/configuration/overview/extension.rst | 2 +- docs/root/version_history/current.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/root/configuration/overview/extension.rst b/docs/root/configuration/overview/extension.rst index 9149cefcd265..652d3f69f62f 100644 --- a/docs/root/configuration/overview/extension.rst +++ b/docs/root/configuration/overview/extension.rst @@ -81,7 +81,7 @@ for HTTP filters. Extension config discovery service has a :ref:`statistics ` tree rooted at *.extension_config_discovery.*. For HTTP -filters, the value of ** is *http_filters*. In addition to the +filters, the value of ** is *http_filter*. In addition to the common subscription statistics, it also provides the following: .. csv-table:: diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 8b852c783057..b7f1c8a4114d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -17,7 +17,7 @@ Minor Behavior Changes * config: warning messages for protobuf unknown fields now contain ancestors for easier troubleshooting. * decompressor: decompressor does not duplicate `accept-encoding` header values anymore. This behavioral change can be reverted by setting runtime guard ``envoy.reloadable_features.append_to_accept_content_encoding_only_once`` to false. * dynamic_forward_proxy: if a DNS resolution fails, failing immediately with a specific resolution error, rather than finishing up all local filters and failing to select an upstream host. -* ecds: changed to use ``http_filters`` stat prefix as the metrics root for ECDS subscriptions. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.top_level_ecds_stats`` to false. +* ecds: changed to use ``http_filter`` stat prefix as the metrics root for ECDS subscriptions. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.top_level_ecds_stats`` to false. * ext_authz: added requested server name in ext_authz network filter for auth review. * file: changed disk based files to truncate files which are not being appended to. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.append_or_truncate`` to false. * grpc: flip runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to be default enabled. async grpc client created through getOrCreateRawAsyncClient will be cached by default. From 3856a8e15d02d1ef29993d264bb32e6aa8f0c9af Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 17 Mar 2022 15:25:13 -0700 Subject: [PATCH 3/4] clang tidy Signed-off-by: Kuat Yessenov --- source/common/filter/config_discovery_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 07f8102535f4..4cead26b6064 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -344,7 +344,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa return std::make_unique>(config, filter_config_name); } - virtual absl::string_view statPrefix() const override PURE; + absl::string_view statPrefix() const override PURE; protected: virtual ProtobufTypes::MessagePtr From 4408876fafb7288beca10605fd9ab6b88be639eb Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Mon, 21 Mar 2022 09:58:49 -0700 Subject: [PATCH 4/4] review Signed-off-by: Kuat Yessenov --- .../root/configuration/overview/extension.rst | 2 +- source/common/filter/config_discovery_impl.cc | 8 +- source/common/filter/config_discovery_impl.h | 24 +++--- test/common/filter/BUILD | 1 + .../filter/config_discovery_impl_test.cc | 74 +++++++++++++------ .../extension_discovery_integration_test.cc | 42 +++++------ 6 files changed, 93 insertions(+), 58 deletions(-) diff --git a/docs/root/configuration/overview/extension.rst b/docs/root/configuration/overview/extension.rst index 652d3f69f62f..fd99aa96f000 100644 --- a/docs/root/configuration/overview/extension.rst +++ b/docs/root/configuration/overview/extension.rst @@ -80,7 +80,7 @@ for HTTP filters. Extension config discovery service has a :ref:`statistics ` tree rooted at -*.extension_config_discovery.*. For HTTP +*extension_config_discovery..*. For HTTP filters, the value of ** is *http_filter*. In addition to the common subscription statistics, it also provides the following: diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc index 8b0cc7508dfd..226b0acf7e21 100644 --- a/source/common/filter/config_discovery_impl.cc +++ b/source/common/filter/config_discovery_impl.cc @@ -61,7 +61,7 @@ const std::string& DynamicFilterConfigProviderImplBase::name() { return subscrip FilterConfigSubscription::FilterConfigSubscription( const envoy::config::core::v3::ConfigSource& config_source, const std::string& filter_config_name, - Server::Configuration::ServerFactoryContext& factory_context, absl::string_view stat_prefix, + Server::Configuration::ServerFactoryContext& factory_context, const std::string& stat_prefix, FilterConfigProviderManagerImplBase& filter_config_provider_manager, const std::string& subscription_id) : Config::SubscriptionBase( @@ -69,9 +69,7 @@ FilterConfigSubscription::FilterConfigSubscription( filter_config_name_(filter_config_name), factory_context_(factory_context), init_target_(fmt::format("FilterConfigSubscription init {}", filter_config_name_), [this]() { start(); }), - scope_(factory_context.scope().createScope( - absl::StrCat(stat_prefix, "extension_config_discovery.", filter_config_name_, "."))), - stat_prefix_(stat_prefix), + scope_(factory_context.scope().createScope(stat_prefix)), stats_({ALL_EXTENSION_CONFIG_DISCOVERY_STATS(POOL_COUNTER(*scope_))}), filter_config_provider_manager_(filter_config_provider_manager), subscription_id_(subscription_id) { @@ -180,7 +178,7 @@ void FilterConfigSubscription::incrementConflictCounter() { stats_.config_confli std::shared_ptr FilterConfigProviderManagerImplBase::getSubscription( const envoy::config::core::v3::ConfigSource& config_source, const std::string& name, - Server::Configuration::FactoryContext& factory_context, absl::string_view stat_prefix) { + Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix) { // FilterConfigSubscriptions are unique based on their config source and filter config name // combination. // TODO(https://github.com/envoyproxy/envoy/issues/11967) Hash collision can cause subscription diff --git a/source/common/filter/config_discovery_impl.h b/source/common/filter/config_discovery_impl.h index 4cead26b6064..d7c69dfdcaab 100644 --- a/source/common/filter/config_discovery_impl.h +++ b/source/common/filter/config_discovery_impl.h @@ -192,7 +192,7 @@ class FilterConfigSubscription FilterConfigSubscription(const envoy::config::core::v3::ConfigSource& config_source, const std::string& filter_config_name, Server::Configuration::ServerFactoryContext& factory_context, - absl::string_view stat_prefix, + const std::string& stat_prefix, FilterConfigProviderManagerImplBase& filter_config_provider_manager, const std::string& subscription_id); @@ -230,7 +230,6 @@ class FilterConfigSubscription bool started_{false}; Stats::ScopePtr scope_; - const std::string stat_prefix_; ExtensionConfigDiscoveryStats stats_; // FilterConfigProviderManagerImplBase maintains active subscriptions in a map. @@ -277,7 +276,7 @@ class FilterConfigProviderManagerImplBase : Logger::Loggable std::shared_ptr getSubscription(const envoy::config::core::v3::ConfigSource& config_source, const std::string& name, Server::Configuration::FactoryContext& factory_context, - absl::string_view stat_prefix); + const std::string& stat_prefix); void applyLastOrDefaultConfig(std::shared_ptr& subscription, DynamicFilterConfigProviderImplBase& provider, const std::string& filter_config_name); @@ -300,13 +299,20 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, bool last_filter_in_filter_chain, const std::string& filter_chain_type) override { - absl::string_view actual_stat_prefix = - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.top_level_ecds_stats") - ? statPrefix() - : stat_prefix; + std::string subscription_stat_prefix; + absl::string_view provider_stat_prefix; + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.top_level_ecds_stats")) { + subscription_stat_prefix = + absl::StrCat("extension_config_discovery.", statPrefix(), filter_config_name, "."); + provider_stat_prefix = subscription_stat_prefix; + } else { + subscription_stat_prefix = + absl::StrCat(stat_prefix, "extension_config_discovery.", filter_config_name, "."); + provider_stat_prefix = stat_prefix; + } auto subscription = getSubscription(config_source.config_source(), filter_config_name, - factory_context, actual_stat_prefix); + factory_context, subscription_stat_prefix); // For warming, wait until the subscription receives the first response to indicate readiness. // Otherwise, mark ready immediately and start the subscription on initialization. A default // config is expected in the latter case. @@ -328,7 +334,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa auto provider = std::make_unique>( subscription, require_type_urls, factory_context, std::move(default_config), - last_filter_in_filter_chain, filter_chain_type, actual_stat_prefix); + last_filter_in_filter_chain, filter_chain_type, provider_stat_prefix); // Ensure the subscription starts if it has not already. if (config_source.apply_default_config_without_warming()) { diff --git a/test/common/filter/BUILD b/test/common/filter/BUILD index 1bc00f1e8c84..1429f044631e 100644 --- a/test/common/filter/BUILD +++ b/test/common/filter/BUILD @@ -23,6 +23,7 @@ envoy_cc_test( "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", diff --git a/test/common/filter/config_discovery_impl_test.cc b/test/common/filter/config_discovery_impl_test.cc index 8889bc133254..0b725db42d85 100644 --- a/test/common/filter/config_discovery_impl_test.cc +++ b/test/common/filter/config_discovery_impl_test.cc @@ -20,6 +20,7 @@ #include "test/mocks/upstream/mocks.h" #include "test/test_common/printers.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "absl/strings/substitute.h" @@ -110,7 +111,7 @@ config_source: { ads: {} } } return filter_config_provider_manager_->createDynamicFilterConfigProvider( - config_source, name, factory_context_, "", last_filter_config, "http"); + config_source, name, factory_context_, "xds.", last_filter_config, "http"); } void setup(bool warm = true, bool default_configuration = false, bool last_filter_config = true) { @@ -162,9 +163,9 @@ TEST_F(FilterConfigDiscoveryImplTest, Basic) { callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()); EXPECT_NE(absl::nullopt, provider_->config()); EXPECT_EQ(1UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); EXPECT_EQ(0UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value()); } // 2nd request with same response. Based on hash should not reload config. @@ -183,20 +184,49 @@ TEST_F(FilterConfigDiscoveryImplTest, Basic) { TestUtility::decodeResources(response); callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()); EXPECT_EQ(1UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); EXPECT_EQ(0UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value()); } } +TEST_F(FilterConfigDiscoveryImplTest, BasicDeprecatedStatPrefix) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.top_level_ecds_stats", "false"}}); + + InSequence s; + setup(); + EXPECT_EQ("foo", provider_->name()); + EXPECT_EQ(absl::nullopt, provider_->config()); + + const std::string response_yaml = R"EOF( + version_info: "1" + resources: + - "@type": type.googleapis.com/envoy.config.core.v3.TypedExtensionConfig + name: foo + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + const auto response = + TestUtility::parseYaml(response_yaml); + const auto decoded_resources = + TestUtility::decodeResources(response); + + EXPECT_CALL(init_watcher_, ready()); + callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()); + EXPECT_NE(absl::nullopt, provider_->config()); + EXPECT_EQ(1UL, scope_.counter("xds.extension_config_discovery.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("xds.extension_config_discovery.foo.config_fail").value()); +} + TEST_F(FilterConfigDiscoveryImplTest, ConfigFailed) { InSequence s; setup(); EXPECT_CALL(init_watcher_, ready()); callbacks_->onConfigUpdateFailed(Config::ConfigUpdateFailureReason::FetchTimedout, {}); EXPECT_EQ(0UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(1UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); + EXPECT_EQ(1UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value()); } TEST_F(FilterConfigDiscoveryImplTest, TooManyResources) { @@ -223,7 +253,7 @@ TEST_F(FilterConfigDiscoveryImplTest, TooManyResources) { callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()), EnvoyException, "Unexpected number of resources in ExtensionConfigDS response: 2"); EXPECT_EQ(0UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); } TEST_F(FilterConfigDiscoveryImplTest, WrongName) { @@ -246,7 +276,7 @@ TEST_F(FilterConfigDiscoveryImplTest, WrongName) { callbacks_->onConfigUpdate(decoded_resources.refvec_, response.version_info()), EnvoyException, "Unexpected resource name in ExtensionConfigDS response: bar"); EXPECT_EQ(0UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); } TEST_F(FilterConfigDiscoveryImplTest, Incremental) { @@ -276,8 +306,8 @@ version_info: "1" do_xds_response(true); EXPECT_NE(absl::nullopt, provider_->config()); EXPECT_EQ(1UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value()); // Ensure that we honor resource removals. Protobuf::RepeatedPtrField remove; @@ -285,8 +315,8 @@ version_info: "1" callbacks_->onConfigUpdate({}, remove, "1"); EXPECT_EQ(absl::nullopt, provider_->config()); EXPECT_EQ(2UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value()); } TEST_F(FilterConfigDiscoveryImplTest, IncrementalWithDefault) { @@ -320,8 +350,8 @@ version_info: "1" do_xds_response(false); EXPECT_NE(absl::nullopt, provider_->config()); EXPECT_EQ(1UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value()); // If we get a removal while a default is configured, we should revert back to the default // instead of clearing out the factory. @@ -330,8 +360,8 @@ version_info: "1" callbacks_->onConfigUpdate({}, remove, "1"); EXPECT_NE(absl::nullopt, provider_->config()); EXPECT_EQ(2UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value()); } TEST_F(FilterConfigDiscoveryImplTest, ApplyWithoutWarming) { @@ -340,8 +370,8 @@ TEST_F(FilterConfigDiscoveryImplTest, ApplyWithoutWarming) { EXPECT_EQ("foo", provider_->name()); EXPECT_NE(absl::nullopt, provider_->config()); EXPECT_EQ(0UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); - EXPECT_EQ(0UL, scope_.counter("http_filter.extension_config_discovery.foo.config_fail").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); + EXPECT_EQ(0UL, scope_.counter("extension_config_discovery.http_filter.foo.config_fail").value()); } TEST_F(FilterConfigDiscoveryImplTest, DualProviders) { @@ -367,7 +397,7 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProviders) { EXPECT_NE(absl::nullopt, provider_->config()); EXPECT_NE(absl::nullopt, provider2->config()); EXPECT_EQ(1UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); } TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) { @@ -394,7 +424,7 @@ TEST_F(FilterConfigDiscoveryImplTest, DualProvidersInvalid) { "Error: filter config has type URL test.integration.filters.AddBodyFilterConfig but " "expect envoy.extensions.filters.http.router.v3.Router."); EXPECT_EQ(0UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); } // Raise exception when filter is not the last filter in filter chain, but the filter is terminal @@ -421,7 +451,7 @@ TEST_F(FilterConfigDiscoveryImplTest, TerminalFilterInvalid) { "Error: terminal filter named foo of type envoy.filters.http.router must be the last filter " "in a http filter chain."); EXPECT_EQ(0UL, - scope_.counter("http_filter.extension_config_discovery.foo.config_reload").value()); + scope_.counter("extension_config_discovery.http_filter.foo.config_reload").value()); } } // namespace diff --git a/test/integration/extension_discovery_integration_test.cc b/test/integration/extension_discovery_integration_test.cc index 850eb17c5543..1bc92688d316 100644 --- a/test/integration/extension_discovery_integration_test.cc +++ b/test/integration/extension_discovery_integration_test.cc @@ -297,7 +297,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", denyPrivateConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -321,7 +321,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccess) { // Update again but keep the connection. { sendXdsResponse("foo", "2", allowAllConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 2); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); @@ -337,7 +337,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", denyPrivateConfig(), true); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -362,7 +362,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { { // Wait until the the TTL for the resource expires, which will trigger a config load to remove // the resource. - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 2); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); @@ -373,7 +373,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtl) { // Reinstate the previous configuration. sendXdsResponse("foo", "1", denyPrivateConfig(), true); // Wait until the new configuration has been applied. - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 3); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 3); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); @@ -389,7 +389,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", allowAllConfig(), true); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -407,7 +407,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithTtlWithDefault) { { // Wait until the the TTL for the resource expires, which will trigger a config load to remove // the resource. - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 2); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); @@ -423,7 +423,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicSuccessWithMatcher) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponseWithFullYaml("foo", "1", denyPrivateConfigWithMatcher()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -465,7 +465,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicDefaultMatcher) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", invalidConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_fail", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_fail", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -502,7 +502,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfig) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", allowAllConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); @@ -520,7 +520,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfig) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - test_server_->waitForCounterEq("http_filter.extension_config_discovery.foo.config_conflict", 0); + test_server_->waitForCounterEq("extension_config_discovery.http_filter.foo.config_conflict", 0); } // Validate that a listener update falls back to the default extension configuration @@ -533,7 +533,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfigInvalid) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponseWithFullYaml("foo", "1", denyPrivateConfigWithMatcher()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); @@ -559,7 +559,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReuseExtensionConfigInvalid) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("403", response->headers().getStatusValue()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_conflict", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_conflict", 1); } TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { @@ -570,7 +570,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithDefault) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", invalidConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_fail", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_fail", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -591,7 +591,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailWithoutDefault) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", invalidConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_fail", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_fail", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -626,7 +626,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarming) { // Update should cause a different response. sendXdsResponse("bar", "1", denyPrivateConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.bar.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.bar.config_reload", 1); { auto response = codec_client_->makeHeaderOnlyRequest(request_headers); ASSERT_TRUE(response->waitForEndStream()); @@ -647,7 +647,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicWithoutWarmingFail) { codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); // Update should not cause a different response. sendXdsResponse("bar", "1", invalidConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.bar.config_fail", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.bar.config_fail", 1); Http::TestRequestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); @@ -665,7 +665,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicTwoSubscriptionsSameName) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("baz", "1", denyPrivateConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.baz.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.baz.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -704,7 +704,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, BasicFailTerminalFilterNotAtEndOfFilte EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", terminalFilterConfig(), false, false); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_fail", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_fail", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -726,7 +726,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReloadBoth) { EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initializing); registerTestServerPorts({"http"}); sendXdsResponse("foo", "1", denyPrivateConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 1); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 1); test_server_->waitUntilListenersReady(); test_server_->waitForGaugeGe("listener_manager.workers_started", 1); EXPECT_EQ(test_server_->server().initManager().state(), Init::Manager::State::Initialized); @@ -761,7 +761,7 @@ TEST_P(ExtensionDiscoveryIntegrationTest, ReloadBoth) { // Update ECDS but keep the connection. { sendXdsResponse("foo", "2", allowAllConfig()); - test_server_->waitForCounterGe("http_filter.extension_config_discovery.foo.config_reload", 2); + test_server_->waitForCounterGe("extension_config_discovery.http_filter.foo.config_reload", 2); auto response = codec_client_->makeHeaderOnlyRequest(banned_request_headers); ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete());