AlinsRan commented on PR #13600:
URL: https://github.com/apache/apisix/pull/13600#issuecomment-5116089347
Second round, after a review pass. Pushed `fd672b0b2`.
**One of these would have broken CI.**
`t/plugin/limit-conn-shared-counter.t` left `/apisix/routes/2` (uri
`/limit_conn2`, `key-auth`) in etcd. `t/plugin/workflow3.t` runs later in the
same `t/plugin/[l-z]*.t` job and expects that uri to 404 — it got 401 from the
leftover route. The file now deletes its routes and consumers again.
**TEST 5 was not testing what it claimed.** Both consumers were keyed on
`consumer_name`, so `jack` and `bob` produced different key values regardless
of the fix — the block passed with the plugin change reverted. Both are now
keyed on `remote_addr`, so they resolve to the same value on the same route and
only `parent.resource_key` separates them. Verified: reverting the plugin
change now fails TEST 4 *and* TEST 5.
**`allow_degradation`.** Every other failure path in `run_limit_conn()`
falls back to it; the new nil-key branch was the only one that always returned
500. Now consistent with the rest of the function.
**Two invariants pinned.**
- `t/plugin/workflow3.t` TEST 4 asserts both `_vid` suffixes. The two
workflow rules carry identical `limit-conn` confs on one route, so after this
change `_vid` is the *only* thing separating their counters — worth an explicit
assertion.
- `t/stream-plugin/limit-conn.t` TEST 2 asserts
`/apisix/stream_routes/1:...`. Stream routes get `_meta.parent` because
`apisix/router.lua`'s `filter` is passed to both `router_http.init_worker()`
and `router_stream.stream_init_worker()`; that is easy to break without
noticing.
Also dropped the `core.log.info("ver: ", ctx.conf_version)` line at the top
of `_M.increase()` — `ctx.conf_version` no longer takes part in the key, so it
just misleads whoever is debugging.
**One more thing for the description**, beyond the consumer case:
**service-level `limit-conn` changes scope**. The old key carried
`route.modifiedIndex .. "&" .. service.modifiedIndex` (`apisix/init.lua:903`),
i.e. one counter per route; it is now one counter per service shared by all its
routes. That matches `limit-count`/`limit-req` and is the intended semantics,
but it tightens the effective limit for anyone relying on the old behaviour and
is worth calling out in the release notes.
**Out of scope, but found while checking this** — two pre-existing issues
that also affect `limit-req` and `limit-count`, worth separate issues:
1. `conf_version()` hashes the whole conf table, including `_from_consumer`,
which `merge_consumer_route()` (`apisix/plugin.lua:830-838`) sets lazily and
only when the route has no plugin of that name. Whether a worker caches
`conf._version` before or after that flag is set depends on which route it
served first, so the plugin conf version can still differ across workers for
the same consumer.
2. `_M.decrease()` uses `break` rather than `goto CONTINUE` when
`lim:leaving()` fails, so one failing rule skips the release of every remaining
rule.
--
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]
