Re: [I] fix(limit-count): incorrect error() call in gen_limit_key passes string as level argument [apisix]
AlinsRan closed issue #13357: fix(limit-count): incorrect error() call in gen_limit_key passes string as level argument URL: https://github.com/apache/apisix/issues/13357 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [I] fix(limit-count): incorrect error() call in gen_limit_key passes string as level argument [apisix]
hanzhenfang commented on issue #13357:
URL: https://github.com/apache/apisix/issues/13357#issuecomment-4645747083
Hi @Baoyuantop , I was able to reproduce this locally as well. Sharing the
minimal setup and a bit of context in case it helps with reviewing or
validating the fix.
The repro intentionally exercises the defensive branch in
`limit-count/init.lua` by calling `limit_count.rate_limit()` with a manually
constructed config whose `_meta.parent` is missing. This is not meant to show
that a normal Admin API / route configuration naturally loses `_meta.parent`;
it is just a minimal way to trigger the exact `error()` call reported in this
issue.
Minimal standalone repro:
`docker-compose.yaml`
```yaml
services:
apisix:
image: apache/apisix:3.16.0-debian
container_name: apisix-issue-13357
depends_on:
- upstream
volumes:
- ./config.yaml:/usr/local/apisix/conf/config.yaml:ro
- ./apisix.yaml:/usr/local/apisix/conf/apisix.yaml:ro
ports:
- target: 9080
host_ip: 127.0.0.1
protocol: tcp
upstream:
image: nginx:alpine
container_name: apisix-issue-13357-upstream
```
`config.yaml`
```yaml
deployment:
role: data_plane
role_data_plane:
config_provider: yaml
apisix:
node_listen: 9080
plugins:
- serverless-pre-function
- limit-count
```
`apisix.yaml`
```yaml
routes:
- id: issue-13357
uri: /issue-13357
plugins:
serverless-pre-function:
phase: access
functions:
- |
return function(conf, ctx)
local limit_count =
require("apisix.plugins.limit-count.init")
local broken_limit_count_conf = {
policy = "local",
count = 5,
time_window = 60,
key = "remote_addr",
key_type = "var",
rejected_code = 503,
allow_degradation = false,
show_limit_quota_header = true,
_meta = {}
}
return limit_count.rate_limit(broken_limit_count_conf, ctx,
"limit-count", 1)
end
upstream:
type: roundrobin
nodes:
"upstream:80": 1
#END
```
Then run:
```bash
cd issue-13357-limit-count-error-repro
docker compose up -d
APISIX_URL="http://$(docker compose port apisix 9080)"
curl -i "$APISIX_URL/issue-13357"
docker compose logs --tail=120 apisix
```
On the current code, the request returns `500`, and the APISIX log shows:
```text
/usr/local/apisix/apisix/plugins/limit-count/init.lua:262: bad argument #2
to 'error' (number expected, got string)
stack traceback:
[C]: in function 'error'
/usr/local/apisix/apisix/plugins/limit-count/init.lua:262: in function
'gen_limit_key'
/usr/local/apisix/apisix/plugins/limit-count/init.lua:398: in function
'run_rate_limit'
```
With the proposed change, I would expect the request to still fail, but with
the intended message instead:
```text
failed to generate key invalid parent: null
```
One extra context point: in the normal route/service/plugin_config loading
path, APISIX should inject `conf._meta.parent.resource_key` via
`set_plugins_meta_parent()`. So I think this issue is specifically about making
the defensive error path report the intended message, while any real
execution path that loses `_meta.parent.resource_key` would probably be a
separate issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [I] fix(limit-count): incorrect error() call in gen_limit_key passes string as level argument [apisix]
suryaparua-official commented on issue #13357: URL: https://github.com/apache/apisix/issues/13357#issuecomment-4550540032 Hi @Baoyuantop, thank you for the review! I've already added a regression test in t/plugin/limit-count5.t (TEST 9) that covers the missing _meta.parent path. Regarding similar patterns in the codebase — @hachi029 pointed out a similar issue in apisix/discovery/dns/init.lua:81. I will scan the codebase for all error(message, string) patterns and fix them in this PR. I'll update the PR shortly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [I] fix(limit-count): incorrect error() call in gen_limit_key passes string as level argument [apisix]
Baoyuantop commented on issue #13357: URL: https://github.com/apache/apisix/issues/13357#issuecomment-4549893373 The second argument of Lua error() should be a numeric stack level, but the current code can pass an encoded string there, which causes a secondary bad argument #2 error and hides the original problem. The fix should format the full error message as a single string instead of passing the JSON string as the second argument. It would be good to add a regression test for the missing parent/resource-key path and quickly scan for similar error(message, string) patterns. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[I] fix(limit-count): incorrect error() call in gen_limit_key passes string as level argument [apisix]
suryaparua-official opened a new issue, #13357:
URL: https://github.com/apache/apisix/issues/13357
### Current Behavior
In `apisix/plugins/limit-count/init.lua` at line 262, the `error()`
function is called with incorrect arguments:
error("failed to generate key invalid parent: ", core.json.encode(parent))
Lua's error(message, level) expects the second argument to be an integer
(stack level: 0, 1, or 2), not a string.
Passing core.json.encode(parent) as the level causes LuaJIT to raise:
bad argument #2 to 'error' (number expected, got string)
This crashes the request with an unhelpful LuaJIT internal error instead
of the intended error message about the invalid parent configuration.
### Expected Behavior
When conf._meta.parent is nil or missing resource_key, APISIX should log
a clear error message containing the JSON of the invalid parent object,
helping developers debug the issue quickly.
### Error Logs
bad argument #2 to 'error' (number expected, got string)
### Steps to Reproduce
1. Open apisix/plugins/limit-count/init.lua
2. Go to line 262
3. Observe the error() call:
error("failed to generate key invalid parent: ", core.json.encode(parent))
4. The second argument core.json.encode(parent) is a string, but Lua's
error() expects an integer as the second argument (stack level).
5. This path is triggered when conf._meta.parent is nil or
parent.resource_key is missing during limit-count plugin execution.
### Environment
APISIX version: master (3.16.0)
OS: Linux
OpenResty version: 1.29.2.3
File: apisix/plugins/limit-count/init.lua
Line: 262
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
