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.maxim...@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.march...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
