change: Improve results from admin API while GET all plugins by Revolyssup · Pull Request #9580 · apache/apisix

Revolyssup · 2023-05-31T09:19:24Z

Description

When subsystem is wrong, return a 400 error.
When subsystem is unspecified and all is true, return all plugins and not just http plugins

Fixes #9577

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Signed-off-by: revolyssup <[email protected]>

Revolyssup · 2023-06-04T17:17:08Z

@monkeyDluffy6017 Please approve CI

Revolyssup · 2023-06-05T18:29:23Z

@monkeyDluffy6017 the failing tests are unrelated to the PR. Can you rerun CI? These tests are flaky

juststillthinking · 2023-06-06T13:18:02Z

@kayx23 Should we change the default behaviour of this interface? or just add a parameter subsystem=all to get all plugins?

Signed-off-by: revolyssup <[email protected]>

kayx23 · 2023-06-07T01:34:58Z

Should we change the default behaviour

Breaking changes aside:

If by default behaviour you mean to return all plugins when subsystem is not specified after all=true, and use subsystem for filtering purpose, personally I think this makes the most sense.
Note that all=true seems to be a fixed pre-req for using subsystem. If I set it to false, it errors out:

curl -s "http://127.0.0.1:9180/apisix/admin/plugins?all=false"
{"error_msg":"not found plugin name"}

But does this make sense? Why is all=true needed at all?

Signed-off-by: revolyssup <[email protected]>

Revolyssup · 2023-06-08T06:52:09Z

Current state of this PR reflects this proposal: #9577 (comment)

Signed-off-by: revolyssup <[email protected]>

moonming · 2023-06-27T05:07:31Z

@monkeyDluffy6017 ping

Signed-off-by: revolyssup <[email protected]>

juststillthinking · 2023-07-14T10:08:56Z

+        local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
        core.log.warn("failed to load plugin [", name, "] err: ", plugin)
-        return 400, {error_msg = "failed to load plugin " .. name}
+        return 400, {error_msg = err }


There is no need to return an error here? If the plugin doesn't exist, it is fine to return nil?

juststillthinking · 2023-07-14T10:13:12Z

-function _M.get_plugins_list()
-    local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+    local http_plugins


Could you check the subsystem here?

You mean before defining stream_plugins variable?

@monkeyDluffy6017 The check is already performed here https://github.com/apache/apisix/pull/9580/files#diff-8baac20135bcefd8d1beffe08f44b5271bc1c717c6ab2e720a0e14cb1a02599dR262 before calling this function

juststillthinking · 2023-07-14T10:15:33Z

+            local plugin_name = "apisix.plugins." .. name
+            local ok, plugin = pcall(require, plugin_name)
+            if ok and plugin.priority then
+                priorities[name] = plugin.priority


can these two function be used here?

local plugin_get_http = require("apisix.plugin").get local plugin_get_stream = require("apisix.plugin").get_stream

Those functions are to get the entire plugin loaded with plugin.load(). Here we just need the names.

juststillthinking · 2023-07-14T10:15:42Z

+            local plugin_name = "apisix.stream.plugins." .. name
+            local ok, plugin = pcall(require, plugin_name)
+            if ok and plugin.priority then
+                priorities[name] = plugin.priority
+                table_insert(success, name)


Those functions are to get the entire plugin loaded with plugin.load(). Here we just need the names.

juststillthinking · 2023-07-14T10:17:22Z

+| --------- | ----------------------------- | ------- |
+| subsystem | The subsystem of the Plugins. | http    |
+
+The plugin can be filtered on subsystem so that the ({plugin_name}) is searched in the subsystem passed through query params. Note that this query parameter is not available on `/apisix/admin/plugins/list`


I think it's supported on /apisix/admin/plugins/list?

It wasn't present present previously and was later added in the pull request. I removed the later comment.

juststillthinking · 2023-07-14T10:23:34Z

+| --------- | -------------------------------------- | -------- |
+| subsystem | 插件子系统。 | http |
+
+可以在子系统上过滤插件，以便在通过查询参数传递的子系统中搜索 ({plugin_name})。请注意，此查询参数在“/apisix/admin/plugins/list”上不可用


I think it's supported on /apisix/admin/plugins/list?

It wasn't present present previously and was later added in the pull request. I removed the later comment.

juststillthinking · 2023-07-14T10:25:47Z


 你可以使用 `/apisix/admin/plugins?all=true` 接口获取所有插件的所有属性，每个插件包括 `name`，`priority`，`type`，`schema`，`consumer_schema` 和 `version`。

-默认情况下，该接口只返回 L7 插件。如果你需要获取 L4 / Stream 插件，需要使用 `/apisix/admin/plugins?all=true&subsystem=stream`。
+您可以使用“/apisix/admin/plugins?all=true”API 获取所有插件的所有属性。查询参数 `?all=true` 将很快被弃用。


Delete the whole tip, use the following one intead:

您可以使用 “/apisix/admin/plugins?all=true” 获取所有插件的所有属性。这个 API 将很快被弃用

Co-authored-by: Liu Wei <[email protected]>

Signed-off-by: revolyssup <[email protected]>

juststillthinking · 2023-07-17T01:03:13Z

Please fix the doc lint

Signed-off-by: revolyssup <[email protected]>

juststillthinking · 2023-07-18T02:28:55Z

+    local subsystem = arg["subsystem"]
+    if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+        return 400, {error_msg = "unsupported subsystem: "..subsystem}
+    end


Suggested change

local subsystem = arg["subsystem"]

if subsystem and subsystem ~= "http" and subsystem ~= "stream" then

return 400, {error_msg = "unsupported subsystem: "..subsystem}

end

local subsystem = arg["subsystem"] or "http"

if subsystem ~= "http" and subsystem ~= "stream" then

return 400, {error_msg = "unsupported subsystem: "..subsystem}

end

juststillthinking · 2023-07-18T02:29:37Z

-    end
+    local plugin
+    -- By default search through http subsystems
+    subsystem = subsystem or "http"


delete

subsystem = subsystem or "http"

juststillthinking · 2023-07-18T02:30:10Z

+        local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
+        core.log.warn("failed to load plugin [", name, "] err: ", err)


The msg should be changed

juststillthinking · 2023-07-18T02:35:30Z

+        stream_plugins = core.config.local_conf().stream_plugins
+    end
+
+


Redundant blank

juststillthinking · 2023-07-18T03:34:57Z

+    if not plugin then
+        local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
+        core.log.warn("failed to load plugin [", name, "] err: ", err)
+        return 404, {error_msg = "plugin not found"}


"plugin " .. name .. " is not found in subsystem " .. subsystem

Signed-off-by: revolyssup <[email protected]>

juststillthinking · 2023-07-18T06:13:32Z

 	@$(call func_echo_status, "$@ -> [ Start ]")
 	$(ENV_GIT) submodule update --init --recursive
-	prove -I../test-nginx/lib -I./ -r -s t/
+	prove -I../test-nginx/lib -I./ -r -s t/admin/plugins.t


Why do you add this?

Signed-off-by: revolyssup <[email protected]>

Revolyssup added 2 commits May 31, 2023 14:21

fix: Improve results from admin API while GET all plugins

cdb9c42

Signed-off-by: revolyssup <[email protected]>

fix error

75db0b1

Signed-off-by: revolyssup <[email protected]>

Revolyssup changed the title ~~[draft]fix: Improve results from admin API while GET all plugins~~ fix: Improve results from admin API while GET all plugins May 31, 2023

Revolyssup added 3 commits June 1, 2023 10:06

add test

b74771d

Signed-off-by: revolyssup <[email protected]>

fix lint

2679448

Signed-off-by: revolyssup <[email protected]>

fix lint

fea340e

Signed-off-by: revolyssup <[email protected]>

juststillthinking reviewed Jun 2, 2023

View reviewed changes

Comment thread apisix/admin/plugins.lua

add newline

186c792

Signed-off-by: revolyssup <[email protected]>