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.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.
ACK, this would be nice. It might also allow us to run the dp tests in parallel.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
