Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ext_proc fuzzer test trigger ENVOY_BUG when clear route cache #27657

Merged
merged 9 commits into from
Jun 12, 2023
Merged

ext_proc fuzzer test trigger ENVOY_BUG when clear route cache #27657

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d7da5f5
ext_proc fuzzer test trigger ENVOY_BUG when clear route cache for ups…
yanjunxiang-google May 26, 2023
cc634a9
removing outbound stats
yanjunxiang-google May 30, 2023
f866784
Merge branch 'main' of https://github.com/envoyproxy/envoy into ext_p…
yanjunxiang-google May 30, 2023
488332c
addressing comments
yanjunxiang-google May 31, 2023
8a2cf5b
removing logs
yanjunxiang-google Jun 2, 2023
3e80bed
move clear route cache to decoder processing only
yanjunxiang-google Jun 2, 2023
2528ca0
addressing comments
yanjunxiang-google Jun 2, 2023
b09a844
adding back logs
yanjunxiang-google Jun 2, 2023
d8418bb
backing out the change in external_processor.proto as we will change…
yanjunxiang-google Jun 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion source/extensions/filters/http/ext_proc/ext_proc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ namespace ExternalProcessing {
COUNTER(override_message_timeout_received) \
COUNTER(override_message_timeout_ignored) \
COUNTER(clear_route_cache_ignored) \
COUNTER(clear_route_cache_disabled)
COUNTER(clear_route_cache_disabled) \
COUNTER(clear_route_cache_not_outbound)
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved

struct ExtProcFilterStats {
ALL_EXT_PROC_FILTER_STATS(GENERATE_COUNTER_STRUCT)
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/filters/http/ext_proc/processor_state.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,10 @@ void ProcessorState::clearRouteCache(const CommonResponse& common_response) {
if (filter_.config().disableClearRouteCache()) {
filter_.stats().clear_route_cache_disabled_.inc();
ENVOY_LOG(debug, "NOT clearing route cache, it is disabled in the config");
} else if (trafficDirection() == envoy::config::core::v3::TrafficDirection::OUTBOUND ) {
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
// Do not clear the route cache for outbound response traffic.
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
filter_.stats().clear_route_cache_not_outbound_.inc();
ENVOY_LOG(debug, "NOT clearing route cache for OUTBOUND response traffic");
} else if (common_response.has_header_mutation()) {
ENVOY_LOG(debug, "clearing route cache");
filter_callbacks_->downstreamCallbacks()->clearRouteCache();
Expand Down
47 changes: 47 additions & 0 deletions test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2086,4 +2086,51 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersAndTrailersWithHeaderScrubbing) {
verifyDownstreamResponse(*response, 200);
}

TEST_P(ExtProcIntegrationTest, GetAndSetBodyOnBothWithClearRouteCache) {
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
proto_config_.mutable_processing_mode()->set_request_body_mode(ProcessingMode::BUFFERED);
proto_config_.mutable_processing_mode()->set_response_body_mode(ProcessingMode::BUFFERED);
initializeConfig();
HttpIntegrationTest::initialize();

auto response = sendDownstreamRequestWithBody("Replace this!", absl::nullopt);

processRequestHeadersMessage(
*grpc_upstreams_[0], true, [](const HttpHeaders&, HeadersResponse& headers_resp) {
auto* content_length =
headers_resp.mutable_response()->mutable_header_mutation()->add_set_headers();
content_length->mutable_header()->set_key("content-length");
content_length->mutable_header()->set_value("13");
headers_resp.mutable_response()->set_clear_route_cache(true);
return true;
});

processRequestBodyMessage(
*grpc_upstreams_[0], false, [](const HttpBody& body, BodyResponse& body_resp) {
EXPECT_TRUE(body.end_of_stream());
auto* body_mut = body_resp.mutable_response()->mutable_body_mutation();
body_mut->set_body("Hello, World!");
return true;
});

handleUpstreamRequest();

processResponseHeadersMessage(
*grpc_upstreams_[0], false, [](const HttpHeaders&, HeadersResponse& headers_resp) {
headers_resp.mutable_response()->mutable_header_mutation()->add_remove_headers(
"content-length");
headers_resp.mutable_response()->set_clear_route_cache(true);
return true;
});

processResponseBodyMessage(
*grpc_upstreams_[0], false, [](const HttpBody& body, BodyResponse& body_resp) {
EXPECT_TRUE(body.end_of_stream());
body_resp.mutable_response()->mutable_body_mutation()->set_body("123");
return true;
});

verifyDownstreamResponse(*response, 200);
EXPECT_EQ("123", response->body());
}

} // namespace Envoy
17 changes: 11 additions & 6 deletions test/extensions/filters/http/ext_proc/filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2019,7 +2019,8 @@ TEST_F(HttpFilterTest, ProcessingModeResponseHeadersOnlyWithoutCallingDecodeHead
}

// Using the default configuration, verify that the "clear_route_cache" flag makes the appropriate
// callback on the filter when header modifications are also present.
// callback on the filter for inbound traffic when header modifications are also present.
// For outbound traffic, it does not make the callback.
TEST_F(HttpFilterTest, ClearRouteCacheHeaderMutation) {
initialize(R"EOF(
grpc_service:
Expand All @@ -2030,7 +2031,7 @@ TEST_F(HttpFilterTest, ClearRouteCacheHeaderMutation) {
)EOF");

EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true));

// Call ClearRouteCache() for inbound traffic with header mutation.
EXPECT_CALL(decoder_callbacks_.downstream_callbacks_, clearRouteCache());
processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) {
auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation();
Expand All @@ -2047,9 +2048,8 @@ TEST_F(HttpFilterTest, ClearRouteCacheHeaderMutation) {
Buffer::OwnedImpl buffered_response_data;
setUpEncodingBuffering(buffered_response_data);

// Do not call ClearRouteCache() for outbound traffic.
EXPECT_EQ(FilterDataStatus::StopIterationNoBuffer, filter_->encodeData(resp_data, true));

EXPECT_CALL(encoder_callbacks_.downstream_callbacks_, clearRouteCache());
processResponseBody([](const HttpBody&, ProcessingResponse&, BodyResponse& resp) {
auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation();
auto* resp_add = resp_headers_mut->add_set_headers();
Expand All @@ -2060,6 +2060,7 @@ TEST_F(HttpFilterTest, ClearRouteCacheHeaderMutation) {

filter_->onDestroy();

EXPECT_EQ(config_->stats().clear_route_cache_not_outbound_.value(), 1);
EXPECT_EQ(config_->stats().streams_started_.value(), 1);
EXPECT_EQ(config_->stats().stream_msgs_sent_.value(), 3);
EXPECT_EQ(config_->stats().stream_msgs_received_.value(), 3);
Expand Down Expand Up @@ -2106,6 +2107,7 @@ TEST_F(HttpFilterTest, ClearRouteCacheDisabledHeaderMutation) {
filter_->onDestroy();

EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 0);
EXPECT_EQ(config_->stats().clear_route_cache_not_outbound_.value(), 0);
EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 2);
EXPECT_EQ(config_->stats().streams_started_.value(), 1);
EXPECT_EQ(config_->stats().stream_msgs_sent_.value(), 3);
Expand All @@ -2114,7 +2116,7 @@ TEST_F(HttpFilterTest, ClearRouteCacheDisabledHeaderMutation) {
}

// Using the default configuration, verify that the "clear_route_cache" flag does not preform
// route clearing callbacks on the filter when no header changes are present.
// route clearing callbacks for outbound traffic or inbound traffic when no header changes are present.
TEST_F(HttpFilterTest, ClearRouteCacheUnchanged) {
initialize(R"EOF(
grpc_service:
Expand All @@ -2124,6 +2126,7 @@ TEST_F(HttpFilterTest, ClearRouteCacheUnchanged) {
response_body_mode: "BUFFERED"
)EOF");

// Do not call ClearRouteCache() for inbound traffic without header mutation.
EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true));
processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) {
resp.mutable_response()->set_clear_route_cache(true);
Expand All @@ -2136,14 +2139,16 @@ TEST_F(HttpFilterTest, ClearRouteCacheUnchanged) {
Buffer::OwnedImpl buffered_response_data;
setUpEncodingBuffering(buffered_response_data);

// Do not call ClearRouteCache() for outbound traffic.
EXPECT_EQ(FilterDataStatus::StopIterationNoBuffer, filter_->encodeData(resp_data, true));
processResponseBody([](const HttpBody&, ProcessingResponse&, BodyResponse& resp) {
resp.mutable_response()->set_clear_route_cache(true);
});

filter_->onDestroy();

EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 2);
EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 1);
EXPECT_EQ(config_->stats().clear_route_cache_not_outbound_.value(), 1);
EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 0);
EXPECT_EQ(config_->stats().streams_started_.value(), 1);
EXPECT_EQ(config_->stats().stream_msgs_sent_.value(), 3);
Expand Down