[ovs-dev] [PATCH ovn] northd: Fix defrag flows for duplicate vips

Mark Michelson mmichels at redhat.com
Thu Jul 15 13:16:23 UTC 2021


Hi Mark,

I'm a bit curious about this change. Does the removal of the protocol 
from the match mean that traffic that is not of the protocol specified 
in the load balancer will be ct_dnat()'ed? Does that constitute 
unexpected behavior?

On 7/15/21 8:14 AM, Mark Gray wrote:
> When adding two SB flows with the same vip but different protocols, only
> the most recent flow will be added due to the `if` statement:
> 
>              if (!sset_contains(&all_ips, lb_vip->vip_str)) {
>                  sset_add(&all_ips, lb_vip->vip_str);
> 
> This can cause unexpected behaviour when two load balancers with
> the same VIP (and different protocols) are added to a logical router.
> 
> This is due to the addition of "protocol" to the match in
> defrag table flows in a previous commit. Revert that change.
> 
> This bug was discovered through the OVN CI (ovn-kubernetes.yml).
> 
> Fixes: 384a7c6237da ("northd: Refactor Logical Flows for routers with DNAT/Load Balancers")
> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> ---
>   northd/ovn-northd.c  |  8 --------
>   northd/ovn_northd.dl |  9 +--------
>   tests/ovn-northd.at  | 46 ++++++++++++++++++++++----------------------
>   3 files changed, 24 insertions(+), 39 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 999c3f482c29..5fab62c0fcf7 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -9219,11 +9219,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od,
>           for (size_t j = 0; j < lb->n_vips; j++) {
>               struct ovn_lb_vip *lb_vip = &lb->vips[j];
>   
> -            bool is_udp = nullable_string_is_equal(nb_lb->protocol, "udp");
> -            bool is_sctp = nullable_string_is_equal(nb_lb->protocol,
> -                                                    "sctp");
> -            const char *proto = is_udp ? "udp" : is_sctp ? "sctp" : "tcp";
> -
>               struct ds defrag_actions = DS_EMPTY_INITIALIZER;
>               if (!sset_contains(&all_ips, lb_vip->vip_str)) {
>                   sset_add(&all_ips, lb_vip->vip_str);
> @@ -9249,9 +9244,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od,
>                                     lb_vip->vip_str);
>                   }
>   
> -                if (lb_vip->vip_port) {
> -                    ds_put_format(match, " && %s", proto);
> -                }
>                   ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DEFRAG,
>                                           100, ds_cstr(match),
>                                           ds_cstr(&defrag_actions),
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index ceeabe6f384e..b37da86f76aa 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -6167,14 +6167,7 @@ for (RouterLBVIP(
>            *    pick a DNAT ip address from a group.
>            * 2. If there are L4 ports in load balancing rules, we
>            *    need the defragmentation to match on L4 ports. */
> -        var match1 = "ip && ${ipX}.dst == ${ip_address}" in
> -        var match2 =
> -            if (port != 0) {
> -                " && ${proto}"
> -            } else {
> -                ""
> -            } in
> -        var __match = match1 ++ match2 in
> +        var __match = "ip && ${ipX}.dst == ${ip_address}" in
>           var xx = ip_address.xxreg() in
>           var __actions = "${xx}${rEG_NEXT_HOP()} = ${ip_address}; ct_dnat;" in
>           /* One of these flows must be created for each unique LB VIP address.
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 11461d3f4c2a..072616898d63 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3167,7 +3167,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> @@ -3200,7 +3200,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> @@ -3243,7 +3243,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> @@ -3300,7 +3300,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> @@ -3344,7 +3344,7 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.20 && tcp), action=(reg0 = 10.0.0.20; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.20), action=(reg0 = 10.0.0.20; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | grep skip_snat_for_lb | sort], [0], [dnl
> @@ -4361,10 +4361,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>     table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> @@ -4421,10 +4421,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>     table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> @@ -4476,10 +4476,10 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>     table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> @@ -4534,11 +4534,11 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.10), action=(reg0 = 172.168.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>     table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> @@ -4605,12 +4605,12 @@ AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "lr_in_defrag" lr0flows | sort], [0], [dnl
>     table=5 (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 10.0.0.10), action=(reg0 = 10.0.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.10), action=(reg0 = 172.168.0.10; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.100), action=(reg0 = 172.168.0.100; ct_dnat;)
>     table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.200), action=(reg0 = 172.168.0.200; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;)
> -  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip6.dst == def0::2 && tcp), action=(xxreg0 = def0::2; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst == 172.168.0.210), action=(reg0 = 172.168.0.210; ct_dnat;)
> +  table=5 (lr_in_defrag       ), priority=100  , match=(ip && ip6.dst == def0::2), action=(xxreg0 = def0::2; ct_dnat;)
>   ])
>   
>   AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl
> 



More information about the dev mailing list