refactor(pluginservers): code refactor & testing #12858

gszr · 2024-04-13T13:21:33Z

Context

The overall goal of this commit is to refactor the external plugins
implementation, with the following goals in mind:

Make plugin server code more approachable to unfamiliar engineers and
easier to evolve with confidence
Harden configuration; ensure configuration defects are caught before
Kong is started
Extend testing coverage

This is related to ongoing work on the Go PDK, with similar goals in
mind.

Summary

This commit implements the following overall changes to plugin server
code:

Move configuration related code into conf loader, so that configuration
loading and validation happens at startup time, rather than lazily, when
plugin data is loaded or pluginservers are started. Add tests for
current behavior.
Move process-management code - for starting up plugin servers as well
as querying external plugins info - into the process.lua module.
Introduce a kong.runloop.plugin_servers.rpc module that encapsulates
RPC initialization and protocol-specific implementations. This further
simplifies the main plugin server main module.
Factor exposed API and phase handlers bridging code into a new plugin
module, which encapsulates an external plugin representation, including
the expected fields for any Kong plugin, plus external plugin-specific
bits, such as the RPC instance. Part of this external plugin-specific part
is the instance life cycle management. With this structure, the kong.runloop.plugin_servers
main module contains only general external plugin code, including a list
of loaded external plugins, and associated start/stop functions for
plugin servers.

Testing

This commit also implements the following improvements to tests:

Restructure fixtures to accommodate new external plugin servers --
namely, targeting for now in the existing Python and Javascript
Add new test cases for external plugins:
- External plugin configuration: add test cases for current behavior;
  in particular:
  - Fail if no query_cmd is provided;
  - Warn if no start_cmd is provided - this is by design, as
    external plugins servers can be managed outside of Kong
- Plugin server start / stop - for both Go and Python plugins
- External plugin info querying for both Go and Python plugins
- External plugin execution - for both Go and Python plugins<!--

KAG-2496

Context ------- The overall goal of this commit is to refactor the external plugins implementation, with the following goals in mind: - Make plugin server code more approachable to unfamiliar engineers and easier to evolve with confidence - Harden configuration; ensure configuration defects are caught before Kong is started - Extend testing coverage This is related to ongoing work on the Go PDK, with similar goals in mind. Summary ------- This commit implements the following overall changes to plugin server code: - Move configuration related code into conf loader, so that configuration loading and validation happens at startup time, rather than lazily, when plugin data is loaded or pluginservers are started. Add tests for current behavior. - Move process-management code - for starting up plugin servers as well as querying external plugins info - into the `process.lua` module. - Introduce a `kong.runloop.plugin_servers.rpc` module that encapsulates RPC initialization and protocol-specific implementations. This further simplifies the main plugin server main module. - Factor exposed API and phase handlers bridging code into a new `plugin` module, which encapsulates an external plugin representation, including the expected fields for any Kong plugin, plus external plugin-specific bits, such as the RPC instance. Part of this external plugin-specific part is the instance life cycle management. With this structure, the `kong.runloop.plugin_servers` main module contains only general external plugin code, including a list of loaded external plugins, and associated start/stop functions for plugin servers. Testing ------- This commit also implements the following improvements to tests: - Restructure fixtures to accommodate new external plugin servers -- namely, targeting for now in the existing Python and Javascript - Add new test cases for external plugins: * External plugin configuration: add test cases for current behavior; in particular: - Fail if no `query_cmd` is provided; - Warn if no `start_cmd` is provided - this is by design, as external plugins servers can be managed outside of Kong * Plugin server start / stop - for both Go and Python plugins * External plugin info querying for both Go and Python plugins * External plugin execution - for both Go and Python plugins Internal flow ------------- `.plugin_servers.init:` loads all external plugins, by calling .plugin_servers.process and `.plugin_servers.plugin` `.plugin_servers.process`: queries external plugins info with the command specified in `_query_cmd` proeprties `.plugin_servers.plugin`: with info obtained as described above, `.plugin:new` returns a kong-compatible representation of an external plugin, with phase handlers, PRIORITY, and wrappers to the PDK. Calls `.plugin_servers.rpc` to create an RPC through which Kong communicates with the plugin process `.plugin_servers.rpc`: based on info contained in the plugin (protocol field), creates the correct RPC for the given external plugin `.plugin_servers.rpc.pb_rpc`: protobuf rpc implementation - used by Golang `.plugin_servers.rpc.mp.rpc`: messagepack rpc implementation - used by JS and Python `.plugin_servers.init`: calls `.plugin_servers.process` to start external plugin servers `.plugin_servers.process`: optionally starts all external plugin servers (if a `_start_cmd` is found) uses the resty pipe API to manage the external plugin process

hanshuebner · 2024-04-24T14:20:49Z

kong/runloop/plugin_servers/plugin.lua

+    local saved = get_saved()
+    return saved and saved.req_start_time or req_start_time()
+  end,
+}


It seems like almost all of the handler functions require access to the saved request data. Would it make sense to pass it to the handler functions as request parameter rather than having them all call get_saved()? I'm also not thrilled about saved and get_saved as names. Would req and get_req not be more appropriate?

It's more like "retrieve_request_context" here.

StarlightIbuki · 2024-04-28T14:22:27Z

kong/init.lua

@@ -956,7 +956,7 @@ function Kong.init_worker()
  end

  if is_not_control_plane then
-    plugin_servers.start()
+    assert(plugin_servers.start())


Is it a good idea to fail a worker starting? For some of the deployments it will cause the workers to be restarted repeatly.

StarlightIbuki · 2024-04-28T14:23:08Z

kong/init.lua

@@ -976,7 +976,7 @@ end

 function Kong.exit_worker()
  if process.type() ~= "privileged agent" and not is_control_plane(kong.configuration) then
-    plugin_servers.stop()
+    assert(plugin_servers.stop())


And here it prevents the worker to do other clean-up tasks if it fails.

StarlightIbuki · 2024-04-28T14:24:24Z

kong/runloop/plugin_servers/init.lua

+  stop = stop,
+  load_schema = load_schema,
+  load_plugin = load_plugin,
+}


Suggested change

}

}

EoL.

StarlightIbuki · 2024-04-28T14:26:11Z

kong/runloop/plugin_servers/plugin.lua

+    local saved = get_saved()
+    return saved and saved.req_start_time or req_start_time()
+  end,
+}


It's more like "retrieve_request_context" here.

StarlightIbuki · 2024-04-28T14:26:31Z

kong/runloop/plugin_servers/plugin.lua

+return {
+  new = new,
+  reset_instances_for_plugin = reset_instances_for_plugin,
+}


Suggested change

}

}

StarlightIbuki · 2024-04-28T14:27:24Z

kong/runloop/plugin_servers/rpc/init.lua

+
+return {
+  new = new,
+}


Suggested change

}

}

pull-request-size bot added the size/XXL label Apr 13, 2024

github-actions bot added core/language/go core/proxy core/configuration core/language/python core/language/js chore Not part of the core functionality of kong, but still needed cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Apr 13, 2024

gszr force-pushed the chore/plugin-server-improvements branch 2 times, most recently from 50d7d76 to 38ddd0e Compare April 13, 2024 13:29

hanshuebner self-assigned this Apr 16, 2024

AndyZhang0707 requested review from StarlightIbuki and hanshuebner April 17, 2024 09:50

hanshuebner approved these changes Apr 24, 2024

View reviewed changes

StarlightIbuki reviewed Apr 28, 2024

View reviewed changes

StarlightIbuki added the skip-changelog label Apr 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(pluginservers): code refactor & testing #12858

refactor(pluginservers): code refactor & testing #12858

gszr commented Apr 13, 2024 •

edited

hanshuebner Apr 24, 2024

StarlightIbuki Apr 28, 2024

StarlightIbuki Apr 28, 2024

StarlightIbuki Apr 28, 2024

StarlightIbuki Apr 28, 2024

StarlightIbuki Apr 28, 2024

StarlightIbuki Apr 28, 2024

StarlightIbuki Apr 28, 2024

refactor(pluginservers): code refactor & testing #12858

Are you sure you want to change the base?

refactor(pluginservers): code refactor & testing #12858

Conversation

gszr commented Apr 13, 2024 • edited

Context

Summary

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gszr commented Apr 13, 2024 •

edited