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

Mark Gray mark.d.gray at redhat.com
Fri Jun 11 14:07:58 UTC 2021


On 11/06/2021 11:41, Dumitru Ceara wrote:
> On 6/11/21 11:52 AM, Mark Gray wrote:
>> ovn-trace does not set translated ip address for ct_dnat()
>> actions when tracing. This causes the trace to end prematurely.
>>
>> 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'
>>
>> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
>> ---
> 
> Hi Mark,
> 
> I have some of nits on the tests below, feel free to add my ack to the
> next revision after addressing them:
> 
> Acked-by: Dumitru Ceara <dceara at redhat.com>

Thanks
> 
> Thanks,
> Dumitru
> 
>>  tests/ovn-northd.at   | 80 ++++++++++++++++++++++++++++++++++++++++++-
>>  utilities/ovn-trace.c | 10 ++++++
>>  2 files changed, 89 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 4692775ad720..e3df3ee65000 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -483,7 +483,7 @@ check ovn-nbctl --wait=sb lsp-set-options sw0-lr1 router-port=lr1-sw0
>>  # connected to lr0
>>  exp_ref_ch_list="$comp1_ch_uuid $comp2_ch_uuid"
>>  
>> -wait_column "$exp_ref_ch_list" HA_Chassis_Group ref_chassis 
>> +wait_column "$exp_ref_ch_list" HA_Chassis_Group ref_chassis
> > Unrelated change, I guess it can be skipped and addressed separately.

Split it out
> 
>>  
>>  # Unind sw1-p1. comp2 should not be in the ref_chassis.
>>  ovn-sbctl lsp-unbind sw1-p1
>> @@ -3644,3 +3644,81 @@ check ovn-nbctl --wait=sb sync
>>  OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
>>  AT_CLEANUP
>>  ])
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([ovn -- trace with IPv4 dnat])
>> +AT_KEYWORDS([dnat])
>> +ovn_start
>> +
>> +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
> 
> The AT_CHECK would probably fail if any of the nbctl commands above fail
> but it's probably best if we prefix them with check().  To avoid check()
> all over the place we can merge all of them into a single ovn-nbctl command.
> 

I added a check in front of every command

>> +check ovn-nbctl --wait=sb sync
>> +
>> +AT_CHECK([ovn-trace --minimal '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' | grep "output(\"sw0-port1\")"], [0], [ignore])
>> +
>> +dnl If we remove the DNAT entry we will be unable to trace to the DNAT address
>> +ovn-nbctl lr-nat-del lr0 dnat 42.42.42.42
> 
> Same here.
> 
>> +check ovn-nbctl --wait=sb sync
>> +
>> +AT_CHECK([ovn-trace --minimal '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' | grep "output(\"sw0-port1\")"], [1], [ignore])
>> +
>> +AT_CLEANUP
>> +])
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([ovn -- trace with IPv6 dnat])
>> +AT_KEYWORDS([dnat])
>> +ovn_start
>> +
>> +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 fd68::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 fd11::2"
>> +
>> +ovn-nbctl lr-add lr0
>> +ovn-nbctl lrp-add lr0 lrp0 00:00:00:00:ff:01 fd68::1/64
>> +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 fd11::1/64 -- 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 fd42::42 fd68::2
> 
> Here too.
> 
>> +check ovn-nbctl --wait=sb sync
>> +
>> +AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip6.dst == fd42::42 && ip6.src == fd11::2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [0], [ignore])
>> +
>> +dnl If we remove the DNAT entry we will be unable to trace to the DNAT address
>> +ovn-nbctl lr-nat-del lr0 dnat fd42::42
> 
> And here.
> 
>> +check ovn-nbctl --wait=sb sync
>> +
>> +AT_CHECK([ovn-trace --minimal 'inport == "sw1-port1" && eth.src == 50:54:00:00:00:03 && eth.dst == 00:00:00:00:ff:02 && ip6.dst == fd42::42 && ip6.src == fd11::2 && ip.ttl == 64' | grep "output(\"sw0-port1\")"], [1], [ignore])
>> +
>> +AT_CLEANUP
>> +])
>> \ No newline at end of file
> 
> No newline at end of file.
> 
>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
>> index 3b26b5af1d69..49463c5c2652 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;
>> +            }
>>          } 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;
>> +            }
>>          }
>>  
>>          uint8_t state = is_dst ? CS_DST_NAT : CS_SRC_NAT;
>>
> 
> Thanks,
> Dumitru
> 



More information about the dev mailing list