From 76011c431266366f3987c2fe2eaf56a4b3697785 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 3 Oct 2024 14:02:23 -0700 Subject: [PATCH] http: allow runtime override of default for max response headers kb Also, update docs and tests for similar runtime overrides that already existed Signed-off-by: Greg Greenway --- api/envoy/config/core/v3/protocol.proto | 3 + .../v3/http_connection_manager.proto | 1 + changelogs/current.yaml | 3 +- envoy/http/codec.h | 2 + source/common/upstream/upstream_impl.cc | 13 +++- test/common/upstream/upstream_impl_test.cc | 63 +++++++++++++++++++ .../http_connection_manager/config_test.cc | 23 +++++++ 7 files changed, 105 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index e566278600ab..423ec11b8cd9 100644 --- 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/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 3d438ae87881..3b49f1329567 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/api/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 index b773fb76f626..200f299a1284 100644 --- 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 - ` in responses. + ` 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 index 7408d4085a30..0297a8a32356 100644 --- 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 index 195d2ba59acc..49f7bdaa932e 100644 --- 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 { + 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 index 38917ad1be96..fc78e4c7189d 100644 --- 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 index 9805d0f72c76..9c8aa06424e0 100644 --- 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(