diff --git a/apisix/admin/init.lua b/apisix/admin/init.lua index c3f543e62949..a189bc76b7a8 100644 --- a/apisix/admin/init.lua +++ b/apisix/admin/init.lua @@ -285,9 +285,22 @@ 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 don't broadcast the event to the other workers + 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 + local success, err = events:post(reload_event, get_method(), ngx_time()) if not success then core.response.exit(503, err) @@ -373,13 +386,33 @@ 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") - plugin.load() + local ok, err = plugin.load() + 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 496b8b57d5fe..72bde7c82e5b 100644 --- a/apisix/control/v1.lua +++ b/apisix/control/v1.lua @@ -409,6 +409,15 @@ function _M.dump_plugin_metadata() end function _M.post_reload_plugins() + -- 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 + local success, err = events:post(_M.RELOAD_EVENT, ngx.req.get_method(), ngx.time()) if not success then core.response.exit(503, err) diff --git a/apisix/plugin.lua b/apisix/plugin.lua index 65179364e741..e48272144e89 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,97 @@ 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. + local old_plugins = core.table.clone(local_plugins) + for _, old_plugin in ipairs(old_plugins) do + destroy_plugin(old_plugin, http_plugin_type(old_plugin)) + end + + local load_err + for _, 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 + end + + if load_err then + for _, plugin in ipairs(new_plugins) do + 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 - for i, plugin in ipairs(local_plugins) do + 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 +452,80 @@ 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 _, old_plugin in ipairs(old_plugins) do + destroy_plugin(old_plugin, PLUGIN_TYPE_STREAM) + end + + local load_err + for _, plugin in ipairs(new_plugins) do + local ok, err = init_plugin(plugin, PLUGIN_TYPE_STREAM) + if not ok then + load_err = err + break + end + end + + if load_err then + for _, plugin in ipairs(new_plugins) do + destroy_plugin(plugin, 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 - for i, plugin in ipairs(stream_local_plugins) do + 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 +586,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 +599,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,9 +610,14 @@ 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 @@ -921,7 +1101,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 4a5e6e9a5008..729dcfded73d 100644 --- a/docs/en/latest/control-api.md +++ b/docs/en/latest/control-api.md @@ -486,6 +486,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..f97d726f253e --- /dev/null +++ b/t/admin/plugins-reload-transaction.t @@ -0,0 +1,190 @@ +# +# 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/ 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