fix(dbless): nest duplicate entity primary key errors for children (#8891)

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"
}
```

(cherry picked from commit b8891eb)

Co-authored-by: Michael Martin <[email protected]>

Author: team-gateway-bot

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

kong/db/errors.lua

@@ -835,12 +835,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
@@ -1073,13 +1080,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

kong/db/schema/others/declarative_config.lua

@@ -352,7 +352,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)
 
@@ -380,7 +380,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]
@@ -398,8 +398,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)

spec/02-integration/04-admin_api/15-off_spec.lua

@@ -2773,6 +2773,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)