ir: Fix Service updates not propagating through translation #12809

timflannagan · 2025-11-03T19:44:11Z

Description

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 to get the desired functional behavior.

The root cause was the HttpRouteIR.rulesEqual() implementation didn't compare the 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.

Change Type

/kind fix

Changelog

NONE

Additional Notes

In order to reproduce locally:

``yaml

apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: test-gateway
namespace: default
spec:
gatewayClassName: kgateway
listeners:
- name: http
protocol: HTTP
port: 8080

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: test-route
namespace: default
spec:
parentRefs:
- name: test-gateway
hostnames:
- "test.example.com"
rules:
- backendRefs:
- name: echo-svc
port: 80

apiVersion: v1
kind: Service
metadata:
name: echo-svc
namespace: default
spec:
selector:
app: echo-server
ports:
- protocol: TCP
port: 80
targetPort: 8080
# appProtocol: kubernetes.io/ws

apiVersion: apps/v1
kind: Deployment
metadata:
name: echo-server
namespace: default
spec:
replicas: 1
selector:
matchLabels:
app: echo-server
template:
metadata:
labels:
app: echo-server
spec:
containers:
- name: echo
image: jmalloc/echo-server:latest
ports:
- containerPort: 8080

k -n default port-forward deploy/test-gateway 8080:8080 &

curl -v -H "Connection: Upgrade" -H "Upgrade: websocket" -H "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==" -H "Sec-WebSocket-Version: 13" -H "Host: test.example.com" http://localhost:8080


</details>

Deploying that manifest, validating that the curl command fails, and then uncommenting the appProtocol change + re-validating the curl command should return a 101 response code.

pkg/pluginsdk/ir/backend.go

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

Copilot

Pull Request Overview

This PR refactors the equality comparison logic for HTTP backend objects to properly include AppProtocol field comparisons. The key changes centralize backend equality checks and make the ResourceName field public for better API consistency.

Key Changes

Refactored HttpBackendOrDelegate.Equals() method to properly delegate equality checks to constituent types
Updated BackendObjectIR.Equals() to include AppProtocol comparison
Made BackendObjectIR.ResourceName field public and removed redundant accessor method
Added comprehensive test coverage for AppProtocol differences in backend equality

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/pluginsdk/ir/backend.go	Made `ResourceName` public, removed accessor method, added `AppProtocol` to equality check
pkg/pluginsdk/ir/gw.go	Added `HttpBackendOrDelegate.Equals()` method and moved type definition
pkg/pluginsdk/ir/routes.go	Refactored to use new `HttpBackendOrDelegate.Equals()` method
pkg/pluginsdk/ir/routes_test.go	Added test coverage for `AppProtocol` differences in backends
pkg/pluginsdk/ir/model.go	Updated to use public `ResourceName` field
internal/kgateway/krtcollections/policy.go	Updated to use public `ResourceName` field
internal/kgateway/extensions2/plugins/inferenceextension/endpointpicker/collections.go	Updated to use public `ResourceName` field
internal/kgateway/extensions2/plugins/inferenceextension/endpointpicker/backends.go	Updated to use public `ResourceName` field
internal/kgateway/extensions2/plugins/backendtlspolicy/plugin.go	Removed explicit type parameter from `kubeclient.Register` call

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

shashankram

It would be nice to have a test where we can change the appProtocol or an equivalent field and verify that it triggers a translation update on the route

shashankram · 2025-11-03T19:51:46Z

pkg/pluginsdk/ir/gw.go

+}
+
+func (h HttpBackendOrDelegate) Equals(other HttpBackendOrDelegate) bool {
+	if !h.AttachedPolicies.Equals(other.AttachedPolicies) {


nit: order comparision in the order of fields declared in the struct

shashankram · 2025-11-03T19:52:42Z

pkg/pluginsdk/ir/routes.go

-					return false
-				}
-			} else {
+			if !backend.Equals(backendsb[j]) {


Were we intentionally restricting Backend comparison to specific fields before or was this an oversight?

I think the latter is very likely. If you go through the git history for this file, you'll reach the massive refactor PR that Yuval did during the early days of the donation that predated my time: 5d4e603#diff-5b7e43b2f30d5fce0ff29295ccad60d0882ce08abb34ba7e6f9464b942e02a6f. @lgadban @yuval-k if you can confirm

it is intentional - there's no need to propagate changes like app protocol for routes - i believe it is a no-op in envoy

i.e. i don't care about backend properties that don't end up on the envoy route object

@timflannagan do you know if something about the app protcol impacts the envoy route?

the problem is that appProtocol, at least for websockets, does contribute to envoy config:

kgateway/internal/kgateway/translator/irtranslator/route.go

Lines 594 to 603 in e74aaa4

if back := backend.Backend.BackendObject; back != nil && back.AppProtocol == ir.WebSocketAppProtocol {

// add websocket upgrade if not already present

if !slices.ContainsFunc(action.GetUpgradeConfigs(), func(uc *envoyroutev3.RouteAction_UpgradeConfig) bool {

return uc.GetUpgradeType() == webSocketUpgradeType

}) {

action.UpgradeConfigs = append(action.GetUpgradeConfigs(), &envoyroutev3.RouteAction_UpgradeConfig{

UpgradeType: webSocketUpgradeType,

})

}

}

. the issue is that before this logic, we weren't comparing the Backend.BackendObject, which meant updates to Services that change the appProtocol weren't reflected in the system.

FWIW for the BackendObjectIR's Equals() not checking the AppProtocol field. I think we're all set due to

kgateway/pkg/pluginsdk/ir/iface.go

Lines 306 to 314 in 7a9e2af

func versionEquals(a, b metav1.Object) bool {

var versionEquals bool

if a.GetGeneration() != 0 && b.GetGeneration() != 0 {

versionEquals = a.GetGeneration() == b.GetGeneration()

} else {

versionEquals = a.GetResourceVersion() == b.GetResourceVersion()

}

return versionEquals && a.GetUID() == b.GetUID()

}

. As an aside, that function implementation looks like it wouldn't work with fields that are derived from annotations (e.g. traffic distribution, disable Istio auto mtls) and require those fields to explicitly contribute to the Equals implementation vs. piggybacking off the object check.

So the issue here is solely due to the lack of explicit backend.Backend equals checking.

I see, we are automatically add an upgrade to the route based on the backend app protocol. then yea the app protocol is effectivly a route policy in this case, so it should be in the equals

shashankram · 2025-11-03T19:54:00Z

pkg/pluginsdk/ir/backend.go

 	}

-	return objEq && objVersionEq && objIrEq && polEq && nameEq && disableIstioAutoMTLSEq
+	return objEq && objVersionEq && objIrEq && polEq && nameEq && disableIstioAutoMTLSEq && c.AppProtocol == in.AppProtocol


nit: Can we collapse L173-177 into L187 for consistency

timflannagan · 2025-11-03T23:02:57Z

Going to look into an IR-based linter in parallel. Throwing the WIP label depending on the level of effort for that.

timflannagan · 2025-11-05T22:43:11Z

It would be nice to have a test where we can change the appProtocol or an equivalent field and verify that it triggers a translation update on the route

SG. I'll look into either a setup test || e2e test here in addition to the unit test added.

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 kgateway-dev#12576. Signed-off-by: timflannagan <[email protected]>

Copilot AI review requested due to automatic review settings November 3, 2025 19:44

timflannagan commented Nov 3, 2025

View reviewed changes

pkg/pluginsdk/ir/backend.go Outdated Show resolved Hide resolved

timflannagan commented Nov 3, 2025

View reviewed changes

internal/kgateway/extensions2/plugins/backendtlspolicy/plugin.go Show resolved Hide resolved

Copilot AI reviewed Nov 3, 2025

View reviewed changes

internal/kgateway/extensions2/plugins/backendtlspolicy/plugin.go Show resolved Hide resolved

shashankram reviewed Nov 3, 2025

View reviewed changes

timflannagan added the work in progress Indicates that a PR should not merge because it is a work in progress label Nov 3, 2025

timflannagan mentioned this pull request Nov 4, 2025

Add custom KRT IR Equals() linter #12818

Merged

timflannagan removed the work in progress Indicates that a PR should not merge because it is a work in progress label Nov 5, 2025

timflannagan force-pushed the bug/stale-svc branch from ba400db to 978d069 Compare November 6, 2025 00:56

	if back := backend.Backend.BackendObject; back != nil && back.AppProtocol == ir.WebSocketAppProtocol {
	// add websocket upgrade if not already present
	if !slices.ContainsFunc(action.GetUpgradeConfigs(), func(uc *envoyroutev3.RouteAction_UpgradeConfig) bool {
	return uc.GetUpgradeType() == webSocketUpgradeType
	}) {
	action.UpgradeConfigs = append(action.GetUpgradeConfigs(), &envoyroutev3.RouteAction_UpgradeConfig{
	UpgradeType: webSocketUpgradeType,
	})
	}
	}

	func versionEquals(a, b metav1.Object) bool {
	var versionEquals bool
	if a.GetGeneration() != 0 && b.GetGeneration() != 0 {
	versionEquals = a.GetGeneration() == b.GetGeneration()
	} else {
	versionEquals = a.GetResourceVersion() == b.GetResourceVersion()
	}
	return versionEquals && a.GetUID() == b.GetUID()
	}

ir: Fix Service updates not propagating through translation #12809

Are you sure you want to change the base?

ir: Fix Service updates not propagating through translation #12809

Conversation

timflannagan commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Change Type

Changelog

Additional Notes

``yaml

apiVersion: gateway.networking.k8s.io/v1 kind: Gateway metadata: name: test-gateway namespace: default spec: gatewayClassName: kgateway listeners: - name: http protocol: HTTP port: 8080

apiVersion: gateway.networking.k8s.io/v1 kind: HTTPRoute metadata: name: test-route namespace: default spec: parentRefs: - name: test-gateway hostnames: - "test.example.com" rules: - backendRefs: - name: echo-svc port: 80

apiVersion: v1 kind: Service metadata: name: echo-svc namespace: default spec: selector: app: echo-server ports: - protocol: TCP port: 80 targetPort: 8080 # appProtocol: kubernetes.io/ws

k -n default port-forward deploy/test-gateway 8080:8080 &

curl -v -H "Connection: Upgrade" -H "Upgrade: websocket" -H "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==" -H "Sec-WebSocket-Version: 13" -H "Host: test.example.com" http://localhost:8080

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

shashankram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timflannagan commented Nov 3, 2025

Uh oh!

timflannagan commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timflannagan commented Nov 3, 2025 •

edited

Loading

apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: test-gateway
namespace: default
spec:
gatewayClassName: kgateway
listeners:
- name: http
protocol: HTTP
port: 8080

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: test-route
namespace: default
spec:
parentRefs:
- name: test-gateway
hostnames:
- "test.example.com"
rules:
- backendRefs:
- name: echo-svc
port: 80

apiVersion: v1
kind: Service
metadata:
name: echo-svc
namespace: default
spec:
selector:
app: echo-server
ports:
- protocol: TCP
port: 80
targetPort: 8080
# appProtocol: kubernetes.io/ws