Copilot commented on code in PR #13192:
URL: https://github.com/apache/apisix/pull/13192#discussion_r3057707185
##########
apisix/plugin.lua:
##########
@@ -988,6 +988,88 @@ local function get_plugin_schema_for_gde(name, schema_type)
end
+-- Process a single encrypt_field path on the given config table.
+-- Supports:
+-- - Arbitrary depth dotted paths (e.g., "a.b.c.d")
+-- - Array traversal at intermediate nodes (iterate each element)
+-- - Leaf type dispatch: string, array of strings, map of strings
+local function process_encrypt_field(conf, key_path, operation, plugin_name,
op_name)
+ local dot_pos = core.string.find(key_path, ".")
Review Comment:
`core.string.find(key_path, ".")` is very likely treating `"."` as a pattern
(match-any) rather than a literal dot, which would make `dot_pos` non-nil for
almost any non-empty key (breaking flat keys and path splitting). Use a literal
search (e.g., `key_path:find(".", 1, true)` or an escaped pattern like `"%."`)
so dotted-path detection only triggers when a real dot exists.
```suggestion
local dot_pos = key_path:find(".", 1, true)
```
##########
apisix/plugin.lua:
##########
@@ -988,6 +988,88 @@ local function get_plugin_schema_for_gde(name, schema_type)
end
+-- Process a single encrypt_field path on the given config table.
+-- Supports:
+-- - Arbitrary depth dotted paths (e.g., "a.b.c.d")
+-- - Array traversal at intermediate nodes (iterate each element)
+-- - Leaf type dispatch: string, array of strings, map of strings
+local function process_encrypt_field(conf, key_path, operation, plugin_name,
op_name)
+ local dot_pos = core.string.find(key_path, ".")
+
+ if not dot_pos then
+ -- leaf segment
+ local val = conf[key_path]
+ if val == nil then
+ return
+ end
+
+ if type(val) == "string" then
+ local result, err = operation(val, "data_encrypt")
+ if not result then
+ core.log.warn("failed to ", op_name, " the conf of plugin [",
+ plugin_name, "] key [", key_path, "], err: ",
err)
+ else
+ conf[key_path] = result
+ end
+
+ elseif type(val) == "table" then
+ if core.table.isarray(val) then
+ -- array of strings
+ for i, item in ipairs(val) do
+ if type(item) == "string" then
+ local result, err = operation(item, "data_encrypt")
+ if not result then
+ core.log.warn("failed to ", op_name, " the conf of
plugin [",
+ plugin_name, "] key [", key_path,
+ "] index [", i, "], err: ", err)
+ else
+ val[i] = result
+ end
+ end
+ end
+ else
+ -- map of strings
+ for k, v in pairs(val) do
+ if type(v) == "string" then
+ local result, err = operation(v, "data_encrypt")
+ if not result then
+ core.log.warn("failed to ", op_name, " the conf of
plugin [",
+ plugin_name, "] key [", key_path,
+ ".", k, "], err: ", err)
+ else
+ val[k] = result
+ end
+ end
+ end
+ end
+ end
+
+ else
+ -- intermediate segment: split on first dot and recurse
+ local segment = key_path:sub(1, dot_pos - 1)
+ local rest = key_path:sub(dot_pos + 1)
+ local val = conf[segment]
+
+ if val == nil or type(val) ~= "table" then
+ return
+ end
+
+ if core.table.isarray(val) then
+ -- array: iterate each element and recurse
+ for _, item in ipairs(val) do
+ if type(item) == "table" then
+ process_encrypt_field(item, rest, operation, plugin_name,
op_name)
+ end
+ end
+ else
+ -- map: recurse into it
+ process_encrypt_field(val, rest, operation, plugin_name, op_name)
+ end
+ end
+end
+_M.process_encrypt_field = process_encrypt_field
Review Comment:
`process_encrypt_field` is exported on `_M`, which makes it part of the
module’s public API surface (even if it is only intended as an internal
helper). If this isn’t meant to be a supported external API, consider keeping
it local (or clearly marking it internal, e.g., `_M._process_encrypt_field`)
and adjusting `t/node/data_encrypt3.t` to validate behavior through the normal
encrypt/decrypt paths rather than calling the helper directly.
```suggestion
_M._process_encrypt_field = process_encrypt_field
```
##########
t/node/data_encrypt3.t:
##########
@@ -0,0 +1,537 @@
+#
+# 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.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+log_level("info");
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!defined $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: ai-proxy: encrypt auth.header (map of strings) and
auth.gcp.service_account_json (3-level nested string)
+--- yaml_config
+apisix:
+ data_encryption:
+ enable_encrypt_fields: true
+ keyring:
+ - edd1c9f0985e76a2
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require("toolkit.json")
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "ai-proxy": {
+ "provider": "openai",
+ "auth": {
+ "header": {
+ "Authorization": "Bearer sk-test-key"
+ },
+ "query": {
+ "api-key": "my-query-secret"
+ },
+ "gcp": {
+ "service_account_json":
"{\"type\":\"service_account\"}"
+ }
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ ngx.sleep(0.1)
+
+ -- admin API should return decrypted values
+ local code, message, res = t('/apisix/admin/routes/1',
+ ngx.HTTP_GET
+ )
+ res = json.decode(res)
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(message)
+ return
Review Comment:
Fixed sleeps in integration tests can be flaky under load/slow CI, since
propagation timing (etcd/watchers) isn’t deterministic. Prefer polling with a
short timeout until the expected state is observed (e.g., retry GET until the
admin view reflects the write) to make the test reliably deterministic.
```suggestion
-- admin API should return decrypted values; poll until the
write is visible
local deadline = ngx.now() + 1
local message
local res
while true do
code, message, res = t('/apisix/admin/routes/1',
ngx.HTTP_GET
)
if code < 300 then
res = json.decode(res)
local ai_proxy = res
and res.value
and res.value.plugins
and res.value.plugins["ai-proxy"]
if ai_proxy
and ai_proxy.auth
and ai_proxy.auth.header
and ai_proxy.auth.query
and ai_proxy.auth.gcp
and ai_proxy.auth.header.Authorization == "Bearer
sk-test-key"
and ai_proxy.auth.query["api-key"] ==
"my-query-secret"
and ai_proxy.auth.gcp.service_account_json ==
"{\"type\":\"service_account\"}" then
break
end
end
if ngx.now() >= deadline then
if code >= 300 then
ngx.status = code
ngx.say(message)
else
ngx.status = 500
ngx.say("timed out waiting for admin API to reflect
decrypted route values")
end
return
end
ngx.sleep(0.01)
```
--
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]
