feat: add wasm filter-chain entity support #987
Conversation
hishamhm
force-pushed
the
feat/filter-chains
branch
from
9b72eaf
to
fe5e0bc
Compare
|
|
flrgh
force-pushed
the
feat/filter-chains
branch
2 times, most recently
from
a84e1ea
to
f9c3ca5
Compare
|
See also: Kong/go-database-reconciler#72 |
flrgh
force-pushed
the
feat/filter-chains
branch
from
f9c3ca5
to
3b9a749
Compare
flrgh
force-pushed
the
feat/filter-chains
branch
from
3b9a749
to
08a2be1
Compare
flrgh
force-pushed
the
feat/filter-chains
branch
2 times, most recently
from
fc9da65
to
071d975
Compare
|
Edit: both of these are done. |
flrgh
changed the title
flrgh
force-pushed
the
feat/filter-chains
branch
4 times, most recently
from
907e29f
to
b6dbadd
Compare
| workspace := client.Workspace() | ||
| client.SetWorkspace("") | ||
| defer client.SetWorkspace(workspace) |
There was a problem hiding this comment.
This feels like a mega hack, but I get test failures without it because sometimes tests run against a workspace that doesn't exist. Is there a better way?
There was a problem hiding this comment.
I added a comment related to this here: https://github.com/Kong/deck/pull/987/files#r1629376245
In case we need to do this check, do you think we can modify the fetchKongVersion function to also deal with it?
There was a problem hiding this comment.
Ah, so fetchKongVersion() is actually where I got the idea for this. It does something similar by trying to fetch / before trying the /{workspace} endpoint.
There was a problem hiding this comment.
I suggested to reuse fetchKongVersion() so that we will not have to run the same query twice.
Also, the current code would fail with 401 if decK doesn't have the permissions to query the default workspace.
flrgh
force-pushed
the
feat/filter-chains
branch
2 times, most recently
from
f88707c
to
9f2c566
Compare
|
re:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #987 +/- ##
==========================================
- Coverage 22.42% 22.18% -0.25%
==========================================
Files 54 54
Lines 4508 4558 +50
==========================================
Hits 1011 1011
- Misses 3397 3447 +50
Partials 100 100 ☔ View full report in Codecov by Sentry. |
|
👋 @GGabriele do you think you could review or help source a reviewer for this PR? |
| @@ -318,6 +325,9 @@ func syncMain(ctx context.Context, filenames []string, dry bool, parallelism, | |||
| if err := checkForRBACResources(*rawState, dumpConfig.RBACResourcesOnly); err != nil { | |||
| return err | |||
| } | |||
| if err := checkFilterChainsAllowed(*rawState, dumpConfig, parsedKongVersion); err != nil { | |||
There was a problem hiding this comment.
Is this even needed? If decK attempts to create a filter chain on a Gateway that doesn't support it, can't we just return the API error back to the caller?
Right now decK is mostly best effort when it comes to features driven by Kong configuration, for example decK doesn't check whether a custom plugin exists before attempting to configure it.
There was a problem hiding this comment.
If we don't do some kind of feature detection, pretty much all deck commands will throw an error when executing against a Kong node without wasm support (too old version, not turned on, etc) because GET /filter-chains will return 404/405.
So you are correct that checkFilterChainsAllowed is not strictly required, but determineFilterChainSupport (feature detection) is. I would prefer to keep the checkFilterChainsAllowed guard in place because it's nicer on the user to fail fast and protects against things like incomplete sync, but I can remove it if you think it's just not a good fit with the rest of the codebase.
There was a problem hiding this comment.
GET /filter-chains will return 404/405.
For reference, we do this already for Consumer Groups: https://github.com/Kong/go-database-reconciler/blob/main/pkg/dump/dump.go#L90
I'm okay if you prefer to do this extra check though.
There was a problem hiding this comment.
Ahhhhh interesting. I think I prefer it that way actually. I'll do some testing and push this logic to go-database-reconciler.
This brings support for wasm filter chains to deck.
Most of the groundwork for this was laid in the other repos:
Wasm filters were added in 3.4 and are an optional feature. Feature detection in deck works by fetching the root admin API endpoint and inspecting the configuration to see if
wasmis enabled (this works for versions <3.4 and versions >=3.4 with wasm disabled). Feature detection does not run in Konnect mode; therefore Konnect is not yet supported, only Kong gateway OSS/EEFor the sake of keeping things reasonable in size (we're at >3k new lines of code if you include the other 2 PRs), there are some validation steps that have been left out:
These things are of course still validated by the Kong admin API, but at a later date it will be possible to validate them client-side for better UX.
KAG-4005
KOKO-1275