Message ID | 20200622174124.1271839-1-numans@ovn.org |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,ovn] Fix selection fields for UDP and SCTP load balancers. | expand |
On 6/22/20 1:41 PM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > The commit 5af304e7478a ("Support selection fields in load balancer.") > didn't handle the selection fields for UDP and SCTP protocol. > If CMS has set the selection fields - tp_src and tp_dst for UDP or SCTP > load balancers, ovn-northd was adding lflows as > ct_lb(backends=<backends>, hash_fields(tp_src,tp_dst)) > instead of ct_lb(backends=<backends>, hash_fields(udp_src,udp_dst)) and > ct_lb(backends=<backends>, hash_fields(sctp_src,sctp_dst)) respectively. > > Because of this, select group flows were encoded with tcp_src and tcp_dst > hash fields. > > This patch fixes this issue and refactors the load balancer code in ovn-northd > to better handle the protocols. > > Test cases are now added for UDP and SCTP protocols. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1846189 > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/ovn-northd.c | 71 ++++++++---------- > tests/ovn.at | 179 ++++++++++++++++++++++++++++++++++++++++++-- > tests/system-ovn.at | 23 ++++++ > 3 files changed, 227 insertions(+), 46 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 40aeb0a58..7887d0d2f 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -3121,6 +3121,7 @@ struct ovn_lb { > > const struct nbrec_load_balancer *nlb; /* May be NULL. */ > char *selection_fields; > + char *proto; > struct lb_vip *vips; > size_t n_vips; > }; > @@ -3218,6 +3219,12 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, > struct smap_node *node; > size_t n_vips = 0; > > + if (!nbrec_lb->protocol || !nbrec_lb->protocol[0]) { > + lb->proto = xstrdup("tcp"); > + } else { > + lb->proto = xstrdup(nbrec_lb->protocol); > + } > + Hm, I don't think this is quite right. Load balancers that do not specify ports are L4-agnostic. This patch does not change the behavior, but it sets lb->proto to "tcp" even if no port is specified. This could result in confusion and potential future buggy code. > SMAP_FOR_EACH (node, &nbrec_lb->vips) { > char *vip; > uint16_t port; > @@ -3234,7 +3241,7 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, > lb->vips[n_vips].backend_ips = xstrdup(node->value); > > struct nbrec_load_balancer_health_check *lb_health_check = NULL; > - if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) { > + if (!strcmp(lb->proto, "sctp")) { > if (nbrec_lb->n_health_check > 0) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, > @@ -3309,15 +3316,11 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, > lb->vips[n_vips].backends[i].svc_mon_src_ip = svc_mon_src_ip; > > if (lb_health_check && op && svc_mon_src_ip) { > - const char *protocol = nbrec_lb->protocol; > - if (!protocol || !protocol[0]) { > - protocol = "tcp"; > - } > lb->vips[n_vips].backends[i].health_check = true; > struct service_monitor_info *mon_info = > create_or_get_service_mon(ctx, monitor_map, backend_ip, > op->nbsp->name, backend_port, > - protocol); > + lb->proto); > > ovs_assert(mon_info); > sbrec_service_monitor_set_options( > @@ -3351,7 +3354,14 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, > if (lb->nlb->n_selection_fields) { > struct ds sel_fields = DS_EMPTY_INITIALIZER; > for (size_t i = 0; i < lb->nlb->n_selection_fields; i++) { > - ds_put_format(&sel_fields, "%s,", lb->nlb->selection_fields[i]); > + char *field = lb->nlb->selection_fields[i]; > + if (!strcmp(field, "tp_src")) { > + ds_put_format(&sel_fields, "%s_src,", lb->proto); > + } else if (!strcmp(field, "tp_dst")) { > + ds_put_format(&sel_fields, "%s_dst,", lb->proto); > + } else { > + ds_put_format(&sel_fields, "%s,", field); > + } > } > ds_chomp(&sel_fields, ','); > lb->selection_fields = ds_steal_cstr(&sel_fields); > @@ -3374,6 +3384,7 @@ ovn_lb_destroy(struct ovn_lb *lb) > > free(lb->vips[i].backends); > } > + free(lb->proto); > free(lb->vips); > free(lb->selection_fields); > } > @@ -4820,7 +4831,7 @@ ls_has_dns_records(const struct nbrec_logical_switch *nbs) > > static void > build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows, > - struct lb_vip *lb_vip, > + struct lb_vip *lb_vip, const char *proto, > struct nbrec_load_balancer *lb, > int pl, struct shash *meter_groups) > { > @@ -4837,11 +4848,11 @@ build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows, > > ds_put_format(&match, "ip%s.dst == %s && %s", > lb_vip->addr_family == AF_INET ? "4": "6", > - lb_vip->vip, lb->protocol); > + lb_vip->vip, proto); > > char *vip = lb_vip->vip; > if (lb_vip->vip_port) { > - ds_put_format(&match, " && %s.dst == %u", lb->protocol, > + ds_put_format(&match, " && %s.dst == %u", proto, > lb_vip->vip_port); > vip = xasprintf("%s:%u", lb_vip->vip, lb_vip->vip_port); > } > @@ -4851,7 +4862,7 @@ build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows, > "protocol = \"%s\", " > "load_balancer = \"" UUID_FMT "\");", > event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS), > - meter, vip, lb->protocol, > + meter, vip, proto, > UUID_ARGS(&lb->header_.uuid)); > ovn_lflow_add_with_hint(lflows, od, pl, 130, ds_cstr(&match), action, > &lb->header_); > @@ -4906,7 +4917,7 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows, > sset_add(&all_ips_v6, lb_vip->vip); > } > > - build_empty_lb_event_flow(od, lflows, lb_vip, nb_lb, > + build_empty_lb_event_flow(od, lflows, lb_vip, lb->proto, nb_lb, > S_SWITCH_IN_PRE_LB, meter_groups); > > /* Ignore L4 port information in the key because fragmented packets > @@ -5577,7 +5588,7 @@ build_lb(struct ovn_datapath *od, struct hmap *lflows) > static void > build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows, > struct ovn_lb *lb, struct lb_vip *lb_vip, > - const char *ip_match, const char *proto) > + const char *ip_match) > { > if (lb_vip->n_backends == 0) { > return; > @@ -5605,7 +5616,7 @@ build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows, > */ > if (lb_vip->vip_port) { > ds_put_format(&proto_match, " && %s.dst == %"PRIu16, > - proto, backend->port); > + lb->proto, backend->port); > } > ds_put_format(&match_initiator, "(%s.src == %s && %s.dst == %s%s)", > ip_match, backend->ip, ip_match, backend->ip, > @@ -5614,7 +5625,7 @@ build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows, > /* Replies to hairpinned traffic are originated by backend->ip:port. */ > ds_clear(&proto_match); > if (lb_vip->vip_port) { > - ds_put_format(&proto_match, " && %s.src == %"PRIu16, proto, > + ds_put_format(&proto_match, " && %s.src == %"PRIu16, lb->proto, > backend->port); > } > ds_put_format(&match_reply, "(%s.src == %s%s)", ip_match, backend->ip, > @@ -5665,18 +5676,6 @@ build_lb_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb) > ip_match = "ip6"; > } > > - const char *proto = NULL; > - if (lb_vip->vip_port) { > - proto = "tcp"; > - if (lb->nlb->protocol) { > - if (!strcmp(lb->nlb->protocol, "udp")) { > - proto = "udp"; > - } else if (!strcmp(lb->nlb->protocol, "sctp")) { > - proto = "sctp"; > - } > - } > - } > - > /* New connections in Ingress table. */ > struct ds action = DS_EMPTY_INITIALIZER; > build_lb_vip_ct_lb_actions(lb_vip, &action, lb->selection_fields); > @@ -5684,7 +5683,8 @@ build_lb_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb) > struct ds match = DS_EMPTY_INITIALIZER; > ds_put_format(&match, "ct.new && %s.dst == %s", ip_match, lb_vip->vip); > if (lb_vip->vip_port) { > - ds_put_format(&match, " && %s.dst == %d", proto, lb_vip->vip_port); > + ds_put_format(&match, " && %s.dst == %d", lb->proto, > + lb_vip->vip_port); > ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_STATEFUL, 120, > ds_cstr(&match), ds_cstr(&action), > &lb->nlb->header_); > @@ -5701,7 +5701,7 @@ build_lb_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb) > * a load balancer VIP is DNAT-ed to a backend that happens to be > * the source of the traffic). > */ > - build_lb_hairpin_rules(od, lflows, lb, lb_vip, ip_match, proto); > + build_lb_hairpin_rules(od, lflows, lb, lb_vip, ip_match); > } > } > > @@ -7667,7 +7667,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od, > const char *proto, struct nbrec_load_balancer *lb, > struct shash *meter_groups, struct sset *nat_entries) > { > - build_empty_lb_event_flow(od, lflows, lb_vip, lb, S_ROUTER_IN_DNAT, > + build_empty_lb_event_flow(od, lflows, lb_vip, proto, lb, S_ROUTER_IN_DNAT, > meter_groups); > > /* A match and actions for new connections. */ > @@ -9358,14 +9358,9 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > } > > int prio = 110; > - 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); > + ds_put_format(&match, " && %s && %s.dst == %d", lb->proto, > + lb->proto, lb_vip->vip_port); > prio = 120; > } > > @@ -9374,7 +9369,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > od->l3redirect_port->json_key); > } > add_router_lb_flow(lflows, od, &match, &actions, prio, > - lb_force_snat_ip, lb_vip, proto, > + lb_force_snat_ip, lb_vip, lb->proto, > nb_lb, meter_groups, &nat_entries); > } > } > diff --git a/tests/ovn.at b/tests/ovn.at > index 8ee348397..a28563a5e 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1009,6 +1009,18 @@ ct_lb(backends=fd0f::2,fd0f::3; hash_fields="eth_src,eth_dst,ip_src,ip_dst,tp_sr > encodes as group:5 > uses group: id(5), name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,tp_src,tp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > has prereqs ip > +ct_lb(backends=fd0f::2,fd0f::3; hash_fields="eth_src,eth_dst,ip_src,ip_dst,tcp_src,tcp_dst"); > + encodes as group:6 > + uses group: id(6), name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,tcp_src,tcp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > + has prereqs ip > +ct_lb(backends=fd0f::2,fd0f::3; hash_fields="eth_src,eth_dst,ip_src,ip_dst,udp_src,udp_dst"); > + encodes as group:7 > + uses group: id(7), name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,udp_src,udp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > + has prereqs ip > +ct_lb(backends=fd0f::2,fd0f::3; hash_fields="eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst"); > + encodes as group:8 > + uses group: id(8), name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > + has prereqs ip > > # ct_next > ct_next; > @@ -1543,13 +1555,13 @@ handle_svc_check(reg0); > # select > reg9[16..31] = select(1=50, 2=100, 3, ); > formats as reg9[16..31] = select(1=50, 2=100, 3=100); > - encodes as group:6 > - uses group: id(6), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) > + encodes as group:9 > + uses group: id(9), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) > > reg0 = select(1, 2); > formats as reg0 = select(1=100, 2=100); > - encodes as group:7 > - uses group: id(7), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) > + encodes as group:10 > + uses group: id(10), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) > > reg0 = select(1=, 2); > Syntax error at `,' expecting weight. > @@ -1565,12 +1577,12 @@ reg0[0..14] = select(1, 2, 3); > cannot use 15-bit field reg0[0..14] for "select", which requires at least 16 bits. > > fwd_group(liveness="true", childports="eth0", "lsp1"); > - encodes as group:8 > - uses group: id(8), name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) > + encodes as group:11 > + uses group: id(11), name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) > > fwd_group(childports="eth0", "lsp1"); > - encodes as group:9 > - uses group: id(9), name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) > + encodes as group:12 > + uses group: id(12), name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) > > fwd_group(childports=eth0); > Syntax error at `eth0' expecting logical switch port. > @@ -19905,3 +19917,154 @@ OVS_WAIT_UNTIL([test 0 = $(ovn-sbctl show | grep Port_Binding -c)], [0]) > > OVN_CLEANUP([hv1]) > AT_CLEANUP > + > +AT_SETUP([ovn -- Load balancer selection fields]) > +AT_KEYWORDS([lb]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > +ovs-vsctl -- add-port br-int hv1-vif2 -- \ > + set interface hv1-vif2 external-ids:iface-id=sw0-p2 \ > + options:tx_pcap=hv1/vif2-tx.pcap \ > + options:rxq_pcap=hv1/vif2-rx.pcap \ > + ofport-request=2 > + > +sim_add hv2 > +as hv2 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.2 > +ovs-vsctl -- add-port br-int hv2-vif1 -- \ > + set interface hv2-vif1 external-ids:iface-id=sw1-p1 \ > + options:tx_pcap=hv2/vif1-tx.pcap \ > + options:rxq_pcap=hv2/vif1-rx.pcap \ > + ofport-request=1 > + > +ovn-nbctl ls-add sw0 > + > +ovn-nbctl lsp-add sw0 sw0-p1 > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" > + > +ovn-nbctl lsp-add sw0 sw0-p2 > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" > + > +# Create port group and ACLs for sw0 ports. > +ovn-nbctl pg-add pg0_drop sw0-p1 sw0-p2 > +ovn-nbctl acl-add pg0_drop from-lport 1001 "inport == @pg0_drop && ip" drop > +ovn-nbctl acl-add pg0_drop to-lport 1001 "outport == @pg0_drop && ip" drop > + > +ovn-nbctl pg-add pg0 sw0-p1 sw0-p2 > +ovn-nbctl acl-add pg0 from-lport 1002 "inport == @pg0 && ip4" allow-related > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && icmp4" allow-related > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related > + > +# Create the second logical switch with one port > +ovn-nbctl ls-add sw1 > +ovn-nbctl lsp-add sw1 sw1-p1 > +ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3" > +ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3" > + > +# Create port group and ACLs for sw1 ports. > +ovn-nbctl pg-add pg1_drop sw1-p1 > +ovn-nbctl acl-add pg1_drop from-lport 1001 "inport == @pg1_drop && ip" drop > +ovn-nbctl acl-add pg1_drop to-lport 1001 "outport == @pg1_drop && ip" drop > + > +ovn-nbctl pg-add pg1 sw1-p1 > +ovn-nbctl acl-add pg1 from-lport 1002 "inport == @pg1 && ip4" allow-related > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && icmp4" allow-related > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related > + > +# Create a logical router and attach both logical switches > +ovn-nbctl lr-add lr0 > +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > +ovn-nbctl lsp-add sw0 sw0-lr0 > +ovn-nbctl lsp-set-type sw0-lr0 router > +ovn-nbctl lsp-set-addresses sw0-lr0 router > +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > + > +ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24 > +ovn-nbctl lsp-add sw1 sw1-lr0 > +ovn-nbctl lsp-set-type sw1-lr0 router > +ovn-nbctl lsp-set-addresses sw1-lr0 router > +ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1 > + > +ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.3:80,20.0.0.3:80 > +lb1_uuid=$(ovn-nbctl --bare --columns _uuid list load_balancer) > +ovn-nbctl ls-lb-add sw0 lb1 > +ovn-nbctl ls-lb-add sw1 lb1 > +ovn-nbctl lr-lb-add lr0 lb1 > + > +OVS_WAIT_UNTIL([ > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=dp_hash" -c) -eq 1 > +]) > + > +OVS_WAIT_UNTIL([ > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=dp_hash" -c) -eq 1 > +]) > + > +echo "lb1_uuid = $lb1_uuid" > + > +ovn-nbctl set load_balancer $lb1_uuid selection_fields="ip_src,ip_dst,tp_src,tp_dst" > + > +OVS_WAIT_UNTIL([ > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 1 > +]) > + > +OVS_WAIT_UNTIL([ > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 1 > +]) > + > +# Change the protocol to udp. > +ovn-nbctl set load_balancer $lb1_uuid protocol=udp > +OVS_WAIT_UNTIL([ > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 1 > +]) > + > +OVS_WAIT_UNTIL([ > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 1 > +]) > + > +# Change the protocol to udp. > +ovn-nbctl set load_balancer $lb1_uuid protocol=sctp > +OVS_WAIT_UNTIL([ > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 > +]) > + > +OVS_WAIT_UNTIL([ > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 > +]) > + > +ovn-nbctl set load_balancer $lb1_uuid selection_fields="ip_src,ip_dst,tp_dst" > +OVS_WAIT_UNTIL([ > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq 1 > +]) > + > +OVS_WAIT_UNTIL([ > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq 1 > +]) > + > +OVN_CLEANUP([hv1], [hv2]) > +AT_CLEANUP > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 52f05f07e..2999e52fd 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -1234,6 +1234,29 @@ else > AT_CHECK([test $bar3_ct -eq 0]) > fi > > +# Change the protocol of lb2 to udp and set tp_src and tp_dst. > +ovn-nbctl set load_balancer $lb2_uuid selection_fields="ip_src,ip_dst,tp_src,tp_dst" > + > +OVS_WAIT_UNTIL([ > + test $(ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 2 > +]) > + > +ovn-nbctl set load_balancer $lb2_uuid protocol=udp > + > +OVS_WAIT_UNTIL([ > + test $(ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 2 > +]) > + > +# Change the protocol of lb2 to sctp. > +ovn-nbctl set load_balancer $lb2_uuid protocol=sctp > + > +OVS_WAIT_UNTIL([ > + test $(ovs-ofctl dump-groups br-int | \ > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 2 > +]) > + > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb >
On Mon, Jun 29, 2020 at 8:48 PM Mark Michelson <mmichels@redhat.com> wrote: > On 6/22/20 1:41 PM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > The commit 5af304e7478a ("Support selection fields in load balancer.") > > didn't handle the selection fields for UDP and SCTP protocol. > > If CMS has set the selection fields - tp_src and tp_dst for UDP or SCTP > > load balancers, ovn-northd was adding lflows as > > ct_lb(backends=<backends>, hash_fields(tp_src,tp_dst)) > > instead of ct_lb(backends=<backends>, hash_fields(udp_src,udp_dst)) and > > ct_lb(backends=<backends>, hash_fields(sctp_src,sctp_dst)) respectively. > > > > Because of this, select group flows were encoded with tcp_src and tcp_dst > > hash fields. > > > > This patch fixes this issue and refactors the load balancer code in > ovn-northd > > to better handle the protocols. > > > > Test cases are now added for UDP and SCTP protocols. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1846189 > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > northd/ovn-northd.c | 71 ++++++++---------- > > tests/ovn.at | 179 ++++++++++++++++++++++++++++++++++++++++++-- > > tests/system-ovn.at | 23 ++++++ > > 3 files changed, 227 insertions(+), 46 deletions(-) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 40aeb0a58..7887d0d2f 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -3121,6 +3121,7 @@ struct ovn_lb { > > > > const struct nbrec_load_balancer *nlb; /* May be NULL. */ > > char *selection_fields; > > + char *proto; > > struct lb_vip *vips; > > size_t n_vips; > > }; > > @@ -3218,6 +3219,12 @@ ovn_lb_create(struct northd_context *ctx, struct > hmap *lbs, > > struct smap_node *node; > > size_t n_vips = 0; > > > > + if (!nbrec_lb->protocol || !nbrec_lb->protocol[0]) { > > + lb->proto = xstrdup("tcp"); > > + } else { > > + lb->proto = xstrdup(nbrec_lb->protocol); > > + } > > + > > Hm, I don't think this is quite right. Load balancers that do not > specify ports are L4-agnostic. This patch does not change the behavior, > but it sets lb->proto to "tcp" even if no port is specified. This could > result in confusion and potential future buggy code. > Lets say, CMS creates a load balancer without specifying protocol ovn-nbctl lb-add lb0 "10.0.0.10" "10.0.0.3,20.0.0.3" And then ovn-nbctl set load_balancer <lb0_uuid> vips:"10.0.0.12:80"="10.0.0.4:80, 20.0.0.4:80" In this case we should consider the protocol as TCP right ? And anyway we add the L4 port checks only if the VIP has port specified. So I think it's ok to assume that the load balancer is TCP even if no VIP of the load balancer has a port specified. And lets say user specifies the selection_fields as "ip_src,ip_dst,tp_src,tp_dst" for the below load balancer (with no protocol specified) ovn-nbctl lb-add lb1 "10.0.0.20" "10.0.0.30,20.0.0.30" ovn-nbctl set load_balancer <lb1_uuid> selection_fields="ip_src,ip_dst,tp_src,tp_dst". In this case even without this patch, ovs-vswitchd will consider it as TCP protocol. So do you think it's fair to consider the protocol as tcp for a load balancer (when protocol is not explicitly set) - If a VIP has a port specified - If selection_fields has "tp_src/tp_dst" set ? Thanks Numan Thanks Numan > > > SMAP_FOR_EACH (node, &nbrec_lb->vips) { > > char *vip; > > uint16_t port; > > @@ -3234,7 +3241,7 @@ ovn_lb_create(struct northd_context *ctx, struct > hmap *lbs, > > lb->vips[n_vips].backend_ips = xstrdup(node->value); > > > > struct nbrec_load_balancer_health_check *lb_health_check = > NULL; > > - if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) { > > + if (!strcmp(lb->proto, "sctp")) { > > if (nbrec_lb->n_health_check > 0) { > > static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(1, 1); > > VLOG_WARN_RL(&rl, > > @@ -3309,15 +3316,11 @@ ovn_lb_create(struct northd_context *ctx, struct > hmap *lbs, > > lb->vips[n_vips].backends[i].svc_mon_src_ip = > svc_mon_src_ip; > > > > if (lb_health_check && op && svc_mon_src_ip) { > > - const char *protocol = nbrec_lb->protocol; > > - if (!protocol || !protocol[0]) { > > - protocol = "tcp"; > > - } > > lb->vips[n_vips].backends[i].health_check = true; > > struct service_monitor_info *mon_info = > > create_or_get_service_mon(ctx, monitor_map, > backend_ip, > > op->nbsp->name, > backend_port, > > - protocol); > > + lb->proto); > > > > ovs_assert(mon_info); > > sbrec_service_monitor_set_options( > > @@ -3351,7 +3354,14 @@ ovn_lb_create(struct northd_context *ctx, struct > hmap *lbs, > > if (lb->nlb->n_selection_fields) { > > struct ds sel_fields = DS_EMPTY_INITIALIZER; > > for (size_t i = 0; i < lb->nlb->n_selection_fields; i++) { > > - ds_put_format(&sel_fields, "%s,", > lb->nlb->selection_fields[i]); > > + char *field = lb->nlb->selection_fields[i]; > > + if (!strcmp(field, "tp_src")) { > > + ds_put_format(&sel_fields, "%s_src,", lb->proto); > > + } else if (!strcmp(field, "tp_dst")) { > > + ds_put_format(&sel_fields, "%s_dst,", lb->proto); > > + } else { > > + ds_put_format(&sel_fields, "%s,", field); > > + } > > } > > ds_chomp(&sel_fields, ','); > > lb->selection_fields = ds_steal_cstr(&sel_fields); > > @@ -3374,6 +3384,7 @@ ovn_lb_destroy(struct ovn_lb *lb) > > > > free(lb->vips[i].backends); > > } > > + free(lb->proto); > > free(lb->vips); > > free(lb->selection_fields); > > } > > @@ -4820,7 +4831,7 @@ ls_has_dns_records(const struct > nbrec_logical_switch *nbs) > > > > static void > > build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows, > > - struct lb_vip *lb_vip, > > + struct lb_vip *lb_vip, const char *proto, > > struct nbrec_load_balancer *lb, > > int pl, struct shash *meter_groups) > > { > > @@ -4837,11 +4848,11 @@ build_empty_lb_event_flow(struct ovn_datapath > *od, struct hmap *lflows, > > > > ds_put_format(&match, "ip%s.dst == %s && %s", > > lb_vip->addr_family == AF_INET ? "4": "6", > > - lb_vip->vip, lb->protocol); > > + lb_vip->vip, proto); > > > > char *vip = lb_vip->vip; > > if (lb_vip->vip_port) { > > - ds_put_format(&match, " && %s.dst == %u", lb->protocol, > > + ds_put_format(&match, " && %s.dst == %u", proto, > > lb_vip->vip_port); > > vip = xasprintf("%s:%u", lb_vip->vip, lb_vip->vip_port); > > } > > @@ -4851,7 +4862,7 @@ build_empty_lb_event_flow(struct ovn_datapath *od, > struct hmap *lflows, > > "protocol = \"%s\", " > > "load_balancer = \"" UUID_FMT "\");", > > event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS), > > - meter, vip, lb->protocol, > > + meter, vip, proto, > > UUID_ARGS(&lb->header_.uuid)); > > ovn_lflow_add_with_hint(lflows, od, pl, 130, ds_cstr(&match), > action, > > &lb->header_); > > @@ -4906,7 +4917,7 @@ build_pre_lb(struct ovn_datapath *od, struct hmap > *lflows, > > sset_add(&all_ips_v6, lb_vip->vip); > > } > > > > - build_empty_lb_event_flow(od, lflows, lb_vip, nb_lb, > > + build_empty_lb_event_flow(od, lflows, lb_vip, lb->proto, > nb_lb, > > S_SWITCH_IN_PRE_LB, > meter_groups); > > > > /* Ignore L4 port information in the key because > fragmented packets > > @@ -5577,7 +5588,7 @@ build_lb(struct ovn_datapath *od, struct hmap > *lflows) > > static void > > build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows, > > struct ovn_lb *lb, struct lb_vip *lb_vip, > > - const char *ip_match, const char *proto) > > + const char *ip_match) > > { > > if (lb_vip->n_backends == 0) { > > return; > > @@ -5605,7 +5616,7 @@ build_lb_hairpin_rules(struct ovn_datapath *od, > struct hmap *lflows, > > */ > > if (lb_vip->vip_port) { > > ds_put_format(&proto_match, " && %s.dst == %"PRIu16, > > - proto, backend->port); > > + lb->proto, backend->port); > > } > > ds_put_format(&match_initiator, "(%s.src == %s && %s.dst == > %s%s)", > > ip_match, backend->ip, ip_match, backend->ip, > > @@ -5614,7 +5625,7 @@ build_lb_hairpin_rules(struct ovn_datapath *od, > struct hmap *lflows, > > /* Replies to hairpinned traffic are originated by > backend->ip:port. */ > > ds_clear(&proto_match); > > if (lb_vip->vip_port) { > > - ds_put_format(&proto_match, " && %s.src == %"PRIu16, proto, > > + ds_put_format(&proto_match, " && %s.src == %"PRIu16, > lb->proto, > > backend->port); > > } > > ds_put_format(&match_reply, "(%s.src == %s%s)", ip_match, > backend->ip, > > @@ -5665,18 +5676,6 @@ build_lb_rules(struct ovn_datapath *od, struct > hmap *lflows, struct ovn_lb *lb) > > ip_match = "ip6"; > > } > > > > - const char *proto = NULL; > > - if (lb_vip->vip_port) { > > - proto = "tcp"; > > - if (lb->nlb->protocol) { > > - if (!strcmp(lb->nlb->protocol, "udp")) { > > - proto = "udp"; > > - } else if (!strcmp(lb->nlb->protocol, "sctp")) { > > - proto = "sctp"; > > - } > > - } > > - } > > - > > /* New connections in Ingress table. */ > > struct ds action = DS_EMPTY_INITIALIZER; > > build_lb_vip_ct_lb_actions(lb_vip, &action, > lb->selection_fields); > > @@ -5684,7 +5683,8 @@ build_lb_rules(struct ovn_datapath *od, struct > hmap *lflows, struct ovn_lb *lb) > > struct ds match = DS_EMPTY_INITIALIZER; > > ds_put_format(&match, "ct.new && %s.dst == %s", ip_match, > lb_vip->vip); > > if (lb_vip->vip_port) { > > - ds_put_format(&match, " && %s.dst == %d", proto, > lb_vip->vip_port); > > + ds_put_format(&match, " && %s.dst == %d", lb->proto, > > + lb_vip->vip_port); > > ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_STATEFUL, > 120, > > ds_cstr(&match), ds_cstr(&action), > > &lb->nlb->header_); > > @@ -5701,7 +5701,7 @@ build_lb_rules(struct ovn_datapath *od, struct > hmap *lflows, struct ovn_lb *lb) > > * a load balancer VIP is DNAT-ed to a backend that happens to > be > > * the source of the traffic). > > */ > > - build_lb_hairpin_rules(od, lflows, lb, lb_vip, ip_match, proto); > > + build_lb_hairpin_rules(od, lflows, lb, lb_vip, ip_match); > > } > > } > > > > @@ -7667,7 +7667,7 @@ add_router_lb_flow(struct hmap *lflows, struct > ovn_datapath *od, > > const char *proto, struct nbrec_load_balancer *lb, > > struct shash *meter_groups, struct sset > *nat_entries) > > { > > - build_empty_lb_event_flow(od, lflows, lb_vip, lb, S_ROUTER_IN_DNAT, > > + build_empty_lb_event_flow(od, lflows, lb_vip, proto, lb, > S_ROUTER_IN_DNAT, > > meter_groups); > > > > /* A match and actions for new connections. */ > > @@ -9358,14 +9358,9 @@ build_lrouter_flows(struct hmap *datapaths, > struct hmap *ports, > > } > > > > int prio = 110; > > - 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); > > + ds_put_format(&match, " && %s && %s.dst == %d", > lb->proto, > > + lb->proto, lb_vip->vip_port); > > prio = 120; > > } > > > > @@ -9374,7 +9369,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > > od->l3redirect_port->json_key); > > } > > add_router_lb_flow(lflows, od, &match, &actions, prio, > > - lb_force_snat_ip, lb_vip, proto, > > + lb_force_snat_ip, lb_vip, lb->proto, > > nb_lb, meter_groups, &nat_entries); > > } > > } > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 8ee348397..a28563a5e 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -1009,6 +1009,18 @@ ct_lb(backends=fd0f::2,fd0f::3; > hash_fields="eth_src,eth_dst,ip_src,ip_dst,tp_sr > > encodes as group:5 > > uses group: id(5), > name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,tp_src,tp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > > has prereqs ip > > +ct_lb(backends=fd0f::2,fd0f::3; > hash_fields="eth_src,eth_dst,ip_src,ip_dst,tcp_src,tcp_dst"); > > + encodes as group:6 > > + uses group: id(6), > name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,tcp_src,tcp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > > + has prereqs ip > > +ct_lb(backends=fd0f::2,fd0f::3; > hash_fields="eth_src,eth_dst,ip_src,ip_dst,udp_src,udp_dst"); > > + encodes as group:7 > > + uses group: id(7), > name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,udp_src,udp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > > + has prereqs ip > > +ct_lb(backends=fd0f::2,fd0f::3; > hash_fields="eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst"); > > + encodes as group:8 > > + uses group: id(8), > name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) > > + has prereqs ip > > > > # ct_next > > ct_next; > > @@ -1543,13 +1555,13 @@ handle_svc_check(reg0); > > # select > > reg9[16..31] = select(1=50, 2=100, 3, ); > > formats as reg9[16..31] = select(1=50, 2=100, 3=100); > > - encodes as group:6 > > - uses group: id(6), > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) > > + encodes as group:9 > > + uses group: id(9), > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) > > > > reg0 = select(1, 2); > > formats as reg0 = select(1=100, 2=100); > > - encodes as group:7 > > - uses group: id(7), > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) > > + encodes as group:10 > > + uses group: id(10), > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) > > > > reg0 = select(1=, 2); > > Syntax error at `,' expecting weight. > > @@ -1565,12 +1577,12 @@ reg0[0..14] = select(1, 2, 3); > > cannot use 15-bit field reg0[0..14] for "select", which requires > at least 16 bits. > > > > fwd_group(liveness="true", childports="eth0", "lsp1"); > > - encodes as group:8 > > - uses group: id(8), > name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) > > + encodes as group:11 > > + uses group: id(11), > name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) > > > > fwd_group(childports="eth0", "lsp1"); > > - encodes as group:9 > > - uses group: id(9), > name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) > > + encodes as group:12 > > + uses group: id(12), > name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) > > > > fwd_group(childports=eth0); > > Syntax error at `eth0' expecting logical switch port. > > @@ -19905,3 +19917,154 @@ OVS_WAIT_UNTIL([test 0 = $(ovn-sbctl show | > grep Port_Binding -c)], [0]) > > > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > + > > +AT_SETUP([ovn -- Load balancer selection fields]) > > +AT_KEYWORDS([lb]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > +ovs-vsctl -- add-port br-int hv1-vif2 -- \ > > + set interface hv1-vif2 external-ids:iface-id=sw0-p2 \ > > + options:tx_pcap=hv1/vif2-tx.pcap \ > > + options:rxq_pcap=hv1/vif2-rx.pcap \ > > + ofport-request=2 > > + > > +sim_add hv2 > > +as hv2 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.2 > > +ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > + set interface hv2-vif1 external-ids:iface-id=sw1-p1 \ > > + options:tx_pcap=hv2/vif1-tx.pcap \ > > + options:rxq_pcap=hv2/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +ovn-nbctl ls-add sw0 > > + > > +ovn-nbctl lsp-add sw0 sw0-p1 > > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" > > + > > +ovn-nbctl lsp-add sw0 sw0-p2 > > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" > > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" > > + > > +# Create port group and ACLs for sw0 ports. > > +ovn-nbctl pg-add pg0_drop sw0-p1 sw0-p2 > > +ovn-nbctl acl-add pg0_drop from-lport 1001 "inport == @pg0_drop && ip" > drop > > +ovn-nbctl acl-add pg0_drop to-lport 1001 "outport == @pg0_drop && ip" > drop > > + > > +ovn-nbctl pg-add pg0 sw0-p1 sw0-p2 > > +ovn-nbctl acl-add pg0 from-lport 1002 "inport == @pg0 && ip4" > allow-related > > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src > == 0.0.0.0/0 && icmp4" allow-related > > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src > == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related > > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src > == 0.0.0.0/0 && udp && udp.dst == 80" allow-related > > + > > +# Create the second logical switch with one port > > +ovn-nbctl ls-add sw1 > > +ovn-nbctl lsp-add sw1 sw1-p1 > > +ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3" > > +ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3" > > + > > +# Create port group and ACLs for sw1 ports. > > +ovn-nbctl pg-add pg1_drop sw1-p1 > > +ovn-nbctl acl-add pg1_drop from-lport 1001 "inport == @pg1_drop && ip" > drop > > +ovn-nbctl acl-add pg1_drop to-lport 1001 "outport == @pg1_drop && ip" > drop > > + > > +ovn-nbctl pg-add pg1 sw1-p1 > > +ovn-nbctl acl-add pg1 from-lport 1002 "inport == @pg1 && ip4" > allow-related > > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src > == 0.0.0.0/0 && icmp4" allow-related > > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src > == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related > > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src > == 0.0.0.0/0 && udp && udp.dst == 80" allow-related > > + > > +# Create a logical router and attach both logical switches > > +ovn-nbctl lr-add lr0 > > +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > > +ovn-nbctl lsp-add sw0 sw0-lr0 > > +ovn-nbctl lsp-set-type sw0-lr0 router > > +ovn-nbctl lsp-set-addresses sw0-lr0 router > > +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > > + > > +ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24 > > +ovn-nbctl lsp-add sw1 sw1-lr0 > > +ovn-nbctl lsp-set-type sw1-lr0 router > > +ovn-nbctl lsp-set-addresses sw1-lr0 router > > +ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1 > > + > > +ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.3:80,20.0.0.3:80 > > +lb1_uuid=$(ovn-nbctl --bare --columns _uuid list load_balancer) > > +ovn-nbctl ls-lb-add sw0 lb1 > > +ovn-nbctl ls-lb-add sw1 lb1 > > +ovn-nbctl lr-lb-add lr0 lb1 > > + > > +OVS_WAIT_UNTIL([ > > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=dp_hash" -c) -eq 1 > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=dp_hash" -c) -eq 1 > > +]) > > + > > +echo "lb1_uuid = $lb1_uuid" > > + > > +ovn-nbctl set load_balancer $lb1_uuid > selection_fields="ip_src,ip_dst,tp_src,tp_dst" > > + > > +OVS_WAIT_UNTIL([ > > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" > -c) -eq 1 > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" > -c) -eq 1 > > +]) > > + > > +# Change the protocol to udp. > > +ovn-nbctl set load_balancer $lb1_uuid protocol=udp > > +OVS_WAIT_UNTIL([ > > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" > -c) -eq 1 > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" > -c) -eq 1 > > +]) > > + > > +# Change the protocol to udp. > > +ovn-nbctl set load_balancer $lb1_uuid protocol=sctp > > +OVS_WAIT_UNTIL([ > > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > > + grep > "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > > + grep > "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 > > +]) > > + > > +ovn-nbctl set load_balancer $lb1_uuid > selection_fields="ip_src,ip_dst,tp_dst" > > +OVS_WAIT_UNTIL([ > > + test $(as hv1 ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq > 1 > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test $(as hv2 ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq > 1 > > +]) > > + > > +OVN_CLEANUP([hv1], [hv2]) > > +AT_CLEANUP > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 52f05f07e..2999e52fd 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -1234,6 +1234,29 @@ else > > AT_CHECK([test $bar3_ct -eq 0]) > > fi > > > > +# Change the protocol of lb2 to udp and set tp_src and tp_dst. > > +ovn-nbctl set load_balancer $lb2_uuid > selection_fields="ip_src,ip_dst,tp_src,tp_dst" > > + > > +OVS_WAIT_UNTIL([ > > + test $(ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" > -c) -eq 2 > > +]) > > + > > +ovn-nbctl set load_balancer $lb2_uuid protocol=udp > > + > > +OVS_WAIT_UNTIL([ > > + test $(ovs-ofctl dump-groups br-int | \ > > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" > -c) -eq 2 > > +]) > > + > > +# Change the protocol of lb2 to sctp. > > +ovn-nbctl set load_balancer $lb2_uuid protocol=sctp > > + > > +OVS_WAIT_UNTIL([ > > + test $(ovs-ofctl dump-groups br-int | \ > > + grep > "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 2 > > +]) > > + > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > > > as ovn-sb > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Mon, Jun 29, 2020 at 9:29 PM Numan Siddique <numans@ovn.org> wrote: > > > On Mon, Jun 29, 2020 at 8:48 PM Mark Michelson <mmichels@redhat.com> > wrote: > >> On 6/22/20 1:41 PM, numans@ovn.org wrote: >> > From: Numan Siddique <numans@ovn.org> >> > >> > The commit 5af304e7478a ("Support selection fields in load balancer.") >> > didn't handle the selection fields for UDP and SCTP protocol. >> > If CMS has set the selection fields - tp_src and tp_dst for UDP or SCTP >> > load balancers, ovn-northd was adding lflows as >> > ct_lb(backends=<backends>, hash_fields(tp_src,tp_dst)) >> > instead of ct_lb(backends=<backends>, hash_fields(udp_src,udp_dst)) and >> > ct_lb(backends=<backends>, hash_fields(sctp_src,sctp_dst)) respectively. >> > >> > Because of this, select group flows were encoded with tcp_src and >> tcp_dst >> > hash fields. >> > >> > This patch fixes this issue and refactors the load balancer code in >> ovn-northd >> > to better handle the protocols. >> > >> > Test cases are now added for UDP and SCTP protocols. >> > >> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1846189 >> > Signed-off-by: Numan Siddique <numans@ovn.org> >> > --- >> > northd/ovn-northd.c | 71 ++++++++---------- >> > tests/ovn.at | 179 >> ++++++++++++++++++++++++++++++++++++++++++-- >> > tests/system-ovn.at | 23 ++++++ >> > 3 files changed, 227 insertions(+), 46 deletions(-) >> > >> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> > index 40aeb0a58..7887d0d2f 100644 >> > --- a/northd/ovn-northd.c >> > +++ b/northd/ovn-northd.c >> > @@ -3121,6 +3121,7 @@ struct ovn_lb { >> > >> > const struct nbrec_load_balancer *nlb; /* May be NULL. */ >> > char *selection_fields; >> > + char *proto; >> > struct lb_vip *vips; >> > size_t n_vips; >> > }; >> > @@ -3218,6 +3219,12 @@ ovn_lb_create(struct northd_context *ctx, struct >> hmap *lbs, >> > struct smap_node *node; >> > size_t n_vips = 0; >> > >> > + if (!nbrec_lb->protocol || !nbrec_lb->protocol[0]) { >> > + lb->proto = xstrdup("tcp"); >> > + } else { >> > + lb->proto = xstrdup(nbrec_lb->protocol); >> > + } >> > + >> >> Hm, I don't think this is quite right. Load balancers that do not >> specify ports are L4-agnostic. This patch does not change the behavior, >> but it sets lb->proto to "tcp" even if no port is specified. This could >> result in confusion and potential future buggy code. >> > > > Lets say, CMS creates a load balancer without specifying protocol > > ovn-nbctl lb-add lb0 "10.0.0.10" "10.0.0.3,20.0.0.3" > > And then > > ovn-nbctl set load_balancer <lb0_uuid> vips:"10.0.0.12:80"="10.0.0.4:80, > 20.0.0.4:80" > > In this case we should consider the protocol as TCP right ? > > And anyway we add the L4 port checks only if the VIP has port specified. > So I think > it's ok to assume that the load balancer is TCP even if no VIP of the load > balancer has a port specified. > > And lets say user specifies the selection_fields as > "ip_src,ip_dst,tp_src,tp_dst" for the below load balancer > (with no protocol specified) > > ovn-nbctl lb-add lb1 "10.0.0.20" "10.0.0.30,20.0.0.30" > ovn-nbctl set load_balancer <lb1_uuid> > selection_fields="ip_src,ip_dst,tp_src,tp_dst". > > In this case even without this patch, ovs-vswitchd will consider it as TCP > protocol. > > So do you think it's fair to consider the protocol as tcp for a load > balancer (when protocol is not explicitly set) > - If a VIP has a port specified > - If selection_fields has "tp_src/tp_dst" set ? > > Hi Mark, I removed the refactoring part of the code to address your concern. Please check out v2 - https://patchwork.ozlabs.org/project/openvswitch/patch/20200630052114.2813829-1-numans@ovn.org/ Thanks Numan > Thanks > Numan > > Thanks > Numan > > > > >> >> > SMAP_FOR_EACH (node, &nbrec_lb->vips) { >> > char *vip; >> > uint16_t port; >> > @@ -3234,7 +3241,7 @@ ovn_lb_create(struct northd_context *ctx, struct >> hmap *lbs, >> > lb->vips[n_vips].backend_ips = xstrdup(node->value); >> > >> > struct nbrec_load_balancer_health_check *lb_health_check = >> NULL; >> > - if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) >> { >> > + if (!strcmp(lb->proto, "sctp")) { >> > if (nbrec_lb->n_health_check > 0) { >> > static struct vlog_rate_limit rl = >> VLOG_RATE_LIMIT_INIT(1, 1); >> > VLOG_WARN_RL(&rl, >> > @@ -3309,15 +3316,11 @@ ovn_lb_create(struct northd_context *ctx, >> struct hmap *lbs, >> > lb->vips[n_vips].backends[i].svc_mon_src_ip = >> svc_mon_src_ip; >> > >> > if (lb_health_check && op && svc_mon_src_ip) { >> > - const char *protocol = nbrec_lb->protocol; >> > - if (!protocol || !protocol[0]) { >> > - protocol = "tcp"; >> > - } >> > lb->vips[n_vips].backends[i].health_check = true; >> > struct service_monitor_info *mon_info = >> > create_or_get_service_mon(ctx, monitor_map, >> backend_ip, >> > op->nbsp->name, >> backend_port, >> > - protocol); >> > + lb->proto); >> > >> > ovs_assert(mon_info); >> > sbrec_service_monitor_set_options( >> > @@ -3351,7 +3354,14 @@ ovn_lb_create(struct northd_context *ctx, struct >> hmap *lbs, >> > if (lb->nlb->n_selection_fields) { >> > struct ds sel_fields = DS_EMPTY_INITIALIZER; >> > for (size_t i = 0; i < lb->nlb->n_selection_fields; i++) { >> > - ds_put_format(&sel_fields, "%s,", >> lb->nlb->selection_fields[i]); >> > + char *field = lb->nlb->selection_fields[i]; >> > + if (!strcmp(field, "tp_src")) { >> > + ds_put_format(&sel_fields, "%s_src,", lb->proto); >> > + } else if (!strcmp(field, "tp_dst")) { >> > + ds_put_format(&sel_fields, "%s_dst,", lb->proto); >> > + } else { >> > + ds_put_format(&sel_fields, "%s,", field); >> > + } >> > } >> > ds_chomp(&sel_fields, ','); >> > lb->selection_fields = ds_steal_cstr(&sel_fields); >> > @@ -3374,6 +3384,7 @@ ovn_lb_destroy(struct ovn_lb *lb) >> > >> > free(lb->vips[i].backends); >> > } >> > + free(lb->proto); >> > free(lb->vips); >> > free(lb->selection_fields); >> > } >> > @@ -4820,7 +4831,7 @@ ls_has_dns_records(const struct >> nbrec_logical_switch *nbs) >> > >> > static void >> > build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap >> *lflows, >> > - struct lb_vip *lb_vip, >> > + struct lb_vip *lb_vip, const char *proto, >> > struct nbrec_load_balancer *lb, >> > int pl, struct shash *meter_groups) >> > { >> > @@ -4837,11 +4848,11 @@ build_empty_lb_event_flow(struct ovn_datapath >> *od, struct hmap *lflows, >> > >> > ds_put_format(&match, "ip%s.dst == %s && %s", >> > lb_vip->addr_family == AF_INET ? "4": "6", >> > - lb_vip->vip, lb->protocol); >> > + lb_vip->vip, proto); >> > >> > char *vip = lb_vip->vip; >> > if (lb_vip->vip_port) { >> > - ds_put_format(&match, " && %s.dst == %u", lb->protocol, >> > + ds_put_format(&match, " && %s.dst == %u", proto, >> > lb_vip->vip_port); >> > vip = xasprintf("%s:%u", lb_vip->vip, lb_vip->vip_port); >> > } >> > @@ -4851,7 +4862,7 @@ build_empty_lb_event_flow(struct ovn_datapath >> *od, struct hmap *lflows, >> > "protocol = \"%s\", " >> > "load_balancer = \"" UUID_FMT "\");", >> > event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS), >> > - meter, vip, lb->protocol, >> > + meter, vip, proto, >> > UUID_ARGS(&lb->header_.uuid)); >> > ovn_lflow_add_with_hint(lflows, od, pl, 130, ds_cstr(&match), >> action, >> > &lb->header_); >> > @@ -4906,7 +4917,7 @@ build_pre_lb(struct ovn_datapath *od, struct hmap >> *lflows, >> > sset_add(&all_ips_v6, lb_vip->vip); >> > } >> > >> > - build_empty_lb_event_flow(od, lflows, lb_vip, nb_lb, >> > + build_empty_lb_event_flow(od, lflows, lb_vip, lb->proto, >> nb_lb, >> > S_SWITCH_IN_PRE_LB, >> meter_groups); >> > >> > /* Ignore L4 port information in the key because >> fragmented packets >> > @@ -5577,7 +5588,7 @@ build_lb(struct ovn_datapath *od, struct hmap >> *lflows) >> > static void >> > build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows, >> > struct ovn_lb *lb, struct lb_vip *lb_vip, >> > - const char *ip_match, const char *proto) >> > + const char *ip_match) >> > { >> > if (lb_vip->n_backends == 0) { >> > return; >> > @@ -5605,7 +5616,7 @@ build_lb_hairpin_rules(struct ovn_datapath *od, >> struct hmap *lflows, >> > */ >> > if (lb_vip->vip_port) { >> > ds_put_format(&proto_match, " && %s.dst == %"PRIu16, >> > - proto, backend->port); >> > + lb->proto, backend->port); >> > } >> > ds_put_format(&match_initiator, "(%s.src == %s && %s.dst == >> %s%s)", >> > ip_match, backend->ip, ip_match, backend->ip, >> > @@ -5614,7 +5625,7 @@ build_lb_hairpin_rules(struct ovn_datapath *od, >> struct hmap *lflows, >> > /* Replies to hairpinned traffic are originated by >> backend->ip:port. */ >> > ds_clear(&proto_match); >> > if (lb_vip->vip_port) { >> > - ds_put_format(&proto_match, " && %s.src == %"PRIu16, proto, >> > + ds_put_format(&proto_match, " && %s.src == %"PRIu16, >> lb->proto, >> > backend->port); >> > } >> > ds_put_format(&match_reply, "(%s.src == %s%s)", ip_match, >> backend->ip, >> > @@ -5665,18 +5676,6 @@ build_lb_rules(struct ovn_datapath *od, struct >> hmap *lflows, struct ovn_lb *lb) >> > ip_match = "ip6"; >> > } >> > >> > - const char *proto = NULL; >> > - if (lb_vip->vip_port) { >> > - proto = "tcp"; >> > - if (lb->nlb->protocol) { >> > - if (!strcmp(lb->nlb->protocol, "udp")) { >> > - proto = "udp"; >> > - } else if (!strcmp(lb->nlb->protocol, "sctp")) { >> > - proto = "sctp"; >> > - } >> > - } >> > - } >> > - >> > /* New connections in Ingress table. */ >> > struct ds action = DS_EMPTY_INITIALIZER; >> > build_lb_vip_ct_lb_actions(lb_vip, &action, >> lb->selection_fields); >> > @@ -5684,7 +5683,8 @@ build_lb_rules(struct ovn_datapath *od, struct >> hmap *lflows, struct ovn_lb *lb) >> > struct ds match = DS_EMPTY_INITIALIZER; >> > ds_put_format(&match, "ct.new && %s.dst == %s", ip_match, >> lb_vip->vip); >> > if (lb_vip->vip_port) { >> > - ds_put_format(&match, " && %s.dst == %d", proto, >> lb_vip->vip_port); >> > + ds_put_format(&match, " && %s.dst == %d", lb->proto, >> > + lb_vip->vip_port); >> > ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_STATEFUL, >> 120, >> > ds_cstr(&match), ds_cstr(&action), >> > &lb->nlb->header_); >> > @@ -5701,7 +5701,7 @@ build_lb_rules(struct ovn_datapath *od, struct >> hmap *lflows, struct ovn_lb *lb) >> > * a load balancer VIP is DNAT-ed to a backend that happens >> to be >> > * the source of the traffic). >> > */ >> > - build_lb_hairpin_rules(od, lflows, lb, lb_vip, ip_match, >> proto); >> > + build_lb_hairpin_rules(od, lflows, lb, lb_vip, ip_match); >> > } >> > } >> > >> > @@ -7667,7 +7667,7 @@ add_router_lb_flow(struct hmap *lflows, struct >> ovn_datapath *od, >> > const char *proto, struct nbrec_load_balancer *lb, >> > struct shash *meter_groups, struct sset >> *nat_entries) >> > { >> > - build_empty_lb_event_flow(od, lflows, lb_vip, lb, S_ROUTER_IN_DNAT, >> > + build_empty_lb_event_flow(od, lflows, lb_vip, proto, lb, >> S_ROUTER_IN_DNAT, >> > meter_groups); >> > >> > /* A match and actions for new connections. */ >> > @@ -9358,14 +9358,9 @@ build_lrouter_flows(struct hmap *datapaths, >> struct hmap *ports, >> > } >> > >> > int prio = 110; >> > - 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); >> > + ds_put_format(&match, " && %s && %s.dst == %d", >> lb->proto, >> > + lb->proto, lb_vip->vip_port); >> > prio = 120; >> > } >> > >> > @@ -9374,7 +9369,7 @@ build_lrouter_flows(struct hmap *datapaths, >> struct hmap *ports, >> > od->l3redirect_port->json_key); >> > } >> > add_router_lb_flow(lflows, od, &match, &actions, prio, >> > - lb_force_snat_ip, lb_vip, proto, >> > + lb_force_snat_ip, lb_vip, lb->proto, >> > nb_lb, meter_groups, &nat_entries); >> > } >> > } >> > diff --git a/tests/ovn.at b/tests/ovn.at >> > index 8ee348397..a28563a5e 100644 >> > --- a/tests/ovn.at >> > +++ b/tests/ovn.at >> > @@ -1009,6 +1009,18 @@ ct_lb(backends=fd0f::2,fd0f::3; >> hash_fields="eth_src,eth_dst,ip_src,ip_dst,tp_sr >> > encodes as group:5 >> > uses group: id(5), >> name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,tp_src,tp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) >> > has prereqs ip >> > +ct_lb(backends=fd0f::2,fd0f::3; >> hash_fields="eth_src,eth_dst,ip_src,ip_dst,tcp_src,tcp_dst"); >> > + encodes as group:6 >> > + uses group: id(6), >> name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,tcp_src,tcp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) >> > + has prereqs ip >> > +ct_lb(backends=fd0f::2,fd0f::3; >> hash_fields="eth_src,eth_dst,ip_src,ip_dst,udp_src,udp_dst"); >> > + encodes as group:7 >> > + uses group: id(7), >> name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,udp_src,udp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) >> > + has prereqs ip >> > +ct_lb(backends=fd0f::2,fd0f::3; >> hash_fields="eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst"); >> > + encodes as group:8 >> > + uses group: id(8), >> name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) >> > + has prereqs ip >> > >> > # ct_next >> > ct_next; >> > @@ -1543,13 +1555,13 @@ handle_svc_check(reg0); >> > # select >> > reg9[16..31] = select(1=50, 2=100, 3, ); >> > formats as reg9[16..31] = select(1=50, 2=100, 3=100); >> > - encodes as group:6 >> > - uses group: id(6), >> name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) >> > + encodes as group:9 >> > + uses group: id(9), >> name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) >> > >> > reg0 = select(1, 2); >> > formats as reg0 = select(1=100, 2=100); >> > - encodes as group:7 >> > - uses group: id(7), >> name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) >> > + encodes as group:10 >> > + uses group: id(10), >> name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) >> > >> > reg0 = select(1=, 2); >> > Syntax error at `,' expecting weight. >> > @@ -1565,12 +1577,12 @@ reg0[0..14] = select(1, 2, 3); >> > cannot use 15-bit field reg0[0..14] for "select", which requires >> at least 16 bits. >> > >> > fwd_group(liveness="true", childports="eth0", "lsp1"); >> > - encodes as group:8 >> > - uses group: id(8), >> name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) >> > + encodes as group:11 >> > + uses group: id(11), >> name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) >> > >> > fwd_group(childports="eth0", "lsp1"); >> > - encodes as group:9 >> > - uses group: id(9), >> name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) >> > + encodes as group:12 >> > + uses group: id(12), >> name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) >> > >> > fwd_group(childports=eth0); >> > Syntax error at `eth0' expecting logical switch port. >> > @@ -19905,3 +19917,154 @@ OVS_WAIT_UNTIL([test 0 = $(ovn-sbctl show | >> grep Port_Binding -c)], [0]) >> > >> > OVN_CLEANUP([hv1]) >> > AT_CLEANUP >> > + >> > +AT_SETUP([ovn -- Load balancer selection fields]) >> > +AT_KEYWORDS([lb]) >> > +ovn_start >> > + >> > +net_add n1 >> > + >> > +sim_add hv1 >> > +as hv1 >> > +ovs-vsctl add-br br-phys >> > +ovn_attach n1 br-phys 192.168.0.1 >> > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ >> > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ >> > + options:tx_pcap=hv1/vif1-tx.pcap \ >> > + options:rxq_pcap=hv1/vif1-rx.pcap \ >> > + ofport-request=1 >> > +ovs-vsctl -- add-port br-int hv1-vif2 -- \ >> > + set interface hv1-vif2 external-ids:iface-id=sw0-p2 \ >> > + options:tx_pcap=hv1/vif2-tx.pcap \ >> > + options:rxq_pcap=hv1/vif2-rx.pcap \ >> > + ofport-request=2 >> > + >> > +sim_add hv2 >> > +as hv2 >> > +ovs-vsctl add-br br-phys >> > +ovn_attach n1 br-phys 192.168.0.2 >> > +ovs-vsctl -- add-port br-int hv2-vif1 -- \ >> > + set interface hv2-vif1 external-ids:iface-id=sw1-p1 \ >> > + options:tx_pcap=hv2/vif1-tx.pcap \ >> > + options:rxq_pcap=hv2/vif1-rx.pcap \ >> > + ofport-request=1 >> > + >> > +ovn-nbctl ls-add sw0 >> > + >> > +ovn-nbctl lsp-add sw0 sw0-p1 >> > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" >> > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" >> > + >> > +ovn-nbctl lsp-add sw0 sw0-p2 >> > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" >> > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" >> > + >> > +# Create port group and ACLs for sw0 ports. >> > +ovn-nbctl pg-add pg0_drop sw0-p1 sw0-p2 >> > +ovn-nbctl acl-add pg0_drop from-lport 1001 "inport == @pg0_drop && ip" >> drop >> > +ovn-nbctl acl-add pg0_drop to-lport 1001 "outport == @pg0_drop && ip" >> drop >> > + >> > +ovn-nbctl pg-add pg0 sw0-p1 sw0-p2 >> > +ovn-nbctl acl-add pg0 from-lport 1002 "inport == @pg0 && ip4" >> allow-related >> > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src >> == 0.0.0.0/0 && icmp4" allow-related >> > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src >> == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related >> > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src >> == 0.0.0.0/0 && udp && udp.dst == 80" allow-related >> > + >> > +# Create the second logical switch with one port >> > +ovn-nbctl ls-add sw1 >> > +ovn-nbctl lsp-add sw1 sw1-p1 >> > +ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3" >> > +ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3" >> > + >> > +# Create port group and ACLs for sw1 ports. >> > +ovn-nbctl pg-add pg1_drop sw1-p1 >> > +ovn-nbctl acl-add pg1_drop from-lport 1001 "inport == @pg1_drop && ip" >> drop >> > +ovn-nbctl acl-add pg1_drop to-lport 1001 "outport == @pg1_drop && ip" >> drop >> > + >> > +ovn-nbctl pg-add pg1 sw1-p1 >> > +ovn-nbctl acl-add pg1 from-lport 1002 "inport == @pg1 && ip4" >> allow-related >> > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src >> == 0.0.0.0/0 && icmp4" allow-related >> > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src >> == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related >> > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src >> == 0.0.0.0/0 && udp && udp.dst == 80" allow-related >> > + >> > +# Create a logical router and attach both logical switches >> > +ovn-nbctl lr-add lr0 >> > +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 >> > +ovn-nbctl lsp-add sw0 sw0-lr0 >> > +ovn-nbctl lsp-set-type sw0-lr0 router >> > +ovn-nbctl lsp-set-addresses sw0-lr0 router >> > +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 >> > + >> > +ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24 >> > +ovn-nbctl lsp-add sw1 sw1-lr0 >> > +ovn-nbctl lsp-set-type sw1-lr0 router >> > +ovn-nbctl lsp-set-addresses sw1-lr0 router >> > +ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1 >> > + >> > +ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.3:80,20.0.0.3:80 >> > +lb1_uuid=$(ovn-nbctl --bare --columns _uuid list load_balancer) >> > +ovn-nbctl ls-lb-add sw0 lb1 >> > +ovn-nbctl ls-lb-add sw1 lb1 >> > +ovn-nbctl lr-lb-add lr0 lb1 >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv1 ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=dp_hash" -c) -eq 1 >> > +]) >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv2 ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=dp_hash" -c) -eq 1 >> > +]) >> > + >> > +echo "lb1_uuid = $lb1_uuid" >> > + >> > +ovn-nbctl set load_balancer $lb1_uuid >> selection_fields="ip_src,ip_dst,tp_src,tp_dst" >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv1 ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" >> -c) -eq 1 >> > +]) >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv2 ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" >> -c) -eq 1 >> > +]) >> > + >> > +# Change the protocol to udp. >> > +ovn-nbctl set load_balancer $lb1_uuid protocol=udp >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv1 ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" >> -c) -eq 1 >> > +]) >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv2 ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" >> -c) -eq 1 >> > +]) >> > + >> > +# Change the protocol to udp. >> > +ovn-nbctl set load_balancer $lb1_uuid protocol=sctp >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv1 ovs-ofctl dump-groups br-int | \ >> > + grep >> "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 >> > +]) >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv2 ovs-ofctl dump-groups br-int | \ >> > + grep >> "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 >> > +]) >> > + >> > +ovn-nbctl set load_balancer $lb1_uuid >> selection_fields="ip_src,ip_dst,tp_dst" >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv1 ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) >> -eq 1 >> > +]) >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(as hv2 ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) >> -eq 1 >> > +]) >> > + >> > +OVN_CLEANUP([hv1], [hv2]) >> > +AT_CLEANUP >> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> > index 52f05f07e..2999e52fd 100644 >> > --- a/tests/system-ovn.at >> > +++ b/tests/system-ovn.at >> > @@ -1234,6 +1234,29 @@ else >> > AT_CHECK([test $bar3_ct -eq 0]) >> > fi >> > >> > +# Change the protocol of lb2 to udp and set tp_src and tp_dst. >> > +ovn-nbctl set load_balancer $lb2_uuid >> selection_fields="ip_src,ip_dst,tp_src,tp_dst" >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" >> -c) -eq 2 >> > +]) >> > + >> > +ovn-nbctl set load_balancer $lb2_uuid protocol=udp >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(ovs-ofctl dump-groups br-int | \ >> > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" >> -c) -eq 2 >> > +]) >> > + >> > +# Change the protocol of lb2 to sctp. >> > +ovn-nbctl set load_balancer $lb2_uuid protocol=sctp >> > + >> > +OVS_WAIT_UNTIL([ >> > + test $(ovs-ofctl dump-groups br-int | \ >> > + grep >> "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 2 >> > +]) >> > + >> > OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> > >> > as ovn-sb >> > >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >>
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 40aeb0a58..7887d0d2f 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -3121,6 +3121,7 @@ struct ovn_lb { const struct nbrec_load_balancer *nlb; /* May be NULL. */ char *selection_fields; + char *proto; struct lb_vip *vips; size_t n_vips; }; @@ -3218,6 +3219,12 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, struct smap_node *node; size_t n_vips = 0; + if (!nbrec_lb->protocol || !nbrec_lb->protocol[0]) { + lb->proto = xstrdup("tcp"); + } else { + lb->proto = xstrdup(nbrec_lb->protocol); + } + SMAP_FOR_EACH (node, &nbrec_lb->vips) { char *vip; uint16_t port; @@ -3234,7 +3241,7 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, lb->vips[n_vips].backend_ips = xstrdup(node->value); struct nbrec_load_balancer_health_check *lb_health_check = NULL; - if (nbrec_lb->protocol && !strcmp(nbrec_lb->protocol, "sctp")) { + if (!strcmp(lb->proto, "sctp")) { if (nbrec_lb->n_health_check > 0) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, @@ -3309,15 +3316,11 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, lb->vips[n_vips].backends[i].svc_mon_src_ip = svc_mon_src_ip; if (lb_health_check && op && svc_mon_src_ip) { - const char *protocol = nbrec_lb->protocol; - if (!protocol || !protocol[0]) { - protocol = "tcp"; - } lb->vips[n_vips].backends[i].health_check = true; struct service_monitor_info *mon_info = create_or_get_service_mon(ctx, monitor_map, backend_ip, op->nbsp->name, backend_port, - protocol); + lb->proto); ovs_assert(mon_info); sbrec_service_monitor_set_options( @@ -3351,7 +3354,14 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, if (lb->nlb->n_selection_fields) { struct ds sel_fields = DS_EMPTY_INITIALIZER; for (size_t i = 0; i < lb->nlb->n_selection_fields; i++) { - ds_put_format(&sel_fields, "%s,", lb->nlb->selection_fields[i]); + char *field = lb->nlb->selection_fields[i]; + if (!strcmp(field, "tp_src")) { + ds_put_format(&sel_fields, "%s_src,", lb->proto); + } else if (!strcmp(field, "tp_dst")) { + ds_put_format(&sel_fields, "%s_dst,", lb->proto); + } else { + ds_put_format(&sel_fields, "%s,", field); + } } ds_chomp(&sel_fields, ','); lb->selection_fields = ds_steal_cstr(&sel_fields); @@ -3374,6 +3384,7 @@ ovn_lb_destroy(struct ovn_lb *lb) free(lb->vips[i].backends); } + free(lb->proto); free(lb->vips); free(lb->selection_fields); } @@ -4820,7 +4831,7 @@ ls_has_dns_records(const struct nbrec_logical_switch *nbs) static void build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows, - struct lb_vip *lb_vip, + struct lb_vip *lb_vip, const char *proto, struct nbrec_load_balancer *lb, int pl, struct shash *meter_groups) { @@ -4837,11 +4848,11 @@ build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows, ds_put_format(&match, "ip%s.dst == %s && %s", lb_vip->addr_family == AF_INET ? "4": "6", - lb_vip->vip, lb->protocol); + lb_vip->vip, proto); char *vip = lb_vip->vip; if (lb_vip->vip_port) { - ds_put_format(&match, " && %s.dst == %u", lb->protocol, + ds_put_format(&match, " && %s.dst == %u", proto, lb_vip->vip_port); vip = xasprintf("%s:%u", lb_vip->vip, lb_vip->vip_port); } @@ -4851,7 +4862,7 @@ build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows, "protocol = \"%s\", " "load_balancer = \"" UUID_FMT "\");", event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS), - meter, vip, lb->protocol, + meter, vip, proto, UUID_ARGS(&lb->header_.uuid)); ovn_lflow_add_with_hint(lflows, od, pl, 130, ds_cstr(&match), action, &lb->header_); @@ -4906,7 +4917,7 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows, sset_add(&all_ips_v6, lb_vip->vip); } - build_empty_lb_event_flow(od, lflows, lb_vip, nb_lb, + build_empty_lb_event_flow(od, lflows, lb_vip, lb->proto, nb_lb, S_SWITCH_IN_PRE_LB, meter_groups); /* Ignore L4 port information in the key because fragmented packets @@ -5577,7 +5588,7 @@ build_lb(struct ovn_datapath *od, struct hmap *lflows) static void build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb, struct lb_vip *lb_vip, - const char *ip_match, const char *proto) + const char *ip_match) { if (lb_vip->n_backends == 0) { return; @@ -5605,7 +5616,7 @@ build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows, */ if (lb_vip->vip_port) { ds_put_format(&proto_match, " && %s.dst == %"PRIu16, - proto, backend->port); + lb->proto, backend->port); } ds_put_format(&match_initiator, "(%s.src == %s && %s.dst == %s%s)", ip_match, backend->ip, ip_match, backend->ip, @@ -5614,7 +5625,7 @@ build_lb_hairpin_rules(struct ovn_datapath *od, struct hmap *lflows, /* Replies to hairpinned traffic are originated by backend->ip:port. */ ds_clear(&proto_match); if (lb_vip->vip_port) { - ds_put_format(&proto_match, " && %s.src == %"PRIu16, proto, + ds_put_format(&proto_match, " && %s.src == %"PRIu16, lb->proto, backend->port); } ds_put_format(&match_reply, "(%s.src == %s%s)", ip_match, backend->ip, @@ -5665,18 +5676,6 @@ build_lb_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb) ip_match = "ip6"; } - const char *proto = NULL; - if (lb_vip->vip_port) { - proto = "tcp"; - if (lb->nlb->protocol) { - if (!strcmp(lb->nlb->protocol, "udp")) { - proto = "udp"; - } else if (!strcmp(lb->nlb->protocol, "sctp")) { - proto = "sctp"; - } - } - } - /* New connections in Ingress table. */ struct ds action = DS_EMPTY_INITIALIZER; build_lb_vip_ct_lb_actions(lb_vip, &action, lb->selection_fields); @@ -5684,7 +5683,8 @@ build_lb_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb) struct ds match = DS_EMPTY_INITIALIZER; ds_put_format(&match, "ct.new && %s.dst == %s", ip_match, lb_vip->vip); if (lb_vip->vip_port) { - ds_put_format(&match, " && %s.dst == %d", proto, lb_vip->vip_port); + ds_put_format(&match, " && %s.dst == %d", lb->proto, + lb_vip->vip_port); ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_STATEFUL, 120, ds_cstr(&match), ds_cstr(&action), &lb->nlb->header_); @@ -5701,7 +5701,7 @@ build_lb_rules(struct ovn_datapath *od, struct hmap *lflows, struct ovn_lb *lb) * a load balancer VIP is DNAT-ed to a backend that happens to be * the source of the traffic). */ - build_lb_hairpin_rules(od, lflows, lb, lb_vip, ip_match, proto); + build_lb_hairpin_rules(od, lflows, lb, lb_vip, ip_match); } } @@ -7667,7 +7667,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od, const char *proto, struct nbrec_load_balancer *lb, struct shash *meter_groups, struct sset *nat_entries) { - build_empty_lb_event_flow(od, lflows, lb_vip, lb, S_ROUTER_IN_DNAT, + build_empty_lb_event_flow(od, lflows, lb_vip, proto, lb, S_ROUTER_IN_DNAT, meter_groups); /* A match and actions for new connections. */ @@ -9358,14 +9358,9 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, } int prio = 110; - 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); + ds_put_format(&match, " && %s && %s.dst == %d", lb->proto, + lb->proto, lb_vip->vip_port); prio = 120; } @@ -9374,7 +9369,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, od->l3redirect_port->json_key); } add_router_lb_flow(lflows, od, &match, &actions, prio, - lb_force_snat_ip, lb_vip, proto, + lb_force_snat_ip, lb_vip, lb->proto, nb_lb, meter_groups, &nat_entries); } } diff --git a/tests/ovn.at b/tests/ovn.at index 8ee348397..a28563a5e 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1009,6 +1009,18 @@ ct_lb(backends=fd0f::2,fd0f::3; hash_fields="eth_src,eth_dst,ip_src,ip_dst,tp_sr encodes as group:5 uses group: id(5), name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,tp_src,tp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) has prereqs ip +ct_lb(backends=fd0f::2,fd0f::3; hash_fields="eth_src,eth_dst,ip_src,ip_dst,tcp_src,tcp_dst"); + encodes as group:6 + uses group: id(6), name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,tcp_src,tcp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) + has prereqs ip +ct_lb(backends=fd0f::2,fd0f::3; hash_fields="eth_src,eth_dst,ip_src,ip_dst,udp_src,udp_dst"); + encodes as group:7 + uses group: id(7), name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,udp_src,udp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) + has prereqs ip +ct_lb(backends=fd0f::2,fd0f::3; hash_fields="eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst"); + encodes as group:8 + uses group: id(8), name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15])) + has prereqs ip # ct_next ct_next; @@ -1543,13 +1555,13 @@ handle_svc_check(reg0); # select reg9[16..31] = select(1=50, 2=100, 3, ); formats as reg9[16..31] = select(1=50, 2=100, 3=100); - encodes as group:6 - uses group: id(6), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) + encodes as group:9 + uses group: id(9), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19)) reg0 = select(1, 2); formats as reg0 = select(1=100, 2=100); - encodes as group:7 - uses group: id(7), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) + encodes as group:10 + uses group: id(10), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19)) reg0 = select(1=, 2); Syntax error at `,' expecting weight. @@ -1565,12 +1577,12 @@ reg0[0..14] = select(1, 2, 3); cannot use 15-bit field reg0[0..14] for "select", which requires at least 16 bits. fwd_group(liveness="true", childports="eth0", "lsp1"); - encodes as group:8 - uses group: id(8), name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) + encodes as group:11 + uses group: id(11), name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) fwd_group(childports="eth0", "lsp1"); - encodes as group:9 - uses group: id(9), name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) + encodes as group:12 + uses group: id(12), name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64)) fwd_group(childports=eth0); Syntax error at `eth0' expecting logical switch port. @@ -19905,3 +19917,154 @@ OVS_WAIT_UNTIL([test 0 = $(ovn-sbctl show | grep Port_Binding -c)], [0]) OVN_CLEANUP([hv1]) AT_CLEANUP + +AT_SETUP([ovn -- Load balancer selection fields]) +AT_KEYWORDS([lb]) +ovn_start + +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 +ovs-vsctl -- add-port br-int hv1-vif2 -- \ + set interface hv1-vif2 external-ids:iface-id=sw0-p2 \ + options:tx_pcap=hv1/vif2-tx.pcap \ + options:rxq_pcap=hv1/vif2-rx.pcap \ + ofport-request=2 + +sim_add hv2 +as hv2 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.2 +ovs-vsctl -- add-port br-int hv2-vif1 -- \ + set interface hv2-vif1 external-ids:iface-id=sw1-p1 \ + options:tx_pcap=hv2/vif1-tx.pcap \ + options:rxq_pcap=hv2/vif1-rx.pcap \ + ofport-request=1 + +ovn-nbctl ls-add sw0 + +ovn-nbctl lsp-add sw0 sw0-p1 +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" + +ovn-nbctl lsp-add sw0 sw0-p2 +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" + +# Create port group and ACLs for sw0 ports. +ovn-nbctl pg-add pg0_drop sw0-p1 sw0-p2 +ovn-nbctl acl-add pg0_drop from-lport 1001 "inport == @pg0_drop && ip" drop +ovn-nbctl acl-add pg0_drop to-lport 1001 "outport == @pg0_drop && ip" drop + +ovn-nbctl pg-add pg0 sw0-p1 sw0-p2 +ovn-nbctl acl-add pg0 from-lport 1002 "inport == @pg0 && ip4" allow-related +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && icmp4" allow-related +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related + +# Create the second logical switch with one port +ovn-nbctl ls-add sw1 +ovn-nbctl lsp-add sw1 sw1-p1 +ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3" +ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3" + +# Create port group and ACLs for sw1 ports. +ovn-nbctl pg-add pg1_drop sw1-p1 +ovn-nbctl acl-add pg1_drop from-lport 1001 "inport == @pg1_drop && ip" drop +ovn-nbctl acl-add pg1_drop to-lport 1001 "outport == @pg1_drop && ip" drop + +ovn-nbctl pg-add pg1 sw1-p1 +ovn-nbctl acl-add pg1 from-lport 1002 "inport == @pg1 && ip4" allow-related +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && icmp4" allow-related +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related + +# Create a logical router and attach both logical switches +ovn-nbctl lr-add lr0 +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 +ovn-nbctl lsp-add sw0 sw0-lr0 +ovn-nbctl lsp-set-type sw0-lr0 router +ovn-nbctl lsp-set-addresses sw0-lr0 router +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 + +ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24 +ovn-nbctl lsp-add sw1 sw1-lr0 +ovn-nbctl lsp-set-type sw1-lr0 router +ovn-nbctl lsp-set-addresses sw1-lr0 router +ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1 + +ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.3:80,20.0.0.3:80 +lb1_uuid=$(ovn-nbctl --bare --columns _uuid list load_balancer) +ovn-nbctl ls-lb-add sw0 lb1 +ovn-nbctl ls-lb-add sw1 lb1 +ovn-nbctl lr-lb-add lr0 lb1 + +OVS_WAIT_UNTIL([ + test $(as hv1 ovs-ofctl dump-groups br-int | \ + grep "selection_method=dp_hash" -c) -eq 1 +]) + +OVS_WAIT_UNTIL([ + test $(as hv2 ovs-ofctl dump-groups br-int | \ + grep "selection_method=dp_hash" -c) -eq 1 +]) + +echo "lb1_uuid = $lb1_uuid" + +ovn-nbctl set load_balancer $lb1_uuid selection_fields="ip_src,ip_dst,tp_src,tp_dst" + +OVS_WAIT_UNTIL([ + test $(as hv1 ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 1 +]) + +OVS_WAIT_UNTIL([ + test $(as hv2 ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 1 +]) + +# Change the protocol to udp. +ovn-nbctl set load_balancer $lb1_uuid protocol=udp +OVS_WAIT_UNTIL([ + test $(as hv1 ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 1 +]) + +OVS_WAIT_UNTIL([ + test $(as hv2 ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 1 +]) + +# Change the protocol to udp. +ovn-nbctl set load_balancer $lb1_uuid protocol=sctp +OVS_WAIT_UNTIL([ + test $(as hv1 ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 +]) + +OVS_WAIT_UNTIL([ + test $(as hv2 ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 +]) + +ovn-nbctl set load_balancer $lb1_uuid selection_fields="ip_src,ip_dst,tp_dst" +OVS_WAIT_UNTIL([ + test $(as hv1 ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq 1 +]) + +OVS_WAIT_UNTIL([ + test $(as hv2 ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq 1 +]) + +OVN_CLEANUP([hv1], [hv2]) +AT_CLEANUP diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 52f05f07e..2999e52fd 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -1234,6 +1234,29 @@ else AT_CHECK([test $bar3_ct -eq 0]) fi +# Change the protocol of lb2 to udp and set tp_src and tp_dst. +ovn-nbctl set load_balancer $lb2_uuid selection_fields="ip_src,ip_dst,tp_src,tp_dst" + +OVS_WAIT_UNTIL([ + test $(ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 2 +]) + +ovn-nbctl set load_balancer $lb2_uuid protocol=udp + +OVS_WAIT_UNTIL([ + test $(ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 2 +]) + +# Change the protocol of lb2 to sctp. +ovn-nbctl set load_balancer $lb2_uuid protocol=sctp + +OVS_WAIT_UNTIL([ + test $(ovs-ofctl dump-groups br-int | \ + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 2 +]) + OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb