buffer filter: add content-length when ending stream with trailers#8609
buffer filter: add content-length when ending stream with trailers#8609junr03 merged 3 commits intoenvoyproxy:masterfrom junr03:buffer-filter
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
| // request_headers_ is initialized iff plugin is enabled. | ||
| if (request_headers_ != nullptr && request_headers_->ContentLength() == nullptr) { | ||
| ASSERT(!settings_->disabled()); | ||
| if (Runtime::runtimeFeatureEnabled( | ||
| "envoy.reloadable_features.buffer_filter_populate_content_length")) { | ||
| request_headers_->insertContentLength().value(content_length_); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Can this code be moved to a shared function, please?
It's non-trivial enough to care about the code duplication.
| codec_client_->sendData(*request_encoder_, "789", false); | ||
| codec_client_->sendTrailers(*request_encoder_, Http::TestHeaderMapImpl{}); |
There was a problem hiding this comment.
Trying to improve my knowledge of internal envoy logic: is this semantically different from sendData(..., true)?
How are either of them materialized on the wire?
There was a problem hiding this comment.
yep it is different. sendTrailers will encode a HEADERS frame with END_STREAM true while sendData(data, true) encodes a DATA frame with END_STREAM true. Envoy mobile closes client streams with trailers, so the content-length headers was not getting set in our edge infrastructure.
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM modulo shared utility function
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM - feel free to self merge once CI is working
|
looking at test failures |
Signed-off-by: Jose Nino <jnino@lyft.com>
| codec_client_->sendData(*request_encoder_, "456", false); | ||
| codec_client_->sendData(*request_encoder_, "789", false); | ||
| Http::TestHeaderMapImpl request_trailers{{"request", "trailer"}}; | ||
| codec_client_->sendTrailers(*request_encoder_, request_trailers); |
There was a problem hiding this comment.
@alyssawilk with an empty map I got an asan failure because of taking a reference from a nullptr here:
envoy/source/common/http/http2/codec_impl.cc
Line 216 in cc8fe6d
Things obviously end up fine because nghttp2 first checks the array length. Do you think it is worth doing something envoy side so that we can send empty trailers without tripping the address sanitizer?
There was a problem hiding this comment.
I'm also confused how content length changes here result in empty trailers. Can you clue me in a bit more?
There was a problem hiding this comment.
oh sorry, I meant that when I originally wrote the test by sending an empty trailers map (78f6b39#diff-13f71f37c14f0986b566cbf97601bf44R78) I was getting an ASAN failure. So not an effect of the patch, but rather of how I wrote the test.
There was a problem hiding this comment.
Oh gotcha! Yeah, if sending an empty trailers map causes ASAN bugs, we should file and fix that as a separate PR lest some filters clear the only trailers entry and cause problems
@mattklein123 for visibility.
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM modulo filing a tracking issue regarding the trailers bug
* master: (54 commits) Update dependencies - Go, Bazel toos, xxHash, nanopb, rules_go, curl, protobuf, Jinja, yaml-cpp, re2 (envoyproxy#8728) test: increase coverage of listener_manager_impl.cc (envoyproxy#8737) test: modify some macros to reduce number of uncovered lines reported (envoyproxy#8725) build: add a script to setup clang (envoyproxy#8682) http: fix ssl_redirect on external (envoyproxy#8664) docs: update fedora build requirements (envoyproxy#8641) fix draining listener removal logs (envoyproxy#8733) dubbo: fix router doc (envoyproxy#8734) server: provide server health status when stats disabled (envoyproxy#8482) router: adding a knob to configure a cap on buffering for shadowing/retries (envoyproxy#8574) tcp proxy: add default 1 hour idle timeout (envoyproxy#8705) thrift: fix filter names in docs (envoyproxy#8726) Quiche changes to avoid gcc flags on Windows (envoyproxy#8514) test: increase test coverage in Router::HeaderParser (envoyproxy#8721) admin: add drain listeners endpoint (envoyproxy#8415) buffer filter: add content-length when ending stream with trailers (envoyproxy#8609) clarify draining option docs (envoyproxy#8712) build: ignore go-control-plane mirror git commit error code (envoyproxy#8703) api: remove API type database from checked in artifacts. (envoyproxy#8716) admin: correct help strings (envoyproxy#8710) ... Signed-off-by: Spencer Lewis <slewis@squareup.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: #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.