AlinsRan commented on PR #13912:
URL: https://github.com/apache/apisix/pull/13912#issuecomment-5522360592

   Thanks — P1 reproduced exactly as you describe, and it is fixed in 2a2d39f.
   
   ## P1
   
   I built the same isolated reproducer (OpenResty 1.29.2.4) with a backend 
that does **not** parse the PROXY protocol, so the header arrives as raw bytes:
   
   ```
   BACKEND raw first line = [PROXY TCP4 127.0.0.1 unix:/tmp/pp-inner.sock 56058 
0]
   ```
   
   Byte for byte what you predicted. And your diagnosis of why the Lua 
assignment does not help is right — `api_ctx.var.server_addr` only writes the 
ctx cache, while `ngx_proxy_protocol_write` reads `c->local_sockaddr`. 
`set_real_ip_from` replaces `c->sockaddr` only, which is also why the family 
check still says `TCP4` while the destination is a unix path.
   
   I looked at making the header correct instead of refusing it. Switching the 
internal hop to a TCP loopback socket does produce a syntactically valid line:
   
   ```
   PROXY TCP4 127.0.0.1 127.0.0.1 38214 19902
   ```
   
   but the destination is then the internal port rather than the port the 
client actually reached, so a backend that uses it for routing or logging is 
silently given the wrong value — and a loopback port is easier for a local 
process to connect to and forge than a unix socket under `logs/`. Neither 
seemed better than being explicit, so **the combination is now rejected at 
configuration time**:
   
   ```
   invalid stream_proxy.tcp entry: `9103` can not combine 
proxy_protocol_to_upstream
   with a mixed `tls` + `tls_passthrough` listen; use a dedicated listen for 
either mode
   ```
   
   Only mixed listens are affected. A dedicated `tls: true` or 
`tls_passthrough: true` listen has no internal hop, so nginx builds the header 
from the real listen address and it is correct — that path is unchanged.
   
   Regression tests in `t/cli/test_stream_tls_passthrough.sh` cover the flag 
set on the entry and inherited from the global 
`proxy_protocol.enable_tcp_pp_to_upstream` default; both must be refused. The 
restriction is documented in `conf/config.yaml.example` and in the stream-proxy 
guide, en and zh.
   
   ## P2
   
   Confirmed, and the extra hop is inherent to sharing one listener rather than 
an implementation choice. `ssl_preread on` and `listen ... ssl` cannot coexist 
on one server — I measured it: the handshake completes and the preread phase 
then never sees a ClientHello, so the connection stalls. The terminating half 
therefore has to live in a separate `server`, and the only way to hand a 
connection from Lua to another server block is a socket.
   
   So the topology you describe is exactly right: mixed listens make 2 outbound 
connections per session (1 internal + 1 to the business upstream) instead of 1. 
Dedicated `tls` and `tls_passthrough` listens keep the direct path and are 
unaffected — which is why the docs recommend a dedicated `tls_passthrough` port 
whenever a whole port passes through, and treat mixed as the case you opt into 
only when both modes must share one port.
   
   I have not measured latency or throughput either; the cost is a local socket 
plus one more copy per connection, and I would rather state that plainly than 
quote a number I have not taken. If you would like the mixed path gated behind 
something more explicit than "both flags set", or dropped from this PR and 
proposed separately, say so and I will do that.


-- 
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]

Reply via email to