Skip to content

Commit ba400db

Browse files
committed
ir: Fix Service updates not propagating through translation
This change fixes an issue where patching Kubernetes Services (e.g. changing appProtocol or ports) did not dynamically propagate to the xDS config, and required restarting the kgw container. The root cause was HttpRouteIR.rulesEqual() not comparing BackendObject fields when checking backend equality. It was only comparing ClusterName and Weight, missing changes to the underlying Service configuration like appProtocol. Implements an Equals() method for the HttpBackendOrDelegate type to fix this edge case and cleanup the overall implementation to be consistent with the rest of the codebase and avoid harcoding the conditional checks related to Equals-based collection checking. Fixes #12576. Signed-off-by: timflannagan <[email protected]>
1 parent 6341471 commit ba400db

File tree

9 files changed

+100
-36
lines changed

9 files changed

+100
-36
lines changed

internal/kgateway/extensions2/plugins/backendtlspolicy/plugin.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func (d *backendTlsPolicy) Equals(in any) bool {
7070
}
7171

7272
func registerTypes() {
73-
kubeclient.Register[*gwv1.BackendTLSPolicy](
73+
kubeclient.Register(
7474
backendTlsPolicyGvr,
7575
backendTlsPolicyGroupKind,
7676
func(c kubeclient.ClientGetter, namespace string, o metav1.ListOptions) (runtime.Object, error) {

internal/kgateway/extensions2/plugins/inferenceextension/endpointpicker/backends.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func processPoolBackendObjIR(
3636
// If the pool has errors, create an empty LoadAssignment to return a 503
3737
if irPool.hasErrors() {
3838
logger.Debug("skipping endpoints due to InferencePool errors",
39-
"pool", in.ResourceName(),
39+
"pool", in.ResourceName,
4040
"errors", irPool.errors,
4141
)
4242
out.LoadAssignment = &envoyendpointv3.ClusterLoadAssignment{

internal/kgateway/extensions2/plugins/inferenceextension/endpointpicker/collections.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ func initInferencePoolCollections(
142142

143143
// Index pools by NamespacedName for status management & policy wiring
144144
poolIdx := krtpkg.UnnamedIndex(backendsCtl, func(be ir.BackendObjectIR) []string {
145-
return []string{be.ResourceName()}
145+
return []string{be.ResourceName}
146146
})
147147

148148
// Build a PolicyWrapper collection for the per-route metadata filter

internal/kgateway/krtcollections/policy.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ func (i *BackendIndex) getBackendFromAlias(kctx krt.HandlerContext, gk schema.Gr
273273
} else if res.Obj.GetCreationTimestamp().Time.Before(out.Obj.GetCreationTimestamp().Time) {
274274
out = &res // newer
275275
} else if res.Obj.GetCreationTimestamp().Time.Equal(out.Obj.GetCreationTimestamp().Time) &&
276-
res.ResourceName() < out.ResourceName() {
276+
res.ResourceName < out.ResourceName {
277277
out = &res // use name for tiebreaker
278278
}
279279
}

pkg/pluginsdk/ir/backend.go

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,8 @@ type BackendObjectIR struct {
136136
// Not added to Equals() as it is derived from the inner ObjIr, which is already evaluated
137137
Errors []error
138138

139-
// Name is the pre-calculated resource name. used as the krt resource name.
140-
resourceName string
139+
// ResourceName is the pre-calculated resource name. used as the krt resource name.
140+
ResourceName string
141141

142142
// TrafficDistribution is the desired traffic distribution for the backend.
143143
// Default is any (no priority).
@@ -153,14 +153,10 @@ func NewBackendObjectIR(objSource ObjectSource, port int32, extraKey string) Bac
153153
ObjectSource: objSource,
154154
Port: port,
155155
ExtraKey: extraKey,
156-
resourceName: BackendResourceName(objSource, port, extraKey),
156+
ResourceName: BackendResourceName(objSource, port, extraKey),
157157
}
158158
}
159159

160-
func (c BackendObjectIR) ResourceName() string {
161-
return c.resourceName
162-
}
163-
164160
func BackendResourceName(objSource ObjectSource, port int32, extraKey string) string {
165161
var sb strings.Builder
166162
sb.WriteString(objSource.ResourceName())
@@ -177,7 +173,7 @@ func (c BackendObjectIR) Equals(in BackendObjectIR) bool {
177173
objEq := c.ObjectSource.Equals(in.ObjectSource)
178174
objVersionEq := versionEquals(c.Obj, in.Obj)
179175
polEq := c.AttachedPolicies.Equals(in.AttachedPolicies)
180-
nameEq := c.resourceName == in.resourceName
176+
nameEq := c.ResourceName == in.ResourceName
181177
disableIstioAutoMTLSEq := c.DisableIstioAutoMTLS == in.DisableIstioAutoMTLS
182178

183179
// objIr may currently be nil in the case of k8s Services
@@ -188,7 +184,7 @@ func (c BackendObjectIR) Equals(in BackendObjectIR) bool {
188184
objIrEq = c.ObjIr.Equals(in.ObjIr)
189185
}
190186

191-
return objEq && objVersionEq && objIrEq && polEq && nameEq && disableIstioAutoMTLSEq
187+
return objEq && objVersionEq && objIrEq && polEq && nameEq && disableIstioAutoMTLSEq && c.AppProtocol == in.AppProtocol
192188
}
193189

194190
func (c BackendObjectIR) ClusterName() string {

pkg/pluginsdk/ir/gw.go

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -265,12 +265,6 @@ type BackendRefIR struct {
265265
Err error
266266
}
267267

268-
type HttpBackendOrDelegate struct {
269-
Backend *BackendRefIR
270-
Delegate *ObjectSource
271-
AttachedPolicies AttachedPolicies
272-
}
273-
274268
type HttpRouteRuleIR struct {
275269
ExtensionRefs AttachedPolicies
276270
AttachedPolicies AttachedPolicies
@@ -282,3 +276,28 @@ type HttpRouteRuleIR struct {
282276
// that should be propagated through to translation to any derived ir.HttpRouteRuleMatchIRs
283277
Err error
284278
}
279+
280+
type HttpBackendOrDelegate struct {
281+
Backend *BackendRefIR
282+
Delegate *ObjectSource
283+
AttachedPolicies AttachedPolicies
284+
}
285+
286+
func (h HttpBackendOrDelegate) Equals(other HttpBackendOrDelegate) bool {
287+
if !h.AttachedPolicies.Equals(other.AttachedPolicies) {
288+
return false
289+
}
290+
if h.Backend != nil && other.Backend != nil {
291+
return h.Backend.Equals(*other.Backend)
292+
}
293+
if h.Backend == nil && other.Backend == nil {
294+
if h.Delegate == nil && other.Delegate == nil {
295+
return true
296+
}
297+
if h.Delegate != nil && other.Delegate != nil {
298+
return h.Delegate.Equals(*other.Delegate)
299+
}
300+
return false
301+
}
302+
return false
303+
}

pkg/pluginsdk/ir/model.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ func NewEndpointsForBackend(us BackendObjectIR) *EndpointsForBackend {
139139
BackendLabels: labels,
140140
LbEps: make(map[PodLocality][]EndpointWithMd),
141141
ClusterName: us.ClusterName(),
142-
UpstreamResourceName: us.ResourceName(),
142+
UpstreamResourceName: us.ResourceName,
143143
Port: uint32(us.Port), //nolint:gosec // G115: upstream port is always valid port range
144144
Hostname: us.CanonicalHostname,
145145
LbEpsEqualityHash: upstreamHash,

pkg/pluginsdk/ir/routes.go

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ type Route interface {
1919
GetSourceObject() metav1.Object
2020
}
2121

22+
var _ Route = &HttpRouteIR{}
23+
2224
// this is 1:1 with httproute, and is a krt type
2325
// maybe move this to krtcollections package?
2426
type HttpRouteIR struct {
@@ -51,7 +53,6 @@ func (c HttpRouteIR) ResourceName() string {
5153
return c.ObjectSource.ResourceName()
5254
}
5355

54-
// get hostnames
5556
func (c *HttpRouteIR) GetHostnames() []string {
5657
if c == nil {
5758
return nil
@@ -96,21 +97,7 @@ func (c HttpRouteIR) rulesEqual(in HttpRouteIR) bool {
9697
return false
9798
}
9899
for j, backend := range backendsa {
99-
otherbackend := backendsb[j]
100-
if backend.Backend == nil && otherbackend.Backend == nil {
101-
continue
102-
}
103-
if backend.Backend != nil && otherbackend.Backend != nil {
104-
if backend.Backend.ClusterName != otherbackend.Backend.ClusterName {
105-
return false
106-
}
107-
if backend.Backend.Weight != otherbackend.Backend.Weight {
108-
return false
109-
}
110-
if !backend.AttachedPolicies.Equals(otherbackend.AttachedPolicies) {
111-
return false
112-
}
113-
} else {
100+
if !backend.Equals(backendsb[j]) {
114101
return false
115102
}
116103
}

pkg/pluginsdk/ir/routes_test.go

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,18 @@ func TestHTTPRouteIREquals(t *testing.T) {
235235
}
236236
}
237237

238+
// Helper to create backend with specific BackendObject (for testing AppProtocol)
239+
makeHttpBackendWithBackendObject := func(cluster string, weight uint32, backendObj *BackendObjectIR) HttpBackendOrDelegate {
240+
return HttpBackendOrDelegate{
241+
Backend: &BackendRefIR{
242+
ClusterName: cluster,
243+
Weight: weight,
244+
BackendObject: backendObj,
245+
},
246+
AttachedPolicies: AttachedPolicies{},
247+
}
248+
}
249+
238250
// Test data
239251
base := makeHTTPRoute("route1", "test-ns", "1", 1, types.UID("uid1"))
240252
differentName := makeHTTPRoute("route2", "test-ns", "1", 1, types.UID("uid1"))
@@ -293,6 +305,36 @@ func TestHTTPRouteIREquals(t *testing.T) {
293305
Name: "ruleA",
294306
}}
295307

308+
// Test data for testing backing destination object comparison
309+
httpBackendObj := BackendObjectIR{
310+
ObjectSource: ObjectSource{Group: "", Kind: "Service", Namespace: "ns", Name: "svc"},
311+
Port: 80,
312+
Obj: base,
313+
ResourceName: "/Service/ns/svc:80",
314+
AppProtocol: DefaultAppProtocol,
315+
AttachedPolicies: AttachedPolicies{},
316+
}
317+
wsBackendObj := BackendObjectIR{
318+
ObjectSource: ObjectSource{Group: "", Kind: "Service", Namespace: "ns", Name: "svc"},
319+
Port: 80,
320+
Obj: base,
321+
ResourceName: "/Service/ns/svc:80",
322+
AppProtocol: WebSocketAppProtocol,
323+
AttachedPolicies: AttachedPolicies{},
324+
}
325+
ruleWithHttpBackend := []HttpRouteRuleIR{{
326+
ExtensionRefs: emptyPolicies,
327+
AttachedPolicies: emptyPolicies,
328+
Backends: []HttpBackendOrDelegate{makeHttpBackendWithBackendObject("clusterA", 5, &httpBackendObj)},
329+
Name: "ruleA",
330+
}}
331+
ruleWithWsBackend := []HttpRouteRuleIR{{
332+
ExtensionRefs: emptyPolicies,
333+
AttachedPolicies: emptyPolicies,
334+
Backends: []HttpBackendOrDelegate{makeHttpBackendWithBackendObject("clusterA", 5, &wsBackendObj)},
335+
Name: "ruleA",
336+
}}
337+
296338
tests := []struct {
297339
name string
298340
a, b HttpRouteIR
@@ -678,6 +720,26 @@ func TestHTTPRouteIREquals(t *testing.T) {
678720
},
679721
want: true,
680722
},
723+
{
724+
name: "diff_rules_backend_object_app_protocol",
725+
a: HttpRouteIR{
726+
ObjectSource: ObjectSource{Namespace: "test-ns", Name: "route1"},
727+
SourceObject: base,
728+
AttachedPolicies: emptyPolicies,
729+
Rules: ruleWithHttpBackend,
730+
PrecedenceWeight: 0,
731+
DelegationInheritParentMatcher: false,
732+
},
733+
b: HttpRouteIR{
734+
ObjectSource: ObjectSource{Namespace: "test-ns", Name: "route1"},
735+
SourceObject: base,
736+
AttachedPolicies: emptyPolicies,
737+
Rules: ruleWithWsBackend,
738+
PrecedenceWeight: 0,
739+
DelegationInheritParentMatcher: false,
740+
},
741+
want: false,
742+
},
681743
}
682744

683745
for _, tt := range tests {

0 commit comments

Comments
 (0)