On Fri, Mar 24, 2023 at 1:41 PM Dumitru Ceara <[email protected]> wrote:
> This was working fine for IPv4 but partially by accident because IPv4 > addresses don't contain ':'. Fix it for the general case by trying to > parse explicit backends if parsing templates fails. > > Also add unit and system tests for all supported cases. > > Signed-off-by: Dumitru Ceara <[email protected]> > Hi Dumitru, just one comment below. > --- > lib/lb.c | 50 ++++++++++++++++++++++++++++++------ > lib/lb.h | 6 +++-- > tests/ovn-nbctl.at | 26 +++++++++++++++++++ > tests/system-ovn.at | 60 +++++++++++++++++++++++++++++++++++++------ > utilities/ovn-nbctl.c | 2 +- > 5 files changed, 125 insertions(+), 19 deletions(-) > > diff --git a/lib/lb.c b/lib/lb.c > index 66c8152750..1543f6279b 100644 > --- a/lib/lb.c > +++ b/lib/lb.c > @@ -38,6 +38,7 @@ static const char *lb_neighbor_responder_mode_names[] = { > static struct nbrec_load_balancer_health_check * > ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb, > const char *vip_port_str, bool template); > +static void ovn_lb_backends_clear(struct ovn_lb_vip *vip); > > struct ovn_lb_ip_set * > ovn_lb_ip_set_create(void) > @@ -238,6 +239,8 @@ ovn_lb_backends_init_template(struct ovn_lb_vip > *lb_vip, const char *value_) > ds_put_format(&errors, "%s: should be a template of the form: > " > > "'^backendip_variable1[:^port_variable1|:port]', ", > atom); > + free(backend_port); > + free(backend_ip); > } > free(atom); > } > @@ -286,7 +289,25 @@ ovn_lb_vip_init_template(struct ovn_lb_vip *lb_vip, > const char *lb_key_, > } > > lb_vip->address_family = address_family; > - return ovn_lb_backends_init_template(lb_vip, lb_value); > + lb_vip->template_backends = true; > + char *error = ovn_lb_backends_init_template(lb_vip, lb_value); > + char *result = error; > + > + if (error) { > + char *error2; > + > + ovn_lb_backends_clear(lb_vip); > + error2 = ovn_lb_backends_init_explicit(lb_vip, lb_value); > + lb_vip->template_backends = false; > + > + if (error2) { > + free(error2); > + } else { > + free(error); > + result = NULL; > + } > + } > + return result; > This error handling part is confusing. AFAICT we could first try the 'ovn_lb_backends_init_explicit' if that fails, proceed to 'ovn_lb_backends_init_template' and share the error message from the second operation without the juggling of error2. Other than that it looks good. > } > > /* Returns NULL on success, an error string on failure. The caller is > @@ -304,15 +325,29 @@ ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const > char *lb_key, > address_family); > } > > -void > -ovn_lb_vip_destroy(struct ovn_lb_vip *vip) > +static void > +ovn_lb_backends_destroy(struct ovn_lb_vip *vip) > { > - free(vip->vip_str); > - free(vip->port_str); > for (size_t i = 0; i < vip->n_backends; i++) { > free(vip->backends[i].ip_str); > free(vip->backends[i].port_str); > } > +} > + > +static void > +ovn_lb_backends_clear(struct ovn_lb_vip *vip) > +{ > + ovn_lb_backends_destroy(vip); > + vip->backends = NULL; > + vip->n_backends = 0; > +} > + > +void > +ovn_lb_vip_destroy(struct ovn_lb_vip *vip) > +{ > + free(vip->vip_str); > + free(vip->port_str); > + ovn_lb_backends_destroy(vip); > free(vip->backends); > } > > @@ -357,11 +392,10 @@ ovn_lb_vip_format(const struct ovn_lb_vip *vip, > struct ds *s, bool template) > } > > void > -ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s, > - bool template) > +ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s) > { > bool needs_brackets = vip->address_family == AF_INET6 && vip->port_str > - && !template; > + && !vip->template_backends; > for (size_t i = 0; i < vip->n_backends; i++) { > struct ovn_lb_backend *backend = &vip->backends[i]; > > diff --git a/lib/lb.h b/lib/lb.h > index ddd41703da..e24f519dbb 100644 > --- a/lib/lb.h > +++ b/lib/lb.h > @@ -96,6 +96,9 @@ struct ovn_lb_vip { > */ > struct ovn_lb_backend *backends; > size_t n_backends; > + bool template_backends; /* True if the backends are templates. False > if > + * they're explicitly specified. > + */ > bool empty_backend_rej; > int address_family; > }; > @@ -211,8 +214,7 @@ char *ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const > char *lb_key, > void ovn_lb_vip_destroy(struct ovn_lb_vip *vip); > void ovn_lb_vip_format(const struct ovn_lb_vip *vip, struct ds *s, > bool template); > -void ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds > *s, > - bool template); > +void ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds > *s); > > struct ovn_lb_5tuple { > struct hmap_node hmap_node; > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 2fffe18500..aa5baade40 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -1482,6 +1482,32 @@ UUID LB > PROTO VIP > > dnl --------------------------------------------------------------------- > > +OVN_NBCTL_TEST([ovn_nbctl_template_lbs], [Template LBs], [ > +check ovn-nbctl --template lb-add lb0 ^vip ^backend > +check ovn-nbctl --template lb-add lb1 ^vip:^vport ^backend udp > +check ovn-nbctl --template lb-add lb2 ^vip:^vport ^backend udp ipv4 > +check ovn-nbctl --template lb-add lb3 ^vip:^vport ^backend udp ipv6 > +check ovn-nbctl --template lb-add lb4 ^vip:^vport ^backend:^bport udp ipv4 > +check ovn-nbctl --template lb-add lb5 ^vip:^vport ^backend:^bport udp ipv6 > +check ovn-nbctl --template lb-add lb6 ^vip:^vport 1.1.1.1:111 udp ipv4 > +check ovn-nbctl --template lb-add lb7 ^vip:^vport [[1::1]]:111 udp ipv6 > + > +AT_CHECK([ovn-nbctl lb-list | uuidfilt], [0], [dnl > +UUID LB PROTO > VIP IPs > +<0> lb0 tcp ^vip ^backend > +<1> lb1 udp ^vip:^vport ^backend > +<2> lb2 udp ^vip:^vport ^backend > +<3> lb3 udp ^vip:^vport ^backend > +<4> lb4 udp ^vip:^vport ^backend:^bport > +<5> lb5 udp ^vip:^vport ^backend:^bport > +<6> lb6 udp ^vip:^vport 1.1.1.1:111 > +<7> lb7 udp ^vip:^vport [[1::1]]:111 > +]) > + > +]) > + > +dnl --------------------------------------------------------------------- > + > OVN_NBCTL_TEST([ovn_nbctl_basic_lr], [basic logical router commands], [ > AT_CHECK([ovn-nbctl lr-add lr0]) > AT_CHECK([ovn-nbctl lr-list | uuidfilt], [0], [dnl > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 8afb4db564..53daf27f23 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -9045,7 +9045,7 @@ start_daemon ovn-controller > # | > # VM2 ----+ > # > -# Two templated load balancer applied on LS1 and GW-Router with > +# Four templated load balancer applied on LS1 and GW-Router with > # VM1 as backend. The VIPs should be accessible from both VM2 and VM3. > > check ovn-nbctl \ > @@ -9073,7 +9073,7 @@ check ovn-nbctl > \ > # VIP=66.66.66.66:777 backends=42.42.42.2:4343 proto=udp > > AT_CHECK([ovn-nbctl -- create chassis_template_var chassis="hv1" \ > - variables="{vip=66.66.66.66,vport1=666,backends1=\"42.42.42.2:4242 > \",vport2=777,backends2=\"42.42.42.2:4343\"}"], > + variables="{vip=66.66.66.66,vport1=666,backends1=\"42.42.42.2:4242 > \",vport2=777,backends2=\"42.42.42.2:4343\",vport3=888,vport4=999}"], > [0], [ignore]) > > check ovn-nbctl --template lb-add lb-test-tcp "^vip:^vport1" "^backends1" > tcp \ > @@ -9084,6 +9084,18 @@ check ovn-nbctl --template lb-add lb-test-udp > "^vip:^vport2" "^backends2" udp \ > -- ls-lb-add ls1 lb-test-udp > \ > -- lr-lb-add rtr lb-test-udp > > +# Add a TCP template LB with explicit backends that eventually expands to: > +# VIP=66.66.66.66:888 backends=42.42.42.2:4242 proto=tcp > +# And a UDP template LB that eventually expands to: > +# VIP=66.66.66.66:999 backends=42.42.42.2:4343 proto=udp > +check ovn-nbctl --template lb-add lb-test-tcp2 "^vip:^vport3" " > 42.42.42.2:4242" tcp ipv4 \ > + -- ls-lb-add ls1 lb-test-tcp2 > \ > + -- lr-lb-add rtr lb-test-tcp2 > + > +check ovn-nbctl --template lb-add lb-test-udp2 "^vip:^vport4" " > 42.42.42.2:4343" udp ipv4 \ > + -- ls-lb-add ls1 lb-test-udp2 > \ > + -- lr-lb-add rtr lb-test-udp2 > + > ADD_NAMESPACES(vm1) > ADD_VETH(vm1, vm1, br-int, "42.42.42.2/24", "00:00:00:00:00:01", > "42.42.42.1") > > @@ -9104,13 +9116,15 @@ name: 'backends2' value: '42.42.42.2:4343' > name: 'vip' value: '66.66.66.66' > name: 'vport1' value: '666' > name: 'vport2' value: '777' > +name: 'vport3' value: '888' > +name: 'vport4' value: '999' > ]) > > # Start IPv4 TCP server on vm1. > NETNS_DAEMONIZE([vm1], [nc -k -l 42.42.42.2 4242], [nc-vm1.pid]) > > NETNS_DAEMONIZE([vm1], > - [tcpdump -n -i vm1 -nnleX -c3 udp and dst 42.42.42.2 and dst port > 4343 > vm1.pcap 2>/dev/null], > + [tcpdump -n -i vm1 -nnleX -c6 udp and dst 42.42.42.2 and dst port > 4343 > vm1.pcap 2>/dev/null], > [tcpdump1.pid]) > > # Make sure connecting to the VIP works (hairpin, via ls and via lr). > @@ -9122,9 +9136,17 @@ NS_CHECK_EXEC([vm1], [echo a | nc -u 66.66.66.66 > 777 &], [0]) > NS_CHECK_EXEC([vm2], [echo a | nc -u 66.66.66.66 777 &], [0]) > NS_CHECK_EXEC([vm3], [echo a | nc -u 66.66.66.66 777 &], [0]) > > +NS_CHECK_EXEC([vm1], [nc 66.66.66.66 888 -z], [0], [ignore], [ignore]) > +NS_CHECK_EXEC([vm2], [nc 66.66.66.66 888 -z], [0], [ignore], [ignore]) > +NS_CHECK_EXEC([vm3], [nc 66.66.66.66 888 -z], [0], [ignore], [ignore]) > + > +NS_CHECK_EXEC([vm1], [echo a | nc -u 66.66.66.66 999 &], [0]) > +NS_CHECK_EXEC([vm2], [echo a | nc -u 66.66.66.66 999 &], [0]) > +NS_CHECK_EXEC([vm3], [echo a | nc -u 66.66.66.66 999 &], [0]) > + > OVS_WAIT_UNTIL([ > requests=`grep "UDP" -c vm1.pcap` > - test "${requests}" -ge "3" > + test "${requests}" -ge "6" > ]) > > AT_CLEANUP > @@ -9159,7 +9181,7 @@ start_daemon ovn-controller > # | > # VM2 ----+ > # > -# Two templated load balancer applied on LS1 and GW-Router with > +# Four templated load balancer applied on LS1 and GW-Router with > # VM1 as backend. The VIPs should be accessible from both VM2 and VM3. > > check ovn-nbctl \ > @@ -9187,7 +9209,7 @@ check ovn-nbctl > \ > # VIP=[6666::1]:777 backends=[4242::2]:4343 proto=udp > > AT_CHECK([ovn-nbctl -- create chassis_template_var chassis="hv1" \ > - > variables="{vip=\"6666::1\",vport1=666,backends1=\"[[4242::2]]:4242\",vport2=777,backends2=\"[[4242::2]]:4343\"}"], > + > variables="{vip=\"6666::1\",vport1=666,backends1=\"[[4242::2]]:4242\",vport2=777,backends2=\"[[4242::2]]:4343\",vport3=888,vport4=999}"], > [0], [ignore]) > > check ovn-nbctl --template lb-add lb-test-tcp "^vip:^vport1" "^backends1" > tcp ipv6 \ > @@ -9198,6 +9220,18 @@ check ovn-nbctl --template lb-add lb-test-udp > "^vip:^vport2" "^backends2" udp ip > -- ls-lb-add ls1 lb-test-udp > \ > -- lr-lb-add rtr lb-test-udp > > +# Add a TCP template LB with explicit backends that eventually expands to: > +# VIP=[6666::1]:888 backends=[4242::2]:4242 proto=tcp > +# And a UDP template LB that eventually expands to: > +# VIP=[6666::1]:999 backends=[4242::2]:4343 proto=udp > +check ovn-nbctl --template lb-add lb-test-tcp2 "^vip:^vport3" > "[[4242::2]]:4242" tcp ipv6 \ > + -- ls-lb-add ls1 lb-test-tcp2 > \ > + -- lr-lb-add rtr lb-test-tcp2 > + > +check ovn-nbctl --template lb-add lb-test-udp2 "^vip:^vport4" > "[[4242::2]]:4343" udp ipv6 \ > + -- ls-lb-add ls1 lb-test-udp2 > \ > + -- lr-lb-add rtr lb-test-udp2 > + > ADD_NAMESPACES(vm1) > ADD_VETH(vm1, vm1, br-int, "4242::2/64", "00:00:00:00:00:01", "4242::1") > OVS_WAIT_UNTIL([test "$(ip netns exec vm1 ip a | grep 4242::2 | grep > tentative)" = ""]) > @@ -9221,13 +9255,15 @@ name: 'backends2' value: '[[4242::2]]:4343' > name: 'vip' value: '6666::1' > name: 'vport1' value: '666' > name: 'vport2' value: '777' > +name: 'vport3' value: '888' > +name: 'vport4' value: '999' > ]) > > # Start IPv6 TCP server on vm1. > NETNS_DAEMONIZE([vm1], [nc -k -l 4242::2 4242], [nc-vm1.pid]) > > NETNS_DAEMONIZE([vm1], > - [tcpdump -n -i vm1 -nnleX -c3 udp and dst 4242::2 and dst port 4343 > > vm1.pcap 2>/dev/null], > + [tcpdump -n -i vm1 -nnleX -c6 udp and dst 4242::2 and dst port 4343 > > vm1.pcap 2>/dev/null], > [tcpdump1.pid]) > > # Make sure connecting to the VIP works (hairpin, via ls and via lr). > @@ -9239,9 +9275,17 @@ NS_CHECK_EXEC([vm1], [echo a | nc -u 6666::1 777 > &], [0]) > NS_CHECK_EXEC([vm2], [echo a | nc -u 6666::1 777 &], [0]) > NS_CHECK_EXEC([vm3], [echo a | nc -u 6666::1 777 &], [0]) > > +NS_CHECK_EXEC([vm1], [nc 6666::1 888 -z], [0], [ignore], [ignore]) > +NS_CHECK_EXEC([vm2], [nc 6666::1 888 -z], [0], [ignore], [ignore]) > +NS_CHECK_EXEC([vm3], [nc 6666::1 888 -z], [0], [ignore], [ignore]) > + > +NS_CHECK_EXEC([vm1], [echo a | nc -u 6666::1 999 &], [0]) > +NS_CHECK_EXEC([vm2], [echo a | nc -u 6666::1 999 &], [0]) > +NS_CHECK_EXEC([vm3], [echo a | nc -u 6666::1 999 &], [0]) > + > OVS_WAIT_UNTIL([ > requests=`grep "UDP" -c vm1.pcap` > - test "${requests}" -ge "3" > + test "${requests}" -ge "6" > ]) > > AT_CLEANUP > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 45572fd304..22313ecd55 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -3033,7 +3033,7 @@ nbctl_lb_add(struct ctl_context *ctx) > } > > ovn_lb_vip_format(&lb_vip_parsed, &lb_vip_normalized, template); > - ovn_lb_vip_backends_format(&lb_vip_parsed, &lb_ips_new, template); > + ovn_lb_vip_backends_format(&lb_vip_parsed, &lb_ips_new); > ovn_lb_vip_destroy(&lb_vip_parsed); > > const struct nbrec_load_balancer *lb = NULL; > -- > 2.31.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
