fix(batch-requests): missing the port number error when APISIX listens on a unix socket#13715
Open
AlinsRan wants to merge 3 commits into
Open
fix(batch-requests): missing the port number error when APISIX listens on a unix socket#13715AlinsRan wants to merge 3 commits into
AlinsRan wants to merge 3 commits into
Conversation
… unix socket
The plugin loops pipelined requests back to APISIX with
`httpc:connect("127.0.0.1", ngx.var.server_port)`. When the inbound request
arrives on a unix domain socket, nginx renders `$server_port` as an empty
string, so the connect fails with "missing the port number" and the endpoint
returns a 500.
Fall back to the first TCP port in `apisix.node_listen` when `$server_port` is
empty. The value read from the local conf is not normalized by the CLI, so both
a plain port number and an array of port numbers or `{ip, port}` tables are
handled. If no TCP port can be resolved, return a 503 explaining that
batch-requests needs a TCP listener to loop back requests.
Fixes apache#11781
… address The loopback connect targets 127.0.0.1, so a node_listen entry bound to a specific external address does not necessarily accept it. Prefer the first entry that is reachable over loopback (no ip, 0.0.0.0 or 127.0.0.1) and only fall back to a specifically-bound entry when there is no other option.
TEST 32 drives the 503 branch by leaving node_listen empty, which is the shape a unix-socket-only deployment has. Also set an explicit timeout on the test client so a stalled socket cannot hold the suite open.
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.
Description
When APISIX listens on a unix domain socket (e.g. added via
nginx_config.http_server_configuration_snippet), calling the batch-requests endpoint over that socket fails with:{"error_msg":"connect to apisix failed: missing the port number"}The plugin loops the pipelined requests back to APISIX itself with
httpc:connect("127.0.0.1", ngx.var.server_port), but nginx renders$server_portas an empty string for a request that arrived over a unix socket. The empty string is neither a number nor aunix:prefixed host, so lua-nginx-module'stcpsock:connect()rejects it withmissing the port number; resty.http passes that through and the plugin turns it into the 500 above.This PR makes the loopback connect fall back to a TCP port from
apisix.node_listenwhen$server_portis empty. The value read from the local conf is not normalized by the CLI, so both config forms are handled: a plain port number, and an array of port numbers or{ip, port}tables. Since the loopback connect targets127.0.0.1, an entry bound to a specific external address is only used as a last resort — the first entry reachable over loopback (noip,0.0.0.0, or127.0.0.1) wins. If no TCP port can be resolved at all, the plugin returns a 503 explaining that batch-requests needs a TCP listener to loop back requests.An alternative would be to connect back over the unix socket itself, but the listening socket path is not exposed through any nginx variable, so that is left as possible future work.
This revives #13530, which was closed by its author while still a draft and never reviewed. Two changes relative to that branch:
node_listenentry selection is now address-aware, as described above; the original always took the first entry and could pick a port bound to an unreachable addresscore.json.encodeinstead of an inlined long-bracket JSON string, which can trip the nginx config parser when the block crosses a config-buffer boundaryAdded a regression test that injects an extra
listen unix:listener into the test server and sends the batch-requests call over it.Test status: the code passes
luajit -blsyntax checking andluacheck. I have not run the test-nginx suite locally; relying on CI for that.Which issue(s) this PR fixes:
Fixes #11781
Checklist