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

Dumitru Ceara dceara at redhat.com
Fri Jun 11 10:41:37 UTC 2021


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,
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.

>  
>  # 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.

> +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