diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 86f8d8ad7f955..e828a7978a5bd 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -11,6 +11,7 @@ Minor Behavior Changes * config: the log message for "gRPC config stream closed" now uses the most recent error message, and reports seconds instead of milliseconds for how long the most recent status has been received. * dns: now respecting the returned DNS TTL for resolved hosts, rather than always relying on the hard-coded :ref:`dns_refresh_rate. ` This behavior can be temporarily reverted by setting the runtime guard ``envoy.reloadable_features.use_dns_ttl`` to false. +* listener: destroy per network filter chain stats when a network filter chain is removed during the listener in place update. Bug Fixes --------- diff --git a/source/server/filter_chain_manager_impl.cc b/source/server/filter_chain_manager_impl.cc index f99ed1f1858c0..26dee2401e9ce 100644 --- a/source/server/filter_chain_manager_impl.cc +++ b/source/server/filter_chain_manager_impl.cc @@ -31,7 +31,9 @@ Network::Address::InstanceConstSharedPtr fakeAddress() { PerFilterChainFactoryContextImpl::PerFilterChainFactoryContextImpl( Configuration::FactoryContext& parent_context, Init::Manager& init_manager) - : parent_context_(parent_context), init_manager_(init_manager) {} + : parent_context_(parent_context), scope_(parent_context_.scope().createScope("")), + filter_chain_scope_(parent_context_.listenerScope().createScope("")), + init_manager_(init_manager) {} bool PerFilterChainFactoryContextImpl::drainClose() const { return is_draining_.load() || parent_context_.drainDecision().drainClose(); @@ -101,7 +103,7 @@ Envoy::Runtime::Loader& PerFilterChainFactoryContextImpl::runtime() { return parent_context_.runtime(); } -Stats::Scope& PerFilterChainFactoryContextImpl::scope() { return parent_context_.scope(); } +Stats::Scope& PerFilterChainFactoryContextImpl::scope() { return *scope_; } Singleton::Manager& PerFilterChainFactoryContextImpl::singletonManager() { return parent_context_.singletonManager(); @@ -135,9 +137,7 @@ PerFilterChainFactoryContextImpl::getTransportSocketFactoryContext() const { return parent_context_.getTransportSocketFactoryContext(); } -Stats::Scope& PerFilterChainFactoryContextImpl::listenerScope() { - return parent_context_.listenerScope(); -} +Stats::Scope& PerFilterChainFactoryContextImpl::listenerScope() { return *filter_chain_scope_; } bool PerFilterChainFactoryContextImpl::isQuicListener() const { return parent_context_.isQuicListener(); diff --git a/source/server/filter_chain_manager_impl.h b/source/server/filter_chain_manager_impl.h index 6bc170c93e01d..4e9cb2576797b 100644 --- a/source/server/filter_chain_manager_impl.h +++ b/source/server/filter_chain_manager_impl.h @@ -88,6 +88,10 @@ class PerFilterChainFactoryContextImpl : public Configuration::FilterChainFactor private: Configuration::FactoryContext& parent_context_; + // The scope that has empty prefix. + Stats::ScopePtr scope_; + // filter_chain_scope_ has the same prefix as listener owners scope. + Stats::ScopePtr filter_chain_scope_; Init::Manager& init_manager_; std::atomic is_draining_{false}; }; diff --git a/test/integration/server.h b/test/integration/server.h index 5144f2203646e..28d9aa96691f0 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -478,6 +478,11 @@ class IntegrationTestServer : public Logger::Loggable, notifyingStatsAllocator().waitForCounterExists(name); } + // TODO(#17956): Add Gauge type to NotifyingAllocator and adopt it in this method. + void waitForGaugeDestroyed(const std::string& name) override { + ASSERT_TRUE(TestUtility::waitForGaugeDestroyed(statStore(), name, time_system_)); + } + void waitUntilHistogramHasSamples( const std::string& name, std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()) override { diff --git a/test/integration/server_stats.h b/test/integration/server_stats.h index 66cb7e07e7d27..d4520d3456db5 100644 --- a/test/integration/server_stats.h +++ b/test/integration/server_stats.h @@ -66,6 +66,12 @@ class IntegrationTestServerStats { waitForGaugeEq(const std::string& name, uint64_t value, std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()) PURE; + /** + * Wait for a gauge to be destroyed. Note that MockStatStore does not destroy stat. + * @param name gauge name. + */ + virtual void waitForGaugeDestroyed(const std::string& name) PURE; + /** * Counter lookup. This is not thread safe, since we don't get a consistent * snapshot, uses counters() instead for this behavior. diff --git a/test/integration/xds_integration_test.cc b/test/integration/xds_integration_test.cc index f0a0dec1f52e4..b225c4e1f527b 100644 --- a/test/integration/xds_integration_test.cc +++ b/test/integration/xds_integration_test.cc @@ -301,6 +301,9 @@ class LdsInplaceUpdateHttpIntegrationTest std::string tls_inspector_config = ConfigHelper::tlsInspectorFilter(); config_helper_.addListenerFilter(tls_inspector_config); config_helper_.addSslConfig(); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { hcm.mutable_stat_prefix()->assign("hcm0"); }); config_helper_.addConfigModifier([this, add_default_filter_chain]( envoy::config::bootstrap::v3::Bootstrap& bootstrap) { if (!use_default_balancer_) { @@ -335,6 +338,7 @@ class LdsInplaceUpdateHttpIntegrationTest ->mutable_routes(0) ->mutable_route() ->set_cluster("cluster_1"); + hcm_config.mutable_stat_prefix()->assign("hcm1"); config_blob->PackFrom(hcm_config); bootstrap.mutable_static_resources()->mutable_clusters()->Add()->MergeFrom( *bootstrap.mutable_static_resources()->mutable_clusters(0)); @@ -381,7 +385,7 @@ class LdsInplaceUpdateHttpIntegrationTest } } - void expectConnenctionServed(std::string alpn = "alpn0") { + void expectConnectionServed(std::string alpn = "alpn0") { auto codec_client_after_config_update = createHttpCodec(alpn); expectResponseHeaderConnectionClose(*codec_client_after_config_update, false); codec_client_after_config_update->close(); @@ -395,7 +399,7 @@ class LdsInplaceUpdateHttpIntegrationTest }; // Verify that http response on filter chain 1 and default filter chain have "Connection: close" -// header when these 2 filter chains are deleted during the listener update. +// header when these 2 filter chains are deleted during the listener update. TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigDeletingFilterChain) { inplaceInitialize(/*add_default_filter_chain=*/true); @@ -403,12 +407,6 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigDeletingFilterChain) { auto codec_client_0 = createHttpCodec("alpn0"); auto codec_client_default = createHttpCodec("alpndefault"); - Cleanup cleanup([c1 = codec_client_1.get(), c0 = codec_client_0.get(), - c_default = codec_client_default.get()]() { - c1->close(); - c0->close(); - c_default->close(); - }); ConfigHelper new_config_helper( version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); new_config_helper.addConfigModifier( @@ -422,12 +420,20 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigDeletingFilterChain) { test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1); test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 1); + test_server_->waitForGaugeGe("http.hcm0.downstream_cx_active", 1); + test_server_->waitForGaugeGe("http.hcm1.downstream_cx_active", 1); + expectResponseHeaderConnectionClose(*codec_client_1, true); expectResponseHeaderConnectionClose(*codec_client_default, true); test_server_->waitForGaugeGe("listener_manager.total_filter_chains_draining", 0); expectResponseHeaderConnectionClose(*codec_client_0, false); - expectConnenctionServed(); + expectConnectionServed(); + + codec_client_1->close(); + test_server_->waitForGaugeDestroyed("http.hcm1.downstream_cx_active"); + codec_client_0->close(); + codec_client_default->close(); } // Verify that http clients of filter chain 0 survives if new listener config adds new filter @@ -438,15 +444,19 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigAddingFilterChain) { auto codec_client_0 = createHttpCodec("alpn0"); Cleanup cleanup0([c0 = codec_client_0.get()]() { c0->close(); }); + test_server_->waitForGaugeGe("http.hcm0.downstream_cx_active", 1); + ConfigHelper new_config_helper( version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap())); new_config_helper.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); - listener->mutable_filter_chains()->Add()->MergeFrom(*listener->mutable_filter_chains(1)); + // Note that HCM2 copies the stats prefix from HCM0 + listener->mutable_filter_chains()->Add()->MergeFrom(*listener->mutable_filter_chains(0)); *listener->mutable_filter_chains(2) ->mutable_filter_chain_match() ->mutable_application_protocols(0) = "alpn2"; + auto default_filter_chain = bootstrap.mutable_static_resources()->mutable_listeners(0)->mutable_default_filter_chain(); default_filter_chain->MergeFrom(*listener->mutable_filter_chains(1)); @@ -458,6 +468,9 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigAddingFilterChain) { auto codec_client_2 = createHttpCodec("alpn2"); auto codec_client_default = createHttpCodec("alpndefault"); + // 1 connection from filter chain 0 and 1 connection from filter chain 2. + test_server_->waitForGaugeGe("http.hcm0.downstream_cx_active", 2); + Cleanup cleanup2([c2 = codec_client_2.get(), c_default = codec_client_default.get()]() { c2->close(); c_default->close(); @@ -465,7 +478,7 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigAddingFilterChain) { expectResponseHeaderConnectionClose(*codec_client_2, false); expectResponseHeaderConnectionClose(*codec_client_default, false); expectResponseHeaderConnectionClose(*codec_client_0, false); - expectConnenctionServed(); + expectConnectionServed(); } // Verify that http clients of default filter chain is drained and recreated if the default filter @@ -493,7 +506,7 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, ReloadConfigUpdatingDefaultFilterCha Cleanup cleanup2([c_default_v3 = codec_client_default_v3.get()]() { c_default_v3->close(); }); expectResponseHeaderConnectionClose(*codec_client_default, true); expectResponseHeaderConnectionClose(*codec_client_default_v3, false); - expectConnenctionServed(); + expectConnectionServed(); } // Verify that balancer is inherited. Test only default balancer because ExactConnectionBalancer @@ -515,7 +528,7 @@ TEST_P(LdsInplaceUpdateHttpIntegrationTest, OverlappingFilterChainServesNewConne new_config_helper.setLds("1"); test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1); expectResponseHeaderConnectionClose(*codec_client_0, false); - expectConnenctionServed(); + expectConnectionServed(); } // Verify default filter chain update is filter chain only update. diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index fe0db0bcf7ee8..74d009a421890 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -238,6 +238,14 @@ AssertionResult TestUtility::waitForGaugeEq(Stats::Store& store, const std::stri return AssertionSuccess(); } +AssertionResult TestUtility::waitForGaugeDestroyed(Stats::Store& store, const std::string& name, + Event::TestTimeSystem& time_system) { + while (findGauge(store, name) != nullptr) { + time_system.advanceTimeWait(std::chrono::milliseconds(10)); + } + return AssertionSuccess(); +} + AssertionResult TestUtility::waitUntilHistogramHasSamples(Stats::Store& store, const std::string& name, Event::TestTimeSystem& time_system, diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 64000d047bc93..0b3b0c4a6bb82 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -277,6 +277,17 @@ class TestUtility { Event::TestTimeSystem& time_system, std::chrono::milliseconds timeout = std::chrono::milliseconds::zero()); + /** + * Wait for a gauge to be destroyed. + * @param store supplies the stats store. + * @param name gauge name. + * @param time_system the time system to use for waiting. + * @return AssertionSuccess() if the gauge is destroyed within a fixed timeout, else + * AssertionFailure(). + */ + static AssertionResult waitForGaugeDestroyed(Stats::Store& store, const std::string& name, + Event::TestTimeSystem& time_system); + /** * Wait for a histogram to have samples. * @param store supplies the stats store.