Skip to content

router: remove upstream failure details from downstream body #44746

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

Merged
ggreenway merged 4 commits into from
May 1, 2026
Merged

router: remove upstream failure details from downstream body #44746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13ca4f3
router: remove upstream failure details from downstream body
ggreenway Apr 29, 2026
713fa6b
Merge branch 'main' into nobody
ggreenway Apr 29, 2026
25198c8
fix tests
ggreenway Apr 30, 2026
b1f0d1f
Merge remote-tracking branch 'upstream/main' into nobody
ggreenway Apr 30, 2026
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
8 changes: 8 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ behavior_changes:

minor_behavior_changes:
# *Changes that may cause incompatibilities for some users, but should not for most*
- area: router
change: |
The upstream transport failure reason (e.g. TLS certificate validation errors) is no longer included
in the HTTP response body sent to downstream clients. It remains available in access logs via
``%UPSTREAM_TRANSPORT_FAILURE_REASON%``. This behavioral change can be temporarily reverted by setting
runtime guard ``envoy.reloadable_features.hide_transport_failure_reason_in_response_body`` to ``false``.
This is being changed because in many cases the upstream failure details are inappropriate to send to
the downstream client as it discloses too many internal details.
- area: golang
change: |
Reduced the per-cgo-call mutex acquisition on the Golang HTTP filter by making the
Expand Down
8 changes: 6 additions & 2 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1622,12 +1622,16 @@ void Filter::onUpstreamReset(Http::StreamResetReason reset_reason,

const StreamInfo::CoreResponseFlag response_flags = streamResetReasonToResponseFlag(reset_reason);

const bool show_transport_failure =
!transport_failure_reason.empty() &&
!Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.hide_transport_failure_reason_in_response_body");
const std::string body =
absl::StrCat("upstream connect error or disconnect/reset before headers. ",
(is_retry_ ? "retried and the latest " : ""),
"reset reason: ", Http::Utility::resetReasonToString(reset_reason),
!transport_failure_reason.empty() ? ", transport failure reason: " : "",
transport_failure_reason);
show_transport_failure ? ", transport failure reason: " : "",
show_transport_failure ? transport_failure_reason : "");
const std::string& basic_details =
downstream_response_started_ ? StreamInfo::ResponseCodeDetails::get().LateUpstreamReset
: StreamInfo::ResponseCodeDetails::get().EarlyUpstreamReset;
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ RUNTIME_GUARD(envoy_reloadable_features_grpc_side_stream_flow_control);
RUNTIME_GUARD(envoy_reloadable_features_happy_eyeballs_sort_non_ip_addresses);
RUNTIME_GUARD(envoy_reloadable_features_header_mutation_url_encode_query_params);
RUNTIME_GUARD(envoy_reloadable_features_health_check_after_cluster_warming);
RUNTIME_GUARD(envoy_reloadable_features_hide_transport_failure_reason_in_response_body);
RUNTIME_GUARD(envoy_reloadable_features_http1_close_connection_on_zombie_stream_complete);
RUNTIME_GUARD(envoy_reloadable_features_http2_discard_host_header);
RUNTIME_GUARD(envoy_reloadable_features_http_async_client_retry_respect_buffer_limits);
Expand Down
4 changes: 2 additions & 2 deletions test/common/router/router_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ TEST_F(RouterTest, PoolFailureWithPriority) {
}));

Http::TestResponseHeaderMapImpl response_headers{
{":status", "503"}, {"content-length", "146"}, {"content-type", "text/plain"}};
{":status", "503"}, {"content-length", "98"}, {"content-type", "text/plain"}};
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false));
Comment on lines +418 to 419
Copy link
Copy Markdown
Contributor

@ravenblackx ravenblackx May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI for future things, you can implement this sort of thing in pure matchers something like

EXPECT_CALL(callbacks_, encodeHeaders_(AllOf(ContainsHeader(":status", "503"), ContainsHeader("content-type", "text/plain")), false));

And rather than validating content-length which is fragile and unreadable, could validate

EXPECT_CALL(callbacks_, encodeData(BufferStringContains("some substr of the body"), true));

(Which is also a bad matcher but I've already digressed.)

Copy link
Copy Markdown
Member Author

@ggreenway ggreenway May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know! I agree these are really fragile right now.

EXPECT_CALL(callbacks_, encodeData(_, true));
EXPECT_CALL(callbacks_.stream_info_,
Expand Down Expand Up @@ -448,7 +448,7 @@ TEST_F(RouterTest, PoolFailureDueToConnectTimeout) {
}));

Http::TestResponseHeaderMapImpl response_headers{
{":status", "503"}, {"content-length", "134"}, {"content-type", "text/plain"}};
{":status", "503"}, {"content-length", "91"}, {"content-type", "text/plain"}};
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false));
EXPECT_CALL(callbacks_, encodeData(_, true));
EXPECT_CALL(callbacks_.stream_info_,
Expand Down
96 changes: 44 additions & 52 deletions test/integration/local_reply_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,11 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJson) {
initialize();

std::string expected_body;
if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
expected_body = R"({
"level": "TRACE",
"user_agent": null,
"response_body": "upstream connect error or disconnect/reset before headers. reset reason: connection termination, transport failure reason: QUIC_NO_ERROR|FROM_PEER|Closed by application"
})";
} else {
expected_body = R"({
expected_body = R"({
"level": "TRACE",
"user_agent": null,
"response_body": "upstream connect error or disconnect/reset before headers. reset reason: connection termination"
})";
}

codec_client_ = makeHttpConnection(lookupPort("http"));

Expand Down Expand Up @@ -92,11 +84,7 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJson) {

EXPECT_TRUE(response->complete());
EXPECT_EQ("application/json-custom", response->headers().ContentType()->value().getStringView());
if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
EXPECT_EQ("223", response->headers().ContentLength()->value().getStringView());
} else {
EXPECT_EQ("150", response->headers().ContentLength()->value().getStringView());
}
EXPECT_EQ("150", response->headers().ContentLength()->value().getStringView());
EXPECT_EQ("550", response->headers().Status()->value().getStringView());
if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
EXPECT_EQ(response->headers().getProxyStatusValue(),
Expand Down Expand Up @@ -186,17 +174,10 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJson4Grpc) {

std::string expected_grpc_message;

if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
expected_grpc_message = R"({
"code": 503,
"message":"upstream connect error or disconnect/reset before headers. reset reason: connection termination, transport failure reason: QUIC_NO_ERROR|FROM_PEER|Closed by application"
})";
} else {
expected_grpc_message = R"({
expected_grpc_message = R"({
"code": 503,
"message":"upstream connect error or disconnect/reset before headers. reset reason: connection termination"
})";
}

codec_client_ = makeHttpConnection(lookupPort("http"));

Expand Down Expand Up @@ -247,17 +228,9 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormat2Text4Grpc) {
initialize();

// Note: there should be an %0A at the end.
std::string expected_grpc_message;
if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
expected_grpc_message =
"upstream connect error or disconnect/reset before headers. reset reason:"
" connection termination, transport failure reason: "
"QUIC_NO_ERROR|FROM_PEER|Closed by application:503:path=/package.service/method%0A";
} else {
expected_grpc_message =
"upstream connect error or disconnect/reset before headers. reset reason:"
" connection termination:503:path=/package.service/method%0A";
}
std::string expected_grpc_message =
"upstream connect error or disconnect/reset before headers. reset reason:"
" connection termination:503:path=/package.service/method%0A";

codec_client_ = makeHttpConnection(lookupPort("http"));

Expand Down Expand Up @@ -411,20 +384,11 @@ TEST_P(LocalReplyIntegrationTest, ShouldNotMatchAnyFilter) {
setLocalReplyConfig(yaml);
initialize();

std::string expected_body;
if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
expected_body = R"({
"level": "TRACE",
"response_flags": "UC",
"response_body": "upstream connect error or disconnect/reset before headers. reset reason: connection termination, transport failure reason: QUIC_NO_ERROR|FROM_PEER|Closed by application"
})";
} else {
expected_body = R"({
std::string expected_body = R"({
"level": "TRACE",
"response_flags": "UC",
"response_body": "upstream connect error or disconnect/reset before headers. reset reason: connection termination"
})";
}

codec_client_ = makeHttpConnection(lookupPort("http"));

Expand Down Expand Up @@ -455,11 +419,7 @@ TEST_P(LocalReplyIntegrationTest, ShouldNotMatchAnyFilter) {

EXPECT_TRUE(response->complete());
EXPECT_EQ("application/json", response->headers().ContentType()->value().getStringView());
if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
EXPECT_EQ("227", response->headers().ContentLength()->value().getStringView());
} else {
EXPECT_EQ("154", response->headers().ContentLength()->value().getStringView());
}
EXPECT_EQ("154", response->headers().ContentLength()->value().getStringView());
EXPECT_EQ("503", response->headers().Status()->value().getStringView());
// Check if returned json is same as expected
EXPECT_TRUE(TestUtility::jsonStringEqual(response->body(), expected_body));
Expand Down Expand Up @@ -523,13 +483,45 @@ TEST_P(LocalReplyIntegrationTest, ShouldMapResponseCodeAndMapToDefaultTextRespon

EXPECT_TRUE(response->complete());
EXPECT_EQ("text/plain", response->headers().ContentType()->value().getStringView());
if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
EXPECT_EQ("168", response->headers().ContentLength()->value().getStringView());
EXPECT_EQ("95", response->headers().ContentLength()->value().getStringView());

EXPECT_EQ("551", response->headers().Status()->value().getStringView());

EXPECT_EQ(response->body(), "upstream connect error or disconnect/reset before headers. reset "
"reason: connection termination");
}

// When the runtime guard is disabled, transport failure reason should appear in the response body.
TEST_P(LocalReplyIntegrationTest, TransportFailureReasonInBodyWhenRuntimeGuardDisabled) {
config_helper_.addRuntimeOverride(
"envoy.reloadable_features.hide_transport_failure_reason_in_response_body", "false");
initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));

auto encoder_decoder =
codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"},
{":path", "/test/long/url"},
{":scheme", "http"},
{":authority", "sni.lyft.com"}});
auto response = std::move(encoder_decoder.second);

ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));

ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
ASSERT_TRUE(fake_upstream_connection_->close());
ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
ASSERT_TRUE(response->waitForEndStream());

if (downstream_protocol_ == Http::CodecType::HTTP1) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
EXPECT_EQ("95", response->headers().ContentLength()->value().getStringView());
codec_client_->close();
}

EXPECT_EQ("551", response->headers().Status()->value().getStringView());
EXPECT_TRUE(response->complete());
EXPECT_EQ("503", response->headers().Status()->value().getStringView());

if (GetParam().upstream_protocol == Http::CodecType::HTTP3) {
EXPECT_EQ(response->body(), "upstream connect error or disconnect/reset before headers. reset "
Expand Down
Loading