On Mon, Sep 04, 2023 at 07:45:07PM +0400, 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]>
Hi Viacheslav, thanks for your patch-set. I agree this is a problem that ought to be fixed. Some feedback on the patchset from my side: 1. The test added by patch 1/2 fails without patch 2/2. I think it would be good to reverse the order of the patches in the series. 2. With this series applied the following check-system-userspace tests seem to fail. (I only checked with TESTSUITEFLAGS="-k conntrack" so far.) 149: conntrack - IPv6 FTP with SNAT FAILED (system-traffic.at:7119) 150: conntrack - IPv6 FTP Passive with SNAT FAILED (system-traffic.at:7180) 151: conntrack - IPv6 FTP with SNAT - orig tuple FAILED (system-traffic.at:7240) In each case the critical part of the test log seems to be something like (this is for test #151): ./system-traffic.at:7240: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC wget ftp://[fc00::2] -6 --no-passive-ftp -t 3 -T 1 --retry-connrefused -v --serv er-response --no-remove-listing -o wget0.log -d NS_EXEC_HEREDOC [I 2023-09-29 13:00:21] fc00::240:43342-[] FTP session opened (connect) [I 2023-09-29 13:00:21] fc00::240:43342-[anonymous] USER 'anonymous' logged in. [I 2023-09-29 13:00:22] fc00::240:43342-[anonymous] FTP session closed (disconne ct). [I 2023-09-29 13:00:23] fc00::240:43344-[] FTP session opened (connect) [I 2023-09-29 13:00:23] fc00::240:43344-[anonymous] USER 'anonymous' logged in. [I 2023-09-29 13:00:24] fc00::240:43344-[anonymous] FTP session closed (disconne ct). [I 2023-09-29 13:00:26] fc00::240:52606-[] FTP session opened (connect) [I 2023-09-29 13:00:26] fc00::240:52606-[anonymous] USER 'anonymous' logged in. [I 2023-09-29 13:00:27] fc00::240:52606-[anonymous] FTP session closed (disconne ct). ./system-traffic.at:7240: exit code was 4, expected 0 3. In v1 [1] there was some discussion of the reasoning behind this change, I think it would be good to incorporate some of that information in the commit message of this patch. [1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-July/406585.html "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. "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. "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. > --- > lib/conntrack.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 47a443fba..3d57e55ca 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -1249,6 +1249,10 @@ process_one(struct conntrack *ct, struct dp_packet > *pkt, > conn = NULL; > } > > + if (conn && helper == NULL) { > + helper = conn->alg; > + } > + > enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt, tp_src, tp_dst, > helper); > > -- > 2.42.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
