diff --git a/apisix/admin/init.lua b/apisix/admin/init.lua index 3ef8fafc8fd0..cc747a346a1d 100644 --- a/apisix/admin/init.lua +++ b/apisix/admin/init.lua @@ -294,22 +294,40 @@ local function unsupported_methods_reload_plugin() end +-- defined after sync_local_conf_to_etcd +local reload_plugins_and_sync + + local function post_reload_plugins() set_ctx_and_check_token() + -- reload on this worker first: if the new plugin set cannot be loaded, + -- report the error to the operator instead of an unconditional "done", + -- and neither bump the version nor broadcast, so that neither the other + -- workers nor the reconciliation timer are asked to apply a plugin set + -- which is already known not to load + local ok, err = reload_plugins_and_sync() + if not ok then + core.log.error("failed to hot reload plugins: ", err) + core.response.exit(500, {error_msg = "failed to reload plugins: " .. err}) + end + if plugins_conf_ver_dict then - -- bump the version before broadcasting, so that a process which never - -- receives the event (e.g. the privileged agent while it is - -- reconnecting to the events broker) still converges through the - -- periodic reconciliation below - local _, err = plugins_conf_ver_dict:incr(PLUGINS_CONF_VERSION_KEY, 1, 0) - if err then + -- bump the version, so that a process which never receives the event + -- (e.g. the privileged agent while it is reconnecting to the events + -- broker) still converges through the periodic reconciliation below + local ver, incr_err = plugins_conf_ver_dict:incr(PLUGINS_CONF_VERSION_KEY, 1, 0) + if incr_err then -- if the version cannot be bumped the reconciliation timer will -- never notice a change, so a worker that misses the broadcast -- would stay stale forever; fail loud instead of pretending success - core.log.error("failed to increase plugins conf version: ", err) + core.log.error("failed to increase plugins conf version: ", incr_err) core.response.exit(503, {error_msg = "failed to record plugins reload"}) end + + -- this worker already applied the new set above, don't let its own + -- reconciliation timer redo the work one second later + applied_plugins_conf_version = ver end local success, err = events:post(reload_event, get_method(), ngx_time()) @@ -397,7 +415,7 @@ local function sync_local_conf_to_etcd(reset) end -local function reload_plugins(data, event, source, pid) +function reload_plugins_and_sync() core.log.info("start to hot reload plugins") -- sample the version before loading: if another reload is accepted while @@ -408,15 +426,39 @@ local function reload_plugins(data, event, source, pid) ver = plugins_conf_ver_dict:get(PLUGINS_CONF_VERSION_KEY) end - plugin.load() + local ok, err = plugin.load() + -- record the sampled version even when the load failed: the reconciliation + -- timer would otherwise retry the very same plugin set every second, and + -- every attempt tears the live set down and rebuilds it if ver then applied_plugins_conf_version = ver end + if not ok then + return nil, err + end + if ngx_worker_id() == 0 then sync_local_conf_to_etcd() end + + return true +end + + +local function reload_plugins(data, event, source, wid) + if wid == ngx_worker_id() then + -- this worker has already reloaded synchronously while serving the + -- Admin API request, see post_reload_plugins() + return + end + + local ok, err = reload_plugins_and_sync() + if not ok then + core.log.error("failed to hot reload plugins: ", err, + ", this worker keeps the old plugin set") + end end diff --git a/apisix/control/router.lua b/apisix/control/router.lua index e5044bf54be2..356f72d2ca32 100644 --- a/apisix/control/router.lua +++ b/apisix/control/router.lua @@ -199,9 +199,18 @@ end end -- do -local function reload_plugins() +local function reload_plugins(data, event, source, wid) + if wid == ngx.worker.id() then + -- already reloaded synchronously in post_reload_plugins() + return + end + core.log.info("start to hot reload plugins") - plugin_mod.load() + local ok, err = plugin_mod.load() + if not ok then + core.log.error("failed to hot reload plugins: ", err, + ", this worker keeps the old plugin set") + end end diff --git a/apisix/control/v1.lua b/apisix/control/v1.lua index 856bbeea30e2..9c35819acdea 100644 --- a/apisix/control/v1.lua +++ b/apisix/control/v1.lua @@ -412,11 +412,21 @@ function _M.dump_plugin_metadata() end function _M.post_reload_plugins() - -- Bump the shared version before broadcasting so that a worker which misses the - -- event (the resty.events broker gives no delivery guarantee while a worker is - -- reconnecting) still converges through the admin reconciliation timer. This is - -- the same guard the admin reload path added in #13714; the control path was - -- left out. When the admin is disabled the timer is absent and this is a no-op. + -- reload on this worker first so that a plugin set which cannot be loaded + -- is reported to the caller instead of being broadcast + core.log.info("start to hot reload plugins") + local ok, err = plugin.load() + if not ok then + core.log.error("failed to hot reload plugins: ", err) + core.response.exit(500, {error_msg = "failed to reload plugins: " .. err}) + end + + -- Bump the shared version once the load succeeded so that a worker which + -- misses the event (the resty.events broker gives no delivery guarantee + -- while a worker is reconnecting) still converges through the admin + -- reconciliation timer. This is the same guard the admin reload path added + -- in #13714; the control path was left out. When the admin is disabled the + -- timer is absent and this is a no-op. if plugins_conf_ver_dict then local _, incr_err = plugins_conf_ver_dict:incr(PLUGINS_CONF_VERSION_KEY, 1, 0) if incr_err then diff --git a/apisix/plugin.lua b/apisix/plugin.lua index 7a0159fd9341..45b70f01c61b 100644 --- a/apisix/plugin.lua +++ b/apisix/plugin.lua @@ -166,24 +166,92 @@ end local PLUGIN_TYPE_HTTP = 1 local PLUGIN_TYPE_STREAM = 2 local PLUGIN_TYPE_HTTP_WASM = 3 -local function unload_plugin(name, plugin_type) + +local function plugin_pkg_name(name, plugin_type) + if plugin_type == PLUGIN_TYPE_STREAM then + return "apisix.stream.plugins." .. name + end + + return "apisix.plugins." .. name +end + + +-- whether the init/destroy hooks of this plugin type run in the current +-- subsystem: stream plugins are also loaded in the HTTP subsystem so that +-- the Admin API can validate their schemas, but their hooks must only run +-- in the stream subsystem; wasm plugins have no init/destroy hooks +local function has_lifecycle(plugin_type) if plugin_type == PLUGIN_TYPE_HTTP_WASM then - return + return false end - -- Don't unload stream plugins in the HTTP subsystem. if plugin_type == PLUGIN_TYPE_STREAM and is_http then + return false + end + + return true +end + + +local function http_plugin_type(plugin) + if plugin.type == "wasm" then + return PLUGIN_TYPE_HTTP_WASM + end + + return PLUGIN_TYPE_HTTP +end + + +local function destroy_plugin(plugin, plugin_type) + if not has_lifecycle(plugin_type) then return end - local pkg_name = "apisix.plugins." .. name - if plugin_type == PLUGIN_TYPE_STREAM then - pkg_name = "apisix.stream.plugins." .. name + if type(plugin.destroy) ~= "function" then + return + end + + local ok, err = pcall(plugin.destroy) + if not ok then + core.log.error("failed to destroy plugin [", plugin.name, "]: ", err) + end +end + + +local function init_plugin(plugin, plugin_type) + if not has_lifecycle(plugin_type) then + return true + end + + if plugin.init then + local ok, err = pcall(plugin.init) + if not ok then + return nil, "failed to init plugin [" .. tostring(plugin.name) + .. "]: " .. tostring(err) + end + end + + if plugin.workflow_handler then + local ok, err = pcall(plugin.workflow_handler) + if not ok then + return nil, "failed to run the workflow handler of plugin [" + .. tostring(plugin.name) .. "]: " .. tostring(err) + end end + return true +end + + +local function unload_plugin(name, plugin_type) + if not has_lifecycle(plugin_type) then + return + end + + local pkg_name = plugin_pkg_name(name, plugin_type) local old_plugin = pkg_loaded[pkg_name] - if old_plugin and type(old_plugin.destroy) == "function" then - old_plugin.destroy() + if old_plugin then + destroy_plugin(old_plugin, plugin_type) end pkg_loaded[pkg_name] = nil @@ -197,12 +265,7 @@ local function load_plugin(name, plugins_list, plugin_type) ok, plugin = wasm.require(name) name = name.name else - local pkg_name = "apisix.plugins." .. name - if plugin_type == PLUGIN_TYPE_STREAM then - pkg_name = "apisix.stream.plugins." .. name - end - - ok, plugin = pcall(require, pkg_name) + ok, plugin = pcall(require, plugin_pkg_name(name, plugin_type)) end if not ok then @@ -252,21 +315,9 @@ local function load_plugin(name, plugins_list, plugin_type) plugin.attr = plugin_attr(name) core.table.insert(plugins_list, plugin) - -- Don't initialize stream plugins in the HTTP subsystem. - -- The modules are loaded for schema validation (admin API), - -- but init/workflow_handler functions must only run in the stream subsystem. - if plugin_type == PLUGIN_TYPE_STREAM and is_http then - return - end - - if plugin.init then - plugin.init() - end - - if plugin.workflow_handler then - plugin.workflow_handler() - end - + -- the init/workflow_handler hooks are not run here: they are run by the + -- caller once the whole new plugin set has been built, so that a failing + -- hook can be rolled back without leaving a half-built plugin table return end @@ -286,32 +337,108 @@ local function load(plugin_names, wasm_plugin_names) core.log.warn("new plugins: ", core.json.delay_encode(processed)) - for name, plugin in pairs(local_plugins_hash) do - local ty = PLUGIN_TYPE_HTTP - if plugin.type == "wasm" then - ty = PLUGIN_TYPE_HTTP_WASM + -- phase 1: build the new plugin set in a local table. The tables read by + -- the request path (local_plugins / local_plugins_hash) keep serving and + -- stay untouched if anything below fails. Drop the cached modules of the + -- currently loaded plugins so require() re-reads their code from disk on a + -- reload, and snapshot them for the rollback. Only the already-loaded set + -- is dropped: on the first load there is nothing to drop, so the modules + -- initialized in init_by_lua are reused as-is. + local pkg_snapshot = {} + for name, old_plugin in pairs(local_plugins_hash) do + if old_plugin.type ~= "wasm" then + local pkg_name = plugin_pkg_name(name, PLUGIN_TYPE_HTTP) + pkg_snapshot[pkg_name] = pkg_loaded[pkg_name] or false + pkg_loaded[pkg_name] = nil end - unload_plugin(name, ty) end - core.table.clear(local_plugins) - core.table.clear(local_plugins_hash) - + local new_plugins = core.table.new(32, 0) for name, value in pairs(processed) do local ty = PLUGIN_TYPE_HTTP if type(value) == "table" then ty = PLUGIN_TYPE_HTTP_WASM name = value end - load_plugin(name, local_plugins, ty) + load_plugin(name, new_plugins, ty) end -- sort by plugin's priority - if #local_plugins > 1 then - sort_tab(local_plugins, sort_plugin) + if #new_plugins > 1 then + sort_tab(new_plugins, sort_plugin) + end + + -- phase 2: destroy the old instances first, then run the init hooks of the + -- new instances. The order matters: some plugins register global resources + -- keyed by name (timers.register_timer), so a new instance registering + -- before the old one unregisters would lose the resource. If any hook + -- fails, roll everything back and keep serving with the current set. + -- destroy() runs in the reverse order of init(): plugins which wrap a + -- shared function (gm, then ocsp-stapling on top of it) restore what they + -- saved, so the wrappers have to be unwound LIFO, the last one installed + -- first. Only the new instances whose init() actually ran are destroyed + -- during the rollback: destroy() of an instance that was never initialized + -- would publish its uninitialized state, e.g. set + -- radixtree_sni.set_cert_and_key to a nil upvalue. + local old_plugins = core.table.clone(local_plugins) + for i = #old_plugins, 1, -1 do + local old_plugin = old_plugins[i] + destroy_plugin(old_plugin, http_plugin_type(old_plugin)) + end + + local load_err + local inited = 0 + for i, plugin in ipairs(new_plugins) do + local ok, err = init_plugin(plugin, http_plugin_type(plugin)) + if not ok then + load_err = err + break + end + inited = i end - for i, plugin in ipairs(local_plugins) do + if load_err then + for i = inited, 1, -1 do + local plugin = new_plugins[i] + destroy_plugin(plugin, http_plugin_type(plugin)) + end + + for pkg_name, mod in pairs(pkg_snapshot) do + pkg_loaded[pkg_name] = mod or nil + end + + for _, old_plugin in ipairs(old_plugins) do + local ok, err = init_plugin(old_plugin, http_plugin_type(old_plugin)) + if not ok then + core.log.error("failed to restore the old plugin after the ", + "aborted reload: ", err) + end + end + + return nil, load_err + end + + -- phase 3: commit. Unload the modules of the removed plugins, then + -- repopulate the live tables in place: their identity never changes + -- (_M.plugins / _M.plugins_hash keep pointing to them) and there is no + -- yield point between the clear and the end of the loop, so concurrent + -- requests never observe a partially updated plugin set. + local new_names = core.table.new(0, #new_plugins) + for _, plugin in ipairs(new_plugins) do + new_names[plugin.name] = true + end + + for name, old_plugin in pairs(local_plugins_hash) do + if not new_names[name] and old_plugin.type ~= "wasm" then + pkg_loaded[plugin_pkg_name(name, PLUGIN_TYPE_HTTP)] = nil + end + end + + core.table.clear(local_plugins) + core.table.clear(local_plugins_hash) + + for i, plugin in ipairs(new_plugins) do + local_plugins[i] = plugin local_plugins_hash[plugin.name] = plugin if enable_debug() then core.log.warn("loaded plugin and sort by priority:", @@ -336,23 +463,82 @@ local function load_stream(plugin_names) core.log.warn("new plugins: ", core.json.delay_encode(processed)) - for name in pairs(stream_local_plugins_hash) do - unload_plugin(name, PLUGIN_TYPE_STREAM) + -- the three phases below mirror load(), see the comments there. Only the + -- already-loaded stream plugins are dropped, so the first load reuses the + -- modules initialized in init_by_lua. + local pkg_snapshot = {} + if has_lifecycle(PLUGIN_TYPE_STREAM) then + for name in pairs(stream_local_plugins_hash) do + local pkg_name = plugin_pkg_name(name, PLUGIN_TYPE_STREAM) + pkg_snapshot[pkg_name] = pkg_loaded[pkg_name] or false + pkg_loaded[pkg_name] = nil + end end - core.table.clear(stream_local_plugins) - core.table.clear(stream_local_plugins_hash) - + local new_plugins = core.table.new(32, 0) for name in pairs(processed) do - load_plugin(name, stream_local_plugins, PLUGIN_TYPE_STREAM) + load_plugin(name, new_plugins, PLUGIN_TYPE_STREAM) end -- sort by plugin's priority - if #stream_local_plugins > 1 then - sort_tab(stream_local_plugins, sort_plugin) + if #new_plugins > 1 then + sort_tab(new_plugins, sort_plugin) + end + + local old_plugins = core.table.clone(stream_local_plugins) + for i = #old_plugins, 1, -1 do + destroy_plugin(old_plugins[i], PLUGIN_TYPE_STREAM) end - for i, plugin in ipairs(stream_local_plugins) do + local load_err + local inited = 0 + for i, plugin in ipairs(new_plugins) do + local ok, err = init_plugin(plugin, PLUGIN_TYPE_STREAM) + if not ok then + load_err = err + break + end + inited = i + end + + if load_err then + for i = inited, 1, -1 do + destroy_plugin(new_plugins[i], PLUGIN_TYPE_STREAM) + end + + for pkg_name, mod in pairs(pkg_snapshot) do + pkg_loaded[pkg_name] = mod or nil + end + + for _, old_plugin in ipairs(old_plugins) do + local ok, err = init_plugin(old_plugin, PLUGIN_TYPE_STREAM) + if not ok then + core.log.error("failed to restore the old stream plugin after ", + "the aborted reload: ", err) + end + end + + return nil, load_err + end + + if has_lifecycle(PLUGIN_TYPE_STREAM) then + local new_names = core.table.new(0, #new_plugins) + for _, plugin in ipairs(new_plugins) do + new_names[plugin.name] = true + end + + for name in pairs(stream_local_plugins_hash) do + if not new_names[name] then + pkg_loaded[plugin_pkg_name(name, PLUGIN_TYPE_STREAM)] = nil + end + end + end + + core.table.clear(stream_local_plugins) + core.table.clear(stream_local_plugins_hash) + + for i, plugin in ipairs(new_plugins) do + stream_local_plugins[i] = plugin stream_local_plugins_hash[plugin.name] = plugin if enable_debug() then core.log.warn("loaded stream plugin and sort by priority:", @@ -413,6 +599,7 @@ function _M.load(config) return local_plugins end + local load_err if ngx.config.subsystem == "http" then if not http_plugin_names then core.log.error("failed to read plugin list from local file") @@ -425,6 +612,7 @@ function _M.load(config) local ok, err = load(http_plugin_names, wasm_plugin_names) if not ok then core.log.error("failed to load plugins: ", err) + load_err = err end end end @@ -435,28 +623,31 @@ function _M.load(config) local ok, err = load_stream(stream_plugin_names) if not ok then core.log.error("failed to load stream plugins: ", err) + load_err = load_err or err end end + if load_err then + return nil, load_err + end + -- for test return local_plugins end function _M.exit_worker() - for name, plugin in pairs(local_plugins_hash) do - local ty = PLUGIN_TYPE_HTTP - if plugin.type == "wasm" then - ty = PLUGIN_TYPE_HTTP_WASM - end - unload_plugin(name, ty) + -- same LIFO unwind as a reload does, see load() + for i = #local_plugins, 1, -1 do + local plugin = local_plugins[i] + unload_plugin(plugin.name, http_plugin_type(plugin)) end -- we need to load stream plugin so that we can check their schemas in -- Admin API. Maybe we can avoid calling `load` in this case? So that -- we don't need to call `destroy` too - for name in pairs(stream_local_plugins_hash) do - unload_plugin(name, PLUGIN_TYPE_STREAM) + for i = #stream_local_plugins, 1, -1 do + unload_plugin(stream_local_plugins[i].name, PLUGIN_TYPE_STREAM) end end @@ -921,7 +1112,13 @@ end function _M.init_worker() -- someone's plugin needs to be initialized after prometheus -- see https://github.com/apache/apisix/issues/3286 - _M.load() + local _, err = _M.load() + if err then + -- fail loudly on the initial load, like the unprotected init() used + -- to: starting with a silently reduced plugin set would fail open, + -- e.g. the auth plugins would simply be skipped + error("failed to load the plugins: " .. err) + end if local_conf and not local_conf.apisix.enable_admin then init_plugins_syncer() diff --git a/docs/en/latest/admin-api.md b/docs/en/latest/admin-api.md index 31496f1aadfd..9ee7307bc1be 100644 --- a/docs/en/latest/admin-api.md +++ b/docs/en/latest/admin-api.md @@ -1473,6 +1473,14 @@ The interface of getting properties of all plugins via `/apisix/admin/plugins?al ::: +:::note + +If the new plugin list cannot be loaded, for instance because the `init()` function of +one of the plugins fails, `/apisix/admin/plugins/reload` returns `500` together with the +error message and every worker keeps serving with its previous plugin set. + +::: + ### Request Body Parameters The Plugin ({plugin_name}) of the data structure. diff --git a/docs/en/latest/control-api.md b/docs/en/latest/control-api.md index ae7f59d0ed38..6328039e35a3 100644 --- a/docs/en/latest/control-api.md +++ b/docs/en/latest/control-api.md @@ -478,6 +478,10 @@ Triggers a hot reload of the plugins. curl "http://127.0.0.1:9090/v1/plugins/reload" -X PUT ``` +If the new plugin list cannot be loaded, for instance because the `init()` function of one +of the plugins fails, the endpoint returns `500` together with the error message and every +worker keeps serving with its previous plugin set. + ### GET /v1/discovery/{service}/dump Get memory dump of discovered service endpoints and configuration details: diff --git a/t/admin/plugins-reload-transaction.t b/t/admin/plugins-reload-transaction.t new file mode 100644 index 000000000000..71a0f9cb3771 --- /dev/null +++ b/t/admin/plugins-reload-transaction.t @@ -0,0 +1,314 @@ +# +# 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"); +# a single worker so that the Admin API request and the plugin table +# inspected afterwards always belong to the same worker +workers(1); + +run_tests; + +__DATA__ + +=== TEST 1: a plugin whose init() throws aborts the reload and rolls it back +--- yaml_config +apisix: + node_listen: 1984 +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +plugins: + - response-rewrite +--- config +location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local core = require("apisix.core") + local http = require("resty.http") + + local route_conf = [[{ + "uri": "/hello", + "plugins": {"response-rewrite": {"body": "REWRITTEN\n"}}, + "upstream": {"nodes": {"127.0.0.1:1980": 1}, "type": "roundrobin"} + }]] + + local code = t('/apisix/admin/routes/1', ngx.HTTP_PUT, route_conf) + ngx.say("admin PUT before reload: ", code) + + ngx.sleep(0.6) + local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/hello" + local res = http.new():request_uri(uri) + ngx.print("dataplane before reload: ", res.body) + + -- keep response-rewrite and add a plugin whose init() throws + require("lib.test_admin").set_config_yaml([[ +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +apisix: + node_listen: 1984 +plugins: + - response-rewrite + - reload-bad-init +]]) + local code2, body2 = t('/apisix/admin/plugins/reload', ngx.HTTP_PUT) + ngx.say("reload: ", code2, " ", body2) + ngx.sleep(2) + + -- the live plugin tables must still hold the previous plugin set: + -- plugin.plugins is read by the request path, plugin.plugins_hash by + -- the Admin API schema validation + local plugin = require("apisix.plugin") + local names = {} + for _, p in ipairs(plugin.plugins) do + core.table.insert(names, p.name) + end + ngx.say("after reload: plugins_hash has response-rewrite=", + plugin.plugins_hash["response-rewrite"] ~= nil, + ", plugins array=[", core.table.concat(names, ","), "]") + + -- the very same route conf is still accepted + local code3 = t('/apisix/admin/routes/1', ngx.HTTP_PUT, route_conf) + ngx.say("admin PUT after reload: ", code3) + + local res2 = http.new():request_uri(uri) + ngx.print("dataplane after reload: ", res2.body) + } +} +--- request +GET /t +--- response_body eval +qr/^admin PUT before reload: 20[01] +dataplane before reload: REWRITTEN +reload: 500 \{"error_msg":"failed to reload plugins: failed to init plugin \[reload-bad-init\].*boom.*"\}\s* +after reload: plugins_hash has response-rewrite=true, plugins array=\[response-rewrite\] +admin PUT after reload: 200 +dataplane after reload: REWRITTEN +$/s +--- timeout: 15 +--- error_log eval +qr/reload-bad-init: init\(\) boom/ + + + +=== TEST 2: a failed reload leaves no sticky state, the next one succeeds +--- yaml_config +apisix: + node_listen: 1984 +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +plugins: + - response-rewrite +--- config +location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local core = require("apisix.core") + + require("lib.test_admin").set_config_yaml([[ +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +apisix: + node_listen: 1984 +plugins: + - response-rewrite + - reload-bad-init +]]) + local code, _, body = t('/apisix/admin/plugins/reload', ngx.HTTP_PUT) + ngx.say("failing reload: ", code) + ngx.sleep(1) + + require("lib.test_admin").set_config_yaml([[ +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +apisix: + node_listen: 1984 +plugins: + - response-rewrite + - key-auth +]]) + local code2, _, body2 = t('/apisix/admin/plugins/reload', ngx.HTTP_PUT) + ngx.say("recovering reload: ", code2, " ", body2) + ngx.sleep(1) + + local plugin = require("apisix.plugin") + local names = {} + for _, p in ipairs(plugin.plugins) do + core.table.insert(names, p.name) + end + table.sort(names) + ngx.say("plugins array=[", core.table.concat(names, ","), "]") + ngx.say("key-auth in hash: ", plugin.plugins_hash["key-auth"] ~= nil) + } +} +--- request +GET /t +--- response_body +failing reload: 500 +recovering reload: 200 done +plugins array=[key-auth,response-rewrite] +key-auth in hash: true +--- timeout: 15 +--- error_log eval +qr/reload-bad-init: init\(\) boom/ + + + +=== TEST 3: the rollback only destroys the new instances which were initialized +--- yaml_config +apisix: + node_listen: 1984 +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +plugins: + - response-rewrite + - reload-probe +--- config +location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local state = require("lib.reload_probe_state") + + -- the initial load has initialized the old reload-probe instance + ngx.say("after start: init=", state.init, " destroy=", state.destroy) + + -- reload-bad-init has a higher priority than reload-probe, so it fails + -- before the new reload-probe instance is initialized + require("lib.test_admin").set_config_yaml([[ +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +apisix: + node_listen: 1984 +plugins: + - response-rewrite + - reload-bad-init + - reload-probe +]]) + local code = t('/apisix/admin/plugins/reload', ngx.HTTP_PUT) + ngx.say("reload: ", code) + ngx.sleep(1) + + -- the old instance was destroyed once and re-initialized by the + -- rollback; the new one was never initialized, so it must never have + -- been destroyed either + ngx.say("after rollback: init=", state.init, " destroy=", state.destroy, + " destroy_without_init=", state.destroy_without_init) + + local plugin = require("apisix.plugin") + ngx.say("reload-probe still live: ", + plugin.plugins_hash["reload-probe"] ~= nil) + } +} +--- request +GET /t +--- response_body +after start: init=1 destroy=0 +reload: 500 +after rollback: init=2 destroy=1 destroy_without_init=0 +reload-probe still live: true +--- timeout: 15 +--- error_log eval +qr/reload-bad-init: init\(\) boom/ + + + +=== TEST 4: the destroy hooks run in the reverse order of the init hooks +--- yaml_config +apisix: + node_listen: 1984 +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +plugins: + - reload-probe + - reload-probe-2 +--- config +location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local core = require("apisix.core") + local state = require("lib.reload_probe_state") + + -- reload-bad-init has the highest priority of the three, so it fails + -- before any new instance is initialized: nothing of the new set may + -- be destroyed, and the old set is unwound in the reverse order of + -- its init() and then restored in the init() order + require("lib.test_admin").set_config_yaml([[ +deployment: + role: traditional + role_traditional: + config_provider: etcd + admin: + admin_key: null +apisix: + node_listen: 1984 +plugins: + - reload-probe + - reload-bad-init + - reload-probe-2 +]]) + local code = t('/apisix/admin/plugins/reload', ngx.HTTP_PUT) + ngx.say("reload: ", code) + ngx.sleep(1) + + ngx.say("hooks: ", core.table.concat(state.events, " ")) + ngx.say("destroy_without_init=", state.destroy_without_init) + } +} +--- request +GET /t +--- response_body +reload: 500 +hooks: reload-probe:init reload-probe-2:init reload-probe-2:destroy reload-probe:destroy reload-probe:init reload-probe-2:init +destroy_without_init=0 +--- timeout: 15 +--- error_log eval +qr/reload-bad-init: init\(\) boom/ diff --git a/t/apisix/plugins/reload-bad-init.lua b/t/apisix/plugins/reload-bad-init.lua new file mode 100644 index 000000000000..3d845800c66e --- /dev/null +++ b/t/apisix/plugins/reload-bad-init.lua @@ -0,0 +1,33 @@ +-- +-- 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. +-- + +-- A test only plugin whose init() always throws, used to check that a failing +-- plugin reload is rolled back instead of leaving a half-built plugin table. +local _M = { + version = 0.1, + priority = 412, + name = "reload-bad-init", + schema = {type = "object"}, +} + + +function _M.init() + error("reload-bad-init: init() boom") +end + + +return _M diff --git a/t/apisix/plugins/reload-probe-2.lua b/t/apisix/plugins/reload-probe-2.lua new file mode 100644 index 000000000000..2934f3114d29 --- /dev/null +++ b/t/apisix/plugins/reload-probe-2.lua @@ -0,0 +1,52 @@ +-- +-- 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. +-- + +-- A second lifecycle probe, with a lower priority than reload-probe, so that +-- the two are initialized in a known order and the destroy order can be +-- asserted against it. +local state = require("lib.reload_probe_state") + +local plugin_name = "reload-probe-2" +local inited = false + +local _M = { + version = 0.1, + priority = 410, + name = plugin_name, + schema = {type = "object"}, +} + + +function _M.init() + inited = true + state.init = state.init + 1 + state.record(plugin_name, "init") +end + + +function _M.destroy() + if not inited then + state.destroy_without_init = state.destroy_without_init + 1 + end + + inited = false + state.destroy = state.destroy + 1 + state.record(plugin_name, "destroy") +end + + +return _M diff --git a/t/apisix/plugins/reload-probe.lua b/t/apisix/plugins/reload-probe.lua new file mode 100644 index 000000000000..76883649db18 --- /dev/null +++ b/t/apisix/plugins/reload-probe.lua @@ -0,0 +1,52 @@ +-- +-- 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. +-- + +-- A test only plugin recording its lifecycle hooks. Its priority is lower than +-- the one of reload-bad-init, so it is initialized after it: when the reload is +-- aborted this instance has never been initialized and must not be destroyed. +local state = require("lib.reload_probe_state") + +local plugin_name = "reload-probe" +local inited = false + +local _M = { + version = 0.1, + priority = 411, + name = plugin_name, + schema = {type = "object"}, +} + + +function _M.init() + inited = true + state.init = state.init + 1 + state.record(plugin_name, "init") +end + + +function _M.destroy() + if not inited then + state.destroy_without_init = state.destroy_without_init + 1 + end + + inited = false + state.destroy = state.destroy + 1 + state.record(plugin_name, "destroy") +end + + +return _M diff --git a/t/lib/reload_probe_state.lua b/t/lib/reload_probe_state.lua new file mode 100644 index 000000000000..3ec287fc5b6b --- /dev/null +++ b/t/lib/reload_probe_state.lua @@ -0,0 +1,36 @@ +-- +-- 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. +-- + +-- Records the lifecycle hooks the reload-probe test plugins have seen. It lives +-- outside the apisix.plugins package so that the plugin loader never drops it +-- from package.loaded, hence the counters survive a reload. +local _M = { + init = 0, + destroy = 0, + -- destroy() calls on an instance whose init() never ran + destroy_without_init = 0, + -- ":" entries in the order the hooks ran + events = {}, +} + + +function _M.record(name, hook) + _M.events[#_M.events + 1] = name .. ":" .. hook +end + + +return _M