Skip to content

feat: support dynamic modules #5669

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Xunzhuo
Copy link
Member

@Xunzhuo Xunzhuo commented Apr 6, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #5668

Release Notes: Yes

@Xunzhuo Xunzhuo requested a review from a team as a code owner April 6, 2025 04:35
@Xunzhuo Xunzhuo force-pushed the feat-dm-support branch 2 times, most recently from ac887f1 to 3f82e3a Compare April 6, 2025 04:58
@Xunzhuo Xunzhuo requested a review from mathetake April 6, 2025 05:05
Copy link

codecov bot commented Apr 6, 2025

Codecov Report

Attention: Patch coverage is 76.96629% with 41 lines in your changes missing coverage. Please review.

Project coverage is 65.29%. Comparing base (e374db4) to head (e3c3251).

Files with missing lines Patch % Lines
internal/xds/translator/dynamicmodule.go 69.36% 22 Missing and 12 partials ⚠️
internal/gatewayapi/envoyextensionpolicy.go 84.00% 3 Missing and 1 partial ⚠️
internal/gatewayapi/dynamicmodule.go 92.50% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5669      +/-   ##
==========================================
+ Coverage   65.20%   65.29%   +0.08%     
==========================================
  Files         213      215       +2     
  Lines       34108    34273     +165     
==========================================
+ Hits        22241    22379     +138     
- Misses      10521    10541      +20     
- Partials     1346     1353       +7     

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

@arkodg
Copy link
Contributor

arkodg commented Apr 6, 2025

  1. EEP is probably not the best place for this, since a bad module can take out the entire proxy, not just the route
  2. Can you share a workflow of how a user will fetch a module into the pod

@zhaohuabing
Copy link
Member

zhaohuabing commented Apr 7, 2025

Some options that I can think of:

  • use a docker image to download dynamic modules. This docker image can be deployed as a side car in the Envoy pod and the lib can be loaded to Envoy via a shared volume.
  • use a customized envoy docker image provided by users. The libs for dynamic modules are already loaded to that image.

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Apr 7, 2025

The first approach can solve the same issues for golang filter, so I'm +1 for this.

Second one needs some prerequisites for features like golang/dynamic module, use envoyproxy to customize the image before using EEP for dm/go filter

@arkodg
Copy link
Contributor

arkodg commented Apr 7, 2025

Some options that I can think of:

  • use a docker image to download dynamic modules. This docker image can be deployed as a side car in the Envoy pod and the lib can be loaded to Envoy via a shared volume.
  • use a customized envoy docker image provided by users. The libs for dynamic modules are already loaded to that image.

this experience is not seamless, so maybe lets first document this in docs post v1.4, and then get feedback, and then we can eliminate the need for the EnvoyPatchPolicy with an API field (maybe in EnvoyProxy) in the future

@mathetake
Copy link
Member

mathetake commented Apr 7, 2025

FYI: Dynamic modules require the exact version match (at least for now), so for example, the modules working with Envoy v1.50.0 are not guaranteed to work with v1.51.0 (if the compatibility check fails, then HTTP filter configuration xds request will be rejected). So, coupling Envoy images would be the safest way; either using the Envoy container itself or with init container downloading (with some version argument). I believe the same restriction applies to golang filter.

So either way, the life cycle of downloading into Envoy container should match the one of Envoy container itself. that's my 2p

@zhaohuabing
Copy link
Member

zhaohuabing commented Apr 8, 2025

@mathetake Thanks for clarifying how dynamic modules need to match the version of Enovy Proxy.

This would be a problem for upgrade - since the ABI version is defined in the abi_version.h, EG can't check if a dynamic module will still be compatible or not after upgrade. Users can only find out after something is broken.

Is there a way for EG to detect an incompatibility between Envoy and a dynamic module, and then surfaced that issue in the status of EG resources?

@mathetake
Copy link
Member

mathetake commented Apr 9, 2025

There's no mechanism to check that without actually loading Envoy. Like i suggested, the current best way is to tie the Envoy container with modules as in the examples or EG introduce some module management init container like the container receives the Envoy version and downloads the specific dynamic modules for that version before Envoy running. To be clear, the same problem exists for golang filter already, and nothing specific to dynamic modules.

@mathetake
Copy link
Member

so i think we can go with this as-is leaving the lifecycle of dynamic modules to users for now, and then we can later add the "managed" installation of remote dynamic modules like in EnvoyProxy API or EnvoyGateway API maybe

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
  name: envoy-ai-gateway
spec:
  dynamic_modules:
  - name: mymodule
    location:
      url_template: https://foo.bar.com/path/to/${ENVOY_VERSION}/libmymodule.so

@arkodg
Copy link
Contributor

arkodg commented Apr 15, 2025

yah above approach looks good @mathetake, this is maybe hard to predict, but any guesstimate on average and max sizes of the module ?

@mathetake
Copy link
Member

but any guesstimate on average and max sizes of the module ?

shared library is almost the same as normal application binary, so the question would be how large a normal application would be; a few MB at least for Rust and Go, and tens of MB if it contains debug info:

~/dynamic-modules-examples/rust$ ls -hl target/debug/librust_module.so 
-rwxr-xr-x 2 mathetake mathetake 23M Apr 16 01:58 target/debug/librust_module.so

// Config is the configuration for the Dynamic Module extension.
// This configuration will be passed to the Dynamic Module extension.
// +optional
Config *apiextensionsv1.JSON `json:"config,omitempty"`
Copy link
Member

@mathetake mathetake Apr 15, 2025

Choose a reason for hiding this comment

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

i am not sure if we want to force JSON all the time. The rationale behind why the Envoy API is Any is that we don't want to force dynamic modules to pull in dependencies to parse the input. Simply making this to string should work i guess?

For example, let's say i develop a module that allows javascript to run inside envoy to modify headers. Then this config will likely be a javascrip string instead of json

Copy link
Member

Choose a reason for hiding this comment

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

Related to the comment above, I would make this ExtensionConfig but not that strong preference

//
// +kubebuilder:validation:MaxItems=16
// +optional
DynamicModule []DynamicModule `json:"dynamicModule,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I guess plural ?

Suggested change
DynamicModule []DynamicModule `json:"dynamicModule,omitempty"`
DynamicModules []DynamicModule `json:"dynamicModules,omitempty"`

// If not specified, EG will generate a unique name for the Dynamic Module extension.
//
// +optional
Name *string `json:"name,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

i would make it ExtensionName to clarify what is this for. I would also want to add the comments about one module can contain multiple extensions so this name is used to specify one out of many

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 dynamic module in EEP
4 participants