From 707921d02052094cb4fe0691a0fff3c2dcb36be3 Mon Sep 17 00:00:00 2001 From: Paul Ogilby Date: Mon, 28 Oct 2024 15:16:37 -0400 Subject: [PATCH] [balsa] fix for 1xx response mixup Signed-off-by: Paul Ogilby Signed-off-by: Ryan Northey --- changelogs/current.yaml | 4 + source/common/http/http1/balsa_parser.cc | 5 +- source/common/http/http1/balsa_parser.h | 3 + source/common/runtime/runtime_features.cc | 1 + test/integration/protocol_integration_test.cc | 84 +++++++++++++++++++ 5 files changed, 96 insertions(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index feb4e2fd4398..981e449d60d3 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -14,6 +14,10 @@ bug_fixes: - area: happy_eyeballs change: | Validate that ``additional_address`` are IP addresses instead of crashing when sorting. +- area: balsa + change: | + Fix incorrect handling of non-101 1xx responses. This fix can be temporarily reverted by setting runtime guard + ``envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done`` to false. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/http/http1/balsa_parser.cc b/source/common/http/http1/balsa_parser.cc index 5ffffd1de890..70493fc0ec4d 100644 --- a/source/common/http/http1/balsa_parser.cc +++ b/source/common/http/http1/balsa_parser.cc @@ -360,7 +360,10 @@ void BalsaParser::HeaderDone() { void BalsaParser::ContinueHeaderDone() {} void BalsaParser::MessageDone() { - if (status_ == ParserStatus::Error) { + if (status_ == ParserStatus::Error || + // In the case of early 1xx, MessageDone() can be called twice in a row. + // The !first_byte_processed_ check is to make this function idempotent. + (wait_for_first_byte_before_msg_done_ && !first_byte_processed_)) { return; } status_ = convertResult(connection_->onMessageComplete()); diff --git a/source/common/http/http1/balsa_parser.h b/source/common/http/http1/balsa_parser.h index 61a6e327b556..321dc0a01bfb 100644 --- a/source/common/http/http1/balsa_parser.h +++ b/source/common/http/http1/balsa_parser.h @@ -85,6 +85,9 @@ class BalsaParser : public Parser, public quiche::BalsaVisitorInterface { // Latched value of `envoy.reloadable_features.http1_balsa_delay_reset`. const bool delay_reset_ = Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_balsa_delay_reset"); + // Latched value of `envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done`. + const bool wait_for_first_byte_before_msg_done_ = Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done"); }; } // namespace Http1 diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index babfb11c2a0a..b65b07ea86df 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -107,6 +107,7 @@ RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_lis RUNTIME_GUARD(envoy_reloadable_features_validate_connect); RUNTIME_GUARD(envoy_reloadable_features_validate_grpc_header_before_log_grpc_status); RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers); +RUNTIME_GUARD(envoy_reloadable_features_wait_for_first_byte_before_balsa_msg_done); RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled); RUNTIME_GUARD(envoy_reloadable_features_xdstp_path_avoid_colon_encoding); RUNTIME_GUARD(envoy_restart_features_allow_client_socket_creation_failure); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 510cd6924d9c..58154161a6e6 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1557,6 +1557,90 @@ TEST_P(ProtocolIntegrationTest, EnvoyProxying104) { testEnvoyProxying1xx(false, false, false, "104"); } +TEST_P(DownstreamProtocolIntegrationTest, EnvoyProxying102DelayBalsaReset) { + if (GetParam().http1_implementation != Http1ParserImpl::BalsaParser || + GetParam().upstream_protocol != Http::CodecType::HTTP1 || + GetParam().downstream_protocol != Http::CodecType::HTTP1) { + GTEST_SKIP() << "This test is only relevant for HTTP1 BalsaParser"; + } + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.set_proxy_100_continue(true); }); + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done", "false"); + config_helper_.addRuntimeOverride("envoy.reloadable_features.http1_balsa_delay_reset", "true"); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "HEAD"}, + {":path", "/dynamo/url"}, + {":scheme", "http"}, + {":authority", "sni.lyft.com"}, + {"expect", "100-contINUE"}}); + waitForNextUpstreamRequest(); + upstream_request_->encode1xxHeaders(Http::TestResponseHeaderMapImpl{{":status", "102"}}); + response->waitFor1xxHeaders(); + upstream_request_->encodeHeaders(default_response_headers_, true); + + EXPECT_FALSE(response->waitForEndStream()); + + cleanupUpstreamAndDownstream(); +} + +TEST_P(DownstreamProtocolIntegrationTest, EnvoyProxying102DelayBalsaResetWaitForFirstByte) { + if (GetParam().http1_implementation != Http1ParserImpl::BalsaParser || + GetParam().upstream_protocol != Http::CodecType::HTTP1) { + GTEST_SKIP() << "This test is only relevant for HTTP1 upstream with BalsaParser"; + } + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.set_proxy_100_continue(true); }); + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done", "true"); + config_helper_.addRuntimeOverride("envoy.reloadable_features.http1_balsa_delay_reset", "true"); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "HEAD"}, + {":path", "/dynamo/url"}, + {":scheme", "http"}, + {":authority", "sni.lyft.com"}, + {"expect", "100-contINUE"}}); + waitForNextUpstreamRequest(); + upstream_request_->encode1xxHeaders(Http::TestResponseHeaderMapImpl{{":status", "102"}}); + response->waitFor1xxHeaders(); + upstream_request_->encodeHeaders(default_response_headers_, true); + ASSERT_TRUE(response->waitForEndStream()); +} + +TEST_P(DownstreamProtocolIntegrationTest, EnvoyProxying102NoDelayBalsaReset) { + if (GetParam().http1_implementation != Http1ParserImpl::BalsaParser || + GetParam().upstream_protocol != Http::CodecType::HTTP1) { + GTEST_SKIP() << "This test is only relevant for HTTP1 upstream with BalsaParser"; + } + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.set_proxy_100_continue(true); }); + config_helper_.addRuntimeOverride("envoy.reloadable_features.http1_balsa_delay_reset", "false"); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "HEAD"}, + {":path", "/dynamo/url"}, + {":scheme", "http"}, + {":authority", "sni.lyft.com"}, + {"expect", "100-contINUE"}}); + + waitForNextUpstreamRequest(); + upstream_request_->encode1xxHeaders(Http::TestResponseHeaderMapImpl{{":status", "102"}}); + response->waitFor1xxHeaders(); + upstream_request_->encodeHeaders(default_response_headers_, true); + ASSERT_TRUE(response->waitForEndStream()); +} + TEST_P(ProtocolIntegrationTest, TwoRequests) { testTwoRequests(); } TEST_P(ProtocolIntegrationTest, TwoRequestsWithForcedBackup) { testTwoRequests(true); }