fix: force HTTP1 for upstream connections for WS and WSS backends#8699
fix: force HTTP1 for upstream connections for WS and WSS backends#8699zhaohuabing wants to merge 5 commits intoenvoyproxy:mainfrom
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 canceled.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8699 +/- ##
==========================================
- Coverage 74.43% 74.42% -0.02%
==========================================
Files 245 245
Lines 38973 39007 +34
==========================================
+ Hits 29010 29031 +21
- Misses 7960 7972 +12
- Partials 2003 2004 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
| }, | ||
| } | ||
| // If forceHTTP1UpstreamProtocol is set, force Envoy to use HTTP1 upstream regardless of other settings. | ||
| case forceHTTP1UpstreamProtocol: |
There was a problem hiding this comment.
can we reuse requiresHTTP1Options ?
There was a problem hiding this comment.
We can’t. forceHTTP1UpstreamProtocol takes precedence over requiresHTTP2Options, which in turn takes precedence over requiresHTTP1Options.
| // influence transport socket specific settings | ||
| requiresAutoHTTPConfig := len(args.settings) > 0 | ||
| requiresHTTP2Options := false | ||
| forceHTTP1UpstreamProtocol := false |
There was a problem hiding this comment.
instead of this, would it make sense to use AppProtocol of WS in xds layer and set requiresHTTP1Options for that case ?
There was a problem hiding this comment.
requiresHTTP1Options and forceHTTP1UpstreamProtocol have different meanings. forceHTTP1UpstreamProtocol takes precedence over requiresHTTP2Options, which in turn takes precedence over requiresHTTP1Options.
There was a problem hiding this comment.
It would be easier to read if requiresHTTP2Options was named hasHTTP2OrGRPCUpstream
if ds.Protocol == ir.GRPC ||
ds.Protocol == ir.HTTP2 {
requiresHTTP2Options = true
}
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
zhaohuabing
added
cherrypick/release-v1.7.3
and removed
cherrypick/release-v1.7.2
labels
|
/retest |
arkodg
left a comment
arkodg
left a comment
There was a problem hiding this comment.
would be good to have per port support in the future
2 participants
This PR forces forces HTTP/1.1 upstream connections instead of negotiating HTTP/2 for
wsandwssBackend appProtocols. This change avoids compatibility issues with WebSocket backends that do not support RFC 8441 extended CONNECT.Fixes: #8693