From 33dbd4f17c713ea0a2500f14901b0d30673bed28 Mon Sep 17 00:00:00 2001 From: Michael Martin Date: Fri, 28 Jul 2023 11:58:42 -0700 Subject: [PATCH] fix(wasm): validate filters in all persistence modes (#11288) The primary purpose of this change is to ensure that each .filters[].name is validated against available/installed filters in dbless mode (this was only working in db mode prior). I also added an additional entity check which will return a more descriptive error when wasm is disabled or no filters are present (this is more useful for dbless mode than anything). --- kong-3.5.0-0.rockspec | 1 - kong/clustering/init.lua | 5 +- kong/db/dao/filter_chains.lua | 108 -------- kong/db/schema/entities/filter_chains.lua | 24 +- kong/db/schema/init.lua | 5 +- kong/db/schema/metaschema.lua | 1 + kong/init.lua | 2 - kong/runloop/wasm.lua | 117 +++++++-- .../20-wasm/01-admin-api_spec.lua | 12 +- spec/02-integration/20-wasm/02-db_spec.lua | 18 +- .../20-wasm/03-runtime_spec.lua | 14 +- .../20-wasm/04-proxy-wasm_spec.lua | 6 +- .../20-wasm/05-cache-invalidation_spec.lua | 9 +- .../20-wasm/07-reports_spec.lua | 4 +- .../20-wasm/08-declarative_spec.lua | 246 ++++++++++++++++++ 15 files changed, 409 insertions(+), 163 deletions(-) delete mode 100644 kong/db/dao/filter_chains.lua create mode 100644 spec/02-integration/20-wasm/08-declarative_spec.lua diff --git a/kong-3.5.0-0.rockspec b/kong-3.5.0-0.rockspec index a4faa4b63846..96c8e50785ca 100644 --- a/kong-3.5.0-0.rockspec +++ b/kong-3.5.0-0.rockspec @@ -201,7 +201,6 @@ build = { ["kong.db.schema"] = "kong/db/schema/init.lua", ["kong.db.dao.keys"] = "kong/db/dao/keys.lua", ["kong.db.dao.key_sets"] = "kong/db/dao/key_sets.lua", - ["kong.db.dao.filter_chains"] = "kong/db/dao/filter_chains.lua", ["kong.db.schema.entities.keys"] = "kong/db/schema/entities/keys.lua", ["kong.db.schema.entities.key_sets"] = "kong/db/schema/entities/key_sets.lua", ["kong.db.schema.entities.consumers"] = "kong/db/schema/entities/consumers.lua", diff --git a/kong/clustering/init.lua b/kong/clustering/init.lua index 6cf89e283e89..f09a194e3e4f 100644 --- a/kong/clustering/init.lua +++ b/kong/clustering/init.lua @@ -6,6 +6,7 @@ local pl_tablex = require("pl.tablex") local clustering_utils = require("kong.clustering.utils") local events = require("kong.clustering.events") local clustering_tls = require("kong.clustering.tls") +local wasm = require("kong.runloop.wasm") local assert = assert @@ -102,8 +103,8 @@ function _M:init_worker() end, plugins_list) local filters = {} - if kong.db.filter_chains.filters then - for _, filter in ipairs(kong.db.filter_chains.filters) do + if wasm.enabled() and wasm.filters then + for _, filter in ipairs(wasm.filters) do filters[filter.name] = { name = filter.name } end end diff --git a/kong/db/dao/filter_chains.lua b/kong/db/dao/filter_chains.lua deleted file mode 100644 index d7e54dc601f5..000000000000 --- a/kong/db/dao/filter_chains.lua +++ /dev/null @@ -1,108 +0,0 @@ -local filter_chains = {} - -local insert = table.insert -local fmt = string.format - -local EMPTY = {} - - -local function check_enabled_filters(self, chain) - if not self.filters then - local err_t = self.errors:schema_violation({ - filters = "no wasm filters are configured", - }) - return nil, tostring(err_t), err_t - end - - if type(chain.filters) ~= "table" then - return true - end - - local errs - - for i, filter in ipairs(chain.filters) do - local name = filter.name - - -- let the standard schema validation catch invalid name errors - if type(name) == "string" - and not self.filters_by_name[name] - then - errs = errs or {} - errs[i] = { name = "no such filter: " .. filter.name } - end - end - - if errs then - local err_t = self.errors:schema_violation({ - filters = errs, - }) - return nil, tostring(err_t), err_t - end - - return true -end - - -function filter_chains:load_filters(wasm_filters) - local filters = {} - local filters_by_name = {} - - local errors = {} - - for i, filter in ipairs(wasm_filters or EMPTY) do - insert(filters, filter) - - if type(filter.name) ~= "string" then - insert(errors, fmt("filter #%d name is not a string", i)) - - elseif filters_by_name[filter.name] then - insert(errors, fmt("duplicate filter name (%s) at #%d", filter.name, i)) - - else - filters_by_name[filter.name] = filter - - end - end - - if #errors > 0 then - return nil, "failed to load filters: " .. table.concat(errors, ", ") - end - - self.filters = filters - self.filters_by_name = filters_by_name - - return true -end - - -function filter_chains:insert(entity, options) - local ok, err, err_t = check_enabled_filters(self, entity) - if not ok then - return nil, err, err_t - end - - return self.super.insert(self, entity, options) -end - - -function filter_chains:update(primary_key, entity, options) - local ok, err, err_t = check_enabled_filters(self, entity) - if not ok then - return nil, err, err_t - end - - return self.super.update(self, primary_key, entity, options) -end - - -function filter_chains:upsert(primary_key, entity, options) - local ok, err, err_t = check_enabled_filters(self, entity) - if not ok then - return nil, err, err_t - end - - return self.super.upsert(self, primary_key, entity, options) -end - - -return filter_chains diff --git a/kong/db/schema/entities/filter_chains.lua b/kong/db/schema/entities/filter_chains.lua index 5710632cd02d..001aea9782dd 100644 --- a/kong/db/schema/entities/filter_chains.lua +++ b/kong/db/schema/entities/filter_chains.lua @@ -1,4 +1,6 @@ local typedefs = require "kong.db.schema.typedefs" +local wasm = require "kong.runloop.wasm" + ---@class kong.db.schema.entities.filter_chain : table --- @@ -20,10 +22,12 @@ local typedefs = require "kong.db.schema.typedefs" ---@field enabled boolean ---@field config string|table|nil + local filter = { type = "record", fields = { - { name = { type = "string", required = true, }, }, + { name = { type = "string", required = true, one_of = wasm.filter_names, + err = "no such filter", }, }, { config = { type = "string", required = false, }, }, { enabled = { type = "boolean", default = true, required = true, }, }, }, @@ -37,7 +41,6 @@ return { admin_api_name = "filter-chains", generate_admin_api = true, workspaceable = true, - dao = "kong.db.dao.filter_chains", cache_key = { "route", "service" }, fields = { @@ -65,5 +68,22 @@ return { "route", } }, + + -- This check is for user experience and is not strictly necessary to + -- validate filter chain input. + -- + -- The `one_of` check on `filters[].name` already covers validation, but in + -- the case where wasm is disabled or no filters are installed, this check + -- adds an additional entity-level error (e.g. "wasm support is not enabled" + -- or "no wasm filters are available"). + -- + -- All of the wasm API routes are disabled when wasm is also disabled, so + -- this primarily serves the dbless `/config` endpoint. + { custom_entity_check = { + field_sources = { "filters" }, + run_with_invalid_fields = true, + fn = wasm.status, + }, + }, }, } diff --git a/kong/db/schema/init.lua b/kong/db/schema/init.lua index 122578824b1f..380c9e0438ae 100644 --- a/kong/db/schema/init.lua +++ b/kong/db/schema/init.lua @@ -1221,7 +1221,10 @@ local function run_entity_check(self, name, input, arg, full_check, errors) end -- Don't run check if any of its fields has errors - if not all_ok and not checker.run_with_invalid_fields then + if not all_ok + and not checker.run_with_invalid_fields + and not arg.run_with_invalid_fields + then return end diff --git a/kong/db/schema/metaschema.lua b/kong/db/schema/metaschema.lua index 2d8ea7b1afce..50fa65ca2ace 100644 --- a/kong/db/schema/metaschema.lua +++ b/kong/db/schema/metaschema.lua @@ -224,6 +224,7 @@ local entity_checkers = { { field_sources = { type = "array", elements = { type = "string" } } }, { fn = { type = "function" } }, { run_with_missing_fields = { type = "boolean" } }, + { run_with_invalid_fields = { type = "boolean" } }, } } }, diff --git a/kong/init.lua b/kong/init.lua index 97e9798a9fdd..811d16ebd25e 100644 --- a/kong/init.lua +++ b/kong/init.lua @@ -628,8 +628,6 @@ function Kong.init() -- Load plugins as late as possible so that everything is set up assert(db.plugins:load_plugin_schemas(config.loaded_plugins)) - assert(db.filter_chains:load_filters(config.wasm_modules_parsed)) - if is_stream_module then stream_api.load_handlers() end diff --git a/kong/runloop/wasm.lua b/kong/runloop/wasm.lua index d9a10ca4aecc..08ef1ffc9bf2 100644 --- a/kong/runloop/wasm.lua +++ b/kong/runloop/wasm.lua @@ -1,4 +1,28 @@ -local _M = {} +local _M = { + -- these filter lookup tables are created once and then reset/re-used when + -- `wasm.init()` is called. This means other modules are permitted to stash + -- a reference to them, which helps to avoid several chicken/egg dependency + -- ordering issues. + + ---@type kong.configuration.wasm_filter[] + filters = {}, + + ---@type table + filters_by_name = {}, + + ---@type string[] + filter_names = {}, +} + + +--- This represents a wasm module discovered by the conf_loader in +--- `kong.configuration.wasm_filters_path` +--- +---@class kong.configuration.wasm_filter +--- +---@field name string +---@field path string + local utils = require "kong.tools.utils" local dns = require "kong.tools.dns" @@ -46,8 +70,13 @@ local TYPE_SERVICE = 0 local TYPE_ROUTE = 1 local TYPE_COMBINED = 2 +local STATUS_DISABLED = "wasm support is not enabled" +local STATUS_NO_FILTERS = "no wasm filters are available" +local STATUS_ENABLED = "wasm support is enabled" local ENABLED = false +local STATUS = STATUS_DISABLED + local hash_chain do @@ -513,33 +542,79 @@ local function get_filter_chain_for_request(route, service) end +---@param filters kong.configuration.wasm_filter[]|nil +local function set_available_filters(filters) + clear_tab(_M.filters) + clear_tab(_M.filters_by_name) + clear_tab(_M.filter_names) + + if filters then + for i, filter in ipairs(filters) do + _M.filters[i] = filter + _M.filters_by_name[filter.name] = filter + _M.filter_names[i] = filter.name + end + end +end + + +---@param reason string +local function disable(reason) + set_available_filters(nil) + + _G.dns_client = nil + + ENABLED = false + STATUS = reason or STATUS_DISABLED +end + + +local function enable(kong_config) + set_available_filters(kong_config.wasm_modules_parsed) + + -- setup a DNS client for ngx_wasm_module + _G.dns_client = _G.dns_client or dns(kong_config) + + proxy_wasm = proxy_wasm or require "resty.wasmx.proxy_wasm" + ATTACH_OPTS.isolation = proxy_wasm.isolations.FILTER + + ENABLED = true + STATUS = STATUS_ENABLED +end + + _M.get_version = get_version _M.update_in_place = update_in_place _M.set_state = set_state +function _M.enable(filters) + enable({ + wasm = true, + wasm_modules_parsed = filters, + }) +end ----@param kong_config table -function _M.init(kong_config) - if not kong_config.wasm then - return - end +_M.disable = disable - local modules = kong_config.wasm_modules_parsed - if not modules or #modules == 0 then - return - end - reports.add_immutable_value("wasm_cnt", #modules) +---@param kong_config table +function _M.init(kong_config) + if kong_config.wasm then + local filters = kong_config.wasm_modules_parsed - -- setup a DNS client for ngx_wasm_module - _G.dns_client = dns(kong_config) + if filters and #filters > 0 then + reports.add_immutable_value("wasm_cnt", #filters) + enable(kong_config) - proxy_wasm = require "resty.wasmx.proxy_wasm" - ENABLED = true + else + disable(STATUS_NO_FILTERS) + end - ATTACH_OPTS.isolation = proxy_wasm.isolations.FILTER + else + disable(STATUS_DISABLED) + end end @@ -653,4 +728,14 @@ function _M.enabled() end +---@return boolean? ok +---@return string? error +function _M.status() + if not ENABLED then + return nil, STATUS + end + + return true +end + return _M diff --git a/spec/02-integration/20-wasm/01-admin-api_spec.lua b/spec/02-integration/20-wasm/01-admin-api_spec.lua index 59cf76a735ac..ce318f01c2d2 100644 --- a/spec/02-integration/20-wasm/01-admin-api_spec.lua +++ b/spec/02-integration/20-wasm/01-admin-api_spec.lua @@ -21,17 +21,17 @@ describe("wasm admin API [#" .. strategy .. "]", function() local service, route lazy_setup(function() + require("kong.runloop.wasm").enable({ + { name = "tests" }, + { name = "response_transformer" }, + }) + bp, db = helpers.get_db_utils(strategy, { "routes", "services", "filter_chains", }) - db.filter_chains:load_filters({ - { name = "tests" }, - { name = "response_transformer" }, - }) - service = assert(db.services:insert { name = "wasm-test", url = "http://wasm.test", @@ -111,7 +111,7 @@ describe("wasm admin API [#" .. strategy .. "]", function() local body = assert.response(res).has.jsonbody() assert.same({ data = {}, next = ngx.null }, body) - local chain = assert(bp.filter_chains:insert({ + local chain = assert(bp.filter_chains:insert({ filters = { { name = "tests" } }, service = { id = service.id }, tags = { "a" }, diff --git a/spec/02-integration/20-wasm/02-db_spec.lua b/spec/02-integration/20-wasm/02-db_spec.lua index 2f9ec3703b63..6ac7ca6c73d2 100644 --- a/spec/02-integration/20-wasm/02-db_spec.lua +++ b/spec/02-integration/20-wasm/02-db_spec.lua @@ -10,7 +10,6 @@ describe("wasm DB entities [#" .. strategy .. "]", function() local function reset_db() if not db then return end db.filter_chains:truncate() - db.filter_chains:load_filters({}) db.routes:truncate() db.services:truncate() db.workspaces:truncate() @@ -18,6 +17,11 @@ describe("wasm DB entities [#" .. strategy .. "]", function() lazy_setup(function() + require("kong.runloop.wasm").enable({ + { name = "test" }, + { name = "other" }, + }) + local _ _, db = helpers.get_db_utils(strategy, { "workspaces", @@ -27,10 +31,6 @@ describe("wasm DB entities [#" .. strategy .. "]", function() }) dao = db.filter_chains - dao:load_filters({ - { name = "test", }, - { name = "other", }, - }) end) lazy_teardown(reset_db) @@ -314,8 +314,8 @@ describe("wasm DB entities [#" .. strategy .. "]", function() assert.is_table(err_t.fields) assert.same({ filters = { - [2] = { name = "no such filter: missing" }, - [4] = { name = "no such filter: also-missing" }, + [2] = { name = "no such filter" }, + [4] = { name = "no such filter" }, }, }, err_t.fields) @@ -340,8 +340,8 @@ describe("wasm DB entities [#" .. strategy .. "]", function() assert.is_table(err_t.fields) assert.same({ filters = { - [2] = { name = "no such filter: missing" }, - [4] = { name = "no such filter: also-missing" }, + [2] = { name = "no such filter" }, + [4] = { name = "no such filter" }, }, }, err_t.fields) diff --git a/spec/02-integration/20-wasm/03-runtime_spec.lua b/spec/02-integration/20-wasm/03-runtime_spec.lua index 0d8281897ee4..4802632fb5c2 100644 --- a/spec/02-integration/20-wasm/03-runtime_spec.lua +++ b/spec/02-integration/20-wasm/03-runtime_spec.lua @@ -24,18 +24,16 @@ for _, strategy in helpers.each_strategy({ "postgres", "off" }) do describe("#wasm filter execution (#" .. strategy .. ")", function() lazy_setup(function() - local bp, db = helpers.get_db_utils("postgres", { - "routes", - "services", - "filter_chains", - }) - - - db.filter_chains:load_filters({ + require("kong.runloop.wasm").enable({ { name = "tests" }, { name = "response_transformer" }, }) + local bp = helpers.get_db_utils("postgres", { + "routes", + "services", + "filter_chains", + }) local function service_and_route(name) local service = assert(bp.services:insert({ diff --git a/spec/02-integration/20-wasm/04-proxy-wasm_spec.lua b/spec/02-integration/20-wasm/04-proxy-wasm_spec.lua index 459f4a1e3ed0..e2ce7ca9fe00 100644 --- a/spec/02-integration/20-wasm/04-proxy-wasm_spec.lua +++ b/spec/02-integration/20-wasm/04-proxy-wasm_spec.lua @@ -19,14 +19,16 @@ describe("proxy-wasm filters (#wasm)", function() local hosts_file lazy_setup(function() + require("kong.runloop.wasm").enable({ + { name = "tests" }, + }) + local bp, db = helpers.get_db_utils(DATABASE, { "routes", "services", "filter_chains", }) - db.filter_chains:load_filters({ { name = "tests" } }) - mock_service = assert(bp.services:insert { host = helpers.mock_upstream_host, port = helpers.mock_upstream_port, diff --git a/spec/02-integration/20-wasm/05-cache-invalidation_spec.lua b/spec/02-integration/20-wasm/05-cache-invalidation_spec.lua index 23b3c4d2b7c5..82e17b939dbe 100644 --- a/spec/02-integration/20-wasm/05-cache-invalidation_spec.lua +++ b/spec/02-integration/20-wasm/05-cache-invalidation_spec.lua @@ -196,6 +196,11 @@ describe("#wasm filter chain cache " .. mode_suffix, function() lazy_setup(function() + require("kong.runloop.wasm").enable({ + { name = "tests" }, + { name = "response_transformer" }, + }) + local bp bp, db = helpers.get_db_utils("postgres", { "services", @@ -203,10 +208,6 @@ describe("#wasm filter chain cache " .. mode_suffix, function() "filter_chains", }) - db.filter_chains:load_filters({ - { name = "response_transformer", }, - }) - services.filter = bp.services:insert({ name = hosts.filter }) services.no_filter = bp.services:insert({ name = hosts.no_filter }) diff --git a/spec/02-integration/20-wasm/07-reports_spec.lua b/spec/02-integration/20-wasm/07-reports_spec.lua index 0051d41ae2ec..307dea67e461 100644 --- a/spec/02-integration/20-wasm/07-reports_spec.lua +++ b/spec/02-integration/20-wasm/07-reports_spec.lua @@ -30,7 +30,7 @@ for _, strategy in helpers.each_strategy() do assert(fd:write("127.0.0.1 " .. constants.REPORTS.ADDRESS)) assert(fd:close()) - local bp, db = assert(helpers.get_db_utils(strategy, { + local bp = assert(helpers.get_db_utils(strategy, { "services", "routes", "plugins", @@ -51,7 +51,7 @@ for _, strategy in helpers.each_strategy() do config = {} }) - db.filter_chains:load_filters({ + require("kong.runloop.wasm").enable({ { name = "tests" }, }) diff --git a/spec/02-integration/20-wasm/08-declarative_spec.lua b/spec/02-integration/20-wasm/08-declarative_spec.lua new file mode 100644 index 000000000000..e79473243db9 --- /dev/null +++ b/spec/02-integration/20-wasm/08-declarative_spec.lua @@ -0,0 +1,246 @@ +local helpers = require "spec.helpers" +local cjson = require "cjson" + + +local function post_config(client, config) + config._format_version = config._format_version or "3.0" + + local res = client:post("/config?flatten_errors=1", { + body = config, + headers = { + ["Content-Type"] = "application/json" + }, + }) + + assert.response(res).has.jsonbody() + + assert.logfile().has.no.line("[emerg]", true, 0) + assert.logfile().has.no.line("[crit]", true, 0) + assert.logfile().has.no.line("[alert]", true, 0) + assert.logfile().has.no.line("[error]", true, 0) + assert.logfile().has.no.line("[warn]", true, 0) + + return res +end + + +local function expect_entity_error(res, err) + assert.response(res).has.status(400) + + local json = assert.response(res).has.jsonbody() + assert.is_table(json.flattened_errors) + + local found = false + + + for _, entity in ipairs(json.flattened_errors) do + assert.is_table(entity.errors) + for _, elem in ipairs(entity.errors) do + if elem.type == "entity" then + assert.same(err, elem.message) + found = true + break + end + end + end + + assert.is_true(found, "expected '" .. err .. "' message in response") +end + + +describe("#wasm declarative config", function() + local admin + local proxy + local header_name = "x-wasm-dbless" + + lazy_setup(function() + assert(helpers.start_kong({ + database = "off", + nginx_conf = "spec/fixtures/custom_nginx.template", + wasm = true, + })) + + admin = helpers.admin_client() + proxy = helpers.proxy_client() + end) + + lazy_teardown(function() + if admin then admin:close() end + if proxy then proxy:close() end + helpers.stop_kong() + end) + + it("permits valid filter chain entities", function() + local res = post_config(admin, { + services = { + { name = "test", + url = helpers.mock_upstream_url, + routes = { + { name = "test", + hosts = { "wasm.test" } + }, + }, + filter_chains = { + { name = "test", + filters = { + { name = "response_transformer", + config = cjson.encode { + append = { + headers = { + header_name .. ":hello!" + }, + }, + }, + } + }, + }, + }, + }, + }, + }) + + assert.response(res).has.status(201) + + assert + .eventually(function() + res = proxy:get("/status/200", { + headers = { host = "wasm.test" }, + }) + + res:read_body() + + if res.status ~= 200 then + return nil, { exp = 200, got = res.status } + end + + local header = res.headers[header_name] + + if header == nil then + return nil, header_name .. " header not present in the response" + + elseif header ~= "hello!" then + return nil, { exp = "hello!", got = header } + end + + return true + end) + .is_truthy("filter-chain created by POST /config is active") + end) + + it("rejects filter chains with non-existent filters", function() + local res = post_config(admin, { + services = { + { name = "test", + url = "http://wasm.test/", + filter_chains = { + { name = "test", + filters = { + { name = "i_do_not_exist" } + }, + }, + }, + }, + }, + }) + + assert.response(res).has.status(400) + + local json = assert.response(res).has.jsonbody() + + assert.is_table(json.flattened_errors) + + assert.same(1, #json.flattened_errors) + assert.is_table(json.flattened_errors[1]) + + assert.is_table(json.flattened_errors[1].errors) + assert.same(1, #json.flattened_errors[1].errors) + + local err = assert.is_table(json.flattened_errors[1].errors[1]) + + assert.same("filters.1.name", err.field) + assert.same("field", err.type) + assert.same("no such filter", err.message) + end) +end) + + +describe("#wasm declarative config (no installed filters)", function() + local client + local tmp_dir + + lazy_setup(function() + tmp_dir = helpers.make_temp_dir() + + assert(helpers.start_kong({ + database = "off", + nginx_conf = "spec/fixtures/custom_nginx.template", + wasm = true, + wasm_filters_path = tmp_dir, + })) + + client = helpers.admin_client() + end) + + lazy_teardown(function() + if client then client:close() end + helpers.stop_kong() + helpers.dir.rmtree(tmp_dir) + end) + + it("warns clients that no filters are installed", function() + local res = post_config(client, { + services = { + { name = "test", + url = "http://wasm.test/", + filter_chains = { + { name = "test", + filters = { + { name = "i_do_not_exist" } + }, + }, + }, + }, + }, + }) + + expect_entity_error(res, "no wasm filters are available") + end) +end) + +describe("#wasm declarative config (wasm = off)", function() + local client + + lazy_setup(function() + assert(helpers.start_kong({ + database = "off", + nginx_conf = "spec/fixtures/custom_nginx.template", + wasm = "off", + })) + + client = helpers.admin_client() + end) + + lazy_teardown(function() + if client then client:close() end + helpers.stop_kong() + end) + + it("warns clients that wasm is disabled", function() + local res = post_config(client, { + services = { + { name = "test", + url = "http://wasm.test/", + filter_chains = { + { name = "test", + filters = { + { name = "i_do_not_exist" } + }, + }, + }, + }, + }, + }) + + expect_entity_error(res, "wasm support is not enabled") + end) +end)