Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Minor Behavior Changes
* http: added :ref:`headers_to_add <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.ResponseMapper.headers_to_add>` to :ref:`local reply mapper <config_http_conn_man_local_reply>` to allow its users to add/append/override response HTTP headers to local replies.
* http: added HCM level configuration of :ref:`error handling on invalid messaging <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>` which substantially changes Envoy's behavior when encountering invalid HTTP/1.1 defaulting to closing the connection instead of allowing reuse. This can temporarily be reverted by setting `envoy.reloadable_features.hcm_stream_error_on_invalid_message` to false, or permanently reverted by setting the HTTP/1 configuration :ref:`override_stream_error_on_invalid_http_message <envoy_v3_api_field_config.core.v3.Http1ProtocolOptions.override_stream_error_on_invalid_http_message>` to true to restore prior HTTP/1.1 behavior (i.e. connection isn't terminated) and to retain prior HTTP/2 behavior (i.e. connection is terminated).
* http: added HCM level configuration of :ref:`error handling on invalid messaging <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>` which substantially changes Envoy's behavior when encountering invalid HTTP/1.1 defaulting to closing the connection instead of allowing reuse. This can temporarily be reverted by setting `envoy.reloadable_features.hcm_stream_error_on_invalid_message` to false, or permanently reverted by setting the :ref:`HCM option <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>` to true to restore prior HTTP/1.1 beavior and setting the *new* HTTP/2 configuration :ref:`override_stream_error_on_invalid_http_message <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.override_stream_error_on_invalid_http_message>` to false to retain prior HTTP/2 behavior.
* http: applying route level header modifications to local replies sent on that route. This behavior may be temporarily reverted by setting `envoy.reloadable_features.always_apply_route_header_rules` to false.
* http: changed Envoy to send GOAWAY to HTTP2 downstreams when the :ref:`disable_keepalive <config_overload_manager_overload_actions>` overload action is active. This behavior may be temporarily reverted by setting `envoy.reloadable_features.overload_manager_disable_keepalive_drain_http2` to false.
* http: changed Envoy to send error headers and body when possible. This behavior may be temporarily reverted by setting `envoy.reloadable_features.allow_response_for_timeout` to false.
* http: changed empty trailers encoding behavior by sending empty data with ``end_stream`` true (instead of sending empty trailers) for HTTP/2. This behavior can be reverted temporarily by setting runtime feature ``envoy.reloadable_features.http2_skip_encoding_empty_trailers`` to false.
Expand Down
22 changes: 20 additions & 2 deletions source/common/http/filter_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,16 @@ void FilterManager::sendLocalReplyViaFilterChain(
Utility::sendLocalReply(
state_.destroyed_,
Utility::EncodeFunctions{
modify_headers,
[this, modify_headers](ResponseHeaderMap& headers) -> void {
if (streamInfo().route_entry_ &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to make a test for the case where route_entry_ is nullptr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every early response test (invalid HTTP/1.1 sending 400 before we have headers and can select route) tests this. Did you want a unit test as well, or just to make sure we had coverage?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to make sure we have coverage

Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.always_apply_route_header_rules")) {
streamInfo().route_entry_->finalizeResponseHeaders(headers, streamInfo());
}
if (modify_headers) {
modify_headers(headers);
}
},
[this](ResponseHeaderMap& response_headers, Code& code, std::string& body,
absl::string_view& content_type) -> void {
// TODO(snowp): This &get() business isn't nice, rework LocalReply and others to accept
Expand Down Expand Up @@ -846,7 +855,16 @@ void FilterManager::sendDirectLocalReply(
Http::Utility::sendLocalReply(
state_.destroyed_,
Utility::EncodeFunctions{
modify_headers,
[this, modify_headers](ResponseHeaderMap& headers) -> void {
if (streamInfo().route_entry_ &&
Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.always_apply_route_header_rules")) {
streamInfo().route_entry_->finalizeResponseHeaders(headers, streamInfo());
}
if (modify_headers) {
modify_headers(headers);
}
Comment on lines +859 to +866
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stanza is duplicated verbatim from above and is a little tricky so might be worth de-deduping but up to you.

},
[&](ResponseHeaderMap& response_headers, Code& code, std::string& body,
absl::string_view& content_type) -> void {
local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().has_value()
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ constexpr const char* runtime_features[] = {
"envoy.reloadable_features.test_feature_true",
// Begin alphabetically sorted section.
"envoy.deprecated_features.allow_deprecated_extension_names",
"envoy.reloadable_features.always_apply_route_header_rules",
"envoy.reloadable_features.activate_fds_next_event_loop",
"envoy.reloadable_features.activate_timers_next_event_loop",
"envoy.reloadable_features.allow_500_after_100",
Expand Down
7 changes: 7 additions & 0 deletions test/integration/idle_timeout_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest {
auto* route = virtual_host->mutable_routes(0)->mutable_route();
route->mutable_idle_timeout()->set_seconds(0);
route->mutable_idle_timeout()->set_nanos(IdleTimeoutMs * 1000 * 1000);

auto* header = virtual_host->mutable_response_headers_to_add()->Add()->mutable_header();
header->set_key("foo");
header->set_value("bar");
}
if (enable_request_timeout_) {
hcm.mutable_request_timeout()->set_seconds(0);
Expand Down Expand Up @@ -178,6 +182,9 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterDownstreamHeaders) {
EXPECT_EQ(0U, upstream_request_->bodyLength());
EXPECT_TRUE(response->complete());
EXPECT_EQ("408", response->headers().getStatusValue());
auto foo = Http::LowerCaseString("foo");
ASSERT_TRUE(response->headers().get(foo) != nullptr);
EXPECT_EQ("bar", response->headers().get(foo)->value().getStringView());
EXPECT_EQ("stream timeout", response->body());

EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("stream_idle_timeout"));
Expand Down
15 changes: 15 additions & 0 deletions test/integration/protocol_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,16 @@ TEST_P(DownstreamProtocolIntegrationTest, HittingDecoderFilterLimit) {
// Test hitting the encoder buffer filter with too many response bytes to buffer. Given the request
// headers are sent on early, the stream/connection will be reset.
TEST_P(ProtocolIntegrationTest, HittingEncoderFilterLimit) {
config_helper_.addConfigModifier(
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
hcm) -> void {
auto* route_config = hcm.mutable_route_config();
auto* virtual_host = route_config->mutable_virtual_hosts(0);
auto* header = virtual_host->mutable_response_headers_to_add()->Add()->mutable_header();
header->set_key("foo");
header->set_value("bar");
});

useAccessLog();
config_helper_.addFilter("{ name: encoder-decoder-buffer-filter, typed_config: { \"@type\": "
"type.googleapis.com/google.protobuf.Empty } }");
Expand Down Expand Up @@ -886,6 +896,11 @@ TEST_P(ProtocolIntegrationTest, HittingEncoderFilterLimit) {
response->waitForEndStream();
EXPECT_TRUE(response->complete());
EXPECT_EQ("500", response->headers().getStatusValue());
// Regression test all sendLocalReply paths add route-requested headers.
auto foo = Http::LowerCaseString("foo");
ASSERT_TRUE(response->headers().get(foo) != nullptr);
EXPECT_EQ("bar", response->headers().get(foo)->value().getStringView());

// Regression test https://github.com/envoyproxy/envoy/issues/9881 by making
// sure this path does standard HCM header transformations.
EXPECT_TRUE(response->headers().Date() != nullptr);
Expand Down