dynamic_modules: support per route config by yuval-k · Pull Request #39348 · envoyproxy/envoy

yuval-k · 2025-05-05T22:57:42Z

Commit Message: Support per route filter configuration in dynamic modules

Risk Level: Low. New functionality. existing functionality should not be impacted.

Testing: Updated unit & integration test

Release Notes:
dynamic_modules now support per route filter config

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

repokitteh-read-only · 2025-05-05T22:57:51Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #39348 was opened by yuval-k.

see: more, trace.

yuval-k · 2025-05-05T23:32:20Z

/coverage

repokitteh-read-only · 2025-05-05T23:32:24Z

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/39348/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #39348 (comment) was created by @yuval-k.

see: more, trace.

mathetake

Do you really need this? disabled API of HTTP filter can be used to achieve per route configuration without specific API in each HTTP filter.

E.g. https://github.com/envoyproxy/dynamic-modules-examples/compare/perroute?expand=1 this should be enough to achieve everything in this PR if i am not missing something else

/wait

yuval-k · 2025-05-06T01:46:55Z

If I understand the suggested approach correctly - you will need a unique filter in the listener filter chain for each unique per-route config. while technically possible, it has issues - making control plane & data plane more complex.

With this approach only one filter is needed in the filter chain.

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

mathetake · 2025-05-06T01:52:00Z

making control plane & data plane more complex.

Could you elaborate on how? This PR is not needed so definitely data plane is not more complex than with this IMO. I believe this is introduced by @wbpcode to eliminate the need to have per-route API for every single one of extension, notably the need for PRs like this. disabled can be used for any existing and future filters.

cc @arkodg I believe this is how Envoy Gateway handles with per-route settings.

yuval-k · 2025-05-06T01:57:33Z

With the approach you are suggesting, will i be able to use one filter on the HCM filter chain, and multiple different configs on the route?

I believe the answer is no, please correct me if this is not the case.
Assuming the answer is no, then that means that i need to coordinate RDS config with LDS which makes control plane more complex.

Is envoy getting rid of per-filter config for other http filters?

mathetake · 2025-05-06T02:39:57Z

thank you for elaborating, and I think i am getting to understand the "complexity" in the control plane you meant. However, I believe this should be resolved at more fundamental level without introducing per-route config for each existing extension. could you give me some time to find other approach ?

zhaohuabing · 2025-05-06T02:43:16Z

Just my two cents: The current per-route configuration of each HTTP filter feels quite messy. There’s duplication at two levels, and the approaches are inconsistent — some filters duplicate all configurations at the route level, others only duplicate parts, and some introduce entirely different configurations. Setting up multi-filters on the LDS and simply enabling the necessary ones on the RDS seems like a much cleaner and more consistent solution to me.

yuval-k · 2025-05-06T02:46:48Z

Just my two cents: The current per-route configuration of each HTTP filter feels quite messy. There’s duplication at two levels, and the approaches are inconsistent — some filters duplicate all configurations at the route level, others only duplicate parts, and some introduce entirely different configurations. Setting up multi-filters on the LDS and simply enabling the necessary ones on the RDS seems like a much cleaner and more consistent solution to me.

While I understand your perspective, I still prefer to use the per route config mechanism.

Given that this PR doesn't impede you from using your approach, what's the harm in merging it?

wbpcode · 2025-05-06T06:35:14Z

I personally think this make sense. Actually, type_per_filter_config is still the main way to enable different behavior of same filter in different routes.

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

mathetake

I had a lot of meaningful offline discussion with @yuval-k and I agree with the general direction. left minor comments where i am mainly concerned with the of lifetime of per-specific config. but overall looking good

/wait

source/extensions/dynamic_modules/abi.h

source/extensions/dynamic_modules/sdk/rust/src/lib.rs

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

source/extensions/dynamic_modules/sdk/rust/src/lib.rs

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

mathetake

sorry for the delay (was on ooo for a few days). almost there but left a high level comment!

source/extensions/dynamic_modules/sdk/rust/src/lib.rs

mathetake · 2025-05-14T00:02:59Z

api/envoy/extensions/filters/http/dynamic_modules/v3/dynamic_modules.proto

+  // The name for this filter configuration. This can be used to distinguish between different filter implementations
+  // inside a dynamic module. For example, a module can have completely different filter implementations.
+  // When Envoy receives this configuration, it passes the filter_name to the dynamic module's HTTP per-route filter config init function
+  // together with the filter_config.
+  // That way a module can decide which in-module filter implementation to use based on the name at load time.
+  string filter_name = 2;


i think this is in reality not a filter but a filter_configuration. That is, the name here is nothing to do with the filter_name on the referencing HTTP filter side configuration. At least i would like to call this filter_config_name or per_route_config_name to differentiate these two since otherwise, this feels like these two are somehow associated. In fact, at the higher level on SDK, this will passed to NewHttpFilterPerRouteConfigFunction which matches the name i suggested.

Suggested change

// The name for this filter configuration. This can be used to distinguish between different filter implementations

// inside a dynamic module. For example, a module can have completely different filter implementations.

// When Envoy receives this configuration, it passes the filter_name to the dynamic module's HTTP per-route filter config init function

// together with the filter_config.

// That way a module can decide which in-module filter implementation to use based on the name at load time.

string filter_name = 2;

// The name for this per-route configuration. This can be used to distinguish between different per-route config implementations

// inside a dynamic module. For example, a module can have completely different implementations.

// When Envoy receives this configuration, it passes the filter_name to the dynamic module's HTTP per-route filter config init function

// together with the filter_config.

// That way a module can decide which in-module filter implementation to use based on the name at load time.

string per_route_config_name = 2;

or maybe we might want to move NewHttpFilterPerRouteConfigFunction into a part of HttpFilterConfig trait and use the name here to point to that to clarify these relationship.

Usually the way we do these filters, is that the same filter does both the listener level config, and route level config. so it's helpful to have access to the filter_name when creating a route level config (and have it be the same name as the listener level config).

That's why i think the filter_name is appropriate here. That said, if you feel strongly about this change, I ok making it. LMK what you think!

yeah what i meant was that this filter_name can be arbitrary regardless of where it is used. For example

diff --git a/test/extensions/dynamic_modules/http/integration_test.cc b/test/extensions/dynamic_modules/http/integration_test.cc index 476ec00cf1..b74c9c400a 100644 --- a/test/extensions/dynamic_modules/http/integration_test.cc +++ b/test/extensions/dynamic_modules/http/integration_test.cc @@ -45,7 +45,7 @@ filter_config: envoy::extensions::filters::http::dynamic_modules::v3::DynamicModuleFilterPerRoute per_route_config_proto; TestUtility::loadFromYaml( - fmt::format(filter_per_route_config, filter_name, type_url, per_route_config), + fmt::format(filter_per_route_config, "anything", type_url, per_route_config), per_route_config_proto); config_helper_.addConfigModifier( diff --git a/test/extensions/dynamic_modules/test_data/rust/http_integration_test.rs b/test/extensions/dynamic_modules/test_data/rust/http_integration_test.rs index b7ff963ee8..1de6bef25e 100644 --- a/test/extensions/dynamic_modules/test_data/rust/http_integration_test.rs +++ b/test/extensions/dynamic_modules/test_data/rust/http_integration_test.rs @@ -38,7 +38,7 @@ fn new_http_filter_config_fn<EC: EnvoyHttpFilterConfig, EHF: EnvoyHttpFilter>( fn new_http_filter_per_route_config_fn(name: &str, config: &[u8]) -> Option<Box<dyn Any>> { match name { - "per_route_config" => Some(Box::new(PerRoutePerRouteFilterConfig { + "anything" => Some(Box::new(PerRoutePerRouteFilterConfig { value: String::from_utf8(config.to_owned()).unwrap(), })), _ => panic!("Unknown filter name: {}", name),

this can also pass the integration test here where this "filter_name" ("anything") is irrelevant to the actual HCM dynamic modules http filter's filter_name ("per_route_config"). I wondered if there's any better way to somehow associate "filter_name" here with the listener level one if we use it here as well or I would prefer to rename it to per_route_config_name.

I'm not aware of way to associate these in envoy. i can rename to per_route_config_name

on second thought, i think i am good with the original filter_name at the end of the day. i am sorry about the round trip but could you revert the rename change to filter_name f804884 ? 🙇

after that i will approve this 🙏

kindly ping @yuval-k in case you missed my comment ^ 👍

nvm (either works for me)

source/extensions/filters/http/dynamic_modules/factory.h

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

yuval-k · 2025-05-15T15:09:34Z

check failes on //test/common/http:http2_codec_impl_fuzz_test the same check fails in other PRs. for example, in
#39482 (run: https://github.com/envoyproxy/envoy/actions/runs/15038384319/job/42264352786)
so it is most likely a flake.

yuval-k · 2025-05-15T16:49:40Z

/retest

another flake :/

docker: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit

yuval-k · 2025-05-15T16:50:44Z

/retest

mathetake

thank you! looks good to me so far modulo the API naming as per the comment.

i would like an input from @mattklein123 regarding the API as well as the Rust SDK side, especially around the use of Any/Arc etc.

/wait

mathetake · 2025-05-19T16:25:53Z

api/envoy/extensions/filters/http/dynamic_modules/v3/dynamic_modules.proto

+  // The name for this filter configuration. This can be used to distinguish between different filter implementations
+  // inside a dynamic module. For example, a module can have completely different filter implementations.
+  // When Envoy receives this configuration, it passes the filter_name to the dynamic module's HTTP per-route filter config init function
+  // together with the filter_config.
+  // That way a module can decide which in-module filter implementation to use based on the name at load time.
+  string filter_name = 2;


yeah what i meant was that this filter_name can be arbitrary regardless of where it is used. For example

diff --git a/test/extensions/dynamic_modules/http/integration_test.cc b/test/extensions/dynamic_modules/http/integration_test.cc index 476ec00cf1..b74c9c400a 100644 --- a/test/extensions/dynamic_modules/http/integration_test.cc +++ b/test/extensions/dynamic_modules/http/integration_test.cc @@ -45,7 +45,7 @@ filter_config: envoy::extensions::filters::http::dynamic_modules::v3::DynamicModuleFilterPerRoute per_route_config_proto; TestUtility::loadFromYaml( - fmt::format(filter_per_route_config, filter_name, type_url, per_route_config), + fmt::format(filter_per_route_config, "anything", type_url, per_route_config), per_route_config_proto); config_helper_.addConfigModifier( diff --git a/test/extensions/dynamic_modules/test_data/rust/http_integration_test.rs b/test/extensions/dynamic_modules/test_data/rust/http_integration_test.rs index b7ff963ee8..1de6bef25e 100644 --- a/test/extensions/dynamic_modules/test_data/rust/http_integration_test.rs +++ b/test/extensions/dynamic_modules/test_data/rust/http_integration_test.rs @@ -38,7 +38,7 @@ fn new_http_filter_config_fn<EC: EnvoyHttpFilterConfig, EHF: EnvoyHttpFilter>( fn new_http_filter_per_route_config_fn(name: &str, config: &[u8]) -> Option<Box<dyn Any>> { match name { - "per_route_config" => Some(Box::new(PerRoutePerRouteFilterConfig { + "anything" => Some(Box::new(PerRoutePerRouteFilterConfig { value: String::from_utf8(config.to_owned()).unwrap(), })), _ => panic!("Unknown filter name: {}", name),

this can also pass the integration test here where this "filter_name" ("anything") is irrelevant to the actual HCM dynamic modules http filter's filter_name ("per_route_config"). I wondered if there's any better way to somehow associate "filter_name" here with the listener level one if we use it here as well or I would prefer to rename it to per_route_config_name.

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

yuval-k · 2025-05-19T21:38:44Z

tsan flake

/reset

yuval-k · 2025-05-19T23:19:46Z

/retest

yuval-k · 2025-05-22T17:56:17Z

hi @mathetake @mattklein123, friendly ping on this. Happy to address any additional feedback!

mathetake

Thanks, could you add a unit test where the Arc reference is actually working as intended in filter_test.cc? like the case where:

write a filter that retrieves the per_route config in on_request_header and save it in a filter instance, and invoke the event hook
release the per-route config on the host side so that Envoy side memory is released as well as envoy_dynamic_module_on_http_filter_per_route_config_destroy is called.
Invoke on_response_header and try to access the arc wrapped memory region

i think if the arc is not working as we intended the test should crash or TSAN should detect the memory leak in that case. just in case!

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

yuval-k · 2025-05-23T14:25:24Z

hi @mathetake! just pushed a unit tests as you suggested

yuval-k · 2025-05-23T15:32:51Z

flake:

ERROR: /source/test/config_test/BUILD:32:14: Testing //test/config_test:example_configs_test failed: (Exit 34): 2 errors during bulk transfer:

yuval-k · 2025-05-23T15:32:56Z

/retest

mathetake

looks good to me, thank you for adding the new test case! That really helps.

Deferring to @abeyad for api-shepherds approval.

test/extensions/dynamic_modules/http/filter_test.cc

Co-authored-by: Takeshi Yoneda <t.y.mathetake@gmail.com> Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

yuval-k · 2025-06-02T16:37:52Z

hi @abeyad ! friendly ping if you are able to review this?
it's only blocked on api review.
If not, maybe someone else from @envoyproxy/api-shepherds can?

abeyad

/lgtm api

dynamic_modules: support per route config

5eb10f3

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

yuval-k requested review from mathetake and mattklein123 as code owners May 5, 2025 22:57

repokitteh-read-only bot added the api label May 5, 2025

repokitteh-read-only bot assigned abeyad May 5, 2025

mathetake reviewed May 5, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label May 5, 2025

mathetake self-assigned this May 5, 2025

try to make CI happy

1cf6415

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

repokitteh-read-only bot removed the waiting label May 6, 2025

mathetake added the waiting label May 6, 2025

repokitteh-read-only bot removed the waiting label May 6, 2025

try to make CI happy 2

996a05d

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

yuval-k force-pushed the dynamic-module-per-route branch from c7f8da7 to 996a05d Compare May 6, 2025 13:49

mathetake reviewed May 6, 2025

View reviewed changes

mathetake added the waiting label May 6, 2025

repokitteh-read-only bot removed the waiting label May 6, 2025

PR feedback

6f557a3

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

yuval-k force-pushed the dynamic-module-per-route branch from b29e656 to 6f557a3 Compare May 6, 2025 21:16

mathetake reviewed May 6, 2025

View reviewed changes

source/extensions/dynamic_modules/sdk/rust/src/lib.rs Show resolved Hide resolved

yuval-k added 3 commits May 12, 2025 13:22

Merge remote-tracking branch 'origin/main' into dynamic-module-per-route

c92a68e

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

Merge remote-tracking branch 'origin/main' into dynamic-module-per-route

82257d6

tsan build flake

d66e41d

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

mathetake reviewed May 14, 2025

View reviewed changes

yuval-k added 2 commits May 15, 2025 09:53

pr comments

3989b1b

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

kick ci - http2_codec_impl_fuzz_test failed; gotta be a flake

d494ba8

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

Merge remote-tracking branch 'origin/main' into dynamic-module-per-route

328849b

mathetake reviewed May 19, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label May 19, 2025

per_route_config_name

f804884

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

repokitteh-read-only bot removed the waiting label May 19, 2025

mathetake reviewed May 22, 2025

View reviewed changes

add lifetime unit test

59822bd

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

mathetake reviewed May 23, 2025

View reviewed changes

test/extensions/dynamic_modules/http/filter_test.cc Outdated Show resolved Hide resolved

Update test/extensions/dynamic_modules/http/filter_test.cc

f80e5ca

Co-authored-by: Takeshi Yoneda <t.y.mathetake@gmail.com> Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

abeyad approved these changes Jun 4, 2025

View reviewed changes

repokitteh-read-only bot removed the api label Jun 4, 2025

mathetake approved these changes Jun 4, 2025

View reviewed changes

mathetake merged commit b841979 into envoyproxy:main Jun 4, 2025
25 checks passed

mathetake mentioned this pull request Sep 16, 2025

Flaky Test: IpVersions/DynamicModulesIntegrationTest.PerRouteConfig/IPv4 ASAN #41094

Closed

Conversation

yuval-k commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 5, 2025

Uh oh!

yuval-k commented May 5, 2025

Uh oh!

repokitteh-read-only bot commented May 5, 2025

Uh oh!

mathetake left a comment

Choose a reason for hiding this comment

Uh oh!

yuval-k commented May 6, 2025

Uh oh!

mathetake commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuval-k commented May 6, 2025

Uh oh!

mathetake commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhaohuabing commented May 6, 2025

Uh oh!

yuval-k commented May 6, 2025

Uh oh!

wbpcode commented May 6, 2025

Uh oh!

mathetake left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mathetake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mathetake May 14, 2025

Choose a reason for hiding this comment

Uh oh!

yuval-k May 15, 2025

Choose a reason for hiding this comment

Uh oh!

mathetake May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuval-k May 19, 2025

Choose a reason for hiding this comment

Uh oh!

mathetake May 29, 2025

Choose a reason for hiding this comment

Uh oh!

mathetake Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

mathetake Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuval-k commented May 15, 2025

Uh oh!

yuval-k commented May 15, 2025

Uh oh!

yuval-k commented May 15, 2025

Uh oh!

mathetake left a comment

Choose a reason for hiding this comment

Uh oh!

mathetake May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuval-k commented May 19, 2025

Uh oh!

yuval-k commented May 5, 2025 •

edited

Loading

mathetake commented May 6, 2025 •

edited

Loading

mathetake commented May 6, 2025 •

edited

Loading

mathetake left a comment •

edited

Loading

mathetake May 19, 2025 •

edited

Loading

mathetake May 19, 2025 •

edited

Loading