Skip to content

Commit da56f6d

Browse files
paul-r-gallphlax
paul-r-gall
authored and
phlax
committed
[balsa] fix for 1xx response mixup
Signed-off-by: Paul Ogilby <[email protected]> Signed-off-by: Ryan Northey <[email protected]>
1 parent c312e48 commit da56f6d

File tree

5 files changed

+96
-1
lines changed

5 files changed

+96
-1
lines changed

‎changelogs/current.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,10 @@ bug_fixes:
169169
- area: happy_eyeballs
170170
change: |
171171
Validate that ``additional_address`` are IP addresses instead of crashing when sorting.
172+
- area: balsa
173+
change: |
174+
Fix incorrect handling of non-101 1xx responses. This fix can be temporarily reverted by setting runtime guard
175+
``envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done`` to false.
172176
173177
removed_config_or_runtime:
174178
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

‎source/common/http/http1/balsa_parser.cc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,10 @@ void BalsaParser::HeaderDone() {
360360
void BalsaParser::ContinueHeaderDone() {}
361361

362362
void BalsaParser::MessageDone() {
363-
if (status_ == ParserStatus::Error) {
363+
if (status_ == ParserStatus::Error ||
364+
// In the case of early 1xx, MessageDone() can be called twice in a row.
365+
// The !first_byte_processed_ check is to make this function idempotent.
366+
(wait_for_first_byte_before_msg_done_ && !first_byte_processed_)) {
364367
return;
365368
}
366369
status_ = convertResult(connection_->onMessageComplete());

‎source/common/http/http1/balsa_parser.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,9 @@ class BalsaParser : public Parser, public quiche::BalsaVisitorInterface {
8585
// Latched value of `envoy.reloadable_features.http1_balsa_delay_reset`.
8686
const bool delay_reset_ =
8787
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_balsa_delay_reset");
88+
// Latched value of `envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done`.
89+
const bool wait_for_first_byte_before_msg_done_ = Runtime::runtimeFeatureEnabled(
90+
"envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done");
8891
};
8992

9093
} // namespace Http1

‎source/common/runtime/runtime_features.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,7 @@ RUNTIME_GUARD(envoy_reloadable_features_use_route_host_mutation_for_auto_sni_san
102102
RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_listener);
103103
RUNTIME_GUARD(envoy_reloadable_features_validate_connect);
104104
RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers);
105+
RUNTIME_GUARD(envoy_reloadable_features_wait_for_first_byte_before_balsa_msg_done);
105106
RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled);
106107
RUNTIME_GUARD(envoy_reloadable_features_xds_prevent_resource_copy);
107108
RUNTIME_GUARD(envoy_reloadable_features_xdstp_path_avoid_colon_encoding);

‎test/integration/protocol_integration_test.cc

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1551,6 +1551,90 @@ TEST_P(ProtocolIntegrationTest, EnvoyProxying104) {
15511551
testEnvoyProxying1xx(false, false, false, "104");
15521552
}
15531553

1554+
TEST_P(DownstreamProtocolIntegrationTest, EnvoyProxying102DelayBalsaReset) {
1555+
if (GetParam().http1_implementation != Http1ParserImpl::BalsaParser ||
1556+
GetParam().upstream_protocol != Http::CodecType::HTTP1 ||
1557+
GetParam().downstream_protocol != Http::CodecType::HTTP1) {
1558+
GTEST_SKIP() << "This test is only relevant for HTTP1 BalsaParser";
1559+
}
1560+
config_helper_.addConfigModifier(
1561+
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
1562+
hcm) -> void { hcm.set_proxy_100_continue(true); });
1563+
config_helper_.addRuntimeOverride(
1564+
"envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done", "false");
1565+
config_helper_.addRuntimeOverride("envoy.reloadable_features.http1_balsa_delay_reset", "true");
1566+
initialize();
1567+
1568+
codec_client_ = makeHttpConnection(lookupPort("http"));
1569+
auto response = codec_client_->makeHeaderOnlyRequest(
1570+
Http::TestRequestHeaderMapImpl{{":method", "HEAD"},
1571+
{":path", "/dynamo/url"},
1572+
{":scheme", "http"},
1573+
{":authority", "sni.lyft.com"},
1574+
{"expect", "100-contINUE"}});
1575+
waitForNextUpstreamRequest();
1576+
upstream_request_->encode1xxHeaders(Http::TestResponseHeaderMapImpl{{":status", "102"}});
1577+
response->waitFor1xxHeaders();
1578+
upstream_request_->encodeHeaders(default_response_headers_, true);
1579+
1580+
EXPECT_FALSE(response->waitForEndStream());
1581+
1582+
cleanupUpstreamAndDownstream();
1583+
}
1584+
1585+
TEST_P(DownstreamProtocolIntegrationTest, EnvoyProxying102DelayBalsaResetWaitForFirstByte) {
1586+
if (GetParam().http1_implementation != Http1ParserImpl::BalsaParser ||
1587+
GetParam().upstream_protocol != Http::CodecType::HTTP1) {
1588+
GTEST_SKIP() << "This test is only relevant for HTTP1 upstream with BalsaParser";
1589+
}
1590+
config_helper_.addConfigModifier(
1591+
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
1592+
hcm) -> void { hcm.set_proxy_100_continue(true); });
1593+
config_helper_.addRuntimeOverride(
1594+
"envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done", "true");
1595+
config_helper_.addRuntimeOverride("envoy.reloadable_features.http1_balsa_delay_reset", "true");
1596+
initialize();
1597+
1598+
codec_client_ = makeHttpConnection(lookupPort("http"));
1599+
auto response = codec_client_->makeHeaderOnlyRequest(
1600+
Http::TestRequestHeaderMapImpl{{":method", "HEAD"},
1601+
{":path", "/dynamo/url"},
1602+
{":scheme", "http"},
1603+
{":authority", "sni.lyft.com"},
1604+
{"expect", "100-contINUE"}});
1605+
waitForNextUpstreamRequest();
1606+
upstream_request_->encode1xxHeaders(Http::TestResponseHeaderMapImpl{{":status", "102"}});
1607+
response->waitFor1xxHeaders();
1608+
upstream_request_->encodeHeaders(default_response_headers_, true);
1609+
ASSERT_TRUE(response->waitForEndStream());
1610+
}
1611+
1612+
TEST_P(DownstreamProtocolIntegrationTest, EnvoyProxying102NoDelayBalsaReset) {
1613+
if (GetParam().http1_implementation != Http1ParserImpl::BalsaParser ||
1614+
GetParam().upstream_protocol != Http::CodecType::HTTP1) {
1615+
GTEST_SKIP() << "This test is only relevant for HTTP1 upstream with BalsaParser";
1616+
}
1617+
config_helper_.addConfigModifier(
1618+
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
1619+
hcm) -> void { hcm.set_proxy_100_continue(true); });
1620+
config_helper_.addRuntimeOverride("envoy.reloadable_features.http1_balsa_delay_reset", "false");
1621+
initialize();
1622+
1623+
codec_client_ = makeHttpConnection(lookupPort("http"));
1624+
auto response = codec_client_->makeHeaderOnlyRequest(
1625+
Http::TestRequestHeaderMapImpl{{":method", "HEAD"},
1626+
{":path", "/dynamo/url"},
1627+
{":scheme", "http"},
1628+
{":authority", "sni.lyft.com"},
1629+
{"expect", "100-contINUE"}});
1630+
1631+
waitForNextUpstreamRequest();
1632+
upstream_request_->encode1xxHeaders(Http::TestResponseHeaderMapImpl{{":status", "102"}});
1633+
response->waitFor1xxHeaders();
1634+
upstream_request_->encodeHeaders(default_response_headers_, true);
1635+
ASSERT_TRUE(response->waitForEndStream());
1636+
}
1637+
15541638
TEST_P(ProtocolIntegrationTest, TwoRequests) { testTwoRequests(); }
15551639

15561640
TEST_P(ProtocolIntegrationTest, TwoRequestsWithForcedBackup) { testTwoRequests(true); }

0 commit comments

Comments
 (0)