feat: add MergeBackends option for backend cluster deduplication - #9477
feat: add MergeBackends option for backend cluster deduplication#9477muwaqar-cflt wants to merge 12 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.75% 76.03% +0.28%
==========================================
Files 254 259 +5
Lines 42322 43275 +953
==========================================
+ Hits 32059 32906 +847
- Misses 8107 8177 +70
- Partials 2156 2192 +36 ☔ 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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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
There was a problem hiding this comment.
our existing APIs already follows the pattern mentioned
- also
http3 - TLS session resumption similarly uses empty
stateless: {}/stateful: {}
an explicitenabledis not needed here
There was a problem hiding this comment.
Looking into dropping Enabled entirely (so a present, even empty mergeBackends: {} means enabled, absent means disabled) rather than defaulting it to true later.
One tradeoff to flag before I make the change: today Enabled: false at a more-specific level (a Gateway-level EnvoyProxy override, or GatewayClass-level vs. the config-wide default) lets you explicitly opt back out of an ancestor level's true. Two existing tests exercise exactly that (TestShouldMergeBackend's "enabled globally, but Gateway-level EnvoyProxy disables it" and TestIsMergeBackendsEnabled's "gatewayclass envoyproxy takes precedence over default spec" disable case). Removing Enabled removes that capability too — once presence alone means "on," there's no way to represent "explicitly off" at a level that would otherwise inherit "on" from below.
There was a problem hiding this comment.
mergeBackends: null in the child could solve this ?
There was a problem hiding this comment.
null value is indistinguishable from not being set at all. a quick AI lookup says there may be some ways to make it work +kubebuilder:validation:Nullable but I do not have much knowledge there.
either way I am sensing strong opposition to Enabled right now, and potentially add the relevant capability (disablement on a granular scope overriding the broader one) later if a need arises. so I will make changes to remove Enabled.
|
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>
There was a problem hiding this comment.
LGTM. Thanks for your patience and the great work!
Could we also fix the follow-up issue, #9588, in v1.9.0—or v1.9.1 at the latest? Without that fix, this feature won’t work correctly with policy overrides.
|
/retest |
|
/retest |
|
@muwaqar-cflt need rebase to fix doc-lint. |
…th mergeGateways MergeBackends configures backend cluster deduplication: routes referencing the same backend can share a single Envoy cluster instead of getting one per route. Disabled by default. mergeGateways and mergeBackends cannot both be enabled on the same EnvoyProxy spec (CEL-enforced) - supporting that combination added complexity around a mode that may eventually be deprecated in favor of ListenerSet. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
BackendCluster is a shared, identity-deduplicated backend: exactly one backend identity, one setting. Xds.BackendClusters holds every distinct merged BackendCluster for a gateway - the single source of truth for a cluster's Settings/Metadata/Traffic. RouteDestination.BackendClusterRefs holds references into that registry by name, alongside the existing Settings field for non-merged backends. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…bility rules
Introduces a per-gateway backend cluster registry (BackendClusterMap,
Xds.BackendClusters): each distinct, merge-eligible backend is resolved
once via processBackendRef/getOrCreateBackendCluster, and its
RouteDestination holds a BackendClusterRef into that registry instead of
its own settings. A single rule's backendRefs can be a mix of both - some
merge-eligible, others not - merge eligibility is decided per-backendRef,
not per-rule.
shouldMergeBackend owns the eligibility decision, gated on (in order): the
gateway's effective MergeBackends setting, the backend's kind (custom and
dynamic-resolver backends never merge), per-backendRef filters, and a set
of rule/route/listener-level indexes built once per Translate() call:
- BTPClusterSettingsIndex / CTPClusterSettingsIndex: whether a
route-rule/route/listener-level BackendTrafficPolicy or
ClientTrafficPolicy sets a cluster-scoped field (circuit breaker,
health check, load balancer, HTTP1 settings, etc.) that would
incorrectly apply to every route sharing a merged cluster if merged.
ClientTrafficPolicy targets are resolved against both Gateway and
ListenerSet objects; gateway-wide/listenerSet-wide settings that apply
uniformly across everything they cover are exempted, since a merged
cluster can never see them diverge.
- BTPRoutingTypeIndex / BTPLoadBalancerIndex: RoutingType divergence
from the gateway's baseline, and ConsistentHash load balancing on a
multi-backendRef rule, both of which require the full backend pool
kept together rather than split per identity.
- mergeIncompatibleForWeightedRule / mergeIncompatibleForSingleClusterRule:
session persistence, a fallback backend, and (for TCP/UDP/TLS, which
have no weighted-clusters mechanism) any rule with more than one
backendRef.
Gateway-level BackendTrafficPolicy Traffic features (circuit breaker,
health check, timeout, UseClientProtocol, etc.) are applied to every
merged BackendCluster once accepted, since gateway level is the only one
guaranteed uniform across a merged cluster's routes.
Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…/SDS building processMergedBackendClusters builds the Envoy Cluster and client-cert secrets for each distinct merged backend cluster in xdsIR.BackendClusters, once. buildXdsRouteAction/buildXdsWeightedRouteAction resolve a route's BackendClusterRefs against a name-keyed backendClusterIndex alongside its own Settings, so a single weighted rule can route across a mix of merged and non-merged clusters. processSDSClusters scans xdsIR.BackendClusters directly for SDS URLs rather than re-resolving each merged cluster's settings once per referencing route. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
Two routes referencing the same backend both route correctly, and Envoy generates a single deduplicated Cluster for that backend instead of one per route; a third route with its own rule-level BackendTrafficPolicy setting a cluster-scoped field keeps its own separate Cluster, proving the demerge direction as well as the merge direction. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…lvedMetricSinks Unrelated drive-by fix: iterate by index and take the address instead of copying each ResolvedMetricSink by value on every loop iteration. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…envoyproxy#9184/envoyproxy#9030 SDS changes main's new TLSRoute dynamic-resolver support referenced a destSettings variable that doesn't exist on this branch, which renamed it to allDs/backendClusterRefs earlier in this branch's own history. git's 3-way merge combined the two without a textual conflict, silently producing code that failed to compile. Adapt the check to use allDs (a dynamic resolver is never merge-eligible, so it can only appear there) while counting both allDs and backendClusterRefs for the single-destination requirement. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…unset route policies envoyproxy#9588: a route/rule-targeted BackendTrafficPolicy with mergeType unset that sets no cluster-scoped field of its own still incorrectly inherits a shared cluster's gateway-level Traffic, since merge-eligibility here only checks for an explicit cluster-scoped field, not whether the policy merges with a parent at all. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…ckends Mirrors the existing MergeGateways *bool precedent: mergeBackends being set at all (even empty) now enables cluster deduplication, so there is no separate on/off toggle to get out of sync with field presence. This also simplifies the mergeGateways/mergeBackends mutual-exclusion CEL rule to a presence check. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
8a29608 to
2cc4d16
Compare
arkodg
left a comment
arkodg
left a comment
There was a problem hiding this comment.
LGTM thanks, this feature should help increase adoption for users who were previously concerned with xDS cluster duplication
|
@muwaqar-cflt let's merge instead of rebase when resolving conflicts - it's easier to review after the conflicts are fixed. Thanks! |
# Conflicts: # internal/gatewayapi/backendtrafficpolicy.go # internal/gatewayapi/backendtrafficpolicy_test.go # internal/gatewayapi/route.go # internal/gatewayapi/runner/runner.go # internal/gatewayapi/translator.go
EnvoyGatewayKubernetesInfrastructureConfiguration's introduction (envoyproxy#9027) left one composite literal missing its closing brace, breaking go vet/go test for this package. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…nationToProto Our BackendClusterRefs field addition to ir.RouteDestination pushed the struct to 80 bytes, tripping gocritic's hugeParam check on this by-value param inherited from main's remote-infrastructure-provider work. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
|
Hi @muwaqar-cflt Do we need to handle |
ListenerSet feature came in from main pretty late. its too late to handle it for now since we want this merged for 1.9 but I can look into it when fixing #9588 |
5 participants
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 destinationmergeGatewaysandmergeBackendscannot both be enabled (CEL-enforced) — supporting that combination added complexity around a mode that may eventually be deprecated in favor of ListenerSet.Gateway-level
BackendTrafficPolicyTraffic features (circuit breaker, health check, timeout, etc.) now apply correctly to mergedBackendClusters too — previously they were silently dropped for any merged cluster.Approach: 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.BackendClusters,ir.BackendCluster): each distinct, merge-eligible backend is resolved once, and itsRouteDestinationholds aBackendClusterRef(a name) into that registry instead of its own settings. A single rule's backendRefs can be a mix of both — some merge-eligible (viaBackendClusterRefs), others not (kept inSettings) — merge eligibility is decided per-backendRef, not per-rule.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, theXds.BackendClustersregistry, andBackendCluster.Trafficinternal/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 wrapsshouldMergeBackend/mergeIncompatibleForWeightedRule/mergeIncompatibleForSingleClusterRule(own the full eligibility decision, split by whether the route type supports weighted-cluster route actions),backendClusterKey/backendClusterKeyForGateway(build the registry cache key), andgetOrCreateBackendCluster(the find-or-create registry lookup)internal/gatewayapi/backendtrafficpolicy.go/clienttrafficpolicy.go-BuildBTPIndexes/BuildCTPIndexes(build the per-target eligibility indexes used by the checks above) andtranslateBackendTrafficPolicyForGateway(populatesBackendCluster.Trafficfrom the accepted whole-gateway policy)internal/xds/translator/backend_registry.go- registry resolution (resolveBackendClusters,routeDestinationSettings,needsRouteCluster) consumed by the HTTP/GRPC route-action and TCP/UDP listener builders, andcluster.go's cluster-per-setting/weight logic andBackendClusterTranslatorA good portion of the remaining diff is golden testdata:
internal/gatewayapi/testdata/*.out.yamlfixtures re-derived to reflect destinations serializing asbackendClusterRefs+ a top-levelbackendClusters: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.Known, accepted limitations (discussed in review, tracked separately):
hostname— merging never applies a route's own hostname to a shared cluster, since it isn't necessarily invariant across the routes sharing it.EnvoyProxy.spec.telemetry.metrics.clusterStatName(alt_stat_name) is dropped for merged clusters rather than applied, since it's a single per-route pattern with no way to express merged-cluster behavior. Tracked in Merging backends drops alt_stat_name (EnvoyProxy clusterStatName) for merged clusters #9570.mergeTypeunset is not currently treated as a merge-disqualifying signal by itself — only an explicit cluster-scoped field is checked. Since an unsetmergeTypediscards its resolved parent's settings entirely, a sibling route inheriting from that same parent can end up with different resolved Traffic settings than this one, even though neither route's own policy sets a cluster-scoped field directly. Fix is in progress on a separate branch, staged behind BackendTrafficPolicy RoutingType ignores MergeType, always inherits from broader-scoped policies #9541.Which issue(s) this PR fixes:
Fixes #5307, #9541