docs: add stream proxy example for limit-conn plugin - #13936
bhuvan-somisetty wants to merge 4 commits into
Conversation
The limit-conn plugin doc had no example for stream proxy usage, even though the plugin has supported it since it was written. Add a "Apply Rate Limiting in Stream Proxy" section with a full, copy-pasteable stream_routes example (Admin API, ADC, and Ingress Controller), and correct a common misconception: when key_type is var (the default), key is resolved as an NGINX stream variable via ctx.var[key], so it is not limited to remote_addr/server_addr. Any stream variable (e.g. server_port) can be used. Add a regression test in t/stream-plugin/limit-conn.t that exercises key_type "var" with a variable other than remote_addr/server_addr, guarding the corrected documentation claim against future regressions in the key resolution logic. Resolves apache#4933
788e2fb to
451911e
Compare
kayx23
left a comment
kayx23
left a comment
There was a problem hiding this comment.
I found a few issues that affect the accuracy and reproducibility of the new stream example.
|
|
||
| :::note | ||
|
|
||
| When `key_type` is `var` (the default), `key` is resolved as an [NGINX stream module variable](https://nginx.org/en/docs/stream/ngx_stream_core_module.html). It is not limited to `remote_addr` or `server_addr`: any variable available in the stream context, such as `server_port`, can be used. |
There was a problem hiding this comment.
The stream plugin has a distinct, smaller schema: conn, burst, default_conn_delay, only_use_default_delay, key, and key_type; conn and burst are integers. The page’s existing Attributes table describes the HTTP plugin and includes HTTP-only fields such as Redis policies, rules, degradation, and custom HTTP rejection fields. Adding a stream example without separating or explicitly scoping those attributes can lead readers to apply unsupported fields to stream routes. Could this PR add a separate stream attribute table or clearly qualify the existing table?
There was a problem hiding this comment.
The stream attributes are now separated, but the only_use_default_delay description still reflects HTTP behavior. The stream subsystem does not calculate request latency, so this setting does not change stream behavior. Also, connections at conn + burst are accepted; use “at or below,” not “below.”
There was a problem hiding this comment.
One factual detail remains: “excess connections are always delayed by default_conn_delay” is not correct. resty.limit.conn calculates the delay as default_conn_delay × floor((current connections - 1) / conn), so it can be a multiple of default_conn_delay. Please keep the first two sentences and replace the final clause with that calculation.
There was a problem hiding this comment.
good catch, fixed the formula
- make the example runnable by documenting the stream_proxy listener prerequisite and providing a real TCP echo upstream via socat - scope the attribute table to the stream plugin's actual schema instead of reusing the HTTP one - replace the outdated Gateway API limitation with a TCPRoute + L4RoutePolicy example (supported since Ingress Controller 2.2.0), and fix the undefined external Service in the CRD example - use conn=1/burst=0 with a deterministic connection test that shows the actual rejection instead of a racy nc fan-out - strengthen the stream test to assert the resolved limit key and rule out a silent fallback to remote_addr
|
Thanks for the detailed review! Fixed all of these - added the missing listener/upstream prerequisites, scoped the attribute table to the stream schema, swapped the outdated Gateway API note for a TCPRoute + L4RoutePolicy example, made the rejection test deterministic with conn=1/burst=0, and tightened the stream test to actually assert the resolved key. PTAL |
kayx23
left a comment
kayx23
left a comment
There was a problem hiding this comment.
Three details remain in the revised documentation.
…ample - note the Docker networking caveat for the local socat upstream - correct the only_use_default_delay description for stream Routes and fix the conn/burst boundary wording - use gateway.networking.k8s.io/v1 for TCPRoute, matching what Ingress Controller 2.2.0's Gateway API 1.6.0 actually serves - make the background connection in the rejection test deterministic with sleep + $!, and rework the closing steps so they don't contradict the kill already in the script - drop the dangling ❶❷ markers that didn't point at anything in the examples
|
Good catches, all fixed - docker networking note added, only_use_default_delay description corrected for stream, TCPRoute bumped to v1, the nc test now uses sleep+$! and doesn't contradict itself, and dropped the dangling markers. PTAL |
The description said excess connections are always delayed by a flat default_conn_delay, but resty.limit.conn scales it by how far over conn the current connection count is.
2 participants
Description
The
limit-connplugin doc has no example showing how to use it in stream (L4/TCP) proxy mode, even though the plugin has supported stream proxy since it was written (apisix/stream/plugins/limit-conn.lua).Root cause: the doc gap, plus a prior attempt (#13052) that tried to close this issue documented an incorrect restriction: only
remote_addr/server_addrcan be used askey. That's not accurate: inapisix/plugins/limit-conn/init.lua, whenkey_typeisvar(the default), the key is resolved viactx.var[conf_key], i.e. plain NGINX stream-variable lookup. Any variable available in the stream context works (e.g.server_port), not justremote_addr/server_addr.Solution:
docs/en/latest/plugins/limit-conn.md, following the doc's existing Admin API / ADC / Ingress Controller tabbed example format, with a full, copy-pasteablestream_routesexample (includingupstream).keyresolution behavior accurately instead of the narrower (incorrect) restriction from the earlier attempt.t/stream-plugin/limit-conn.t(key: "server_port"with defaultkey_type: var) so the corrected documentation claim is backed by an actual test, guarding against future regressions in the key-resolution logic.Testing: ran the modified
t/stream-plugin/limit-conn.t(all 12 test blocks, including the 2 new ones) locally against a real OpenResty + etcd environment; all pass. Also ranmarkdownlint-cliagainst the modified doc file with the repo's.markdownlint.ymlconfig; no findings.Which issue(s) this PR fixes:
Fixes #4933
Checklist