dynamic_modules: scaffolds config API & HTTP Filter #36448
Conversation
Signed-off-by: Takeshi Yoneda <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
|
/retest |
|
cc @marc-barry ps the flaky CI failure is nothing to do with this PR
|
| envoy.extensions.dynamic_modules.v3.DynamicModuleConfig dynamic_module_config = 1; | ||
|
|
||
| // 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. |
There was a problem hiding this comment.
Why is this level of indirection useful? Why not just have multiple modules in this case?
There was a problem hiding this comment.
The reason is that each module must have its own copy of language runtime which is an unnecessary overhead (~a few MB) in the memory space as well as some languages do not support multiple language runtime existence in one process such as Go.
| // 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.
| // Currently, the implementation is work in progress and not usable. | ||
| message DynamicModuleFilter { | ||
| // Specifies the shared-object level configuration. | ||
| envoy.extensions.dynamic_modules.v3.DynamicModuleConfig dynamic_module_config = 1; |
There was a problem hiding this comment.
Does it really make sense for the control plane to have to know the actual path to the dynamic module, or will that be something local controlled by the workload or deployment? I'm wondering if it would be valuable to have a layer of indirection here, where the bootstrap file defines some dynamic module instances, giving each one a name, and the control plane sends the instance name of the module to use. This way, the API between the control plane and the workload is defined in terms of these abstract instance names rather than in terms of specific paths.
There was a problem hiding this comment.
I think this is a good question. In fact, for the case of Wasm, even though it has the same DataSource config like this PR, the reality of Istio is that the control plane always passes a local path and the sidecar agent is the one downloading the binary from remote source and place it at the location specified by the control plane because the Envoy's filter chain set up routine cannot be made asynchronous, hence the filter chain would end up in a dangerous filter state (e.g. downloading failed but the filter chain initialized).
For the case of dynamic modules, now I think at least DataSource configuration is overkill since unlike Wasm case, dlopen can only refer a local file and cannot use the in-memory bytes. So at the end of the day, we would end up using only local file paths in reality just like the Wasm case, e.g. Envoy would need to write inline_bytes into a temporary files etc (which I think is messy tbh).
Regarding your idea of using the bootstrap file stuff, if I understand correctly, we would lose the ability to load/distribute module files at runtime without restarting Envoy vs if we can specify the local file path here, at least we can achieve that with the help of a side car agent etc.
From implementation point of view, there's almost no difference between these choices,, so I think the question is here is whether or not we want to tie the Envoy deployment with loadable dynamic modules by having a dynamic modules specific config in the Envoy bootstrap config, or have the ability to load/discover modules at runtime. wdyt? @mattklein123 @marc-barry
There was a problem hiding this comment.
I guess in practice, users wouldn't distribute native binaries at runtime as it's tied to the machine arch/os Envoy is running on, and therefore they would deploy the modules together with Envoy. So, personally either choice works for me.
There was a problem hiding this comment.
so basically what I am thinking right now is to stop using DataSource (the reason I described above) and have a dedicated oneof so that we can later add another option like the bootstrap config thingy discussed here like
diff --git a/api/envoy/extensions/dynamic_modules/v3/dynamic_modules.proto b/api/envoy/extensions/dynamic_modules/v3/dynamic_modules.proto
index 204c482b02..5e612e2a7c 100644
--- a/api/envoy/extensions/dynamic_modules/v3/dynamic_modules.proto
+++ b/api/envoy/extensions/dynamic_modules/v3/dynamic_modules.proto
@@ -36,8 +36,15 @@ option (xds.annotations.v3.file_status).work_in_progress = true;
// Currently, the implementation is work in progress and not usable.
// [#next-free-field: 8]
message DynamicModuleConfig {
- // The location of the object file. Currently, only the local path is supported.
- config.core.v3.DataSource object_file = 7;
+ // The location of the object file. Intentionally this does not use config.core.v3.DataSource, as the
+ // object file must be loaded via dlopen, which requires a physical file path, not a in-memory buffer.
+ oneof location {
+ // The object file's local path. It can be a relative path or an absolute path.
+ // E.g. /path/to/module.so or module.so
+ string local_path = 2;
+
+ // TODO: maybe a "name" pointing to a file configured in the bootstrap.
+ }
// Set true to prevent the module from being unloaded with dlclose.
// This is useful for modules that have global state that should not be unloaded.There was a problem hiding this comment.
Note that the xDS API style guidelines discourage use of oneof.
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:]