dynamic modules: refactor existing go sdk to prep for expansion#44910
Open
AdamEAnderson wants to merge 7 commits into
Open
dynamic modules: refactor existing go sdk to prep for expansion#44910AdamEAnderson wants to merge 7 commits into
AdamEAnderson wants to merge 7 commits into
Conversation
Signed-off-by: Adam Anderson <6754028+AdamEAnderson@users.noreply.github.com>
AdamEAnderson
requested review from
agrawroh,
mathetake,
mattklein123 and
wbpcode
as code owners
Contributor
Author
|
fyi @wbpcode |
…k-refactoring Signed-off-by: Adam Anderson <6754028+AdamEAnderson@users.noreply.github.com>
Contributor
|
/wait |
wbpcode
reviewed
May 16, 2026
Member
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for the contribution to make this SDK better. This PR contains lots of different content, files renaming, document improvement, refactoring to existing API, pure style changes.
For the pure style changes, could you revert it and keep previous style if no strong reason?
For the refactoring to existing API, could you split it to another independent PR?
Thanks again!!!
/wait
| // unwrap returns the live wrapper for the given pointer key, or nil if the wrapper is no | ||
| // longer registered (e.g., remove already ran). Callers MUST handle a nil return — a stale | ||
| // pointer cast would otherwise alias freed memory and crash or corrupt unrelated state when | ||
| // Envoy delivers a late callback after destroy. |
Member
There was a problem hiding this comment.
If Envoy delivers a late callback after destroy. It's a serious bug and we should let it crash. (That should never happen.
Comment on lines
-37
to
-40
| type httpFilterSharedDataWrapper struct { | ||
| data any | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
This is used for cross-filters data sharing in the golang world. Could you keep this?
Comment on lines
+375
to
+376
| hostPluginPtr C.envoy_dynamic_module_type_http_filter_envoy_ptr | ||
| filter *dymHttpFilterHandle |
Member
There was a problem hiding this comment.
why you change the original members order?
Comment on lines
+407
to
+410
| C.envoy_dynamic_module_callback_http_span_set_sampled( | ||
| s.spanPtr, | ||
| (C.bool)(sampled), | ||
| ) |
Member
There was a problem hiding this comment.
could we revert this type of unnecessary change?
…k-refactoring Signed-off-by: Adam Anderson <6754028+AdamEAnderson@users.noreply.github.com>
Signed-off-by: Adam Anderson <6754028+AdamEAnderson@users.noreply.github.com>
Signed-off-by: Adam Anderson <6754028+AdamEAnderson@users.noreply.github.com>
Signed-off-by: Adam Anderson <6754028+AdamEAnderson@users.noreply.github.com>
Contributor
Author
wbpcode
pushed a commit
that referenced
this pull request
May 21, 2026
…nsion (#45163) Commit Message: dynamic modules: refactor existing go sdk to prep for expansion Additional Description: getting ready to expand Go dynamic modules SDK to match the full ABI. This PR does some refactoring of the API code to split it into files that are http-related or cross-surface and to standardize on Godoc comments. Previously the http api used javadoc (i.e. `@param`, `@returns`) which isn't consistent with Golang expectations. The network filter api used Godoc as expected. Updated the Javadoc comments to conform to expectations. Otherwise no changes to the api itself, just splitting up the files differently and changing the comment formatting. See #44910 for more details related to abi changes. Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a --------- Signed-off-by: Adam Anderson <6754028+AdamEAnderson@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message: dynamic modules: refactor existing go sdk to prep for expansion
Additional Description: getting ready to expand Go dynamic modules SDK to match the full ABI. This involves some refactoring to move code around to make more sense in a world with many ABI surfaces.
Reorganized some files:
Fixed some linker issues with abi test imports that will become a problem when we have multiple abi surfaces and tests only use one of them.
Added misc missing test coverage for HTTP and network filters (including Rust)
Fixed an incorrect public SDK function signature:
The underlying ABI returns an integer, this was broken.
Rewrote a ton of docs to be more clear, to match the level of detail present in Rust SDK docs, and to match Godoc conventions.
Risk Level: low
Testing: included
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a