On 19 Jan 2024, at 13:53, David Marchand wrote:
> On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets <[email protected]> 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 <[email protected]> >>> --- >>> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
