| Message ID | 20210715121446.2614139-1-mark.d.gray@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [ovs-dev] northd: Fix defrag flows for duplicate vips | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
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@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 >
On 15/07/2021 14:16, Mark Michelson wrote: > 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? > Yes, this is the case. It's a tradeoff between number of flows and reirculations but thinking about it again, it may be better to have more flows. I will create a v2. 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@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 >> >
On 7/15/21 3:54 PM, Mark Gray wrote: > On 15/07/2021 14:16, Mark Michelson wrote: >> Hi Mark, Hi Mark, 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? >> > > Yes, this is the case. It's a tradeoff between number of flows and > reirculations but thinking about it again, it may be better to have more > flows. I will create a v2. > Unless we match on proto *and* L4 port I don't think it's worth adding per proto flows. Assuming a TCP load balancer, all TCP traffic with destination VIP will still be ct_dnat()'ed, even if the TCP destination port is not the one defined in the load balancer VIP. On the other hand, using the same VIP for multiple ports is probably a common use case so if we add the L4 port to the match the number of logical flows might increase significantly. Regards, Dumitru
On 15/07/2021 15:29, Dumitru Ceara wrote: > On 7/15/21 3:54 PM, Mark Gray wrote: >> On 15/07/2021 14:16, Mark Michelson wrote: >>> Hi Mark, > > Hi Mark, 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? >>> >> >> Yes, this is the case. It's a tradeoff between number of flows and >> reirculations but thinking about it again, it may be better to have more >> flows. I will create a v2. >> > > Unless we match on proto *and* L4 port I don't think it's worth adding > per proto flows. Assuming a TCP load balancer, all TCP traffic with > destination VIP will still be ct_dnat()'ed, even if the TCP destination > port is not the one defined in the load balancer VIP. > > On the other hand, using the same VIP for multiple ports is probably a > common use case so if we add the L4 port to the match the number of > logical flows might increase significantly. I don't think we can match on L4 port AFAIK, this could cause misses with fragmented packets (which is the whole point of the defrag table). I guess it depends on the use case as that will determine the number of vips (which will generate a certain number of flows) and the traffic pattern (which will determine the average number of ct_dnat()s). In a system with a handful of VIPs for TCP traffic but mostly hosting UDP traffic, it may be better to do as Mark M. suggests. The number of flows may not be that high as we only add one flow per protocol rather than one per port. So I guess in the worst case, this could be 4x the number of load balancer VIPs associated with the logical router. > > Regards, > Dumitru >
On 7/15/21 7:04 PM, Mark Gray wrote: > On 15/07/2021 15:29, Dumitru Ceara wrote: >> On 7/15/21 3:54 PM, Mark Gray wrote: >>> On 15/07/2021 14:16, Mark Michelson wrote: >>>> Hi Mark, >> >> Hi Mark, 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? >>>> >>> >>> Yes, this is the case. It's a tradeoff between number of flows and >>> reirculations but thinking about it again, it may be better to have more >>> flows. I will create a v2. >>> >> >> Unless we match on proto *and* L4 port I don't think it's worth adding >> per proto flows. Assuming a TCP load balancer, all TCP traffic with >> destination VIP will still be ct_dnat()'ed, even if the TCP destination >> port is not the one defined in the load balancer VIP. >> >> On the other hand, using the same VIP for multiple ports is probably a >> common use case so if we add the L4 port to the match the number of >> logical flows might increase significantly. > > I don't think we can match on L4 port AFAIK, this could cause misses > with fragmented packets (which is the whole point of the defrag table). You're right, sorry about the noise. > > I guess it depends on the use case as that will determine the number of > vips (which will generate a certain number of flows) and the traffic > pattern (which will determine the average number of ct_dnat()s). In a > system with a handful of VIPs for TCP traffic but mostly hosting UDP > traffic, it may be better to do as Mark M. suggests. > > The number of flows may not be that high as we only add one flow per > protocol rather than one per port. So I guess in the worst case, this > could be 4x the number of load balancer VIPs associated with the logical > router. > Ok, it doesn't sound that bad indeed.
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
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@redhat.com> --- northd/ovn-northd.c | 8 -------- northd/ovn_northd.dl | 9 +-------- tests/ovn-northd.at | 46 ++++++++++++++++++++++---------------------- 3 files changed, 24 insertions(+), 39 deletions(-)