From 518df6deaa8a717a08fa9a3c3f39701f259ff9ed Mon Sep 17 00:00:00 2001 From: Michael Martin Date: Wed, 27 Mar 2024 17:34:59 -0700 Subject: [PATCH] fix(dbless): nest duplicate entity primary key errors for children This makes a change to the shape/structure of dbless errors. Before this change, an upstream with duplicate targets would yield a response like this: ``` { "code": 14, "fields": { "targets": [ null, "uniqueness violation: 'targets' entity with primary key set to '48322e4a-b3b0-591b-8ed6-fd95a6d75019' already declared" ] }, "message": "declarative config is invalid: {targets={[2]=\"uniqueness violation: 'targets' entity with primary key set to '48322e4a-b3b0-591b-8ed6-fd95a6d75019' already declared\"}}", "name": "invalid declarative configuration" } ``` After this change, the errors are nested under the parent upstream: ``` { "code": 14, "fields": { "upstreams": [ null, { "targets": [ null, "uniqueness violation: 'targets' entity with primary key set to '48322e4a-b3b0-591b-8ed6-fd95a6d75019' already declared" ] } ] }, "message": "declarative config is invalid: {upstreams={[2]={targets={[2]=\"uniqueness violation: 'targets' entity with primary key set to '48322e4a-b3b0-591b-8ed6-fd95a6d75019' already declared\"}}}}", "name": "invalid declarative configuration" } ``` As a result, the error can now be properly mapped to the input target, so `POST /config?flatten_errors=1` returns a helpful result: ``` { "code": 14, "fields": {}, "flattened_errors": [ { "entity": { "id": "48322e4a-b3b0-591b-8ed6-fd95a6d75019", "tags": [ "target-2" ], "target": "10.244.0.12:80", "upstream": { "id": "f9792964-6797-482c-bfdf-08220a4f6839" }, "weight": 1 }, "entity_id": "48322e4a-b3b0-591b-8ed6-fd95a6d75019", "entity_tags": [ "target-2" ], "entity_type": "target", "errors": [ { "message": "uniqueness violation: 'targets' entity with primary key set to '48322e4a-b3b0-591b-8ed6-fd95a6d75019' already declared", "type": "entity" } ] } ], "message": "declarative config is invalid: {}", "name": "invalid declarative configuration" } ``` --- .../fix-dbless-duplicate-target-error.yml | 3 + kong/db/errors.lua | 19 +-- kong/db/schema/others/declarative_config.lua | 23 +++- .../04-admin_api/15-off_spec.lua | 109 ++++++++++++++++++ 4 files changed, 141 insertions(+), 13 deletions(-) create mode 100644 changelog/unreleased/kong/fix-dbless-duplicate-target-error.yml diff --git a/changelog/unreleased/kong/fix-dbless-duplicate-target-error.yml b/changelog/unreleased/kong/fix-dbless-duplicate-target-error.yml new file mode 100644 index 00000000000..082e5dd5218 --- /dev/null +++ b/changelog/unreleased/kong/fix-dbless-duplicate-target-error.yml @@ -0,0 +1,3 @@ +message: "Fixed an issue wherein `POST /config?flatten_errors=1` could not return a proper response if the input included duplicate upstream targets" +type: bugfix +scope: Core diff --git a/kong/db/errors.lua b/kong/db/errors.lua index 7139c636ddb..67276bf41b2 100644 --- a/kong/db/errors.lua +++ b/kong/db/errors.lua @@ -795,12 +795,19 @@ do ---@param err_t table ---@param flattened table local function add_entity_errors(entity_type, entity, err_t, flattened) - if type(err_t) ~= "table" or nkeys(err_t) == 0 then + local err_type = type(err_t) + + -- promote error strings to `@entity` type errors + if err_type == "string" then + err_t = { ["@entity"] = err_t } + + elseif err_type ~= "table" or nkeys(err_t) == 0 then return + end -- this *should* be unreachable, but it's relatively cheap to guard against -- compared to everything else we're doing in this code path - elseif type(entity) ~= "table" then + if type(entity) ~= "table" then log(WARN, "could not parse ", entity_type, " errors for non-table ", "input: '", tostring(entity), "'") return @@ -1033,13 +1040,7 @@ do for i, err_t_i in drain(section_errors) do local entity = entities[i] - - -- promote error strings to `@entity` type errors - if type(err_t_i) == "string" then - err_t_i = { ["@entity"] = err_t_i } - end - - if type(entity) == "table" and type(err_t_i) == "table" then + if type(entity) == "table" then add_entity_errors(entity_type, entity, err_t_i, flattened) else diff --git a/kong/db/schema/others/declarative_config.lua b/kong/db/schema/others/declarative_config.lua index 15d291f6c0b..455c6fa129f 100644 --- a/kong/db/schema/others/declarative_config.lua +++ b/kong/db/schema/others/declarative_config.lua @@ -335,7 +335,7 @@ local function uniqueness_error_msg(entity, key, value) end -local function populate_references(input, known_entities, by_id, by_key, expected, errs, parent_entity) +local function populate_references(input, known_entities, by_id, by_key, expected, errs, parent_entity, parent_idx) for _, entity in ipairs(known_entities) do yield(true) @@ -363,7 +363,7 @@ local function populate_references(input, known_entities, by_id, by_key, expecte for i, item in ipairs(input[entity]) do yield(true) - populate_references(item, known_entities, by_id, by_key, expected, errs, entity) + populate_references(item, known_entities, by_id, by_key, expected, errs, entity, i) local item_id = DeclarativeConfig.pk_string(entity_schema, item) local key = use_key and item[endpoint_key] @@ -381,8 +381,23 @@ local function populate_references(input, known_entities, by_id, by_key, expecte if item_id then by_id[entity] = by_id[entity] or {} if (not failed) and by_id[entity][item_id] then - errs[entity] = errs[entity] or {} - errs[entity][i] = uniqueness_error_msg(entity, "primary key", item_id) + local err_t + + if parent_entity and parent_idx then + errs[parent_entity] = errs[parent_entity] or {} + errs[parent_entity][parent_idx] = errs[parent_entity][parent_idx] or {} + errs[parent_entity][parent_idx][entity] = errs[parent_entity][parent_idx][entity] or {} + + -- e.g. errs["upstreams"][5]["targets"] + err_t = errs[parent_entity][parent_idx][entity] + + else + errs[entity] = errs[entity] or {} + err_t = errs[entity] + end + + err_t[i] = uniqueness_error_msg(entity, "primary key", item_id) + else by_id[entity][item_id] = item table.insert(by_id[entity], item_id) diff --git a/spec/02-integration/04-admin_api/15-off_spec.lua b/spec/02-integration/04-admin_api/15-off_spec.lua index 3ca5d34b80e..eee02e99e9f 100644 --- a/spec/02-integration/04-admin_api/15-off_spec.lua +++ b/spec/02-integration/04-admin_api/15-off_spec.lua @@ -2724,6 +2724,115 @@ R6InCcH2Wh8wSeY5AuDXvu2tv9g/PW9wIJmPuKSHMA== }, }, flattened) end) + + it("correctly handles duplicate upstream target errors", function() + local target = { + target = "10.244.0.12:80", + weight = 1, + tags = { "target-1" }, + } + -- this has the same : tuple as the first target, so it will + -- be assigned the same id + local dupe_target = utils.deep_copy(target) + dupe_target.tags = { "target-2" } + + local input = { + _format_version = "3.0", + services = { + { + connect_timeout = 60000, + host = "httproute.default.httproute-testing.0", + id = "4e3cb785-a8d0-5866-aa05-117f7c64f24d", + name = "httproute.default.httproute-testing.0", + port = 8080, + protocol = "http", + read_timeout = 60000, + retries = 5, + routes = { + { + https_redirect_status_code = 426, + id = "073fc413-1c03-50b4-8f44-43367c13daba", + name = "httproute.default.httproute-testing.0.0", + path_handling = "v0", + paths = { + "~/httproute-testing$", + "/httproute-testing/", + }, + preserve_host = true, + protocols = { + "http", + "https", + }, + strip_path = true, + tags = {}, + }, + }, + tags = {}, + write_timeout = 60000, + }, + }, + upstreams = { + { + algorithm = "round-robin", + name = "httproute.default.httproute-testing.0", + id = "e9792964-6797-482c-bfdf-08220a4f6832", + tags = { + "k8s-name:httproute-testing", + "k8s-namespace:default", + "k8s-kind:HTTPRoute", + "k8s-uid:f9792964-6797-482c-bfdf-08220a4f6839", + "k8s-group:gateway.networking.k8s.io", + "k8s-version:v1", + }, + targets = { + { + target = "10.244.0.11:80", + weight = 1, + }, + { + target = "10.244.0.12:80", + weight = 1, + }, + }, + }, + { + algorithm = "round-robin", + name = "httproute.default.httproute-testing.1", + id = "f9792964-6797-482c-bfdf-08220a4f6839", + tags = { + "k8s-name:httproute-testing", + "k8s-namespace:default", + "k8s-kind:HTTPRoute", + "k8s-uid:f9792964-6797-482c-bfdf-08220a4f6839", + "k8s-group:gateway.networking.k8s.io", + "k8s-version:v1", + }, + targets = { + target, + dupe_target, + }, + }, + }, + } + + local flattened = post_config(input) + local entry = get_by_tag(dupe_target.tags[1], flattened) + assert.not_nil(entry, "no error for duplicate target in the response") + + -- sanity + assert.same(dupe_target.tags, entry.entity_tags) + + assert.is_table(entry.errors, "missing entity errors table") + assert.equals(1, #entry.errors, "expected 1 entity error") + assert.is_table(entry.errors[1], "entity error is not a table") + + local e = entry.errors[1] + assert.equals("entity", e.type) + + local exp = string.format("uniqueness violation: 'targets' entity with primary key set to '%s' already declared", entry.entity_id) + + assert.equals(exp, e.message) + end) end)