dynamic_modules: scaffolds config API & HTTP Filter#36448
dynamic_modules: scaffolds config API & HTTP Filter#36448mattklein123 merged 19 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
/retest |
|
cc @marc-barry ps the flaky CI failure is nothing to do with this PR
|
api/envoy/extensions/filters/http/dynamic_modules/v3/dynamic_modules.proto
Show resolved
Hide resolved
| // For example, if a module has two filter implementations, one for logging and one for header manipulation, | ||
| // filter_name is used to choose either logging or header manipulation. The filter_config can be used to | ||
| // configure the logging level or the header manipulation behavior. | ||
| string filter_config = 3; |
There was a problem hiding this comment.
Why is this a string instead of a google.protobuf.Any?
There was a problem hiding this comment.
good q and this choice of string is intentional - this config will go across the C ABI boundary as-is, hence we need a plain binary representation, otherwise this will end up forcing all dynamic modules to be able to understand protobuf which is unnecessary. For example, each module would need a copy of protobuf dependency.
There was a problem hiding this comment.
This is unfriendly for users who want to provided a strutured configuration. I will at least prefer to use google.protobuf.Value here and then we convert the value to json string internaly and propagate it to the dynamic modules?
Then at least at the control plane, we can use this field to provide typed configuration.
There was a problem hiding this comment.
That would end up forcing the modules to be able to parse json by default - C language doesn't have any standard json serde and C++ either by default. The structured configuration can be at the end of the day a string right? Controlplanes should be able to serialize their structures into string on theirown, so I don't see any "inconvenience" for them vs forcing all modules to be able to understand json by default.
There was a problem hiding this comment.
what will the string configuration like? All depends on the implementation of module and different modules may have complete different way to configure?
There was a problem hiding this comment.
yes exactly, completely depends on the module implementations (hence the users code)
api/envoy/extensions/filters/http/dynamic_modules/v3/dynamic_modules.proto
Show resolved
Hide resolved
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
markdroth
left a comment
There was a problem hiding this comment.
Just one small comment about documentation; otherwise, the API looks good to me!
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
| // In Envoy, the search path can only be configured via the environment variable ``ENVOY_DYNAMIC_MODULES_SEARCH_PATH``. | ||
| // The actual search path is ``${ENVOY_DYNAMIC_MODULES_SEARCH_PATH}/lib${name}.so``. TODO: make the search path configurable via | ||
| // command line options. | ||
| string name = 1 [(validate.rules).string = {min_len: 1}]; |
There was a problem hiding this comment.
Note(summary of the discussion): control plane should be local paths agnostic (DataSource doesn't make sense as shared libraries cannot be opened via in-memory bytes, but they must be a local file) -> Envoy should be able to search modules by name in a pre-configured locations -> Temporarily using an Env var for search location following the semantics of LD_LIBRARY_PATH env in linux. This "module search path" style is almost identical mechanism as NGINX FWIW.
|
/lgtm api |
mattklein123
left a comment
There was a problem hiding this comment.
Few high level comments to get started, thanks.
/wait
| // At the start up of a filter chain, this name is passed to the module's HTTP filter initialization function. | ||
| // That way the module can decide which in-module filter implementation to use based on the name. | ||
| string filter_name = 2; |
There was a problem hiding this comment.
Per the other comment this should be done at configuration load/validation time, not when the filter chain is created.
There was a problem hiding this comment.
Also I'm not sure filter_name is a good name here if we eventually support loadable modules for non filter extensions. Perhaps something like fully_qualified_extension_name or something. To that end, I wonder if this should actually go inside the shared DynamicModuleConfig ? Then that config could de-dup what to load based on the module name, but still have the "extension name" in that structure.
There was a problem hiding this comment.
Sorry my documentation was misleading and wrong. This is intended to be done at the load/validation time of HTTP filter configuration (when it creates a factory).
As for the naming and placement, from implementation perspective, the initialization function will differ among different extension points (envoy_dynamic_modules_http_filter_config_new, envoy_dynamic_modules_tcp_filter_config_new, envoy_dynamic_modules_access_logging_new, etc), so we would need a "typed" fully_qualified_extension_name which has a info about which extension point being used. I think then it would be simpler to put this field in an extension specific proto (this case http/dynamic_modules.proto) instead of putting this in a common proto.
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
…ingl Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
no idea why suddenly gcc is getting killed ... (merged main again and hope all good) |
|
found the exact same failure on main: https://github.com/envoyproxy/envoy/actions/runs/11463146676/job/31896388874 |
|
looks like pending RBE jobs are piling up and that's why it's getting killed - I will retry later |
|
/retest |
1 similar comment
|
/retest |
|
@phlax i am getting flaky |
|
hi anything that has to whatever build or test is failing add: rbe_pool = "6gig" |
|
hmm - the test target is really small and if they need the |
|
FYI https://github.com/envoyproxy/envoy/blob/551982e5f2d950b51ee718ab60b320dbb98fb733/test/extensions/dynamic_modules/http/factory_test.cc this is one of two cases killed during build - I would be really surprised if this one needs such setting |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Head branch was pushed to by a user without write access
|
I removed the unnecessary test build deps ae9756f - hope this resolves this |
|
hmm still happening. @phlax this hasn't happened before I did a main merge yesterday. I am inclined to believe that build target dependency added recently into the test framework (//test/mock etc?) or whatever is causing this. The current failing target is envoy/test/extensions/dynamic_modules/http/BUILD Lines 11 to 22 in ae9756f From the log, it's failing to link crazy amount of files for this really small build target. |
|
ok now I see |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
problem/solution is as i said above ci was changed yesterday - altho tbf its been changing over the last few days/week - so not sure about previous runs |
|
yeah anyways added the option - f46d3d7 thanks for the advice! (I think we need to audit the build dependency graph especially around the common build test targets) |
|
to remove extraneous build/test deps? that would be amazing - did a bit of this previously - but thinning out the tree would make things much faster/use less resources |
exactly. I think the problem i am having now is essentially because of that (judging by the fact that this has started happening after a certain main merge + happening at the linking phase where linker tried to put every dependencies in-memory regardless an object file is used or not) |
Commit Message: api: removes 'wip' from dynamic_modules Additional Description: The dynamic modules API was introduced 6 months ago in #36448 and at the time we didn't have either an implementation or confidence on the configuration API, so it has been marked WIP just in case. Now that the initial implementation is done, and the documentation as well as the example repository is hosted at envoyproxy/dynamic-modules-examples, early adopters have already tried it out and we haven't heard any concern about the configuration API, so this commit removes the WIP marker towards the next release of Envoy v1.34. Note that the extension itself is still in alpha, it follows the same caveat like any other alpha extensions. Risk Level: low Testing: existing tests. Docs Changes: Already done in the past commits Release Notes: Already done in the past commits Platform Specific Features: n/a --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Commit Message: api: removes 'wip' from dynamic_modules Additional Description: The dynamic modules API was introduced 6 months ago in envoyproxy#36448 and at the time we didn't have either an implementation or confidence on the configuration API, so it has been marked WIP just in case. Now that the initial implementation is done, and the documentation as well as the example repository is hosted at envoyproxy/dynamic-modules-examples, early adopters have already tried it out and we haven't heard any concern about the configuration API, so this commit removes the WIP marker towards the next release of Envoy v1.34. Note that the extension itself is still in alpha, it follows the same caveat like any other alpha extensions. Risk Level: low Testing: existing tests. Docs Changes: Already done in the past commits Release Notes: Already done in the past commits Platform Specific Features: n/a --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Commit Message: api: removes 'wip' from dynamic_modules Additional Description: The dynamic modules API was introduced 6 months ago in envoyproxy#36448 and at the time we didn't have either an implementation or confidence on the configuration API, so it has been marked WIP just in case. Now that the initial implementation is done, and the documentation as well as the example repository is hosted at envoyproxy/dynamic-modules-examples, early adopters have already tried it out and we haven't heard any concern about the configuration API, so this commit removes the WIP marker towards the next release of Envoy v1.34. Note that the extension itself is still in alpha, it follows the same caveat like any other alpha extensions. Risk Level: low Testing: existing tests. Docs Changes: Already done in the past commits Release Notes: Already done in the past commits Platform Specific Features: n/a --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Commit Message: dynamic_modules: scaffolds config API & HTTP Filter
Additional Description:
This scaffolds the configuration API marked as work-in-progress, and
the skeleton HTTP filter implementation based on the configuration.
The real implementations will follow after this commit.
Risk Level: low
Testing: done
Docs Changes: n/a
Release Notes: n/a (not enabled yet)
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]