From 817380e8553bbaf8e109b67860a53702cae6f071 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 20 Jul 2026 15:28:10 +0800 Subject: [PATCH 1/5] fix(config_etcd): keep the previous value when a full reload gets invalid data The full reload path (load_full_data) and the incremental watch path treat an item that fails validation differently: - the watch path only cancels the update of that item and keeps the value already in memory (`goto CONTINUE`, see #11268); - the full reload path fires the clean handlers of every old item before reading from etcd and then skips the invalid item, so the item silently disappears from the data plane. A full reload is triggered whenever the watch is cancelled by etcd with `compacted`, which happens on ordinary etcd auto compaction when the global revision moved on outside the watched prefix. An item that has been rejected by the validation for a long time without any impact therefore vanishes at a random point in time. Since global plugins usually all live on the same global rule, one invalid plugin conf takes down every global plugin. Make the full reload behave like the watch path: pass the previous values to load_full_data and, when the new data of a key fails the validation and a previous value for the same key exists, carry the old item over as is - no clean handlers, no filter re-run, no version bump. Keys that are absent from the readdir result are still dropped, and an invalid item without a previous value (first load) is still skipped. The clean handlers of the old items that were replaced or deleted are now fired after the new table has been built. Fixes #12834 --- apisix/core/config_etcd.lua | 72 +++++++++-- t/core/config_etcd.t | 245 ++++++++++++++++++++++++++++++++++++ 2 files changed, 307 insertions(+), 10 deletions(-) diff --git a/apisix/core/config_etcd.lua b/apisix/core/config_etcd.lua index a56a221b6a18..79a66bf883db 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/t/core/config_etcd.t b/t/core/config_etcd.t index 9ccac862f441..9264748ec707 100644 --- a/t/core/config_etcd.t +++ b/t/core/config_etcd.t @@ -615,3 +615,248 @@ 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 +--- 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 +--- 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 +--- 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 From 522650a8df84089a5b1e169ea8b791644e5afac3 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 20 Jul 2026 15:29:43 +0800 Subject: [PATCH 2/5] change(plugin): accept a disabled plugin whose check_schema fails A plugin conf carrying `_meta.disable: true` is never executed, but its check_schema is still run unconditionally, and a failure invalidates the whole item (route, service, global rule, ...). Some plugins validate the local environment inside check_schema - proxy-cache requires cache_zone to be declared in the config.yaml of this very node - so a conf that is legal elsewhere permanently invalidates the item on this node, even though the plugin is turned off. Keep running check_schema (its side effects, e.g. default injection, are still wanted) but only reject the conf when the plugin is enabled; a disabled plugin logs a warning and is accepted. Re-enabling it goes through a full write and is validated again, so an invalid conf cannot silently become active. Same treatment for the stream subsystem. --- apisix/plugin.lua | 21 ++++++++-- t/plugin/proxy-cache/disk.t | 79 +++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/apisix/plugin.lua b/apisix/plugin.lua index 65179364e741..2c2ae67727e2 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 not check_disable(plugin_conf) 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, ", accept it anyway, err: ", err) 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 not check_disable(plugin_conf) 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, "], accept it anyway: ", err) end end diff --git a/t/plugin/proxy-cache/disk.t b/t/plugin/proxy-cache/disk.t index 1d3a12459f51..ccf631249200 100644 --- a/t/plugin/proxy-cache/disk.t +++ b/t/plugin/proxy-cache/disk.t @@ -753,3 +753,82 @@ 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 +--- 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/ From f2acdab2b75b5685bcd4117a4e97276ba30e8d06 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 20 Jul 2026 15:43:14 +0800 Subject: [PATCH 3/5] test(config_etcd): give the full-reload cases room to run The blocks drive a real etcd sync round and sleep past the 5s default block timeout, so they died on a client socket timeout rather than on an assertion. --- t/core/config_etcd.t | 3 +++ 1 file changed, 3 insertions(+) diff --git a/t/core/config_etcd.t b/t/core/config_etcd.t index 9264748ec707..16deda060d2e 100644 --- a/t/core/config_etcd.t +++ b/t/core/config_etcd.t @@ -619,6 +619,7 @@ 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 @@ -711,6 +712,7 @@ 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 @@ -791,6 +793,7 @@ 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 From 8f7a574dcff73459a079348888b1c787cc1a5dab Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 20 Jul 2026 16:39:54 +0800 Subject: [PATCH 4/5] fix(plugin): only accept an invalid conf when the plugin is explicitly disabled check_disable returns _meta.disable verbatim, so a truthy non-boolean value such as the string "false" would have taken the downgrade path and silently accepted a conf that the schema should have rejected. --- apisix/plugin.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apisix/plugin.lua b/apisix/plugin.lua index 2c2ae67727e2..7fe13ca1e861 100644 --- a/apisix/plugin.lua +++ b/apisix/plugin.lua @@ -1017,7 +1017,7 @@ 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 - if not check_disable(plugin_conf) then + if check_disable(plugin_conf) ~= true then return false, "failed to check the configuration of plugin " .. name .. " err: " .. err end @@ -1270,7 +1270,7 @@ 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 - if not check_disable(plugin_conf) then + if check_disable(plugin_conf) ~= true then return false, "failed to check the configuration of " .. "stream plugin [" .. name .. "]: " .. err end From 85e78848277967fa93483edb14057315067f75e3 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Tue, 21 Jul 2026 08:06:09 +0800 Subject: [PATCH 5/5] fix(plugin): do not log the schema error for a disabled invalid plugin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The downgrade for _meta.disable plugins piped the check_schema error straight into a warn. That string can echo config values (e.g. a pattern mismatch), so a disabled plugin's bad config could leak into the shared error log. Keep the warn as the signal that the config did not validate, but drop the error detail — the caller can still see it by re-submitting with the plugin enabled. Also assert the admin PUT actually returns passed in the disk.t case, not just the log line. --- apisix/plugin.lua | 4 ++-- t/plugin/proxy-cache/disk.t | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/apisix/plugin.lua b/apisix/plugin.lua index 7fe13ca1e861..a2898bc0d404 100644 --- a/apisix/plugin.lua +++ b/apisix/plugin.lua @@ -1026,7 +1026,7 @@ local function check_single_plugin_schema(name, plugin_conf, schema_type, skip_d -- 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, ", accept it anyway, err: ", err) + name, ", accepting it anyway") end if plugin_conf._meta then @@ -1276,7 +1276,7 @@ local function stream_check_schema(plugins_conf, schema_type, skip_disabled_plug end core.log.warn("failed to check the configuration of disabled ", - "stream plugin [", name, "], accept it anyway: ", err) + "stream plugin [", name, "], accepting it anyway") end end diff --git a/t/plugin/proxy-cache/disk.t b/t/plugin/proxy-cache/disk.t index ccf631249200..06892f378e25 100644 --- a/t/plugin/proxy-cache/disk.t +++ b/t/plugin/proxy-cache/disk.t @@ -790,6 +790,8 @@ hello world! } --- request GET /t +--- response_body +passed --- error_log failed to check the configuration of disabled plugin proxy-cache