http: changing mutate[request|response] to special case all upgrade headers #3735
Conversation
…eaders Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
So I noticed that I was stripping the transferEncoding header on the response path and not the request path, which was clearly inconsistent and wrong. After some poking around and a question posited on #3301 I'm leaning towards just properly handling the transfer encoding header as I proposed there. For now, I'm leaving both mutate functions as-is with respect to the T-E header but simply adding tests of the existing behavior and TODOs to fix it. |
| const bool no_body = | ||
| (!response_headers.TransferEncoding() && !response_headers.ContentLength()); | ||
| if (no_body) { | ||
| response_headers.insertContentLength().value(uint64_t(0)); |
There was a problem hiding this comment.
Why is this needed in the upgrade path but not other cases? I think I'm just ignorant of the standard here...
There was a problem hiding this comment.
For response headers I believe you can frame by connection close. That won't work for WebSocket upgrades because you can't close between HTTP and WebSocket payload.
Extending the mutateRequestHeaders to special case all upgrades, not just WebSocket.
Adding special handling for mutateResponse headers to do similar special casing of upgrade responses.
Changing the utility we use to determine if there is an upgrade header. The now removed caseInsensitiveContains had a bug where it would parse "upgrade: Websocket Keep-Alive" as a websocket upgrade. The existing Envoy::StringUtil::caseFindToken will parse that as one token but still handles "upgrade: Webocket, keep-alive" correctly.
Risk Level: Medium (changing existing websocket handling)
Testing: new unit tests, regression test for fixed bug.
Docs Changes: n/a
Release Notes: not added. We could note the bugfix if we think it noteworthy.
Part of #3301