fix(gatewayapi): don't skip MergeBackends precedence-index claims for policies that set nothing - #9590
Open
muwaqar-cflt wants to merge 9 commits into
Open
fix(gatewayapi): don't skip MergeBackends precedence-index claims for policies that set nothing#9590muwaqar-cflt wants to merge 9 commits into
muwaqar-cflt wants to merge 9 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Author
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9590 +/- ##
==========================================
+ Coverage 76.02% 76.04% +0.01%
==========================================
Files 259 260 +1
Lines 43275 43254 -21
==========================================
- Hits 32900 32891 -9
+ Misses 8183 8175 -8
+ Partials 2192 2188 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kkk777-7
previously approved these changes
Jul 29, 2026
Member
|
LGTM, thanks! |
Member
|
/retest |
zhaohuabing
reviewed
Jul 30, 2026
zhaohuabing
reviewed
Jul 30, 2026
zhaohuabing
reviewed
Jul 30, 2026
…ergeType is unset Fixes envoyproxy#9541: a route-rule/route-targeted BackendTrafficPolicy with both RoutingType and MergeType unset incorrectly inherited RoutingType from its resolved gateway/listener parent. MergeType nil means the policy doesn't merge with a parent at all, so it must resolve to its own (nil) value instead. Migrates BTPRoutingTypeIndex onto a new, generic policyIndex[T] primitive that reuses the existing policyScope type (previously used only by SecurityPolicy's override/merge tracking), rather than patching the old ad hoc btpRoutingKey-based maps directly. Each entry carries a single effective bool decided at write time - via isRouteEffective's MergeType-aware rule for route-rule/route scopes, or the caller-supplied hasValue for listener/gateway scopes - so Lookup resolves any scope uniformly. policyScope gains a RouteKind tag (a 1-byte enum, to keep the struct under gocritic's hugeParam threshold) so route-rule/route scopes targeting different route kinds under the same name never collide. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…et fix Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
muwaqar-cflt
force-pushed
the
fix-routingtype-mergetype-9541
branch
from
July 30, 2026 20:21
811b558 to
9e073e9
Compare
…ngs, LoadBalancer, and ClientTrafficPolicy Applies the same "every accepted policy must claim its precedence slot, even one that sets nothing" fix from BTPRoutingTypeIndex to BTPClusterSettingsIndex, BTPLoadBalancerIndex, and the newly-migrated CTPClusterSettingsIndex, closing the same class of bug for envoyproxy#9588. All three indexes are now built on the shared policyIndex[T] primitive instead of ad-hoc raw maps, and the BTP indexes gained ListenerSet support. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
muwaqar-cflt
changed the title
Contributor
|
@codex review |
….go and its test The test file had grown its own copy of route.go's listener-scope-checking loop just to keep compiling after CTPClusterSettingsIndex became a plain policyIndex alias, so the test wasn't actually exercising production code and could silently drift from it. Extract the loop into a package function both call. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
![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: 9e4ccd881f
ℹ️ 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".
…onally The LoadBalancer switch only ran under `if hasLoadBalancer`, so an accepted Gateway-wide BTP that left LoadBalancer unset skipped claiming its slot entirely - letting a younger conflicting BTP that does set ConsistentHash win it. Same "must claim slot unconditionally" fix already applied to RoutingType and ClusterSettings. Found via automated PR review on envoyproxy#9590. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
Contributor
Author
|
A note on scope: this PR originally targeted So the scope here is intentionally wider than the original description, now covering #9541 and #9588. For 1.8.4 (which doesn't have MergeBackends' ClusterSettings/LoadBalancer/CTP indexes at all), a separate, isolated fix scoped to |
…aliasing Go won't allow new methods on a type alias to an instantiated generic (the reason these indexes were reduced to plain policyIndex[T] aliases), but a struct that anonymously embeds *policyIndex[T] is a distinct named type, so it can have its own methods. This restores BTPRoutingTypeIndex.LookupBTPRoutingType/LookupGatewayBTRoutingType, BTPClusterSettingsIndex.HasRouteLevelClusterSettings, BTPLoadBalancerIndex.IsConsistentHash, and CTPClusterSettingsIndex.HasListenerLevelClusterSettings - matching main's PR #4 design - while keeping call sites free of any .index field indirection: because the embed is anonymous, methods forward via promotion (idx.Lookup(...), not idx.index.Lookup(...)). Each dedicated method checks for a nil receiver before touching the embedded field, so nil-safety doesn't regress. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…omments - Port TestCTPClusterSettingsIndex's 2 missing oldest-accepted-blocks- younger regression cases (Gateway-listener and ListenerSet-wide level), matching the same coverage already ported for BTP indexes. - Port the missing mergebackends-http-demerge-route-replaces-gateway- cluster-settings golden fixture pair, verified byte-identical to PR #4's own validated output (aside from the mergeBackends schema drift already fixed in the other 3 ported fixtures). - Strip every test-case comment added by this PR (per review feedback); comments already present before this PR are left untouched. - Merge TestPolicyIndexLookupListenerSet into TestPolicyIndexLookup for consistency with TestPolicyIndexLookupExact's single-table style, and settle on a consistent useRuleName/useListener/useListenerSet naming convention for the three optional Lookup arguments. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
zhaohuabing
reviewed
Jul 30, 2026
| name: "" | ||
| prefix: /a | ||
| traffic: | ||
| faultInjection: |
Member
|
/retest |
zhaohuabing
reviewed
Jul 31, 2026
… via hasRouteLevelClusterSettings The route/rule-level BackendTrafficPolicy conflict this TODO described is already excluded from cluster merging by hasRouteLevelClusterSettings's pinned-slot tracking, so it never reaches x.BackendClusters here. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
zhaohuabing
reviewed
Jul 31, 2026
…dence fix MergeBackends itself hasn't shipped in a release yet, so a separate bug-fix note in the same release cycle as its own new-feature note is redundant and confusing. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
zhaohuabing
reviewed
Jul 31, 2026
| if idx == nil { | ||
| return false | ||
| } | ||
| value, pinned := idx.Lookup(routeKind, routeNN, gatewayNN, listenerName, nil, routeRuleName) |
Member
There was a problem hiding this comment.
Nit, for clarity and maintainability, I'd prefer longer but more descriptive names here.
Suggested change
| value, pinned := idx.Lookup(routeKind, routeNN, gatewayNN, listenerName, nil, routeRuleName) | |
| hasExplicitClusterSettings, replacesParent := idx.lookup(routeKind, routeNN, gatewayNN, listenerName, nil, routeRuleName) |
zhaohuabing
reviewed
Jul 31, 2026
| listenerName *gwapiv1.SectionName, | ||
| listenerSetNN *types.NamespacedName, | ||
| routeRuleName *gwapiv1.SectionName, | ||
| ) (value T, pinned bool) { |
Member
There was a problem hiding this comment.
Prefer a more descriptive name here:
Suggested change
| ) (value T, pinned bool) { | |
| ) (value T, replacesParent bool) { |
zhaohuabing
reviewed
Jul 31, 2026
| if idx == nil { | ||
| return false | ||
| } | ||
| value, pinned := idx.Lookup(routeKind, routeNN, gatewayNN, listenerName, nil, routeRuleName) |
Member
There was a problem hiding this comment.
Lookup → resolveEntry → parentLevels eventually checks gatewayScope(gatewayNN). This is currently harmless because BuildBTPIndexes does not add gateway-scoped entries to this index. However, this constraint is not apparent from the Lookup path itself and could confuse future readers. For maintainability, could we make it explicit in the lookup logic?
The below fix is suggested by Claude code, for reference only:
type scopeDepth uint8
const (
throughGatewayScope scopeDepth = iota
belowGatewayScope
)
func newPolicyIndex[T comparable](depth scopeDepth) *policyIndex[T] {
return &policyIndex[T]{entries: make(map[policyScope]policyIndexEntry[T]), depth: depth}
}
func (idx *policyIndex[T]) parentLevels(gatewayNN types.NamespacedName, listenerName *gwapiv1.SectionName, listenerSetNN *types.NamespacedName) T {
var zero T
if idx == nil {
return zero
}
if listenerSetNN != nil {
if listenerName != nil {
if value, found := idx.LookupExact(listenerSetListenerScope(*listenerSetNN, *listenerName)); found {
return value
}
}
if value, found := idx.LookupExact(listenerSetScope(*listenerSetNN)); found {
return value
}
} else if listenerName != nil {
if value, found := idx.LookupExact(gatewayListenerScope(gatewayNN, *listenerName)); found {
return value
}
}
if idx.depth == belowGatewayScope {
return zero
}
value, _ := idx.LookupExact(gatewayScope(gatewayNN))
return value
}
parentLevels returns before its final gateway step when depth is belowGatewayScox is constructed with it, BTPRoutingTypeIndex keeps the full walk.
kkk777-7
reviewed
Jul 31, 2026
| } | ||
|
|
||
| // setListenerSetLevel is setGatewayLevel's counterpart for a ListenerSet. | ||
| func (idx *policyIndex[T]) setListenerSetLevel(ls types.NamespacedName, value T) { |
Member
There was a problem hiding this comment.
Should we also have hasValue here to determine whether we should fall back to the Gateway policy?
zhaohuabing
reviewed
Aug 1, 2026
Comment on lines
+114
to
+120
| // HasRouteLevelClusterSettings reports whether a route-rule, route, or listener-level | ||
| // BackendTrafficPolicy contributes backend-cluster-scoped settings for the given target, or | ||
| // targets it with MergeType unset. A gateway-level setting isn't checked: it applies uniformly to | ||
| // every route sharing a merged cluster, so it can't cause a divergence. ListenerSet-level BTP | ||
| // attachment isn't tracked here yet (envoyproxy/gateway#9619), so listenerSetNN is always resolved | ||
| // as unset. | ||
| func (idx *BTPClusterSettingsIndex) HasRouteLevelClusterSettings( |
Member
There was a problem hiding this comment.
The name HasRouteLevleCLusterSettings is not percise, it covers two cases:
- this rule's backendRefs have explicit Cluster Settings below Gateway, including routeRule, route, and listener
- this rule's backendRefs don't have explicit Cluster Settings below Gateway，but replace Gateway-level cluser settings with nil
Suggested change
| // HasRouteLevelClusterSettings reports whether a route-rule, route, or listener-level | |
| // BackendTrafficPolicy contributes backend-cluster-scoped settings for the given target, or | |
| // targets it with MergeType unset. A gateway-level setting isn't checked: it applies uniformly to | |
| // every route sharing a merged cluster, so it can't cause a divergence. ListenerSet-level BTP | |
| // attachment isn't tracked here yet (envoyproxy/gateway#9619), so listenerSetNN is always resolved | |
| // as unset. | |
| func (idx *BTPClusterSettingsIndex) HasRouteLevelClusterSettings( | |
| // ClusterSettingsDefinedBelowGateway reports whether the backend-cluster-scoped settings applying | |
| // to the given target are defined below Gateway scope: either a route-rule, route, or | |
| // listener-level BackendTrafficPolicy contributes them, or a route-rule/route-level one targets it | |
| // with MergeType unset, which replaces the Gateway's settings instead of merging with them - even | |
| // when that policy sets none of its own, leaving the target with empty settings rather than the | |
| // Gateway's. A gateway-level setting is never consulted - the index resolves belowGatewayScope, so | |
| // the walk stops short of it: a Gateway-wide setting applies uniformly to every route sharing a | |
| // merged cluster, so it can never be the reason two of them diverge. ListenerSet-level BTP | |
| // attachment isn't tracked here yet (envoyproxy/gateway#9619), so listenerSetNN is always resolved | |
| // as unset. | |
| func (idx *BTPClusterSettingsIndex) ClusterSettingsDefinedBelowGateway( |
zhaohuabing
reviewed
Aug 1, 2026
Comment on lines
+530
to
534
| // hasListenerLevelClusterSettings reports whether any of the route rule's attached listeners | ||
| // (parentRef.listeners - the route's actual resolved attachment(s) under gatewayCtx's gateway) has | ||
| // a ClientTrafficPolicy-sourced HTTP1 override, checking each listener against its own owner (the | ||
| // Gateway, or the ListenerSet it came from). | ||
| func (t *Translator) hasListenerLevelClusterSettings(gatewayCtx *GatewayContext, parentRef *RouteParentContext) bool { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
zhaohuabing
reviewed
Aug 1, 2026
| } | ||
| // Delegate to the remaining multi-backendRef checks. | ||
| return t.weightedRuleBackendsMustBeInOneCluster(routeCtx, backendRefs, sessionPersistent, gatewayCtx) | ||
| } |
Member
There was a problem hiding this comment.
Having hasRouteLevelClusterSettings(a better name should be clusterSettingsDefinedBelowGateway) and hasListenerLevelClusterSettings called side by side reads oddly now, since "hasRouteLevelClusterSettings" already includes listener scope — the real difference is BTP vs CTP, not the scope.
Suggest folding the CTP lookup in hasRouteLevelClusterSettings, rename it to clusterSettingsDefinedBelowGateway, and dropping the second wrapper:
// clusterSettingsDefinedBelowGateway reports whether the cluster-scoped settings applying to this
// rule are defined below Gateway scope, in any of three ways:
//
// - explicit setting: a BackendTrafficPolicy at route-rule/route/listener scope sets a
// cluster-scoped field.
// - empty replacement: a BackendTrafficPolicy at route-rule/route scope sets no cluster-scoped
// field at all, but leaves MergeType unset, so it replaces the Gateway's settings rather than
// merging with them - leaving the rule with its own empty set.
// - client traffic: a ClientTrafficPolicy at listener scope sets an HTTP1 override.
//
// All three make cluster deduplication unsafe, though not for the same reason: the BTP settings
// would wrongly apply to the other routes sharing the cluster, while the CTP ones would be lost
// entirely, since a merged BackendCluster carries no HTTP1 settings.
//
// The CTP case checks parentRef.listeners - the route's actual resolved attachment(s) under
// gatewayCtx's gateway - against each listener's own owner (the Gateway, or the ListenerSet it
// came from).
func (t *Translator) clusterSettingsDefinedBelowGateway(
gatewayCtx *GatewayContext,
routeCtx RouteContext,
parentRef *RouteParentContext,
routeRuleName *gwapiv1.SectionName,
) bool {
if gatewayCtx == nil {
return false
}
gatewayNN := types.NamespacedName{Namespace: gatewayCtx.GetNamespace(), Name: gatewayCtx.GetName()}
if t.BTPClusterSettingsIndex.ClusterSettingsDefinedBelowGateway(
routeCtx.GetRouteType(),
types.NamespacedName{Namespace: routeCtx.GetNamespace(), Name: routeCtx.GetName()},
gatewayNN,
parentRef.SectionName,
routeRuleName,
) {
return true
}
return t.CTPClusterSettingsIndex.HasListenerLevelClusterSettings(gatewayNN, parentRef.listeners)
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
MergeBackends' precedence-resolution indexes for
BackendTrafficPolicy(RoutingType, cluster-scoped settings eligibility,LoadBalancerConsistentHash) andClientTrafficPolicy(cluster-scoped settings eligibility) all shared the same bug: population logic skipped any accepted policy that set nothing at its target, regardless ofmergeType. For a route-rule/route-targeted BTP withmergeTypealso unset (meaning it doesn't merge with any parent at all), this let the lookup silently fall through to a broader listener/gateway policy's settings instead of resolving to the rule's own (empty) default - or let a skipped policy's target slot be silently claimed by a younger, conflicting policy instead.This migrates all of
BTPRoutingTypeIndex,BTPClusterSettingsIndex,BTPLoadBalancerIndex, and the newly-introducedCTPClusterSettingsIndexonto a shared, minimalpolicyIndex[T]primitive built on the existingpolicyScopetype (already used bySecurityPolicy's override/merge tracking), rather than patching the old ad hoc raw-map-based indexes directly. Every accepted policy now claims its precedence slot unconditionally (even when it sets nothing), preserving the existing "oldest accepted policy wins over a younger conflicting one" guarantee - dedicated tests cover this at route-rule/route/listener/gateway/ListenerSet level for each index.policyScopegains aRouteKindtag (a 1-byte enum, to keep the struct undergocritic'shugeParamthreshold) so route-rule/route scopes targeting different route kinds under the same name never collide. The BTP indexes also gained ListenerSet-attach support (Gateway/Listener/ListenerSet/ListenerSet-listener levels), extending #9419/#9425's ListenerSet-for-BTP work to this precedence layer.Out of scope: #9619 (ClusterSettings/LoadBalancer indexes don't yet track ListenerSet-level BTP attachment the way
RoutingTypenow does) is a related but separate gap, left for a follow-up.Which issue(s) this PR fixes:
Fixes #9541
Fixes #9588
PR Checklist
git commit -s)./api.go build ./...,go vet ./...,gofmt -l,make lint.golint, andgo test ./...all pass.make gen-checkshows no drift beyond this PR's own changes.backendtrafficpolicy_test.go,clienttrafficpolicy_test.go, and a newpolicyindex_test.gocovering the shared primitive directly (including oldest-accepted-vs-younger-conflicting-policy cases at every level), plus three new golden translator fixtures.release-notes/current/bug_fixes/9541-9588-mergebackends-precedence-index-early-exit.md.BTPRoutingTypeIndex,BTPClusterSettingsIndex,BTPLoadBalancerIndex,CTPClusterSettingsIndex); no breaking changes.