-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat(build): build wasmx as static or dynamic module #11100
Conversation
01d839c
to
ff0ee63
Compare
build/BUILD.bazel
Outdated
@@ -90,13 +96,27 @@ wasmx_vm_cmd = select({ | |||
"//conditions:default": "", | |||
}) | |||
|
|||
wasmx_load_module = select({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's stopping us from just adding this to the template?
[...]
error_log ${{PROXY_ERROR_LOG}} ${{LOG_LEVEL}};
> if wasm then
load_module /path/to/ngx_wasm_module.so;
> end
[...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the module is built statically, ngx_wasm_module.so
will not exist and the config check will fail. We can remove the line in this case instead of adding, but it seems the build script complexity would be similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I bet there's a way to detect this in the conf loader.
We could parse ngx.config.nginx_configure to check for --add-module
vs --add-dynamic-module
and choose whether or not to add load_module
. This would even help to provide nicer errors before doing a conf test (i.e. if the user turns wasm on, but we can't find ngx_wasm_module in the configure string, raise an error).
Either that or maybe we copy the dynamic module to some known path, and the conf loader can check for the existence of /path/to/dynamic/ngx_wasm_module.so in order to determine adding load_module
.
Does that sound like it work, or am I not understanding things fully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice idea! I made the needed changes, wdyt?
Now the module is looked up in 3 different locations, in order: KONG_DYNAMIC_MODULES_PATH
env var, the build prefix path and then /usr/local/openresty/nginx/modules
. The second one is needed for the dev env but will not work in the production env. I think it's a acceptable performance penalty as this function is called only when starting.
59e5423
to
b510727
Compare
fe7a522
to
4a39424
Compare
2b8d251
to
58abe47
Compare
added options to select if ngx_wasm_module will be built as static or dynamic module. Signed-off-by: Vinicius Mignot <vinicius.mignot@gmail.com>
this change edits kong/template/nginx.lua in build time to add a `load_module` entry for ngx_wasm_module if it is needed (in case wasmx module is dynamic)
551802e
to
79ec36f
Compare
a4f513d
to
f830022
Compare
2d66358
to
d459d6d
Compare
d459d6d
to
b9b8f7b
Compare
b9b8f7b
to
3917dac
Compare
867d440
to
4849451
Compare
2839107
to
9681744
Compare
9681744
to
b55e56a
Compare
* feat(build): build wasmx as static or dynamic module added options to select if ngx_wasm_module will be built as static or dynamic module. Signed-off-by: Vinicius Mignot <vinicius.mignot@gmail.com> * feat(build): add `load_module` directive to template this change edits kong/template/nginx.lua in build time to add a `load_module` entry for ngx_wasm_module if it is needed (in case wasmx module is dynamic) * chore(build): buildifier things * chore(build): build wasmx as a dyn mod by default * chore(explain_manifest): add wasmx to manifest * tests(build): auto update the fixture template with mod path * feat(conf_loader): lookup and add wasmx dynamic module to conf * fix(build): use wasmx flag instead of duplicating code * chore(build) naming convention --------- Co-authored-by: Guilherme Salazar <gsz@acm.org> Signed-off-by: Vinicius Mignot <vinicius.mignot@gmail.com>
* feat(build): build wasmx as static or dynamic module added options to select if ngx_wasm_module will be built as static or dynamic module. Signed-off-by: Vinicius Mignot <vinicius.mignot@gmail.com> * feat(build): add `load_module` directive to template this change edits kong/template/nginx.lua in build time to add a `load_module` entry for ngx_wasm_module if it is needed (in case wasmx module is dynamic) * chore(build): buildifier things * chore(build): build wasmx as a dyn mod by default * chore(explain_manifest): add wasmx to manifest * tests(build): auto update the fixture template with mod path * feat(conf_loader): lookup and add wasmx dynamic module to conf * fix(build): use wasmx flag instead of duplicating code * chore(build) naming convention --------- Co-authored-by: Guilherme Salazar <gsz@acm.org> Signed-off-by: Vinicius Mignot <vinicius.mignot@gmail.com>
* feat(build): build wasmx as static or dynamic module added options to select if ngx_wasm_module will be built as static or dynamic module. Signed-off-by: Vinicius Mignot <vinicius.mignot@gmail.com> * feat(build): add `load_module` directive to template this change edits kong/template/nginx.lua in build time to add a `load_module` entry for ngx_wasm_module if it is needed (in case wasmx module is dynamic) * chore(build): buildifier things * chore(build): build wasmx as a dyn mod by default * chore(explain_manifest): add wasmx to manifest * tests(build): auto update the fixture template with mod path * feat(conf_loader): lookup and add wasmx dynamic module to conf * fix(build): use wasmx flag instead of duplicating code * chore(build) naming convention --------- Co-authored-by: Guilherme Salazar <gsz@acm.org> Signed-off-by: Vinicius Mignot <vinicius.mignot@gmail.com>
Summary
This change makes it possible to build ngx_wasm_module as a dynamic Nginx module.
It adds the new flag
--//:wasmx_module
, which acceptsdynamic
andstatic
options. Currentlydynamic
is the default value.There is a new feature to include the
load_module
directive innginx.conf
when dynamic module is in use andwasm = on
. It will search for the shared object in the path defined by the environment variableKONG_DYNAMIC_MODULES_PATH
, the nginx build prefix and then in/usr/local/openresty/nginx/modules
(the default installation path).Checklist
Full changelog
Issue reference
KAG-664