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(*): reject config if both deprecated and new field defined and their values mismatch #13565

Merged
merged 3 commits into from
Aug 29, 2024
+386 −47
Merged

fix(*): reject config if both deprecated and new field defined and their values mismatch #13565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eabcd44
fix(*): reject config if both deprecated and new field defined and th…
nowNick Aug 23, 2024
e69e9fb
fixup! fix(*): reject config if both deprecated and new field defined…
nowNick Aug 29, 2024
125c653
fixup! fix(*): reject config if both deprecated and new field defined…
nowNick Aug 29, 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
5 changes: 5 additions & 0 deletions changelog/unreleased/kong/reject-config-on-deprecated-fields-mismatch.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
message: |
Changed the behaviour of shorthand fields that are used to describe deprecated fields. If
both fields are sent in the request and their values mismatch - the request will be rejected.
type: bugfix
scope: Core
77 changes: 57 additions & 20 deletions kong/db/schema/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ local table_merge = require "kong.tools.table".table_merge
local null_aware_table_merge = require "kong.tools.table".null_aware_table_merge
local table_path = require "kong.tools.table".table_path
local is_array = require "kong.tools.table".is_array
local join_string = require "kong.tools.string".join


local setmetatable = setmetatable
Expand Down Expand Up @@ -1704,6 +1705,35 @@ local function collect_field_reference(refs, key, reference)
end


local function validate_deprecation_exclusiveness(data, shorthand_value, shorthand_name, shorthand_definition)
if shorthand_value == nil or
shorthand_value == ngx.null or
shorthand_definition.deprecation == nil or
shorthand_definition.deprecation.replaced_with == nil then
return true
end

for _, replaced_with_element in ipairs(shorthand_definition.deprecation.replaced_with) do
local new_field_value = replaced_with_element.reverse_mapping_function and replaced_with_element.reverse_mapping_function(data)
or table_path(data, replaced_with_element.path)

if new_field_value and
new_field_value ~= ngx.null and
not deepcompare(new_field_value, shorthand_value) then
local new_field_name = join_string(".", replaced_with_element.path)

return nil, string.format(
"both deprecated and new field are used but their values mismatch: %s = %s vs %s = %s",
shorthand_name, tostring(shorthand_value),
new_field_name, tostring(new_field_value)
)
end
end

return true
end


--- Given a table, update its fields whose schema
-- definition declares them as `auto = true`,
-- based on its CRUD operation context, and set
Expand Down Expand Up @@ -1741,27 +1771,34 @@ function Schema:process_auto_fields(data, context, nulls, opts)
errs[sname] = err
has_errs = true
else
data[sname] = nil
local new_values = sdata.func(value)
if new_values then
-- a shorthand field may have a deprecation property, that is used
-- to determine whether the shorthand's return value takes precedence
-- over the similarly named actual schema fields' value when both
-- are present. On deprecated shorthand fields the actual schema
-- field value takes the precedence, otherwise the shorthand's
-- return value takes the precedence.
local deprecation = sdata.deprecation
for k, v in pairs(new_values) do
if type(v) == "table" then
local source = {}
if data[k] and data[k] ~= null then
source = data[k]
end
data[k] = deprecation and null_aware_table_merge(v, source)
or table_merge(source, v)
local _, deprecation_error = validate_deprecation_exclusiveness(data, value, sname, sdata)

elseif not deprecation or (data[k] == nil or data[k] == null) then
data[k] = v
if deprecation_error then
errs[sname] = deprecation_error
has_errs = true
else
data[sname] = nil
local new_values = sdata.func(value)
if new_values then
-- a shorthand field may have a deprecation property, that is used
-- to determine whether the shorthand's return value takes precedence
-- over the similarly named actual schema fields' value when both
-- are present. On deprecated shorthand fields the actual schema
-- field value takes the precedence, otherwise the shorthand's
-- return value takes the precedence.
local deprecation = sdata.deprecation
for k, v in pairs(new_values) do
if type(v) == "table" then
local source = {}
if data[k] and data[k] ~= null then
source = data[k]
end
data[k] = deprecation and null_aware_table_merge(v, source)
or table_merge(source, v)

elseif not deprecation or (data[k] == nil or data[k] == null) then
data[k] = v
end
end
end
end
Expand Down
9 changes: 9 additions & 0 deletions kong/db/schema/metaschema.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,15 @@ local field_schema = {
{ message = { type = "string", required = true } },
{ removal_in_version = { type = "string", required = true } },
{ old_default = { type = "any", required = false } },
{ replaced_with = { type = "array", required = false,
elements = { type = "record",
required = false,
fields = {
{ path = { type = "array", len_min = 1, required = true, elements = { type = "string"}} },
bungle marked this conversation as resolved.
Show resolved Hide resolved
{ reverse_mapping_function = { type = "function", required = false }}
},
}
} },
},
} },
}
Expand Down
4 changes: 4 additions & 0 deletions kong/plugins/acme/schema.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ local LEGACY_SCHEMA_TRANSLATIONS = {
len_min = 0,
translate_backwards = {'password'},
deprecation = {
replaced_with = { { path = { 'password' } } },
message = "acme: config.storage_config.redis.auth is deprecated, please use config.storage_config.redis.password instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -53,6 +54,7 @@ local LEGACY_SCHEMA_TRANSLATIONS = {
type = "string",
translate_backwards = {'server_name'},
deprecation = {
replaced_with = { { path = { 'server_name' } } },
Comment on lines 55 to +57
Copy link
Member

Choose a reason for hiding this comment

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

We have translate_backwards and now replaced_with. what is the difference here and what is this path = ..., there is no path = with translate_backwards. Can we use single thing for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan is to use only one of them - I want to get rid of translate_backwards - however it would require quite a lot code removal so I wanted to get it in another PR. It's going to be a refactor PR so it's not going to block the release.
This addition - although it's a little bit of code duplication I think it's just safer / quicker path. I'll prepare cleanup PRs once this one is merged - I promise 😄

As a bonus I think we can even automate the message - I'm planning to still allow custom message but the way right now it's defined is rather repetitive and it looks like we have all the knowledge we need to generate those deprecation warnings automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plus... the more I work with it the more I hate the name translate_backwards - it's just confusing. replaced_with inside deprecation right now makes more sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

The plan is to use only one of them

If both translate_backwards and replaced_with can fix the bug, we'd better stick with the translate_backwards and avoid introducing an extra metaschema in this PR.

The refactor and introduction of replaced_with can be made in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we cannot use only one of the since translate_backwards is a single just a table path and we need to have an array of table paths. The reason is that one field can be replaced by a few fields ex: redis.timeout and redis.connect_timeout / redis.send_timeout / redis.read_timeout.
We need to have a way to check against all 3 of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a test case just for that. Maybe it explains it a little bit better: https://github.com/Kong/kong/pull/13565/files#diff-be157ebd3368ec7ae7c30ab60332aebbd18f627ec4fa680156e6d6cd1706f668R4233

message = "acme: config.storage_config.redis.ssl_server_name is deprecated, please use config.storage_config.redis.server_name instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -64,6 +66,7 @@ local LEGACY_SCHEMA_TRANSLATIONS = {
len_min = 0,
translate_backwards = {'extra_options', 'namespace'},
deprecation = {
replaced_with = { { path = { 'extra_options', 'namespace' } } },
message = "acme: config.storage_config.redis.namespace is deprecated, please use config.storage_config.redis.extra_options.namespace instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -74,6 +77,7 @@ local LEGACY_SCHEMA_TRANSLATIONS = {
type = "integer",
translate_backwards = {'extra_options', 'scan_count'},
deprecation = {
replaced_with = { { path = { 'extra_options', 'scan_count' } } },
message = "acme: config.storage_config.redis.scan_count is deprecated, please use config.storage_config.redis.extra_options.scan_count instead",
removal_in_version = "4.0", },
func = function(value)
Expand Down
9 changes: 9 additions & 0 deletions kong/plugins/rate-limiting/schema.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ return {
type = "string",
translate_backwards = {'redis', 'host'},
deprecation = {
replaced_with = { { path = { 'redis', 'host' } } },
message = "rate-limiting: config.redis_host is deprecated, please use config.redis.host instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -114,6 +115,7 @@ return {
type = "integer",
translate_backwards = {'redis', 'port'},
deprecation = {
replaced_with = { { path = { 'redis', 'port' } } },
message = "rate-limiting: config.redis_port is deprecated, please use config.redis.port instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -125,6 +127,7 @@ return {
len_min = 0,
translate_backwards = {'redis', 'password'},
deprecation = {
replaced_with = { { path = { 'redis', 'password' } } },
message = "rate-limiting: config.redis_password is deprecated, please use config.redis.password instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -135,6 +138,7 @@ return {
type = "string",
translate_backwards = {'redis', 'username'},
deprecation = {
replaced_with = { { path = { 'redis', 'username' } } },
message = "rate-limiting: config.redis_username is deprecated, please use config.redis.username instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -145,6 +149,7 @@ return {
type = "boolean",
translate_backwards = {'redis', 'ssl'},
deprecation = {
replaced_with = { { path = { 'redis', 'ssl' } } },
message = "rate-limiting: config.redis_ssl is deprecated, please use config.redis.ssl instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -155,6 +160,7 @@ return {
type = "boolean",
translate_backwards = {'redis', 'ssl_verify'},
deprecation = {
replaced_with = { { path = { 'redis', 'ssl_verify' } } },
message = "rate-limiting: config.redis_ssl_verify is deprecated, please use config.redis.ssl_verify instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -165,6 +171,7 @@ return {
type = "string",
translate_backwards = {'redis', 'server_name'},
deprecation = {
replaced_with = { { path = { 'redis', 'server_name' } } },
message = "rate-limiting: config.redis_server_name is deprecated, please use config.redis.server_name instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -175,6 +182,7 @@ return {
type = "integer",
translate_backwards = {'redis', 'timeout'},
deprecation = {
replaced_with = { { path = { 'redis', 'timeout' } } },
message = "rate-limiting: config.redis_timeout is deprecated, please use config.redis.timeout instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -185,6 +193,7 @@ return {
type = "integer",
translate_backwards = {'redis', 'database'},
deprecation = {
replaced_with = { { path = { 'redis', 'database' } } },
message = "rate-limiting: config.redis_database is deprecated, please use config.redis.database instead",
removal_in_version = "4.0", },
func = function(value)
Expand Down
9 changes: 9 additions & 0 deletions kong/plugins/response-ratelimiting/schema.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ return {
type = "string",
translate_backwards = {'redis', 'host'},
deprecation = {
replaced_with = { { path = { 'redis', 'host' } } },
message = "response-ratelimiting: config.redis_host is deprecated, please use config.redis.host instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -153,6 +154,7 @@ return {
type = "integer",
translate_backwards = {'redis', 'port'},
deprecation = {
replaced_with = { { path = {'redis', 'port'} } },
message = "response-ratelimiting: config.redis_port is deprecated, please use config.redis.port instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -164,6 +166,7 @@ return {
len_min = 0,
translate_backwards = {'redis', 'password'},
deprecation = {
replaced_with = { { path = {'redis', 'password'} } },
message = "response-ratelimiting: config.redis_password is deprecated, please use config.redis.password instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -174,6 +177,7 @@ return {
type = "string",
translate_backwards = {'redis', 'username'},
deprecation = {
replaced_with = { { path = {'redis', 'username'} } },
message = "response-ratelimiting: config.redis_username is deprecated, please use config.redis.username instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -184,6 +188,7 @@ return {
type = "boolean",
translate_backwards = {'redis', 'ssl'},
deprecation = {
replaced_with = { { path = {'redis', 'ssl'} } },
message = "response-ratelimiting: config.redis_ssl is deprecated, please use config.redis.ssl instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -194,6 +199,7 @@ return {
type = "boolean",
translate_backwards = {'redis', 'ssl_verify'},
deprecation = {
replaced_with = { { path = {'redis', 'ssl_verify'} } },
message = "response-ratelimiting: config.redis_ssl_verify is deprecated, please use config.redis.ssl_verify instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -204,6 +210,7 @@ return {
type = "string",
translate_backwards = {'redis', 'server_name'},
deprecation = {
replaced_with = { { path = {'redis', 'server_name'} } },
message = "response-ratelimiting: config.redis_server_name is deprecated, please use config.redis.server_name instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -214,6 +221,7 @@ return {
type = "integer",
translate_backwards = {'redis', 'timeout'},
deprecation = {
replaced_with = { { path = {'redis', 'timeout'} } },
message = "response-ratelimiting: config.redis_timeout is deprecated, please use config.redis.timeout instead",
removal_in_version = "4.0", },
func = function(value)
Expand All @@ -224,6 +232,7 @@ return {
type = "integer",
translate_backwards = {'redis', 'database'},
deprecation = {
replaced_with = { { path = {'redis', 'database'} } },
message = "response-ratelimiting: config.redis_database is deprecated, please use config.redis.database instead",
removal_in_version = "4.0", },
func = function(value)
Expand Down
Loading
Loading