From f6148e9b3fcac312dcd36e7e4b9789059897d39d Mon Sep 17 00:00:00 2001 From: Aapo Talvensaari Date: Wed, 15 Jan 2020 20:26:10 +0200 Subject: [PATCH] fix(declarative) schema validation in db-less mode ### Summary This was reported with #5401. When I added DAO transformations, for some reason I ended up writing stupid code that just broke the whole schema validation on declarative. This commit is an attempt to fix that. Sorry for all the pain caused. While fixing this, I also found out that original validation didn't work correctly when you had entity validations that relied on values of foreign keys. This is now also corrected. ### Issues Resolved Fix #5401 --- kong/db/schema/others/declarative_config.lua | 168 +++++---- .../11-declarative_config/03-flatten_spec.lua | 328 ++++++++++++++++++ 2 files changed, 425 insertions(+), 71 deletions(-) diff --git a/kong/db/schema/others/declarative_config.lua b/kong/db/schema/others/declarative_config.lua index e27825753f8f..62f40f8cbf19 100644 --- a/kong/db/schema/others/declarative_config.lua +++ b/kong/db/schema/others/declarative_config.lua @@ -121,8 +121,12 @@ local function add_top_level_entities(fields, entities) end -local function copy_without_foreign(record) +local function copy_record(record, include_foreign) local copy = utils.deep_copy(record, false) + if include_foreign then + return copy + end + for i = #copy.fields, 1, -1 do local f = copy.fields[i] local _, fdata = next(f) @@ -144,10 +148,10 @@ end -- (e.g. `service` as a string key in the `routes` entry). -- @tparam map records A map of top-level record definitions, -- indexable by entity name. These records are modified in-place. -local function nest_foreign_relationships(records) +local function nest_foreign_relationships(records, include_foreign) for entity, record in pairs(records) do for _, f in ipairs(record.fields) do - local fname, fdata = next(f) + local _, fdata = next(f) if fdata.type == "foreign" then local ref = fdata.reference -- allow nested entities @@ -155,7 +159,7 @@ local function nest_foreign_relationships(records) table.insert(records[ref].fields, { [entity] = { type = "array", - elements = copy_without_foreign(record, fname), + elements = copy_record(record, include_foreign), }, }) end @@ -187,7 +191,7 @@ local function reference_foreign_by_name(records) end -local function build_fields(entities) +local function build_fields(entities, include_foreign) local fields = { { _format_version = { type = "string", required = true, eq = "1.1" } }, } @@ -197,7 +201,7 @@ local function build_fields(entities) }) local records = add_top_level_entities(fields, entities) - nest_foreign_relationships(records) + nest_foreign_relationships(records, include_foreign) return fields, records end @@ -482,41 +486,98 @@ local function generate_ids(input, known_entities, parent_entity) end +local function populate_ids(input, known_entities, parent_entity, by_id, by_key) + local by_id = by_id or {} + local by_key = by_key or {} + for _, entity in ipairs(known_entities) do + if type(input[entity]) ~= "table" then + goto continue + end + + local parent_fk + local child_key + if parent_entity then + local parent_schema = all_schemas[parent_entity] + if parent_schema.fields[entity] then + goto continue + end + parent_fk = parent_schema:extract_pk_values(input) + child_key = foreign_children[parent_entity][entity] + end + + local schema = all_schemas[entity] + for _, item in ipairs(input[entity]) do + local pk_name, key = get_key_for_uuid_gen(entity, item, schema, + parent_fk, child_key) + if key and not item[pk_name] then + item[pk_name] = generate_uuid(schema.name, key) + end + + populate_ids(item, known_entities, entity, by_id, by_key) + + local item_id = DeclarativeConfig.pk_string(schema, item) + by_id[entity] = by_id[entity] or {} + by_id[entity][item_id] = item + + local key + if schema.endpoint_key then + key = item[schema.endpoint_key] + if key then + by_key[entity] = by_key[entity] or {} + by_key[entity][key] = item + end + end + + if parent_fk and not item[child_key] then + item[child_key] = utils.deep_copy(parent_fk, false) + end + end + + ::continue:: + end + + if not parent_entity then + for entity, entries in pairs(by_id) do + local schema = all_schemas[entity] + for _, entry in pairs(entries) do + for name, field in schema:each_field(entry) do + if field.type == "foreign" and type(entry[name]) == "string" then + local found = find_entity(entry[name], field.reference, by_key, by_id) + if found then + entry[name] = all_schemas[field.reference]:extract_pk_values(found) + end + end + end + end + end + end +end + + local function flatten(self, input) local output = {} - -- This may give invalid errors as it validates input that does not have - -- foreign keys filled (e.g. basicauth_credentials fail on this with this - -- config: - -- - -- consumers: - -- username: consumer - -- basicauth_credentials: - -- - username: username - -- password: password - -- - -- because it tries to validate the credential that has no consumer.id at - -- this point filled in, and transformation implicitly adds mutually_required - -- check on that entity. - -- - -- As a short-term solution, we don't error right away here. local ok, err = self:validate(input) + if not ok then + -- the error may be due entity validation that depends on foreign entity, + -- and that is the reason why we try to validate the input again with the + -- filled foreign keys + local input_copy = utils.deep_copy(input, false) + populate_ids(input_copy, self.known_entities) + local schema = DeclarativeConfig.load(self.plugin_set, true) + if not schema:validate(input_copy) then + return nil, err + end + end generate_ids(input, self.known_entities) local processed = self:process_auto_fields(input, "insert") - local by_id, by_key_or_err = validate_references(self, processed) + local by_id, by_key = validate_references(self, processed) if not by_id then - -- let's return original error in case it is defined, as the reference - -- validation could have failed because of that. - if not ok then - return nil, err - end - - return nil, by_key_or_err + return nil, by_key end - local by_key = by_key_or_err for entity, entries in pairs(by_id) do local schema = all_schemas[entity] @@ -535,44 +596,6 @@ local function flatten(self, input) end end - -- TODO: is the plugin not enabled a sign of something wrong somewhere? - local ok2, err2 = schema:validate(flat_entry) - if not ok2 - and err2 - and err2.name ~= string.format("plugin '%s' not enabled; add it to " .. - "the 'plugins' configuration property", - flat_entry.name) - then - - if err then - if err2["@entity"] then - for _, errmsg2 in ipairs(err2["@entity"]) do - local found - if err["@entity"] then - for _, errmsg in ipairs(err["@entity"]) do - if errmsg2 == errmsg then - found = true - break - end - end - end - - if not found then - if not err["@entity"] then - err["@entity"] = {} - end - - table.insert(err["@entity"], errmsg2) - end - end - end - - return nil, err - end - - return nil, err2 - end - output[entity][id] = flat_entry end end @@ -596,7 +619,7 @@ local function load_entity_subschemas(entity_name, entity) end -function DeclarativeConfig.load(plugin_set) +function DeclarativeConfig.load(plugin_set, include_foreign) if not core_entities then -- a copy of constants.CORE_ENTITIES without "tags" core_entities = {} @@ -633,7 +656,7 @@ function DeclarativeConfig.load(plugin_set) end end - local fields, records = build_fields(known_entities) + local fields, records = build_fields(known_entities, include_foreign) -- assert(no_foreign(fields)) local ok, err = load_plugin_subschemas(fields, plugin_set) @@ -644,7 +667,9 @@ function DeclarativeConfig.load(plugin_set) -- we replace the "foreign"-type fields at the top-level -- with "string"-type fields only after the subschemas have been loaded, -- otherwise they will detect the mismatch. - reference_foreign_by_name(records) + if not include_foreign then + reference_foreign_by_name(records) + end local def = { name = "declarative_config", @@ -656,6 +681,7 @@ function DeclarativeConfig.load(plugin_set) schema.known_entities = known_entities schema.flatten = flatten + schema.plugin_set = plugin_set return schema, nil, def end diff --git a/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua b/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua index 5ea0cac4ec05..9e5637431d30 100644 --- a/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua +++ b/spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua @@ -1151,6 +1151,334 @@ describe("declarative config: flatten", function() end) describe("custom entities", function() + describe("basicauth_credentials:", function() + it("accepts an empty list", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + basicauth_credentials: + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({}, idempotent(config)) + + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + basicauth_credentials: + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({}, idempotent(config)) + + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("accepts as a nested entity", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - username: username + password: password + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("accepts as a nested entity by id", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - id: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: consumer + basicauth_credentials: + - username: username + password: password + consumer: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("fails as a nested entity by incorrect id", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - id: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: consumer + basicauth_credentials: + - username: username + password: password + consumer: 00000000-0000-0000-0000-000000000000 + ]])) + local config, err = DeclarativeConfig:flatten(config) + + assert.equal(nil, config) + assert.same({ + consumers = { + { + basicauth_credentials = { + { + ["@entity"] = { + "all or none of these fields must be set: 'password', 'consumer.id'", + }, + consumer = 'value must be null', + }, + }, + }, + }, + }, idempotent(err)) + end) + + it("accepts as a nested entity by username", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - username: username + password: password + consumer: consumer + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("fails as a nested entity by incorrect username", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - username: username + password: password + consumer: incorrect + ]])) + local config, err = DeclarativeConfig:flatten(config) + assert.equal(nil, config) + assert.same({ + consumers = { + { + basicauth_credentials = { + { + ["@entity"] = { + "all or none of these fields must be set: 'password', 'consumer.id'", + }, + consumer = 'value must be null', + }, + }, + }, + }, + }, idempotent(err)) + end) + + it("accepts as an unnested entity by id", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - id: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: consumer + basicauth_credentials: + - consumer: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: username + password: password + + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("fails as an unnested entity by incorrect id", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - id: 0fe87b4a-ce29-515a-88ec-8547e66550b9 + username: consumer + basicauth_credentials: + - consumer: 00000000-0000-0000-0000-000000000000 + username: username + password: password + + ]])) + local config, err = DeclarativeConfig:flatten(config) + assert.equal(nil, config) + assert.same({ + basicauth_credentials = { + { + ["@entity"] = { + "all or none of these fields must be set: 'password', 'consumer.id'", + }, + }, + }, + }, idempotent(err)) + end) + + it("accepts as an unnested entity by username", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - consumer: consumer + username: username + password: password + + ]])) + config = DeclarativeConfig:flatten(config) + assert.same({ + consumers = { + { + id = 'UUID', + username = 'consumer', + custom_id = null, + created_at = 1234567890, + tags = null, + }, + }, + basicauth_credentials = { + { + id = 'UUID', + consumer = { + id = 'UUID', + }, + username = 'username', + password = 'password', + created_at = 1234567890, + tags = null, + }, + }, + }, idempotent(config)) + end) + + it("fails as an unnested entity by incorrect username", function() + local config = assert(lyaml.load([[ + _format_version: "1.1" + consumers: + - username: consumer + basicauth_credentials: + - consumer: incorrect + username: username + password: password + + ]])) + local config, err = DeclarativeConfig:flatten(config) + assert.equal(nil, config) + assert.same({ + basicauth_credentials = { + { + ["@entity"] = { + "all or none of these fields must be set: 'password', 'consumer.id'", + }, + }, + }, + }, idempotent(err)) + end) + end) + describe("oauth2_credentials:", function() it("accepts an empty list", function() local config = assert(lyaml.load([[