http: turning up H2 websocket support #4767
Conversation
Signed-off-by: Alyssa Wilk <[email protected]>
|
@rshriram or @moderation may want to take a look too? |
|
Huh, #4761 landed, it caused a new docker image, https://hub.docker.com/r/envoyproxy/envoy/tags/, this should Just Work (TM) but something about the old nghttp2 library is getting cached somewhere. I'll see if I can sort it out tomorrow. |
|
@alyssawilk you need to update the build image in |
|
This is great!! don't care much about being able to support upgrades on requests with bodies. |
alyssawilk
force-pushed
the
h2_final
branch
3 times, most recently
from
703ab00 to
4ab867c
Compare
|
Oh thanks Piotr! I'd remembered it was latched somewhere and was failing to find it. That plus the proper hash (I was grabbing envoyproxy/envoy not envoyproxy/envoy-build hashes) did it. |
alyssawilk
force-pushed
the
h2_final
branch
2 times, most recently
from
d226559 to
fe3ef7a
Compare
Signed-off-by: Alyssa Wilk <[email protected]>
|
Ping! |
| @@ -20,8 +20,8 @@ one can set up custom | |||
| for the given upgrade type, up to and including only using the router filter to send the WebSocket | |||
| data upstream. | |||
|
|
|||
| Handling H2 hops (implementation in progress) | |||
| --------------------------------------------- | |||
| Handling H2 hops | |||
source/common/http/utility.cc
Outdated
| // nghttp2 rejects upgrade requests/responses with content length, so strip | ||
| // any unnecessary content length header. | ||
| if (headers.ContentLength() != nullptr && | ||
| headers.ContentLength()->value().c_str() == std::string("0")) { |
There was a problem hiding this comment.
perf nit: you can do this w/o string creation. Same below.
| @@ -6,6 +6,7 @@ Version history | |||
| * admin: added support for displaying subject alternate names in :ref:`certs<operations_admin_interface_certs>` end point. | |||
| * config: removed support for the v1 API. | |||
| * fault: removed integer percentage support. | |||
| * http: Added HTTP/2 WebSocket proxying via :ref:`extended CONNECT <envoy_api_field_core.Http2ProtocolOptions.allow_connect>` | |||
There was a problem hiding this comment.
Related to my comment on the old style removal PR, but do we need any stats/observability for this stuff? If so it's fine to track as a follow up but might want to put in a TODO somewhere?
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
| @@ -17,6 +17,7 @@ namespace Http { | |||
| COUNTER (downstream_cx_total) \ | |||
| COUNTER (downstream_cx_ssl_total) \ | |||
| COUNTER (downstream_cx_http1_total) \ | |||
| COUNTER (downstream_cx_upgrades_total) \ | |||
There was a problem hiding this comment.
I'm a little concerned this might be confusing for people looking for WS socket stats? Probably not actionable right now but something to think about. Not sure if folks will find it confusing or not. We might want to have more docs on this? Feel free to do in your other removal PR if you are inclined.
The main difference between what we had and official nghttp2 support is lack of support for upgrade-with-bodies (on request or response path). Adjusted header munging, tests, and docs accordingly.
Risk Level: Low (changes code on a "hidden" code path)
Testing: updated tests, new unit tests
Docs Changes: updated
Release Notes: noted H2 websocket support
Fixes #1630