Skip to content

feat: support passing backend context on listener/vhost level #6518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

Xunzhuo
Copy link
Member

@Xunzhuo Xunzhuo commented Jul 14, 2025

What type of PR is this?

feat: support passing backend context on listener/vhost level

What this PR does / why we need it:

Support to pass customBackend Context on Listener Hook and VHost Level

At listener level, it needs to know all custom backends under itself, to add/update/delete http filters.

At VH level, it needs to know all custom backends under itself, to update relevant routes with route level config.

@Xunzhuo Xunzhuo requested a review from a team as a code owner July 14, 2025 12:53
@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 14, 2025

Blocker of envoyproxy/ai-gateway#823

Copy link

codecov bot commented Jul 14, 2025

Codecov Report

Attention: Patch coverage is 86.95652% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.76%. Comparing base (64f3576) to head (cd81309).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
internal/extension/registry/xds_hook.go 64.70% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6518   +/-   ##
=======================================
  Coverage   70.75%   70.76%           
=======================================
  Files         220      220           
  Lines       37757    37787   +30     
=======================================
+ Hits        26715    26739   +24     
- Misses       9482     9487    +5     
- Partials     1560     1561    +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 14, 2025

cc @mathetake

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

I have 100% grasp on why this is needed and the change makes sense but the complete lack of tests makes me nervous to approve (i guess generally the extension server lacks the unit tests not limited to this pr?)

@arkodg arkodg requested a review from guydc July 14, 2025 18:08
@guydc
Copy link
Contributor

guydc commented Jul 14, 2025

Is there a way to achieve this using custom policies that we currently support for the listener?

@arkodg
Copy link
Contributor

arkodg commented Jul 14, 2025

Can an upstream ext proc filter be used for this case ?

@mathetake
Copy link
Member

Can an upstream ext proc filter be used for this case ?

i don't think so. this is not a matter of upstream filter etc, and purely the listener context matter beyond cluster layer. Especially to modify a route level filter config using which backendref it references. A simple form of the problem @Xunzhuo is trying to solve is "Check if a route does NOT reference any extention-ref based backend, then add some configuration at that route. Otherwise, do nothing".

@arkodg
Copy link
Contributor

arkodg commented Jul 15, 2025

my preference would be to enhance PostTranslateModify hook

rpc PostTranslateModify(PostTranslateModifyRequest) returns (PostTranslateModifyResponse) {};
to support sending listeners and routes as well as receiving them

This becomes the kitchen sink hook to use when mappings between Gateway API and xDS resources are not 1:1
This request and response data can be large, so we'll also have to think of a way to limit the resource types sent and received

@mathetake
Copy link
Member

i discussed with Arko offline and second his comment ^^

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 15, 2025

PostTranslateModify will be large when config grows.

An example would be we got 100 listener, each one has 2 VH and 3 routes. But we only have one listener has the custom backend, we only need to modify the target listener under listener level hook. Instead we need to iterate all listener and routes to see if a listener referred to a route binding with custom backend, the cost will be large

@arkodg
Copy link
Contributor

arkodg commented Jul 15, 2025

there is no direct parent child relation between listener and cluster, it is 1 to many, or many to 1

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 15, 2025

@mathetake WDYT? I think what Arko proposed can solve the problem we met as well

@mathetake
Copy link
Member

I would prefer the Arko suggestion as it's simpler and it seems like more natural expansion of existing API

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 17, 2025

would close this one this we are using #6524

@Xunzhuo Xunzhuo closed this Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants