Skip to content

feat: support listener/routes in post translate hook level #6524

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

Merged
merged 8 commits into from
Jul 18, 2025

Conversation

Xunzhuo
Copy link
Member

@Xunzhuo Xunzhuo commented Jul 15, 2025

What type of PR is this?

feat: support listener/routes in post translate hook level

Which issue(s) this PR fixes:

Fixes #6523

Release Notes: Yes

@Xunzhuo Xunzhuo requested a review from a team as a code owner July 15, 2025 03:11
@Xunzhuo Xunzhuo requested review from arkodg and mathetake July 15, 2025 03:15
Copy link

codecov bot commented Jul 15, 2025

Codecov Report

Attention: Patch coverage is 89.21569% with 11 lines in your changes missing coverage. Please review.

Project coverage is 71.09%. Comparing base (268aef8) to head (f7c4749).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
api/v1alpha1/envoygateway_helpers.go 58.33% 8 Missing and 2 partials ⚠️
internal/extension/registry/xds_hook.go 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6524      +/-   ##
==========================================
+ Coverage   71.05%   71.09%   +0.03%     
==========================================
  Files         220      220              
  Lines       37892    37965      +73     
==========================================
+ Hits        26926    26990      +64     
- Misses       9393     9399       +6     
- Partials     1573     1576       +3     

☔ 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.

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.

looks good to me but maybe leave a TODO about subsetting the resources (e.g. selecting clusters by labels etc) sent via PostTranslateModify since the request/response will be potentially a few mb-tens of mb ?

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 16, 2025

new approach has been tested in envoyproxy/ai-gateway@03573fd and it worked well.

enablePostTranslateListenersAndRoutes API looks like:

apiVersion: v1
kind: ConfigMap
metadata:
  name: envoy-gateway-config
  namespace: "envoy-gateway-system"
  labels:
    helm.sh/chart: gateway-helm-v0.0.0-latest
    app.kubernetes.io/name: gateway-helm
    app.kubernetes.io/instance: eg
    app.kubernetes.io/version: "latest"
    app.kubernetes.io/managed-by: Helm
data:
  envoy-gateway.yaml: |
    apiVersion: gateway.envoyproxy.io/v1alpha1
    kind: EnvoyGateway
    gateway:
      controllerName: gateway.envoyproxy.io/gatewayclass-controller
    logging:
      level:
        default: info
    provider:
      kubernetes:
        rateLimitDeployment:
          patch:
            type: StrategicMerge
            value:
              spec:
                template:
                  spec:
                    containers:
                    - imagePullPolicy: IfNotPresent
                      name: envoy-ratelimit
                      image: docker.io/envoyproxy/ratelimit:60d8e81b
      type: Kubernetes
    extensionApis:
      enableEnvoyPatchPolicy: true
      enableBackend: true
    extensionManager:
      backendResources:
        - group: inference.networking.x-k8s.io
          kind: InferencePool
          version: v1alpha2
      hooks:
        enablePostTranslateListenersAndRoutes: true
        xdsTranslator:
          post:
            - Translation
            - Cluster
            - Route
      service:
        fqdn:
          hostname: ai-gateway-controller.envoy-ai-gateway-system.svc.cluster.local
          port: 1063
    rateLimit:
      backend:
        type: Redis
        redis:
          url: redis.redis-system.svc.cluster.local:6379

@Xunzhuo Xunzhuo force-pushed the post-translate branch 2 times, most recently from 3da1ad6 to 94bd9c7 Compare July 16, 2025 06:26
@@ -576,6 +576,15 @@ type ExtensionManager struct {
type ExtensionHooks struct {
// XDSTranslator defines all the supported extension hooks for the xds-translator runner
XDSTranslator *XDSTranslatorHooks `json:"xdsTranslator,omitempty"`

// EnablePostTranslateListenersAndRoutes controls whether listeners and routes
Copy link
Contributor

@arkodg arkodg Jul 16, 2025

Choose a reason for hiding this comment

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

my preference would be to have something like

resource:
  listener:
    selector: {}

which selects all listeners, these key values are based on resource metadata

or some form with a selector, so we can only send specific resources

  • should the top level be resource or translationHook ?

Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt @guydc

Copy link
Member Author

Choose a reason for hiding this comment

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

Created an issue for track: #6539

@arkodg arkodg added this to the v1.5.0-rc.1 Release milestone Jul 17, 2025
@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 17, 2025

option1: make hook config explicitly set:

    extensionManager:
      backendResources:
        - group: inference.networking.x-k8s.io
          kind: InferencePool
          version: v1alpha2
      hooks:
        xdsTranslator:
          hookConfig:
            postTranslation:
              includeAll: false
              resources:
                listener: {}
                route: {}
                cluster: {}
                secret: {}
            postListener: {}
            postVirtualHost: {}
            postCluster: {}
            postRoute: {}              
          post:
            - Translation
            - Cluster
            - Route
Clipboard_Screenshot_1752724258

option2: make hook config inline:

    extensionManager:
      backendResources:
        - group: inference.networking.x-k8s.io
          kind: InferencePool
          version: v1alpha2
      hooks:
        xdsTranslator:
          postTranslation:
            includeAll: false
            resources:
              listener: {}
              route: {}
              cluster: {}
              secret: {}
          postListener: {}
          postVirtualHost: {}
          postCluster: {}
          postRoute: {}              
          post:
            - Translation
            - Cluster
            - Route
Clipboard_Screenshot_1752724205

option3: dont need hook config, just put resources under xdsTranslator:

    extensionManager:
      backendResources:
        - group: inference.networking.x-k8s.io
          kind: InferencePool
          version: v1alpha2
      hooks:
        xdsTranslator:
          includeAll: false
          resources:
            listener: {}
            route: {}
            cluster: {}
            secret: {}      
          post:
            - Translation
            - Cluster
            - Route

@arkodg
Copy link
Contributor

arkodg commented Jul 17, 2025

thanks @Xunzhuo, prefer

       xdsTranslator:
          // translation hook setting common for pre and post, since its unlikely both will be set
          translation:
            includeAll: false

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 17, 2025

should add a stage in it? Like
stage: post
stage: pre

@arkodg
Copy link
Contributor

arkodg commented Jul 17, 2025

should add a stage in it? Like stage: post stage: pre

yeah we could add that and extend it in the future, afaik most users are using post rn for xds hook

Signed-off-by: bitliu <[email protected]>
@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 17, 2025

cc @arkodg a3f13e4

@Xunzhuo Xunzhuo requested a review from arkodg July 17, 2025 21:51
@arkodg arkodg requested a review from guydc July 17, 2025 22:03
Signed-off-by: bitliu <[email protected]>
Signed-off-by: bitliu <[email protected]>
@Xunzhuo Xunzhuo requested review from arkodg and guydc July 17, 2025 22:40
Signed-off-by: bitliu <[email protected]>
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

Copy link
Contributor

@guydc guydc left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks for adding this!

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jul 18, 2025

/retest

@Xunzhuo Xunzhuo merged commit d86baf2 into envoyproxy:main Jul 18, 2025
27 of 29 checks passed
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.

Support Listeners/Routes at PostTranslateModifyHook
4 participants