diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 044548ae6528..d06103d37389 100644 --- a/source/common/http/BUILD +++ b/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", diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 3c92f5903871..fab1473b0216 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/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 "extensions/quic_listeners/quiche/codec_impl.h" @@ -653,9 +654,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 == @@ -1265,23 +1264,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(); } - return true; } void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 9302459a1f09..ab1af23a7ddb 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -497,10 +497,9 @@ class ConnectionManagerImpl : Logger::Loggable, 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(); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 2b8cc61b2a1f..00d80d4c6d1d 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/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( scopedRouteConfigProvider()->config().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,21 +4527,19 @@ 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( @@ -4547,31 +4547,15 @@ TEST_F(HttpConnectionManagerImplTest, TestSRDSUpdate) { 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 route1 = std::make_shared>(); EXPECT_CALL(route1->route_entry_, clusterName()).WillRepeatedly(ReturnRef(fake_cluster1_name)); @@ -4579,20 +4563,31 @@ TEST_F(HttpConnectionManagerImplTest, TestSRDSUpdate) { std::make_shared>(); 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 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); diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 197522b3cd59..1dbe5cd37fdd 100644 --- a/test/integration/http2_integration_test.cc +++ b/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 diff --git a/test/integration/scoped_rds_integration_test.cc b/test/integration/scoped_rds_integration_test.cc index 4bcc2a38997b..5f4bb27395e4 100644 --- a/test/integration/scoped_rds_integration_test.cc +++ b/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.