| Message ID | 20230713092633.7507-3-viacheslav.galaktionov@arknetworks.am |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] conntrack: Use helpers from committed connections. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <viacheslav.galaktionov@arknetworks.am> > Acked-by: Ivan Malov <ivan.malov@arknetworks.am> > --- > 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.
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 <viacheslav.galaktionov@arknetworks.am> >> Acked-by: Ivan Malov <ivan.malov@arknetworks.am> >> --- >> 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. 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.
Viacheslav Galaktionov <Viacheslav.Galaktionov@arknetworks.am> 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 <viacheslav.galaktionov@arknetworks.am> >>> Acked-by: Ivan Malov <ivan.malov@arknetworks.am> >>> --- >>> 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.
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; + } + enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt, tp_src, tp_dst, helper);