From ac42a62d6b8a2599c9a6101b6bb0bd11220c63e6 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Fri, 26 Jan 2024 19:08:27 -0500 Subject: [PATCH] Runtime flag remove: Deprecated legacy code paths for allow-codec-error-after-1xx. (#32072) Deprecated legacy code paths for allow-codec-error-after-1xx. Signed-off-by: Kevin Baichoo --- changelogs/current.yaml | 3 + source/common/http/http1/codec_impl.cc | 7 +-- source/common/runtime/runtime_features.cc | 1 - test/integration/overload_integration_test.cc | 63 +------------------ 4 files changed, 7 insertions(+), 67 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index ffa817656c1e..7314ee8850db 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -20,6 +20,9 @@ removed_config_or_runtime: - area: http change: | Removed ``envoy.reloadable_features.allow_absolute_url_with_mixed_scheme`` runtime flag and legacy code paths. +- area: http1 + change: | + Removed ``envoy.reloadable_features.http1_allow_codec_error_response_after_1xx_headers`` runtime flag and legacy code paths. - area: overload manager change: | removed ``envoy.reloadable_features.overload_manager_error_unknown_action`` and legacy code paths. diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index eceb5f81cb0d..dae3b4d3be06 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -130,11 +130,8 @@ void StreamEncoderImpl::encodeFormattedHeader(absl::string_view key, absl::strin void ResponseEncoderImpl::encode1xxHeaders(const ResponseHeaderMap& headers) { ASSERT(HeaderUtility::isSpecial1xx(headers)); encodeHeaders(headers, false); - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.http1_allow_codec_error_response_after_1xx_headers")) { - // Don't consider 100-continue responses as the actual response. - started_response_ = false; - } + // Don't consider 100-continue responses as the actual response. + started_response_ = false; } void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& headers, diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 6465f087e1ca..fcca372836d8 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -51,7 +51,6 @@ RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_change_http_st RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_handle_empty_response); RUNTIME_GUARD(envoy_reloadable_features_handle_uppercase_scheme); RUNTIME_GUARD(envoy_reloadable_features_hmac_base64_encoding_only); -RUNTIME_GUARD(envoy_reloadable_features_http1_allow_codec_error_response_after_1xx_headers); RUNTIME_GUARD(envoy_reloadable_features_http1_connection_close_header_in_redirect); // Ignore the automated "remove this flag" issue: we should keep this for 1 year. RUNTIME_GUARD(envoy_reloadable_features_http1_use_balsa_parser); diff --git a/test/integration/overload_integration_test.cc b/test/integration/overload_integration_test.cc index 1232968546dc..d9060673cd3f 100644 --- a/test/integration/overload_integration_test.cc +++ b/test/integration/overload_integration_test.cc @@ -585,16 +585,12 @@ TEST_P(LoadShedPointIntegrationTest, Http1ServerDispatchAbortShedsLoadWhenNewReq // Test using 100-continue to start the response before doing triggering Overload. // Even though Envoy has encoded the 1xx headers, 1xx headers are not the actual response // so Envoy should still send the local reply when shedding load. -TEST_P( - LoadShedPointIntegrationTest, - Http1ServerDispatchAbortShedsLoadWithLocalReplyWhen1xxResponseStartedWithFlagAllowCodecErrorResponseAfter1xxHeadersEnabled) { +TEST_P(LoadShedPointIntegrationTest, + Http1ServerDispatchAbortShedsLoadWithLocalReplyWhen1xxResponseStarted) { // Test only applies to HTTP1. if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) { return; } - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.http1_allow_codec_error_response_after_1xx_headers", "true"}}); initializeOverloadManager( TestUtility::parseYaml(R"EOF( @@ -635,61 +631,6 @@ TEST_P( test_server_->waitForCounterEq("http.config_test.downstream_rq_overload_close", 1); } -// TODO(kbaichoo): remove this test when the runtime flag is removed. -TEST_P( - LoadShedPointIntegrationTest, - Http1ServerDispatchAbortShedsLoadWithLocalReplyWhen1xxResponseStartedWithFlagAllowCodecErrorResponseAfter1xxHeadersDisabled) { - // Test only applies to HTTP1. - if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) { - return; - } - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.http1_allow_codec_error_response_after_1xx_headers", "false"}}); - - initializeOverloadManager( - TestUtility::parseYaml(R"EOF( - name: "envoy.load_shed_points.http1_server_abort_dispatch" - triggers: - - name: "envoy.resource_monitors.testonly.fake_resource_monitor" - threshold: - value: 0.90 - )EOF")); - test_server_->waitForCounterEq("http.config_test.downstream_rq_overload_close", 0); - - // Start the 100-continue request - codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); - auto encoder_decoder = - codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":path", "/dynamo/url"}, - {":scheme", "http"}, - {":authority", "sni.lyft.com"}, - {"expect", "100-contINUE"}}); - - // Wait for 100-continue - request_encoder_ = &encoder_decoder.first; - auto response = std::move(encoder_decoder.second); - ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); - // The continue headers should arrive immediately. - response->waitFor1xxHeaders(); - ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); - - // Put envoy in overloaded state and check that it rejects the continuing - // dispatch. - updateResource(0.95); - test_server_->waitForGaugeEq( - "overload.envoy.load_shed_points.http1_server_abort_dispatch.scale_percent", 100); - codec_client_->sendData(*request_encoder_, 10, true); - - // Since the runtime flag `http1_allow_codec_error_response_after_1xx_headers` - // is disabled the downstream client ends up just getting a closed connection. - // Envoy's downstream codec does not provide a local reply as we've started - // the response. - ASSERT_TRUE(codec_client_->waitForDisconnect()); - EXPECT_FALSE(response->complete()); - test_server_->waitForCounterEq("http.config_test.downstream_rq_overload_close", 1); -} - TEST_P(LoadShedPointIntegrationTest, Http1ServerDispatchShouldNotAbortEncodingUpstreamResponse) { // Test only applies to HTTP1. if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) {