fix(limit-conn): use parent resource key for consumer isolation - #13600
Open
DanielWu-star wants to merge 4 commits into
Open
fix(limit-conn): use parent resource key for consumer isolation#13600DanielWu-star wants to merge 4 commits into
DanielWu-star wants to merge 4 commits into
Conversation
Previously, when limit-conn was configured at the consumer level, connection limits were applied per-route instead of globally per consumer. This occurred because the key generation concatenated ctx.conf_type and ctx.conf_version, which included route-specific information. This commit fixes the issue by using conf._meta.parent.resource_key for key generation, consistent with the same fix already applied to limit-req (apache#13019) and limit-count plugins. Now consumer-level connection limits are properly shared across all routes accessed by the same consumer. Fixes apache#13584 Changes: - Added gen_limit_key() function using parent.resource_key approach - Replaced key = key .. ctx.conf_type .. ctx.conf_version with gen_limit_key(conf, ctx, key) in run_limit_conn() - Updated test cases to match new key format - Added test cases (TEST 35-42) verifying consumer isolation across multiple routes
dosubot
Bot
added
size:L
The expected keys were written as `routes/1`, but `resource_key` is the full etcd key, so the assertions in limit-conn.t never matched; limit-conn-redis.t still carried the pre-fix format. `error_log_like` is not a test-nginx section either, so those two blocks were never evaluated. The consumer-level blocks are moved to t/plugin/limit-conn-shared-counter.t and rewritten to fire the requests in parallel: limit-conn counts concurrent connections and releases the counter in the log phase, so the pipelined requests copied from the limit-req test could never hit the limit.
Every other failure path in run_limit_conn() falls back to allow_degradation; the new nil-key branch was the only one that always returned 500. Also drop the now misleading `ver:` log line, since ctx.conf_version no longer takes part in the key. Tests: - limit-conn-shared-counter.t keys both consumers on remote_addr, so they resolve to the same value and only the parent resource_key keeps them apart. TEST 5 previously passed with or without the fix; now it fails without it. - The file deletes its routes and consumers again. /apisix/routes/2 (uri /limit_conn2) leaked into t/plugin/workflow3.t, which expects that uri to 404 and got 401 from the leftover key-auth route. - workflow3.t asserts both _vid suffixes: the two workflow rules carry identical limit-conn confs on one route, so _vid is all that separates them. - stream-plugin/limit-conn.t asserts the stream_route key shape, pinning the fact that stream routes go through the same router.lua filter.
There was a problem hiding this comment.
Pull request overview
This PR fixes limit-conn consumer-level counter isolation so that the same consumer shares a single connection limit across all routes (and across APISIX instances), by generating limiter keys from the parent resource key plus a plugin-level config version—matching the approach already used by limit-req and limit-count.
Changes:
- Add
gen_limit_key()inlimit-connto build keys usingconf._meta.parent.resource_key+apisix.plugin.conf_version(conf)(and_vidfor workflow actions). - Update existing test expectations to the new key format for HTTP, stream, and workflow cases.
- Add a new test file validating shared counters across routes for a consumer, while ensuring different consumers remain isolated.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apisix/plugins/limit-conn/init.lua | Switch limiter key generation to parent resource_key + plugin-level conf version (plus workflow _vid). |
| t/stream-plugin/limit-conn.t | Update stream test to assert the new key format. |
| t/plugin/workflow3.t | Update workflow test to assert the new key format including _vid. |
| t/plugin/limit-conn.t | Update existing HTTP tests to match the new key prefix/version format. |
| t/plugin/limit-conn-redis.t | Update Redis-policy tests to match the new key prefix/version format. |
| t/plugin/limit-conn-shared-counter.t | New tests validating shared per-consumer counters across routes and isolation between consumers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| __DATA__ | ||
|
|
||
| === TEST 1: consumer jack with limit-conn (conn = 1, key shared with bob) |
Comment on lines
+233
to
+235
| --- error_log eval | ||
| qr/limit key: \/apisix\/consumers\/bob:\d+:127\.0\.0\.1/ | ||
| --- timeout: 10 |
shreemaan-abhishek
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, when limit-conn was configured at the consumer level, connection limits were applied per-route instead of globally per consumer. This occurred because the key generation concatenated ctx.conf_type and ctx.conf_version, which included route-specific information.
This commit fixes the issue by using conf._meta.parent.resource_key for key generation, consistent with the same fix already applied to limit-req (#13019) and limit-count plugins. Now consumer-level connection limits are properly shared across all routes accessed by the same consumer.
Changes:
Description
Which issue(s) this PR fixes:
Fixes #13584
Checklist