vhds: fixing a filter teardown bug (#11341)

This is to help with #9784 Risk Level: Low (added a single test) Testing: a new unit test and integration test Docs Changes: n/a Release Notes: n/a Signed-off-by: Dmitri Dolguikh <[email protected]>
envoyproxy · Jun 10, 2020 · 9d702c1 · 9d702c1
1 parent 8fd1a52
commit 9d702c1
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 3 deletions.
diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc
@@ -283,7 +283,7 @@ void RdsRouteConfigProviderImpl::onConfigUpdate() {
       Http::RequestHeaderMapImpl host_header;
       host_header.setHost(VhdsSubscription::aliasToDomainName(it->alias_));
       const bool host_exists = config->virtualHostExists(host_header);
-      auto current_cb = it->cb_;
+      std::weak_ptr<Http::RouteConfigUpdatedCallback> current_cb(it->cb_);
       it->thread_local_dispatcher_.post([current_cb, host_exists] {
         if (auto cb = current_cb.lock()) {
           (*cb)(host_exists);

diff --git a/source/extensions/filters/http/on_demand/on_demand_update.cc b/source/extensions/filters/http/on_demand/on_demand_update.cc
@@ -37,6 +37,11 @@ void OnDemandRouteUpdate::setDecoderFilterCallbacks(Http::StreamDecoderFilterCal
   callbacks_ = &callbacks;
 }
 
+// A weak_ptr copy of the route_config_updated_callback_ is kept by RdsRouteConfigProviderImpl
+// in config_update_callbacks_. By resetting the pointer in onDestroy() callback we ensure
+// that this filter/filter-chain will not be resumed if the corresponding has been closed
+void OnDemandRouteUpdate::onDestroy() { route_config_updated_callback_.reset(); }
+
 // This is the callback which is called when an update requested in requestRouteConfigUpdate()
 // has been propagated to workers, at which point the request processing is restarted from the
 // beginning.

diff --git a/source/extensions/filters/http/on_demand/on_demand_update.h b/source/extensions/filters/http/on_demand/on_demand_update.h
@@ -27,7 +27,7 @@ class OnDemandRouteUpdate : public Http::StreamDecoderFilter {
 
   void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override;
 
-  void onDestroy() override {}
+  void onDestroy() override;
 
 private:
   Http::StreamDecoderFilterCallbacks* callbacks_{};

diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc
@@ -653,5 +653,38 @@ TEST_P(VhdsIntegrationTest, VhdsOnDemandUpdateFailToResolveOneAliasOutOfSeveral)
   cleanupUpstreamAndDownstream();
 }
 
+// Verify that an vhds update succeeds even when the client closes its connection
+TEST_P(VhdsIntegrationTest, VhdsOnDemandUpdateHttpConnectionCloses) {
+  // RDS exchange with a non-empty virtual_hosts field
+  useRdsWithVhosts();
+
+  testRouterHeaderOnlyRequestAndResponse(nullptr, 1);
+  cleanupUpstreamAndDownstream();
+  EXPECT_TRUE(codec_client_->waitForDisconnect());
+
+  // Attempt to make a request to an unknown host
+  codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));
+  Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"},
+                                                 {":path", "/"},
+                                                 {":scheme", "http"},
+                                                 {":authority", "vhost_1"},
+                                                 {"x-lyft-user-id", "123"}};
+  auto encoder_decoder = codec_client_->startRequest(request_headers);
+  Http::RequestEncoder& encoder = encoder_decoder.first;
+  IntegrationStreamDecoderPtr response = std::move(encoder_decoder.second);
+  EXPECT_TRUE(compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost,
+                                           {vhdsRequestResourceName("vhost_1")}, {}, vhds_stream_));
+
+  envoy::api::v2::DeltaDiscoveryResponse vhds_update =
+      createDeltaDiscoveryResponseWithResourceNameUsedAsAlias();
+  vhds_stream_->sendGrpcMessage(vhds_update);
+
+  codec_client_->sendReset(encoder);
+  response->waitForReset();
+  EXPECT_TRUE(codec_client_->connected());
+
+  cleanupUpstreamAndDownstream();
+}
+
 } // namespace
-} // namespace Envoy
+} // namespace Envoy