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]>

Author: timflannagan

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

@@ -70,7 +70,7 @@ func (d *backendTlsPolicy) Equals(in any) bool {
 }
 
 func registerTypes() {
-	kubeclient.Register[*gwv1.BackendTLSPolicy](
+	kubeclient.Register(
 		backendTlsPolicyGvr,
 		backendTlsPolicyGroupKind,
 		func(c kubeclient.ClientGetter, namespace string, o metav1.ListOptions) (runtime.Object, error) {

internal/kgateway/setup/setup_test.go

@@ -322,6 +322,116 @@ spec:
 	})
 }
 
+func TestServiceAppProtocolUpdate(t *testing.T) {
+	st, err := envtestutil.BuildSettings()
+	if err != nil {
+		t.Fatalf("can't get settings %v", err)
+	}
+	setupEnvTestAndRun(t, st, func(t *testing.T, ctx context.Context, kdbg *krt.DebugHandler, client istiokube.CLIClient, xdsPort, _ int) {
+		client.Kube().CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "gwtest"}}, metav1.CreateOptions{})
+
+		err = client.ApplyYAMLContents("gwtest", `kind: Gateway
+apiVersion: gateway.networking.k8s.io/v1
+metadata:
+  name: http-gw
+  namespace: gwtest
+spec:
+  gatewayClassName: kgateway
+  listeners:
+  - protocol: HTTP
+    port: 8080
+    name: http
+    allowedRoutes:
+      namespaces:
+        from: All`, `apiVersion: gateway.networking.k8s.io/v1
+kind: HTTPRoute
+metadata:
+  name: ws-route
+  namespace: gwtest
+spec:
+  parentRefs:
+    - name: http-gw
+  hostnames:
+    - "ws.example.com"
+  rules:
+    - backendRefs:
+        - name: test-service
+          port: 8080
+      matches:
+        - path:
+            type: PathPrefix
+            value: /`, `apiVersion: v1
+kind: Service
+metadata:
+  name: test-service
+  namespace: gwtest
+spec:
+  selector:
+    app: reviews
+  ports:
+    - protocol: TCP
+      port: 8080
+      targetPort: 8080
+`)
+		if err != nil {
+			t.Fatalf("failed to apply initial resources: %v", err)
+		}
+
+		time.Sleep(time.Second * 2)
+		t.Cleanup(func() {
+			if t.Failed() {
+				logKrtState(t, fmt.Sprintf("krt state for failed test: %s", t.Name()), kdbg)
+			} else if os.Getenv("KGW_DUMP_KRT_ON_SUCCESS") == "true" {
+				logKrtState(t, fmt.Sprintf("krt state for successful test: %s", t.Name()), kdbg)
+			}
+		})
+		initialDump, err := dumpXdsConfig(t, ctx, xdsPort, "http-gw")
+		if err != nil {
+			t.Fatalf("failed to get initial xDS dump: %v", err)
+		}
+		hasWebsocketUpgrade := hasWebsocketUpgradeConfig(initialDump)
+		if hasWebsocketUpgrade {
+			t.Fatalf("expected no websocket upgrade in initial state, but found one")
+		}
+
+		err = client.ApplyYAMLContents("gwtest", `apiVersion: v1
+kind: Service
+metadata:
+  name: test-service
+  namespace: gwtest
+spec:
+  selector:
+    app: reviews
+  ports:
+    - protocol: TCP
+      appProtocol: kubernetes.io/ws
+      port: 8080
+      targetPort: 8080
+`)
+		if err != nil {
+			t.Fatalf("failed to update service with appProtocol: %v", err)
+		}
+
+		time.Sleep(time.Second * 2)
+
+		updatedDump, err := dumpXdsConfig(t, ctx, xdsPort, "http-gw")
+		if err != nil {
+			t.Fatalf("failed to get updated xDS dump: %v", err)
+		}
+		if len(updatedDump.Routes) > 0 {
+			hasWebsocketUpgrade := hasWebsocketUpgradeConfig(updatedDump)
+			if !hasWebsocketUpgrade {
+				t.Fatalf("expected websocket upgrade configuration after updating appProtocol, but found none")
+			}
+			t.Logf("SUCCESS: Found websocket upgrade configuration after updating appProtocol")
+		} else {
+			t.Fatalf("No routes found in updated dump")
+		}
+
+		t.Logf("%s finished", t.Name())
+	})
+}
+
 func runScenario(t *testing.T, scenarioDir string, globalSettings *apisettings.Settings) {
 	setupEnvTestAndRun(t, globalSettings, func(t *testing.T, ctx context.Context, kdbg *krt.DebugHandler, client istiokube.CLIClient, xdsPort, _ int) {
 		// list all yamls in test data
@@ -1023,3 +1133,29 @@ func getroutesnames(l *envoylistenerv3.Listener) []string {
 	}
 	return routes
 }
+
+// dumpXdsConfig dumps the xDS config for the given gateway name. helper function to
+// ensure the dumper is closed after the dump is complete.
+func dumpXdsConfig(t *testing.T, ctx context.Context, xdsPort int, gwName string) (xdsDump, error) {
+	dumper := newXdsDumper(t, ctx, xdsPort, gwName)
+	defer dumper.Close()
+	return dumper.Dump(t, ctx)
+}
+
+// hasWebsocketUpgradeConfig checks if any route in the xDS dump has websocket upgrade configuration
+func hasWebsocketUpgradeConfig(dump xdsDump) bool {
+	for _, route := range dump.Routes {
+		for _, vhost := range route.GetVirtualHosts() {
+			for _, r := range vhost.GetRoutes() {
+				if routeAction := r.GetRoute(); routeAction != nil {
+					for _, upgradeConfig := range routeAction.GetUpgradeConfigs() {
+						if upgradeConfig.GetUpgradeType() == "websocket" {
+							return true
+						}
+					}
+				}
+			}
+		}
+	}
+	return false
+}

pkg/pluginsdk/ir/backend.go

@@ -142,10 +142,10 @@ type BackendObjectIR struct {
 
 	// Errors is a list of errors, if any, encountered while constructing this BackendObject
 	// Not added to Equals() as it is derived from the inner ObjIr, which is already evaluated
-	// +krtEqualsTodo decide whether construction errors should impact equality
+	// +noKrtEquals
 	Errors []error
 
-	// Name is the pre-calculated resource name. used as the krt resource name.
+	// resourceName is the pre-calculated resource name. used as the krt resource name.
 	resourceName string
 
 	// TrafficDistribution is the desired traffic distribution for the backend.
@@ -167,10 +167,6 @@ func NewBackendObjectIR(objSource ObjectSource, port int32, extraKey string) Bac
 	}
 }
 
-func (c BackendObjectIR) ResourceName() string {
-	return c.resourceName
-}
-
 func BackendResourceName(objSource ObjectSource, port int32, extraKey string) string {
 	var sb strings.Builder
 	sb.WriteString(objSource.ResourceName())
@@ -183,22 +179,32 @@ func BackendResourceName(objSource ObjectSource, port int32, extraKey string) st
 	return sb.String()
 }
 
+// ResourceName returns the pre-calculated resource name for this backend.
+// This method is required to implement the krt.Named interface.
+func (c BackendObjectIR) ResourceName() string {
+	return c.resourceName
+}
+
 func (c BackendObjectIR) Equals(in BackendObjectIR) bool {
-	objEq := c.ObjectSource.Equals(in.ObjectSource)
-	objVersionEq := versionEquals(c.Obj, in.Obj)
-	polEq := c.AttachedPolicies.Equals(in.AttachedPolicies)
-	nameEq := c.resourceName == in.resourceName
-	disableIstioAutoMTLSEq := c.DisableIstioAutoMTLS == in.DisableIstioAutoMTLS
-
-	// objIr may currently be nil in the case of k8s Services
-	// TODO: add an IR for Services to avoid the need for this
-	// see: internal/kgateway/extensions2/plugins/kubernetes/k8s.go
-	objIrEq := true
-	if c.ObjIr != nil {
-		objIrEq = c.ObjIr.Equals(in.ObjIr)
+	if !c.ObjectSource.Equals(in.ObjectSource) {
+		return false
 	}
-
-	return objEq && objVersionEq && objIrEq && polEq && nameEq && disableIstioAutoMTLSEq
+	if !versionEquals(c.Obj, in.Obj) {
+		return false
+	}
+	if c.ObjIr != nil && !c.ObjIr.Equals(in.ObjIr) {
+		return false
+	}
+	if !c.AttachedPolicies.Equals(in.AttachedPolicies) {
+		return false
+	}
+	if c.resourceName != in.resourceName {
+		return false
+	}
+	if c.DisableIstioAutoMTLS != in.DisableIstioAutoMTLS {
+		return false
+	}
+	return true
 }
 
 func (c BackendObjectIR) ClusterName() string {
@@ -349,20 +355,28 @@ func (c Gateway) Equals(in Gateway) bool {
 		c.DeniedListenerSets.Equals(in.DeniedListenerSets)
 }
 
+type BackendRefIR struct {
+	// TODO: remove cluster name from here, it's redundant.
+	ClusterName string
+	Weight      uint32
+
+	// backend could be nil if not found or no ref grant
+	BackendObject *BackendObjectIR
+	// if nil, error might say why
+	Err error
+}
+
 // Equals returns true if the two BackendRefIR instances are equal in cluster name, weight, backend object equality, and error.
 func (a BackendRefIR) Equals(b BackendRefIR) bool {
 	if a.ClusterName != b.ClusterName || a.Weight != b.Weight {
 		return false
 	}
-
 	if !backendObjectEqual(a.BackendObject, b.BackendObject) {
 		return false
 	}
-
 	if !errorsEqual(a.Err, b.Err) {
 		return false
 	}
-
 	return true
 }
 

pkg/pluginsdk/ir/gw.go

@@ -254,23 +254,6 @@ func (l AttachedPolicies) MarshalJSON() ([]byte, error) {
 	return json.Marshal(m)
 }
 
-type BackendRefIR struct {
-	// TODO: remove cluster name from here, it's redundant.
-	ClusterName string
-	Weight      uint32
-
-	// backend could be nil if not found or no ref grant
-	BackendObject *BackendObjectIR
-	// if nil, error might say why
-	Err error
-}
-
-type HttpBackendOrDelegate struct {
-	Backend          *BackendRefIR
-	Delegate         *ObjectSource
-	AttachedPolicies AttachedPolicies
-}
-
 type HttpRouteRuleIR struct {
 	ExtensionRefs    AttachedPolicies
 	AttachedPolicies AttachedPolicies
@@ -282,3 +265,28 @@ type HttpRouteRuleIR struct {
 	// that should be propagated through to translation to any derived ir.HttpRouteRuleMatchIRs
 	Err error
 }
+
+type HttpBackendOrDelegate struct {
+	AttachedPolicies AttachedPolicies
+	Backend          *BackendRefIR
+	Delegate         *ObjectSource
+}
+
+func (h HttpBackendOrDelegate) Equals(other HttpBackendOrDelegate) bool {
+	if !h.AttachedPolicies.Equals(other.AttachedPolicies) {
+		return false
+	}
+	if h.Backend != nil && other.Backend != nil {
+		return h.Backend.Equals(*other.Backend)
+	}
+	if h.Backend == nil && other.Backend == nil {
+		if h.Delegate == nil && other.Delegate == nil {
+			return true
+		}
+		if h.Delegate != nil && other.Delegate != nil {
+			return h.Delegate.Equals(*other.Delegate)
+		}
+		return false
+	}
+	return false
+}

pkg/pluginsdk/ir/routes.go

@@ -19,6 +19,8 @@ type Route interface {
 	GetSourceObject() metav1.Object
 }
 
+var _ Route = &HttpRouteIR{}
+
 // this is 1:1 with httproute, and is a krt type
 // maybe move this to krtcollections package?
 type HttpRouteIR struct {
@@ -54,7 +56,6 @@ func (c HttpRouteIR) ResourceName() string {
 	return c.ObjectSource.ResourceName()
 }
 
-// get hostnames
 func (c *HttpRouteIR) GetHostnames() []string {
 	if c == nil {
 		return nil
@@ -99,21 +100,7 @@ func (c HttpRouteIR) rulesEqual(in HttpRouteIR) bool {
 			return false
 		}
 		for j, backend := range backendsa {
-			otherbackend := backendsb[j]
-			if backend.Backend == nil && otherbackend.Backend == nil {
-				continue
-			}
-			if backend.Backend != nil && otherbackend.Backend != nil {
-				if backend.Backend.ClusterName != otherbackend.Backend.ClusterName {
-					return false
-				}
-				if backend.Backend.Weight != otherbackend.Backend.Weight {
-					return false
-				}
-				if !backend.AttachedPolicies.Equals(otherbackend.AttachedPolicies) {
-					return false
-				}
-			} else {
+			if !backend.Equals(backendsb[j]) {
 				return false
 			}
 		}