Skip to content

Commit 9e09d12

Browse files
committed
gatewayapi: fix BTPRoutingTypeIndex to not inherit RoutingType when MergeType is unset
Fixes #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>
1 parent 0e77573 commit 9e09d12

12 files changed

Lines changed: 974 additions & 313 deletions

internal/gatewayapi/backendtrafficpolicy.go

Lines changed: 44 additions & 272 deletions
Original file line numberDiff line numberDiff line change
@@ -39,135 +39,9 @@ const (
3939
ResponseBodyConfigMapKey = "response.body"
4040
)
4141

42-
// btpRoutingKey identifies a BTP routing type target
43-
type btpRoutingKey struct {
44-
Kind, Namespace, Name, SectionName string
45-
}
46-
47-
// BTPRoutingTypeIndex holds RoutingType values from BackendTrafficPolicies
48-
// This avoids an O(BTPs) lookup for every iteration of processDestination.
49-
type BTPRoutingTypeIndex struct {
50-
routeRuleLevel map[btpRoutingKey]*egv1a1.RoutingType
51-
routeLevel map[btpRoutingKey]*egv1a1.RoutingType
52-
listenerSetListenerLevel map[btpRoutingKey]*egv1a1.RoutingType
53-
listenerSetLevel map[btpRoutingKey]*egv1a1.RoutingType
54-
listenerLevel map[btpRoutingKey]*egv1a1.RoutingType
55-
gatewayLevel map[btpRoutingKey]*egv1a1.RoutingType
56-
}
57-
58-
// btpRoutingTypeIndexMaps allocates BTPRoutingTypeIndex's maps.
59-
func btpRoutingTypeIndexMaps() *BTPRoutingTypeIndex {
60-
return &BTPRoutingTypeIndex{
61-
routeRuleLevel: make(map[btpRoutingKey]*egv1a1.RoutingType),
62-
routeLevel: make(map[btpRoutingKey]*egv1a1.RoutingType),
63-
listenerSetListenerLevel: make(map[btpRoutingKey]*egv1a1.RoutingType),
64-
listenerSetLevel: make(map[btpRoutingKey]*egv1a1.RoutingType),
65-
listenerLevel: make(map[btpRoutingKey]*egv1a1.RoutingType),
66-
gatewayLevel: make(map[btpRoutingKey]*egv1a1.RoutingType),
67-
}
68-
}
69-
70-
// LookupBTPRoutingType resolves the RoutingType for a specific route rule
71-
// and gateway/listener combination by checking the index in
72-
// priority order: routeRule > route > listener > gateway.
73-
// Returns nil if no matching BTP RoutingType is found, or if the index is nil.
74-
func (idx *BTPRoutingTypeIndex) LookupBTPRoutingType(
75-
routeKind gwapiv1.Kind,
76-
routeNN types.NamespacedName,
77-
gatewayNN types.NamespacedName,
78-
listenerName *gwapiv1.SectionName,
79-
listenerSetNN *types.NamespacedName,
80-
routeRuleName *gwapiv1.SectionName,
81-
) *egv1a1.RoutingType {
82-
if idx == nil {
83-
return nil
84-
}
85-
86-
// 1. Route-rule level (most specific)
87-
if routeRuleName != nil {
88-
key := btpRoutingKey{
89-
Kind: string(routeKind),
90-
Namespace: routeNN.Namespace,
91-
Name: routeNN.Name,
92-
SectionName: string(*routeRuleName),
93-
}
94-
if rt, ok := idx.routeRuleLevel[key]; ok {
95-
return rt
96-
}
97-
}
98-
99-
// 2. Route level
100-
routeKey := btpRoutingKey{
101-
Kind: string(routeKind),
102-
Namespace: routeNN.Namespace,
103-
Name: routeNN.Name,
104-
}
105-
if rt, ok := idx.routeLevel[routeKey]; ok {
106-
return rt
107-
}
108-
109-
// 3. ListenerSet listener level, then ListenerSet level for routes attached through a ListenerSet.
110-
if listenerSetNN != nil {
111-
if listenerName != nil {
112-
listenerSetListenerKey := btpRoutingKey{
113-
Kind: resource.KindListenerSet,
114-
Namespace: listenerSetNN.Namespace,
115-
Name: listenerSetNN.Name,
116-
SectionName: string(*listenerName),
117-
}
118-
if rt, ok := idx.listenerSetListenerLevel[listenerSetListenerKey]; ok {
119-
return rt
120-
}
121-
}
122-
123-
listenerSetKey := btpRoutingKey{
124-
Kind: resource.KindListenerSet,
125-
Namespace: listenerSetNN.Namespace,
126-
Name: listenerSetNN.Name,
127-
}
128-
if rt, ok := idx.listenerSetLevel[listenerSetKey]; ok {
129-
return rt
130-
}
131-
}
132-
133-
// 4. Gateway listener level. ListenerSet-attached routes intentionally skip
134-
// Gateway listener policy lookup because Gateway listeners and ListenerSet
135-
// listeners are sibling scopes.
136-
if listenerSetNN == nil && listenerName != nil {
137-
listenerKey := btpRoutingKey{
138-
Kind: resource.KindGateway,
139-
Namespace: gatewayNN.Namespace,
140-
Name: gatewayNN.Name,
141-
SectionName: string(*listenerName),
142-
}
143-
if rt, ok := idx.listenerLevel[listenerKey]; ok {
144-
return rt
145-
}
146-
}
147-
148-
// 5. Gateway level (least specific)
149-
return idx.LookupGatewayBTRoutingType(gatewayNN)
150-
}
151-
152-
// LookupGatewayBTRoutingType resolves the RoutingType from a gateway-level BTP only, ignoring any
153-
// listener/route/route-rule level override. Returns nil if no matching BTP RoutingType is found,
154-
// or if the index is nil.
155-
func (idx *BTPRoutingTypeIndex) LookupGatewayBTRoutingType(gatewayNN types.NamespacedName) *egv1a1.RoutingType {
156-
if idx == nil {
157-
return nil
158-
}
159-
160-
gwKey := btpRoutingKey{
161-
Kind: resource.KindGateway,
162-
Namespace: gatewayNN.Namespace,
163-
Name: gatewayNN.Name,
164-
}
165-
if rt, ok := idx.gatewayLevel[gwKey]; ok {
166-
return rt
167-
}
168-
169-
return nil
170-
}
42+
// BTPRoutingTypeIndex holds RoutingType values from BackendTrafficPolicies, keyed by attachment
43+
// scope. This avoids an O(BTPs) lookup for every iteration of processDestination.
44+
type BTPRoutingTypeIndex = policyIndex[*egv1a1.RoutingType]
17145

17246
// btpSpecHasClusterScopedFields reports whether spec sets any backend-cluster-scoped (CDS) field —
17347
// either directly inside its embedded ClusterSettings, or via a sibling field on the spec that also
@@ -190,34 +64,12 @@ func btpSpecHasClusterScopedFields(spec *egv1a1.BackendTrafficPolicySpec) bool {
19064
}
19165

19266
// BTPClusterSettingsIndex holds, per route-rule/route/listener target, whether a
193-
// BackendTrafficPolicy contributes backend-cluster-scoped (CDS) settings.
194-
type BTPClusterSettingsIndex struct {
195-
routeRuleLevel map[btpRoutingKey]bool
196-
routeLevel map[btpRoutingKey]bool
197-
listenerLevel map[btpRoutingKey]bool
198-
}
199-
200-
// btpClusterSettingsIndexMaps allocates BTPClusterSettingsIndex's maps.
201-
func btpClusterSettingsIndexMaps() *BTPClusterSettingsIndex {
202-
return &BTPClusterSettingsIndex{
203-
routeRuleLevel: make(map[btpRoutingKey]bool),
204-
routeLevel: make(map[btpRoutingKey]bool),
205-
listenerLevel: make(map[btpRoutingKey]bool),
206-
}
207-
}
67+
// BackendTrafficPolicy sets a cluster-scoped field, or has MergeType unset.
68+
type BTPClusterSettingsIndex = policyIndex[bool]
20869

20970
// BTPLoadBalancerIndex reports, per gateway, whether a BackendTrafficPolicy attached to it sets
21071
// LoadBalancer to ConsistentHash.
211-
type BTPLoadBalancerIndex struct {
212-
gatewayLevel map[types.NamespacedName]bool
213-
}
214-
215-
// btpLoadBalancerIndexMaps allocates BTPLoadBalancerIndex's maps.
216-
func btpLoadBalancerIndexMaps() *BTPLoadBalancerIndex {
217-
return &BTPLoadBalancerIndex{
218-
gatewayLevel: make(map[types.NamespacedName]bool),
219-
}
220-
}
72+
type BTPLoadBalancerIndex = policyIndex[bool]
22173

22274
// BTPIndexes groups the three pre-computed BackendTrafficPolicy indexes BuildBTPIndexes builds
22375
// together in one pass over btps.
@@ -237,9 +89,9 @@ func BuildBTPIndexes(
23789
namespaceLookup func(string) *corev1.Namespace,
23890
mergeBackendsEnabled bool,
23991
) *BTPIndexes {
240-
routingTypeIdx := btpRoutingTypeIndexMaps()
241-
clusterSettingsIdx := btpClusterSettingsIndexMaps()
242-
loadBalancerIdx := btpLoadBalancerIndexMaps()
92+
routingTypeIdx := newPolicyIndex[*egv1a1.RoutingType]()
93+
clusterSettingsIdx := newPolicyIndex[bool]()
94+
loadBalancerIdx := newPolicyIndex[bool]()
24395

24496
allTargets := make([]client.Object, 0, len(routes)+len(gateways)+len(listenerSets))
24597
allTargets = append(allTargets, routes...)
@@ -252,15 +104,13 @@ func BuildBTPIndexes(
252104

253105
for _, btp := range btps {
254106
hasRoutingType := btp.Spec.RoutingType != nil
255-
// ClusterSettings/LoadBalancer only inform merge-eligibility, so they're moot when no
256-
// accepted gateway can enable merging; RoutingType applies regardless of MergeBackends.
257-
hasClusterScoped := mergeBackendsEnabled && btpSpecHasClusterScopedFields(&btp.Spec)
258-
hasLoadBalancer := mergeBackendsEnabled && btp.Spec.LoadBalancer != nil
259-
260-
if !hasRoutingType && !hasClusterScoped && !hasLoadBalancer {
261-
continue
262-
}
107+
hasClusterScoped := btpSpecHasClusterScopedFields(&btp.Spec)
108+
hasLoadBalancer := btp.Spec.LoadBalancer != nil
263109

110+
// Unlike ClusterSettings/LoadBalancer, RoutingType can never be skipped here: every
111+
// accepted BTP must claim its target's first-write-wins slot, even one that sets nothing
112+
// at all, so a younger conflicting policy can't silently win, and so a route/rule-level
113+
// policy with MergeType unset can still pin its scope to nil instead of inheriting.
264114
refs := resolvePolicyTargets(
265115
btp.Spec.PolicyTargetReferences,
266116
allTargets,
@@ -273,66 +123,47 @@ func BuildBTPIndexes(
273123

274124
for _, ref := range refs {
275125
kind := string(ref.Kind)
276-
key := btpRoutingKey{
277-
Kind: kind,
278-
Namespace: string(ref.Namespace),
279-
Name: string(ref.Name),
280-
SectionName: string(ptr.Deref(ref.SectionName, "")),
281-
}
126+
nn := types.NamespacedName{Namespace: string(ref.Namespace), Name: string(ref.Name)}
282127

283-
if hasRoutingType {
284-
switch {
285-
case kind == resource.KindGateway && ref.SectionName != nil:
286-
if _, exists := routingTypeIdx.listenerLevel[key]; !exists {
287-
routingTypeIdx.listenerLevel[key] = btp.Spec.RoutingType
288-
}
289-
case kind == resource.KindGateway:
290-
if _, exists := routingTypeIdx.gatewayLevel[key]; !exists {
291-
routingTypeIdx.gatewayLevel[key] = btp.Spec.RoutingType
292-
}
293-
case kind == resource.KindListenerSet && ref.SectionName != nil:
294-
if _, exists := routingTypeIdx.listenerSetListenerLevel[key]; !exists {
295-
routingTypeIdx.listenerSetListenerLevel[key] = btp.Spec.RoutingType
296-
}
297-
case kind == resource.KindListenerSet:
298-
if _, exists := routingTypeIdx.listenerSetLevel[key]; !exists {
299-
routingTypeIdx.listenerSetLevel[key] = btp.Spec.RoutingType
300-
}
301-
case ref.SectionName != nil:
302-
if _, exists := routingTypeIdx.routeRuleLevel[key]; !exists {
303-
routingTypeIdx.routeRuleLevel[key] = btp.Spec.RoutingType
304-
}
305-
default:
306-
if _, exists := routingTypeIdx.routeLevel[key]; !exists {
307-
routingTypeIdx.routeLevel[key] = btp.Spec.RoutingType
308-
}
309-
}
128+
switch {
129+
case kind == resource.KindGateway && ref.SectionName != nil:
130+
routingTypeIdx.setGatewayListenerLevel(nn, *ref.SectionName, btp.Spec.RoutingType, hasRoutingType)
131+
case kind == resource.KindGateway:
132+
routingTypeIdx.setGatewayLevel(nn, btp.Spec.RoutingType)
133+
case kind == resource.KindListenerSet && ref.SectionName != nil:
134+
routingTypeIdx.setListenerSetListenerLevel(nn, *ref.SectionName, btp.Spec.RoutingType, hasRoutingType)
135+
case kind == resource.KindListenerSet:
136+
routingTypeIdx.setListenerSetLevel(nn, btp.Spec.RoutingType)
137+
case ref.SectionName != nil:
138+
routingTypeIdx.setRouteRuleLevel(nn, kind, *ref.SectionName, btp.Spec.RoutingType, btp.Spec.MergeType)
139+
default:
140+
routingTypeIdx.setRouteLevel(nn, kind, btp.Spec.RoutingType, btp.Spec.MergeType)
310141
}
311142

312-
if hasClusterScoped {
143+
// ClusterSettings/LoadBalancer only inform merge-eligibility, so they're moot when no
144+
// accepted gateway can enable merging; RoutingType (above) applies regardless.
145+
if mergeBackendsEnabled {
313146
switch {
314147
case kind == resource.KindGateway && ref.SectionName != nil:
315-
clusterSettingsIdx.listenerLevel[key] = true
148+
clusterSettingsIdx.setGatewayListenerLevel(nn, *ref.SectionName, hasClusterScoped, true)
316149
case kind == resource.KindGateway:
317150
// Gateway-level settings apply uniformly to every route sharing a merged
318151
// cluster, so they don't disqualify merging - no entry needed.
319152
case ref.SectionName != nil:
320-
clusterSettingsIdx.routeRuleLevel[key] = true
153+
clusterSettingsIdx.setRouteRuleLevel(nn, kind, *ref.SectionName, hasClusterScoped, btp.Spec.MergeType)
321154
default:
322-
clusterSettingsIdx.routeLevel[key] = true
155+
clusterSettingsIdx.setRouteLevel(nn, kind, hasClusterScoped, btp.Spec.MergeType)
323156
}
324-
}
325157

326-
if hasLoadBalancer {
327-
switch {
328-
case kind == resource.KindGateway && ref.SectionName == nil:
329-
gwKey := types.NamespacedName{Namespace: string(ref.Namespace), Name: string(ref.Name)}
330-
if _, exists := loadBalancerIdx.gatewayLevel[gwKey]; !exists {
331-
loadBalancerIdx.gatewayLevel[gwKey] = btp.Spec.LoadBalancer.Type == egv1a1.ConsistentHashLoadBalancerType
158+
if hasLoadBalancer {
159+
switch {
160+
case kind == resource.KindGateway && ref.SectionName == nil:
161+
loadBalancerIdx.setGatewayLevel(nn, btp.Spec.LoadBalancer.Type == egv1a1.ConsistentHashLoadBalancerType)
162+
default:
163+
// A listener/route-rule/route-level LoadBalancer setting already
164+
// disqualifies its own rule from merging on its own, so it's never looked
165+
// up here.
332166
}
333-
default:
334-
// A listener/route-rule/route-level LoadBalancer setting already disqualifies
335-
// its own rule from merging on its own, so it's never looked up here.
336167
}
337168
}
338169
}
@@ -345,65 +176,6 @@ func BuildBTPIndexes(
345176
}
346177
}
347178

348-
// HasRouteLevelClusterSettings reports whether a route-rule, route, or listener-level
349-
// BackendTrafficPolicy contributes backend-cluster-scoped settings for the given target. A
350-
// gateway-level setting isn't checked: it applies uniformly to every route sharing a merged
351-
// cluster, so it can't cause a divergence.
352-
func (idx *BTPClusterSettingsIndex) HasRouteLevelClusterSettings(
353-
routeKind gwapiv1.Kind,
354-
routeNN types.NamespacedName,
355-
gatewayNN types.NamespacedName,
356-
listenerName *gwapiv1.SectionName,
357-
routeRuleName *gwapiv1.SectionName,
358-
) bool {
359-
if idx == nil {
360-
return false
361-
}
362-
363-
if routeRuleName != nil {
364-
key := btpRoutingKey{
365-
Kind: string(routeKind),
366-
Namespace: routeNN.Namespace,
367-
Name: routeNN.Name,
368-
SectionName: string(*routeRuleName),
369-
}
370-
if idx.routeRuleLevel[key] {
371-
return true
372-
}
373-
}
374-
375-
routeKey := btpRoutingKey{
376-
Kind: string(routeKind),
377-
Namespace: routeNN.Namespace,
378-
Name: routeNN.Name,
379-
}
380-
if idx.routeLevel[routeKey] {
381-
return true
382-
}
383-
384-
if listenerName != nil {
385-
listenerKey := btpRoutingKey{
386-
Kind: resource.KindGateway,
387-
Namespace: gatewayNN.Namespace,
388-
Name: gatewayNN.Name,
389-
SectionName: string(*listenerName),
390-
}
391-
if idx.listenerLevel[listenerKey] {
392-
return true
393-
}
394-
}
395-
396-
return false
397-
}
398-
399-
// IsConsistentHash reports whether gatewayNN has a BackendTrafficPolicy setting LoadBalancer to
400-
// ConsistentHash.
401-
func (idx *BTPLoadBalancerIndex) IsConsistentHash(gatewayNN types.NamespacedName) bool {
402-
if idx == nil {
403-
return false
404-
}
405-
return idx.gatewayLevel[gatewayNN]
406-
}
407179

408180
// deprecatedFieldsUsedInBackendTrafficPolicy returns a map of deprecated field paths to their alternatives.
409181
func deprecatedFieldsUsedInBackendTrafficPolicy(policy *egv1a1.BackendTrafficPolicy) map[string]string {
@@ -770,7 +542,7 @@ func (t *Translator) processBackendTrafficPolicyForRoute(
770542
// parentRefCtxs holds parent gateway/listener contexts for using in policy merge logic.
771543
parentRefCtxs := make([]*RouteParentContext, 0, len(parentRefs))
772544
routeNN := utils.NamespacedName(targetedRoute)
773-
routeAsChildScope := routeScope(routeNN)
545+
routeAsChildScope := routeScope(routeNN, string(targetedRoute.GetRouteType()))
774546
for _, p := range parentRefs {
775547
parentNamespace := targetedRoute.GetNamespace()
776548
if p.Namespace != nil {

0 commit comments

Comments
 (0)