envoyproxy · mattklein123 · Oct 15, 2021 · Sep 2, 2021 · Oct 11, 2021 · Oct 12, 2021
diff --git 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. <envoy_v3_api_field_config.cluster.v3.Cluster.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
@@ -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
@@ -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<bool> is_draining_{false};
 };

diff --git a/test/integration/server.h b/test/integration/server.h
@@ -478,6 +478,11 @@ class IntegrationTestServer : public Logger::Loggable<Logger::Id::testing>,
     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
@@ -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
@@ -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,20 +399,14 @@ 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);
 
   auto codec_client_1 = createHttpCodec("alpn1");
   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,14 +468,17 @@ 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();
   });
   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
@@ -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
@@ -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.