Runtime flag remove: Deprecated legacy code paths for allow-codec-err…

…or-after-1xx. (#32072) Deprecated legacy code paths for allow-codec-error-after-1xx. Signed-off-by: Kevin Baichoo <envoy@kevinbaichoo.com>
envoyproxy · Jan 27, 2024 · ac42a62 · ac42a62
1 parent c9d11a0
commit ac42a62
Show file tree

Hide file tree

Showing 4 changed files with 7 additions and 67 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<envoy::config::overload::v3::LoadShedPoint>(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<envoy::config::overload::v3::LoadShedPoint>(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) {