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.

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

Conversation

StarlightIbuki
Copy link
Contributor

Summary

reload_kong does not pass the same config as when it starts. This issue was found during the attempt to reproduce KAG-3161.

Full changelog

  • refactor to extract parameter generation logic
  • introduce reload_kong_ex to correctly reload kong

Issue reference

Partial fix of KAG-3161



--- 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.

@StarlightIbuki
Copy link
Contributor Author

@flrgh @gszr After reviewing the helpers.lua I found that kong_exec supports passing env as a parameter, so the issue can be considered a user-side issue.
Also, I noticed that CE and EE have different interfaces for reload_kong. I feel they needs to be unified but not sure which one we should take.

@StarlightIbuki StarlightIbuki deleted the tests/reload branch January 21, 2025 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants