Copilot commented on code in PR #13312:
URL: https://github.com/apache/apisix/pull/13312#discussion_r3153078406
##########
apisix/plugin.lua:
##########
@@ -921,7 +982,89 @@ local function check_single_plugin_schema(name,
plugin_conf, schema_type, skip_d
end
if plugin_obj.check_schema then
- local ok, err = plugin_obj.check_schema(plugin_conf, schema_type)
+ local ok, err
+
+ if secret.has_secret_ref(plugin_conf) then
+ -- Strip secret ref fields recursively so they bypass all schema
+ -- constraints (enum, pattern, minLength, maxLength, etc.).
+ -- We deep-copy both conf and schema, remove secret-ref leaves from
+ -- the conf copy, and remove the corresponding property definitions
+ -- and required entries from the schema copy.
+ local conf_copy = core.table.deepcopy(plugin_conf)
+ local schema_raw
+ if schema_type == _M.TYPE_CONSUMER then
+ schema_raw = plugin_obj.consumer_schema or plugin_obj.schema
+ elseif schema_type == _M.TYPE_METADATA then
+ schema_raw = plugin_obj.metadata_schema
+ else
+ schema_raw = plugin_obj.schema
+ end
+ local schema_copy = core.table.deepcopy(schema_raw)
+
+ local function strip_secret_refs(conf, schema)
+ if type(conf) ~= "table" or type(schema) ~= "table" then
+ return
+ end
+
+ local props = schema.properties
+ local removed = {}
+
+ for k, v in pairs(conf) do
+ if type(v) == "string" and secret.is_secret_ref(v) then
+ conf[k] = nil
+ core.table.insert(removed, k)
+ if props then
+ props[k] = nil
+ end
+ elseif type(v) == "table" then
+ local sub_schema = props and props[k]
+ if sub_schema then
+ if sub_schema.type == "object" or
sub_schema.properties then
+ strip_secret_refs(v, sub_schema)
+ elseif sub_schema.type == "array" and
sub_schema.items then
+ for _, item in ipairs(v) do
+ if type(item) == "table" then
+ strip_secret_refs(item,
sub_schema.items)
+ end
+ end
+ end
Review Comment:
`strip_secret_refs` doesn’t handle arrays whose `items` are strings: in the
`array` branch it only recurses into items that are tables. This means secret
refs inside arrays (e.g. `openid-connect.introspection_addon_headers` where
items have a `pattern` and `minItems=1`) will still fail schema validation,
contrary to the intended bypass behavior. Update the array handling to detect
secret-ref string items and bypass item-level constraints appropriately
(without making the array invalid due to `minItems`).
##########
apisix/plugin.lua:
##########
@@ -65,6 +66,50 @@ local merge_global_rule_lrucache = core.lrucache.new({
ttl = 300, count = 512
})
+-- Cache for resolved plugin confs: original_conf -> {resolved, secret_vals}
+-- Weak keys ensure entries are GC'd when original conf is replaced (config
reload)
+local _resolved_cache = setmetatable({}, {__mode = "k"})
+
+local function vals_equal(a, b)
+ if a == b then
+ return true
+ end
+ if not a or not b then
+ return false
+ end
+ for k, v in pairs(a) do
+ if b[k] ~= v then
+ return false
+ end
+ end
+ for k in pairs(b) do
+ if a[k] == nil then
+ return false
+ end
+ end
+ return true
+end
+
+local function resolve_plugin_conf(conf)
+ if not secret.has_secret_ref(conf) then
+ return conf
+ end
+
+ local current_vals = secret.collect_secret_values(conf, true)
+
+ local cached = _resolved_cache[conf]
+ if cached and vals_equal(cached.secret_vals, current_vals) then
+ return cached.resolved
+ end
+
+ local resolved = secret.fetch_secrets(conf, true)
+ if not resolved then
+ return conf
+ end
Review Comment:
`secret.fetch_secrets(conf, true)` always deep-copies and returns a new
table for table inputs, so `resolved` will never be nil here (given
`has_secret_ref` only returns true for tables). This fallback path looks
unreachable and can mask real errors (since missing secret values are
represented by leaving the original `$env://...` string intact, not by
returning nil). Consider removing the `not resolved` branch and, if needed,
handling secret lookup failures explicitly via logging/metrics instead.
```suggestion
```
##########
t/secret/central-secret-refs.t:
##########
@@ -0,0 +1,288 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+BEGIN {
+ $ENV{TEST_HOST} = "test.example.com";
+ $ENV{TEST_URI} = "/new-uri";
+ $ENV{TEST_HEADER_VALUE} = "from-env";
+}
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!defined $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: proxy-rewrite resolves $env:// references automatically (central
resolution)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "proxy-rewrite": {
+ "host": "$env://TEST_HOST"
+ }
+ },
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ return ngx.say(body)
+ end
+ ngx.say("success")
+ }
+ }
+--- response_body
+success
+
+
+
+=== TEST 2: verify host header is resolved from env variable
+--- request
+GET /hello
+--- response_headers
+!X-Test-Error
+--- more_headers
+--- response_body_like
+.*
Review Comment:
This test doesn’t assert that the `Host` header was actually rewritten to
the resolved env value. With `response_body_like: .*`, the test would still
pass if the upstream sees `Host: $env://TEST_HOST`. Consider asserting the
echoed upstream response includes `host: test.example.com` (similar to existing
`t/plugin/proxy-rewrite.t` host rewrite tests).
```suggestion
host: test\.example\.com
```
##########
docs/zh/latest/terminology/secret.md:
##########
@@ -42,7 +42,13 @@ APISIX 目前支持通过以下方式存储密钥：
- [AWS Secrets Manager](#使用-aws-secrets-manager-管理密钥)
- [GCP Secrets Manager](#使用-gcp-secrets-manager-管理密钥)
-你可以在以下插件的 consumer 配置中通过指定格式的变量来使用 APISIX Secret 功能，比如 `key-auth` 插件。
+你可以在任何插件的配置中通过指定格式的变量来使用 APISIX Secret
功能。字符串类型的配置字段中的密钥引用会在运行时自动解析。例如，你可以在任何插件的字符串配置字段中使用 `$ENV://MY_API_KEY` 或
`$secret://vault/my-secret/key`。
+
+:::tip
+
+当插件配置字段使用了 `$secret://...` 或 `$env://...` 格式的密钥引用时，该字段上的 schema 校验约束（如
`enum`、`pattern`、`minLength`、`maxLength`）会在配置加载时自动跳过。实际解析后的值会在运行时进行校验。
Review Comment:
这里写到“实际解析后的值会在运行时进行校验”，但本 PR 的实现是在配置加载时绕过 schema 校验、并在请求阶段解析密钥引用；解析后并没有看到对
resolved conf 再做一次 schema 校验的路径（除非仓库中另有运行时校验逻辑）。建议要么补充运行时对解析后值的校验，要么修改文档说明以避免误导。
```suggestion
当插件配置字段使用了 `$secret://...` 或 `$env://...` 格式的密钥引用时，该字段上的 schema 校验约束（如
`enum`、`pattern`、`minLength`、`maxLength`）会在配置加载时自动跳过。因此，不应假定实际解析后的值会自动再次经过这些
schema 约束校验；请确保密钥管理服务中返回的真实值符合插件对该字段的要求。
```
##########
apisix/plugin.lua:
##########
@@ -921,7 +982,89 @@ local function check_single_plugin_schema(name,
plugin_conf, schema_type, skip_d
end
if plugin_obj.check_schema then
- local ok, err = plugin_obj.check_schema(plugin_conf, schema_type)
+ local ok, err
+
+ if secret.has_secret_ref(plugin_conf) then
+ -- Strip secret ref fields recursively so they bypass all schema
+ -- constraints (enum, pattern, minLength, maxLength, etc.).
+ -- We deep-copy both conf and schema, remove secret-ref leaves from
+ -- the conf copy, and remove the corresponding property definitions
+ -- and required entries from the schema copy.
+ local conf_copy = core.table.deepcopy(plugin_conf)
Review Comment:
When a plugin conf contains a secret ref, this branch bypasses
`plugin_obj.check_schema(...)` entirely and runs `core.schema.check` on a
modified schema instead. Many plugins rely on custom `check_schema` logic
and/or in-place normalization (e.g. `openid-connect` converts `ssl_verify="no"`
to `false` and enforces `session.secret`, `jwt-auth` enforces
`secret/public_key` based on algorithm, `clickhouse-logger` runs
`check_https`/`check_tls_bool`). Skipping those checks can allow invalid
configs to be accepted or break backward-compat normalization. Consider keeping
the plugin’s `check_schema` semantics by moving secret-ref bypass into the
schema checker (so plugins can still call `core.schema.check`) or otherwise
ensuring plugin-specific validations still execute for non-secret fields.
##########
docs/en/latest/terminology/secret.md:
##########
@@ -42,7 +42,13 @@ APISIX currently supports storing secrets in the following
ways:
- [AWS Secrets Manager](#use-aws-secrets-manager-to-manage-secrets)
- [GCP Secrets Manager](#use-gcp-secrets-manager-to-manage-secrets)
-You can use APISIX Secret functions by specifying format variables in the
consumer configuration of the following plugins, such as `key-auth`.
+You can use APISIX Secret functions by specifying format variables in the
plugin configuration of any plugin. Secret references in string fields are
automatically resolved at runtime. For example, you can use `$ENV://MY_API_KEY`
or `$secret://vault/my-secret/key` in any plugin's string configuration field.
+
+:::tip
+
+When a plugin configuration field uses a secret reference like `$secret://...`
or `$env://...`, schema validation constraints (such as `enum`, `pattern`,
`minLength`, `maxLength`) on that field are automatically bypassed during
configuration loading. The actual resolved value is validated at runtime.
Review Comment:
The tip claims “The actual resolved value is validated at runtime”, but the
implementation shown in this PR bypasses schema validation during config load
and resolves secrets at request time without re-running schema checks on the
resolved conf. Unless there is a separate runtime validation path elsewhere,
this statement is misleading; either document that validation may be bypassed
entirely, or add runtime validation of resolved values.
```suggestion
When a plugin configuration field uses a secret reference like
`$secret://...` or `$env://...`, schema validation constraints (such as `enum`,
`pattern`, `minLength`, `maxLength`) on that field are automatically bypassed
during configuration loading. Do not assume those constraints will be enforced
on the resolved value unless that field or plugin performs separate runtime
validation after secret resolution.
```
##########
t/secret/central-secret-refs.t:
##########
@@ -0,0 +1,288 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+BEGIN {
+ $ENV{TEST_HOST} = "test.example.com";
+ $ENV{TEST_URI} = "/new-uri";
+ $ENV{TEST_HEADER_VALUE} = "from-env";
+}
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!defined $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: proxy-rewrite resolves $env:// references automatically (central
resolution)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "proxy-rewrite": {
+ "host": "$env://TEST_HOST"
+ }
+ },
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ return ngx.say(body)
+ end
+ ngx.say("success")
+ }
+ }
+--- response_body
+success
+
+
+
+=== TEST 2: verify host header is resolved from env variable
+--- request
+GET /hello
+--- response_headers
+!X-Test-Error
+--- more_headers
+--- response_body_like
+.*
+--- error_log
+plugin.lua request matched route 1
+
+
+
+=== TEST 3: proxy-rewrite resolves $env:// in uri field with schema constraints
+proxy-rewrite.uri has minLength, maxLength, and pattern constraints.
+The $env://TEST_URI string itself doesn't match the ^/.* pattern,
+but central schema validation strips secret ref fields before checking.
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "proxy-rewrite": {
+ "uri": "$env://TEST_URI"
+ }
+ },
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ return ngx.say(body)
+ end
+ ngx.say("success")
+ }
+ }
+--- response_body
+success
+
+
+
+=== TEST 4: verify uri is resolved from env variable
+--- request
+GET /hello
+--- error_code: 200
+
+
+
+=== TEST 5: schema validation strips nested secret ref fields
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "proxy-rewrite": {
+ "headers": {
+ "set": {
+ "X-Custom": "$env://TEST_HEADER_VALUE"
+ }
+ }
+ }
+ },
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ return ngx.say(body)
+ end
+ ngx.say("success")
+ }
+ }
+--- response_body
+success
+
+
+
+=== TEST 6: central resolve_plugin_conf returns stable reference
+--- config
+ location /t {
+ content_by_lua_block {
+ local secret = require("apisix.secret")
+ local plugin = require("apisix.plugin")
+
+ -- access the resolve function via the filter mechanism
+ -- simulate a conf with env ref
+ local conf = {
+ host = "$env://TEST_HOST",
+ uri = "/test"
+ }
+ -- first call: should resolve and cache
+ local resolved1 = secret.fetch_secrets(conf, true)
+ ngx.say("resolved host: ", resolved1.host)
+
+ -- verify the original conf is not mutated
+ ngx.say("original host: ", conf.host)
+ }
Review Comment:
TEST 6’s description/comments say this is validating “central
resolve_plugin_conf returns stable reference”, but the code calls
`secret.fetch_secrets` once and only checks non-mutation. It neither exercises
`plugin.filter` nor checks reference stability across calls (and
`fetch_secrets` is expected to return a new table each time due to deepcopy).
Please adjust the test name/description to what it actually verifies, or update
it to assert the central cache/reference-stability behavior you intend to
guarantee.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
