Copilot commented on code in PR #12761:
URL: https://github.com/apache/trafficserver/pull/12761#discussion_r2666458741
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -431,11 +432,14 @@ def MakeATSProcess(
port_str += " {ssl_port}:quic {ssl_portv6}:quic:ipv6".format(
ssl_port=p.Variables.ssl_port,
ssl_portv6=p.Variables.ssl_portv6)
if enable_proxy_protocol:
- port_str += f" {p.Variables.proxy_protocol_port}:pp
{p.Variables.proxy_protocol_portv6}:pp:ipv6"
+ ppcfg = "pp"
+ if enable_proxy_protocol_cp_src:
+ ppcfg += ":pp-clnt"
+ port_str += f" {p.Variables.proxy_protocol_port}:{ppcfg}
{p.Variables.proxy_protocol_portv6}:pp:ipv6"
Review Comment:
Inconsistent configuration for IPv6 proxy protocol port. When
enable_proxy_protocol_cp_src is True, the IPv4 ports use the dynamic ppcfg
variable that includes ":pp-clnt", but the IPv6 port (proxy_protocol_portv6)
still uses the hardcoded "pp:ipv6" configuration. It should use f"{ppcfg}:ipv6"
to ensure consistency between IPv4 and IPv6 proxy protocol ports when the
pp-clnt flag is enabled.
```suggestion
port_str += f" {p.Variables.proxy_protocol_port}:{ppcfg}
{p.Variables.proxy_protocol_portv6}:{ppcfg}:ipv6"
```
##########
include/iocore/net/NetVConnection.h:
##########
@@ -510,7 +524,8 @@ class NetVConnection : public VConnection, public
PluginUserArgs<TS_USER_ARGS_VC
N_SERVICES,
};
- IpEndpoint local_addr;
+ IpEndpoint local_addr;
+ // This can be the remote peer address or the proxy protocol SRC IP address
Review Comment:
The comment "This can be the remote peer address or the proxy protocol SRC
IP address" is misleading. Based on the code logic in
get_effective_remote_addr(), the remote_addr field always contains the actual
remote peer address. The proxy protocol SRC IP is stored in pp_info.src_addr
and is only returned by get_effective_remote_addr() when the pp-clnt flag is
enabled. The comment should be clarified to reflect that remote_addr always
stores the peer address, not the proxy protocol address.
```suggestion
// Always the actual remote peer socket address; proxy protocol SRC IP (if
any)
// is stored separately in pp_info.src_addr and used via
get_effective_remote_addr().
```
##########
src/records/RecHttp.cc:
##########
@@ -182,20 +182,21 @@ const char *const HttpProxyPort::OPT_INBOUND_IP_PREFIX =
"ip-in";
const char *const HttpProxyPort::OPT_HOST_RES_PREFIX = "ip-resolve";
const char *const HttpProxyPort::OPT_PROTO_PREFIX = "proto";
-const char *const HttpProxyPort::OPT_IPV6 = "ipv6";
-const char *const HttpProxyPort::OPT_IPV4 = "ipv4";
-const char *const HttpProxyPort::OPT_TRANSPARENT_INBOUND = "tr-in";
-const char *const HttpProxyPort::OPT_TRANSPARENT_OUTBOUND = "tr-out";
-const char *const HttpProxyPort::OPT_TRANSPARENT_FULL = "tr-full";
-const char *const HttpProxyPort::OPT_TRANSPARENT_PASSTHROUGH = "tr-pass";
-const char *const HttpProxyPort::OPT_ALLOW_PLAIN = "allow-plain";
-const char *const HttpProxyPort::OPT_SSL = "ssl";
-const char *const HttpProxyPort::OPT_PROXY_PROTO = "pp";
-const char *const HttpProxyPort::OPT_PLUGIN = "plugin";
-const char *const HttpProxyPort::OPT_BLIND_TUNNEL = "blind";
-const char *const HttpProxyPort::OPT_COMPRESSED = "compressed";
-const char *const HttpProxyPort::OPT_MPTCP = "mptcp";
-const char *const HttpProxyPort::OPT_QUIC = "quic";
+const char *const HttpProxyPort::OPT_IPV6 = "ipv6";
+const char *const HttpProxyPort::OPT_IPV4 = "ipv4";
+const char *const HttpProxyPort::OPT_TRANSPARENT_INBOUND = "tr-in";
+const char *const HttpProxyPort::OPT_TRANSPARENT_OUTBOUND = "tr-out";
+const char *const HttpProxyPort::OPT_TRANSPARENT_FULL = "tr-full";
+const char *const HttpProxyPort::OPT_TRANSPARENT_PASSTHROUGH = "tr-pass";
+const char *const HttpProxyPort::OPT_ALLOW_PLAIN = "allow-plain";
+const char *const HttpProxyPort::OPT_SSL = "ssl";
+const char *const HttpProxyPort::OPT_PROXY_PROTO = "pp";
+const char *const HttpProxyPort::OPT_PLUGIN = "plugin";
+const char *const HttpProxyPort::OPT_BLIND_TUNNEL = "blind";
+const char *const HttpProxyPort::OPT_COMPRESSED = "compressed";
+const char *const HttpProxyPort::OPT_MPTCP = "mptcp";
+const char *const HttpProxyPort::OPT_QUIC = "quic";
+const char *const HttpProxyPort::OPT_PROXY_PROTO_CLIENT_SRC_IP = "pp-clnt";
Review Comment:
The flag name "pp-clnt" uses an abbreviation "clnt" which is inconsistent
with the codebase naming pattern. Other related configuration options use full
words like "pp", "ssl", "tr-in", "tr-out", etc. Consider using a more
descriptive name like "pp-client-src" or "pp-use-src" to make the purpose
clearer and maintain consistency with naming conventions.
##########
doc/admin-guide/logging/formatting.en.rst:
##########
@@ -499,16 +499,19 @@ incoming/outgoing ports, and network interfaces used
during transactions.
Field Source Description
===== ============== ==========================================================
chi Client IP address of the client's host. If :ref:`Proxy Protocol
<proxy-protocol>`
- is used, this represents the IP address of the peer,
rather than
- the client IP behind the peer.
+ is configured with the pp-clnt flag, this represents the
proxy protocol SRC IP address, otherwise
+ the IP is that of the connected peer.
chih Client IP address of the client's host, in hexadecimal.
chiv Client IP address of the client's host verified by a plugin. If
not available,
``chi`` is used.
+rchi Remote Client This is alway the IP address of the inbound remote peer.
Review Comment:
Spelling error: "alway" should be "always".
```suggestion
rchi Remote Client This is always the IP address of the inbound remote
peer.
```
##########
doc/admin-guide/logging/formatting.en.rst:
##########
@@ -529,7 +532,6 @@ ppd Proxy Protocol Destination IP received via Proxy
Protocol context from the
Dest IP to the |TS|
ppa Proxy Protocol The Authority TLV from Proxy Protocol context from the LB
Authority to the |TS|
-
===== ============== ==========================================================
Review Comment:
An unnecessary blank line has been removed at the end of the table. While
this is a minor change, it's better to preserve existing formatting when it
doesn't affect functionality, to avoid cluttering the diff with unrelated
changes.
```suggestion
```
--
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]
