From aec500c7b0ff41dccc25e01d57e28e0c5eab19bf Mon Sep 17 00:00:00 2001 From: Taylor Barrella Date: Wed, 7 Apr 2021 21:14:14 +0000 Subject: [PATCH 1/2] http: split strict_1xx_and_204_response_headers into two settings Signed-off-by: Taylor Barrella --- docs/root/version_history/current.rst | 5 +++++ source/common/http/http1/codec_impl.cc | 18 +++++++++++------- source/common/http/http1/codec_impl.h | 5 +++-- source/common/runtime/runtime_features.cc | 3 ++- test/common/http/http1/codec_impl_test.cc | 8 ++++---- 5 files changed, 25 insertions(+), 14 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a3c7b27d34e0..ab9ccdee6050 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -53,6 +53,11 @@ Minor Behavior Changes `envoy.reloadable_features.remove_forked_chromium_url`. * http: increase the maximum allowed number of initial connection WINDOW_UPDATE frames sent by the peer from 1 to 5. * http: no longer adding content-length: 0 for requests which should not have bodies. This behavior can be temporarily reverted by setting `envoy.reloadable_features.dont_add_content_length_for_bodiless_requests` false. +* http: replaced setting `envoy.reloadable_features.strict_1xx_and_204_response_headers` with settings + `envoy.reloadable_features.accept_strict_1xx_and_204_response_headers` + (do not accept upstream 1xx or 204 responses with Transfer-Encoding or non-zero Content-Length headers) and + `envoy.reloadable_features.send_strict_1xx_and_204_response_headers` + (do not send 1xx or 204 responses with these headers). Both are true by default. * http: upstream flood and abuse checks increment the count of opened HTTP/2 streams when Envoy sends initial HEADERS frame for the new stream. Before the counter was incrementred when Envoy received response HEADERS frame with the END_HEADERS flag set from upstream server. diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 3b245d4f8712..6dff75a8ee5d 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -195,9 +195,10 @@ void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& head } else if (connection_.protocol() == Protocol::Http10) { chunk_encoding_ = false; } else if (status && (*status < 200 || *status == 204) && - connection_.strict1xxAnd204Headers()) { - // TODO(zuercher): when the "envoy.reloadable_features.strict_1xx_and_204_response_headers" - // feature flag is removed, this block can be coalesced with the 100 Continue logic above. + connection_.sendStrict1xxAnd204Headers()) { + // TODO(zuercher): when the + // "envoy.reloadable_features.send_strict_1xx_and_204_response_headers" feature flag is + // removed, this block can be coalesced with the 100 Continue logic above. // For 1xx and 204 responses, do not send the chunked encoding header or enable chunked // encoding: https://tools.ietf.org/html/rfc7230#section-3.3.1 @@ -458,8 +459,10 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat encode_only_header_key_formatter_(encodeOnlyFormatterFromSettings(settings)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), - strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.strict_1xx_and_204_response_headers")), + accept_strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.accept_strict_1xx_and_204_response_headers")), + send_strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.send_strict_1xx_and_204_response_headers")), dispatching_(false), output_buffer_(connection.dispatcher().getWatermarkFactory().create( [&]() -> void { this->onBelowLowWatermark(); }, [&]() -> void { this->onAboveHighWatermark(); }, @@ -853,7 +856,8 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { os << spaces << "Http1::ConnectionImpl " << this << DUMP_MEMBER(dispatching_) << DUMP_MEMBER(dispatching_slice_already_drained_) << DUMP_MEMBER(reset_stream_called_) << DUMP_MEMBER(handling_upgrade_) << DUMP_MEMBER(deferred_end_stream_headers_) - << DUMP_MEMBER(strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_) + << DUMP_MEMBER(accept_strict_1xx_and_204_headers_) + << DUMP_MEMBER(send_strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_) << DUMP_MEMBER(buffered_body_.length()); // Dump header parsing state, and any progress on headers. @@ -1279,7 +1283,7 @@ Envoy::StatusOr ClientConnectionImpl::onHeadersCompleteBase() { handling_upgrade_ = true; } - if (strict_1xx_and_204_headers_ && + if (accept_strict_1xx_and_204_headers_ && (parser_->statusCode() < 200 || parser_->statusCode() == 204)) { if (headers->TransferEncoding()) { RETURN_IF_ERROR( diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index f4c5e7d46f8e..15aeb7eb605a 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -229,7 +229,7 @@ class ConnectionImpl : public virtual Connection, void onUnderlyingConnectionAboveWriteBufferHighWatermark() override { onAboveHighWatermark(); } void onUnderlyingConnectionBelowWriteBufferLowWatermark() override { onBelowLowWatermark(); } - bool strict1xxAnd204Headers() { return strict_1xx_and_204_headers_; } + bool sendStrict1xxAnd204Headers() { return send_strict_1xx_and_204_headers_; } // Codec errors found in callbacks are overridden within the http_parser library. This holds those // errors to propagate them through to dispatch() where we can handle the error. @@ -280,7 +280,8 @@ class ConnectionImpl : public virtual Connection, // HTTP/1 message has been flushed from the parser. This allows raising an HTTP/2 style headers // block with end stream set to true with no further protocol data remaining. bool deferred_end_stream_headers_ : 1; - const bool strict_1xx_and_204_headers_ : 1; + const bool accept_strict_1xx_and_204_headers_ : 1; + const bool send_strict_1xx_and_204_headers_ : 1; bool dispatching_ : 1; bool dispatching_slice_already_drained_ : 1; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 90808c6548eb..2cac0486acfc 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -57,6 +57,7 @@ constexpr const char* runtime_features[] = { // Begin alphabetically sorted section. "envoy.deprecated_features.allow_deprecated_extension_names", "envoy.reloadable_features.always_apply_route_header_rules", + "envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "envoy.reloadable_features.activate_timers_next_event_loop", "envoy.reloadable_features.add_and_validate_scheme_header", "envoy.reloadable_features.allow_500_after_100", @@ -84,7 +85,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.remove_forked_chromium_url", "envoy.reloadable_features.require_ocsp_response_for_must_staple_certs", "envoy.reloadable_features.return_502_for_upstream_protocol_errors", - "envoy.reloadable_features.strict_1xx_and_204_response_headers", + "envoy.reloadable_features.send_strict_1xx_and_204_response_headers", "envoy.reloadable_features.tls_use_io_handle_bio", "envoy.reloadable_features.treat_host_like_authority", "envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure", diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index e8dd86558239..11807c884009 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2295,7 +2295,7 @@ TEST_F(Http1ClientConnectionImplTest, 204ResponseContentLengthNotAllowed) { { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.strict_1xx_and_204_response_headers", "false"}}); + {{"envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "false"}}); initialize(); @@ -2331,7 +2331,7 @@ TEST_F(Http1ClientConnectionImplTest, 204ResponseWithContentLength0) { { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.strict_1xx_and_204_response_headers", "false"}}); + {{"envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "false"}}); NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); @@ -2365,7 +2365,7 @@ TEST_F(Http1ClientConnectionImplTest, 204ResponseTransferEncodingNotAllowed) { { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.strict_1xx_and_204_response_headers", "false"}}); + {{"envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "false"}}); initialize(); @@ -2467,7 +2467,7 @@ TEST_F(Http1ClientConnectionImplTest, 101ResponseTransferEncodingNotAllowed) { { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.strict_1xx_and_204_response_headers", "false"}}); + {{"envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "false"}}); initialize(); From be45659db77e7f96f850b6b36e355f9d4aa5d969 Mon Sep 17 00:00:00 2001 From: Taylor Barrella Date: Wed, 14 Apr 2021 20:49:46 +0000 Subject: [PATCH 2/2] accept_strict -> require_strict Signed-off-by: Taylor Barrella --- docs/root/version_history/current.rst | 4 ++-- source/common/http/http1/codec_impl.cc | 8 ++++---- source/common/http/http1/codec_impl.h | 2 +- source/common/runtime/runtime_features.cc | 2 +- test/common/http/http1/codec_impl_test.cc | 8 ++++---- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ab9ccdee6050..4230de3d70d4 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -54,8 +54,8 @@ Minor Behavior Changes * http: increase the maximum allowed number of initial connection WINDOW_UPDATE frames sent by the peer from 1 to 5. * http: no longer adding content-length: 0 for requests which should not have bodies. This behavior can be temporarily reverted by setting `envoy.reloadable_features.dont_add_content_length_for_bodiless_requests` false. * http: replaced setting `envoy.reloadable_features.strict_1xx_and_204_response_headers` with settings - `envoy.reloadable_features.accept_strict_1xx_and_204_response_headers` - (do not accept upstream 1xx or 204 responses with Transfer-Encoding or non-zero Content-Length headers) and + `envoy.reloadable_features.require_strict_1xx_and_204_response_headers` + (require upstream 1xx or 204 responses to not have Transfer-Encoding or non-zero Content-Length headers) and `envoy.reloadable_features.send_strict_1xx_and_204_response_headers` (do not send 1xx or 204 responses with these headers). Both are true by default. * http: upstream flood and abuse checks increment the count of opened HTTP/2 streams when Envoy sends diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 6dff75a8ee5d..ae4dc2a8a39a 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -459,8 +459,8 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat encode_only_header_key_formatter_(encodeOnlyFormatterFromSettings(settings)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), - accept_strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.accept_strict_1xx_and_204_response_headers")), + require_strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.require_strict_1xx_and_204_response_headers")), send_strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.send_strict_1xx_and_204_response_headers")), dispatching_(false), output_buffer_(connection.dispatcher().getWatermarkFactory().create( @@ -856,7 +856,7 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { os << spaces << "Http1::ConnectionImpl " << this << DUMP_MEMBER(dispatching_) << DUMP_MEMBER(dispatching_slice_already_drained_) << DUMP_MEMBER(reset_stream_called_) << DUMP_MEMBER(handling_upgrade_) << DUMP_MEMBER(deferred_end_stream_headers_) - << DUMP_MEMBER(accept_strict_1xx_and_204_headers_) + << DUMP_MEMBER(require_strict_1xx_and_204_headers_) << DUMP_MEMBER(send_strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_) << DUMP_MEMBER(buffered_body_.length()); @@ -1283,7 +1283,7 @@ Envoy::StatusOr ClientConnectionImpl::onHeadersCompleteBase() { handling_upgrade_ = true; } - if (accept_strict_1xx_and_204_headers_ && + if (require_strict_1xx_and_204_headers_ && (parser_->statusCode() < 200 || parser_->statusCode() == 204)) { if (headers->TransferEncoding()) { RETURN_IF_ERROR( diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 15aeb7eb605a..031d972a5a08 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -280,7 +280,7 @@ class ConnectionImpl : public virtual Connection, // HTTP/1 message has been flushed from the parser. This allows raising an HTTP/2 style headers // block with end stream set to true with no further protocol data remaining. bool deferred_end_stream_headers_ : 1; - const bool accept_strict_1xx_and_204_headers_ : 1; + const bool require_strict_1xx_and_204_headers_ : 1; const bool send_strict_1xx_and_204_headers_ : 1; bool dispatching_ : 1; bool dispatching_slice_already_drained_ : 1; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 2cac0486acfc..36ad15a06b1f 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -57,7 +57,6 @@ constexpr const char* runtime_features[] = { // Begin alphabetically sorted section. "envoy.deprecated_features.allow_deprecated_extension_names", "envoy.reloadable_features.always_apply_route_header_rules", - "envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "envoy.reloadable_features.activate_timers_next_event_loop", "envoy.reloadable_features.add_and_validate_scheme_header", "envoy.reloadable_features.allow_500_after_100", @@ -84,6 +83,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.preserve_downstream_scheme", "envoy.reloadable_features.remove_forked_chromium_url", "envoy.reloadable_features.require_ocsp_response_for_must_staple_certs", + "envoy.reloadable_features.require_strict_1xx_and_204_response_headers", "envoy.reloadable_features.return_502_for_upstream_protocol_errors", "envoy.reloadable_features.send_strict_1xx_and_204_response_headers", "envoy.reloadable_features.tls_use_io_handle_bio", diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 11807c884009..49ab6beb301a 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2295,7 +2295,7 @@ TEST_F(Http1ClientConnectionImplTest, 204ResponseContentLengthNotAllowed) { { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "false"}}); + {{"envoy.reloadable_features.require_strict_1xx_and_204_response_headers", "false"}}); initialize(); @@ -2331,7 +2331,7 @@ TEST_F(Http1ClientConnectionImplTest, 204ResponseWithContentLength0) { { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "false"}}); + {{"envoy.reloadable_features.require_strict_1xx_and_204_response_headers", "false"}}); NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); @@ -2365,7 +2365,7 @@ TEST_F(Http1ClientConnectionImplTest, 204ResponseTransferEncodingNotAllowed) { { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "false"}}); + {{"envoy.reloadable_features.require_strict_1xx_and_204_response_headers", "false"}}); initialize(); @@ -2467,7 +2467,7 @@ TEST_F(Http1ClientConnectionImplTest, 101ResponseTransferEncodingNotAllowed) { { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.accept_strict_1xx_and_204_response_headers", "false"}}); + {{"envoy.reloadable_features.require_strict_1xx_and_204_response_headers", "false"}}); initialize();