srds: Allow SRDS pass on scope-not-found queries to filter-chain (issue #8236).

source/common/http/BUILD

@@ -184,6 +184,7 @@ envoy_cc_library(
         "//source/common/http/http1:codec_lib",
         "//source/common/http/http2:codec_lib",
         "//source/common/network:utility_lib",
+        "//source/common/router:config_lib",
         "//source/common/runtime:uuid_util_lib",
         "//source/common/stream_info:stream_info_lib",
         "//source/common/tracing:http_tracer_lib",

source/common/http/conn_manager_impl.cc

@@ -34,6 +34,7 @@
 #include "common/http/path_utility.h"
 #include "common/http/utility.h"
 #include "common/network/utility.h"
+#include "common/router/config_impl.h"
 #include "common/runtime/runtime_impl.h"
 
 #include "absl/strings/escaping.h"
@@ -635,9 +636,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
     ASSERT(snapped_route_config_ == nullptr,
            "Route config already latched to the active stream when scoped RDS is enabled.");
     // We need to snap snapped_route_config_ here as it's used in mutateRequestHeaders later.
-    if (!snapScopedRouteConfig()) {
-      return;
-    }
+    snapScopedRouteConfig();
   }
 
   if (Http::Headers::get().MethodValues.Head ==
@@ -1247,23 +1246,19 @@ void ConnectionManagerImpl::startDrainSequence() {
   drain_timer_->enableTimer(config_.drainTimeout());
 }
 
-bool ConnectionManagerImpl::ActiveStream::snapScopedRouteConfig() {
+void ConnectionManagerImpl::ActiveStream::snapScopedRouteConfig() {
   ASSERT(request_headers_ != nullptr,
          "Try to snap scoped route config when there is no request headers.");
 
-  snapped_route_config_ = snapped_scoped_routes_config_->getRouteConfig(*request_headers_);
   // NOTE: if a RDS subscription hasn't got a RouteConfiguration back, a Router::NullConfigImpl is
   // returned, in that case we let it pass.
+  snapped_route_config_ = snapped_scoped_routes_config_->getRouteConfig(*request_headers_);
   if (snapped_route_config_ == nullptr) {
     ENVOY_STREAM_LOG(trace, "can't find SRDS scope.", *this);
-    // Stop decoding now.
-    maybeEndDecode(true);
-    sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Http::Code::NotFound,
-                   "route scope not found", nullptr, is_head_request_, absl::nullopt,
-                   StreamInfo::ResponseCodeDetails::get().RouteConfigurationNotFound);
-    return false;
+    // TODO(stevenzzzz): Consider to pass an error message to router filter, so that it can
+    // send back 404 with some more details.
+    snapped_route_config_ = std::make_shared<Router::NullConfigImpl>();
   }
-  return true;
 }
 
 void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() {

source/common/http/conn_manager_impl.h

@@ -497,10 +497,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
 
     void traceRequest();
 
-    // Updates the snapped_route_config_ if scope found, or ends the stream by
-    // sending local reply.
-    // Returns true if scoped route config snapped, false otherwise.
-    bool snapScopedRouteConfig();
+    // Updates the snapped_route_config_ (by reselecting scoped route configuration), if a scope is
+    // not found, snapped_route_config_ is set to Router::NullConfigImpl.
+    void snapScopedRouteConfig();
 
     void refreshCachedRoute();
 

test/common/http/conn_manager_impl_test.cc

@@ -4510,13 +4510,15 @@ TEST_F(HttpConnectionManagerImplTest, TestSessionTrace) {
 }
 
 // SRDS no scope found.
-TEST_F(HttpConnectionManagerImplTest, TestSRDSRouteNotFound) {
+TEST_F(HttpConnectionManagerImplTest, TestSrdsRouteNotFound) {
   setup(false, "", true, true);
+  setupFilterChain(1, 0); // Recreate the chain for second stream.
 
   EXPECT_CALL(*static_cast<const Router::MockScopedConfig*>(
                   scopedRouteConfigProvider()->config<Router::ScopedConfig>().get()),
               getRouteConfig(_))
-      .WillOnce(Return(nullptr));
+      .Times(2)
+      .WillRepeatedly(Return(nullptr));
   EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void {
     StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_);
     HeaderMapPtr headers{
@@ -4525,74 +4527,67 @@ TEST_F(HttpConnectionManagerImplTest, TestSRDSRouteNotFound) {
     data.drain(4);
   }));
 
-  EXPECT_CALL(response_encoder_, encodeHeaders(_, false))
-      .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void {
-        EXPECT_EQ("404", headers.Status()->value().getStringView());
+  EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true))
+      .WillOnce(InvokeWithoutArgs([&]() -> FilterHeadersStatus {
+        EXPECT_EQ(nullptr, decoder_filters_[0]->callbacks_->route());
+        return FilterHeadersStatus::StopIteration;
       }));
-
-  std::string response_body;
-  EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body));
+  EXPECT_CALL(*decoder_filters_[0], decodeComplete()); // end_stream=true.
 
   Buffer::OwnedImpl fake_input("1234");
   conn_manager_->onData(fake_input, false);
-  EXPECT_EQ(response_body, "route scope not found");
 }
 
 // SRDS updating scopes affects routing.
-TEST_F(HttpConnectionManagerImplTest, TestSRDSUpdate) {
+TEST_F(HttpConnectionManagerImplTest, TestSrdsUpdate) {
   setup(false, "", true, true);
 
   EXPECT_CALL(*static_cast<const Router::MockScopedConfig*>(
                   scopedRouteConfigProvider()->config<Router::ScopedConfig>().get()),
               getRouteConfig(_))
       .Times(3)
       .WillOnce(Return(nullptr))
-      .WillOnce(Return(route_config_))
-      .WillOnce(Return(route_config_)); // refreshCachedRoute
-  EXPECT_CALL(*codec_, dispatch(_))
-      .Times(2) // Once for no scoped routes, once for scoped routing
-      .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void {
-        StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_);
-        HeaderMapPtr headers{
-            new TestHeaderMapImpl{{":authority", "host"}, {":method", "GET"}, {":path", "/foo"}}};
-        decoder->decodeHeaders(std::move(headers), true);
-        data.drain(4);
-      }));
-  EXPECT_CALL(response_encoder_, encodeHeaders(_, false))
-      .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void {
-        EXPECT_EQ("404", headers.Status()->value().getStringView());
-      }));
-
-  std::string response_body;
-  EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body));
-
-  Buffer::OwnedImpl fake_input("1234");
-  conn_manager_->onData(fake_input, false);
-  EXPECT_EQ(response_body, "route scope not found");
-
-  // Now route config provider returns something.
-  setupFilterChain(1, 0); // Recreate the chain for second stream.
+      .WillOnce(Return(nullptr))        // refreshCachedRoute first time.
+      .WillOnce(Return(route_config_)); // triggered by callbacks_->route(), SRDS now updated.
+  EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void {
+    StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_);
+    HeaderMapPtr headers{
+        new TestHeaderMapImpl{{":authority", "host"}, {":method", "GET"}, {":path", "/foo"}}};
+    decoder->decodeHeaders(std::move(headers), true);
+    data.drain(4);
+  }));
   const std::string fake_cluster1_name = "fake_cluster1";
   std::shared_ptr<Router::MockRoute> route1 = std::make_shared<NiceMock<Router::MockRoute>>();
   EXPECT_CALL(route1->route_entry_, clusterName()).WillRepeatedly(ReturnRef(fake_cluster1_name));
   std::shared_ptr<Upstream::MockThreadLocalCluster> fake_cluster1 =
       std::make_shared<NiceMock<Upstream::MockThreadLocalCluster>>();
   EXPECT_CALL(cluster_manager_, get(_)).WillOnce(Return(fake_cluster1.get()));
   EXPECT_CALL(*route_config_, route(_, _)).WillOnce(Return(route1));
+  // First no-scope-found request will be handled by decoder_filters_[0].
+  setupFilterChain(1, 0);
   EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true))
       .WillOnce(InvokeWithoutArgs([&]() -> FilterHeadersStatus {
+        EXPECT_EQ(nullptr, decoder_filters_[0]->callbacks_->route());
+
+        // Clear route and next call on callbacks_->route() will trigger a re-snapping of the
+        // snapped_route_config_.
+        decoder_filters_[0]->callbacks_->clearRouteCache();
+
+        // Now route config provider returns something.
         EXPECT_EQ(route1, decoder_filters_[0]->callbacks_->route());
         EXPECT_EQ(route1->routeEntry(), decoder_filters_[0]->callbacks_->streamInfo().routeEntry());
         EXPECT_EQ(fake_cluster1->info(), decoder_filters_[0]->callbacks_->clusterInfo());
         return FilterHeadersStatus::StopIteration;
+
+        return FilterHeadersStatus::StopIteration;
       }));
-  EXPECT_CALL(*decoder_filters_[0], decodeComplete());
-  Buffer::OwnedImpl fake_input2("1234");
-  conn_manager_->onData(fake_input2, false);
+  EXPECT_CALL(*decoder_filters_[0], decodeComplete()); // end_stream=true.
+  Buffer::OwnedImpl fake_input("1234");
+  conn_manager_->onData(fake_input, false);
 }
 
 // SRDS Scope header update cause cross-scope reroute.
-TEST_F(HttpConnectionManagerImplTest, TestSRDSCrossScopeReroute) {
+TEST_F(HttpConnectionManagerImplTest, TestSrdsCrossScopeReroute) {
   setup(false, "", true, true);
 
   std::shared_ptr<Router::MockConfig> route_config1 =
@@ -4650,7 +4645,7 @@ TEST_F(HttpConnectionManagerImplTest, TestSRDSCrossScopeReroute) {
 }
 
 // SRDS scoped RouteConfiguration found and route found.
-TEST_F(HttpConnectionManagerImplTest, TestSRDSRouteFound) {
+TEST_F(HttpConnectionManagerImplTest, TestSrdsRouteFound) {
   setup(false, "", true, true);
   setupFilterChain(1, 0);
 

test/integration/http2_integration_test.cc

@@ -1433,7 +1433,7 @@ const int64_t TransmitThreshold = 100 * 1024 * 1024;
 
 void Http2FloodMitigationTest::setNetworkConnectionBufferSize() {
   // nghttp2 library has its own internal mitigation for outbound control frames. The mitigation is
-  // trigerred when there are more than 10000 PING or SETTINGS frames with ACK flag in the nghttp2
+  // triggered when there are more than 10000 PING or SETTINGS frames with ACK flag in the nghttp2
   // internal outbound queue. It is possible to trigger this mitigation in nghttp2 before triggering
   // Envoy's own flood mitigation. This can happen when a buffer larger enough to contain over 10K
   // PING or SETTINGS frames is dispatched to the nghttp2 library. To prevent this from happening

test/integration/scoped_rds_integration_test.cc

@@ -266,7 +266,7 @@ route_configuration_name: {}
                               {":scheme", "http"},
                               {"Addr", "x-foo-key=xyz-route"}});
   response->waitForEndStream();
-  verifyResponse(std::move(response), "404", Http::TestHeaderMapImpl{}, "route scope not found");
+  verifyResponse(std::move(response), "404", Http::TestHeaderMapImpl{}, "");
   cleanupUpstreamAndDownstream();
 
   // Test "foo-route" and 'bar-route' both gets routed to cluster_0.
@@ -337,7 +337,7 @@ route_configuration_name: {}
                               {":scheme", "http"},
                               {"Addr", "x-foo-key=foo-route"}});
   response->waitForEndStream();
-  verifyResponse(std::move(response), "404", Http::TestHeaderMapImpl{}, "route scope not found");
+  verifyResponse(std::move(response), "404", Http::TestHeaderMapImpl{}, "");
   cleanupUpstreamAndDownstream();
   // Add a new scope foo_scope4.
   const std::string& scope_route4 =
@@ -398,7 +398,7 @@ route_configuration_name: foo_route1
                                                                    {":scheme", "http"},
                                                                    {"Addr", "x-foo-key=foo"}});
   response->waitForEndStream();
-  verifyResponse(std::move(response), "404", Http::TestHeaderMapImpl{}, "route scope not found");
+  verifyResponse(std::move(response), "404", Http::TestHeaderMapImpl{}, "");
   cleanupUpstreamAndDownstream();
 
   // SRDS update fixed the problem.