fix: force HTTP1 for upstream connections for WS and WSS backends#8699
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 <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
| }, | ||
| } | ||
| // 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 <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
|
/retest |
arkodg
left a comment
There was a problem hiding this comment.
would be good to have per port support in the future
|
/retest |
Agreed. The current Backend appProtocols field is backend-wide, so this PR keeps the behavior at that level. Per-port appProtocol support would need API changes and can be handled separately. |
…voyproxy#8699) * force HTTP1 for upstream connections for WS and WSS backends Signed-off-by: Huabing (Robin) Zhao <[email protected]> * use different clusters for mixed upstream protocols Signed-off-by: Huabing (Robin) Zhao <[email protected]> * update Signed-off-by: Huabing (Robin) Zhao <[email protected]> * fix lint Signed-off-by: Huabing (Robin) Zhao <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> Signed-off-by: Jake Oliver <[email protected]>
* fix json report (#8614) Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit 4768ca7) * fix: deep copy status in translator layer to avoid race (#8778) Signed-off-by: Rudrakh Panigrahi <[email protected]> (cherry picked from commit 3f70a89) * fix: force HTTP1 for upstream connections for WS and WSS backends (#8699) * force HTTP1 for upstream connections for WS and WSS backends Signed-off-by: Huabing (Robin) Zhao <[email protected]> * use different clusters for mixed upstream protocols Signed-off-by: Huabing (Robin) Zhao <[email protected]> * update Signed-off-by: Huabing (Robin) Zhao <[email protected]> * fix lint Signed-off-by: Huabing (Robin) Zhao <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit 7633125) * fix: reason with multiple errors rejected validation (#8859) * fix: reason with multiple errors rejected validation Signed-off-by: zirain <[email protected]> * release notes Signed-off-by: zirain <[email protected]> * fix lint Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]> (cherry picked from commit 7811d86) * feat(chart): Allow configuring envoy proxy image via helm chart (#8785) * feat: Allow configuring envoy proxy defaults via helm chart This commit is a continuation of the previous work to support supplying default proxy settings added in #7698 and adds three new chart values under `global.images.envoyProxy`: | Value | Type | Default | Description | |----------------------------------------|--------|------|---------------------------------------------------------------------| | `global.images.envoyProxy.image` | string | `""` | Full image name (`registry/repo:tag`) for the Envoy Proxy container | | `global.images.envoyProxy.pullPolicy` | string | `""` | Image pull policy | | `global.images.envoyProxy.pullSecrets` | list | `[]` | Image pull secrets | When any of these are set, the chart generates an `envoyProxy:` block inside the `EnvoyGateway` ConfigMap, wiring into the existing `EnvoyGatewaySpec.envoyProxy` field (added in #7698). The global `imageRegistry` override takes highest precedence, consistent with other chart components. Full EnvoyProxy defaults (replicas, resources, etc.) can be provided via `config.envoyGateway.envoyProxy`; the image values are merged on top. Closes #4764. Signed-off-by: Michael Sommerville <[email protected]> (cherry picked from commit 8570285) * add rn and version bump Signed-off-by: Karol Szwaj <[email protected]> * fix: respect backend endpoint hostname for health checks (#8929) * fix: respect backend endpoint hostname for health checks - Keep BackendTrafficPolicy HTTP health check hostnames as explicit cluster-level hosts, and leave route-derived host fallback to xDS cluster translation. - Preserve Backend endpoint hostnames as per-endpoint overrides via Endpoint.HealthCheckConfig.hostname, ahead of the route fallback. - Update gatewayapi/xDS fixtures, release notes, and generated API docs/CRDs for the host selection order. - go test ./internal/ir - go test ./internal/xds/translator - go test ./internal/gatewayapi -run TestTranslate/backendtrafficpolicy - go test ./internal/gatewayapi -run TestTranslate/(clienttrafficpolicy-http-health-check|envoyextensionpolicy-with-extproc-with-retries|envoyextensionpolicy-with-extproc-with-traffic-features|envoyproxy-accesslog-with-traffic|envoyproxy-tracing-backend-uds|envoyproxy-tracing-backend|securitypolicy-with-jwt-backendcluster|securitypolicy-with-jwt-backendsettings) - make generate - make manifests - git diff --check Signed-off-by: Arko Dasgupta <[email protected]> Co-authored-by: Codex <[email protected]> * fix gen Signed-off-by: zirain <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: zirain <[email protected]> Co-authored-by: Codex <[email protected]> Co-authored-by: zirain <[email protected]> * update release notes Signed-off-by: Karol Szwaj <[email protected]> * fix gen-check Signed-off-by: Karol Szwaj <[email protected]> * Revert "feat(chart): Allow configuring envoy proxy image via helm chart (#8785)" This reverts commit 092cc67. Signed-off-by: Karol Szwaj <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]> Signed-off-by: Rudrakh Panigrahi <[email protected]> Signed-off-by: zirain <[email protected]> Signed-off-by: Michael Sommerville <[email protected]> Signed-off-by: Karol Szwaj <[email protected]> Signed-off-by: Arko Dasgupta <[email protected]> Co-authored-by: Huabing (Robin) Zhao <[email protected]> Co-authored-by: Rudrakh Panigrahi <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Michael Sommerville <[email protected]> Co-authored-by: Arko Dasgupta <[email protected]> Co-authored-by: Codex <[email protected]>
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