Viacheslav Galaktionov <[email protected]> writes:
> On 7/14/23 19:11, Ilya Maximets wrote: >> On 7/13/23 11:26, Viacheslav Galaktionov via dev wrote: >>> Currently, if the user wants to track related connections, they have to >>> specify a helper in all CT actions, which contradicts the behaviour >>> described in the documentation. >>> >>> Fix this by using the helper committed along with the connection whenever >>> a given CT action does not specify a helper of its own. >>> >>> Signed-off-by: Viacheslav Galaktionov >>> <[email protected]> >>> Acked-by: Ivan Malov <[email protected]> >>> --- >>> lib/conntrack.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/lib/conntrack.c b/lib/conntrack.c >>> index 4375c03e2..d505ffed1 100644 >>> --- a/lib/conntrack.c >>> +++ b/lib/conntrack.c >>> @@ -1323,6 +1323,10 @@ process_one(struct conntrack *ct, struct dp_packet >>> *pkt, >>> } >>> } >>> + if (conn && helper == NULL) { >>> + helper = conn->alg; >>> + } >>> + >> Hi, Viacheslav. Thanks for the fix! >> >> We have tests in the system-traffic-userspace testsuite that should cover >> use of alg for FTP, for example. Do you know why they are not triggering >> the issue? Could you add a test that demonstrates it? >> >> Best regards, Ilya Maximets. > Hi Ilya! > > Sorry, it's been a while since I actually implemented this fix, so one > important detail is missing from the patch's description: this fix is > needed when the L7 server is running on a non-standard port. For example, > my testing involved a mock FTP server running on a random port, so this > problem popped up quickly. We should be able to do this in the standard test suite using a system traffic test. Right now, they use wget ftp://xxxxx/ and I guess we could modify test-l7 as follows: --- diff --git a/tests/test-l7.py b/tests/test-l7.py index 32a77392c6..4c3fa4519c 100755 --- a/tests/test-l7.py +++ b/tests/test-l7.py @@ -86,6 +86,8 @@ def main(): description='Run basic application servers.') parser.add_argument('proto', default='http', nargs='?', help='protocol to serve (%s)' % protocols) + parser.add_argument('port', default=0, nargs='?', + help="Port number") args = parser.parse_args() if args.proto not in protocols: @@ -95,6 +97,8 @@ def main(): constructor = SERVERS[args.proto][0] handler = SERVERS[args.proto][1] port = SERVERS[args.proto][2] + if args.port != 0: + port = args.port srv = constructor(('', port), handler) srv.serve_forever() --- Then we can run tests on non-standard ports and confuse the get_alg_ctl_type(), etc. > As I understand, there is a certain degree of guessing which L7 protocol > is used by a particular connection, see the get_alg_ctl_type function. > This function appears to look at a packet's src and dst ports and compare > them to standard FTP/TFTP ports. If it worked perfectly, I'd expect it > to be possible to completely omit the alg arguments from the flows used > in tests. However, doing so breaks the tests. I haven't had the time to > figure out why. > > It's clear that the helper that was committed along with the connection > is largely (or completely) ignored in the code. This means that only > the helper specified in the ct action can influence the result of this > "guess", i.e. that if an action doesn't specify a helper, then it's all > up to the packet's ports. This is what my patch aims to fix by letting > the connection's helper play its part in this decision process. +1 > Adding a test for this isn't exactly trivial since the test suite can > use only standard ports right now. I managed to reproduce the issue on > my local machine but this required me to comment out some waits and > adjust some hardcoded constants, which probably isn't suitable for > inclusion into the main repo. The above patch should allow for testing on non-standard ports. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
