Skip to content
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

dynamic_modules: adds send response #37856

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

Conversation

bplotnick
Copy link
Contributor

Commit Message: Add envoy_dynamic_module_callback_http_send_response to the dynamic modules ABI and rust SDK
Additional Description: This adds the ability to send a local response back. I've also fixed up abi_impl_test.cc. Before, it was not asserting on the correct headers. Now i've moved to parameterized tests so we can assert on the correct headers and also split out the tests into smaller units.
Risk Level: low
Testing: unit tests
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #37856 was opened by bplotnick.

see: more, trace.

@bplotnick
Copy link
Contributor Author

@mathetake I took your send response impl from your PoC and ported it here. I haven't yet tested this with my module (hence why the PR is in draft). But i wanted to post the PR so you could take an early look if you wanted to

Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
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.

thank you for taking a shot at this - i left a coupe of comments. Mainly I do not want to bring in a third party dependency in the core API layer.

@mathetake
Copy link
Member

thank you for fixing up the unit tests btw!

Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
@bplotnick bplotnick marked this pull request as ready for review January 3, 2025 21:53
@mattklein123 mattklein123 self-assigned this Jan 3, 2025
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
Signed-off-by: Ben Plotnick <bplotnick@netflix.com>
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, thank you for your work. deferring to @mattklein123 for the rest

source/extensions/dynamic_modules/sdk/rust/src/lib.rs Outdated Show resolved Hide resolved
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