Skip to content

Commit

Permalink
Populate stream reset errors to transport failure reason (#34974)
Browse files Browse the repository at this point in the history
Commit Message: Populate stream reset errors to transport failure reason
Additional Description:
Risk Level: Low
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

---------

Signed-off-by: Renjie Tang <renjietang@google.com>
  • Loading branch information
RenjieTang committed Jul 12, 2024
1 parent 5b33739 commit 5b70fd8
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 18 deletions.
5 changes: 5 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,11 @@ new_features:
Added support for wildcard resolution in :ref:`inline_dns_table
<envoy_v3_api_field_extensions.filters.udp.dns_filter.v3.DnsFilterConfig.ServerContextConfig.inline_dns_table>`
when DNS filter is working in server mode.
- area: quic
change: |
QUIC stream reset error code will be added to transport failure reason.
This behavior can be reverted by setting the runtime flag ``envoy.reloadable_features.report_stream_reset_error_code``
to false.
deprecated:
- area: tracing
Expand Down
21 changes: 16 additions & 5 deletions source/common/quic/envoy_quic_client_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "source/common/http/utility.h"
#include "source/common/quic/envoy_quic_client_session.h"
#include "source/common/quic/envoy_quic_utils.h"
#include "source/common/runtime/runtime_features.h"

#include "quiche/quic/core/http/quic_header_list.h"
#include "quiche/quic/core/quic_session.h"
Expand Down Expand Up @@ -255,8 +256,11 @@ bool EnvoyQuicClientStream::OnStopSending(quic::QuicResetStreamError error) {
if (read_side_closed() && !end_stream_encoded) {
// If both directions are closed but end stream hasn't been encoded yet, notify reset callbacks.
// Treat this as a remote reset, since the stream will be closed in both directions.
runResetCallbacks(quicRstErrorToEnvoyRemoteResetReason(error.internal_code()),
absl::string_view());
runResetCallbacks(
quicRstErrorToEnvoyRemoteResetReason(error.internal_code()),
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.report_stream_reset_error_code")
? quic::QuicRstStreamErrorCodeToString(error.internal_code())
: absl::string_view());
}
return true;
}
Expand Down Expand Up @@ -354,16 +358,23 @@ void EnvoyQuicClientStream::OnStreamReset(const quic::QuicRstStreamFrame& frame)
quic::QuicSpdyClientStream::OnStreamReset(frame);
ASSERT(read_side_closed());
if (write_side_closed() && !end_stream_decoded_and_encoded) {
runResetCallbacks(quicRstErrorToEnvoyRemoteResetReason(frame.error_code), absl::string_view());
runResetCallbacks(
quicRstErrorToEnvoyRemoteResetReason(frame.error_code),
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.report_stream_reset_error_code")
? quic::QuicRstStreamErrorCodeToString(frame.error_code)
: absl::string_view());
}
}

void EnvoyQuicClientStream::ResetWithError(quic::QuicResetStreamError error) {
ENVOY_STREAM_LOG(debug, "sending reset code={}", *this, error.internal_code());
stats_.tx_reset_.inc();
// Upper layers expect calling resetStream() to immediately raise reset callbacks.
runResetCallbacks(quicRstErrorToEnvoyLocalResetReason(error.internal_code()),
absl::string_view());
runResetCallbacks(
quicRstErrorToEnvoyLocalResetReason(error.internal_code()),
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.report_stream_reset_error_code")
? quic::QuicRstStreamErrorCodeToString(error.internal_code())
: absl::string_view());
if (session()->connection()->connected()) {
quic::QuicSpdyClientStream::ResetWithError(error);
}
Expand Down
21 changes: 16 additions & 5 deletions source/common/quic/envoy_quic_server_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "source/common/quic/envoy_quic_server_session.h"
#include "source/common/quic/envoy_quic_utils.h"
#include "source/common/quic/quic_stats_gatherer.h"
#include "source/common/runtime/runtime_features.h"

#include "quiche/quic/core/http/quic_header_list.h"
#include "quiche/quic/core/quic_session.h"
Expand Down Expand Up @@ -328,8 +329,11 @@ bool EnvoyQuicServerStream::OnStopSending(quic::QuicResetStreamError error) {
if (!end_stream_encoded) {
// If both directions are closed but end stream hasn't been encoded yet, notify reset callbacks.
// Treat this as a remote reset, since the stream will be closed in both directions.
runResetCallbacks(quicRstErrorToEnvoyRemoteResetReason(error.internal_code()),
absl::string_view());
runResetCallbacks(
quicRstErrorToEnvoyRemoteResetReason(error.internal_code()),
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.report_stream_reset_error_code")
? quic::QuicRstStreamErrorCodeToString(error.internal_code())
: absl::string_view());
}
return true;
}
Expand All @@ -344,7 +348,11 @@ void EnvoyQuicServerStream::OnStreamReset(const quic::QuicRstStreamFrame& frame)
if (write_side_closed() && !end_stream_decoded_and_encoded) {
// If both directions are closed but upstream hasn't received or sent end stream, run reset
// stream callback.
runResetCallbacks(quicRstErrorToEnvoyRemoteResetReason(frame.error_code), absl::string_view());
runResetCallbacks(
quicRstErrorToEnvoyRemoteResetReason(frame.error_code),
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.report_stream_reset_error_code")
? quic::QuicRstStreamErrorCodeToString(frame.error_code)
: absl::string_view());
}
}

Expand All @@ -353,8 +361,11 @@ void EnvoyQuicServerStream::ResetWithError(quic::QuicResetStreamError error) {
stats_.tx_reset_.inc();
if (!local_end_stream_) {
// Upper layers expect calling resetStream() to immediately raise reset callbacks.
runResetCallbacks(quicRstErrorToEnvoyLocalResetReason(error.internal_code()),
absl::string_view());
runResetCallbacks(
quicRstErrorToEnvoyLocalResetReason(error.internal_code()),
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.report_stream_reset_error_code")
? quic::QuicRstStreamErrorCodeToString(error.internal_code())
: absl::string_view());
}
quic::QuicSpdyServerStreamBase::ResetWithError(error);
}
Expand Down
15 changes: 11 additions & 4 deletions source/common/router/upstream_codec_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include "source/common/common/logger.h"
#include "source/common/config/well_known_names.h"
#include "source/common/runtime/runtime_features.h"
#include "source/extensions/filters/http/common/factory_base.h"

namespace Envoy {
Expand Down Expand Up @@ -64,12 +65,18 @@ class UpstreamCodecFilter : public Http::StreamDecoderFilter,
filter_.deferred_reset_ = true;
return;
}
std::string failure_reason(transport_failure_reason);
if (reason == Http::StreamResetReason::LocalReset) {
ASSERT(transport_failure_reason.empty());
// Use this to communicate to the upstream request to not force-terminate.
transport_failure_reason = "codec_error";
if (!Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.report_stream_reset_error_code")) {
ASSERT(transport_failure_reason.empty());
// Use this to communicate to the upstream request to not force-terminate.
failure_reason = "codec_error";
} else {
failure_reason = absl::StrCat(transport_failure_reason, "|codec_error");
}
}
filter_.callbacks_->resetStream(reason, transport_failure_reason);
filter_.callbacks_->resetStream(reason, failure_reason);
}
void onAboveWriteBufferHighWatermark() override {
filter_.callbacks_->onDecoderFilterAboveWriteBufferHighWatermark();
Expand Down
9 changes: 7 additions & 2 deletions source/common/router/upstream_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -813,8 +813,13 @@ void UpstreamRequestFilterManagerCallbacks::resetStream(
// which should force reset the stream, and a codec driven reset, which should
// tell the router the stream reset, and let the router make the decision to
// send a local reply, or retry the stream.
if (reset_reason == Http::StreamResetReason::LocalReset &&
transport_failure_reason != "codec_error") {
bool is_codec_error;
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.report_stream_reset_error_code")) {
is_codec_error = absl::StrContains(transport_failure_reason, "codec_error");
} else {
is_codec_error = transport_failure_reason == "codec_error";
}
if (reset_reason == Http::StreamResetReason::LocalReset && !is_codec_error) {
upstream_request_.parent_.callbacks()->resetStream();
return;
}
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ RUNTIME_GUARD(envoy_reloadable_features_quic_send_server_preferred_address_to_al
RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_reads_fixed_number_packets);
RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_socket_use_address_cache_for_read);
RUNTIME_GUARD(envoy_reloadable_features_reject_invalid_yaml);
RUNTIME_GUARD(envoy_reloadable_features_report_stream_reset_error_code);
RUNTIME_GUARD(envoy_reloadable_features_sanitize_http2_headers_without_nghttp2);
RUNTIME_GUARD(envoy_reloadable_features_sanitize_te);
RUNTIME_GUARD(envoy_reloadable_features_send_header_raw_value);
Expand Down
8 changes: 6 additions & 2 deletions test/integration/protocol_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4750,8 +4750,12 @@ TEST_P(ProtocolIntegrationTest, InvalidResponseHeaderNameStreamError) {
EXPECT_EQ("502", response->headers().getStatusValue());
test_server_->waitForCounterGe("http.config_test.downstream_rq_5xx", 1);

EXPECT_EQ(waitForAccessLog(access_log_name_),
"upstream_reset_before_response_started{protocol_error}");
std::string error_message =
upstreamProtocol() == Http::CodecType::HTTP3
? "upstream_reset_before_response_started{protocol_error|QUIC_BAD_APPLICATION_PAYLOAD}"
: "upstream_reset_before_response_started{protocol_error}";

EXPECT_EQ(waitForAccessLog(access_log_name_), error_message);
// Upstream connection should stay up
ASSERT_TRUE(fake_upstream_connection_->connected());
}
Expand Down

0 comments on commit 5b70fd8

Please sign in to comment.