From 7aa75981dfc17eb7f0ac9ee7300e346f3b6a0c8e Mon Sep 17 00:00:00 2001
From: Numan Siddique <numans@ovn.org>
Date: Tue, 7 Jul 2020 18:30:20 +0530
Subject: [PATCH 1/5] ovn-northd: Fix the missing lflow issue in LS_OUT_PRE_LB.

When load balancer(s) configured with VIPs are associated to a logical switch,
then ovn-northd adds the below logical flow so that the packets in the egress
switch pipeline enter the conntrack.

table=0 (ls_out_pre_lb      ), priority=100  , match=(ip), action=(reg0[[0]] = 1; next;)

ovn-northd maintains a local boolean variable 'vip_configured' in
build_pre_lb() and adds the above lflow if this is true at the end.
But this variable is overriden as -> vip_configured = !!lb->n_vips;
when it loops through every load balancer associated with the logical switch.

This is wrong and this patch fixes this issue.

A test case is addd to test this scenario and the test case fails without the
fix in this patch.

Fixes: bb9f2b9ce56c("ovn-northd: Consider load balancer active backends in router pipeline")

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1849162
Reported-by: Tim Rozet <trozet@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>

(cherry-picked from master commit 59af6f9048946e16813ad7ad4e453b85989670e4)
---
 northd/ovn-northd.c |  2 +-
 tests/ovn-northd.at | 73 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8a809d020..2b891c68f 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4932,7 +4932,7 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows,
              * table, we will eventually look at L4 information. */
         }
 
-        vip_configured = !!lb->n_vips;
+        vip_configured = (vip_configured || lb->n_vips);
     }
 
     /* 'REGBIT_CONNTRACK_DEFRAG' is set to let the pre-stateful table send
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 37805d3d8..842800b90 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -1485,3 +1485,76 @@ lsp2
 ])
 
 AT_CLEANUP
+
+# This test case tests that when a logical switch has load balancers associated
+# (with VIPs configured), the below logical flow is added by ovn-northd.
+# table=0 (ls_out_pre_lb      ), priority=100  , match=(ip), action=(reg0[[0]] = 1; next;)
+# This test case is added for the BZ -
+# https://bugzilla.redhat.com/show_bug.cgi?id=1849162
+#
+# ovn-northd was not adding the above lflow if the last load balancer associated
+# to the logical switch doesn't have the VIP configured even if other load
+# balancers before the last one in the last have VIPs configured.
+# So make sure that the above lflow is added even if one load balancer has VIP
+# associated.
+
+AT_SETUP([ovn -- Load balancer - missing ls_out_pre_lb flows])
+ovn_start
+
+ovn-nbctl ls-add sw0
+ovn-nbctl lsp-add sw0 sw0-p1
+
+ovn-nbctl lb-add lb1 "10.0.0.10" "10.0.0.3"
+ovn-nbctl lb-add lb2 "10.0.0.11" "10.0.0.4"
+
+ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+])
+
+ovn-nbctl ls-lb-add sw0 lb1
+ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+  table=0 (ls_out_pre_lb      ), priority=100  , match=(ip), action=(reg0[[0]] = 1; next;)
+])
+
+ovn-nbctl ls-lb-add sw0 lb2
+ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+  table=0 (ls_out_pre_lb      ), priority=100  , match=(ip), action=(reg0[[0]] = 1; next;)
+])
+
+lb1_uuid=$(ovn-nbctl --bare --columns _uuid find load_balancer name=lb1)
+lb2_uuid=$(ovn-nbctl --bare --columns _uuid find load_balancer name=lb2)
+
+ovn-nbctl clear load_balancer $lb1_uuid vips
+ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+  table=0 (ls_out_pre_lb      ), priority=100  , match=(ip), action=(reg0[[0]] = 1; next;)
+])
+
+ovn-nbctl clear load_balancer $lb2_uuid vips
+ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+])
+
+ovn-nbctl set load_balancer $lb1_uuid vips:"10.0.0.10"="10.0.0.3"
+ovn-nbctl set load_balancer $lb2_uuid vips:"10.0.0.11"="10.0.0.4"
+
+ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+  table=0 (ls_out_pre_lb      ), priority=100  , match=(ip), action=(reg0[[0]] = 1; next;)
+])
+
+# Now reverse the order of clearing the vip.
+ovn-nbctl clear load_balancer $lb2_uuid vips
+ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+  table=0 (ls_out_pre_lb      ), priority=100  , match=(ip), action=(reg0[[0]] = 1; next;)
+])
+
+ovn-nbctl clear load_balancer $lb1_uuid vips
+ovn-nbctl --wait=sb sync
+AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl
+])
+
+AT_CLEANUP
-- 
2.26.2