envoyproxy · htuch · Oct 12, 2020 · Oct 8, 2020
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -20,6 +20,7 @@ Removed Config or Runtime
 *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
 
 * ext_authz: removed auto ignore case in HTTP-based `ext_authz` header matching and the runtime guard `envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher`. To ignore case, set the :ref:`ignore_case <envoy_api_field_type.matcher.StringMatcher.ignore_case>` field to true.
+* http: removed `envoy.reloadable_features.http1_flood_protection` and legacy code path for turning flood protection off.
 
 New Features
 ------------

diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -264,9 +264,6 @@ void StreamEncoderImpl::endEncode() {
 }
 
 void ServerConnectionImpl::maybeAddSentinelBufferFragment(Buffer::WatermarkBuffer& output_buffer) {
-  if (!flood_protection_) {
-    return;
-  }
   // It's messy and complicated to try to tag the final write of an HTTP response for response
   // tracking for flood protection. Instead, write an empty buffer fragment after the response,
   // to allow for tracking.
@@ -281,9 +278,6 @@ void ServerConnectionImpl::maybeAddSentinelBufferFragment(Buffer::WatermarkBuffe
 
 Status ServerConnectionImpl::doFloodProtectionChecks() const {
   ASSERT(dispatching_);
-  if (!flood_protection_) {
-    return okStatus();
-  }
   // Before processing another request, make sure that we are below the response flood protection
   // threshold.
   if (outbound_responses_ >= max_outbound_responses_) {
@@ -859,8 +853,6 @@ ServerConnectionImpl::ServerConnectionImpl(
       // maintainer team as it will otherwise be removed entirely soon.
       max_outbound_responses_(
           Runtime::getInteger("envoy.do_not_use_going_away_max_http2_outbound_responses", 2)),
-      flood_protection_(
-          Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_flood_protection")),
       headers_with_underscores_action_(headers_with_underscores_action) {}
 
 uint32_t ServerConnectionImpl::getHeadersSize() {

diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h
@@ -545,7 +545,6 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
   // of Envoy wish to enable pipelining (which is dangerous and ill supported)
   // we could make this configurable.
   uint32_t max_outbound_responses_{};
-  bool flood_protection_{};
   // TODO(mattklein123): This should be a member of ActiveRequest but this change needs dedicated
   // thought as some of the reset and no header code paths make this difficult. Headers are
   // populated on message begin. Trailers are populated on the first parsed trailer field (if

diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc
@@ -265,9 +265,6 @@ void StreamEncoderImpl::endEncode() {
 }
 
 void ServerConnectionImpl::maybeAddSentinelBufferFragment(Buffer::WatermarkBuffer& output_buffer) {
-  if (!flood_protection_) {
-    return;
-  }
   // It's messy and complicated to try to tag the final write of an HTTP response for response
   // tracking for flood protection. Instead, write an empty buffer fragment after the response,
   // to allow for tracking.
@@ -281,9 +278,6 @@ void ServerConnectionImpl::maybeAddSentinelBufferFragment(Buffer::WatermarkBuffe
 }
 
 void ServerConnectionImpl::doFloodProtectionChecks() const {
-  if (!flood_protection_) {
-    return;
-  }
   // Before processing another request, make sure that we are below the response flood protection
   // threshold.
   if (outbound_responses_ >= max_outbound_responses_) {
@@ -800,8 +794,6 @@ ServerConnectionImpl::ServerConnectionImpl(
       // maintainer team as it will otherwise be removed entirely soon.
       max_outbound_responses_(
           Runtime::getInteger("envoy.do_not_use_going_away_max_http2_outbound_responses", 2)),
-      flood_protection_(
-          Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_flood_protection")),
       headers_with_underscores_action_(headers_with_underscores_action) {}
 
 uint32_t ServerConnectionImpl::getHeadersSize() {

diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h
@@ -519,7 +519,6 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
   // of Envoy wish to enable pipelining (which is dangerous and ill supported)
   // we could make this configurable.
   uint32_t max_outbound_responses_{};
-  bool flood_protection_{};
   // TODO(mattklein123): This should be a member of ActiveRequest but this change needs dedicated
   // thought as some of the reset and no header code paths make this difficult. Headers are
   // populated on message begin. Trailers are populated on the first parsed trailer field (if

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -53,7 +53,6 @@ uint64_t getInteger(absl::string_view feature, uint64_t default_value) {
 // problem of the bugs being found after the old code path has been removed.
 constexpr const char* runtime_features[] = {
     // Enabled
-    "envoy.reloadable_features.http1_flood_protection",
     "envoy.reloadable_features.test_feature_true",
     // Begin alphabetically sorted section.
     "envoy.deprecated_features.allow_deprecated_extension_names",

diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc
@@ -1073,42 +1073,6 @@ TEST_P(Http1ServerConnectionImplTest, FloodProtection) {
   }
 }
 
-TEST_P(Http1ServerConnectionImplTest, FloodProtectionOff) {
-  TestScopedRuntime scoped_runtime;
-  Runtime::LoaderSingleton::getExisting()->mergeValues(
-      {{"envoy.reloadable_features.http1_flood_protection", "false"}});
-  initialize();
-
-  NiceMock<MockRequestDecoder> decoder;
-  Buffer::OwnedImpl local_buffer;
-  // With flood protection off, many responses can be queued up.
-  for (int i = 0; i < 4; ++i) {
-    Http::ResponseEncoder* response_encoder = nullptr;
-    EXPECT_CALL(callbacks_, newStream(_, _))
-        .WillOnce(Invoke([&](Http::ResponseEncoder& encoder, bool) -> Http::RequestDecoder& {
-          response_encoder = &encoder;
-          return decoder;
-        }));
-
-    Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\n");
-    auto status = codec_->dispatch(buffer);
-    EXPECT_TRUE(status.ok());
-    EXPECT_EQ(0U, buffer.length());
-
-    // In most tests the write output is serialized to a buffer here it is
-    // ignored to build up queued "end connection" sentinels.
-    EXPECT_CALL(connection_, write(_, _))
-
-        .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> void {
-          // Move the response out of data while preserving the buffer fragment sentinels.
-          local_buffer.move(data);
-        }));
-
-    TestResponseHeaderMapImpl headers{{":status", "200"}};
-    response_encoder->encodeHeaders(headers, true);
-  }
-}
-
 TEST_P(Http1ServerConnectionImplTest, HostHeaderTranslation) {
   initialize();