| Message ID | 20240118130018.4086011-1-david.marchand@redhat.com |
|---|---|
| State | Rejected |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] system-common-macros: Fix conntrack matching. | 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 | fail | test: fail |
On Thu, Jan 18, 2024 at 02:00:18PM +0100, David Marchand wrote: > Seen in GHA recently. > Unit tests are checking conntracks relating to a destination ip address > but the FORMAT_CT macro is not strict enough and would match unrelated > conntracks too. > > Example: > 148. system-traffic.at:6432: testing conntrack - DNAT with > additional SNAT ... > [...] > ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | > grep "dst=10.1.1.1" | > sed -e 's/port=[0-9]*/port=<cleared>/g' > -e 's/id=[0-9]*/id=<cleared>/g' > -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq > [...] > @@ -1,2 +1,7 @@ > tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > > Fixes: 07659514c3c1 ("Add support for connection tracking.") > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > tests/system-common-macros.at | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 01ebe364ee..07be29f673 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) > # and limit the output to the rows containing 'ip-addr'. > # > m4_define([FORMAT_CT], > - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > > # NETNS_DAEMONIZE([namespace], [command], [pidfile]) > # Sorry, I feel I mist be missing something very obvious, but I'm unsure why the match is on "dst=$1\>". I would have thought the match would be "dst=$1," instead.
On Fri, Jan 19, 2024 at 1:20 PM Simon Horman <horms@ovn.org> wrote: > > On Thu, Jan 18, 2024 at 02:00:18PM +0100, David Marchand wrote: > > Seen in GHA recently. > > Unit tests are checking conntracks relating to a destination ip address > > but the FORMAT_CT macro is not strict enough and would match unrelated > > conntracks too. > > > > Example: > > 148. system-traffic.at:6432: testing conntrack - DNAT with > > additional SNAT ... > > [...] > > ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | > > grep "dst=10.1.1.1" | > > sed -e 's/port=[0-9]*/port=<cleared>/g' > > -e 's/id=[0-9]*/id=<cleared>/g' > > -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq > > [...] > > @@ -1,2 +1,7 @@ > > tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... > > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... > > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > > > > Fixes: 07659514c3c1 ("Add support for connection tracking.") > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > --- > > tests/system-common-macros.at | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > > index 01ebe364ee..07be29f673 100644 > > --- a/tests/system-common-macros.at > > +++ b/tests/system-common-macros.at > > @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) > > # and limit the output to the rows containing 'ip-addr'. > > # > > m4_define([FORMAT_CT], > > - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > > + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > > > > # NETNS_DAEMONIZE([namespace], [command], [pidfile]) > > # > > Sorry, I feel I mist be missing something very obvious, but > I'm unsure why the match is on "dst=$1\>". I would have thought > the match would be "dst=$1," instead. \> matches the end of a word. Using , as a delimiter works too in this case.
On 1/18/24 14:00, David Marchand wrote: > Seen in GHA recently. > Unit tests are checking conntracks relating to a destination ip address > but the FORMAT_CT macro is not strict enough and would match unrelated > conntracks too. > > Example: > 148. system-traffic.at:6432: testing conntrack - DNAT with > additional SNAT ... > [...] > ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | > grep "dst=10.1.1.1" | > sed -e 's/port=[0-9]*/port=<cleared>/g' > -e 's/id=[0-9]*/id=<cleared>/g' > -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq > [...] > @@ -1,2 +1,7 @@ > tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > > Fixes: 07659514c3c1 ("Add support for connection tracking.") > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > tests/system-common-macros.at | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 01ebe364ee..07be29f673 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) > # and limit the output to the rows containing 'ip-addr'. > # > m4_define([FORMAT_CT], > - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > > # NETNS_DAEMONIZE([namespace], [command], [pidfile]) > # I remembered why the macro is loose. We wanted to be able to match on "subnets" by supplying only part of the address. There was at least one test that used this functionality. Eelco removed it though here: https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9 Did you check if have any more instances of such tests? They can be tricky to find, as we can supply 10.1.1.2 in order to match 10.1.1.240, for example. Best regards, Ilya Maximets.
On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 1/18/24 14:00, David Marchand wrote: > > Seen in GHA recently. > > Unit tests are checking conntracks relating to a destination ip address > > but the FORMAT_CT macro is not strict enough and would match unrelated > > conntracks too. > > > > Example: > > 148. system-traffic.at:6432: testing conntrack - DNAT with > > additional SNAT ... > > [...] > > ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | > > grep "dst=10.1.1.1" | > > sed -e 's/port=[0-9]*/port=<cleared>/g' > > -e 's/id=[0-9]*/id=<cleared>/g' > > -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq > > [...] > > @@ -1,2 +1,7 @@ > > tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... > > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... > > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > > > > Fixes: 07659514c3c1 ("Add support for connection tracking.") > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > --- > > tests/system-common-macros.at | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > > index 01ebe364ee..07be29f673 100644 > > --- a/tests/system-common-macros.at > > +++ b/tests/system-common-macros.at > > @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) > > # and limit the output to the rows containing 'ip-addr'. > > # > > m4_define([FORMAT_CT], > > - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > > + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > > > > # NETNS_DAEMONIZE([namespace], [command], [pidfile]) > > # > > I remembered why the macro is loose. We wanted to be able > to match on "subnets" by supplying only part of the address. > > There was at least one test that used this functionality. > Eelco removed it though here: > https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9 > > Did you check if have any more instances of such tests? I did not. > They can be tricky to find, as we can supply 10.1.1.2 in order > to match 10.1.1.240, for example. Ok, you can discard my patch. Thanks.
On 19 Jan 2024, at 13:49, Ilya Maximets wrote: > On 1/18/24 14:00, David Marchand wrote: >> Seen in GHA recently. >> Unit tests are checking conntracks relating to a destination ip address >> but the FORMAT_CT macro is not strict enough and would match unrelated >> conntracks too. >> >> Example: >> 148. system-traffic.at:6432: testing conntrack - DNAT with >> additional SNAT ... >> [...] >> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | >> grep "dst=10.1.1.1" | >> sed -e 's/port=[0-9]*/port=<cleared>/g' >> -e 's/id=[0-9]*/id=<cleared>/g' >> -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq >> [...] >> @@ -1,2 +1,7 @@ >> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... >> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... >> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... >> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... >> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... >> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... >> >> Fixes: 07659514c3c1 ("Add support for connection tracking.") >> Signed-off-by: David Marchand <david.marchand@redhat.com> >> --- >> tests/system-common-macros.at | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at >> index 01ebe364ee..07be29f673 100644 >> --- a/tests/system-common-macros.at >> +++ b/tests/system-common-macros.at >> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) >> # and limit the output to the rows containing 'ip-addr'. >> # >> m4_define([FORMAT_CT], >> - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) >> + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) >> >> # NETNS_DAEMONIZE([namespace], [command], [pidfile]) >> # > > I remembered why the macro is loose. We wanted to be able > to match on "subnets" by supplying only part of the address. > > There was at least one test that used this functionality. > Eelco removed it though here: > https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9 > I guess the subnet mask part will work if we add the subnet dot ., i,e. FORMAT_CT(10.1.1.)] But this also requires some more change, i.e. making the dot not being a wildcard. Something like this: - [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) + [[grep "dst=$(echo $1 | sed -e 's/\./\\./g')\>" | \ > Did you check if have any more instances of such tests? > They can be tricky to find, as we can supply 10.1.1.2 in order > to match 10.1.1.240, for example. > > Best regards, Ilya Maximets.
On 19 Jan 2024, at 13:53, David Marchand wrote: > On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 1/18/24 14:00, David Marchand wrote: >>> Seen in GHA recently. >>> Unit tests are checking conntracks relating to a destination ip address >>> but the FORMAT_CT macro is not strict enough and would match unrelated >>> conntracks too. >>> >>> Example: >>> 148. system-traffic.at:6432: testing conntrack - DNAT with >>> additional SNAT ... >>> [...] >>> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | >>> grep "dst=10.1.1.1" | >>> sed -e 's/port=[0-9]*/port=<cleared>/g' >>> -e 's/id=[0-9]*/id=<cleared>/g' >>> -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq >>> [...] >>> @@ -1,2 +1,7 @@ >>> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... >>> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... >>> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... >>> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... >>> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... >>> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... >>> >>> Fixes: 07659514c3c1 ("Add support for connection tracking.") >>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>> --- >>> tests/system-common-macros.at | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at >>> index 01ebe364ee..07be29f673 100644 >>> --- a/tests/system-common-macros.at >>> +++ b/tests/system-common-macros.at >>> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) >>> # and limit the output to the rows containing 'ip-addr'. >>> # >>> m4_define([FORMAT_CT], >>> - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) >>> + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) >>> >>> # NETNS_DAEMONIZE([namespace], [command], [pidfile]) >>> # >> >> I remembered why the macro is loose. We wanted to be able >> to match on "subnets" by supplying only part of the address. >> >> There was at least one test that used this functionality. >> Eelco removed it though here: >> https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9 >> >> Did you check if have any more instances of such tests? > > I did not. > >> They can be tricky to find, as we can supply 10.1.1.2 in order >> to match 10.1.1.240, for example. > > Ok, you can discard my patch. > Thanks. But looking at most of the test cases when they put in an IP they mean that specific IP not 10.1.1.20? But maybe your NS idea works better. //Eelco
On Fri, Jan 19, 2024 at 01:40:03PM +0100, David Marchand wrote: > On Fri, Jan 19, 2024 at 1:20 PM Simon Horman <horms@ovn.org> wrote: > > > > On Thu, Jan 18, 2024 at 02:00:18PM +0100, David Marchand wrote: > > > Seen in GHA recently. > > > Unit tests are checking conntracks relating to a destination ip address > > > but the FORMAT_CT macro is not strict enough and would match unrelated > > > conntracks too. > > > > > > Example: > > > 148. system-traffic.at:6432: testing conntrack - DNAT with > > > additional SNAT ... > > > [...] > > > ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | > > > grep "dst=10.1.1.1" | > > > sed -e 's/port=[0-9]*/port=<cleared>/g' > > > -e 's/id=[0-9]*/id=<cleared>/g' > > > -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq > > > [...] > > > @@ -1,2 +1,7 @@ > > > tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... > > > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > > > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... > > > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > > > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... > > > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... > > > > > > Fixes: 07659514c3c1 ("Add support for connection tracking.") > > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > > --- > > > tests/system-common-macros.at | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > > > index 01ebe364ee..07be29f673 100644 > > > --- a/tests/system-common-macros.at > > > +++ b/tests/system-common-macros.at > > > @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) > > > # and limit the output to the rows containing 'ip-addr'. > > > # > > > m4_define([FORMAT_CT], > > > - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > > > + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) > > > > > > # NETNS_DAEMONIZE([namespace], [command], [pidfile]) > > > # > > > > Sorry, I feel I mist be missing something very obvious, but > > I'm unsure why the match is on "dst=$1\>". I would have thought > > the match would be "dst=$1," instead. > > \> matches the end of a word. > Using , as a delimiter works too in this case. Thanks, now I understand :)
On 1/19/24 14:01, Eelco Chaudron wrote: > > > On 19 Jan 2024, at 13:53, David Marchand wrote: > >> On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets <i.maximets@ovn.org> wrote: >>> >>> On 1/18/24 14:00, David Marchand wrote: >>>> Seen in GHA recently. >>>> Unit tests are checking conntracks relating to a destination ip address >>>> but the FORMAT_CT macro is not strict enough and would match unrelated >>>> conntracks too. >>>> >>>> Example: >>>> 148. system-traffic.at:6432: testing conntrack - DNAT with >>>> additional SNAT ... >>>> [...] >>>> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | >>>> grep "dst=10.1.1.1" | >>>> sed -e 's/port=[0-9]*/port=<cleared>/g' >>>> -e 's/id=[0-9]*/id=<cleared>/g' >>>> -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq >>>> [...] >>>> @@ -1,2 +1,7 @@ >>>> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... >>>> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... >>>> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... >>>> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... >>>> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... >>>> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... >>>> >>>> Fixes: 07659514c3c1 ("Add support for connection tracking.") >>>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>>> --- >>>> tests/system-common-macros.at | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at >>>> index 01ebe364ee..07be29f673 100644 >>>> --- a/tests/system-common-macros.at >>>> +++ b/tests/system-common-macros.at >>>> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) >>>> # and limit the output to the rows containing 'ip-addr'. >>>> # >>>> m4_define([FORMAT_CT], >>>> - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) >>>> + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) >>>> >>>> # NETNS_DAEMONIZE([namespace], [command], [pidfile]) >>>> # >>> >>> I remembered why the macro is loose. We wanted to be able >>> to match on "subnets" by supplying only part of the address. >>> >>> There was at least one test that used this functionality. >>> Eelco removed it though here: >>> https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9 >>> >>> Did you check if have any more instances of such tests? >> >> I did not. >> >>> They can be tricky to find, as we can supply 10.1.1.2 in order >>> to match 10.1.1.240, for example. >> >> Ok, you can discard my patch. >> Thanks. > > But looking at most of the test cases when they put in an IP they > mean that specific IP not 10.1.1.20? The key word here is 'most', it's hard to tell for all of them without actually analyzing the logic of each of them. > But maybe your NS idea works better. It should fix the issue at hands. So I marked this one as rejected for now. We can revisit it later if the need arises. If we move to separate namespaces per test it may be possible to just remove filtering entirely and check the exact content of conntrack tables instead, I hope. Best regards, Ilya Maximets.
On 22 Jan 2024, at 21:51, Ilya Maximets wrote: > On 1/19/24 14:01, Eelco Chaudron wrote: >> >> >> On 19 Jan 2024, at 13:53, David Marchand wrote: >> >>> On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets <i.maximets@ovn.org> wrote: >>>> >>>> On 1/18/24 14:00, David Marchand wrote: >>>>> Seen in GHA recently. >>>>> Unit tests are checking conntracks relating to a destination ip address >>>>> but the FORMAT_CT macro is not strict enough and would match unrelated >>>>> conntracks too. >>>>> >>>>> Example: >>>>> 148. system-traffic.at:6432: testing conntrack - DNAT with >>>>> additional SNAT ... >>>>> [...] >>>>> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | >>>>> grep "dst=10.1.1.1" | >>>>> sed -e 's/port=[0-9]*/port=<cleared>/g' >>>>> -e 's/id=[0-9]*/id=<cleared>/g' >>>>> -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq >>>>> [...] >>>>> @@ -1,2 +1,7 @@ >>>>> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... >>>>> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... >>>>> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... >>>>> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... >>>>> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... >>>>> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... >>>>> >>>>> Fixes: 07659514c3c1 ("Add support for connection tracking.") >>>>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>>>> --- >>>>> tests/system-common-macros.at | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at >>>>> index 01ebe364ee..07be29f673 100644 >>>>> --- a/tests/system-common-macros.at >>>>> +++ b/tests/system-common-macros.at >>>>> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) >>>>> # and limit the output to the rows containing 'ip-addr'. >>>>> # >>>>> m4_define([FORMAT_CT], >>>>> - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) >>>>> + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) >>>>> >>>>> # NETNS_DAEMONIZE([namespace], [command], [pidfile]) >>>>> # >>>> >>>> I remembered why the macro is loose. We wanted to be able >>>> to match on "subnets" by supplying only part of the address. >>>> >>>> There was at least one test that used this functionality. >>>> Eelco removed it though here: >>>> https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9 >>>> >>>> Did you check if have any more instances of such tests? >>> >>> I did not. >>> >>>> They can be tricky to find, as we can supply 10.1.1.2 in order >>>> to match 10.1.1.240, for example. >>> >>> Ok, you can discard my patch. >>> Thanks. >> >> But looking at most of the test cases when they put in an IP they >> mean that specific IP not 10.1.1.20? > > The key word here is 'most', it's hard to tell for all of them without > actually analyzing the logic of each of them. > >> But maybe your NS idea works better. > > It should fix the issue at hands. So I marked this one as rejected for > now. We can revisit it later if the need arises. > > If we move to separate namespaces per test it may be possible to just > remove filtering entirely and check the exact content of conntrack > tables instead, I hope. ACK, this would be nice. It might also allow us to run the dp tests in parallel.
diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at index 01ebe364ee..07be29f673 100644 --- a/tests/system-common-macros.at +++ b/tests/system-common-macros.at @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed 's/csum:.*/csum: <skip>/']) # and limit the output to the rows containing 'ip-addr'. # m4_define([FORMAT_CT], - [[grep "dst=$1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) + [[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq]]) # NETNS_DAEMONIZE([namespace], [command], [pidfile]) #
Seen in GHA recently. Unit tests are checking conntracks relating to a destination ip address but the FORMAT_CT macro is not strict enough and would match unrelated conntracks too. Example: 148. system-traffic.at:6432: testing conntrack - DNAT with additional SNAT ... [...] ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack | grep "dst=10.1.1.1" | sed -e 's/port=[0-9]*/port=<cleared>/g' -e 's/id=[0-9]*/id=<cleared>/g' -e 's/state=[0-9_A-Z]*/state=<cleared>/g' | sort | uniq [...] @@ -1,2 +1,7 @@ tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,... +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=<cleared>,... +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=<cleared>,... +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=<cleared>,... Fixes: 07659514c3c1 ("Add support for connection tracking.") Signed-off-by: David Marchand <david.marchand@redhat.com> --- tests/system-common-macros.at | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)