fix: store one copy of HTTPRoute Extension Filters by guydc · Pull Request #5002 · envoyproxy/gateway

guydc · 2025-01-03T22:29:29Z

What type of PR is this?

What this PR does / why we need it:
Currently, EG accumulates multiple copies of extension filters (one for each reference). As a result, multiple matching filters can be found when processing routes, leading to the reported issue.

Which issue(s) this PR fixes:

Fixes #4989

Release Notes: Yes

codecov · 2025-01-03T22:41:08Z

Codecov Report

Attention: Patch coverage is 88.09524% with 5 lines in your changes missing coverage. Please review.

Project coverage is 66.82%. Comparing base (fcdbafa) to head (eb9b2db).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/provider/kubernetes/routes.go	78.57%	2 Missing and 1 partial ⚠️
internal/provider/kubernetes/filters.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5002      +/-   ##
==========================================
+ Coverage   66.77%   66.82%   +0.04%     
==========================================
  Files         209      209              
  Lines       32101    32100       -1     
==========================================
+ Hits        21437    21451      +14     
+ Misses       9381     9368      -13     
+ Partials     1283     1281       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arkodg · 2025-01-03T23:58:43Z

can this be removed ?

arkodg · 2025-01-03T23:59:11Z

Suggested change

Fixed validation failure when multiple HTTPRoutes refer to the same extension filter

Fixed a validation failure when multiple HTTPRoutes refer to the same extension filter

arkodg · 2025-01-06T19:20:52Z

lets wrap this inside if r.hrfCRDExists { ?

arkodg

LGTM thanks !

Signed-off-by: Guy Daich <guy.daich@sap.com>

* fix: store one copy of HTTPRoute Extension Filters Signed-off-by: Guy Daich <guy.daich@sap.com> * fix code review comments Signed-off-by: Guy Daich <guy.daich@sap.com> * check if httproutefilter crd exists Signed-off-by: Guy Daich <guy.daich@sap.com> --------- Signed-off-by: Guy Daich <guy.daich@sap.com> (cherry picked from commit 2a5ecaf) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

* fix: nil pointer error (#5000) * fix: nil pointer error Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit 10a31f1) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: store one copy of HTTPRoute Extension Filters (#5002) * fix: store one copy of HTTPRoute Extension Filters Signed-off-by: Guy Daich <guy.daich@sap.com> * fix code review comments Signed-off-by: Guy Daich <guy.daich@sap.com> * check if httproutefilter crd exists Signed-off-by: Guy Daich <guy.daich@sap.com> --------- Signed-off-by: Guy Daich <guy.daich@sap.com> (cherry picked from commit 2a5ecaf) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: enable ipv4 compat mode for dual stack cluster support (#5018) enable ipv4 compat mode for dual stack cluster support Signed-off-by: Will Tekulve <tekulve.will@gmail.com> (cherry picked from commit e028254) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: explicitly set ip family and family policy in gateway spec (#5019) * explicitly set ip family and family policy Signed-off-by: Will Tekulve <tekulve.will@gmail.com> * add TestService cases Signed-off-by: Will Tekulve <tekulve.will@gmail.com> (cherry picked from commit 4d5d3f0) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: check before setting cookie TTL in sessionPersistence (#5026) * Check before setting Cookie TTL in Session Persistence Fixes a null ptr exception when the cookie ttl is nil but was being accessed without checking if its valid or not Signed-off-by: Arko Dasgupta <arko@tetrate.io> * simplify logic Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit dff0531) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: dont shift listener ports for Standalone mode (#5027) * fix: dont shift listener ports for Standalone mode Fixes: #4981 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * test Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 84f2ad2) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: shutdown-manager not respecting security context of container spec (#4938) * Fix shutdown-manager not respecting security context of container spec Signed-off-by: Dean Coakley <dean.s.coakley@gmail.com> * Update securityContext testdata Signed-off-by: Dean Coakley <dean.s.coakley@gmail.com> * Lint with gci Signed-off-by: Dean Coakley <dean.s.coakley@gmail.com> --------- Signed-off-by: Dean Coakley <dean.s.coakley@gmail.com> (cherry picked from commit 43621b4) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: use tls config from BTP when connecting to the OIDC provider's well-known endpoint. (#4857) * add e2e test for OIDC provider with TLS Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * delete file Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * use TLS config from BTLPolicy to fetch auth endpoint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * refactor Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * update release note Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * update release note Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> (cherry picked from commit 3a39c35) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Guy Daich <guy.daich@sap.com> Signed-off-by: Will Tekulve <tekulve.will@gmail.com> Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Dean Coakley <dean.s.coakley@gmail.com> Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> Co-authored-by: Guy Daich <guy.daich@sap.com> Co-authored-by: Will Tekulve <tekulvw@users.noreply.github.com> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Co-authored-by: Dean Coakley <dean.s.coakley@gmail.com>

…tch (#2595) **Description** `patchListenerWithInferencePoolFilters` appends one endpoint-picker `ext_proc` HTTP filter per entry in the `inferencePools` slice it is given. That slice is built in `maybeModifyListenerAndRoutes` from `routeNameToVHRouteNameToInferencePool`, which is keyed by xDS route name — so a pool referenced by N route matches appears N times, and the listener ends up with N identically-named copies of the same filter. There is already a guard against duplicates, but it only searches `httpConManager.HttpFilters`. New filters accumulate in the local `poolFilters` slice and are not spliced into `httpConManager.HttpFilters` until after the loop, so the guard never sees what the loop itself just added. Because `httpFilterNameForInferencePool` is derived from the pool's namespace and name, every duplicate shares one filter name. Per-route `typed_per_filter_config` is keyed by filter name, so the existing per-route disable logic cannot express "enable one of these and disable the rest" — it can only enable or disable the whole set. On the pool's own routes the set is enabled, so **every** copy runs and opens its own gRPC stream to the EPP. Impact for a single client request, on a Gateway with two InferencePools whose HTTPRoutes each have 16 InferencePool-backed matches: - 32 `adding inference pool ext proc filter` log lines per translation pass (16 per pool) - 32 identically-named `ext_proc` filters in the listener filter chain - 16 gRPC streams to the EPP, i.e. 16 full scheduling passes, for one request The last point is the damaging one. It is not only wasted latency and 16x EPP load: each redundant scheduling pass independently mutates EPP scheduler state, which makes stateful plugins unusable. In our case `no-hit-lru-scorer` rotated its LRU choice on every one of the 16 passes, so cold requests never actually spread across replicas, and prefix-cache affinity accounting was meaningless. Reducing the HTTPRoute from 16 InferencePool-backed matches to 12 reduced the EPP calls from 16 to 12, confirming the count tracks match count exactly. Observed on Envoy AI Gateway v1.0.0 with Envoy Gateway v1.8.1 (the pinned pairing from the v1.0.0 dependency table), Envoy Proxy v1.38.x, using KServe 0.20 `LLMInferenceService`, which generates an HTTPRoute with 8 InferencePool-backed rules — one of them carrying 8 path matches — for a total of 16. Both affected code paths are unchanged on `main`, and the added test fails there without this patch. **Repro** 1. Revert this patch, or apply only the added test. 2. Run `go test ./internal/extensionserver -run 'TestPatchListenerWithInferencePoolFilters/repeated_inference_pool_is_added_once' -count=1`. 3. The test fails: four entries for one pool produce five filters (four identical `envoy.filters.http.ext_proc/endpointpicker/pool1_test-ns_ext_proc` plus the router) where two are expected. **Fix** Two changes, either of which resolves the reported behavior; together they also guard the other call path: 1. Deduplicate by `namespace/name` when building `listenerToInferencePools`, so the slice handed to `patchListenerWithInferencePoolFilters` holds each pool once per listener. 2. Track filter names added within the loop in `patchListenerWithInferencePoolFilters`, so the duplicate guard accounts for filters not yet spliced into the chain. No behavior change for the single-match case, and none for distinct pools on a shared listener: both existing subtests (`listener with new inference pool filter`, `listener with multiple inference pools`) pass unmodified. **Verification** - `make precommit` passes, with no generated-file churn. - `make test` passes. - The added test fails on `main` without the fix (five filters for one pool where two are expected) and passes with it. Beyond the unit tests, the patched controller was deployed to a staging cluster running the two-InferencePool KServe 0.20 setup described above, replacing the v1.0.0 controller image. The patch applied there is identical to this one. Measured before and after, same cluster, same HTTPRoutes: | Measurement | v1.0.0 | this patch | |---|---|---| | `adding inference pool ext proc filter` per translation pass | 32 | 2 | | EPP gRPC streams per client request | 16 | 1 | | Distinct replicas chosen for 5 distinct cold prompts | 1 of 2 | 3/2 split across both | Controller logs showed no errors across the rollout and the translation pass. The 44 per-route `disabling inference pool filter` lines are unchanged and remain correct: two HTTPRoutes with 16 InferencePool-backed matches each disable the other pool's filter, and 3 Service-backed catch-all matches each disable both. Cache affinity is preserved rather than traded away for spreading: re-sending an already-seen prompt three times pinned to the replica that first served it, scoring 2 points higher there from the prefix-cache scorer, while cold prompts continued to alternate. **Related Issues/PRs (if applicable)** envoyproxy/gateway#5002 fixed the same class of defect in Envoy Gateway, where extension filters accumulated one copy per reference and produced multiple matches while processing routes. Touches the same InferencePool extension-server path as #2142 (preserve InferencePool metadata on extension hooks). Overlaps #2517 only in `internal/extensionserver/extensionserver_test.go`; that PR changes `inferencepool.go` and `post_cluster_modify.go`, so the two should rebase cleanly past each other. For context, istio/istio#58392 describes the inverse failure in a different implementation: multiple InferencePools on one Gateway losing the ext_proc config for all but the first. **Special notes for reviewers (if applicable)** The dedupe key is `namespace/name` rather than the filter name, since `httpFilterNameForInferencePool` already derives from both; using the object identity keeps the intent clear at the call site. AI-assisted patch prep; I reviewed the code and own the change. Signed-off-by: Gerrit van Doorn <gerritd@dropbox.com> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io>

guydc requested a review from a team as a code owner January 3, 2025 22:29

guydc force-pushed the fix-httproutefilter-reuse branch from 6073b77 to 7d004b3 Compare January 3, 2025 22:32

arkodg reviewed Jan 3, 2025

View reviewed changes

Comment thread internal/provider/kubernetes/resource.go Outdated

arkodg Jan 3, 2025

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be removed ?

arkodg reviewed Jan 3, 2025

View reviewed changes

arkodg reviewed Jan 6, 2025

View reviewed changes

Comment thread internal/provider/kubernetes/routes.go Outdated

arkodg Jan 6, 2025

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets wrap this inside if r.hrfCRDExists { ?

arkodg approved these changes Jan 6, 2025

View reviewed changes

arkodg requested review from a team January 6, 2025 21:43

arkodg added the cherrypick/release-v1.2.5 label Jan 6, 2025

zirain approved these changes Jan 6, 2025

View reviewed changes

guydc added 3 commits January 7, 2025 06:45

fix: store one copy of HTTPRoute Extension Filters

abcaecb

Signed-off-by: Guy Daich <guy.daich@sap.com>

fix code review comments

2be279e

Signed-off-by: Guy Daich <guy.daich@sap.com>

check if httproutefilter crd exists

eb9b2db

Signed-off-by: Guy Daich <guy.daich@sap.com>

zirain force-pushed the fix-httproutefilter-reuse branch from 6729ae9 to eb9b2db Compare January 6, 2025 22:45

arkodg merged commit 2a5ecaf into envoyproxy:main Jan 7, 2025

gerritvd mentioned this pull request Aug 28, 2026

fix: add one InferencePool ext proc filter per pool, not per route match theagentrouter/agent-router#2595

Merged

Navigation Menu

Repository navigation

fix: store one copy of HTTPRoute Extension Filters - #5002

fix: store one copy of HTTPRoute Extension Filters#5002
arkodg merged 3 commits into
envoyproxy:mainfrom
guydc:fix-httproutefilter-reuse

guydc commented Jan 3, 2025 •

edited

Loading

Uh oh!

codecov Bot commented Jan 3, 2025 •

edited

Loading

Uh oh!

arkodg Jan 3, 2025

Uh oh!

arkodg Jan 3, 2025

Uh oh!

arkodg Jan 6, 2025

Uh oh!

arkodg left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	Fixed validation failure when multiple HTTPRoutes refer to the same extension filter
	Fixed a validation failure when multiple HTTPRoutes refer to the same extension filter

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

arkodg Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

arkodg Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

arkodg Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guydc commented Jan 3, 2025 •

edited

Loading

codecov Bot commented Jan 3, 2025 •

edited

Loading