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

bug: meta.filter in plugin doesn't work when we use some variable #7852

Closed
starsz opened this issue Sep 2, 2022 · 10 comments · Fixed by #8162
Closed

bug: meta.filter in plugin doesn't work when we use some variable #7852

starsz opened this issue Sep 2, 2022 · 10 comments · Fixed by #8162
Assignees
Labels
bug Something isn't working

Comments

@starsz
Copy link
Contributor

starsz commented Sep 2, 2022

Current Behavior

When I use meta.filter in the plugin like this:

curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '{
  "plugins": {
    "file-logger": {
         "_meta": {
            "filter": [
                ["upstream_status", "~=", 200]
            ]
        },
      "path": "logs/file.log"
    }
  },
  "upstream": {
    "type": "roundrobin",
    "nodes": {
      "httpbin.org": 1
    }
  },
  "uri": "/*"
}'

What I expect is that when the upstream_status is not equal to 200, then we will write a log into the file.
But now, the plugin will be executed even if the upstream_status is 200.

Reason:
Because when we do the plugin filter, we can't get the upstream_status (because we hadn't sent a request to upstream), so the value of ngx.var.upstream_status is nil, and the filter is always matched.

apisix/apisix/plugin.lua

Lines 421 to 458 in 47187fa

function _M.filter(ctx, conf, plugins, route_conf, phase)
local user_plugin_conf = conf.value.plugins
if user_plugin_conf == nil or
core.table.nkeys(user_plugin_conf) == 0 then
trace_plugins_info_for_debug(nil, nil)
-- when 'plugins' is given, always return 'plugins' itself instead
-- of another one
return plugins or core.tablepool.fetch("plugins", 0, 0)
end
local custom_sort = false
local route_plugin_conf = route_conf and route_conf.value.plugins
plugins = plugins or core.tablepool.fetch("plugins", 32, 0)
for _, plugin_obj in ipairs(local_plugins) do
local name = plugin_obj.name
local plugin_conf = user_plugin_conf[name]
if type(plugin_conf) ~= "table" then
goto continue
end
local matched = meta_filter(ctx, name, plugin_conf)
local disable = check_disable(plugin_conf)
if not disable and matched then
if plugin_obj.run_policy == "prefer_route" and route_plugin_conf ~= nil then
local plugin_conf_in_route = route_plugin_conf[name]
local disable_in_route = check_disable(plugin_conf_in_route)
if plugin_conf_in_route and not disable_in_route then
goto continue
end
end
if plugin_conf._meta and plugin_conf._meta.priority then
custom_sort = true
end
core.table.insert(plugins, plugin_obj)
core.table.insert(plugins, plugin_conf)
end

Expected Behavior

The meta.filter can work even if the plugin is run at the log phase.

Error Logs

No response

Steps to Reproduce

  1. Create route
curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '{
  "plugins": {
    "file-logger": {
         "_meta": {
            "filter": [
                ["upstream_status", "~=", 200]
            ]
        },
      "path": "logs/file.log"
    }
  },
  "upstream": {
    "type": "roundrobin",
    "nodes": {
      "httpbin.org": 1
    }
  },
  "uri": "/*"
}'
  1. Send two requests
curl localhost:9080/status/200 -v
curl localhost:9080/status/400 -v
  1. We can see both requests are logged.

image

Environment

  • APISIX version (run apisix version): 2.15.0
  • Operating system (run uname -a):
  • OpenResty / Nginx version (run openresty -V or nginx -V):
  • etcd version, if relevant (run curl http://127.0.0.1:9090/v1/server_info):
  • APISIX Dashboard version, if relevant:
  • Plugin runner version, for issues related to plugin runners:
  • LuaRocks version, for installation issues (run luarocks --version):
@tokers
Copy link
Contributor

tokers commented Sep 2, 2022

The filter evaluation should be postponed just before the plugin runs.

@soulbird
Copy link
Contributor

soulbird commented Sep 2, 2022

Maybe we should run the filter at each phase and cache the results appropriately so the filter can be applied to every plugin.

@tokers
Copy link
Contributor

tokers commented Sep 4, 2022

Maybe we should run the filter at each phase and cache the results appropriately so the filter can be applied to every plugin.

The cache is not necessary but an optional field, should be careful about it.

@tzssangglass
Copy link
Member

Perhaps we could add _meta.post_filter for the header_filter, body_filter, and log phases?

@starsz
Copy link
Contributor Author

starsz commented Sep 7, 2022

Perhaps we could add _meta.post_filter for the header_filter, body_filter, and log phases?

Good idea I think.

@tokers
Copy link
Contributor

tokers commented Sep 7, 2022

Perhaps we could add _meta.post_filter for the header_filter, body_filter, and log phases?

And still reserve the filter? What's the point?

@tzssangglass
Copy link
Member

_meta.filter works before request send to upstream, _meta.post_filter works after get response from upstream.

@tokers
Copy link
Contributor

tokers commented Sep 8, 2022

When will the filter run be decided by the plugin itself, isn't it? Two filter There is no sense if we add a post_filter for a plugin that runs before the forwarding.

@tzssangglass
Copy link
Member

Two filter There is no sense if we add a post_filter for a plugin that runs before the forwarding.

when we add post_filter, we can do

  1. on header_filter, body_filter and log phases, check if plugin config has _meta.post_filter
  2. runs _meta.post_filter and _meta.filter, get the plugins that need to be run
  3. runs plugins' header_filter, body_filter and log functions

@soulbird
Copy link
Contributor

soulbird commented Sep 8, 2022

I think that adding post_filter will increase the difficulty of using APISIX. Users must know at what phase plugin is working, and we do not have any documentation on this aspect. Therefore, before configuring, users also need to read the plugin source code, which I think is a very bad user experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants