| Message ID | 4c0ba5a650c5feb4b15086e57703233dbbb79644.1625479247.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | northd: rework ovn-northd lb flow installation | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | fail | github build: failed |
On 7/5/21 12:05 PM, Lorenzo Bianconi wrote: > Remove add_router_lb_flow routine and move leftover lb flow > installation code in build_lrouter_snat_flows_for_lb routine > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- Hi Lorenzo, > northd/ovn-northd.c | 328 +++++++++++++++++++++----------------------- > 1 file changed, 157 insertions(+), 171 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 7d8033725..6f5c84852 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -656,6 +656,9 @@ struct ovn_datapath { > /* NAT entries configured on the router. */ > struct ovn_nat *nat_entries; > > + /* Set of nat external ips on the router. */ > + struct sset external_ips; > + > /* SNAT IPs owned by the router (shash of 'struct ovn_snat_ip'). */ > struct shash snat_ips; > > @@ -830,6 +833,37 @@ destroy_nat_entries(struct ovn_datapath *od) > } > } > > +static void > +init_router_enternal_ips(struct ovn_datapath *od) Typo: enternal > +{ > + if (!od->nbr) { > + return; > + } > + > + sset_init(&od->external_ips); > + > + for (size_t i = 0; i < od->nbr->n_nat; i++) { > + const struct nbrec_nat *nat = od->nbr->nat[i]; > + > + if (od->l3dgw_port) { > + if (!sset_contains(&od->external_ips, nat->external_ip)) { > + sset_add(&od->external_ips, nat->external_ip); > + } > + } else { > + sset_add(&od->external_ips, nat->external_ip); > + } No need for the "if" above. We can just do: sset_add(&od->external_ips, nat->external_ip); > + } > +} > + > +static void destroy_router_enternal_ips(struct ovn_datapath *od) Typo: enternal > +{ > + if (!od->nbr) { > + return; > + } > + > + sset_destroy(&od->external_ips); > +} > + > static void > init_lb_ips(struct ovn_datapath *od) > { > @@ -892,6 +926,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od) > destroy_ipam_info(&od->ipam_info); > free(od->router_ports); > destroy_nat_entries(od); > + destroy_router_enternal_ips(od); > destroy_lb_ips(od); > free(od->nat_entries); > free(od->localnet_ports); > @@ -1247,6 +1282,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, > } > init_mcast_info_for_datapath(od); > init_nat_entries(od); > + init_router_enternal_ips(od); > init_lb_ips(od); > ovs_list_push_back(lr_list, &od->lr_list); > } > @@ -8768,92 +8804,6 @@ enum lb_snat_type { > SKIP_SNAT, > }; > > -static void > -add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od, > - enum lb_snat_type snat_type, struct ovn_lb_vip *lb_vip, > - const char *proto, struct nbrec_load_balancer *lb, > - struct sset *nat_entries) > -{ > - const char *ip_match = NULL; > - if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { > - ip_match = "ip4"; > - } else { > - ip_match = "ip6"; > - } > - > - if (sset_contains(nat_entries, lb_vip->vip_str)) { > - /* The load balancer vip is also present in the NAT entries. > - * So add a high priority lflow to advance the the packet > - * destined to the vip (and the vip port if defined) > - * in the S_ROUTER_IN_UNSNAT stage. > - * There seems to be an issue with ovs-vswitchd. When the new > - * connection packet destined for the lb vip is received, > - * it is dnat'ed in the S_ROUTER_IN_DNAT stage in the dnat > - * conntrack zone. For the next packet, if it goes through > - * unsnat stage, the conntrack flags are not set properly, and > - * it doesn't hit the established state flows in > - * S_ROUTER_IN_DNAT stage. */ > - struct ds unsnat_match = DS_EMPTY_INITIALIZER; > - ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s", > - ip_match, ip_match, lb_vip->vip_str, proto); > - if (lb_vip->vip_port) { > - ds_put_format(&unsnat_match, " && %s.dst == %d", proto, > - lb_vip->vip_port); > - } > - > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, 120, > - ds_cstr(&unsnat_match), "next;", &lb->header_); > - > - ds_destroy(&unsnat_match); > - } > - > - if (!od->l3dgw_port || !od->l3redirect_port || !lb_vip->n_backends) { > - return; > - } > - > - /* Add logical flows to UNDNAT the load balanced reverse traffic in > - * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the logical > - * router has a gateway router port associated. > - */ > - struct ds undnat_match = DS_EMPTY_INITIALIZER; > - ds_put_format(&undnat_match, "%s && (", ip_match); > - > - for (size_t i = 0; i < lb_vip->n_backends; i++) { > - struct ovn_lb_backend *backend = &lb_vip->backends[i]; > - ds_put_format(&undnat_match, "(%s.src == %s", ip_match, > - backend->ip_str); > - > - if (backend->port) { > - ds_put_format(&undnat_match, " && %s.src == %d) || ", > - proto, backend->port); > - } else { > - ds_put_cstr(&undnat_match, ") || "); > - } > - } > - > - ds_chomp(&undnat_match, ' '); > - ds_chomp(&undnat_match, '|'); > - ds_chomp(&undnat_match, '|'); > - ds_chomp(&undnat_match, ' '); > - ds_put_format(&undnat_match, ") && outport == %s && " > - "is_chassis_resident(%s)", od->l3dgw_port->json_key, > - od->l3redirect_port->json_key); > - if (snat_type == FORCE_SNAT || snat_type == SKIP_SNAT) { > - char *action = xasprintf("flags.%s_snat_for_lb = 1; ct_dnat;", > - snat_type == SKIP_SNAT ? "skip" : "force"); > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, > - ds_cstr(&undnat_match), action, > - &lb->header_); > - free(action); > - } else { > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, > - ds_cstr(&undnat_match), "ct_dnat;", > - &lb->header_); > - } > - > - ds_destroy(&undnat_match); > -} > - > static void > build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > struct ovn_northd_lb *lb, > @@ -8905,10 +8855,67 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > new_match = xasprintf("ct.new && %s", ds_cstr(match)); > est_match = xasprintf("ct.est && %s", ds_cstr(match)); > > + const char *ip_match = NULL; > + if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { > + ip_match = "ip4"; > + } else { > + ip_match = "ip6"; > + } > + > + /* Add logical flows to UNDNAT the load balanced reverse traffic in > + * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the logical > + * router has a gateway router port associated. > + */ > + struct ds undnat_match = DS_EMPTY_INITIALIZER; > + ds_put_format(&undnat_match, "%s && (", ip_match); > + > + for (size_t i = 0; i < lb_vip->n_backends; i++) { > + struct ovn_lb_backend *backend = &lb_vip->backends[i]; > + ds_put_format(&undnat_match, "(%s.src == %s", ip_match, > + backend->ip_str); > + > + if (backend->port) { > + ds_put_format(&undnat_match, " && %s.src == %d) || ", > + proto, backend->port); > + } else { > + ds_put_cstr(&undnat_match, ") || "); > + } > + } > + ds_chomp(&undnat_match, ' '); > + ds_chomp(&undnat_match, '|'); > + ds_chomp(&undnat_match, '|'); > + ds_chomp(&undnat_match, ' '); > + > + struct ds unsnat_match = DS_EMPTY_INITIALIZER; > + ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s", > + ip_match, ip_match, lb_vip->vip_str, proto); > + if (lb_vip->vip_port) { > + ds_put_format(&unsnat_match, " && %s.dst == %d", proto, > + lb_vip->vip_port); > + } > + > for (size_t i = 0; i < lb->n_nb_lr; i++) { > struct ovn_datapath *od = lb->nb_lr[i]; > char *new_match_p = new_match; > char *est_match_p = est_match; > + char *est_actions = NULL; > + > + if (sset_contains(&od->external_ips, lb_vip->vip_str)) { > + /* The load balancer vip is also present in the NAT entries. > + * So add a high priority lflow to advance the the packet > + * destined to the vip (and the vip port if defined) > + * in the S_ROUTER_IN_UNSNAT stage. > + * There seems to be an issue with ovs-vswitchd. When the new > + * connection packet destined for the lb vip is received, > + * it is dnat'ed in the S_ROUTER_IN_DNAT stage in the dnat > + * conntrack zone. For the next packet, if it goes through > + * unsnat stage, the conntrack flags are not set properly, and > + * it doesn't hit the established state flows in > + * S_ROUTER_IN_DNAT stage. */ > + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, 120, > + ds_cstr(&unsnat_match), "next;", > + &lb->nlb->header_); > + } > > if (od->l3redirect_port && > (lb_vip->n_backends || !lb_vip->empty_backend_rej)) { > @@ -8941,12 +8948,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > &lb->nlb->header_); > free(new_actions); > > - char *est_actions = xasprintf("flags.force_snat_for_lb = 1; " > - "ct_dnat;"); > + est_actions = xasprintf("flags.force_snat_for_lb = 1; " > + "ct_dnat;"); > ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio, > est_match_p, est_actions, > &lb->nlb->header_); > - free(est_actions); > } else { > ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio, > new_match_p, ds_cstr(action), > @@ -8962,8 +8968,37 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > if (est_match_p != est_match) { > free(est_match_p); > } > + > + if (!od->l3dgw_port || !od->l3redirect_port || !lb_vip->n_backends) { > + goto next; > + } > + > + char *undnat_match_p = xasprintf("%s) && outport == %s && " > + "is_chassis_resident(%s)", > + ds_cstr(&undnat_match), > + od->l3dgw_port->json_key, > + od->l3redirect_port->json_key); > + if (snat_type == SKIP_SNAT) { > + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, > + undnat_match_p, skip_snat_est_action, > + &lb->nlb->header_); > + } else if (snat_type == FORCE_SNAT) { > + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, > + undnat_match_p, est_actions, > + &lb->nlb->header_); > + } else { > + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, > + undnat_match_p, "ct_dnat;", > + &lb->nlb->header_); > + } > + free(undnat_match_p); > +next: > + free(est_actions); > } > > + ds_destroy(&unsnat_match); > + ds_destroy(&undnat_match); > + > free(skip_snat_new_action); > free(skip_snat_est_action); > free(est_match); > @@ -8972,8 +9007,8 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > > static void > build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows, > - struct shash *meter_groups, struct ds *match, > - struct ds *action) > + struct shash *meter_groups, > + struct ds *match, struct ds *action) > { > if (!lb->n_nb_lr) { > return; > @@ -8982,8 +9017,8 @@ build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows, > for (size_t i = 0; i < lb->n_vips; i++) { > struct ovn_lb_vip *lb_vip = &lb->vips[i]; > > - build_lrouter_nat_flows_for_lb(lb_vip, lb, &lb->vips_nb[i], lflows, > - match, action); > + build_lrouter_nat_flows_for_lb(lb_vip, lb, &lb->vips_nb[i], > + lflows, match, action); > > if (!build_empty_lb_event_flow(lb_vip, lb->nlb, meter_groups, > match, action)) { > @@ -8995,17 +9030,21 @@ build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows, > &lb->nlb->header_); > } > } > + > + if (smap_get_bool(&lb->nlb->options, "skip_snat", false)) { > + for (size_t i = 0; i < lb->n_nb_lr; i++) { > + ovn_lflow_add(lflows, lb->nb_lr[i], S_ROUTER_OUT_SNAT, 120, > + "flags.skip_snat_for_lb == 1 && ip", "next;"); > + } > + } > } > > static void > build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, > - struct hmap *lbs, struct sset *nat_entries, > - struct ds *match) > + 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); > - bool lb_force_snat_ip = > - !lport_addresses_is_empty(&od->lb_force_snat_addrs); > > for (int i = 0; i < od->nbr->n_load_balancer; i++) { > struct nbrec_load_balancer *nb_lb = od->nbr->load_balancer[i]; > @@ -9013,15 +9052,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, > ovn_northd_lb_find(lbs, &nb_lb->header_.uuid); > ovs_assert(lb); > > - enum lb_snat_type snat_type = NO_FORCE_SNAT; > - if (smap_get_bool(&nb_lb->options, "skip_snat", false)) { > - ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 120, > - "flags.skip_snat_for_lb == 1 && ip", "next;"); > - snat_type = SKIP_SNAT; > - } else if (lb_force_snat_ip || od->lb_force_snat_router_ip) { > - snat_type = FORCE_SNAT; > - } > - > for (size_t j = 0; j < lb->n_vips; j++) { > struct ovn_lb_vip *lb_vip = &lb->vips[j]; > > @@ -9047,38 +9077,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, > 100, ds_cstr(match), "ct_next;", > &nb_lb->header_); > } > - > - /* Higher priority rules are added for load-balancing in DNAT > - * table. For every match (on a VIP[:port]), we add two flows > - * via add_router_lb_flow(). One flow is for specific matching > - * on ct.new with an action of "ct_lb($targets);". The other > - * flow is for ct.est with an action of "ct_dnat;". */ > - ds_clear(match); > - if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { > - ds_put_format(match, "ip && ip4.dst == %s", > - lb_vip->vip_str); > - } else { > - ds_put_format(match, "ip && ip6.dst == %s", > - lb_vip->vip_str); > - } > - > - 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"; > - > - if (lb_vip->vip_port) { > - ds_put_format(match, " && %s && %s.dst == %d", proto, > - proto, lb_vip->vip_port); > - } > - > - if (od->l3redirect_port && > - (lb_vip->n_backends || !lb_vip->empty_backend_rej)) { > - ds_put_format(match, " && is_chassis_resident(%s)", > - od->l3redirect_port->json_key); > - } > - add_router_lb_flow(lflows, od, snat_type, lb_vip, proto, nb_lb, > - nat_entries); > } > } > sset_destroy(&all_ips); > @@ -11854,8 +11852,6 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, > return; > } > > - struct sset nat_entries = SSET_INITIALIZER(&nat_entries); > - > bool dnat_force_snat_ip = > !lport_addresses_is_empty(&od->dnat_force_snat_addrs); > bool lb_force_snat_ip = > @@ -11881,31 +11877,24 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, > mask, is_v6); > > /* ARP resolve for NAT IPs. */ > - if (od->l3dgw_port) { > - if (!sset_contains(&nat_entries, nat->external_ip)) { > - ds_clear(match); > - ds_put_format( > - match, "outport == %s && %s == %s", > - od->l3dgw_port->json_key, > - is_v6 ? REG_NEXT_HOP_IPV6 : REG_NEXT_HOP_IPV4, > - nat->external_ip); > - ds_clear(actions); > - ds_put_format( > - actions, "eth.dst = %s; next;", > - distributed ? nat->external_mac : > - od->l3dgw_port->lrp_networks.ea_s); > - ovn_lflow_add_with_hint(lflows, od, > - S_ROUTER_IN_ARP_RESOLVE, > - 100, ds_cstr(match), > - ds_cstr(actions), > - &nat->header_); > - sset_add(&nat_entries, nat->external_ip); > - } > - } else { > - /* Add the NAT external_ip to the nat_entries even for > - * gateway routers. This is required for adding load balancer > - * flows.*/ > - sset_add(&nat_entries, nat->external_ip); > + if (od->l3dgw_port && > + !sset_contains(&od->external_ips, nat->external_ip)) { > + ds_clear(match); > + ds_put_format( > + match, "outport == %s && %s == %s", > + od->l3dgw_port->json_key, > + is_v6 ? REG_NEXT_HOP_IPV6 : REG_NEXT_HOP_IPV4, > + nat->external_ip); > + ds_clear(actions); > + ds_put_format( > + actions, "eth.dst = %s; next;", > + distributed ? nat->external_mac : > + od->l3dgw_port->lrp_networks.ea_s); > + ovn_lflow_add_with_hint(lflows, od, > + S_ROUTER_IN_ARP_RESOLVE, > + 100, ds_cstr(match), > + ds_cstr(actions), > + &nat->header_); This part is not correct now; it will never add flows because the external_ip will always be in od->external_ips. I think we need to revert this section and use a local sset for nat flows. Thanks, Dumitru
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 7d8033725..6f5c84852 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -656,6 +656,9 @@ struct ovn_datapath { /* NAT entries configured on the router. */ struct ovn_nat *nat_entries; + /* Set of nat external ips on the router. */ + struct sset external_ips; + /* SNAT IPs owned by the router (shash of 'struct ovn_snat_ip'). */ struct shash snat_ips; @@ -830,6 +833,37 @@ destroy_nat_entries(struct ovn_datapath *od) } } +static void +init_router_enternal_ips(struct ovn_datapath *od) +{ + if (!od->nbr) { + return; + } + + sset_init(&od->external_ips); + + for (size_t i = 0; i < od->nbr->n_nat; i++) { + const struct nbrec_nat *nat = od->nbr->nat[i]; + + if (od->l3dgw_port) { + if (!sset_contains(&od->external_ips, nat->external_ip)) { + sset_add(&od->external_ips, nat->external_ip); + } + } else { + sset_add(&od->external_ips, nat->external_ip); + } + } +} + +static void destroy_router_enternal_ips(struct ovn_datapath *od) +{ + if (!od->nbr) { + return; + } + + sset_destroy(&od->external_ips); +} + static void init_lb_ips(struct ovn_datapath *od) { @@ -892,6 +926,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od) destroy_ipam_info(&od->ipam_info); free(od->router_ports); destroy_nat_entries(od); + destroy_router_enternal_ips(od); destroy_lb_ips(od); free(od->nat_entries); free(od->localnet_ports); @@ -1247,6 +1282,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, } init_mcast_info_for_datapath(od); init_nat_entries(od); + init_router_enternal_ips(od); init_lb_ips(od); ovs_list_push_back(lr_list, &od->lr_list); } @@ -8768,92 +8804,6 @@ enum lb_snat_type { SKIP_SNAT, }; -static void -add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od, - enum lb_snat_type snat_type, struct ovn_lb_vip *lb_vip, - const char *proto, struct nbrec_load_balancer *lb, - struct sset *nat_entries) -{ - const char *ip_match = NULL; - if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { - ip_match = "ip4"; - } else { - ip_match = "ip6"; - } - - if (sset_contains(nat_entries, lb_vip->vip_str)) { - /* The load balancer vip is also present in the NAT entries. - * So add a high priority lflow to advance the the packet - * destined to the vip (and the vip port if defined) - * in the S_ROUTER_IN_UNSNAT stage. - * There seems to be an issue with ovs-vswitchd. When the new - * connection packet destined for the lb vip is received, - * it is dnat'ed in the S_ROUTER_IN_DNAT stage in the dnat - * conntrack zone. For the next packet, if it goes through - * unsnat stage, the conntrack flags are not set properly, and - * it doesn't hit the established state flows in - * S_ROUTER_IN_DNAT stage. */ - struct ds unsnat_match = DS_EMPTY_INITIALIZER; - ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s", - ip_match, ip_match, lb_vip->vip_str, proto); - if (lb_vip->vip_port) { - ds_put_format(&unsnat_match, " && %s.dst == %d", proto, - lb_vip->vip_port); - } - - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, 120, - ds_cstr(&unsnat_match), "next;", &lb->header_); - - ds_destroy(&unsnat_match); - } - - if (!od->l3dgw_port || !od->l3redirect_port || !lb_vip->n_backends) { - return; - } - - /* Add logical flows to UNDNAT the load balanced reverse traffic in - * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the logical - * router has a gateway router port associated. - */ - struct ds undnat_match = DS_EMPTY_INITIALIZER; - ds_put_format(&undnat_match, "%s && (", ip_match); - - for (size_t i = 0; i < lb_vip->n_backends; i++) { - struct ovn_lb_backend *backend = &lb_vip->backends[i]; - ds_put_format(&undnat_match, "(%s.src == %s", ip_match, - backend->ip_str); - - if (backend->port) { - ds_put_format(&undnat_match, " && %s.src == %d) || ", - proto, backend->port); - } else { - ds_put_cstr(&undnat_match, ") || "); - } - } - - ds_chomp(&undnat_match, ' '); - ds_chomp(&undnat_match, '|'); - ds_chomp(&undnat_match, '|'); - ds_chomp(&undnat_match, ' '); - ds_put_format(&undnat_match, ") && outport == %s && " - "is_chassis_resident(%s)", od->l3dgw_port->json_key, - od->l3redirect_port->json_key); - if (snat_type == FORCE_SNAT || snat_type == SKIP_SNAT) { - char *action = xasprintf("flags.%s_snat_for_lb = 1; ct_dnat;", - snat_type == SKIP_SNAT ? "skip" : "force"); - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, - ds_cstr(&undnat_match), action, - &lb->header_); - free(action); - } else { - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, - ds_cstr(&undnat_match), "ct_dnat;", - &lb->header_); - } - - ds_destroy(&undnat_match); -} - static void build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, struct ovn_northd_lb *lb, @@ -8905,10 +8855,67 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, new_match = xasprintf("ct.new && %s", ds_cstr(match)); est_match = xasprintf("ct.est && %s", ds_cstr(match)); + const char *ip_match = NULL; + if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { + ip_match = "ip4"; + } else { + ip_match = "ip6"; + } + + /* Add logical flows to UNDNAT the load balanced reverse traffic in + * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the logical + * router has a gateway router port associated. + */ + struct ds undnat_match = DS_EMPTY_INITIALIZER; + ds_put_format(&undnat_match, "%s && (", ip_match); + + for (size_t i = 0; i < lb_vip->n_backends; i++) { + struct ovn_lb_backend *backend = &lb_vip->backends[i]; + ds_put_format(&undnat_match, "(%s.src == %s", ip_match, + backend->ip_str); + + if (backend->port) { + ds_put_format(&undnat_match, " && %s.src == %d) || ", + proto, backend->port); + } else { + ds_put_cstr(&undnat_match, ") || "); + } + } + ds_chomp(&undnat_match, ' '); + ds_chomp(&undnat_match, '|'); + ds_chomp(&undnat_match, '|'); + ds_chomp(&undnat_match, ' '); + + struct ds unsnat_match = DS_EMPTY_INITIALIZER; + ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s", + ip_match, ip_match, lb_vip->vip_str, proto); + if (lb_vip->vip_port) { + ds_put_format(&unsnat_match, " && %s.dst == %d", proto, + lb_vip->vip_port); + } + for (size_t i = 0; i < lb->n_nb_lr; i++) { struct ovn_datapath *od = lb->nb_lr[i]; char *new_match_p = new_match; char *est_match_p = est_match; + char *est_actions = NULL; + + if (sset_contains(&od->external_ips, lb_vip->vip_str)) { + /* The load balancer vip is also present in the NAT entries. + * So add a high priority lflow to advance the the packet + * destined to the vip (and the vip port if defined) + * in the S_ROUTER_IN_UNSNAT stage. + * There seems to be an issue with ovs-vswitchd. When the new + * connection packet destined for the lb vip is received, + * it is dnat'ed in the S_ROUTER_IN_DNAT stage in the dnat + * conntrack zone. For the next packet, if it goes through + * unsnat stage, the conntrack flags are not set properly, and + * it doesn't hit the established state flows in + * S_ROUTER_IN_DNAT stage. */ + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, 120, + ds_cstr(&unsnat_match), "next;", + &lb->nlb->header_); + } if (od->l3redirect_port && (lb_vip->n_backends || !lb_vip->empty_backend_rej)) { @@ -8941,12 +8948,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, &lb->nlb->header_); free(new_actions); - char *est_actions = xasprintf("flags.force_snat_for_lb = 1; " - "ct_dnat;"); + est_actions = xasprintf("flags.force_snat_for_lb = 1; " + "ct_dnat;"); ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio, est_match_p, est_actions, &lb->nlb->header_); - free(est_actions); } else { ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio, new_match_p, ds_cstr(action), @@ -8962,8 +8968,37 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, if (est_match_p != est_match) { free(est_match_p); } + + if (!od->l3dgw_port || !od->l3redirect_port || !lb_vip->n_backends) { + goto next; + } + + char *undnat_match_p = xasprintf("%s) && outport == %s && " + "is_chassis_resident(%s)", + ds_cstr(&undnat_match), + od->l3dgw_port->json_key, + od->l3redirect_port->json_key); + if (snat_type == SKIP_SNAT) { + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, + undnat_match_p, skip_snat_est_action, + &lb->nlb->header_); + } else if (snat_type == FORCE_SNAT) { + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, + undnat_match_p, est_actions, + &lb->nlb->header_); + } else { + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, + undnat_match_p, "ct_dnat;", + &lb->nlb->header_); + } + free(undnat_match_p); +next: + free(est_actions); } + ds_destroy(&unsnat_match); + ds_destroy(&undnat_match); + free(skip_snat_new_action); free(skip_snat_est_action); free(est_match); @@ -8972,8 +9007,8 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, static void build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows, - struct shash *meter_groups, struct ds *match, - struct ds *action) + struct shash *meter_groups, + struct ds *match, struct ds *action) { if (!lb->n_nb_lr) { return; @@ -8982,8 +9017,8 @@ build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows, for (size_t i = 0; i < lb->n_vips; i++) { struct ovn_lb_vip *lb_vip = &lb->vips[i]; - build_lrouter_nat_flows_for_lb(lb_vip, lb, &lb->vips_nb[i], lflows, - match, action); + build_lrouter_nat_flows_for_lb(lb_vip, lb, &lb->vips_nb[i], + lflows, match, action); if (!build_empty_lb_event_flow(lb_vip, lb->nlb, meter_groups, match, action)) { @@ -8995,17 +9030,21 @@ build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows, &lb->nlb->header_); } } + + if (smap_get_bool(&lb->nlb->options, "skip_snat", false)) { + for (size_t i = 0; i < lb->n_nb_lr; i++) { + ovn_lflow_add(lflows, lb->nb_lr[i], S_ROUTER_OUT_SNAT, 120, + "flags.skip_snat_for_lb == 1 && ip", "next;"); + } + } } static void build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, - struct hmap *lbs, struct sset *nat_entries, - struct ds *match) + 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); - bool lb_force_snat_ip = - !lport_addresses_is_empty(&od->lb_force_snat_addrs); for (int i = 0; i < od->nbr->n_load_balancer; i++) { struct nbrec_load_balancer *nb_lb = od->nbr->load_balancer[i]; @@ -9013,15 +9052,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, ovn_northd_lb_find(lbs, &nb_lb->header_.uuid); ovs_assert(lb); - enum lb_snat_type snat_type = NO_FORCE_SNAT; - if (smap_get_bool(&nb_lb->options, "skip_snat", false)) { - ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 120, - "flags.skip_snat_for_lb == 1 && ip", "next;"); - snat_type = SKIP_SNAT; - } else if (lb_force_snat_ip || od->lb_force_snat_router_ip) { - snat_type = FORCE_SNAT; - } - for (size_t j = 0; j < lb->n_vips; j++) { struct ovn_lb_vip *lb_vip = &lb->vips[j]; @@ -9047,38 +9077,6 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od, 100, ds_cstr(match), "ct_next;", &nb_lb->header_); } - - /* Higher priority rules are added for load-balancing in DNAT - * table. For every match (on a VIP[:port]), we add two flows - * via add_router_lb_flow(). One flow is for specific matching - * on ct.new with an action of "ct_lb($targets);". The other - * flow is for ct.est with an action of "ct_dnat;". */ - ds_clear(match); - if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { - ds_put_format(match, "ip && ip4.dst == %s", - lb_vip->vip_str); - } else { - ds_put_format(match, "ip && ip6.dst == %s", - lb_vip->vip_str); - } - - 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"; - - if (lb_vip->vip_port) { - ds_put_format(match, " && %s && %s.dst == %d", proto, - proto, lb_vip->vip_port); - } - - if (od->l3redirect_port && - (lb_vip->n_backends || !lb_vip->empty_backend_rej)) { - ds_put_format(match, " && is_chassis_resident(%s)", - od->l3redirect_port->json_key); - } - add_router_lb_flow(lflows, od, snat_type, lb_vip, proto, nb_lb, - nat_entries); } } sset_destroy(&all_ips); @@ -11854,8 +11852,6 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, return; } - struct sset nat_entries = SSET_INITIALIZER(&nat_entries); - bool dnat_force_snat_ip = !lport_addresses_is_empty(&od->dnat_force_snat_addrs); bool lb_force_snat_ip = @@ -11881,31 +11877,24 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, mask, is_v6); /* ARP resolve for NAT IPs. */ - if (od->l3dgw_port) { - if (!sset_contains(&nat_entries, nat->external_ip)) { - ds_clear(match); - ds_put_format( - match, "outport == %s && %s == %s", - od->l3dgw_port->json_key, - is_v6 ? REG_NEXT_HOP_IPV6 : REG_NEXT_HOP_IPV4, - nat->external_ip); - ds_clear(actions); - ds_put_format( - actions, "eth.dst = %s; next;", - distributed ? nat->external_mac : - od->l3dgw_port->lrp_networks.ea_s); - ovn_lflow_add_with_hint(lflows, od, - S_ROUTER_IN_ARP_RESOLVE, - 100, ds_cstr(match), - ds_cstr(actions), - &nat->header_); - sset_add(&nat_entries, nat->external_ip); - } - } else { - /* Add the NAT external_ip to the nat_entries even for - * gateway routers. This is required for adding load balancer - * flows.*/ - sset_add(&nat_entries, nat->external_ip); + if (od->l3dgw_port && + !sset_contains(&od->external_ips, nat->external_ip)) { + ds_clear(match); + ds_put_format( + match, "outport == %s && %s == %s", + od->l3dgw_port->json_key, + is_v6 ? REG_NEXT_HOP_IPV6 : REG_NEXT_HOP_IPV4, + nat->external_ip); + ds_clear(actions); + ds_put_format( + actions, "eth.dst = %s; next;", + distributed ? nat->external_mac : + od->l3dgw_port->lrp_networks.ea_s); + ovn_lflow_add_with_hint(lflows, od, + S_ROUTER_IN_ARP_RESOLVE, + 100, ds_cstr(match), + ds_cstr(actions), + &nat->header_); } /* S_ROUTER_OUT_UNDNAT */ @@ -12024,13 +12013,10 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, /* Load balancing and packet defrag are only valid on * Gateway routers or router with gateway port. */ if (!smap_get(&od->nbr->options, "chassis") && !od->l3dgw_port) { - sset_destroy(&nat_entries); return; } - build_lrouter_lb_flows(lflows, od, lbs, &nat_entries, match); - - sset_destroy(&nat_entries); + build_lrouter_lb_flows(lflows, od, lbs, match); } @@ -12094,8 +12080,8 @@ build_lswitch_and_lrouter_iterate_by_od(struct ovn_datapath *od, &lsi->actions); build_misc_local_traffic_drop_flows_for_lrouter(od, lsi->lflows); build_lrouter_arp_nd_for_datapath(od, lsi->lflows); - build_lrouter_nat_defrag_and_lb(od, lsi->lflows, lsi->lbs, &lsi->match, - &lsi->actions); + build_lrouter_nat_defrag_and_lb(od, lsi->lflows, lsi->lbs, + &lsi->match, &lsi->actions); } /* Helper function to combine all lflow generation which is iterated by port.
Remove add_router_lb_flow routine and move leftover lb flow installation code in build_lrouter_snat_flows_for_lb routine Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/ovn-northd.c | 328 +++++++++++++++++++++----------------------- 1 file changed, 157 insertions(+), 171 deletions(-)