Skip to content

Commit

Permalink
fix(dbless): nest duplicate entity primary key errors for children
Browse files Browse the repository at this point in the history 
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"
}
```
  • Loading branch information
@flrgh
flrgh committed Apr 10, 2024
1 parent c45adf9 commit 518df6d
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 13 deletions.
3 changes: 3 additions & 0 deletions changelog/unreleased/kong/fix-dbless-duplicate-target-error.yml
View file
Original file line number Diff line number Diff line change
@@ -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
19 changes: 10 additions & 9 deletions kong/db/errors.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
23 changes: 19 additions & 4 deletions kong/db/schema/others/declarative_config.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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]
Expand All @@ -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)
Expand Down
109 changes: 109 additions & 0 deletions spec/02-integration/04-admin_api/15-off_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <addr>:<port> 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)


Expand Down

0 comments on commit 518df6d

Please sign in to comment.