Re: [ovs-dev] [PATCH ovn] ovn-trace: correctly handle ct_dnat(IP) action

2021-06-10 Thread Mark Gray 
On 07/06/2021 10:58, Dumitru Ceara wrote:
> On 6/4/21 7:06 PM, Mark Gray wrote:
>> ovn-trace does not set translated ip address for ct_dnat()
>> actions when tracing. This causes the trace to end prematurely.
> 
> Hi Mark,
> 
> Thanks for the patch!

Thanks for the review. New series is at
https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/383754.html

>>
>> This can be tested with the following or an equivalent for IPv6:
>>
>> ovn-nbctl ls-add sw0
>> ovn-nbctl lsp-add sw0 sw0-port1
>> ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2"
>>
>> ovn-nbctl ls-add sw1
>> ovn-nbctl lsp-add sw1 sw1-port1
>> ovn-nbctl lsp-set-addresses sw1-port1 "50:54:00:00:00:03 11.0.0.2"
>>
>> ovn-nbctl lr-add lr0
>> ovn-nbctl lrp-add lr0 lrp0 00:00:00:00:ff:01 192.168.0.1/24
>> ovn-nbctl lsp-add sw0 lrp0-attachment
>> ovn-nbctl lsp-set-type lrp0-attachment router
>> ovn-nbctl lsp-set-addresses lrp0-attachment 00:00:00:00:ff:01
>> ovn-nbctl lsp-set-options lrp0-attachment router-port=lrp0
>> ovn-nbctl lrp-add lr0 lrp1 00:00:00:00:ff:02 11.0.0.1/24 -- 
>> lrp-set-gateway-chassis lrp1 chassis-1
>> ovn-nbctl lsp-add sw1 lrp1-attachment
>> ovn-nbctl lsp-set-type lrp1-attachment router
>> ovn-nbctl lsp-set-addresses lrp1-attachment 00:00:00:00:ff:02
>> ovn-nbctl lsp-set-options lrp1-attachment router-port=lrp1
>>
>> ovn-nbctl lr-nat-add lr0 dnat 42.42.42.42 192.168.0.2
>>
>> ovs-vsctl add-port br-int p1 -- \
>> set Interface p1 external_ids:iface-id=sw0-port1
>> ovs-vsctl add-port br-int p2 -- \
>> set Interface p2 external_ids:iface-id=sw1-port1
>>
>> ovn-trace  'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst 
>> == 00:00:00:00:ff:02 && ip4.dst == 42.42.42.42 && ip4.src == 11.0.0.2 && 
>> ip.ttl == 64'
> 
> We have some ovn-trace self tests already, in ovn-northd.at (for
> configurations that don't need ovn-controller) and ovn.at (for more
> complex scenario), so I think this could be added as a new test there.


Good point. I didn't know there were already tests there. However, I
think I have found them all so I will move them out into an ovn-trace.at
file to make it easier for the next person to find them.
> 
>>
>> Signed-off-by: Mark Gray 
>> ---
>>  utilities/ovn-trace.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
>> index 3b26b5af1d69..15b737b23496 100644
>> --- a/utilities/ovn-trace.c
>> +++ b/utilities/ovn-trace.c
>> @@ -2297,10 +2297,20 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
>>  if (ct_nat->family == AF_INET) {
>>  ds_put_format(&s, "(ip4.%s="IP_FMT")", direction,
>>IP_ARGS(ct_nat->ipv4));
>> +if (is_dst) {
>> +ct_flow.nw_dst=ct_nat->ipv4;
>> +} else {
>> +ct_flow.nw_src=ct_nat->ipv4;
>> +}
> 
> Nit: needs spaces around '='

Thanks
> 
>>  } else {
>>  ds_put_format(&s, "(ip6.%s=", direction);
>>  ipv6_format_addr(&ct_nat->ipv6, &s);
>>  ds_put_char(&s, ')');
>> +if (is_dst) {
>> +ct_flow.ipv6_dst=ct_nat->ipv6;
>> +} else {
>> +ct_flow.ipv6_src=ct_nat->ipv6;
>> +}
> 
> Nit: same here

Thanks

> 
>>  }
>>  
>>  uint8_t state = is_dst ? CS_DST_NAT : CS_SRC_NAT;
>>
> 
> Thanks,
> Dumitru
> 

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

Re: [ovs-dev] [PATCH ovn] ovn-trace: correctly handle ct_dnat(IP) action

2021-06-09 Thread 0-day Robot 
Bleep bloop.  Greetings Mark Gray, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#54 FILE: utilities/ovn-trace.c:2301:
ct_flow.nw_dst=ct_nat->ipv4;

WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#56 FILE: utilities/ovn-trace.c:2303:
ct_flow.nw_src=ct_nat->ipv4;

WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#63 FILE: utilities/ovn-trace.c:2310:
ct_flow.ipv6_dst=ct_nat->ipv6;

WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#65 FILE: utilities/ovn-trace.c:2312:
ct_flow.ipv6_src=ct_nat->ipv6;

Lines checked: 72, Warnings: 8, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ovn-trace: correctly handle ct_dnat(IP) action

2021-06-07 Thread Dumitru Ceara 
On 6/4/21 7:06 PM, Mark Gray wrote:
> ovn-trace does not set translated ip address for ct_dnat()
> actions when tracing. This causes the trace to end prematurely.

Hi Mark,

Thanks for the patch!

> 
> This can be tested with the following or an equivalent for IPv6:
> 
> ovn-nbctl ls-add sw0
> ovn-nbctl lsp-add sw0 sw0-port1
> ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2"
> 
> ovn-nbctl ls-add sw1
> ovn-nbctl lsp-add sw1 sw1-port1
> ovn-nbctl lsp-set-addresses sw1-port1 "50:54:00:00:00:03 11.0.0.2"
> 
> ovn-nbctl lr-add lr0
> ovn-nbctl lrp-add lr0 lrp0 00:00:00:00:ff:01 192.168.0.1/24
> ovn-nbctl lsp-add sw0 lrp0-attachment
> ovn-nbctl lsp-set-type lrp0-attachment router
> ovn-nbctl lsp-set-addresses lrp0-attachment 00:00:00:00:ff:01
> ovn-nbctl lsp-set-options lrp0-attachment router-port=lrp0
> ovn-nbctl lrp-add lr0 lrp1 00:00:00:00:ff:02 11.0.0.1/24 -- 
> lrp-set-gateway-chassis lrp1 chassis-1
> ovn-nbctl lsp-add sw1 lrp1-attachment
> ovn-nbctl lsp-set-type lrp1-attachment router
> ovn-nbctl lsp-set-addresses lrp1-attachment 00:00:00:00:ff:02
> ovn-nbctl lsp-set-options lrp1-attachment router-port=lrp1
> 
> ovn-nbctl lr-nat-add lr0 dnat 42.42.42.42 192.168.0.2
> 
> ovs-vsctl add-port br-int p1 -- \
> set Interface p1 external_ids:iface-id=sw0-port1
> ovs-vsctl add-port br-int p2 -- \
> set Interface p2 external_ids:iface-id=sw1-port1
> 
> ovn-trace  'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst 
> == 00:00:00:00:ff:02 && ip4.dst == 42.42.42.42 && ip4.src == 11.0.0.2 && 
> ip.ttl == 64'

We have some ovn-trace self tests already, in ovn-northd.at (for
configurations that don't need ovn-controller) and ovn.at (for more
complex scenario), so I think this could be added as a new test there.

> 
> Signed-off-by: Mark Gray 
> ---
>  utilities/ovn-trace.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 3b26b5af1d69..15b737b23496 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -2297,10 +2297,20 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
>  if (ct_nat->family == AF_INET) {
>  ds_put_format(&s, "(ip4.%s="IP_FMT")", direction,
>IP_ARGS(ct_nat->ipv4));
> +if (is_dst) {
> +ct_flow.nw_dst=ct_nat->ipv4;
> +} else {
> +ct_flow.nw_src=ct_nat->ipv4;
> +}

Nit: needs spaces around '='

>  } else {
>  ds_put_format(&s, "(ip6.%s=", direction);
>  ipv6_format_addr(&ct_nat->ipv6, &s);
>  ds_put_char(&s, ')');
> +if (is_dst) {
> +ct_flow.ipv6_dst=ct_nat->ipv6;
> +} else {
> +ct_flow.ipv6_src=ct_nat->ipv6;
> +}

Nit: same here

>  }
>  
>  uint8_t state = is_dst ? CS_DST_NAT : CS_SRC_NAT;
> 

Thanks,
Dumitru

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