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

feat(build): build wasmx as static or dynamic module #11100

Merged
merged 9 commits into from
Jul 10, 2023

Conversation

locao
Copy link
Contributor

@locao locao commented Jun 21, 2023

Summary

This change makes it possible to build ngx_wasm_module as a dynamic Nginx module.

It adds the new flag --//:wasmx_module, which accepts dynamic and static options. Currently dynamic is the default value.

There is a new feature to include the load_module directive in nginx.conf when dynamic module is in use and wasm = on. It will search for the shared object in the path defined by the environment variable KONG_DYNAMIC_MODULES_PATH, the nginx build prefix and then in /usr/local/openresty/nginx/modules (the default installation path).

Checklist

Full changelog

  • Build scripts changed.

Issue reference

KAG-664

@@ -90,13 +96,27 @@ wasmx_vm_cmd = select({
"//conditions:default": "",
})

wasmx_load_module = select({
Copy link
Contributor

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

[...]

Copy link
Contributor Author

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.

Copy link
Contributor

@flrgh flrgh Jun 22, 2023

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?

Copy link
Contributor Author

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.

@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch from 59e5423 to b510727 Compare June 23, 2023 15:52
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 29, 2023
@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch from fe7a522 to 4a39424 Compare June 30, 2023 21:59
@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch 2 times, most recently from 2b8d251 to 58abe47 Compare July 4, 2023 19:06
locao added 5 commits July 5, 2023 10:04
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)
@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch from 551802e to 79ec36f Compare July 5, 2023 13:04
@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch from a4f513d to f830022 Compare July 6, 2023 14:26
@github-actions github-actions bot added the chore Not part of the core functionality of kong, but still needed label Jul 6, 2023
@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch from 2d66358 to d459d6d Compare July 6, 2023 19:52
@github-actions github-actions bot removed the chore Not part of the core functionality of kong, but still needed label Jul 6, 2023
@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch from d459d6d to b9b8f7b Compare July 6, 2023 21:07
@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch from b9b8f7b to 3917dac Compare July 7, 2023 12:26
build/BUILD.bazel Outdated Show resolved Hide resolved
build/BUILD.bazel Outdated Show resolved Hide resolved
spec/01-unit/04-prefix_handler_spec.lua Outdated Show resolved Hide resolved
BUILD.bazel Outdated Show resolved Hide resolved
@locao locao force-pushed the feat/wasmx-dyn-ngx_wasm_module branch 3 times, most recently from 867d440 to 4849451 Compare July 10, 2023 15:09
@gszr gszr force-pushed the feat/wasmx-dyn-ngx_wasm_module branch 2 times, most recently from 2839107 to 9681744 Compare July 10, 2023 18:03
@gszr gszr force-pushed the feat/wasmx-dyn-ngx_wasm_module branch from 9681744 to b55e56a Compare July 10, 2023 18:30
@locao locao merged commit ad16705 into feat/wasmx Jul 10, 2023
21 checks passed
@locao locao deleted the feat/wasmx-dyn-ngx_wasm_module branch July 10, 2023 19:16
locao added a commit that referenced this pull request Jul 13, 2023
* 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>
locao added a commit that referenced this pull request Jul 14, 2023
* 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>
flrgh pushed a commit that referenced this pull request Jul 17, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants