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/apisix/plugin.lua b/apisix/plugin.lua index 65179364e741..a2898bc0d404 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 9ccac862f441..16deda060d2e 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 1d3a12459f51..06892f378e25 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/