http: fixing upgrade response bug#10615
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@alyssawilk I might be misunderstanding, but I don't think this fixes #10566. The issue in that ticket AFIUI is that the client codec is shifting into upgrade mode when it received the upgrade header in the response, even though it isn't in response to a valid upgrade request. I think we need to actually remember in the request path if it's a valid upgrade request (like we do for HEAD requests) and then not shift into upgrade mode in the response in that case? |
|
Ah, I think there's 2 bugs. There's what the codec does pulling in data, and what it does when serializing it out. I was indeed fixing only one, making sure it was written out correctly, where we need to fix both. Better fix and integration test coming up! |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Clearing response upgrade headers here would also fix this too, right? #8797 |
|
This does clear them over in conn_manager_utility.cc |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Awesome thanks. Just 1 question and 1 small test comment.
/wait
| if (pending_response_.has_value()) { | ||
| return pending_response_->encoder_.upgrade_request_; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Do we have a test that hits this branch?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Nice thanks, just small nit.
/wait
docs/root/intro/version_history.rst
Outdated
| * http filters: http filter extensions use the "envoy.filters.http" name space. A mapping | ||
| of extension names is available in the :ref:`deprecated <deprecated>` documentation. | ||
| * http: fixing a bug where the upgrade header was not cleared on responses to non-upgrade requests. | ||
| Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false. |
|
Sorry can you merge master and move to 1.15.0? /wait |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@lambdai for ?backport? |
docs/root/intro/version_history.rst
Outdated
| 1.15.0 (Pending) | ||
| ================ | ||
|
|
||
| * http: fixing a bug where the upgrade header was not cleared on responses to non-upgrade requests. |
There was a problem hiding this comment.
s/fixing/fixed (per your own request =P)?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
LGTM. Can you merge master in the morning once CI is fixed so we can make sure this passes? /wait |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
docs CI error looks legit :( /wait |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
3 participants
Fixing a bug where upgrade response headers triggered the upgrade path
Risk Level: low (fixing a bug, runtime guarded)
Testing: fixed unit tests
Docs Changes: n/a
Release Notes: yes
Fixes #10566