diff mbox series

[ovs-dev,ovn] Fix selection fields for UDP and SCTP load balancers.

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

Commit Message

Numan Siddique June 22, 2020, 5:41 p.m. UTC 
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(-)

Comments

Mark Michelson June 29, 2020, 3:17 p.m. UTC | #1 
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
>
Numan Siddique June 29, 2020, 3:59 p.m. UTC | #2 
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
>
>
Numan Siddique June 30, 2020, 5:23 a.m. UTC | #3 
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 mbox series

Patch

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