gatewayapi: deduplicate upstream TLS clusters#9510
Conversation
Signed-off-by: liuhy <liuhongyu@apache.org>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37f6d25f26
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| type stableClusterNameInput struct { | ||
| RouteType gwapiv1.Kind `json:"routeType"` | ||
| Settings []stableDestinationSetting `json:"settings"` | ||
| Traffic *stableClusterTraffic `json:"traffic,omitempty"` | ||
| UseClientProtocol *bool `json:"useClientProtocol,omitempty"` | ||
| StatName *string `json:"statName,omitempty"` | ||
| RouteHostname string `json:"routeHostname,omitempty"` |
There was a problem hiding this comment.
Include listener HTTP/1 settings in stable cluster key
When the same upstream-TLS backend is used from two HTTP listeners with different ClientTrafficPolicy HTTP/1 settings, this stable-name input still hashes to the same value. The xDS path passes httpListener.HTTP1 through HTTPRouteTranslator.asClusterArgs and buildTypedExtensionProtocolOptions uses EnableTrailers, PreserveHeaderCase, and HTTP10 to build the cluster's upstream protocol options, so addXdsCluster will skip the second cluster and one listener silently gets the other's upstream HTTP/1 behavior. Include these listener HTTP/1 settings in the stable key or avoid stable naming when they are set.
Useful? React with 👍 / 👎.
| Ciphers []string `json:"ciphers,omitempty"` | ||
| ECDHCurves []string `json:"ecdhCurves,omitempty"` | ||
| SignatureAlgorithms []string `json:"signatureAlgorithms,omitempty"` | ||
| ALPNProtocols []string `json:"alpnProtocols,omitempty"` |
There was a problem hiding this comment.
Preserve nil versus empty ALPN in stable TLS key
For backend TLS, an empty ALPNProtocols slice explicitly disables ALPN while nil leaves it unset, and the xDS translator only writes CommonTlsContext.AlpnProtocols when the slice is non-nil. Because this field is serialized with omitempty, nil and [] hash identically; two routes to the same backend with otherwise identical TLS settings but different Gateway/EnvoyProxy backend TLS ALPN intent will collide and the first cluster's ALPN behavior will be reused for both.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses Envoy memory growth from duplicated upstream TLS clusters by generating stable, backend-hash-based cluster names for upstream-TLS destinations (when cluster-level inputs match), enabling cluster reuse across routes and reducing redundant TLS context instantiation in Envoy.
Changes:
- Introduces stable cluster naming for eligible upstream-TLS destinations in the Gateway API translator.
- Adjusts xDS translation to keep IR route metadata while emitting backend metadata on stable backend-hash xDS clusters.
- Adds regression tests and refreshes affected golden outputs; adds a breaking-change release note for cluster name changes.
Review findings (required):
internal/gatewayapi/route.go:stableClusterName()returns""onjson.Marshalerror, which can silently produce invalid/empty cluster names (see stored comment).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| release-notes/current/breaking_changes/9489-upstream-tls-cluster-deduplication.md | Documents the breaking change in generated cluster names due to upstream TLS cluster deduplication. |
| internal/xds/translator/translator.go | Selects backend vs route metadata for xDS clusters when stable backend-hash cluster names are used. |
| internal/gatewayapi/translator_test.go | Adds regression coverage asserting cross-route reuse of the same upstream TLS cluster and validating emitted cluster metadata. |
| internal/gatewayapi/route.go | Implements stable backend-hash naming for eligible upstream-TLS route destinations and supporting hashing utilities. |
| internal/gatewayapi/route_test.go | Adds unit tests for stable cluster destination naming inputs and reuse behavior. |
| internal/gatewayapi/testdata/sds.out.yaml | Updates golden output to reflect stable backend-hash cluster names. |
| internal/gatewayapi/testdata/httproute-rule-with-non-service-backends-and-websocket-app-protocols.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/httproute-dynamic-resolver.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/httproute-dynamic-resolver-host-rewriting.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/gateway-tls-frontend-backend.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/envoyproxy-tls-settings.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-wildcard-older-than-sectioned.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-system-truststore.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-subjectaltnames.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-status-conditions-truncated.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-serviceimport-target.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-not-conflict.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-not-conflict-service.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-multiple-targets.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-default-ns-targetrefs.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-conflict.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-ca-only.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-ca-only-secret.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backendtlspolicy-ca-clustertrustbundle.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backend-with-skip-tls-verify.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backend-with-auto-san-sni.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backend-tls-settings.out.yaml | Updates golden output cluster names to stable backend-hash form. |
| internal/gatewayapi/testdata/backend-tls-auto-host-sni.out.yaml | Updates golden output cluster names to stable backend-hash form. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data, err := json.Marshal(input) | ||
| if err != nil { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 041015cb10
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| irRoute.Traffic, | ||
| irRoute.UseClientProtocol, |
There was a problem hiding this comment.
Include BackendTrafficPolicy inputs in stable cluster key
When two HTTPRoutes or GRPCRoutes share the same upstream-TLS backend but only one is targeted by a BackendTrafficPolicy, this hashes the destination before ProcessBackendTrafficPolicies runs; applyTrafficFeatureToRoute later mutates r.Traffic and r.UseClientProtocol, which are cluster-level inputs used by the xDS translator. Both routes can therefore keep the same backend/<hash> name, so addXdsCluster skips the second cluster and one route silently uses the other's timeout/load-balancer/circuit-breaker/use-client-protocol settings. The stable name either needs to be assigned after policy application or recomputed when these policy-derived fields are set.
Useful? React with 👍 / 👎.
Signed-off-by: liuhy <liuhongyu@apache.org>
Signed-off-by: liuhy <liuhongyu@apache.org>
Signed-off-by: liuhy <liuhongyu@apache.org>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9510 +/- ##
==========================================
+ Coverage 75.51% 75.55% +0.04%
==========================================
Files 252 252
Lines 41669 41930 +261
==========================================
+ Hits 31465 31680 +215
- Misses 8079 8108 +29
- Partials 2125 2142 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
I agree 🚀🚀 |
Fixes #9489.
Summary
Tests
git diff --checkgo test ./internal/gatewayapi -run 'TestStableClusterRouteDestinationReusesSameTLSBackend|TestStableClusterRouteDestinationUsesXDSClusterInputs|TestTranslateReusesUpstreamTLSClusterForSameBackend|TestTranslate/(backend-tls-auto-host-sni|backend-tls-settings|backend-with-auto-san-sni|backend-with-skip-tls-verify|backendtlspolicy-ca-clustertrustbundle|backendtlspolicy-ca-only-secret|backendtlspolicy-ca-only|backendtlspolicy-conflict|backendtlspolicy-default-ns-targetrefs|backendtlspolicy-multiple-targets|backendtlspolicy-not-conflict-service|backendtlspolicy-not-conflict|backendtlspolicy-serviceimport-target|backendtlspolicy-status-conditions-truncated|backendtlspolicy-subjectaltnames|backendtlspolicy-system-truststore|backendtlspolicy-wildcard-older-than-sectioned|envoyproxy-tls-settings|gateway-tls-frontend-backend|httproute-dynamic-resolver-host-rewriting|httproute-dynamic-resolver|httproute-rule-with-non-service-backends-and-websocket-app-protocols|sds)$'\n-go test ./internal/xds/translator\n-go test ./...fails only inTestTranslate/securitypolicy-with-oidcbecausehttps://accounts.google.com/.well-known/openid-configurationtimes out in this environment\n