Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core):fix missing info of flattened_errors #13998

Closed
wants to merge 4 commits into from
Closed

fix(core):fix missing info of flattened_errors #13998

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76cb75e
fix commit
lhanjian Dec 10, 2024
913c513
Update changelog/unreleased/kong/fix-missing-flattened-erros.yml
lhanjian Dec 12, 2024
8a4b940
update
lhanjian Dec 16, 2024
87b904e
update comments
lhanjian Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/unreleased/kong/fix-missing-flattened-erros.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
message: |
Fixed an issue where flattened errors had missing info when fields were lost or conflicted in a nested entity.
lhanjian marked this conversation as resolved.
Show resolved Hide resolved
type: bugfix
scope: Core
13 changes: 12 additions & 1 deletion kong/db/errors.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ local pl_keys = require("pl.tablex").keys
local nkeys = require("table.nkeys")
local table_isarray = require("table.isarray")
local uuid = require("kong.tools.uuid")
local split = require("kong.tools.string").split


local type = type
Expand Down Expand Up @@ -862,6 +863,9 @@ do
-- }
-- }
--
if type(err_t[ref.field]) == "string" then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the error message is always string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@outsinre maybe nil, or table.

err_t[ref.field] just like field_err_t

err_t[ref.field] = entity_type..":"..err_t[ref.field] ..":"..ref.field
end
if ref.entity == entity_type then
local field_name = ref.field
local field_value = entity[field_name]
Expand Down Expand Up @@ -1029,7 +1033,14 @@ do
goto next_section
end

local entities = input[entity_type]
local entity_type_list = split(entity_type, "|")
local entitys_level = input
for i, entity_type_elem in ipairs(entity_type_list) do
local next_elem = tonumber(entity_type_elem) or entity_type_elem
entitys_level = entitys_level[next_elem]
end

local entities = entitys_level

if type(entities) ~= "table" then
-- put it back into the error table
Expand Down
8 changes: 6 additions & 2 deletions kong/db/schema/others/declarative_config.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,8 +400,12 @@ local function populate_references(input, known_entities, by_id, by_key, expecte
if key and key ~= ngx.null then
local ok = add_to_by_key(by_key, entity_schema, item, entity, key)
if not ok then
errs[entity] = errs[entity] or {}
errs[entity][i] = uniqueness_error_msg(entity, endpoint_key, key)
local errs_item_name = entity
if parent_entity and tostring(parent_idx) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If parent_idx is nil, then tostring(parent_idx) is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@outsinre
i think tostring(nil) maybe nil?
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The printed nil is a string, not the Lua nil.

errs_item_name = parent_entity.."|"..tostring(parent_idx).."|"..entity
end
errs[errs_item_name] = errs[errs_item_name] or {}
errs[errs_item_name][i] = uniqueness_error_msg(entity, endpoint_key, key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The errs_item_name and entity on this line refer to the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@outsinre no.

entity is its original name.
errs_item_name is key name of errs

failed = true
end
end
Expand Down
109 changes: 109 additions & 0 deletions spec/02-integration/04-admin_api/15-off_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,45 @@ describe("Admin API #off", function()
assert.equals(snis.certificate.id, certificates.id)
end)

it("snis in certificates without certificate", function()
outsinre marked this conversation as resolved.
Show resolved Hide resolved
local res = assert(client:send {
method = "POST",
path = "/config?flatten_errors=1",
body = {
_format_version = "1.1",
consumers = {
{
username = "x",
basicauth_credentials = {
username = "x",
password = "x",
}
}
},
certificates = {
{
cert = ssl_fixtures.cert,
id = "d83994d2-c24c-4315-b431-ee76b6611dcb",
key = ssl_fixtures.key,
snis = {
{
name = "foo.example",
id = "1c6e83b7-c9ad-40ac-94e8-52f5ee7bde44",
},
}
}
},
},
headers = {
["Content-Type"] = "application/json"
}
})

local body = assert.response(res).has.status(400)
Copy link
Contributor

@outsinre outsinre Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 400 error is due to DB-less by default does not support this kind of format. DB-less uses declarative format that needs all entities flattened at top-level.

It expects the snis not nested within the certificates.

local entities = cjson.decode(body)
assert.equals("snis:required field missing:certificate", entities.flattened_errors[1].errors[1].message)
end)

it("can reload upstreams (regression test)", function()
local config = [[
_format_version: "1.1"
Expand Down Expand Up @@ -2323,6 +2362,76 @@ R6InCcH2Wh8wSeY5AuDXvu2tv9g/PW9wIJmPuKSHMA==
}, post_config(input))
end)

it("flatten_errors when conflicting inputs", function()
local conflicting_input = {
_format_version = "3.0",
consumers = {
Copy link
Contributor

@flrgh flrgh Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm 🤔 I don't know if it's possible to properly flatten errors in this case, because the result we get from the declarative validator is ambiguous. Consider this input:

{
  _format_version = "3.0",
  consumers = {
    {
      username = "consumer-01",
      basicauth_credentials = {
        { username = "user-01", password = "pwd" },
        { username = "user-11", password = "pwd" },
        { username = "user-02", password = "pwd" },
        { username = "user-11", password = "pwd" },
      }
    },
    {
      username = "consumer-02",
      basicauth_credentials = {
        { username = "user-99", password = "pwd" },
      }
    },
    {
      username = "consumer-03",
      basicauth_credentials = {
        { username = "user-01", password = "pwd" },
        { username = "user-33", password = "pwd" },
        { username = "user-02", password = "pwd" },
        { username = "user-33", password = "pwd" },
      }
    }
  }
}

It has the following duplicates:

  1. consumers[1].basicauth_credentials[4] (user-11)
  2. consumers[3].basicauth_credentials[1] (user-01)
  3. consumers[3].basicauth_credentials[3] (user-02)
  4. consumers[3].basicauth_credentials[4] (user-33)

The error table we receive from :parse_table() looks like this:

{
  basicauth_credentials = {
    [1] = "uniqueness violation: 'basicauth_credentials' entity with username set to 'user-01' already declared",
    [3] = "uniqueness violation: 'basicauth_credentials' entity with username set to 'user-02' already declared",
    [4] = "uniqueness violation: 'basicauth_credentials' entity with username set to 'user-33' already declared",
  },
}
  1. The declarative code actually overwrote the error in consumers[1].basicauth_credentials[4] with the error in consumers[3].basicauth_credentials[4] (presumably because they both happened at index 4)
  2. We cannot use this information to determine which consumer each basicauth_credentials error corresponds to

In summary, I don't think this can be fixed in the error-flattening code alone. A fix within kong.db.schema.others.declarative_config must be made first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flrgh
I try to add your inputs to test case, it can get 4 results.

Is this what you need?

{
username = "username1",
tags = {
"k8s-group:configuration.konghq.com",
"k8s-version:v1",
"k8s-kind:KongConsumer",
"k8s-namespace:default",
"k8s-name:consumer1"
},
basicauth_credentials = {
{
username = "username1",
password = "password",
tags = {
"k8s-group:",
"k8s-version:v1",
"k8s-kind:Secret",
"k8s-namespace:default",
"k8s-name:basic-1"
}
},
{
username = "username1",
password = "password",
tags = {
"k8s-group:",
"k8s-version:v1",
"k8s-kind:Secret",
"k8s-namespace:default",
"k8s-name:basic-2"
}
},
}
}
}
}
validate({
{
entity = {
username = "username1",
password = "password",
tags = {
"k8s-group:",
"k8s-version:v1",
"k8s-kind:Secret",
"k8s-namespace:default",
"k8s-name:basic-2"
}
},
entity_tags = {
"k8s-group:",
"k8s-version:v1",
"k8s-kind:Secret",
"k8s-namespace:default",
"k8s-name:basic-2"
},
entity_type = "consumers|1|basicauth_credential",
errors = { {
message = "uniqueness violation: 'basicauth_credentials' entity with username set to 'username1' already declared",
type = "entity",
}
}
},
}, post_config(conflicting_input))
end)

it("preserves IDs from the input", function()
local id = "0175e0e8-3de9-56b4-96f1-b12dcb4b6691"
local service = {
Expand Down
Loading