| Message ID | 20210716120702.1191569-1-mark.d.gray@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [ovs-dev,v3] northd: Fix defrag flows for duplicate vips | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
On Fri, Jul 16, 2021 at 8:07 AM Mark Gray <mark.d.gray@redhat.com> 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. > > Add flow to defrag table for every load-balancer in order to resolve this. Flows > for Load Balancers without a port specified are added with priority 100. Flows > for Load Balancers with a port specified are added with priority 110. > > Add a test to check behaviour of Logical Flows when two load balancers > of the same VIP are added. > > 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> > Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks Mark G for fixing this and thanks to Dumitru and Mark M for the reviews. I applied this patch to the main branch. The CI with this patch is green now - https://github.com/ovsrobot/ovn/runs/3086070038 :) Thanks Numan > --- > > Notes: > v2 - Address Mark M.'s comments > * Add flows to defrag table for every LB VIP and every LB VIP + proto > rather than just every unique LB VIP > * Change priority of flows in LB VIP + proto case in order to not clash > with flows in LB VIP case. > * Add additional tests. > v3 - Address Dumitru's comments > * Formatting > > northd/ovn-northd.8.xml | 35 +++++++++++----- > northd/ovn-northd.c | 64 ++++++++++++++-------------- > northd/ovn_northd.dl | 8 ++-- > tests/ovn-northd.at | 93 +++++++++++++++++++++++++++++++---------- > 4 files changed, 130 insertions(+), 70 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index a20c5b90dc66..6599ba194fe5 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -2773,17 +2773,32 @@ icmp6 { > </p> > > <p> > - If load balancing rules with virtual IP addresses (and ports) are > - configured in <code>OVN_Northbound</code> database for a Gateway router, > + If load balancing rules with only virtual IP addresses are configured in > + <code>OVN_Northbound</code> database for a Gateway router, > a priority-100 flow is added for each configured virtual IP address > - <var>VIP</var>. For IPv4 <var>VIPs</var> the flow matches <code>ip > - && ip4.dst == <var>VIP</var></code>. For IPv6 <var>VIPs</var>, > - the flow matches <code>ip && ip6.dst == <var>VIP</var></code>. > - The flow applies the action <code>reg0 = <var>VIP</var> > - && ct_dnat;</code> to send IP packets to the > - connection tracker for packet de-fragmentation and to dnat the > - destination IP for the committed connection before sending it to the > - next table. > + <var>VIP</var>. For IPv4 <var>VIPs</var> the flow matches > + <code>ip && ip4.dst == <var>VIP</var></code>. For IPv6 > + <var>VIPs</var>, the flow matches <code>ip && ip6.dst == > + <var>VIP</var></code>. The flow applies the action <code>reg0 = > + <var>VIP</var>; ct_dnat;</code> (or <code>xxreg0</code> for IPv6) to > + send IP packets to the connection tracker for packet de-fragmentation and > + to dnat the destination IP for the committed connection before sending it > + to the next table. > + </p> > + > + <p> > + If load balancing rules with virtual IP addresses and ports are > + configured in <code>OVN_Northbound</code> database for a Gateway router, > + a priority-110 flow is added for each configured virtual IP address > + <var>VIP</var> and protocol <var>PROTO</var>. For IPv4 <var>VIPs</var> > + the flow matches <code>ip && ip4.dst == <var>VIP</var> && > + <var>PROTO</var></code>. For IPv6 <var>VIPs</var>, the flow matches > + <code>ip && ip6.dst == <var>VIP</var> && > + <var>PROTO</var></code>. The flow applies the action <code>reg0 = > + <var>VIP</var>; ct_dnat;</code> (or <code>xxreg0</code> for IPv6) to send > + IP packets to the connection tracker for packet de-fragmentation and to > + dnat the destination IP for the committed connection before sending it to > + the next table. > </p> > > <p> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 999c3f482c29..45ad909e62b6 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -9207,8 +9207,6 @@ static void > build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, > struct hmap *lbs, struct ds *match) > { > - /* A set to hold all ips that need defragmentation and tracking. */ > - struct sset all_ips = SSET_INITIALIZER(&all_ips); > > for (int i = 0; i < od->nbr->n_load_balancer; i++) { > struct nbrec_load_balancer *nb_lb = od->nbr->load_balancer[i]; > @@ -9217,6 +9215,7 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, > ovs_assert(lb); > > for (size_t j = 0; j < lb->n_vips; j++) { > + int prio = 100; > struct ovn_lb_vip *lb_vip = &lb->vips[j]; > > bool is_udp = nullable_string_is_equal(nb_lb->protocol, "udp"); > @@ -9225,42 +9224,41 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, > 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); > - /* If there are any load balancing rules, we should send > - * the packet to conntrack for defragmentation and > - * tracking. This helps with two things. > - * > - * 1. With tracking, we can send only new connections to > - * 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. */ > - ds_clear(match); > - ds_clear(&defrag_actions); > - if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { > - ds_put_format(match, "ip && ip4.dst == %s", > - lb_vip->vip_str); > - ds_put_format(&defrag_actions, "reg0 = %s; ct_dnat;", > - lb_vip->vip_str); > - } else { > - ds_put_format(match, "ip && ip6.dst == %s", > - lb_vip->vip_str); > - ds_put_format(&defrag_actions, "xxreg0 = %s; ct_dnat;", > - 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), > - &nb_lb->header_); > + /* If there are any load balancing rules, we should send > + * the packet to conntrack for defragmentation and > + * tracking. This helps with two things. > + * > + * 1. With tracking, we can send only new connections to > + * 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. */ > + ds_clear(match); > + ds_clear(&defrag_actions); > + if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { > + ds_put_format(match, "ip && ip4.dst == %s", > + lb_vip->vip_str); > + ds_put_format(&defrag_actions, "reg0 = %s; ct_dnat;", > + lb_vip->vip_str); > + } else { > + ds_put_format(match, "ip && ip6.dst == %s", > + lb_vip->vip_str); > + ds_put_format(&defrag_actions, "xxreg0 = %s; ct_dnat;", > + lb_vip->vip_str); > } > + > + if (lb_vip->vip_port) { > + ds_put_format(match, " && %s", proto); > + prio = 110; > + } > + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DEFRAG, > + prio, ds_cstr(match), > + ds_cstr(&defrag_actions), > + &nb_lb->header_); > + > ds_destroy(&defrag_actions); > } > } > - sset_destroy(&all_ips); > } > > #define ND_RA_MAX_INTERVAL_MAX 1800 > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index ceeabe6f384e..dbf0e6ef1cdd 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -6168,11 +6168,11 @@ for (RouterLBVIP( > * 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 = > + (var prio, var match2) = > if (port != 0) { > - " && ${proto}" > + (110, " && ${proto}") > } else { > - "" > + (100, "") > } in > var __match = match1 ++ match2 in > var xx = ip_address.xxreg() in > @@ -6183,7 +6183,7 @@ for (RouterLBVIP( > * get merged by DDlog. */ > Flow(.logical_datapath = lr_uuid, > .stage = s_ROUTER_IN_DEFRAG(), > - .priority = 100, > + .priority = prio, > .__match = __match, > .actions = __actions, > .external_ids = stage_hint(lb._uuid)); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 11461d3f4c2a..4193b1a933b4 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), 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=110 , match=(ip && ip4.dst == 10.0.0.20 && tcp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip6.dst == def0::2 && tcp), action=(xxreg0 = def0::2; ct_dnat;) > ]) > > AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl > @@ -4652,5 +4652,52 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl > table=2 (lr_out_snat ), priority=33 , match=(ip && ip4.src == 10.0.0.3), action=(ct_snat(172.168.0.20);) > ]) > > +check ovn-nbctl lrp-del lr0-sw0 > +check ovn-nbctl lrp-del lr0-public > +check ovn-nbctl lr-lb-del lr0 > +check ovn-nbctl lr-nat-del lr0 > + > +check ovn-nbctl lb-add lb3 172.168.0.210:60 "10.0.0.50:6062,10.0.0.60:6062" udp > +check ovn-nbctl lb-add lb4 172.168.0.210:60 "10.0.0.50:6062,10.0.0.60:6062" tcp > +check ovn-nbctl lr-lb-add lr0 lb3 > +check ovn-nbctl lr-lb-add lr0 lb4 > +check ovn-nbctl --wait=sb sync > + > +ovn-sbctl dump-flows lr0 > lr0flows > +AT_CAPTURE_FILE([lr0flows]) > + > +AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl > + table=4 (lr_in_unsnat ), priority=0 , match=(1), action=(next;) > +]) > + > +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=110 , match=(ip && ip4.dst == 172.168.0.210 && tcp), action=(reg0 = 172.168.0.210; ct_dnat;) > + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) > +]) > + > +AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl > + table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;) > + table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip4 && reg0 == 172.168.0.210 && ct_label.natted == 1 && tcp), action=(flags.force_snat_for_lb = 1; next;) > + table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip4 && reg0 == 172.168.0.210 && ct_label.natted == 1 && udp), action=(flags.force_snat_for_lb = 1; next;) > + table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip4 && reg0 == 172.168.0.210 && tcp && tcp.dst == 60), action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.50:6062,10.0.0.60:6062);) > + table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip4 && reg0 == 172.168.0.210 && udp && udp.dst == 60), action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.50:6062,10.0.0.60:6062);) > +]) > + > +AT_CHECK([grep "lr_out_undnat" lr0flows | sort], [0], [dnl > + table=0 (lr_out_undnat ), priority=0 , match=(1), action=(next;) > + table=0 (lr_out_undnat ), priority=50 , match=(ip), action=(flags.loopback = 1; ct_dnat;) > +]) > + > +AT_CHECK([grep "lr_out_post_undnat" lr0flows | sort], [0], [dnl > + table=1 (lr_out_post_undnat ), priority=0 , match=(1), action=(next;) > + table=1 (lr_out_post_undnat ), priority=50 , match=(ip && ct.new), action=(ct_commit { } ; next; ) > +]) > + > +AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl > + table=2 (lr_out_snat ), priority=0 , match=(1), action=(next;) > + table=2 (lr_out_snat ), priority=120 , match=(nd_ns), action=(next;) > +]) > + > AT_CLEANUP > ]) > -- > 2.27.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index a20c5b90dc66..6599ba194fe5 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -2773,17 +2773,32 @@ icmp6 { </p> <p> - If load balancing rules with virtual IP addresses (and ports) are - configured in <code>OVN_Northbound</code> database for a Gateway router, + If load balancing rules with only virtual IP addresses are configured in + <code>OVN_Northbound</code> database for a Gateway router, a priority-100 flow is added for each configured virtual IP address - <var>VIP</var>. For IPv4 <var>VIPs</var> the flow matches <code>ip - && ip4.dst == <var>VIP</var></code>. For IPv6 <var>VIPs</var>, - the flow matches <code>ip && ip6.dst == <var>VIP</var></code>. - The flow applies the action <code>reg0 = <var>VIP</var> - && ct_dnat;</code> to send IP packets to the - connection tracker for packet de-fragmentation and to dnat the - destination IP for the committed connection before sending it to the - next table. + <var>VIP</var>. For IPv4 <var>VIPs</var> the flow matches + <code>ip && ip4.dst == <var>VIP</var></code>. For IPv6 + <var>VIPs</var>, the flow matches <code>ip && ip6.dst == + <var>VIP</var></code>. The flow applies the action <code>reg0 = + <var>VIP</var>; ct_dnat;</code> (or <code>xxreg0</code> for IPv6) to + send IP packets to the connection tracker for packet de-fragmentation and + to dnat the destination IP for the committed connection before sending it + to the next table. + </p> + + <p> + If load balancing rules with virtual IP addresses and ports are + configured in <code>OVN_Northbound</code> database for a Gateway router, + a priority-110 flow is added for each configured virtual IP address + <var>VIP</var> and protocol <var>PROTO</var>. For IPv4 <var>VIPs</var> + the flow matches <code>ip && ip4.dst == <var>VIP</var> && + <var>PROTO</var></code>. For IPv6 <var>VIPs</var>, the flow matches + <code>ip && ip6.dst == <var>VIP</var> && + <var>PROTO</var></code>. The flow applies the action <code>reg0 = + <var>VIP</var>; ct_dnat;</code> (or <code>xxreg0</code> for IPv6) to send + IP packets to the connection tracker for packet de-fragmentation and to + dnat the destination IP for the committed connection before sending it to + the next table. </p> <p> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 999c3f482c29..45ad909e62b6 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -9207,8 +9207,6 @@ static void build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, struct hmap *lbs, struct ds *match) { - /* A set to hold all ips that need defragmentation and tracking. */ - struct sset all_ips = SSET_INITIALIZER(&all_ips); for (int i = 0; i < od->nbr->n_load_balancer; i++) { struct nbrec_load_balancer *nb_lb = od->nbr->load_balancer[i]; @@ -9217,6 +9215,7 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, ovs_assert(lb); for (size_t j = 0; j < lb->n_vips; j++) { + int prio = 100; struct ovn_lb_vip *lb_vip = &lb->vips[j]; bool is_udp = nullable_string_is_equal(nb_lb->protocol, "udp"); @@ -9225,42 +9224,41 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, 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); - /* If there are any load balancing rules, we should send - * the packet to conntrack for defragmentation and - * tracking. This helps with two things. - * - * 1. With tracking, we can send only new connections to - * 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. */ - ds_clear(match); - ds_clear(&defrag_actions); - if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { - ds_put_format(match, "ip && ip4.dst == %s", - lb_vip->vip_str); - ds_put_format(&defrag_actions, "reg0 = %s; ct_dnat;", - lb_vip->vip_str); - } else { - ds_put_format(match, "ip && ip6.dst == %s", - lb_vip->vip_str); - ds_put_format(&defrag_actions, "xxreg0 = %s; ct_dnat;", - 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), - &nb_lb->header_); + /* If there are any load balancing rules, we should send + * the packet to conntrack for defragmentation and + * tracking. This helps with two things. + * + * 1. With tracking, we can send only new connections to + * 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. */ + ds_clear(match); + ds_clear(&defrag_actions); + if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { + ds_put_format(match, "ip && ip4.dst == %s", + lb_vip->vip_str); + ds_put_format(&defrag_actions, "reg0 = %s; ct_dnat;", + lb_vip->vip_str); + } else { + ds_put_format(match, "ip && ip6.dst == %s", + lb_vip->vip_str); + ds_put_format(&defrag_actions, "xxreg0 = %s; ct_dnat;", + lb_vip->vip_str); } + + if (lb_vip->vip_port) { + ds_put_format(match, " && %s", proto); + prio = 110; + } + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DEFRAG, + prio, ds_cstr(match), + ds_cstr(&defrag_actions), + &nb_lb->header_); + ds_destroy(&defrag_actions); } } - sset_destroy(&all_ips); } #define ND_RA_MAX_INTERVAL_MAX 1800 diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index ceeabe6f384e..dbf0e6ef1cdd 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -6168,11 +6168,11 @@ for (RouterLBVIP( * 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 = + (var prio, var match2) = if (port != 0) { - " && ${proto}" + (110, " && ${proto}") } else { - "" + (100, "") } in var __match = match1 ++ match2 in var xx = ip_address.xxreg() in @@ -6183,7 +6183,7 @@ for (RouterLBVIP( * get merged by DDlog. */ Flow(.logical_datapath = lr_uuid, .stage = s_ROUTER_IN_DEFRAG(), - .priority = 100, + .priority = prio, .__match = __match, .actions = __actions, .external_ids = stage_hint(lb._uuid)); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 11461d3f4c2a..4193b1a933b4 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), 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=110 , match=(ip && ip4.dst == 10.0.0.20 && tcp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), 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 == 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=110 , match=(ip && ip4.dst == 10.0.0.10 && tcp), action=(reg0 = 10.0.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.10 && tcp), action=(reg0 = 172.168.0.10; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.100 && tcp), action=(reg0 = 172.168.0.100; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip6.dst == def0::2 && tcp), action=(xxreg0 = def0::2; ct_dnat;) ]) AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl @@ -4652,5 +4652,52 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl table=2 (lr_out_snat ), priority=33 , match=(ip && ip4.src == 10.0.0.3), action=(ct_snat(172.168.0.20);) ]) +check ovn-nbctl lrp-del lr0-sw0 +check ovn-nbctl lrp-del lr0-public +check ovn-nbctl lr-lb-del lr0 +check ovn-nbctl lr-nat-del lr0 + +check ovn-nbctl lb-add lb3 172.168.0.210:60 "10.0.0.50:6062,10.0.0.60:6062" udp +check ovn-nbctl lb-add lb4 172.168.0.210:60 "10.0.0.50:6062,10.0.0.60:6062" tcp +check ovn-nbctl lr-lb-add lr0 lb3 +check ovn-nbctl lr-lb-add lr0 lb4 +check ovn-nbctl --wait=sb sync + +ovn-sbctl dump-flows lr0 > lr0flows +AT_CAPTURE_FILE([lr0flows]) + +AT_CHECK([grep "lr_in_unsnat" lr0flows | sort], [0], [dnl + table=4 (lr_in_unsnat ), priority=0 , match=(1), action=(next;) +]) + +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=110 , match=(ip && ip4.dst == 172.168.0.210 && tcp), action=(reg0 = 172.168.0.210; ct_dnat;) + table=5 (lr_in_defrag ), priority=110 , match=(ip && ip4.dst == 172.168.0.210 && udp), action=(reg0 = 172.168.0.210; ct_dnat;) +]) + +AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0], [dnl + table=6 (lr_in_dnat ), priority=0 , match=(1), action=(next;) + table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip4 && reg0 == 172.168.0.210 && ct_label.natted == 1 && tcp), action=(flags.force_snat_for_lb = 1; next;) + table=6 (lr_in_dnat ), priority=120 , match=(ct.est && ip4 && reg0 == 172.168.0.210 && ct_label.natted == 1 && udp), action=(flags.force_snat_for_lb = 1; next;) + table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip4 && reg0 == 172.168.0.210 && tcp && tcp.dst == 60), action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.50:6062,10.0.0.60:6062);) + table=6 (lr_in_dnat ), priority=120 , match=(ct.new && ip4 && reg0 == 172.168.0.210 && udp && udp.dst == 60), action=(flags.force_snat_for_lb = 1; ct_lb(backends=10.0.0.50:6062,10.0.0.60:6062);) +]) + +AT_CHECK([grep "lr_out_undnat" lr0flows | sort], [0], [dnl + table=0 (lr_out_undnat ), priority=0 , match=(1), action=(next;) + table=0 (lr_out_undnat ), priority=50 , match=(ip), action=(flags.loopback = 1; ct_dnat;) +]) + +AT_CHECK([grep "lr_out_post_undnat" lr0flows | sort], [0], [dnl + table=1 (lr_out_post_undnat ), priority=0 , match=(1), action=(next;) + table=1 (lr_out_post_undnat ), priority=50 , match=(ip && ct.new), action=(ct_commit { } ; next; ) +]) + +AT_CHECK([grep "lr_out_snat" lr0flows | sort], [0], [dnl + table=2 (lr_out_snat ), priority=0 , match=(1), action=(next;) + table=2 (lr_out_snat ), priority=120 , match=(nd_ns), action=(next;) +]) + AT_CLEANUP ])
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. Add flow to defrag table for every load-balancer in order to resolve this. Flows for Load Balancers without a port specified are added with priority 100. Flows for Load Balancers with a port specified are added with priority 110. Add a test to check behaviour of Logical Flows when two load balancers of the same VIP are added. 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> Acked-by: Dumitru Ceara <dceara@redhat.com> --- Notes: v2 - Address Mark M.'s comments * Add flows to defrag table for every LB VIP and every LB VIP + proto rather than just every unique LB VIP * Change priority of flows in LB VIP + proto case in order to not clash with flows in LB VIP case. * Add additional tests. v3 - Address Dumitru's comments * Formatting northd/ovn-northd.8.xml | 35 +++++++++++----- northd/ovn-northd.c | 64 ++++++++++++++-------------- northd/ovn_northd.dl | 8 ++-- tests/ovn-northd.at | 93 +++++++++++++++++++++++++++++++---------- 4 files changed, 130 insertions(+), 70 deletions(-)