Skip to content

dynamic_modules: add filter state support for http filter#38979

Merged
mathetake merged 4 commits intoenvoyproxy:mainfrom
zhaoyangli311:dynamic_modules_filter_state
Apr 7, 2025
Merged

dynamic_modules: add filter state support for http filter#38979
mathetake merged 4 commits intoenvoyproxy:mainfrom
zhaoyangli311:dynamic_modules_filter_state

Conversation

@zhaoyangli311
Copy link
Copy Markdown
Contributor

Commit Message: add filter state support for dynamic modules http filter
Additional Description: With filter state support, dynamic modules rust filter can get filter state from envoy
Risk Level: low
Testing: unit test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Zhaoyang Li <zhaoyli311@gmail.com>
Signed-off-by: Zhaoyang Li <zhaoyli311@gmail.com>
@mathetake mathetake self-assigned this Apr 1, 2025
Copy link
Copy Markdown
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.

Thank you, overall looks good but high level comments:

  • Does the string need to be limited to string? i can't find any rationale to restrict it to utf8 at Rust level, so I would expect it to be &[u8] instead of &str. And change the suffix in the ABI to "bytes" instead of string.
  • Could you use the new API in http.rs, and add a test in filter_test.cc accordingly?

/wait

@mathetake mathetake mentioned this pull request Apr 3, 2025
11 tasks
Signed-off-by: Zhaoyang Li <zhaoyli311@gmail.com>
Signed-off-by: Zhaoyang Li <zhaoyli311@gmail.com>
@mathetake mathetake changed the title dynamic_modules: add filter state support for dynamic modules http filter dynamic_modules: add filter state support for http filter Apr 7, 2025
Copy link
Copy Markdown
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.

Thank you for your work!

}

TEST(DynamiModulesTest, DynamicMetadataCallbacks) {
TEST(DynamicModulesTest, DynamicMetadataCallbacks) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good catch!

@mathetake mathetake merged commit 538c9be into envoyproxy:main Apr 7, 2025
24 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.

3 participants