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

Mark Michelson Thu, 15 Jul 2021 06:16:37 -0700

Hi Mark,

I'm a bit curious about this change. Does the removal of the protocolfrom the match mean that traffic that is not of the protocol specifiedin the load balancer will be ct_dnat()'ed? Does that constituteunexpected 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 <[email protected]>
---
  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