buffer filter: Populate content-length header#7848
buffer filter: Populate content-length header#7848mattklein123 merged 3 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, LGTM with some small comments.
/wait
docs/root/intro/version_history.rst
Outdated
| * access log: added :ref:`buffering <envoy_api_field_config.accesslog.v2.CommonGrpcAccessLogConfig.buffer_size_bytes>` and :ref:`periodical flushing <envoy_api_field_config.accesslog.v2.CommonGrpcAccessLogConfig.buffer_flush_interval>` support to gRPC access logger. Defaults to 16KB buffer and flushing every 1 second. | ||
| * admin: added ability to configure listener :ref:`socket options <envoy_api_field_config.bootstrap.v2.Admin.socket_options>`. | ||
| * admin: added config dump support for Secret Discovery Service :ref:`SecretConfigDump <envoy_api_msg_admin.v2alpha.SecretsConfigDump>`. | ||
| * buffer filter: the buffer filter populates content-length header if not present, behavior can be disabled using the runtime feature `envoy.reloadable_features.buffer_populate_content_length`. |
There was a problem hiding this comment.
I would call this envoy.reloadable_features.buffer_filter_populate_content_length. Also, can you refer to this in the fitler docs also?
| Http::FilterDataStatus BufferFilter::decodeData(Buffer::Instance& data, bool end_stream) { | ||
| content_length_ += data.length(); | ||
| if (end_stream || settings_->disabled()) { | ||
| if (request_headers_ != nullptr && request_headers_->ContentLength() == nullptr) { |
There was a problem hiding this comment.
I see that request_headers_ being non-null is guarded by disabled so the if statement above is OK, but can you add a comment about this? It's a little non-obvious. You could potentially also ASSERT(!settings_->disabled())
|
@euroelessar we suspect this may have introduced a regression in the execution time for |
|
Specifically, Running with |
|
Possibly related to #7872 |
|
Appears to be fixed by #7885 |
…8609) Description: #7848 introduced content-length population in the buffer filter. However, it is only populated if the client stream ends on a data frame. This PR adds content-length population when closing the stream via trailers. Risk Level: low - expanded functionality runtime guarded. Testing: unit and integration tests. Release Notes: I believe the current release notes added by #7848 are still adequate. Signed-off-by: Jose Nino <jnino@lyft.com>
…nvoyproxy#8609) Description: envoyproxy#7848 introduced content-length population in the buffer filter. However, it is only populated if the client stream ends on a data frame. This PR adds content-length population when closing the stream via trailers. Risk Level: low - expanded functionality runtime guarded. Testing: unit and integration tests. Release Notes: I believe the current release notes added by envoyproxy#7848 are still adequate. Signed-off-by: Jose Nino <jnino@lyft.com>
Description: Make buffer filter populate
content-lengthheader if it is not present in the request already. As buffer has to fetch the whole request in order to proceed, calculation of the header value involves mostly no extra cost.Risk Level: MEDIUM
Testing: added unit and integration tests
Docs Changes: added section about new behavior around
content-lengthheaderRelease Notes: updated
Fixes #7844