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
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
--
2.27.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
