The problem is that r12742.1.41 is bogus and useless at the same time due to the following reasons:1. To bind some non-local IP address one needs COMM_TRANSPARENT flag which will trigger IP_TRANSPARENT socket option. The flag was not set, therefore attempts to bind any non-local address are bound to fail.2. For COMM_TRANSPARENT we need CAP_NET_ADMIN, but this capability is set only when 'ftp_port' option is set to 'tproxy'Taking this into account it may be concluded that the patch doesn't break this [tproxy] logic in any way.14.02.2017, 23:23, "Alex Rousskov" <[email protected]>:,On 02/14/2017 12:25 PM, Alex wrote:
It seems that the patch doesn't make things worse (if I understood you
correctly).
AFAICT, you are not testing the use case that prompted [trunk] revision
12742.1.41 changes. There are three possibilities:
1. r12742.1.41 changes were bogus/useless.
2. Your changes break use cases supported by r12742.1.41 changes.
3. Your changes do not break use cases supported by r12742.1.41
because all of them have a COMM_TRANSPARENT flag set.
To know which outcome is correct, we need a test case that:
A. Does not work without r12742.1.41 logic.
B. Works with the current (i.e., post-r12742.1.41) unpatched code.
C. Does not have a COMM_TRANSPARENT flag set.
If such a test case exists, then #2 is correct. Unfortunately, I do not
have a ready-to-use instructions on how to construct such a use case,
but it probably has to involve:
* direct/non-intercepted control connection (hence, no COMM_TRANSPARENT
flag if you are testing patched Squid code);
* multiple IP addresses from which Squid can open FTP data connections
to the FTP client (i.e., the FTP client IP has to be reachable from the
Squid box from two different source IP addresses);
* TCP stack selecting the wrong default source IP address when
establishing a connection from Squid to the FTP client.
When all of the above bullets are satisfied, r12742.1.41 starts to
matter in the patched code, meeting conditions A-C above. AFAICT, your
current test cases do not satisfy the last two bullets.
In other words, you patch removes setting the source IP address of the
Squid data connection to the destination address of the control
connection when there is no COMM_TRANSPARENT flag. Squid r12742.1.41
claims that such setting is necessary for some use cases to work. It is
not clear whether r12742.1.41 is only relevant to COMM_TRANSPARENT
connections. If it is, then your patch does not break things. If it is
not, then your patch probably does.
Hope this clarifies,
Alex._______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
_______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
