diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 71c41c24694b5..a19cfedc3e1a0 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -55,6 +55,7 @@ Minor Behavior Changes to false. As part of this change, the use of reuse_port for TCP listeners on both macOS and Windows has been disabled due to suboptimal behavior. See the field documentation for more information. +* 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 f1a74a70e78cd..01e63097f2c63 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 8218e89777127..3919aeb81073d 100644 --- a/source/server/filter_chain_manager_impl.h +++ b/source/server/filter_chain_manager_impl.h @@ -87,6 +87,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 94d5f79504cbe..109743e551b23 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -462,6 +462,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 7a11e6a156470..8e866132782b7 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..c2f68cc000c6d 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 c355b50643394..e7fa952614c92 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 was == to the value within the 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.