http: allow runtime override of default for max response headers kb (e…

…nvoyproxy#36439) Also, update docs and tests for similar runtime overrides that already existed This is a followup to envoyproxy#36231 Risk Level: Low Testing: New tests, plus more tests for existing untested code Docs Changes: Updated proto docs, including adding docs for existing feature Release Notes: updated Signed-off-by: Greg Greenway <[email protected]> Signed-off-by: Steven Jin Xuan <[email protected]>
Stevenjin8 · Oct 10, 2024 · 08f2c92 · 08f2c92
1 parent 13bdb7b
commit 08f2c92
Show file tree

Hide file tree

Showing 7 changed files with 105 additions and 3 deletions.
diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto
@@ -262,6 +262,8 @@ message HttpProtocolOptions {
   // The maximum number of headers (request headers if configured on HttpConnectionManager,
   // response headers when configured on a cluster).
   // If unconfigured, the default maximum number of headers allowed is 100.
+  // The default value for requests can be overridden by setting runtime key ``envoy.reloadable_features.max_request_headers_count``.
+  // The default value for responses can be overridden by setting runtime key ``envoy.reloadable_features.max_response_headers_count``.
   // Downstream requests that exceed this limit will receive a 431 response for HTTP/1.x and cause a stream
   // reset for HTTP/2.
   // Upstream responses that exceed this limit will result in a 503 response.
@@ -270,6 +272,7 @@ message HttpProtocolOptions {
   // The maximum size of response headers.
   // If unconfigured, the default is 60 KiB, except for HTTP/1 response headers which have a default
   // of 80KiB.
+  // The default value can be overridden by setting runtime key ``envoy.reloadable_features.max_response_headers_size_kb``.
   // Responses that exceed this limit will result in a 503 response.
   // In Envoy, this setting is only valid when configured on an upstream cluster, not on the
   // :ref:`HTTP Connection Manager

diff --git a/...envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/...envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
@@ -493,6 +493,7 @@ message HttpConnectionManager {
 
   // The maximum request headers size for incoming connections.
   // If unconfigured, the default max request headers allowed is 60 KiB.
+  // The default value can be overridden by setting runtime key ``envoy.reloadable_features.max_request_headers_size_kb``.
   // Requests that exceed this limit will receive a 431 response.
   //
   // Note: currently some protocol codecs impose limits on the maximum size of a single header:

diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -306,7 +306,8 @@ new_features:
 - area: http
   change: |
     Added configuration setting for the :ref:`maximum size of response headers
-    <envoy_v3_api_field_config.core.v3.HttpProtocolOptions.max_response_headers_kb>` in responses.
+    <envoy_v3_api_field_config.core.v3.HttpProtocolOptions.max_response_headers_kb>` in responses. The default can
+    be overridden with runtime key ``envoy.reloadable_features.max_response_headers_size_kb``.
 - area: http_11_proxy
   change: |
     Added the option to configure the transport socket via locality or endpoint metadata.

diff --git a/envoy/http/codec.h b/envoy/http/codec.h
@@ -46,6 +46,8 @@ const char MaxResponseHeadersCountOverrideKey[] =
     "envoy.reloadable_features.max_response_headers_count";
 const char MaxRequestHeadersSizeOverrideKey[] =
     "envoy.reloadable_features.max_request_headers_size_kb";
+const char MaxResponseHeadersSizeOverrideKey[] =
+    "envoy.reloadable_features.max_response_headers_size_kb";
 
 class Stream;
 class RequestDecoder;

diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc
@@ -1229,8 +1229,17 @@ ClusterInfoImpl::ClusterInfoImpl(
           http_protocol_options_->common_http_protocol_options_, max_headers_count,
           runtime_.snapshot().getInteger(Http::MaxResponseHeadersCountOverrideKey,
                                          Http::DEFAULT_MAX_HEADERS_COUNT))),
-      max_response_headers_kb_(PROTOBUF_GET_OPTIONAL_WRAPPED(
-          http_protocol_options_->common_http_protocol_options_, max_response_headers_kb)),
+      max_response_headers_kb_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(
+          http_protocol_options_->common_http_protocol_options_, max_response_headers_kb,
+          [&]() -> absl::optional<uint16_t> {
+            constexpr uint64_t unspecified = 0;
+            uint64_t runtime_val = runtime_.snapshot().getInteger(
+                Http::MaxResponseHeadersSizeOverrideKey, unspecified);
+            if (runtime_val == unspecified) {
+              return absl::nullopt;
+            }
+            return runtime_val;
+          }())),
       type_(config.type()),
       drain_connections_on_host_removal_(config.ignore_health_on_host_removal()),
       connection_pool_per_downstream_connection_(

diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc
@@ -5594,6 +5594,69 @@ TEST_F(ClusterInfoImplTest, Http2AutoWithNonAlpnMatcherAndValidationOff) {
   EXPECT_NO_THROW(makeCluster(yaml + auto_http2));
 }
 
+TEST_F(ClusterInfoImplTest, MaxResponseHeadersDefault) {
+  const std::string yaml = TestEnvironment::substitute(R"EOF(
+    name: name
+    connect_timeout: 0.25s
+    type: STRICT_DNS
+  )EOF",
+                                                       Network::Address::IpVersion::v4);
+
+  auto cluster = makeCluster(yaml);
+  EXPECT_FALSE(cluster->info()->maxResponseHeadersKb().has_value());
+  EXPECT_EQ(100, cluster->info()->maxResponseHeadersCount());
+}
+
+// Test that the runtime override for the defaults is used when specified.
+TEST_F(ClusterInfoImplTest, MaxResponseHeadersRuntimeOverride) {
+  const std::string yaml = TestEnvironment::substitute(R"EOF(
+    name: name
+    connect_timeout: 0.25s
+    type: STRICT_DNS
+  )EOF",
+                                                       Network::Address::IpVersion::v4);
+
+  EXPECT_CALL(runtime_.snapshot_,
+              getInteger("envoy.reloadable_features.max_response_headers_size_kb", _))
+      .WillRepeatedly(Return(123));
+  EXPECT_CALL(runtime_.snapshot_,
+              getInteger("envoy.reloadable_features.max_response_headers_count", _))
+      .WillRepeatedly(Return(456));
+
+  auto cluster = makeCluster(yaml);
+  EXPECT_EQ(absl::make_optional(uint16_t(123)), cluster->info()->maxResponseHeadersKb());
+  EXPECT_EQ(456, cluster->info()->maxResponseHeadersCount());
+}
+
+// Test that the runtime override is ignored if there is a configured value.
+TEST_F(ClusterInfoImplTest, MaxResponseHeadersRuntimeOverrideIgnored) {
+  const std::string yaml = TestEnvironment::substitute(R"EOF(
+    name: name
+    connect_timeout: 0.25s
+    type: STRICT_DNS
+    typed_extension_protocol_options:
+      envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
+        "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
+        common_http_protocol_options:
+          max_response_headers_kb: 1
+          max_headers_count: 2
+        explicit_http_config:
+          http2_protocol_options: {}
+  )EOF",
+                                                       Network::Address::IpVersion::v4);
+
+  EXPECT_CALL(runtime_.snapshot_,
+              getDouble("envoy.reloadable_features.max_response_headers_size_kb", _))
+      .WillRepeatedly(Return(123));
+  EXPECT_CALL(runtime_.snapshot_,
+              getDouble("envoy.reloadable_features.max_response_headers_count", _))
+      .WillRepeatedly(Return(456));
+
+  auto cluster = makeCluster(yaml);
+  EXPECT_EQ(absl::make_optional(uint16_t(1)), cluster->info()->maxResponseHeadersKb());
+  EXPECT_EQ(2, cluster->info()->maxResponseHeadersCount());
+}
+
 TEST_F(ClusterInfoImplTest, UpstreamFilterTypedAndDynamicConfigThrows) {
   const std::string yaml = R"EOF(
     name: name

diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc
@@ -869,6 +869,29 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbMaxConfiguredViaRunti
   EXPECT_EQ(9000, config.maxRequestHeadersKb());
 }
 
+TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersCountMaxConfiguredViaRuntime) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  route_config:
+    name: local_route
+  http_filters:
+  - name: envoy.filters.http.router
+    typed_config:
+      "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+  )EOF";
+
+  ON_CALL(context_.server_factory_context_.runtime_loader_.snapshot_,
+          getInteger("envoy.reloadable_features.max_request_headers_count", _))
+      .WillByDefault(Return(42));
+
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     &scoped_routes_config_provider_manager_, tracer_manager_,
+                                     filter_config_provider_manager_, creation_status_);
+  ASSERT_TRUE(creation_status_.ok());
+  EXPECT_EQ(42, config.maxRequestHeadersCount());
+}
+
 // Validated that an explicit zero stream idle timeout disables.
 TEST_F(HttpConnectionManagerConfigTest, DisabledStreamIdleTimeout) {
   const std::string yaml_string = R"EOF(