feat(*): add optional wasm filter config validation #11568

flrgh · 2023-09-14T01:09:02Z

Changes:

introduces a new json field type with JSON schema-powered validation
implement discovery and import of {{ filter }}.meta.json files for providing Kong with Wasm filter metadata (such as configuration schema)
Add a new json_config field to filter_chains.filters[] which is optionally validated against JSON schema from the filter metadata file (if present)

kong/db/schema/entities/filter_chains.lua

kong/db/schema/entities/schemas.lua

flrgh · 2023-09-15T00:22:18Z

After a chat with @hishamhm today, a good chunk of this is going to get re-shaped, so I dismissed the pending reviews for now.

flrgh · 2023-09-21T17:18:26Z

kong/db/schema/entities/filter_chains.lua

+    { raw_config = { type = "string", required = false, }, },
+    { enabled    = { type = "boolean", default = true, required = true, }, },
+
+    { config = {
+        type = "json",
+        required = false,
+        json_schema = {
+          parent_subschema_key = "name",
+          namespace = constants.SCHEMA_NAMESPACES.PROXY_WASM_FILTERS,
+          optional = true,
+        },
+      },
+    },


I'm thinking it might be better to reverse this, so config stays the same, and we add a new field called json_config.

It would simplify this PR, because we'd no longer require a migration to rename existing config fields to raw_config.

Users still reap the same convenience benefit of not needing to serialize their configuration, and config/json_config is arguably a bit more intuitive.

It's a little bit more forward-compatible on the off chance we want to support a different serialization format. Having a choice of config | json_config | yaml_config is more intuitive than config | raw_config | yaml_config

I don't think config/raw_config is bad by any stretch, but if we want to go that route we can always do the config => raw_config / json_config => config swap in a smaller PR some time in the coming weeks.

@locao @hishamhm thoughts?

BTW I have a commit for this change that is passing tests locally and ready to be pushed, so it won't add any extra turnaround time if we choose to do this. :)

This feels like the right move for this PR; I'm making a judgment call and pushing that commit. As mentioned, I'm fine with re-introducing the change before the release if we'd like to.

I think it's better for it to remain config, not json_config. At the level of our entity configuration, the meaning of config is that it is our nested configuration data structure, which happens to be validated via JSON Schema and which we happen to serialize as JSON when we pass to the filter. If we ever want to support different serialization formats, we can still use the same config, still validate it using JSON Schema, and then use a new field to specify the serialization format, instead of adding a new data-storage field.

As a side benefit, config helps pushing the JSON-based format as the "preferred default".

Sounds reasonable enough to me; I don't have a strong opinion either way. If you feel like doing some more old school gateway development you can push this through while I'm out next week. Otherwise I don't think it should be too difficult for me to get it over the line in time for feature freeze when I am back at my desk.

flrgh · 2023-09-21T18:24:53Z

scripts/explain_manifest/fixtures/ubuntu-22.04-amd64.txt

@@ -70,6 +70,11 @@
  - libc.so.6
  Runpath   : /usr/local/kong/lib

+- Path      : /usr/local/lib/lua/5.1/cjson.so


Explanation: this feature depends on lua-resty-ljsonschema, which explicitly takes lua-cjson as a dependency, so we're now getting an additional cjson.so in the manifest. On my machine this file is a measly 42K, so we're not blowing up our package/image sizes.

If we want, I can put the lua-resty-ljsonschema addition in its own commit before merging so that it's more clear to future us why this is here.

Update: I did put this in its own commit, but feel free to squash if that makes more sense.

flrgh · 2023-09-21T18:55:20Z

kong/runloop/wasm.lua

@@ -541,18 +580,83 @@ local function get_filter_chain_for_request(route, service)
 end


+---@param filters kong.configuration.wasm_filter[]|nil
+local function discover_filter_metadata(filters)


I think it's just about time to refactor some code out of kong.runloop.wasm and into a new kong.wasm module that is not focused on runloop/proxy things, but that will be done later.

flrgh · 2023-09-21T19:00:40Z

kong/db/schema/metaschema.lua

      end
    end
  end
+
+  if field.type == "json"
+    and field.json_schema
+    and field.json_schema.parent_subschema_key
+  then
+    local parent_subschema_key = field.json_schema.parent_subschema_key
+    local found = false
+
+    for i = 1, #parent_schema.fields do
+      local item = parent_schema.fields[i]
+      local parent_field_name = next(item)
+      local parent_field = item[parent_field_name]
+
+      if parent_subschema_key == parent_field_name then
+        if parent_field.type ~= "string" then
+          errors[k] = errors[k] or {}
+          errors[k].json_schema = {
+            parent_subschema_key = meta_errors.JSON_PARENT_KEY_TYPE
+          }
+        end
+        found = true
+        break
+      end
+    end
+
+    if not found then
+      errors[k] = errors[k] or {}
+      errors[k].json_schema = {
+        parent_subschema_key = meta_errors.JSON_PARENT_KEY
+      }
+      return
+    end
+  end


FYI for the sake of stylistic consistency, this was copy/pasted and modified from the subschema_key metaschema validation code, since it serves a similar function.

flrgh · 2023-09-21T19:51:29Z

Rebased and cleaned up git history. From my side this is ready to go once the tests are green.

omegabytes · 2023-09-21T20:27:04Z

kong/db/schema/entities/filter_chains.lua

+    },
+
+  },
+  entity_checks = {


Is there a reason we don't need at_least_one_of for config, json_config?

There can be filters that take no config.

Yeah, configuration needs to remain optional, as it's perfectly valid to have a proxy-wasm filter that requires no configuration to function.

Specifying a schema for your filter's json_config using this new my-filter.meta.json method is one means of making it effectively required though.

locao

This PR LGTM. Great work, @flrgh!

I left a tiny nitpick comment that can be addressed in a future PR (or never). Also, there is some code style inconsistency, most related to leaving a single line between logical blocks.

Neither are blockers, but I am not approving right now as there is a conflict pending of resolution. I will try to fix it, rebase to latest master and wait for green tests before approving/merging.

locao · 2023-09-28T14:42:30Z

kong/runloop/wasm.lua

+      for _, filter in ipairs(chain.filters) do
+        if filter.enabled then
+          -- serialize all JSON configurations up front
+          if not filter.config and filter.json_config ~= nil then


Suggested change

if not filter.config and filter.json_config ~= nil then

if filter.json_config then

Nit: Per kong.db.schema.entities.filter_chains those fields are already mutually exclusive.

This rock takes lua-cjson as a dependency, so now we have an additional cjson.so file in the manifest. It's looking like about 42K in size, so it shouldn't bloat our packages/images

flrgh requested a review from locao September 14, 2023 01:09

pull-request-size bot added the size/XL label Sep 14, 2023

flrgh requested a review from gszr September 14, 2023 01:09

github-actions bot added core/db/migrations core/db schema-change-noteworthy labels Sep 14, 2023

github-actions bot assigned flrgh Sep 14, 2023

flrgh commented Sep 14, 2023

View reviewed changes

kong/db/schema/entities/filter_chains.lua Outdated Show resolved Hide resolved

flrgh commented Sep 14, 2023

View reviewed changes

kong/db/schema/entities/schemas.lua Outdated Show resolved Hide resolved

flrgh commented Sep 14, 2023

View reviewed changes

kong/db/schema/entities/schemas.lua Outdated Show resolved Hide resolved

dndx mentioned this pull request Sep 14, 2023

feat(changelog-tool): add changelog command line tool Kong/gateway-changelog#1

Merged

flrgh removed request for gszr and locao September 15, 2023 00:21

flrgh added the pr/do not merge label Sep 15, 2023

flrgh force-pushed the feat/wasm-filter-schemas branch from 02cee27 to fbba361 Compare September 19, 2023 22:06

pull-request-size bot removed the size/XL label Sep 19, 2023

github-actions bot added core/proxy core/admin-api size/XL labels Sep 19, 2023

pull-request-size bot added size/XXL and removed size/XL labels Sep 19, 2023

flrgh force-pushed the feat/wasm-filter-schemas branch 3 times, most recently from b9f6c73 to bc03afe Compare September 20, 2023 01:33

flrgh requested review from hishamhm and locao September 20, 2023 01:43

flrgh force-pushed the feat/wasm-filter-schemas branch 2 times, most recently from b7bf6e7 to 8496922 Compare September 20, 2023 16:09

github-actions bot removed the core/db/migrations label Sep 21, 2023

flrgh force-pushed the feat/wasm-filter-schemas branch 2 times, most recently from 19ff350 to 8399ea2 Compare September 21, 2023 02:26

github-actions bot removed the core/admin-api label Sep 21, 2023

flrgh force-pushed the feat/wasm-filter-schemas branch 2 times, most recently from a23a528 to 8d86e51 Compare September 21, 2023 02:33

flrgh changed the title ~~feat(wasm): add optional schema validation for filter configs~~ feat(*): add optional wasm filter config validation Sep 21, 2023

flrgh force-pushed the feat/wasm-filter-schemas branch from f811372 to a4e646b Compare September 21, 2023 03:38

flrgh commented Sep 21, 2023

View reviewed changes

flrgh force-pushed the feat/wasm-filter-schemas branch from 22bb0ed to 017ee6c Compare September 21, 2023 19:51

flrgh removed the pr/do not merge label Sep 21, 2023

omegabytes reviewed Sep 21, 2023

View reviewed changes

flrgh marked this pull request as ready for review September 21, 2023 23:29

team-eng-enablement added author/community PRs from the open-source community (not Kong Inc) and removed author/community PRs from the open-source community (not Kong Inc) labels Sep 26, 2023

locao reviewed Sep 28, 2023

View reviewed changes

chore(deps): add lua-resty-ljsonschema 1.1.6

c88ea85

This rock takes lua-cjson as a dependency, so now we have an additional cjson.so file in the manifest. It's looking like about 42K in size, so it shouldn't bloat our packages/images

flrgh force-pushed the feat/wasm-filter-schemas branch from 017ee6c to 0e3f293 Compare October 2, 2023 16:17

feat(*): add optional wasm filter config validation

5dea9ef

flrgh force-pushed the feat/wasm-filter-schemas branch from 0e3f293 to 5dea9ef Compare October 2, 2023 16:22

locao approved these changes Oct 2, 2023

View reviewed changes

flrgh merged commit 311f807 into master Oct 2, 2023
24 checks passed

flrgh deleted the feat/wasm-filter-schemas branch October 2, 2023 18:10

flrgh mentioned this pull request Oct 6, 2023

feat(wasm): combine config and json_config fields #11697

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(*): add optional wasm filter config validation #11568

feat(*): add optional wasm filter config validation #11568

flrgh commented Sep 14, 2023 •

edited

Loading

flrgh commented Sep 15, 2023

flrgh Sep 21, 2023

flrgh Sep 21, 2023

flrgh Sep 21, 2023

hishamhm Sep 21, 2023

flrgh Sep 21, 2023 •

edited

Loading

flrgh Sep 21, 2023

flrgh Sep 21, 2023

flrgh Sep 21, 2023

flrgh Sep 21, 2023

flrgh commented Sep 21, 2023

omegabytes Sep 21, 2023

hishamhm Sep 21, 2023

flrgh Sep 21, 2023

locao left a comment

locao Sep 28, 2023

	if not filter.config and filter.json_config ~= nil then
	if filter.json_config then

feat(*): add optional wasm filter config validation #11568

feat(*): add optional wasm filter config validation #11568

Conversation

flrgh commented Sep 14, 2023 • edited Loading

flrgh commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flrgh Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flrgh commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

locao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flrgh commented Sep 14, 2023 •

edited

Loading

flrgh Sep 21, 2023 •

edited

Loading