This is an automated email from the ASF dual-hosted git repository.
AlinsRan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/apisix.git
The following commit(s) were added to refs/heads/master by this push:
new b8738ef13 fix(config_etcd): keep the previous value when a full reload
gets invalid data (#13717)
b8738ef13 is described below
commit b8738ef13e4b074dfa993aaeb154096c40e9ebca
Author: AlinsRan <[email protected]>
AuthorDate: Wed Jul 29 11:11:43 2026 +0800
fix(config_etcd): keep the previous value when a full reload gets invalid
data (#13717)
---
apisix/core/config_etcd.lua | 72 +++++++++++--
apisix/plugin.lua | 21 +++-
t/core/config_etcd.t | 248 ++++++++++++++++++++++++++++++++++++++++++++
t/plugin/proxy-cache/disk.t | 81 +++++++++++++++
4 files changed, 408 insertions(+), 14 deletions(-)
diff --git a/apisix/core/config_etcd.lua b/apisix/core/config_etcd.lua
index a56a221b6..79a66bf88 100644
--- a/apisix/core/config_etcd.lua
+++ b/apisix/core/config_etcd.lua
@@ -546,9 +546,24 @@ local function sync_status_to_shdict(status)
end
-local function load_full_data(self, dir_res, headers)
+local function get_prev_item(prev_values, prev_values_hash, key)
+ if not prev_values or not prev_values_hash then
+ return nil
+ end
+
+ -- deleted items are tombstoned as `false` in the values array and removed
+ -- from the hash by the watch path, so a hash hit is always a live item
+ local idx = prev_values_hash[key]
+ return idx and prev_values[idx] or nil
+end
+
+
+local function load_full_data(self, dir_res, headers, prev_values,
prev_values_hash)
local err
local changed = false
+ -- previous items carried over because their new data failed the check;
+ -- their clean handlers must NOT be fired
+ local carried = {}
if self.single_item then
self.values = new_tab(1, 0)
@@ -583,6 +598,19 @@ local function load_full_data(self, dir_res, headers)
if self.filter then
self.filter(item)
end
+
+ elseif item.value ~= nil then
+ -- new data exists but is invalid: keep the previous value like the
+ -- incremental watch path does. An absent value (deleted key) must
+ -- not be resurrected, hence the `item.value ~= nil` guard.
+ local prev_item = get_prev_item(prev_values, prev_values_hash,
self.key)
+ if prev_item then
+ log.warn("failed to check item data of [", self.key,
+ "], keep the previous configuration, err: ", err)
+ insert_tab(self.values, prev_item)
+ self.values_hash[self.key] = #self.values
+ carried[prev_item] = true
+ end
end
self:upgrade_version(item.modifiedIndex)
@@ -601,8 +629,10 @@ local function load_full_data(self, dir_res, headers)
for _, item in ipairs(values) do
local key = short_key(self, item.key)
local data_valid = true
+ err = nil
if type(item.value) ~= "table" then
data_valid = false
+ err = "invalid item data, it should be an object"
log.error("invalid item data of [", self.key .. "/" .. key,
"], val: ", item.value,
", it should be an object")
@@ -637,12 +667,35 @@ local function load_full_data(self, dir_res, headers)
if self.filter then
self.filter(item)
end
+
+ else
+ local prev_item = get_prev_item(prev_values, prev_values_hash,
key)
+ if prev_item then
+ -- keep serving with the last valid configuration instead
of
+ -- silently dropping the whole item on a full reload, see
the
+ -- incremental path in sync_data for the same semantics
+ log.warn("failed to check item data of [", self.key, "/",
key,
+ "], keep the previous configuration, err: ", err)
+ insert_tab(self.values, prev_item)
+ self.values_hash[key] = #self.values
+ carried[prev_item] = true
+ end
end
self:upgrade_version(item.modifiedIndex)
end
end
+ -- fire the clean handlers of the previous items that were not carried
+ -- over: they were either replaced by a new value or deleted from etcd
+ if prev_values then
+ for _, item in ipairs(prev_values) do
+ if item and not carried[item] then
+ config_util.fire_all_clean_handlers(item)
+ end
+ end
+ end
+
if headers then
self.prev_index = tonumber(headers["X-Etcd-Index"]) or 0
self:upgrade_version(headers["X-Etcd-Index"])
@@ -691,16 +744,15 @@ local function sync_data(self)
log.debug("readdir key: ", self.key, " res: ",
json.delay_encode(dir_res))
- if self.values then
- for i, val in ipairs(self.values) do
- config_util.fire_all_clean_handlers(val)
- end
-
- self.values = nil
- self.values_hash = nil
- end
+ -- hand the previous values over to load_full_data so that an item
whose
+ -- new data fails the validation can keep serving with its old value,
+ -- consistent with the incremental watch path. The clean handlers of
the
+ -- replaced / deleted items are fired inside load_full_data.
+ local prev_values, prev_values_hash = self.values, self.values_hash
+ self.values = nil
+ self.values_hash = nil
- load_full_data(self, dir_res, headers)
+ load_full_data(self, dir_res, headers, prev_values, prev_values_hash)
return true
end
diff --git a/apisix/plugin.lua b/apisix/plugin.lua
index 65179364e..a2898bc0d 100644
--- a/apisix/plugin.lua
+++ b/apisix/plugin.lua
@@ -1017,8 +1017,16 @@ local function check_single_plugin_schema(name,
plugin_conf, schema_type, skip_d
if plugin_obj.check_schema then
local ok, err = plugin_obj.check_schema(plugin_conf, schema_type)
if not ok then
- return false, "failed to check the configuration of plugin "
- .. name .. " err: " .. err
+ if check_disable(plugin_conf) ~= true then
+ return false, "failed to check the configuration of plugin "
+ .. name .. " err: " .. err
+ end
+
+ -- the plugin is disabled via _meta.disable so it will never be
+ -- executed: an environment dependent failure (e.g. proxy-cache
+ -- cache_zone not found on this node) must not invalidate the item
+ core.log.warn("failed to check the configuration of disabled
plugin ",
+ name, ", accepting it anyway")
end
if plugin_conf._meta then
@@ -1262,8 +1270,13 @@ local function stream_check_schema(plugins_conf,
schema_type, skip_disabled_plug
if plugin_obj.check_schema then
local ok, err = plugin_obj.check_schema(plugin_conf, schema_type)
if not ok then
- return false, "failed to check the configuration of "
- .. "stream plugin [" .. name .. "]: " .. err
+ if check_disable(plugin_conf) ~= true then
+ return false, "failed to check the configuration of "
+ .. "stream plugin [" .. name .. "]: " .. err
+ end
+
+ core.log.warn("failed to check the configuration of disabled ",
+ "stream plugin [", name, "], accepting it
anyway")
end
end
diff --git a/t/core/config_etcd.t b/t/core/config_etcd.t
index 9ccac862f..16deda060 100644
--- a/t/core/config_etcd.t
+++ b/t/core/config_etcd.t
@@ -615,3 +615,251 @@ passed
qr/invalid or missing X-Etcd-Index header/
--- grep_error_log_out eval
qr/(invalid or missing X-Etcd-Index header\n){1,}/
+
+
+
+=== TEST 16: full reload keeps the previous value of an item whose new data is
invalid
+--- timeout: 20
+--- yaml_config
+deployment:
+ role: traditional
+ role_traditional:
+ config_provider: etcd
+ etcd:
+ host:
+ - "http://127.0.0.1:2379";
+ prefix: /apisix
+--- extra_yaml_config
+nginx_config:
+ worker_processes: 1
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local etcd = require("resty.etcd")
+ local etcd_cli, err = etcd.new({
+ http_host = "http://127.0.0.1:2379";,
+ })
+ if not etcd_cli then
+ ngx.say("failed to create etcd client: ", err)
+ return
+ end
+
+ local valid = {
+ id = "1",
+ create_time = 1700000000,
+ update_time = 1700000000,
+ plugins = {
+ ["response-rewrite"] = {
+ headers = {
+ set = {["X-Global-Test"] = "hit"}
+ }
+ }
+ }
+ }
+ -- proxy-cache validates cache_zone against the zones declared in
+ -- the local config.yaml, so this value is permanently invalid on
+ -- this node while being perfectly storable in etcd
+ local invalid = core.table.deepcopy(valid)
+ invalid.plugins["proxy-cache"] = {
+ cache_zone = "no_such_zone_for_test",
+ cache_strategy = "disk",
+ cache_key = {"$host", "$request_uri"},
+ cache_method = {"GET"},
+ cache_http_status = {200},
+ }
+
+ local function global_rule(id)
+ local obj = core.config.fetch_created_obj("/global_rules")
+ for _, item in ipairs(obj.values or {}) do
+ if item and item.value and item.value.id == id then
+ return item.value
+ end
+ end
+ end
+
+ etcd_cli:set("/apisix/global_rules/1", valid)
+ ngx.sleep(1)
+ ngx.say("valid loaded: ", global_rule("1") ~= nil)
+
+ etcd_cli:set("/apisix/global_rules/1", invalid)
+ ngx.sleep(1)
+ ngx.say("kept by watch path: ", global_rule("1") ~= nil)
+
+ -- reproduce the state an etcd compaction leaves behind, then write
+ -- again to wake up the sync loop blocked on the watch semaphore
+ core.config.fetch_created_obj("/global_rules").need_reload = true
+ etcd_cli:set("/apisix/global_rules/1", invalid)
+ ngx.sleep(2)
+
+ local rule = global_rule("1")
+ ngx.say("kept by full reload: ",
+ rule ~= nil and rule.plugins["response-rewrite"] ~= nil)
+
+ etcd_cli:delete("/apisix/global_rules/1")
+ ngx.sleep(1)
+ }
+ }
+--- request
+GET /t
+--- response_body
+valid loaded: true
+kept by watch path: true
+kept by full reload: true
+--- error_log
+keep the previous configuration
+
+
+
+=== TEST 17: full reload does not resurrect an item that no longer exists in
etcd
+--- timeout: 20
+--- yaml_config
+deployment:
+ role: traditional
+ role_traditional:
+ config_provider: etcd
+ etcd:
+ host:
+ - "http://127.0.0.1:2379";
+ prefix: /apisix
+--- extra_yaml_config
+nginx_config:
+ worker_processes: 1
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local etcd = require("resty.etcd")
+ local etcd_cli, err = etcd.new({
+ http_host = "http://127.0.0.1:2379";,
+ })
+ if not etcd_cli then
+ ngx.say("failed to create etcd client: ", err)
+ return
+ end
+
+ local obj = core.config.fetch_created_obj("/global_rules")
+ obj.values = obj.values or {}
+ obj.values_hash = obj.values_hash or {}
+
+ -- an item that lives in memory but not in etcd, i.e. what the full
+ -- reload is expected to drop
+ local stale = {
+ key = "/apisix/global_rules/ghost",
+ modifiedIndex = 1,
+ clean_handlers = {},
+ value = {id = "ghost", plugins = {}},
+ }
+ core.table.insert(obj.values, stale)
+ obj.values_hash["ghost"] = #obj.values
+
+ obj.need_reload = true
+ etcd_cli:set("/apisix/global_rules/2", {
+ id = "2",
+ create_time = 1700000000,
+ update_time = 1700000000,
+ plugins = {
+ ["response-rewrite"] = {
+ headers = {
+ set = {["X-Global-Test"] = "hit"}
+ }
+ }
+ }
+ })
+ ngx.sleep(2)
+
+ local found_ghost, found_2 = false, false
+ for _, item in ipairs(obj.values or {}) do
+ if item and item.value and item.value.id == "ghost" then
+ found_ghost = true
+ end
+ if item and item.value and item.value.id == "2" then
+ found_2 = true
+ end
+ end
+ ngx.say("stale item resurrected: ", found_ghost)
+ ngx.say("valid item loaded: ", found_2)
+
+ etcd_cli:delete("/apisix/global_rules/2")
+ ngx.sleep(1)
+ }
+ }
+--- request
+GET /t
+--- response_body
+stale item resurrected: false
+valid item loaded: true
+
+
+
+=== TEST 18: full reload still skips an invalid item that has no previous value
+--- timeout: 20
+--- yaml_config
+deployment:
+ role: traditional
+ role_traditional:
+ config_provider: etcd
+ etcd:
+ host:
+ - "http://127.0.0.1:2379";
+ prefix: /apisix
+--- extra_yaml_config
+nginx_config:
+ worker_processes: 1
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local etcd = require("resty.etcd")
+ local etcd_cli, err = etcd.new({
+ http_host = "http://127.0.0.1:2379";,
+ })
+ if not etcd_cli then
+ ngx.say("failed to create etcd client: ", err)
+ return
+ end
+
+ local invalid = {
+ id = "9",
+ create_time = 1700000000,
+ update_time = 1700000000,
+ plugins = {
+ ["proxy-cache"] = {
+ cache_zone = "no_such_zone_for_test",
+ cache_strategy = "disk",
+ cache_key = {"$host", "$request_uri"},
+ cache_method = {"GET"},
+ cache_http_status = {200},
+ }
+ }
+ }
+
+ local function has_rule(id)
+ local obj = core.config.fetch_created_obj("/global_rules")
+ for _, item in ipairs(obj.values or {}) do
+ if item and item.value and item.value.id == id then
+ return true
+ end
+ end
+ return false
+ end
+
+ etcd_cli:set("/apisix/global_rules/9", invalid)
+ ngx.sleep(1)
+
+ core.config.fetch_created_obj("/global_rules").need_reload = true
+ etcd_cli:set("/apisix/global_rules/9", invalid)
+ ngx.sleep(2)
+
+ ngx.say("invalid new item loaded: ", has_rule("9"))
+
+ etcd_cli:delete("/apisix/global_rules/9")
+ ngx.sleep(1)
+ }
+ }
+--- request
+GET /t
+--- response_body
+invalid new item loaded: false
+--- no_error_log
+keep the previous configuration
diff --git a/t/plugin/proxy-cache/disk.t b/t/plugin/proxy-cache/disk.t
index 1d3a12459..06892f378 100644
--- a/t/plugin/proxy-cache/disk.t
+++ b/t/plugin/proxy-cache/disk.t
@@ -753,3 +753,84 @@ passed
GET /hello?bar=a
--- response_body chop
hello world!
+
+
+
+=== TEST 30: a disabled plugin with an invalid cache zone is accepted
+--- 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,
+ [[{
+ "plugins": {
+ "proxy-cache": {
+ "_meta": {"disable": true},
+ "cache_zone": "invalid_disk_cache",
+ "cache_method": ["GET"],
+ "cache_http_status": [200]
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1986": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- error_log
+failed to check the configuration of disabled plugin proxy-cache
+
+
+
+=== TEST 31: re-enabling the same plugin conf is still rejected
+--- 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,
+ [[{
+ "plugins": {
+ "proxy-cache": {
+ "_meta": {"disable": false},
+ "cache_zone": "invalid_disk_cache",
+ "cache_method": ["GET"],
+ "cache_http_status": [200]
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1986": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- error_code: 400
+--- response_body eval
+qr/cache_zone invalid_disk_cache not found/
