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

SkyeYoung · 2025-09-09T07:58:44Z

Description

This PR removes the logic of populating default values before writing in the admin API.

This solves the following problems:

Modifying the user-provided configuration can easily lead to user
confusion, which is clearly not best practice.
This also affects the diff logic in the adc that the apisix ingress
controller depends on.

Since when using GET to retrieve data from the admin API, it will be read directly from etcd, the changes introduced by this PR are compatible with the old version.

Which issue(s) this PR fixes:

Fixes #

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

This reverts commit 5cf530d.

…api-no-longer-populate-default-value

SkyeYoung · 2025-09-15T01:04:10Z

apisix/admin/utils.lua

    -- metadata
    if schema_type == core.schema.TYPE_METADATA then
        local conf = body.node and body.node.value
-        decrypt_func(conf.name, conf, schema_type)


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:

There should not be a hidden property in the schema that is used for plugin-unrelated logic.

fix: plugin metadata add id value for etcd checker #11452 implements a standardized id, which can be consistent with other resources.

SkyeYoung · 2025-09-15T01:08:43Z

apisix/plugins/request-id.lua

+            default = {
+                length = 16,
+                char_set = "abcdefghijklmnopqrstuvwxyzABCDEFGHIGKLMNOPQRSTUVWXYZ0123456789"
+            }


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={}.

SkyeYoung · 2025-09-15T01:11:11Z

t/admin/global-rules.t

+
+
+
+=== TEST 15: not unwanted data, PUT with use_real_request_uri_unsafe


Add tests, when including property use_real_request_uri_unsafe, it will be filled normally.

SkyeYoung · 2025-09-15T01:11:42Z

t/admin/plugin-configs.t

+
+
+
+=== TEST 15: PUT with limit-count policy=local


Add tests, when including property policy, it will be filled normally.

Because I found that most of the tests covered non-required fields, I didn't add similar tests later.

SkyeYoung · 2025-09-15T01:49:18Z

t/plugin/jwt-auth.t

-
-            res = require("toolkit.json").decode(res)
-            assert(res.value.plugins["jwt-auth"].exp == 86400)
+            local core = require("apisix.core")


Because the original method can no longer detect whether the exp variable exists, it's changed to direct validation through the schema, according to the internal logic.

bzp2010 · 2025-09-15T02:09:52Z

apisix/admin/plugin_metadata.lua

-            properties = {},
-        }
-    end
+    inject_metadata_schema(plugin_object)


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.

Let's wait for what others think.

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.

bzp2010 · 2025-09-15T03:00:26Z

apisix/admin/resource.lua

+    local conf_for_check = tbl_deepcopy(conf)
+    local ok, err = self.checker(id, conf_for_check, need_id, self.schema, {secret_type = typ})


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.

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

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.

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

membphis · 2025-09-16T08:50:10Z

apisix/admin/consumer_group.lua



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


pls confirm it never failed

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

It looks like they are the same?

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.

In the current implementation, it will never fail:

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

…he#12603)

SkyeYoung added 6 commits September 9, 2025 15:57

feat: admin api no longer populates default values when writing

3c5b7c0

fix: logic

28b3d6d

fix: part of cases

9377513

try mv logic to schema

5cf530d

Revert "try mv logic to schema"

52c4b98

This reverts commit 5cf530d.

fix: part logic

fdefa4e

This was referenced Sep 12, 2025

change(openid-connect): when bearer_only is false, require the user to fill in session.secret #12609

Merged

change(jwt-auth): when algorithm is not RS256 or ES256, require the user to fill in secret #12611

Merged

SkyeYoung added 6 commits September 12, 2025 11:16

fix

31a0138

fix: part

5d32f93

fix: request-id, jwt-auth

8c0891c

fix

106e1f4

Merge remote-tracking branch 'upstream/master' into young/feat/admin-…

199cf44

…api-no-longer-populate-default-value

fix

cb2d8f6

SkyeYoung marked this pull request as ready for review September 15, 2025 00:42

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Sep 15, 2025

SkyeYoung commented Sep 15, 2025

View reviewed changes

SkyeYoung requested review from AlinsRan, Baoyuantop, bzp2010, membphis and moonming September 15, 2025 01:41

SkyeYoung commented Sep 15, 2025

View reviewed changes

bzp2010 reviewed Sep 15, 2025

View reviewed changes

membphis approved these changes Sep 16, 2025

View reviewed changes

SkyeYoung requested a review from bzp2010 September 17, 2025 00:48

nic-6443 approved these changes Sep 18, 2025

View reviewed changes

AlinsRan approved these changes Sep 18, 2025

View reviewed changes

bzp2010 approved these changes Sep 18, 2025

View reviewed changes

SkyeYoung merged commit 1f2c541 into apache:master Sep 19, 2025
90 of 99 checks passed

Crazy-xyr mentioned this pull request Sep 22, 2025

fix(plugin_metadata): Ensure enable_data_encryption initialization + Querying issue #12624

Open

5 tasks

jizhuozhi pushed a commit to jizhuozhi/apisix that referenced this pull request Oct 18, 2025

feat: admin api no longer populates default values when writing (apac…

89be04d

…he#12603)




		=== TEST 15: not unwanted data, PUT with use_real_request_uri_unsafe

		local conf_for_check = tbl_deepcopy(conf)
		local ok, err = self.checker(id, conf_for_check, need_id, self.schema, {secret_type = typ})



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

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

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

Uh oh!

Conversation

SkyeYoung commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Which issue(s) this PR fixes:

Checklist

Uh oh!

SkyeYoung Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

membphis Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SkyeYoung commented Sep 9, 2025 •

edited

Loading

SkyeYoung Sep 15, 2025 •

edited

Loading

membphis Sep 16, 2025 •

edited

Loading