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(wasm): support bundled filters #12843

Merged
merged 1 commit into from
Apr 24, 2024
Merged

feat(wasm): support bundled filters #12843

merged 1 commit into from
Apr 24, 2024

Conversation

flrgh
Copy link
Contributor

@flrgh flrgh commented Apr 9, 2024

Summary

An upcoming Kong release will ship with at least one bundled Wasm filter.

Previously there was no concept of "bundled" filters: all filters must be provided by the user in the wasm_filters_path directory.

This introduces a new wasm_filters kong.conf property. It is a comma-delimited array that functions quite similarly to the plugins property with the addition of a special user value:

# all filters disabled
wasm_filters = off

# all bundled filters enabled
wasm_filters = bundled

# all user filters (in `wasm_filters_path`) enabled
wasm_filters = user

# all available filters enabled
wasm_filters = bundled, user

# specific filters enabled
wasm_filters = some_bundled_filter, some_user_supplied_filter

In the case of a collision between bundled and user filters, the conf loader will prefer the user filter and log a warning.

Checklist

Issue reference

KAG-4211

@github-actions github-actions bot added core/templates core/configuration cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Apr 9, 2024
@flrgh flrgh added this to the 3.7.0 milestone Apr 9, 2024
@flrgh flrgh added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) core/wasm Everything relevant to [proxy-]wasm labels Apr 9, 2024
@flrgh flrgh marked this pull request as ready for review April 9, 2024 22:44
@flrgh
Copy link
Contributor Author

flrgh commented Apr 9, 2024

The code for this is AFAICT feature-complete and should be considered ready-for-review, but this should not be merged until after the distribution work for bundled filters is done so that I can update the tests accordingly.

(Also I have no idea if my hack for enabling bundled filters in local dev is going to work yet.)

@flrgh flrgh requested a review from hishamhm April 9, 2024 22:45
kong/templates/kong_defaults.lua Outdated Show resolved Hide resolved
kong/constants.lua Outdated Show resolved Hide resolved
kong/conf_loader/init.lua Outdated Show resolved Hide resolved
@flrgh
Copy link
Contributor Author

flrgh commented Apr 16, 2024

Currently rebased onto #12853. Once that is merged this PR will get a bit smaller.

@chobits chobits self-requested a review April 17, 2024 07:59
@flrgh flrgh force-pushed the feat/bundled-wasm-filters branch from 7f132c2 to b5efbd0 Compare April 19, 2024 20:14
@flrgh flrgh removed the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Apr 19, 2024
kong/constants.lua Outdated Show resolved Hide resolved
@flrgh flrgh force-pushed the feat/bundled-wasm-filters branch from 030775e to d78ea4c Compare April 22, 2024 17:47
@kikito
Copy link
Member

kikito commented Apr 23, 2024

This pr has some debug messages on it, I am moving it to Draft until that is addressed. @flrgh please move it back to "Ready to review" when ready.

@kikito kikito marked this pull request as draft April 23, 2024 13:47
@flrgh flrgh force-pushed the feat/bundled-wasm-filters branch 2 times, most recently from cc4b8d4 to b3a54fc Compare April 23, 2024 16:44
@flrgh flrgh marked this pull request as ready for review April 23, 2024 17:12
@brentos
Copy link
Contributor

brentos commented Apr 23, 2024

I'm a bit late for adding my $0.02 on this, but I think some sort of namespacing of filters could nicely solve the collision issue. For example:

kong.datakit
kong.response_transformer
foocompany.datakit
foocompany.response_transfomer

Then we could enable with

wasm_filters = kong.datakit, foocompany.datakit

In addition, setting up some standard file locations for these such that we don't have to configure a base path, and these filters could be installed at:

<default_filter_dir>/kong/datakit
<default_filter_dir>/foocompany/datakit

@hishamhm
Copy link
Contributor

@brentos Users can name filters as they like, so they can definitely use namespacing to avoid collisions. 👍 This is more about allowing for "intentional" collisions, that is, for when a user wants to override a bundled filter that is shipped with Kong (for example, with a more recent version), without having to disable bundled filters altogether.

@@ -177,6 +177,8 @@ describe("#wasm - hybrid mode #postgres", function()
res = res:read_body(),
}
end)
.with_timeout(60)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: all of these timeout changes

This is to account for the increased NGINX startup time incurred from loading the new datakit module. Locally I'm seeing about ~3.6s pretty consistently, but timing in CI (where we are often saturating the CPU) is slower and very inconsistent, so a large timeout is selected to keep the test from being flaky.

There is some work in progress to mitigate the slow startup time: Kong/ngx_wasm_module#536

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: because module loading is an init_worker thing, the privileged agent worker is also delayed on startup, so this affects how long it takes for the DP to establish a connection to the CP. I believe that is why this test suite is struggling so much more than others. Changes:

  1. added some more logfile assertions to watch for a module load event (technically this only guarantees that 1/N processes has finished init_worker, but it's still somewhat helpful).
  2. adjusted the worker procs to 2
  3. disabled bundled filters in this test suite because datakit is the biggest source of slow init_worker

@locao locao force-pushed the feat/bundled-wasm-filters branch from cb21380 to f5a317b Compare April 23, 2024 21:30
@flrgh flrgh added the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Apr 23, 2024
@flrgh
Copy link
Contributor Author

flrgh commented Apr 23, 2024

investigating something, please do not merge

edit: okay I think the tests should be a lot more stable now, good to merge when 🟢

@flrgh flrgh force-pushed the feat/bundled-wasm-filters branch 2 times, most recently from b5154c4 to 2d490a5 Compare April 23, 2024 22:24
@flrgh flrgh removed the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Apr 23, 2024
@locao locao force-pushed the feat/bundled-wasm-filters branch from 2d490a5 to 99901c6 Compare April 24, 2024 14:11
@flrgh flrgh merged commit 1e90a31 into master Apr 24, 2024
25 checks passed
@flrgh flrgh deleted the feat/bundled-wasm-filters branch April 24, 2024 15:43
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

tysoekong pushed a commit that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/bazel cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/configuration core/templates core/wasm Everything relevant to [proxy-]wasm size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants