Skip to content

feat: admin api no longer populates default values when writing #12603

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

Merged
merged 12 commits into from
Sep 19, 2025
Merged

feat: admin api no longer populates default values when writing #12603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3c5b7c0
feat: admin api no longer populates default values when writing
SkyeYoung Sep 9, 2025
28b3d6d
fix: logic
SkyeYoung Sep 10, 2025
9377513
fix: part of cases
SkyeYoung Sep 10, 2025
5cf530d
try mv logic to schema
SkyeYoung Sep 11, 2025
52c4b98
Revert "try mv logic to schema"
SkyeYoung Sep 11, 2025
fdefa4e
fix: part logic
SkyeYoung Sep 11, 2025
31a0138
fix
SkyeYoung Sep 12, 2025
5d32f93
fix: part
SkyeYoung Sep 12, 2025
8c0891c
fix: request-id, jwt-auth
SkyeYoung Sep 12, 2025
106e1f4
fix
SkyeYoung Sep 12, 2025
199cf44
Merge remote-tracking branch 'upstream/master' into young/feat/admin-…
SkyeYoung Sep 12, 2025
cb2d8f6
fix
SkyeYoung Sep 12, 2025
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
7 changes: 7 additions & 0 deletions apisix/admin/consumer_group.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ local core = require("apisix.core")
local consumers = require("apisix.consumer").consumers
local resource = require("apisix.admin.resource")
local schema_plugin = require("apisix.admin.plugins").check_schema
local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf
local type = type
local tostring = tostring
local ipairs = ipairs
Expand All @@ -38,6 +39,11 @@ local function check_conf(id, conf, need_id, schema)
end


local function encrypt_conf(id, conf)
plugins_encrypt_conf(conf.plugins)
Copy link
Member

@membphis membphis Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

pls confirm it never failed

for me, I prefer return plugins_encrypt_conf(conf.plugins)

Copy link
Member Author

@SkyeYoung SkyeYoung Sep 16, 2025

Choose a reason for hiding this comment

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

CleanShot 2025-09-16 at 18 52 42@2x

It looks like they are the same?

Copy link
Member Author

@SkyeYoung SkyeYoung Sep 16, 2025

Choose a reason for hiding this comment

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

CleanShot 2025-09-16 at 19 07 07@2x CleanShot 2025-09-16 at 19 09 13@2x

Regarding the calling method, it references the original local schema_plugin = require(“apisix.admin.plugins”).check_schema. And in the current scenario, no issues have been detected.

Copy link
Member Author

@SkyeYoung SkyeYoung Sep 17, 2025

Choose a reason for hiding this comment

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

In the current implementation, it will never fail:

https://github.com/apache/apisix/pull/12603/files#diff-00090cde519e7280adbc73e830b92089c4e62a22057c1b7577c77dc1456e72e5

end


local function delete_checker(id)
local consumers, consumers_ver = consumers()
if consumers_ver and consumers then
Expand All @@ -61,6 +67,7 @@ return resource.new({
kind = "consumer group",
schema = core.schema.consumer_group,
checker = check_conf,
encrypt_conf = encrypt_conf,
unsupported_methods = {"post"},
delete_checker = delete_checker
})
7 changes: 7 additions & 0 deletions apisix/admin/consumers.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
--
local core = require("apisix.core")
local plugins = require("apisix.admin.plugins")
local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf
local resource = require("apisix.admin.resource")


Expand Down Expand Up @@ -57,10 +58,16 @@ local function check_conf(username, conf, need_username, schema, opts)
end


local function encrypt_conf(id, conf)
plugins_encrypt_conf(conf.plugins, core.schema.TYPE_CONSUMER)
end


return resource.new({
name = "consumers",
kind = "consumer",
schema = core.schema.consumer,
checker = check_conf,
encrypt_conf = encrypt_conf,
unsupported_methods = {"post", "patch"}
})
8 changes: 8 additions & 0 deletions apisix/admin/credentials.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ local core = require("apisix.core")
local plugins = require("apisix.admin.plugins")
local plugin = require("apisix.plugin")
local resource = require("apisix.admin.resource")
local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf
local pairs = pairs

local function check_conf(_id, conf, _need_id, schema)
Expand Down Expand Up @@ -46,6 +47,12 @@ local function check_conf(_id, conf, _need_id, schema)
return true, nil
end


local function encrypt_conf(id, conf)
plugins_encrypt_conf(conf.plugins, core.schema.TYPE_CONSUMER)
end


-- get_credential_etcd_key is used to splice the credential's etcd key (without prefix)
-- from credential_id and sub_path.
-- Parameter credential_id is from the uri or payload; sub_path is in the form of
Expand All @@ -69,6 +76,7 @@ return resource.new({
kind = "credential",
schema = core.schema.credential,
checker = check_conf,
encrypt_conf = encrypt_conf,
get_resource_etcd_key = get_credential_etcd_key,
unsupported_methods = {"post", "patch"}
})
7 changes: 7 additions & 0 deletions apisix/admin/global_rules.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
local core = require("apisix.core")
local resource = require("apisix.admin.resource")
local schema_plugin = require("apisix.admin.plugins").check_schema
local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf


local function check_conf(id, conf, need_id, schema)
Expand All @@ -34,10 +35,16 @@ local function check_conf(id, conf, need_id, schema)
end


local function encrypt_conf(id, conf)
plugins_encrypt_conf(conf.plugins)
end


return resource.new({
name = "global_rules",
kind = "global rule",
schema = core.schema.global_rule,
checker = check_conf,
encrypt_conf = encrypt_conf,
unsupported_methods = {"post"}
})
7 changes: 7 additions & 0 deletions apisix/admin/plugin_config.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ local core = require("apisix.core")
local get_routes = require("apisix.router").http_routes
local resource = require("apisix.admin.resource")
local schema_plugin = require("apisix.admin.plugins").check_schema
local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf
local type = type
local tostring = tostring
local ipairs = ipairs
Expand All @@ -38,6 +39,11 @@ local function check_conf(id, conf, need_id, schema)
end


local function encrypt_conf(id, conf)
plugins_encrypt_conf(conf.plugins)
end


local function delete_checker(id)
local routes, routes_ver = get_routes()
if routes_ver and routes then
Expand All @@ -61,6 +67,7 @@ return resource.new({
kind = "plugin config",
schema = core.schema.plugin_config,
checker = check_conf,
encrypt_conf = encrypt_conf,
unsupported_methods = {"post"},
delete_checker = delete_checker
})
47 changes: 37 additions & 10 deletions apisix/admin/plugin_metadata.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ local pcall = pcall
local require = require
local core = require("apisix.core")
local resource = require("apisix.admin.resource")
local encrypt_conf = require("apisix.plugin").encrypt_conf
local plugin_encrypt_conf = require("apisix.plugin").encrypt_conf

local injected_mark = "injected metadata_schema"

Expand All @@ -35,6 +35,17 @@ local function validate_plugin(name)
end


local function inject_metadata_schema(plugin_object)
if not plugin_object.metadata_schema then
plugin_object.metadata_schema = {
type = "object",
['$comment'] = injected_mark,
properties = {},
}
end
end


local function check_conf(plugin_name, conf)
if not plugin_name then
return nil, {error_msg = "missing plugin name"}
Expand All @@ -45,13 +56,8 @@ local function check_conf(plugin_name, conf)
return nil, {error_msg = "invalid plugin name"}
end

if not plugin_object.metadata_schema then
plugin_object.metadata_schema = {
type = "object",
['$comment'] = injected_mark,
properties = {},
}
end
inject_metadata_schema(plugin_object)
Copy link
Contributor

@bzp2010 bzp2010 Sep 15, 2025

Choose a reason for hiding this comment

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

Although it is not introduced by your PR, it is strange to actively inject an empty schema and use ordinary schema or metadata_schema according to certain conditions for configuration verification for some non-plugin metadata.

If a plugin does not indicate that it supports plugin metadata configuration by actively declaring metadata_schema, it should not be configured in this way. Specifically, if this happens, it may be more appropriate to reject this configuration directly. For example, key-auth does not have the meaning of plugin metadata configuration.

Copy link
Member Author

@SkyeYoung SkyeYoung Sep 15, 2025

Choose a reason for hiding this comment

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

Let's wait for what others think.

Copy link
Member

@nic-6443 nic-6443 Sep 18, 2025

Choose a reason for hiding this comment

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

agree with @bzp2010 , it's strange to inject a empty metadata schema for plugins that don't need plugin metadata, look like we need a refactor for this, I think we can deal with it in another PR.


local schema = plugin_object.metadata_schema

local ok, err
Expand All @@ -64,20 +70,41 @@ local function check_conf(plugin_name, conf)
ok, err = plugin_object.check_schema(conf, core.schema.TYPE_METADATA)
end

encrypt_conf(plugin_name, conf, core.schema.TYPE_METADATA)

if not ok then
return nil, {error_msg = "invalid configuration: " .. err}
end

return plugin_name
end

local function encrypt_conf(plugin_name, conf)
if not plugin_name then
-- This situation shouldn't happen according to the execution order.
core.log.info("missing plugin name")
return
end

local ok, plugin_object = validate_plugin(plugin_name)
if not ok then
-- This situation shouldn't happen according to the execution order.
core.log.info("invalid plugin name")
return
end

inject_metadata_schema(plugin_object)

local schema = plugin_object.metadata_schema
if schema['$comment'] ~= injected_mark and plugin_object.check_schema then
plugin_encrypt_conf(plugin_name, conf, core.schema.TYPE_METADATA)
end
end


return resource.new({
name = "plugin_metadata",
kind = "plugin_metadata",
schema = core.schema.plugin_metadata,
checker = check_conf,
encrypt_conf = encrypt_conf,
unsupported_methods = {"post", "patch"}
})
9 changes: 6 additions & 3 deletions apisix/admin/plugins.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,16 @@ local _M = {}


function _M.check_schema(plugins_conf, schema_type)
local ok, err = check_schema(plugins_conf, schema_type, false)
if ok then
return check_schema(plugins_conf, schema_type, false)
end


function _M.encrypt_conf(plugins_conf, schema_type)
if plugins_conf then
for name, conf in pairs(plugins_conf) do
encrypt_conf(name, conf, schema_type)
end
end
return ok, err
end


Expand Down
8 changes: 7 additions & 1 deletion apisix/admin/resource.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ local core = require("apisix.core")
local utils = require("apisix.admin.utils")
local apisix_ssl = require("apisix.ssl")
local apisix_consumer = require("apisix.consumer")
local tbl_deepcopy = require("apisix.core.table").deepcopy
local setmetatable = setmetatable
local tostring = tostring
local ipairs = ipairs
Expand Down Expand Up @@ -123,7 +124,12 @@ function _M:check_conf(id, conf, need_id, typ, allow_time)
core.log.info("schema: ", core.json.delay_encode(self.schema))
end

local ok, err = self.checker(id, conf, need_id, self.schema, {secret_type = typ})
local conf_for_check = tbl_deepcopy(conf)
local ok, err = self.checker(id, conf_for_check, need_id, self.schema, {secret_type = typ})
Comment on lines +127 to +128
Copy link
Contributor

@bzp2010 bzp2010 Sep 15, 2025

Choose a reason for hiding this comment

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

I'm wondering if it's necessary to keep a switch enabled by a specific querystring to retain the old behavior?

For example, when fillin_default=true, still fill in the default value? This may have some meaning for debugging, otherwise we have to rely only on the correctness of the document. Historical experience tells me that this is usually difficult.

Copy link
Member Author

@SkyeYoung SkyeYoung Sep 15, 2025

Choose a reason for hiding this comment

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

This is actually a question I've thought about. Let's wait for other people's comments.

Copy link
Member

@nic-6443 nic-6443 Sep 18, 2025

Choose a reason for hiding this comment

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

I can't think of any issues that require specifying fillin_default=true to troubleshoot, but adding this option doesn't really have any drawbacks either, making it difficult to decide.

Copy link
Contributor

@AlinsRan AlinsRan Sep 18, 2025

Choose a reason for hiding this comment

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

In terms of debugging purposes only, this PR should not be blocked, and the default value can be confirmed through the version of apisix.


if self.encrypt_conf then
self.encrypt_conf(id, conf)
end

if not ok then
return ok, err
Expand Down
8 changes: 8 additions & 0 deletions apisix/admin/routes.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ local core = require("apisix.core")
local apisix_upstream = require("apisix.upstream")
local resource = require("apisix.admin.resource")
local schema_plugin = require("apisix.admin.plugins").check_schema
local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf
local type = type
local loadstring = loadstring
local ipairs = ipairs
Expand Down Expand Up @@ -173,11 +174,18 @@ local function check_conf(id, conf, need_id, schema, opts)
end


local function encrypt_conf(id, conf)
apisix_upstream.encrypt_conf(conf.upstream)
plugins_encrypt_conf(conf.plugins)
end


return resource.new({
name = "routes",
kind = "route",
schema = core.schema.route,
checker = check_conf,
encrypt_conf = encrypt_conf,
list_filter_fields = {
service_id = true,
upstream_id = true,
Expand Down
10 changes: 9 additions & 1 deletion apisix/admin/services.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ local get_stream_routes = require("apisix.router").stream_routes
local apisix_upstream = require("apisix.upstream")
local resource = require("apisix.admin.resource")
local schema_plugin = require("apisix.admin.plugins").check_schema
local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf
local tostring = tostring
local ipairs = ipairs
local type = type
Expand Down Expand Up @@ -120,10 +121,17 @@ local function delete_checker(id)
end


local function encrypt_conf(id, conf)
apisix_upstream.encrypt_conf(conf.upstream)
plugins_encrypt_conf(conf.plugins)
end


return resource.new({
name = "services",
kind = "service",
schema = core.schema.service,
checker = check_conf,
delete_checker = delete_checker
encrypt_conf = encrypt_conf,
delete_checker = delete_checker,
})
6 changes: 6 additions & 0 deletions apisix/admin/upstreams.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ local function check_conf(id, conf, need_id)
end


local function encrypt_conf(id, conf)
apisix_upstream.encrypt_conf(conf)
end


local function up_id_in_plugins(plugins, up_id)
if plugins and plugins["traffic-split"]
and plugins["traffic-split"].rules then
Expand Down Expand Up @@ -130,5 +135,6 @@ return resource.new({
kind = "upstream",
schema = core.schema.upstream,
checker = check_conf,
encrypt_conf = encrypt_conf,
delete_checker = delete_checker
})
2 changes: 1 addition & 1 deletion apisix/admin/utils.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ function _M.decrypt_params(decrypt_func, body, schema_type)
-- metadata
if schema_type == core.schema.TYPE_METADATA then
local conf = body.node and body.node.value
decrypt_func(conf.name, conf, schema_type)
Copy link
Member Author

@SkyeYoung SkyeYoung Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There is an amazing problem here:

In error-log-logger.lua, the following code exists:

apisix/apisix/plugins/error-log-logger.lua

Line 130 in 36b2b83

name = {type = "string", default = plugin_name},

This is a property not marked in the document.

The intention of its design cannot be seen from the original PR #2592.

At the same time, it is also the only one that contains encrypt_fields in metadata_schema.

apisix/apisix/plugins/error-log-logger.lua

Line 149 in 36b2b83

encrypt_fields = {"clickhouse.password"},

This causes this logic in the actual decrypt_params to take effect only for error-log-logger.lua when schema_type == core.schema.TYPE_METADATA.

However, due to the cancellation of the default value padding, this logic no longer takes effect.

Anyway, I think this is wrong. And it needs to be repaired in subsequent PRs.

The reasons are as follows:

  1. There should not be a hidden property in the schema that is used for plugin-unrelated logic.
  2. fix: plugin metadata add id value for etcd checker #11452 implements a standardized id, which can be consistent with other resources.

decrypt_func(conf.id, conf, schema_type)
end
end

Expand Down
5 changes: 4 additions & 1 deletion apisix/plugins/request-id.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ local schema = {
default = "abcdefghijklmnopqrstuvwxyzABCDEFGHIGKLMNOPQRSTUVWXYZ0123456789"
}
},
default = {}
default = {
length = 16,
char_set = "abcdefghijklmnopqrstuvwxyzABCDEFGHIGKLMNOPQRSTUVWXYZ0123456789"
}
Comment on lines +54 to +57
Copy link
Member Author

@SkyeYoung SkyeYoung Sep 15, 2025

Choose a reason for hiding this comment

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

The tests revealed that this is the only way to properly populate default values when range_id doesn't exist.

The original one should be because it's filled again after writing default={}.

}
}
}
Expand Down
13 changes: 8 additions & 5 deletions apisix/upstream.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,11 +391,6 @@ local function check_upstream_conf(in_dp, conf)
.. "wrong ssl type"
end
end

-- encrypt the key in the admin
if conf.tls and conf.tls.client_key then
conf.tls.client_key = apisix_ssl.aes_encrypt_pkey(conf.tls.client_key)
end
end

if is_http then
Expand Down Expand Up @@ -444,6 +439,14 @@ function _M.check_upstream_conf(conf)
end


function _M.encrypt_conf(conf)
-- encrypt the key in the admin
if conf and conf.tls and conf.tls.client_key then
conf.tls.client_key = apisix_ssl.aes_encrypt_pkey(conf.tls.client_key)
end
end


local function filter_upstream(value, parent)
if not value then
return
Expand Down
1 change: 0 additions & 1 deletion t/admin/consumer-group.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ passed
"plugins": {
"limit-count": {
"time_window": 60,
"policy": "local",
"count": 2,
"key": "remote_addr",
"rejected_code": 503
Expand Down
Loading
Loading