Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-22 Thread Eelco Chaudron 


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  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=/g'
>   -e 's/id=[0-9]*/id=/g'
>   -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> [...]
> @@ -1,2 +1,7 @@
>  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>
> Fixes: 07659514c3c1 ("Add support for connection tracking.")
> Signed-off-by: David Marchand 
> ---
>  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: /'])
>  # and limit the output to the rows containing 'ip-addr'.
>  #
>  m4_define([FORMAT_CT],
> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | 
> sort | uniq]])
> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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

Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-22 Thread Ilya Maximets 
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  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=/g'
   -e 's/id=[0-9]*/id=/g'
   -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
 [...]
 @@ -1,2 +1,7 @@
  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
 +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
 +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
 +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
 +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
 +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...

 Fixes: 07659514c3c1 ("Add support for connection tracking.")
 Signed-off-by: David Marchand 
 ---
  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: /'])
  # and limit the output to the rows containing 'ip-addr'.
  #
  m4_define([FORMAT_CT],
 -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | 
 sort | uniq]])
 +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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

Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-19 Thread Simon Horman 
On Fri, Jan 19, 2024 at 01:40:03PM +0100, David Marchand wrote:
> On Fri, Jan 19, 2024 at 1:20 PM Simon Horman  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=/g'
> > >   -e 's/id=[0-9]*/id=/g'
> > >   -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> > > [...]
> > > @@ -1,2 +1,7 @@
> > >  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> > > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> > > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> > > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> > >
> > > Fixes: 07659514c3c1 ("Add support for connection tracking.")
> > > Signed-off-by: David Marchand 
> > > ---
> > >  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: /'])
> > >  # and limit the output to the rows containing 'ip-addr'.
> > >  #
> > >  m4_define([FORMAT_CT],
> > > -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
> > > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | 
> > > sort | uniq]])
> > > +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
> > > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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 :)

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-19 Thread Eelco Chaudron 


On 19 Jan 2024, at 13:53, David Marchand wrote:

> On Fri, Jan 19, 2024 at 1:49 PM 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=/g'
>>>   -e 's/id=[0-9]*/id=/g'
>>>   -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
>>> [...]
>>> @@ -1,2 +1,7 @@
>>>  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
>>> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
>>> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
>>> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>>> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
>>> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>>>
>>> Fixes: 07659514c3c1 ("Add support for connection tracking.")
>>> Signed-off-by: David Marchand 
>>> ---
>>>  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: /'])
>>>  # and limit the output to the rows containing 'ip-addr'.
>>>  #
>>>  m4_define([FORMAT_CT],
>>> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
>>> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | 
>>> sort | uniq]])
>>> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
>>> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-19 Thread Eelco Chaudron 



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=/g'
>>  -e 's/id=[0-9]*/id=/g'
>>  -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
>> [...]
>> @@ -1,2 +1,7 @@
>>  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
>> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
>> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
>> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
>> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>>
>> Fixes: 07659514c3c1 ("Add support for connection tracking.")
>> Signed-off-by: David Marchand 
>> ---
>>  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: /'])
>>  # and limit the output to the rows containing 'ip-addr'.
>>  #
>>  m4_define([FORMAT_CT],
>> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
>> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | 
>> sort | uniq]])
>> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
>> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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=/g' -e 
's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-19 Thread David Marchand 
On Fri, Jan 19, 2024 at 1:49 PM 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=/g'
> >   -e 's/id=[0-9]*/id=/g'
> >   -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> > [...]
> > @@ -1,2 +1,7 @@
> >  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> >
> > Fixes: 07659514c3c1 ("Add support for connection tracking.")
> > Signed-off-by: David Marchand 
> > ---
> >  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: /'])
> >  # and limit the output to the rows containing 'ip-addr'.
> >  #
> >  m4_define([FORMAT_CT],
> > -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
> > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | 
> > sort | uniq]])
> > +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
> > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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.


-- 
David Marchand

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-19 Thread Ilya Maximets 
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=/g'
>   -e 's/id=[0-9]*/id=/g'
>   -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> [...]
> @@ -1,2 +1,7 @@
>  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> 
> Fixes: 07659514c3c1 ("Add support for connection tracking.")
> Signed-off-by: David Marchand 
> ---
>  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: /'])
>  # and limit the output to the rows containing 'ip-addr'.
>  #
>  m4_define([FORMAT_CT],
> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort 
> | uniq]])
> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-19 Thread David Marchand 
On Fri, Jan 19, 2024 at 1:20 PM Simon Horman  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=/g'
> >   -e 's/id=[0-9]*/id=/g'
> >   -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> > [...]
> > @@ -1,2 +1,7 @@
> >  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> >
> > Fixes: 07659514c3c1 ("Add support for connection tracking.")
> > Signed-off-by: David Marchand 
> > ---
> >  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: /'])
> >  # and limit the output to the rows containing 'ip-addr'.
> >  #
> >  m4_define([FORMAT_CT],
> > -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
> > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | 
> > sort | uniq]])
> > +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
> > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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.


-- 
David Marchand

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-19 Thread Simon Horman 
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=/g'
>   -e 's/id=[0-9]*/id=/g'
>   -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> [...]
> @@ -1,2 +1,7 @@
>  tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> 
> Fixes: 07659514c3c1 ("Add support for connection tracking.")
> Signed-off-by: David Marchand 
> ---
>  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: /'])
>  # and limit the output to the rows containing 'ip-addr'.
>  #
>  m4_define([FORMAT_CT],
> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort 
> | uniq]])
> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.

2024-01-18 Thread David Marchand 
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=/g'
-e 's/id=[0-9]*/id=/g'
-e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
[...]
@@ -1,2 +1,7 @@
 tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
+tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
+tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
+tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
+tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
+tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...

Fixes: 07659514c3c1 ("Add support for connection tracking.")
Signed-off-by: David Marchand 
---
 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: /'])
 # and limit the output to the rows containing 'ip-addr'.
 #
 m4_define([FORMAT_CT],
-[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e 
's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort | 
uniq]])
+[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e 
's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort | 
uniq]])
 
 # NETNS_DAEMONIZE([namespace], [command], [pidfile])
 #
-- 
2.43.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev