Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Core timeout, TLS, header forwarding, default-port, retry, and health-check semantics are currently incomplete.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds APISIX-managed ws/wss proxying with frame-level plugin hooks, retries, documentation, and integration tests.
Changes:
- Introduces WebSocket proxy phases and frame mutation APIs.
- Adds upstream selection, retry reporting, and nginx routing.
- Adds dependency, fixtures, tests, and documentation.
File summaries
| File | Description |
|---|---|
apisix/init.lua |
Implements WebSocket proxy lifecycle. |
apisix/core/websocket.lua |
Provides frame access and mutation APIs. |
apisix/core.lua |
Exports the WebSocket core module. |
apisix/balancer.lua |
Accepts externally reported failures. |
apisix/schema_def.lua |
Adds ws and wss schemes. |
apisix/cli/ngx_tpl.lua |
Adds the WebSocket nginx location. |
apisix/plugins/example-plugin.lua |
Demonstrates the new phases. |
apisix-master-0.rockspec |
Adds the WebSocket dependency. |
t/APISIX.pm |
Mirrors the nginx test location. |
t/lib/server.lua |
Adds WebSocket test backends. |
t/node/websocket-proxy.t |
Runs the integration suite. |
t/node/websocket-proxy.spec.mts |
Tests proxy behavior and retries. |
t/package.json |
Adds Node WebSocket packages. |
t/pnpm-lock.yaml |
Locks the new packages. |
docs/en/latest/admin-api.md |
Documents the new schemes. |
docs/zh/latest/admin-api.md |
Adds Chinese API documentation. |
docs/en/latest/plugin-develop.md |
Documents frame hooks and APIs. |
docs/en/latest/terminology/plugin.md |
Extends the plugin phase overview. |
Review details
Files not reviewed (1)
- t/pnpm-lock.yaml: Generated file
Suppressed comments (8)
apisix/init.lua:1012
- The pinned proxy constructs both WebSocket endpoints with
resty.websocket.*defaults, whose maximum receive payload is 65,535 bytes. Because these options do not raise that limit, any unfragmented frame larger than 64 KiB closes the enhanced proxy even though the existing transparent WebSocket path accepts it. Update the dependency/proxy API to configure protocol-level receive limits consistently with the intended frame-limit policy.
local ok, proxy, err = pcall(ws_proxy.new, {
aggregate_fragments = true,
recv_timeout = recv_timeout_ms,
apisix/init.lua:1008
- Only connect and receive timeouts are passed into this transport;
up_timeout.sendis never applied. Frame writes consequently inherit whichever connect/read timeout was last set on the socket instead of the configured send timeout. The proxy API needs separate read/send timeout handling so the Upstream timeout contract remains effective.
local connect_timeout_ms = up_timeout and up_timeout.connect and up_timeout.connect * 1000
local recv_timeout_ms = up_timeout and up_timeout.read and up_timeout.read * 1000
apisix/init.lua:1081
- Only
hostandserver_nameare supplied to the new upstream handshake, so clientCookie,Authorization,Origin, andSec-WebSocket-Protocolheaders—and header rewrites performed by plugins—are dropped. The proxy also synthesizes the downstream handshake independently instead of forwarding the upstream-selected subprotocol. This breaks authenticated and subprotocol-based WebSockets; propagate a filtered current request-header set and the negotiated response headers.
ok, connect_err = proxy:connect(endpoint, {
host = server.upstream_host,
server_name = server.domain,
})
apisix/init.lua:1079
- Hard-coding the handshake Host to
server.upstream_hostmakes every WebSocket upstream behave likepass_host: node. The defaultpassmode no longer forwards the client's Host, andpass_host: rewriteignoresupstream_host; retries also need to recompute node mode for the newly selected server.
host = server.upstream_host,
apisix/init.lua:1081
- The pinned proxy defaults WSS verification on and honors TLS policy only through connect options, but this call passes none of the Upstream
tlsfields. Consequentlytls.verify: falseis ignored, customca_certscannot be used, and configured client certificates never reach a WSS upstream. Pass the complete TLS policy through, or reject unsupported WSS TLS configurations rather than silently changing their semantics.
ok, connect_err = proxy:connect(endpoint, {
host = server.upstream_host,
server_name = server.domain,
})
apisix/init.lua:1100
- This maps every non-timeout handshake failure to a TCP failure. An upstream that responds normally with 400/401/403 produces an
unexpected HTTP response codeerror here, so APISIX retries the request against other nodes and penalizes a healthy node astcp_failures. Inspect the proxy client's response status and report an HTTP outcome (and avoid retrying non-retryable client/authentication responses).
if connect_err and str_find(connect_err, "timeout", 1, true) then
prev_failure = {state = "failed", code = 504}
else
prev_failure = {state = "failed", code = 599}
apisix/init.lua:1115
- The last failed connection attempt is never reported as a timeout/TCP failure because
prev_failureis only consumed while picking another server. Withretries: 0no failure is reported at all, and with retries the final node is omitted, so passivetcp_failures/timeoutsthresholds can remain inaccurate. Report the final known failure before returning 502.
if not ok then
return core.response.exit(502)
apisix/schema_def.lua:512
- The schema description remains stale after extending the enum, so generated schema/help output still says L7 supports only the four older schemes.
description = "The scheme of the upstream." ..
" For L7 proxy, it can be one of grpc/grpcs/http/https." ..
" For L4 proxy, it can be one of tcp/tls/udp." ..
- Files reviewed: 17/18 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| end | ||
| end | ||
|
|
||
| local ok, proxy, err = pcall(ws_proxy.new, { |
There was a problem hiding this comment.
Single frames larger than 65535 bytes kill the connection on this path.
ws_proxy.new() builds resty.websocket.client and resty.websocket.server without options (see the TODO at resty/websocket/proxy.lua:110), so both sides use the library default max_payload_len = 65535. protocol.lua checks that per frame in recv_frame(), before aggregate_fragments joins anything, so leaving client_max_frame_size & co. unset does not avoid it. An oversized frame marks the socket fatal and the forwarder tears down the whole connection.
enable_websocket has no such limit: after the 101, nginx relays raw bytes and never parses frames. Verified locally against the same upstream (client -> APISIX -> echo server that reports the received length):
| frame size | scheme: ws |
enable_websocket |
|---|---|---|
| 65535 | echoed | echoed |
| 65536 | connection closed (failed to receive the first 2 bytes: closed) |
echoed |
| 1 MiB | - | echoed |
Most client libraries do not fragment by default, so an app sending a large JSON message or a file chunk breaks after switching to ws/wss. resty.websocket.proxy needs to pass max_payload_len (or max_recv_len/max_send_len) through to both constructors, with a default that does not cap ordinary traffic. A test with a >64K frame in each direction would cover it.
There was a problem hiding this comment.
That makes sense, but it won’t be fixed in this PR because:
-
I still need to modify the library so that this parameter can be passed through to lua-resty-websocket, which requires additional work.
-
Even if we complete the work mentioned above, we won’t be able to modify it solely through configuration options in the upstream. I’m inclined to use the existing kafka-proxy plugin model by adding a websocket-proxy plugin to allow for custom configuration. These configurations are written to
ctxvia the plugin and affect the proxy path.
Therefore, there will be another PR following this one to upgrade dependencies and introduce the new plugin.
There was a problem hiding this comment.
I don't agree that these testing errors should be attributed to Jest itself.
If you look at these issues and PRs, it’s not hard to see that they’re all related to Lago tests. There have also been tests related to mcp-bridge in the past.
What they have in common is that they all require downloading resources, such as container images (docker compose up) and npm dependencies (pnpx) during test execution (rather than beforehand), which relies on network communication. As is well known, GitHub CI’s network is unstable, and operations like pulling container images often result in errors or timeouts.
The test::nginx test shell always assumes that tests can complete within an ideal timeframe, almost never considering that these tests may depend on external resources and could fail because of them.
And to be honest, our implementation of the custom --exec extension isn’t perfect either; it relies on mechanisms like ngx.log to report and output errors, which can sometimes cause it to exceed the maximum string length limit. In particular, when timeouts occur due to external resources, Jest may hang and fail to output logs.
However, I do agree with the view that we should no longer rely on the test::nginx shell to launch OpenResty in order to run JavaScript-based tests.
I will consider creating capabilities for JS testing that are consistent with test::nginx for managing the OpenResty lifecycle, including startup, reload, and restart operations. This way, testing will be entirely confined within the JS runtime. Essentially, this involves migrating similar functionality from a legacy Perl codebase (which is also a scripting language and relies on a runtime) to a JS implementation. At the same time, Jest will be replaced with the faster and more modern vitest.
This will begin shortly after this PR is completed, so I think it’s acceptable to tolerate this test running in this less formal manner for the time being. For reference, it does not involve the failure scenarios mentioned above; dependencies are always pulled in advance, and no additional containers need to be downloaded. Therefore, timeouts rarely occur.
25d9628
Description
APISIX already supports WebSocket via
enable_websocketon a Route/Service, but that path is a plain protocol upgrade: after the101handshake, nginx's ownproxy_passforwards the raw TCP stream, and no plugin phase ever sees an individual frame.This PR adds a second, independent way to proxy WebSocket: a new Upstream
scheme: ws/wss. Instead of delegating toproxy_pass, APISIX opens a real bidirectional WebSocket connection to the upstream itself (viaresty.websocket.proxy) and exposes each frame to plugins through four new phases, so a plugin can observe or rewrite frames in flight - for logging, redaction, protocol translation, etc.enable_websocketandscheme: ws/wssdon't combine; the former is ignored when the latter is set, since the connection never reaches theproxy_passpath it configures.What's added
wsandwss, alongside the existinghttp/https/grpc/grpcs/tcp/udp/tls(apisix/schema_def.lua, new@websocket_passnginx location inapisix/cli/ngx_tpl.lua).apisix/init.lua):ws_handshake- once, before APISIX connects to the upstream (in place ofaccess).ws_client_frame/ws_upstream_frame- once per frame, before it's forwarded onward.ws_close- once, when the connection ends (in place oflog; it runs first, thenhttp_log_phase()still runs afterwards so passive health check reporting,after_balance, and theapi_ctx/plugins/route-record tablepool releases all still happen for a WebSocket route the same way they do for an HTTP one).core.websocket(apisix/core/websocket.lua): the API a plugin uses insidews_client_frame/ws_upstream_frameto read and rewrite the frame currently in flight -core.websocket.client/.upstream(orcore.websocket.get_role(role)) exposeget_frame()(type,payload,last,code) andset_frame_data(payload).example-pluginnow implements all four phases as a runnable reference (appends-client/-upstreamto text frames it sees, in each direction).balancer_by_lua*machinery, it can't callngx.balancer.get_last_failure()to report a failed attempt before picking the next node.apisix.balancer.pick_server()gained an optional thirdprev_failureargument ({state, code}, shaped likeget_last_failure()'s return value) so a caller outsidebalancer_by_lua*can report the outcome it already knows from its own connection attempt instead.websocket_content_phaseuses this to retry up toupstream.retriestimes, mapping a connect error to a timeout (504) or a TCP failure (599) outcome. If every node fails before the handshake to the client completes, the client gets a real502; a failure after the handshake (101already sent) instead surfaces as the connection dropping with close code1006, since there's no HTTP response left to send at that point.t/node/websocket-proxy.t+.spec.mts(Jest/TS, following thet/plugin/lago.t-style--exec: pnpm test ...bootstrap), covering frame rewriting, binary frames, fragmentation/aggregate_fragments, ping/pong, both directions of a clean close and of an abrupt disconnect, retry (refused/timeout/multi-node/least_conn), passive health check marking a node unhealthy, URI/query-string forwarding (including withproxy-rewrite), and concurrent-connection isolation. New fixtures added tot/lib/server.lua, andt/APISIX.pmgained the same@websocket_passlocation the real nginx template has, since the test-nginx harness maintains its own copy rather than renderingngx_tpl.lua.schemedocumented as acceptingws/wss, with a note contrasting it againstenable_websocket(admin-api.md, en+zh); the four new phases documented inplugin-develop.md's "extra phase" section with acore.websocketusage example; a pointer to that section added toterminology/plugin.md's phase lifecycle overview.api7-lua-resty-websocket 0.1.0-0(apisix-master-0.rockspec), replacing stockopenresty/lua-resty-websocketforresty.websocket.client/.server/.protocol, and providingresty.websocket.proxy(ported with additional hardening:on_framecallback errors are caught instead of crashing the worker, a fragment-count-limit bug is fixed, upstream TLS verification now defaults on, and thread cleanup on a failedngx.thread.waitno longer leaks).Known limitations (not addressed in this PR)
resty.websocket.proxyalso acceptsclient_max_frame_size/client_max_fragments/upstream_max_frame_size/upstream_max_fragments, which bound how much a fragmented message gets buffered beforeaggregate_fragmentsjoins it. These are deliberately left unset for now (see the comment inwebsocket_content_phase) - the plan is to expose them through a dedicated, dynamically configurable plugin later, not Upstream schema fields.Demo
Checklist