From e9a73096340792bb130495e3e721431fc855c90b Mon Sep 17 00:00:00 2001 From: Zhefeng Chen Date: Fri, 4 Aug 2023 14:56:30 +0800 Subject: [PATCH] fix(*): drop luasocket in cli This is the follow-up PR of https://github.com/Kong/kong/pull/11127 Changing the socket type from luasocket to openresty cosocket causes some test fail weirdly. After investigating, it's mainly because the cosocket support yield and setkeepalive. See the comments in tests. https://konghq.atlassian.net/browse/FTI-4937 --- CHANGELOG.md | 2 + bin/busted | 8 ++ kong/cmd/utils/migrations.lua | 3 + kong/db/strategies/postgres/connector.lua | 23 ++--- .../02-cmd/10-migrations_spec.lua | 4 + spec/02-integration/03-db/01-db_spec.lua | 69 +++++++------- .../03-db/04-db_cluster_mutex_spec.lua | 56 ++++++----- .../28-grpc-gateway/01-proxy_spec.lua | 6 ++ spec/require.lua | 93 +++++++++++++++++++ 9 files changed, 191 insertions(+), 73 deletions(-) create mode 100644 spec/require.lua diff --git a/CHANGELOG.md b/CHANGELOG.md index 0eb35dafb205..5c3b6ca75da8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -150,6 +150,8 @@ [#11090](https://github.com/Kong/kong/pull/11090) - Remove kong branding from kong HTML error template. [#11150](https://github.com/Kong/kong/pull/11150) +- Drop luasocket in cli + [#11177](https://github.com/Kong/kong/pull/11177) #### Status API diff --git a/bin/busted b/bin/busted index dfc41fec1230..e676a55c3acf 100755 --- a/bin/busted +++ b/bin/busted @@ -59,6 +59,14 @@ require("kong.globalpatches")({ rbusted = true }) +-- some libraries used in test like spec/helpers +-- calls cosocket in module level, and as LuaJIT's +-- `require` is implemented in C, this throws +-- "attempt to yield across C-call boundary" error +-- the following pure-lua implementation is to bypass +-- this limitation, without need to modify all tests +_G.require = require "spec.require".require + -- Busted command-line runner require 'busted.runner'({ standalone = false }) diff --git a/kong/cmd/utils/migrations.lua b/kong/cmd/utils/migrations.lua index c450654fbc3b..bd39250b9962 100644 --- a/kong/cmd/utils/migrations.lua +++ b/kong/cmd/utils/migrations.lua @@ -195,6 +195,9 @@ local function reset(schema_state, db, ttl) -- failed to acquire locks - maybe locks table was dropped? log.error(err .. " - retrying without cluster lock") log("Resetting database...") + -- ROLLBACK in order to solve this error + -- ERROR: current transaction is aborted, commands ignored until end of transaction block + assert(db.connector:query("ROLLBACK;")) assert(db:schema_reset()) log("Database successfully reset") return true diff --git a/kong/db/strategies/postgres/connector.lua b/kong/db/strategies/postgres/connector.lua index cd3750ce7654..b4019a42fba2 100644 --- a/kong/db/strategies/postgres/connector.lua +++ b/kong/db/strategies/postgres/connector.lua @@ -195,10 +195,7 @@ local setkeepalive local function reconnect(config) local phase = get_phase() - if phase == "init" or phase == "init_worker" or ngx.IS_CLI then - -- Force LuaSocket usage in the CLI in order to allow for self-signed - -- certificates to be trusted (via opts.cafile) in the resty-cli - -- interpreter (no way to set lua_ssl_trusted_certificate). + if phase == "init" or phase == "init_worker" then config.socket_type = "luasocket" else @@ -219,16 +216,16 @@ local function reconnect(config) return nil, err end - if connection.sock:getreusedtimes() == 0 then - if config.schema == "" then - local res = connection:query("SELECT CURRENT_SCHEMA AS schema") - if res and res[1] and res[1].schema and res[1].schema ~= null then - config.schema = res[1].schema - else - config.schema = "public" - end + if config.schema == "" then + local res = connection:query("SELECT CURRENT_SCHEMA AS schema") + if res and res[1] and res[1].schema and res[1].schema ~= null then + config.schema = res[1].schema + else + config.schema = "public" end + end + if connection.sock:getreusedtimes() == 0 then ok, err = connection:query(concat { "SET SCHEMA ", connection:escape_literal(config.schema), ";\n", "SET TIME ZONE ", connection:escape_literal("UTC"), ";", @@ -298,7 +295,7 @@ function _mt:init() local res, err = self:query("SHOW server_version_num;") local ver = tonumber(res and res[1] and res[1].server_version_num) if not ver then - return nil, "failed to retrieve PostgreSQL server_version_num: " .. err + return nil, "failed to retrieve PostgreSQL server_version_num: " .. (err or "") end local major = floor(ver / 10000) diff --git a/spec/02-integration/02-cmd/10-migrations_spec.lua b/spec/02-integration/02-cmd/10-migrations_spec.lua index c829fad89333..72d9d678c183 100644 --- a/spec/02-integration/02-cmd/10-migrations_spec.lua +++ b/spec/02-integration/02-cmd/10-migrations_spec.lua @@ -44,7 +44,11 @@ for _, strategy in helpers.each_strategy() do local db = assert(DB.new(tmp_conf, strategy)) assert(db:init_connector()) + -- in spec/helpers.lua, db has already been init'ed + -- the stored connection will be reused here, + -- so we need to set schema explicitly to 'kong_migrations_tests' assert(db:connect()) + assert(db.connector:query("SET SCHEMA 'kong_migrations_tests';\n")) finally(function() db.connector:close() end) diff --git a/spec/02-integration/03-db/01-db_spec.lua b/spec/02-integration/03-db/01-db_spec.lua index 91d9c7015880..a4945f6ab76a 100644 --- a/spec/02-integration/03-db/01-db_spec.lua +++ b/spec/02-integration/03-db/01-db_spec.lua @@ -236,12 +236,7 @@ for _, strategy in helpers.each_strategy() do assert(db:close()) end) - it("returns opened connection when using cosockets", function() - -- bin/busted runs with ngx.IS_CLI = true, which forces luasocket to - -- be used in the DB connector (for custom CAs to work) - -- Disable this behavior for this test, especially considering we - -- are running within resty-cli, and thus within timer_by_lua (which - -- can support cosockets). + it("returns opened connection when IS_CLI=false", function() ngx.IS_CLI = false local db, err = DB.new(helpers.test_conf, strategy) @@ -264,7 +259,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - it("returns opened connection when using luasocket", function() + it("returns opened connection when IS_CLI=true", function() ngx.IS_CLI = true local db, err = DB.new(helpers.test_conf, strategy) @@ -278,7 +273,7 @@ for _, strategy in helpers.each_strategy() do assert.is_table(conn) if strategy == "postgres" then - assert.equal("luasocket", db.connector:get_stored_connection().sock_type) + assert.equal("nginx", db.connector:get_stored_connection().sock_type) assert.is_false(db.connector:get_stored_connection().config.ssl) end @@ -286,7 +281,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - postgres_only("returns opened connection with ssl (cosockets)", function() + postgres_only("returns opened connection with ssl (IS_CLI=false)", function() ngx.IS_CLI = false local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -312,7 +307,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - postgres_only("returns opened connection with ssl (luasocket)", function() + postgres_only("returns opened connection with ssl (IS_CLI=true)", function() ngx.IS_CLI = true local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -330,7 +325,7 @@ for _, strategy in helpers.each_strategy() do assert.is_table(conn) if strategy == "postgres" then - assert.equal("luasocket", db.connector:get_stored_connection().sock_type) + assert.equal("nginx", db.connector:get_stored_connection().sock_type) assert.is_true(db.connector:get_stored_connection().config.ssl) end @@ -338,7 +333,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - postgres_only("connects to postgres with readonly account (cosockets)", function() + postgres_only("connects to postgres with readonly account (IS_CLI=false)", function() ngx.IS_CLI = false local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -366,7 +361,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - postgres_only("connects to postgres with readonly account (luasocket)", function() + postgres_only("connects to postgres with readonly account (IS_CLI=true)", function() ngx.IS_CLI = true local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -385,13 +380,13 @@ for _, strategy in helpers.each_strategy() do assert.is_nil(db.connector:get_stored_connection("read")) - assert.equal("luasocket", db.connector:get_stored_connection("write").sock_type) + assert.equal("nginx", db.connector:get_stored_connection("write").sock_type) if strategy == "portgres" then assert.is_false(db.connector:get_stored_connection("write").config.ssl) end - assert.equal("luasocket", db.connector:get_stored_connection().sock_type) + assert.equal("nginx", db.connector:get_stored_connection().sock_type) if strategy == "portgres" then assert.is_false(db.connector:get_stored_connection("write").config.ssl) @@ -407,7 +402,7 @@ for _, strategy in helpers.each_strategy() do helpers.get_db_utils(strategy, {}) end) - it("returns true when there is a stored connection (cosockets)", function() + it("returns true when there is a stored connection (IS_CLI=false)", function() ngx.IS_CLI = false local db, err = DB.new(helpers.test_conf, strategy) @@ -432,7 +427,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - it("returns true when there is a stored connection (luasocket)", function() + it("returns true when there is a stored connection (IS_CLI=true)", function() ngx.IS_CLI = true local db, err = DB.new(helpers.test_conf, strategy) @@ -446,7 +441,7 @@ for _, strategy in helpers.each_strategy() do assert.is_table(conn) if strategy == "postgres" then - assert.equal("luasocket", db.connector:get_stored_connection().sock_type) + assert.equal("nginx", db.connector:get_stored_connection().sock_type) assert.is_false(db.connector:get_stored_connection().config.ssl) end @@ -457,7 +452,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - postgres_only("returns true when there is a stored connection with ssl (cosockets)", function() + postgres_only("returns true when there is a stored connection with ssl (IS_CLI=false)", function() ngx.IS_CLI = false local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -485,7 +480,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - postgres_only("returns true when there is a stored connection with ssl (luasocket)", function() + postgres_only("returns true when there is a stored connection with ssl (IS_CLI=true)", function() ngx.IS_CLI = true local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -503,7 +498,7 @@ for _, strategy in helpers.each_strategy() do assert.is_table(conn) if strategy == "postgres" then - assert.equal("luasocket", db.connector:get_stored_connection().sock_type) + assert.equal("nginx", db.connector:get_stored_connection().sock_type) assert.is_true(db.connector:get_stored_connection().config.ssl) end @@ -514,7 +509,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - it("returns true when there is no stored connection (cosockets)", function() + it("returns true when there is no stored connection (IS_CLI=false)", function() ngx.IS_CLI = false local db, err = DB.new(helpers.test_conf, strategy) @@ -527,7 +522,7 @@ for _, strategy in helpers.each_strategy() do assert.is_true(db:setkeepalive()) end) - it("returns true when there is no stored connection (luasocket)", function() + it("returns true when there is no stored connection (IS_CLI=true)", function() ngx.IS_CLI = true local db, err = DB.new(helpers.test_conf, strategy) @@ -540,7 +535,7 @@ for _, strategy in helpers.each_strategy() do assert.is_true(db:setkeepalive()) end) - postgres_only("keepalives both read only and write connection (cosockets)", function() + postgres_only("keepalives both read only and write connection (IS_CLI=false)", function() ngx.IS_CLI = false local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -577,7 +572,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - postgres_only("connects and keepalives only write connection (luasocket)", function() + postgres_only("connects and keepalives only write connection (IS_CLI=true)", function() ngx.IS_CLI = true local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -598,7 +593,7 @@ for _, strategy in helpers.each_strategy() do assert.is_nil(err) assert.is_table(conn) - assert.equal("luasocket", db.connector:get_stored_connection("write").sock_type) + assert.equal("nginx", db.connector:get_stored_connection("write").sock_type) assert.is_nil(db.connector:get_stored_connection("read")) if strategy == "postgres" then @@ -620,7 +615,7 @@ for _, strategy in helpers.each_strategy() do helpers.get_db_utils(strategy, {}) end) - it("returns true when there is a stored connection (cosockets)", function() + it("returns true when there is a stored connection (IS_CLI=false)", function() ngx.IS_CLI = false local db, err = DB.new(helpers.test_conf, strategy) @@ -643,7 +638,7 @@ for _, strategy in helpers.each_strategy() do assert.is_true(db:close()) end) - it("returns true when there is a stored connection (luasocket)", function() + it("returns true when there is a stored connection (IS_CLI=true)", function() ngx.IS_CLI = true local db, err = DB.new(helpers.test_conf, strategy) @@ -657,7 +652,7 @@ for _, strategy in helpers.each_strategy() do assert.is_table(conn) if strategy == "postgres" then - assert.equal("luasocket", db.connector:get_stored_connection().sock_type) + assert.equal("nginx", db.connector:get_stored_connection().sock_type) assert.is_false(db.connector:get_stored_connection().config.ssl) end @@ -666,7 +661,7 @@ for _, strategy in helpers.each_strategy() do assert.is_true(db:close()) end) - postgres_only("returns true when there is a stored connection with ssl (cosockets)", function() + postgres_only("returns true when there is a stored connection with ssl (IS_CLI=false)", function() ngx.IS_CLI = false local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -692,7 +687,7 @@ for _, strategy in helpers.each_strategy() do assert.is_true(db:close()) end) - postgres_only("returns true when there is a stored connection with ssl (luasocket)", function() + postgres_only("returns true when there is a stored connection with ssl (IS_CLI=true)", function() ngx.IS_CLI = true local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -710,7 +705,7 @@ for _, strategy in helpers.each_strategy() do assert.is_table(conn) if strategy == "postgres" then - assert.equal("luasocket", db.connector:get_stored_connection().sock_type) + assert.equal("nginx", db.connector:get_stored_connection().sock_type) assert.is_true(db.connector:get_stored_connection().config.ssl) end @@ -718,7 +713,7 @@ for _, strategy in helpers.each_strategy() do assert.is_true(db:close()) end) - it("returns true when there is no stored connection (cosockets)", function() + it("returns true when there is no stored connection (IS_CLI=false)", function() ngx.IS_CLI = false local db, err = DB.new(helpers.test_conf, strategy) @@ -731,7 +726,7 @@ for _, strategy in helpers.each_strategy() do assert.is_true(db:close()) end) - it("returns true when there is no stored connection (luasocket)", function() + it("returns true when there is no stored connection (IS_CLI=true)", function() ngx.IS_CLI = true local db, err = DB.new(helpers.test_conf, strategy) @@ -744,7 +739,7 @@ for _, strategy in helpers.each_strategy() do assert.equal(true, db:close()) end) - postgres_only("returns true when both read-only and write connection exists (cosockets)", function() + postgres_only("returns true when both read-only and write connection exists (IS_CLI=false)", function() ngx.IS_CLI = false local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -781,7 +776,7 @@ for _, strategy in helpers.each_strategy() do db:close() end) - postgres_only("returns true when both read-only and write connection exists (luasocket)", function() + postgres_only("returns true when both read-only and write connection exists (IS_CLI=true)", function() ngx.IS_CLI = true local conf = utils.cycle_aware_deep_copy(helpers.test_conf) @@ -801,7 +796,7 @@ for _, strategy in helpers.each_strategy() do assert.is_nil(err) assert.is_table(conn) - assert.equal("luasocket", db.connector:get_stored_connection("write").sock_type) + assert.equal("nginx", db.connector:get_stored_connection("write").sock_type) assert.is_nil(db.connector:get_stored_connection("read")) if strategy == "postgres" then diff --git a/spec/02-integration/03-db/04-db_cluster_mutex_spec.lua b/spec/02-integration/03-db/04-db_cluster_mutex_spec.lua index 198b5ea68eb1..156593be65f7 100644 --- a/spec/02-integration/03-db/04-db_cluster_mutex_spec.lua +++ b/spec/02-integration/03-db/04-db_cluster_mutex_spec.lua @@ -22,44 +22,47 @@ for _, strategy in helpers.each_strategy() do describe("db:cluster_mutex()", function() it("returns 'true' when mutex ran and 'false' otherwise", function() + local ok1, err1, ok2, err2 local t1 = ngx.thread.spawn(function() - local ok, err = db:cluster_mutex("my_key", nil, function() + ok1, err1 = db:cluster_mutex("my_key", nil, function() ngx.sleep(0.1) end) - assert.is_nil(err) - assert.equal(true, ok) end) local t2 = ngx.thread.spawn(function() - local ok, err = db:cluster_mutex("my_key", nil, function() end) - assert.is_nil(err) - assert.equal(false, ok) + ok2, err2 = db:cluster_mutex("my_key", nil, function() end) end) ngx.thread.wait(t1) ngx.thread.wait(t2) + + assert.is_nil(err1) + assert.equal(true, ok1) + assert.is_nil(err2) + assert.equal(false, ok2) end) it("mutex ensures only one callback gets called", function() local cb1 = spy.new(function() end) local cb2 = spy.new(function() ngx.sleep(0.3) end) + local err1, err2 local t1 = ngx.thread.spawn(function() ngx.sleep(0.2) - local _, err = db:cluster_mutex("my_key_2", { owner = "1" }, cb1) - assert.is_nil(err) + _, err1 = db:cluster_mutex("my_key_2", { owner = "1" }, cb1) end) local t2 = ngx.thread.spawn(function() - local _, err = db:cluster_mutex("my_key_2", { owner = "2" }, cb2) - assert.is_nil(err) + _, err2 = db:cluster_mutex("my_key_2", { owner = "2" }, cb2) end) ngx.thread.wait(t1) ngx.thread.wait(t2) + assert.is_nil(err1) + assert.is_nil(err2) assert.spy(cb2).was_called() assert.spy(cb1).was_not_called() end) @@ -68,29 +71,33 @@ for _, strategy in helpers.each_strategy() do it("mutex can be subsequently acquired once released", function() local cb1 = spy.new(function() end) local cb2 = spy.new(function() end) + local err1, err2 local t1 = ngx.thread.spawn(function() - local _, err = db:cluster_mutex("my_key_3", nil, cb1) - assert.is_nil(err) + _, err1 = db:cluster_mutex("my_key_3", nil, cb1) end) + -- to make sure `db:cluster_mutex` is called subsequently. + -- even if we didn't call ngx.sleep() explicitly, it will + -- yield in `db:cluster_mutex` + ngx.thread.wait(t1) + local t2 = ngx.thread.spawn(function() - local _, err = db:cluster_mutex("my_key_3", nil, cb2) - assert.is_nil(err) + _, err2 = db:cluster_mutex("my_key_3", nil, cb2) end) - ngx.thread.wait(t1) ngx.thread.wait(t2) + assert.is_nil(err1) + assert.is_nil(err2) assert.spy(cb1).was_called() assert.spy(cb2).was_called() end) it("mutex cannot be held for longer than opts.ttl across nodes (DB lock)", function() + local ok1, err1, ok2, err2 local cb1 = spy.new(function() - -- remove worker lock - ngx.shared.kong_locks:delete("my_key_5") -- make DB lock expire ngx.sleep(1) end) @@ -98,20 +105,23 @@ for _, strategy in helpers.each_strategy() do local cb2 = spy.new(function() end) local t1 = ngx.thread.spawn(function() - local ok, err = db:cluster_mutex("my_key_5", { ttl = 0.5 }, cb1) - assert.is_nil(err) - assert.equal(true, ok) + ok1, err1 = db:cluster_mutex("my_key_5", { ttl = 0.5 }, cb1) end) + -- remove worker lock + ngx.shared.kong_locks:delete("my_key_5") + local t2 = ngx.thread.spawn(function() - local ok, err = db:cluster_mutex("my_key_5", { ttl = 0.5 }, cb2) - assert.is_nil(ok) - assert.matches("%[%w+ error%] timeout", err) + ok2, err2 = db:cluster_mutex("my_key_5", { ttl = 0.5 }, cb2) end) ngx.thread.wait(t1) ngx.thread.wait(t2) + assert.is_nil(err1) + assert.equal(true, ok1) + assert.is_nil(ok2) + assert.matches("%[%w+ error%] timeout", err2) assert.spy(cb1).was_called() assert.spy(cb2).was_not_called() end) diff --git a/spec/03-plugins/28-grpc-gateway/01-proxy_spec.lua b/spec/03-plugins/28-grpc-gateway/01-proxy_spec.lua index 2acf6a1b05bb..ab81ea106b9d 100644 --- a/spec/03-plugins/28-grpc-gateway/01-proxy_spec.lua +++ b/spec/03-plugins/28-grpc-gateway/01-proxy_spec.lua @@ -10,6 +10,12 @@ for _, strategy in helpers.each_strategy() do lazy_setup(function() assert(helpers.start_grpc_target()) + -- start_grpc_target takes long time, the db socket might already + -- be timeout, so we close it to avoid `db:init_connector` failing + -- in `helpers.get_db_utils` + helpers.db:connect() + helpers.db:close() + local bp = helpers.get_db_utils(strategy, { "routes", "services", diff --git a/spec/require.lua b/spec/require.lua new file mode 100644 index 000000000000..ae6d934b869b --- /dev/null +++ b/spec/require.lua @@ -0,0 +1,93 @@ +-- Bundled from https://github.com/pygy/require.lua +-- License MIT +--- usage: +-- require = require"require".require +-- :o) + +local error, ipairs, newproxy, tostring, type + = error, ipairs, newproxy, tostring, type + +local t_concat = table.concat + +--- Helpers + + +local function checkstring(s) + local t = type(s) + if t == "string" then + return s + elseif t == "number" then + return tostring(s) + else + error("bad argument #1 to 'require' (string expected, got "..t..")", 3) + end +end + +--- for Lua 5.1 + +local package, p_loaded, setmetatable = package, package.loaded, setmetatable + +local sentinel do + local function errhandler() error("the require() sentinel can't be indexed or updated", 2) end + sentinel = newproxy and newproxy() or setmetatable({}, {__index = errhandler, __newindex = errhandler, __metatable = false}) +end + +local function require51 (name) + name = checkstring(name) + if p_loaded[name] == sentinel then + error("loop or previous error loading module '"..name.."'", 2) + end + + local module = p_loaded[name] + if module then return module end + + local msg = {} + local loader + for _, searcher in ipairs(package.loaders) do + loader = searcher(name) + if type(loader) == "function" then break end + if type(loader) == "string" then + -- `loader` is actually an error message + msg[#msg + 1] = loader + end + loader = nil + end + if loader == nil then + error("module '" .. name .. "' not found: "..t_concat(msg), 2) + end + p_loaded[name] = sentinel + local res = loader(name) + if res ~= nil then + module = res + elseif p_loaded[name] == sentinel or not p_loaded[name] then + module = true + else + module = p_loaded[name] + end + + p_loaded[name] = module + return module +end + +local module = { + VERSION = "0.1.8", + require51 = require51, +} + +if _VERSION == "Lua 5.1" then module.require = require51 end + +--- rerequire :o) + +for _, o in ipairs{ + {"rerequiredefault", require}, + {"rerequire", module.require}, + {"rerequire51", require51}, +} do + local rereq, req = o[1], o[2] + module[rereq] = function(name) + p_loaded[name] = nil + return req(name) + end +end + +return module