Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests(*): use the same config to reload kong #12125

Closed
wants to merge 1 commit into from
Closed

tests(*): use the same config to reload kong #12125

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 56 additions & 19 deletions spec/helpers.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3488,6 +3488,29 @@ local function get_grpc_target_port()
return 15010
end

local function kong_conf_args(env)
local prefix = env.prefix or conf.prefix
local nginx_conf = ""


local nginx_conf_flags = { "test" }
if env.nginx_conf then
nginx_conf = " --nginx-conf " .. env.nginx_conf
end

if TEST_COVERAGE_MODE == "true" then
-- render `coverage` blocks in the templates
nginx_conf_flags[#nginx_conf_flags + 1] = 'coverage'
end

local nginx_conf_flags_str = ""
if next(nginx_conf_flags) then
nginx_conf_flags_str = " --nginx-conf-flags " .. table.concat(nginx_conf_flags, ",")
end

return prefix, "--conf " .. TEST_CONF_PATH, nginx_conf, nginx_conf_flags_str
end


--- Start the Kong instance to test against.
-- The fixtures passed to this function can be 3 types:
Expand Down Expand Up @@ -3556,7 +3579,8 @@ local function start_kong(env, tables, preserve_prefix, fixtures)
error("arg #2 must be a list of tables to truncate")
end
env = env or {}
local prefix = env.prefix or conf.prefix

local prefix, conf_args, nginx_conf, flags = kong_conf_args(env)

-- go plugins are enabled
-- compile fixture go plugins if any setting mentions it
Expand All @@ -3577,23 +3601,6 @@ local function start_kong(env, tables, preserve_prefix, fixtures)

truncate_tables(db, tables)

local nginx_conf = ""
local nginx_conf_flags = { "test" }
if env.nginx_conf then
nginx_conf = " --nginx-conf " .. env.nginx_conf
end

if TEST_COVERAGE_MODE == "true" then
-- render `coverage` blocks in the templates
nginx_conf_flags[#nginx_conf_flags + 1] = 'coverage'
end

if next(nginx_conf_flags) then
nginx_conf_flags = " --nginx-conf-flags " .. table.concat(nginx_conf_flags, ",")
else
nginx_conf_flags = ""
end

if dcbp and not env.declarative_config and not env.declarative_config_string then
if not config_yml then
config_yml = prefix .. "/config.yml"
Expand All @@ -3609,7 +3616,7 @@ local function start_kong(env, tables, preserve_prefix, fixtures)
end

assert(render_fixtures(TEST_CONF_PATH .. nginx_conf, env, prefix, fixtures))
return kong_exec("start --conf " .. TEST_CONF_PATH .. nginx_conf .. nginx_conf_flags, env)
return kong_exec("start " .. conf_args .. nginx_conf .. flags, env)
end


Expand Down Expand Up @@ -3774,6 +3781,35 @@ local function reload_kong(strategy, ...)
end


local DEFAULT_WAIT_OPT = {
timeout = 10,
step = 2,
}


--- Reload the Kong instance.
local function reload_kong_ex(env, wait_opts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I ask why we don't fix the existing reload_kong function instead of introducing a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep compatibility. They have different interfaces and I don't want to modify all the existing tests for this change (it makes the PR larger and harder to merge).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it makes the PR larger and harder to merge

I understand, though I would be more in favor of fixing the existing use cases since there are relatively few:

$ git grep -c reload_kong spec/0* spec-ee/0*
spec-ee/02-integration/21-profiling/01-cpu_spec.lua:1
spec/02-integration/02-cmd/03-reload_spec.lua:9
spec/02-integration/04-admin_api/22-debug_spec.lua:1
spec/02-integration/11-dbless/03-config_persistence_spec.lua:1

I would rather us have a slightly bigger PR to review than to have this fragmentation of reload helpers.

As an exercise I tried switching spec/02-integration/02-cmd/03-reload_spec.lua to the new function. Everything was straightforward with the exception of the key-auth plugin invalidation on dbless reload #off test case needing slightly special handling:

diff --git spec/02-integration/02-cmd/03-reload_spec.lua spec/02-integration/02-cmd/03-reload_spec.lua
index e70c84c97d..422dfb2996 100644
--- spec/02-integration/02-cmd/03-reload_spec.lua
+++ spec/02-integration/02-cmd/03-reload_spec.lua
@@ -24,7 +24,7 @@ describe("kong reload #" .. strategy, function()
     local nginx_pid = wait_for_file_contents(helpers.test_conf.nginx_pid, 10)
 
     -- kong_exec uses test conf too, so same prefix
-    assert(helpers.reload_kong(strategy, "reload --prefix " .. helpers.test_conf.prefix))
+    assert(helpers.reload_kong_ex())
 
     local nginx_pid_after = wait_for_file_contents(helpers.test_conf.nginx_pid, 10)
 
@@ -133,7 +133,7 @@ describe("kong reload #" .. strategy, function()
     local pids_1 = json.pids
     client:close()
 
-    assert(helpers.reload_kong(strategy, "reload --prefix " .. helpers.test_conf.prefix))
+    assert(helpers.reload_kong_ex())
 
     client = helpers.admin_client()
     local res = assert(client:get("/"))
@@ -170,7 +170,7 @@ describe("kong reload #" .. strategy, function()
     local node_id_1 = json.node_id
     client:close()
 
-    assert(helpers.reload_kong(strategy, "reload --prefix " .. helpers.test_conf.prefix))
+    assert(helpers.reload_kong_ex())
 
     client = helpers.admin_client()
     local res = assert(client:get("/"))
@@ -246,7 +246,7 @@ describe("kong reload #" .. strategy, function()
             - example.test
       ]], yaml_file)
 
-      assert(helpers.reload_kong(strategy, "reload --prefix " .. helpers.test_conf.prefix, {
+      assert(helpers.reload_kong_ex({
         declarative_config = yaml_file,
       }))
 
@@ -316,7 +316,7 @@ describe("kong reload #" .. strategy, function()
         return true
       end)
 
-      assert(helpers.reload_kong(strategy, "reload --prefix " .. helpers.test_conf.prefix))
+      assert(helpers.reload_kong_ex())
 
       admin_client = assert(helpers.admin_client())
       local res = assert(admin_client:send {
@@ -413,7 +413,7 @@ describe("kong reload #" .. strategy, function()
         return true
       end)
 
-      assert(helpers.reload_kong(strategy, "reload --prefix " .. helpers.test_conf.prefix))
+      assert(helpers.reload_kong_ex())
 
       admin_client = assert(helpers.admin_client())
       local res = assert(admin_client:send {
@@ -504,7 +504,7 @@ describe("kong reload #" .. strategy, function()
             weight: 100
       ]], yaml_file)
 
-      assert(helpers.reload_kong(strategy, "reload --prefix " .. helpers.test_conf.prefix, {
+      assert(helpers.reload_kong_ex({
         declarative_config = yaml_file,
       }))
 
@@ -652,8 +652,11 @@ describe("key-auth plugin invalidation on dbless reload #off", function()
         keyauth_credentials:
         - key: my-new-key
     ]], yaml_file)
-    assert(helpers.reload_kong("off", "reload --prefix " .. helpers.test_conf.prefix, {
+    assert(helpers.reload_kong_ex({
+      database = "off",
       declarative_config = yaml_file,
+      nginx_worker_processes = 1,
+      nginx_conf = "spec/fixtures/custom_nginx.template",
     }))
 
     local res
@@ -740,7 +743,7 @@ describe("Admin GUI config", function ()
 
     client:close()
 
-    assert(helpers.reload_kong("off", "reload --conf " .. helpers.test_conf_path, {
+    assert(helpers.reload_kong_ex({
       database = "off",
       admin_gui_listen = "127.0.0.1:9012",
       admin_gui_url = "http://test2.example.com",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flrgh It will still be a headache when we backport/cherry-pick it. I'm trying to figure out a way to make them compatible.

env = env or {}
local _, conf_args, nginx_conf, flags = kong_conf_args(env)

wait_opts = wait_opts or DEFAULT_WAIT_OPT
local no_wait = wait_opts.no_wait

local workers, ret, err
if not no_wait then
workers = get_kong_workers()
end

ret, err = kong_exec("reload " .. conf_args .. nginx_conf .. flags, env)

if (not no_wait) and ret then
wait_until_no_common_workers(workers, wait_opts.expected_total, wait_opts)
end

return ret, err
end


--- Simulate a Hybrid mode DP and connect to the CP specified in `opts`.
-- @function clustering_client
-- @param opts Options to use, the `host`, `port`, `cert` and `cert_key` fields
Expand Down Expand Up @@ -4049,6 +4085,7 @@ end
cleanup_kong = cleanup_kong,
restart_kong = restart_kong,
reload_kong = reload_kong,
reload_kong_ex = reload_kong_ex,
get_kong_workers = get_kong_workers,
wait_until_no_common_workers = wait_until_no_common_workers,

Expand Down
Loading