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(core): add wasm integration #11218

Merged
merged 3 commits into from
Jul 18, 2023
Merged

feat(core): add wasm integration #11218

merged 3 commits into from
Jul 18, 2023

Conversation

flrgh
Copy link
Contributor

@flrgh flrgh commented Jul 13, 2023

Here we go, folks. At long last, it's wasm time.

overview

This integrates Kong/ngx_wasm_module into Kong gateway.

build system

nginx module

The nginx module and its deps are built by bazel, of course.

There are several different flags/settings that will tweak build-time behavior and produce different permutations of the final package:

  • --//:wasmx=(true|false) - enables/disables wasm (default: true)
  • --//:wasmx_module_flag=(static|dynamic) - enables building as a static or dynamic module (default dynamic)
  • --//:wasmx_runtime=(wasmtime|wasmer|v8) - selects which wasm runtime to build and ship with (default: wasmtime)

These options are all valuable for testing and supporting different permutations of wasmx, but for the time being they are all intended to be left default.

Why the dynamic module?

Since the nginx module is still relatively new and hasn't been battle-tested, the choice was made to ship it as a dynamic module. This way it can be fully disabled if wasm is not turned on in kong.conf. The lua runtime bits obviously still remain, but they were designed to be as minimal and discrete as possible when wasm is not enabled. As of this writing, we don't even require the FFI lua libs unless wasm is turned on.

dev dependencies

Adequately testing this feature requires using a real wasm filter. We currently have two of them at spec/fixtures/proxy_wasm_filters. Building these filters is handled by a github action in CI and script during local development.

db

This change introduces one new DB entity, filter_chains. A filter_chain entity represents a collection of wasm filters (and their respective configurations) which may be associated with either a service or a route.

proxy / runtime

The entrypoint for controlling most runtime behavior is the kong.runloop.wasm module, which follows patterns that are similar to the router and plugins iterator. It has two main responsibilities:

  • build: iterate over filter_chain entities in the DB and compile them into filter execution "plans", which are initialized via FFI calls to ngx_wasm_module. This happens at startup (init_worker) and on config change, via event listeners.
  • attach: in the proxy path, perform lookups of the current service/route against the pre-built state/registry of filter execution plans, and if any are found, call into ngx_wasm_module (FFI) to attach the plan to the request.

configuration

There are two new config parameters in kong.conf:

  • wasm - this is the on/off switch
  • wasm_filters_path - the user is intended to point this to a directory on the local filesystem containing their wasm filters so that they can be discovered at startup

Additionally, the nginx module provides a litany of nginx directives, so we will be providing a means of injecting them via nginx_wasm_* config attributes.

reviewers, fear not

The line count for this PR is staggering, yes. However, as of this writing the diff of kong/**.lua is just over 1k lines, which isn't too bad for a brand new feature.

The bulk of the changes are in tests and test fixtures/dependencies, which really shouldn't need super intense review.

Following tests, the next biggest source of change are build items.

todo

These things should be completed in this context or captured in a ticket/separate PR

  • changelog
  • review comments, docstrings, etc for grammar, typos, coherence
  • user-facing docs
  • update openapi spec
  • working arm64 build

kong/templates/nginx.lua Outdated Show resolved Hide resolved
kong/templates/nginx.lua Outdated Show resolved Hide resolved
kong/api/routes/filter_chains.lua Outdated Show resolved Hide resolved
kong/conf_loader/init.lua Show resolved Hide resolved
kong/conf_loader/init.lua Show resolved Hide resolved
kong/conf_loader/init.lua Outdated Show resolved Hide resolved
kong/conf_loader/init.lua Outdated Show resolved Hide resolved
kong/runloop/wasm.lua Show resolved Hide resolved
kong/runloop/wasm.lua Show resolved Hide resolved
kong/runloop/wasm.lua Show resolved Hide resolved
kong/runloop/wasm.lua Outdated Show resolved Hide resolved
kong/runloop/wasm.lua Outdated Show resolved Hide resolved
@gszr
Copy link
Member

gszr commented Jul 18, 2023

@flrgh and I will do a commit history clean up and merge this in a couple of hours.

@flrgh
Copy link
Contributor Author

flrgh commented Jul 18, 2023

Okay weird, after rebasing on master, we have a failing clustering test. Seems f3ceb74 leaves us broken 🤨

2023/07/18 11:17:34 [error] 2785660#0: *7 [lua] data_plane.lua:225: [clustering] unable to update running config: nil, context: ngx.timer

cc @hishamhm

"NGX_WASM_RUNTIME_LIB": "$$EXT_BUILD_ROOT$$/external/v8/lib",
"NGX_WASM_RUNTIME_INC": "$$EXT_BUILD_ROOT$$/external/v8/include",
# https://github.com/Kong/ngx_wasm_module/blob/0f07c712c48d410190ec5e0cc0b34fdfd190387d/t/10-build/003-dynamic_module.t#L43
"NGX_WASM_RUNTIME_LD_OPT": "$$EXT_BUILD_ROOT$$/external/v8/lib/libwee8.a -lv8bridge -lstdc++ -lm -ldl -lpthread",
Copy link
Member

@thibaultcha thibaultcha Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm the build step does not need these LD_OPT environment variables, they are designed to work without having to specify these options. I just tested removing these linker options locally and it worked. This is how the build step is documented in our INSTALL.md. This is because these flags are already added by our config script, and then appended to Nginx build flags belonging to the runtime here.

These tests in the linked suite should probably be refactored to reflect that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok I think I understand better post-lunch haha. These are for the static linking part. I think they should move to the above --with-ld-opt= flags, indicating that Kong's Bazel build system follows ngx_wasm_module's standard INSTALL.md compilation method: ./configure flags and an NGX_WASM_RUNTIME env variable.

Copy link
Member

@thibaultcha thibaultcha Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only difference with my initial comment is that only $$EXT_BUILD_ROOT$$/external/v8/lib/libwee8.a should be necessary, -lv8bridge -lstdc++ -lm -ldl -lpthread are all automatically added already. For statically linking a runtime, only --with-ld-opt=/path/to/runtime.a should be needed. At least that's the expectation.

Having these options duplicated won't be helpful in the future, I think we should remove them even if in a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for drawing attention to this. There was a lot of throwing things against the wall to see what stuck whilst working on the build/CI integration, so it's unsurprising to me that there is some unnecessary cruft in here.

NGX_WASM_MODULE_BRANCH=main
WASMER_VERSION=3.1.1
WASMTIME_VERSION=8.0.1
V8_VERSION=10.5.18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the current version the module is tested for today is 11.4.183.23

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be addressed when we will release ngx_wasm_module prerelease-0.1.0, which will pin the supported runtime versions.

# although this is centos7 specific, the flag will work on any GNU linker
# we place it here to skip macos, which uses darwin ld
# https://github.com/Kong/ngx_wasm_module/commit/e70a19f53e1dda99d016c5cfa393652720959afd
"--with-ld-opt=\"-Wl,--allow-multiple-definition\"",
Copy link
Member

@thibaultcha thibaultcha Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it is a good idea to unconditionally add this flag... I mean, besides not targeting macos. Do we maintain centos7 packages for Kong Gateway?

hishamhm added a commit that referenced this pull request Jul 18, 2023
This seems to happen only during Hybrid mode CP/DP communication.
A test that will be merged as part of #11218 covers this code.
"@kong//:wasmx_v8": {
"NGX_WASM_RUNTIME": "v8",
"NGX_WASM_RUNTIME_LIB": "$$EXT_BUILD_ROOT$$/external/v8/lib",
"NGX_WASM_RUNTIME_INC": "$$EXT_BUILD_ROOT$$/external/v8/include",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the same actually applies to the other _LIB, _INC env variables? Since they are already specified above as --with-cc-opt and --with-ld-opt. There is nothing else that setting these env variables will do, it's only a convenience method instead of these --with-xx-opt flags. Only NGX_WASM_RUNTIME is needed, unless I missed something larger about how this is invoked in Bazel. But try and remove RUNTIME_{INC,LIB,LD_OPT} and it should still all work as-is.

flrgh pushed a commit that referenced this pull request Jul 18, 2023
This seems to happen only during Hybrid mode CP/DP communication.
A test that will be merged as part of #11218 covers this code.
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will circle back on Thibault's comments in follow-up PRs. Thanks for the reviews, Thibault!

@flrgh flrgh merged commit 4a634e0 into master Jul 18, 2023
24 checks passed
@flrgh flrgh deleted the feat/wasmx branch July 18, 2023 20:16
gszr added a commit that referenced this pull request Jul 19, 2023
The Kong Wasm integration inherits certain configuration properties
defined for Kong; among others, it inherits injected Nginx directives.
As implemented in #11218, the
integration inherited certain directives from the `ngx_http_proxy_module`,
while  the desired behavior is to inherit from the `ngx_lua_module`, so
that Lua and Wasm extensions operate under the same settings. This commit
implements that change.
gszr added a commit that referenced this pull request Jul 19, 2023
The Kong Wasm integration inherits certain configuration properties
defined for Kong; among others, it inherits injected Nginx directives.
As implemented in #11218, the
integration inherited certain directives from the `ngx_http_proxy_module`,
while  the desired behavior is to inherit from the `ngx_lua_module`, so
that Lua and Wasm extensions operate under the same settings. This commit
implements that change.
gszr added a commit that referenced this pull request Jul 19, 2023
The Kong Wasm integration inherits certain configuration properties
defined for Kong; among others, it inherits injected Nginx directives.
As implemented in #11218, the
integration inherited certain directives from the `ngx_http_proxy_module`,
while  the desired behavior is to inherit from the `ngx_lua_module`, so
that Lua and Wasm extensions operate under the same settings. This commit
implements that change.
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.

5 participants