feat: add MergeBackends option for backend cluster deduplication#9477
feat: add MergeBackends option for backend cluster deduplication#9477muwaqar-cflt wants to merge 127 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Muhammad Waqar <waqar.hameed08@gmail.com>
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9477 +/- ##
==========================================
+ Coverage 75.53% 75.79% +0.26%
==========================================
Files 252 253 +1
Lines 41646 42174 +528
==========================================
+ Hits 31457 31967 +510
- Misses 8062 8067 +5
- Partials 2127 2140 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Muhammad Waqar <waqar.hameed08@gmail.com>
d5cbd23 to
0f5efab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6011972ddb
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31fbfa0fa5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // | ||
| // +optional | ||
| // +kubebuilder:default=false | ||
| Enabled *bool `json:"enabled,omitempty"` |
There was a problem hiding this comment.
We may want to default this to true eventually. The fallback approach won't break current user-facing routing behaviors, and it helps with the Envoy memory issue.
It may be too risky to turn this on by default in 1.9, as it does break EPPs and extension servers, but we could keep it opt-in for the first release and consider flipping the default in 1.10 after we have more coverage and user feedback.
There was a problem hiding this comment.
resolving because not seeing an action item for now.
There was a problem hiding this comment.
is this flag needed ? mergeBackends: {} should imply we are turning on the feature
There was a problem hiding this comment.
yeah, not necessarily. A bool flag is more explicit than mergeBackends: {}, I'm fine with both.
There was a problem hiding this comment.
i like an explicit flag, mergeBackends: {} looks odd
|
lint fails :( |
|
This PR looks good. Great work! I left a few minor comments. @muwaqar-cflt Could you please fix the link and check whether all e2e tests pass? I’d like to get this merged before 1.9.0 if possible. Thanks! |
Per maintainer feedback on PR envoyproxy#9477, ship only the Enabled boolean for the first release. Fallback (the only supported behavior) is now unconditional; Force was already rejected by CEL validation and had no implementation, so there was no user-visible behavior to preserve by keeping the enum. A Mode/MergeBackendsMode field can be reintroduced later if a concrete Force use case emerges. Regenerated deepcopy, CRDs, and docs; dropped mode: Fallback from all mergebackends-* input fixtures and their derived golden output; replaced the CEL validation cases exercising Mode with Enabled-only equivalents. Signed-off-by: Muhammad Waqar <waqar.hameed08@gmail.com>
A merged cluster is already scoped to one gateway, so a gateway-level BTP setting applies uniformly to every route sharing it and can never cause the divergence this check exists to catch — worth a one-line note for future readers, per review feedback on PR envoyproxy#9477. Signed-off-by: Muhammad Waqar <waqar.hameed08@gmail.com>
…r_test.go
cluster.go: the three reverted asClusterArgs signatures (UDP/TCP/HTTP) had
collapsed from main's multi-line parameter formatting to single-line during
Task 9's revert, and HTTPRouteTranslator.asClusterArgs's struct literal had
statName/metadata swapped relative to main's field order. Restore both to
match main exactly. Also reformats the new (no main counterpart)
mergedHTTPRouteTranslator.asClusterArgs's signature the same way, for
consistency with its siblings.
cluster_test.go: four TestBuildClusterWithBackendUtilization* tests had
settings/endpointType field order swapped relative to main, left over from
Task 11 converting their xdsClusterArgs{backendCluster: ...} literals to flat
fields. Restore main's field order.
Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…, not per-route Merged cluster construction previously ran inside each HTTP/TCP/UDP route's own cluster-building loop, relying on addXdsCluster's existing-name skip to deduplicate across routes sharing one backend. Since a merged cluster has no Traffic settings of its own, this meant whichever route happened to build it first silently decided the cluster's circuitBreaker/healthCheck/timeout/etc, non-deterministically (see envoyproxy#9477 (comment)). Until BackendCluster gets a real Traffic field of its own (pending clarification from EG maintainers on whether merged clusters should inherit Gateway-level BackendTrafficPolicy settings), stop guessing: build every merged cluster in a single new processBackendClusters pass over xdsIR.Backends, using a stateless BackendClusterTranslator (renamed from mergedHTTPRouteTranslator, and no longer route-scoped) that never applies any route-specific Traffic. Removed the now-redundant per-route merged-cluster loops from the HTTP, TCP, and UDP listener processing blocks, since processBackendClusters already covers every merged backend regardless of which route type registered it. Also moved merged-cluster client-cert processing into the same pass, since it was doing the same per-route-instead- of-per-cluster work (harmless before, since addXdsSecret dedupes by name, but still redundant). Extracted the shared "does this destination need its own route-scoped cluster" guard (len(Settings) > 0 || len(BackendClusterRefs) == 0 - verified by testing that dropping either half breaks a real scenario: the Settings check for mixed merge/non-merge rules, the BackendClusterRefs check for the placeholder-destination edge case) into needsRouteCluster, used identically across HTTP/TCP/UDP instead of duplicated three times. Restructured the HTTP cluster-building block so this check wraps the whole NeedsClusterPerSetting if/else, rather than being nested only inside one branch. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…y need backendClusterIndex
resolveBackendClusters (and everything that had to call it) was a method on
*Translator purely to reach t.backendIndex, forcing buildXdsRoute,
buildXdsRouteAction, buildXdsURLRewriteAction, and processSDSClusters to
either become Translator methods or take a whole *Translator parameter, even
though none of them use anything else on Translator. Convert
resolveBackendClusters to a plain function taking backendClusterIndex
directly (already an existing, immutable-per-Translate()-call type), and
change every one of those functions to take backendIndex backendClusterIndex
instead of t *Translator, declaring exactly what they need. Callers now pass
t.backendIndex explicitly at the one call site where t is genuinely in scope
for other reasons.
Restored processSDSClusters to main's original inline-loop structure (no
scanSettings closure) by extracting a routeDestinationSettings helper that
combines a RouteDestination's own Settings with its resolved backends'
Settings into one slice, so the HTTP/TCP loop bodies stay a single
range-and-check block instead of two separate scan calls per route.
Also: dropped the now-redundant "merged" qualifier throughout
backend_registry.go (everything reachable through backendClusterIndex is
inherently merged by construction, so calling it out repeatedly added no
information) and renamed a few functions for clarity:
mergedBackendClusterRef -> backendClusterRef, allDestinationSettings ->
routeDestinationSettings, mergedHTTPRouteTranslator -> BackendClusterTranslator
(from an earlier commit on this branch), tcpDestinationClusterName ->
singleClusterDestinationName (it's used for TCP, UDP, and TLS routes alike,
not just TCP). Trimmed several doc comments that had drifted into
caller-perspective phrasing ("so callers can...").
Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…deltas - cluster.go: remove BackendClusterTranslator's isRoute: true field. It's dead: ExtraArgs.timeout is never set for merged clusters, so the only reader (hasTimeoutArgs) can never see args.timeout != nil for this type. - translator.go: restore main's multi-line processXdsCluster call formatting and the tSettings intermediate variable in the HTTP per-setting loop, restore the "1:1 between IR UDPRoute and xDS Cluster" comment, and drop the redundant "Skip a fully merged destination..." comments at the needsRouteCluster guards. - translator_test.go: revert the tracing-invalid expected error back to "validation failed for xds resource", matching main. Translate()'s joined error contains both messages, so either substring passes the test's require.Contains check. - route_test.go: restore the dropped Settings field on TestBuildXdsURLRewriteAction_AppendXForwardedHost's fixture, matching main. - utils_test.go: remove TestCreateExtServiceXDSCluster, a new test for createExtServiceXDSCluster, a function this PR never modifies. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
… terminology Rename mergedClusters -> backendClusters and replace the three-way weighted- routing condition with a single clusterCount tally (backend clusters plus one if Settings is non-empty), gated on clusterCount > 1. Share the single-cluster return path between the one-backend-cluster and zero-backend-cluster cases via a clusterName default instead of two near-identical return statements. Move applyWeightedClusterFilters after its only caller, buildXdsWeightedRouteAction. Comments no longer use "merged"/"non-merged" as adjectives; they describe the two data sources (Settings, backend clusters) directly. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
… additions - helpers.go: switch IsMergeBackendsEnabled to inline comments, matching its sibling IsMergeGatewaysEnabled's established style instead of a doc comment. - backendtrafficpolicy_test.go: drop a cross-reference to mergeIncompatibleForWeightedRule from TestBTPLoadBalancerIndexIsConsistentHash's doc comment. Reviewed contexts.go, backendtrafficpolicy.go, translator.go, translator_test.go, runner/runner.go, helpers_test.go, and filters.go against main; all other MergeBackends additions there are purely additive and already consistent with house style. filters.go's xdsIR/gatewayCtx/btpRoutingType threading is a mechanical consequence of route.go's processDestination signature and will be revisited when route.go gets its own pass. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…edback Addresses several review threads from PR envoyproxy#9477 on the performance/naming of the merge-eligibility machinery: - BuildBTPIndexes replaces BuildBTPRoutingTypeIndex + the prior BuildBTPClusterSettingsIndex/BuildBTPLoadBalancerIndex pair, building all three BackendTrafficPolicy indexes (grouped in a new BTPIndexes struct) in a single pass over btps, resolving each policy's targets at most once instead of once per index. mergeBackendsEnabled gates ClusterSettings/ LoadBalancer per-BTP internally (moot when no accepted gateway can enable merging), while RoutingType applies unconditionally - collapsing two separate outer `if` gates in Translate() into one direct call. - Fixed a pre-existing bug in the LoadBalancer index: like RoutingType, it stores a real value (whether LoadBalancer.Type == ConsistentHash) that can legitimately differ between BTPs targeting the same gateway, but unlike RoutingType it had no first-write-wins guard, so the last-processed policy silently won instead of the highest-priority one. - Added btpRoutingTypeIndexMaps/btpClusterSettingsIndexMaps/ btpLoadBalancerIndexMaps constructors for consistency across all three index types. - resolveRoutingContext -> resolveRuleBackendContext, with a doc comment reflecting everything it actually resolves (gateway, BTP routing type, and cluster-settings signal), not just "routing context". - resolveBackendCluster now builds backendClusterIdentity itself, only on the merge-eligible path, instead of processBackendRef building it unconditionally before the merge decision is even made. - Documented the gatewayCtx == nil branch in resolveBackendCluster. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…x stale refs
- Remove backendClusterRefBuilder: its .add() method was a trivial append
with no other logic, so all 5 call sites (HTTP/GRPC/TLS/UDP/TCP) now just
append directly to a plain []*ir.BackendClusterRef.
- shouldMergeBackend: restructure the RoutingType-divergence check to match
the "if X { return false }" shape used by every other check in the
function, ending with an explicit return true.
- Fix two comments pointing at mergedBackendClusterRef/getOrCreateBackendCluster
in ways that had gone stale or were unnecessary cross-references
(mergedBackendClusterRef no longer exists anywhere - it was renamed to
backendClusterRef in a different package earlier on this branch).
- Trim a comment in processBackendRef that named two sibling helper
functions to explain a two-step name assignment; state the fact instead.
- Drop the "Kind is left empty here..." comment on resolveBackendCluster's
non-merge branch - it raised a question (why doesn't this collide?) that
it didn't answer.
Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…work, address route.go review feedback Implements the two agreed P0 items from the route.go performance/structure review, plus a round of follow-up consistency fixes: - Collapse resolveBackendCluster/ResolvedBackendCluster/getOrCreateBackendCluster's three-way split into a single resolveBackendClusterKey (nil = not eligible) folded into processBackendRef, which now returns *ir.BackendClusterRef directly (nil for the non-merge case) instead of an intermediate cluster struct the caller had to unpack at all 5 call sites. BackendClusterKey is now only ever constructed on the actual merge-eligible path - the non-merge case, the common one when MergeBackends is off, no longer allocates one and throws it away. - getOrCreateBackendCluster no longer takes a separate clusterName param (computed internally from key); it also now overwrites the shared Setting's Name to match the BackendCluster's own name rather than whichever route-scoped name ds carried in with, fixing a real bug the reordering above introduced and caught via the full test suite (merged clusters were briefly getting route-scoped destination names). - Exit earlier when merging is disabled: mergeIncompatibleForWeightedRule/ mergeIncompatibleForSingleClusterRule's computation (and the hasRouteLevelClusterSettings BTP index lookup it depends on) is now skipped entirely unless isMergeBackendsEnabledForGateway is true, at all 5 route-type call sites (HTTP/GRPC/TLS/UDP/TCP) for consistency, even though TLS/UDP/TCP's variant is cheap enough that this saves little on its own. - resolveRuleBackendContext only resolves gatewayCtx/btpRoutingType now (needed unconditionally, regardless of MergeBackends); the merge-only hasRouteLevelClusterSettings lookup moved to its own method, computed lazily inside the isMergeBackendsEnabledForGateway guard alongside the BTPLoadBalancerIndex/ConsistentHash lookup it's parallel to. - mergeIncompatibleForSingleClusterRule is now a method on *Translator (was a free function) and takes backendRefs directly rather than a precomputed count, matching mergeIncompatibleForWeightedRule's shape. - Consistent argument order across mergeIncompatibleForWeightedRule/ ForSingleClusterRule/hasRouteLevelClusterSettings/ weightedRuleBackendsMustBeInOneCluster: gatewayCtx, routeCtx, parentRef, routeRuleName, backendRefs, then the rest. RouteContext params renamed route -> routeCtx throughout this call chain. - Removed backendClusterRefBuilder: its only method was a trivial append with no other logic, so all 5 call sites now append directly to a plain []*ir.BackendClusterRef. - Added toBackendObjectReferences, a small generic helper projecting HTTPBackendRef/GRPCBackendRef/BackendRef slices down to their common BackendObjectReference, replacing duplicated flattening loops at 5 call sites. - Various comment trims (dropped stale/dangling cross-references, redundant "merged" qualifiers now that BackendCluster is inherently merged-only) and structural cleanup (var-block grouping in the HTTP/GRPC preambles, consistent blank-line spacing) carried over from the earlier processHTTPRouteRules pass to processGRPCRouteRules and friends. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
The comment explained why udpRoute.Spec.Rules[0] is safe to index, but sat next to a later, unrelated use of the already-computed routeRuleMetadata value instead of near the Spec.Rules[0] indexing itself. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…lvedMetricSinks Found while running the full lint suite: the loop copied each 136-byte ir.ResolvedMetricSink by value per iteration. Iterate by index and take a pointer instead - unrelated to the MergeBackends work on this branch, but was already failing `make lint` at HEAD. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
|
commenting on #9477 (comment) (cc @arkodg) The two priority items are done, plus one more:
|
… rules' backends A TLSRoute has no per-rule match criteria, so all rules' backendRefs pool into a single Envoy cluster destination. Merge eligibility was previously decided per rule using only that rule's own backendRefs, so two rules each with a single distinct backend both looked safe to merge independently, producing two separate merged clusters where only one could ever be referenced downstream and silently dropping traffic to the other. Compute the distinct backend identities across all of a TLSRoute's rules once per parentRef and treat more than one distinct backend as merge- incompatible for every rule, in a new distinctBackendObjectReferences helper reusable by TCP/UDP if their single-rule restriction is ever relaxed. Also extracts resolveBTPRoutingType out of the now-removed resolveRuleBackendContext so gatewayCtx is resolved once per parentRef instead of once per rule. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
|
Nice! The number of changed files is much smaller now. |
… overrides Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…icy HTTP1 override Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…ge-eligibility fields Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
Per review feedback (envoyproxy#9477 (comment)), the MergeGateways+MergeBackends combination adds complexity around a mode (MergeGateways) that may eventually be deprecated in favor of ListenerSet. Rather than supporting the combination, disallow it via CEL validation (separate commit). Drops irBackendClusterName's mergeGateways-conditional suffixing of a merged cluster's rendered name and the MergeGatewaysBackends e2e conformance test and its testdata/fixtures, which only existed to cover this combination. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…ia CEL Per review feedback on PR envoyproxy#9477, MergeGateways and MergeBackends should never be enabled together - add a CEL rule on EnvoyProxySpec rejecting that combination, and cover it in the CEL validation test suite. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
|
found a bug when investigating some changes for this PR: #9541 |
| metadata: | ||
| namespace: default | ||
| name: httproute-1 | ||
| name: httproute-2 |
| metadata: | ||
| namespace: envoy-gateway | ||
| name: httproute-1 | ||
| name: httproute-6 |
| metadata: | ||
| namespace: default | ||
| name: httproute-3 | ||
| name: httproute-4 |
| metadata: | ||
| namespace: default | ||
| name: httproute-1 | ||
| name: httproute-3 |
Could we address this in a separate PR? That would make the change easier to review and allow us to backport the fix to an earlier release if necessary. We could merge that PR first, then proceed with this one. |
What this PR does / why we need it:
Adds an opt-in
EnvoyProxy.spec.mergeBackendsoption: routes referencing the identical backend can share a single Envoy cluster instead of Envoy Gateway generating one cluster per route rule. This reduces xDS size, active health-check traffic, and stats cardinality on gateways with many routes fanning out to a small set of shared backends. Disabled by default and marked experimental.Cluster sharing is skipped wherever it would be unsafe:
UseClientProtocoloverride, both of which are baked into the shared cluster rather than the per-route destinationApproach: enabling this required restructuring how a route's backend data is stored. Previously, every
RouteDestinationembedded its own copy of the resolvedDestinationSettings, so two routes sharing a backend could never share anything at the IR level - deduplication had nowhere to attach. This PR introduces a per-gateway backend cluster registry (ir.Xds.Backends,ir.BackendCluster): each distinct backend is resolved once, and everyRouteDestinationholds only aBackendClusterRef(a name) into that registry.RouteDestination.Settingsis removed onceBackendClusterRefsis the sole source.Beyond enabling MergeBackends, this registry is what makes a future backend-level BackendTrafficPolicy straightforward: since a backend's settings now live in exactly one place instead of being copied into every route that references it, a policy targeting the backend directly can be applied once at the registry entry, rather than needing to be threaded through and reconciled across every route-level copy.
Reviewer notes: this is a large diff; start here:
api/v1alpha1/envoyproxy_types.go- the new user-facingMergeBackends/MergeBackendsConfigfieldinternal/ir/xds.go- theBackendCluster/BackendClusterReftypes and theXds.Backendsregistryinternal/gatewayapi/route.go-processBackendRefis the single entry point every route type (and the RequestMirror filter) uses to resolve a backendRef's destination and cluster assignment together. It wrapsresolveBackendCluster(decides the cache key/name/merge outcome),shouldMergeBackend(owns the full eligibility decision),mergeIncompatibleForWeightedRule/mergeIncompatibleForSingleClusterRule(the per-rule-level exclusions, split by whether the route type supports weighted-cluster route actions), andgetOrCreateBackendCluster(the find-or-create registry lookup)internal/xds/translator/backend_registry.go- registry resolution (getBackendClusters,singleResolvedClusterName) consumed by the HTTP/GRPC route-action and TCP/UDP listener builders, andcluster.go's cluster-per-setting/weight logicA good portion of the remaining diff is golden testdata:
internal/gatewayapi/testdata/*.out.yamlfixtures re-derived to reflect destinations serializing asbackendClusterRefs+ a top-levelbackends:registry instead of inlinesettings:, plus new fixtures (mergebackends-*ininternal/gatewayapi/testdata,merge-backends-*ininternal/xds/translator/testdata) covering the merge-eligibility rules above at both the IR-building layer and the xDS translation layer - several of the latter are derived directly from the former's real output, tracing a single Gateway API resource all the way through to the generated Envoy config.Which issue(s) this PR fixes:
Fixes #5307