Skip to content

Commit

Permalink
srds: allow SRDS pass on scope-not-found queries to filter-chain (issue
Browse files Browse the repository at this point in the history 
envoyproxy#8236).  (envoyproxy#8239)

Description: Allow a no-scope request to pass through the filter chain, so that some special queries (e.g., data plane health-check ) can be processed by the customized filter-chain. By default, the behavior is the same (404).
Risk Level: LOW
Testing: unit test and integration test.
Docs Changes: N/A
Release Notes: N/A
Fixes envoyproxy#8236
Signed-off-by: Xin Zhuang <stevenzzz@google.com>
  • Loading branch information
@stevenzzzz
stevenzzzz committed Sep 16, 2019
1 parent f22771d commit 4146153
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 60 deletions.
1 change: 1 addition & 0 deletions source/common/http/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
19 changes: 7 additions & 12 deletions source/common/http/conn_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 ==
Expand Down Expand Up @@ -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() {
Expand Down
7 changes: 3 additions & 4 deletions source/common/http/conn_manager_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
75 changes: 35 additions & 40 deletions test/common/http/conn_manager_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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 =
Expand Down Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion test/integration/http2_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions test/integration/scoped_rds_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit 4146153

Please sign in to comment.