Copilot commented on code in PR #13939:
URL: https://github.com/apache/apisix/pull/13939#discussion_r4004301501
##########
apisix/init.lua:
##########
@@ -988,6 +998,137 @@ function _M.grpc_access_phase()
end
+-- call ws_x_frame hook
+function _M.websocket_content_phase()
+ ngx.ctx = fetch_ctx()
+ local api_ctx = ngx.ctx.api_ctx
+ local up_conf = api_ctx.upstream_conf
+ local up_timeout = up_conf.timeout
+ 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
+
+ local ok, proxy, err = pcall(ws_proxy.new, {
+ aggregate_fragments = true,
+ recv_timeout = recv_timeout_ms,
+ on_frame = function(proxy, role, typ, payload, last, code)
+ -- proxy: [table] the proxy instance
+ -- role: [string] "client" or "upstream"
+ -- typ: [string] "text", "binary", "ping", "pong", "close"
+ -- payload: [string|nil] payload if any
+ -- last: [boolean] fin flag; true when aggregate_fragments
is on
+ -- code: [number|nil] code for "close" frames
+
+ local role_handler, err = core.websocket.get_role(role)
+ if not role_handler then
+ ngx.log(ngx.ERR, "invalid websocket role: ", err)
+ return
+ end
+
+ role_handler.stash_frame({
+ proxy = proxy,
+ type = typ,
+ payload = payload,
+ last = last,
+ code = code,
+ })
+
+ if role == "client" then
+ common_phase("ws_client_frame")
+ else
+ common_phase("ws_upstream_frame")
+ end
+
+ local new_frame = role_handler.get_frame()
+ return new_frame.payload, new_frame.code
+ end
+ })
+ if not ok or not proxy then
+ ngx.log(ngx.ERR, "failed to create proxy: ", err)
+ return core.response.exit(500)
+ end
+
+ -- proxy:connect() only sends the 101 response to the downstream client
+ -- after it has successfully connected upstream, so it's safe to retry
+ -- against another node here without having committed to the client yet.
+ local retries = up_conf.retries
+ if not retries or retries < 0 then
+ retries = #up_conf.nodes - 1
+ end
+
+ -- upstream_uri is only ever set by plugins like proxy-rewrite that
+ -- explicitly rewrite the forwarded path; the normal proxy_pass paths get
+ -- the client's original request URI for free from nginx's own passthrough
+ -- behavior, but we build the request line ourselves here, so we have to
+ -- fall back to the client's URI (plus query string) the same way
+ -- proxy-mirror.lua does.
+ local request_uri = api_ctx.var.upstream_uri
+ if not request_uri or request_uri == "" then
+ request_uri = api_ctx.var.uri .. (api_ctx.var.is_args or "") ..
(api_ctx.var.args or "")
+ end
+
+ local server = api_ctx.picked_server
+ local ok, connect_err
+ for attempt = 0, retries do
+ if connect_timeout_ms then
Review Comment:
The WebSocket retry loop never enforces `upstream.retry_timeout`. Unlike the
nginx balancer path, this path does not call `set_balancer_opts`, so no retry
deadline is even initialized; multiple connect timeouts can therefore run for
`retries × timeout.connect` despite a smaller configured retry timeout.
Initialize and check a deadline around this loop.
This issue also appears in the following locations of the same file:
- line 1078
- line 1078
- line 1079
- line 1097
- line 1114
##########
docs/en/latest/terminology/plugin.md:
##########
@@ -87,6 +87,8 @@ An installed plugin is first initialized. The configuration
of the plugin is the
When a request goes through APISIX, the plugin's corresponding methods are
executed in one or more of the following phases : `rewrite`, `access`,
`before_proxy`, `header_filter`, `body_filter`, and `log`. These phases are
largely influenced by the [OpenResty
directives](https://openresty-reference.readthedocs.io/en/latest/Directives/).
+A route whose `upstream.scheme` is `ws` or `wss` triggers a different set of
phases instead, one per WebSocket frame: `ws_handshake`, `ws_client_frame`,
`ws_upstream_frame`, and `ws_close`. See the ["extra phase" section of the
plugin development guide](../plugin-develop.md#extra-phase) for details.
Review Comment:
These phases are not a replacement set: the implementation still executes
normal `rewrite`, `access`, `before_proxy`, and `log` phases around them.
Clarify that only `header_filter`/`body_filter` are skipped for this proxy path.
##########
apisix/schema_def.lua:
##########
@@ -506,7 +506,7 @@ local upstream_schema = {
scheme = {
default = "http",
enum = {"grpc", "grpcs", "http", "https", "tcp", "tls", "udp",
- "kafka"},
+ "kafka", "ws", "wss"},
Review Comment:
Adding `ws`/`wss` here without adding them to `scheme_to_port` leaves a node
with no explicit port as `nil` (`apisix/upstream.lua:188-193,263`). Such nodes
are valid in the Upstream schema, but this path later formats the port with
`%d`, so `ws`/`wss` routes only work when every node explicitly specifies a
port. Map `ws` to 80 and `wss` to 443.
This issue also appears on line 510 of the same file.
##########
apisix/init.lua:
##########
@@ -988,6 +998,137 @@ function _M.grpc_access_phase()
end
+-- call ws_x_frame hook
+function _M.websocket_content_phase()
+ ngx.ctx = fetch_ctx()
+ local api_ctx = ngx.ctx.api_ctx
+ local up_conf = api_ctx.upstream_conf
+ local up_timeout = up_conf.timeout
Review Comment:
This bypasses the documented Route timeout override and always reads the
Upstream timeout. A Route with `timeout` therefore behaves differently solely
because its Upstream scheme is `ws`/`wss`.
This issue also appears in the following locations of the same file:
- line 1007
- line 1010
##########
t/node/websocket-proxy.spec.mts:
##########
@@ -0,0 +1,448 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { describe, expect, it, jest } from '@jest/globals';
+import axios from 'axios';
+import WS from 'ws';
+
+import { request as requestAdminAPI } from '../ts/admin_api';
+import { wait } from '../ts/utils';
+
+// Every test here does at least one real websocket handshake plus etcd sync
+// round trip, which the shared 5s Jest default leaves little room for on a
+// loaded machine; the handful of tests that need more than this still set
+// their own per-test timeout on top of it.
+jest.setTimeout(15000);
+
+const PROXY_BASE = 'ws://localhost:1984';
+// a loopback address nothing listens on, used as an unreachable upstream node
+const DEAD_NODE = '127.0.0.1:1';
+const DEAD_NODE_2 = '127.0.0.1:2';
+// TEST-NET-1 (RFC 5737): guaranteed unroutable, so connections to it hang
+// until a connect timeout fires instead of being refused immediately -
+// unlike DEAD_NODE, this exercises the "timeout" (504) branch, not "tcp
+// failure".
+const BLACKHOLE_NODE = '192.0.2.1:1';
Review Comment:
A TEST-NET address is non-forwardable, but it is not guaranteed to hang: a
CI network can fail it immediately with `ENETUNREACH` or an ICMP rejection.
Since the test only checks the eventual echo response, it still passes without
exercising the 504 timeout branch it claims to cover. Use a deterministic
fixture or assert evidence that the first attempt actually timed out.
##########
docs/en/latest/plugin-develop.md:
##########
@@ -217,6 +217,33 @@ function _M.delayed_body_filter(conf, ctx)
end
```
+When a route's `upstream.scheme` is `ws` or `wss`, APISIX proxies WebSocket
frames itself instead of letting nginx's `proxy_pass` transparently forward
them, so it can also run a plugin's logic against each frame. This gives access
to four more phases that only fire for such a route, in place of the usual
`header_filter`/`body_filter`/`log`:
+
+* `ws_handshake` - runs once, at the same point `access` would run for a plain
HTTP route, before APISIX attempts to connect to the upstream.
+* `ws_client_frame` - runs once per frame received from the downstream client,
before it is forwarded to the upstream.
+* `ws_upstream_frame` - runs once per frame received from the upstream, before
it is forwarded to the downstream client.
+* `ws_close` - runs once, when the connection ends, in place of `log`.
Review Comment:
This phase description does not match the implementation: normal `access`
and `before_proxy` run before `ws_handshake`, and normal `log` still runs after
`ws_close`; only the header/body filter phases are absent. Documenting the new
phases as replacements will cause plugins to omit required behavior or execute
it twice.
--
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]
