On Tue, Mar 28, 2023 at 4:31 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.
>
> Fixes: b45853fba816 ("lb: Support using templates.")
> Signed-off-by: Dumitru Ceara <[email protected]>
> Acked-by: Ales Musil <[email protected]>
> (cherry picked from commit 7310a9859a6a5a9ad4faff1e5620e5cfa142eb7d)
> ---
> NOTE: this is a custom backport because it picks up the tests added by
> 086744a645a7 ("northd: Use LB port_str in northd."). It doesn't pick up
> the rest of the changes from that commit because they're not applicable
> on branch-22.12.
> ---
> lib/lb.c | 51 ++++++++++++++---
> lib/lb.h | 6 +-
> tests/ovn-nbctl.at | 26 +++++++++
> tests/system-ovn.at | 126 ++++++++++++++++++++++++++++++++++++------
> utilities/ovn-nbctl.c | 2 +-
> 5 files changed, 182 insertions(+), 29 deletions(-)
>
> diff --git a/lib/lb.c b/lib/lb.c
> index c13d07b99c..b74e4d90c1 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -36,6 +36,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)
> @@ -236,6 +237,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);
> }
> @@ -283,8 +286,27 @@ ovn_lb_vip_init_template(struct ovn_lb_vip *lb_vip,
> const char *lb_key_,
> lb_key_);
> }
>
> + /* Backends can either be templates or explicit IPs and ports. */
> lb_vip->address_family = address_family;
> - return ovn_lb_backends_init_template(lb_vip, lb_value);
> + lb_vip->template_backends = true;
> + char *template_error = ovn_lb_backends_init_template(lb_vip,
> lb_value);
> +
> + if (template_error) {
> + lb_vip->template_backends = false;
> + ovn_lb_backends_clear(lb_vip);
> +
> + char *explicit_error = ovn_lb_backends_init_explicit(lb_vip,
> lb_value);
> + if (explicit_error) {
> + char *error =
> + xasprintf("invalid backend: template (%s) OR explicit
> (%s)",
> + template_error, explicit_error);
> + free(explicit_error);
> + free(template_error);
> + return error;
> + }
> + free(template_error);
> + }
> + return NULL;
> }
>
> /* Returns NULL on success, an error string on failure. The caller is
> @@ -302,15 +324,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);
> }
>
> @@ -355,11 +391,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 55becc1bf7..e5a3875609 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -98,6 +98,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;
> };
> @@ -203,7 +206,6 @@ 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);
>
> #endif /* OVN_LIB_LB_H 1 */
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 8885ac9fcc..8233617536 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -1473,6 +1473,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 1e67678465..1c7fdcb8d0 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -9410,6 +9410,7 @@ AT_CLEANUP
>
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([load-balancer template IPv4])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> AT_SKIP_IF([test $HAVE_NC = no])
> AT_KEYWORDS([ovnlb templates])
>
> @@ -9436,8 +9437,8 @@ start_daemon ovn-controller
> # |
> # VM2 ----+
> #
> -# A templated load balancer applied on LS1 and GW-Router with
> -# VM1 as backend. The VIP should be accessible from both VM2 and VM3.
> +# 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 \
> -- lr-add rtr \
> @@ -9458,15 +9459,34 @@ check ovn-nbctl
> \
> -- lsp-set-options ls2-rtr router-port=rtr-ls2 \
> -- lsp-add ls2 vm3 -- lsp-set-addresses vm3 00:00:00:00:00:03
>
> -# Add a template LB that eventually expands to:
> +# Add a TCP template LB that eventually expands to:
> # VIP=66.66.66.66:666 backends=42.42.42.2:4242 proto=tcp
> +# And a UDP template LB that eventually expands to:
> +# 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,vport=666,backends=\"42.42.42.2:4242\"}"],
> +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\",vport3=888,vport4=999}"],
> [0], [ignore])
>
> -check ovn-nbctl --template lb-add lb-test "^vip:^vport" "^backends" tcp \
> - -- ls-lb-add ls1 lb-test \
> - -- lr-lb-add rtr lb-test
> +check ovn-nbctl --template lb-add lb-test-tcp "^vip:^vport1" "^backends1"
> tcp \
> + -- ls-lb-add ls1 lb-test-tcp
> \
> + -- lr-lb-add rtr lb-test-tcp
> +
> +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")
> @@ -9483,24 +9503,50 @@ check ovn-nbctl --wait=hv sync
>
> AT_CHECK([ovn-appctl -t ovn-controller debug/dump-local-template-vars |
> sort], [0], [dnl
> Local template vars:
> -name: 'backends' value: '42.42.42.2:4242'
> +name: 'backends1' value: '42.42.42.2:4242'
> +name: 'backends2' value: '42.42.42.2:4343'
> name: 'vip' value: '66.66.66.66'
> -name: 'vport' value: '666'
> +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 -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).
> NS_CHECK_EXEC([vm1], [nc 66.66.66.66 666 -z], [0], [ignore], [ignore])
> NS_CHECK_EXEC([vm2], [nc 66.66.66.66 666 -z], [0], [ignore], [ignore])
> NS_CHECK_EXEC([vm3], [nc 66.66.66.66 666 -z], [0], [ignore], [ignore])
>
> +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 "6"
> +])
> +
> AT_CLEANUP
> ])
>
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([load-balancer template IPv6])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> AT_SKIP_IF([test $HAVE_NC = no])
> AT_KEYWORDS([ovnlb templates])
>
> @@ -9527,8 +9573,8 @@ start_daemon ovn-controller
> # |
> # VM2 ----+
> #
> -# A templated load balancer applied on LS1 and GW-Router with
> -# VM1 as backend. The VIP should be accessible from both VM2 and VM3.
> +# 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 \
> -- lr-add rtr \
> @@ -9550,14 +9596,33 @@ check ovn-nbctl
> \
> -- lsp-add ls2 vm3 -- lsp-set-addresses vm3 00:00:00:00:00:03
>
> # Add a template LB that eventually expands to:
> -# VIP=6666::1 backends=[4242::2]:4242 proto=tcp
> +# VIP=[6666::1]:666 backends=[4242::2]:4242 proto=tcp
> +# Add a template LB that eventually expands to:
> +# VIP=[6666::1]:777 backends=[4242::2]:4343 proto=udp
>
> -AT_CHECK([ovn-nbctl -- create chassis_template_var chassis="hv1"
> variables="{vip=\"6666::1\",vport=666,backends=\"[[4242::2]]:4242\"}"],
> +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\",vport3=888,vport4=999}"],
> [0], [ignore])
>
> -check ovn-nbctl --template lb-add lb-test "^vip:^vport" "^backends" tcp
> ipv6 \
> - -- ls-lb-add ls1 lb-test
> \
> - -- lr-lb-add rtr lb-test
> +check ovn-nbctl --template lb-add lb-test-tcp "^vip:^vport1" "^backends1"
> tcp ipv6 \
> + -- ls-lb-add ls1 lb-test-tcp
> \
> + -- lr-lb-add rtr lb-test-tcp
> +
> +check ovn-nbctl --template lb-add lb-test-udp "^vip:^vport2" "^backends2"
> udp ipv6 \
> + -- 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")
> @@ -9577,19 +9642,44 @@ check ovn-nbctl --wait=hv sync
>
> AT_CHECK([ovn-appctl -t ovn-controller debug/dump-local-template-vars |
> sort], [0], [dnl
> Local template vars:
> -name: 'backends' value: '[[4242::2]]:4242'
> +name: 'backends1' value: '[[4242::2]]:4242'
> +name: 'backends2' value: '[[4242::2]]:4343'
> name: 'vip' value: '6666::1'
> -name: 'vport' value: '666'
> +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 -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).
> NS_CHECK_EXEC([vm1], [nc 6666::1 666 -z], [0], [ignore], [ignore])
> NS_CHECK_EXEC([vm2], [nc 6666::1 666 -z], [0], [ignore], [ignore])
> NS_CHECK_EXEC([vm3], [nc 6666::1 666 -z], [0], [ignore], [ignore])
>
> +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 "6"
> +])
> +
> AT_CLEANUP
> ])
>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 9d4fb8c757..699fa4ff42 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -3026,7 +3026,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
>
>
Looks good to me, thanks.
Acked-by: Ales Musil <[email protected]>
--
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
