diff --git a/changelogs/current.yaml b/changelogs/current.yaml index b39e567bcdc32..3b40df030d58c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -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 diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 582943f204a3c..fe3290bccb89e 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -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; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 148f800457ab1..a97925ea9e82e 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -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); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 59f740d894d79..4dbd98eeb9e06 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -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)); EXPECT_CALL(callbacks_, encodeData(_, true)); EXPECT_CALL(callbacks_.stream_info_, @@ -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_, diff --git a/test/integration/local_reply_integration_test.cc b/test/integration/local_reply_integration_test.cc index 95b7234dab05d..df2af113a0417 100644 --- a/test/integration/local_reply_integration_test.cc +++ b/test/integration/local_reply_integration_test.cc @@ -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")); @@ -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(), @@ -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")); @@ -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")); @@ -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")); @@ -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)); @@ -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 "