[ovs-dev] [PATCH ovn] northd: Always generate valid load balancer address set names.

Numan Siddique numans at ovn.org
Fri Oct 8 20:26:50 UTC 2021


On Fri, Oct 8, 2021 at 7:24 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> Address set names have a fixed format ([a-zA-Z_.][a-zA-Z_.0-9]*) while
> logical router names are free form.  This means we cannot directly embed
> the logical router name inside the address set name.
>
> To make sure that address set names are unique and valid use instead
> the Datapath_Binding.tunnel_key value which is guaranteed to be unique
> across logical routers.
>
> Fixes: c1e3896c0a39 ("northd: Use address sets for ARP responder flows for VIPs.")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>

Thanks.  I missed this out during reviews.  I applied this patch to
the main branch.

Numan

> ---
>  northd/northd.c     | 39 +++++++++++++++++++++++++++++++++++----
>  tests/ovn-northd.at | 27 +++++++++++++++------------
>  2 files changed, 50 insertions(+), 16 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index e42795ca0..c331f5fb1 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -878,6 +878,37 @@ lr_has_lb_vip(struct ovn_datapath *od)
>      return false;
>  }
>
> +/* Builds a unique address set compatible name ([a-zA-Z_.][a-zA-Z_.0-9]*)
> + * for the router's load balancer VIP address set, combining the logical
> + * router's datapath tunnel key and address family.
> + *
> + * Also prefixes the name with 'prefix'.
> + */
> +static char *
> +lr_lb_address_set_name_(const struct ovn_datapath *od, const char *prefix,
> +                        int addr_family)
> +{
> +    ovs_assert(od->nbr);
> +    return xasprintf("%s_rtr_lb_%"PRIu32"_ip%s", prefix, od->tunnel_key,
> +                     addr_family == AF_INET ? "4" : "6");
> +}
> +
> +/* Builds the router's load balancer VIP address set name. */
> +static char *
> +lr_lb_address_set_name(const struct ovn_datapath *od, int addr_family)
> +{
> +    return lr_lb_address_set_name_(od, "", addr_family);
> +}
> +
> +/* Builds a string that refers to the the router's load balancer VIP address
> + * set name, that is: $<address_set_name>.
> + */
> +static char *
> +lr_lb_address_set_ref(const struct ovn_datapath *od, int addr_family)
> +{
> +    return lr_lb_address_set_name_(od, "$", addr_family);
> +}
> +
>  static void
>  init_lb_for_datapath(struct ovn_datapath *od)
>  {
> @@ -12009,7 +12040,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
>              }
>
>              /* Create a single ARP rule for all IPs that are used as VIPs. */
> -            char *lb_ips_v4_as = xasprintf("$%s_lb_ip4", op->od->nbr->name);
> +            char *lb_ips_v4_as = lr_lb_address_set_ref(op->od, AF_INET);
>              build_lrouter_arp_flow(op->od, op, lb_ips_v4_as,
>                                     REG_INPORT_ETH_ADDR,
>                                     match, false, 90, NULL, lflows);
> @@ -12025,7 +12056,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
>              }
>
>              /* Create a single ND rule for all IPs that are used as VIPs. */
> -            char *lb_ips_v6_as = xasprintf("$%s_lb_ip6", op->od->nbr->name);
> +            char *lb_ips_v6_as = lr_lb_address_set_ref(op->od, AF_INET6);
>              build_lrouter_nd_flow(op->od, op, "nd_na", lb_ips_v6_as, NULL,
>                                    REG_INPORT_ETH_ADDR, match, false, 90,
>                                    NULL, lflows, meter_groups);
> @@ -13682,7 +13713,7 @@ sync_address_sets(struct northd_context *ctx, struct hmap *datapaths)
>          }
>
>          if (sset_count(&od->lb_ips_v4)) {
> -            char *ipv4_addrs_name = xasprintf("%s_lb_ip4", od->nbr->name);
> +            char *ipv4_addrs_name = lr_lb_address_set_name(od, AF_INET);
>              const char **ipv4_addrs = sset_array(&od->lb_ips_v4);
>
>              sync_address_set(ctx, ipv4_addrs_name, ipv4_addrs,
> @@ -13692,7 +13723,7 @@ sync_address_sets(struct northd_context *ctx, struct hmap *datapaths)
>          }
>
>          if (sset_count(&od->lb_ips_v6)) {
> -            char *ipv6_addrs_name = xasprintf("%s_lb_ip6", od->nbr->name);
> +            char *ipv6_addrs_name = lr_lb_address_set_name(od, AF_INET6);
>              const char **ipv6_addrs = sset_array(&od->lb_ips_v6);
>
>              sync_address_set(ctx, ipv6_addrs_name, ipv6_addrs,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index a31a5046f..8b9049899 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1701,10 +1701,13 @@ ovn-nbctl lr-lb-add lr lb4
>  ovn-nbctl lr-lb-add lr lb5
>
>  ovn-nbctl --wait=sb sync
> +lr_key=$(fetch_column sb:datapath_binding tunnel_key external_ids:name=lr)
> +lb_as_v4="_rtr_lb_${lr_key}_ip4"
> +lb_as_v6="_rtr_lb_${lr_key}_ip6"
>
>  # Check generated VIP address sets.
> -check_column '192.168.2.1 192.168.2.4 192.168.2.5 192.168.2.6' Address_Set addresses name=lr_lb_ip4
> -check_column 'fe80::200:ff:fe00:101 fe80::200:ff:fe00:102' Address_Set addresses name=lr_lb_ip6
> +check_column '192.168.2.1 192.168.2.4 192.168.2.5 192.168.2.6' Address_Set addresses name=${lb_as_v4}
> +check_column 'fe80::200:ff:fe00:101 fe80::200:ff:fe00:102' Address_Set addresses name=${lb_as_v6}
>
>  # Ingress router port ETH address is stored in lr_in_admission.
>  AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_admission.*xreg0\[[0..47\]]" | sort], [0], [dnl
> @@ -1723,7 +1726,7 @@ action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
>  ])
>
>  # Ingress router port ETH address is used for ARP reply/NA in lr_in_ip_input.
> -AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
> +AT_CHECK_UNQUOTED([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
>  match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
> @@ -1737,7 +1740,7 @@ action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */
>  match=(arp.op == 1 && arp.tpa == 43.43.43.4), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> -match=(inport == "lrp" && arp.op == 1 && arp.tpa == $lr_lb_ip4), dnl
> +match=(inport == "lrp" && arp.op == 1 && arp.tpa == \$${lb_as_v4}), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
>  match=(inport == "lrp" && arp.op == 1 && arp.tpa == 42.42.42.1 && arp.spa == 42.42.42.0/24), dnl
> @@ -1746,10 +1749,10 @@ action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */
>  match=(inport == "lrp" && ip6.dst == {fe80::200:ff:fe00:1, ff02::1:ff00:1} && nd_ns && nd.target == fe80::200:ff:fe00:1), dnl
>  action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> -match=(inport == "lrp" && nd_ns && nd.target == $lr_lb_ip6), dnl
> +match=(inport == "lrp" && nd_ns && nd.target == \$${lb_as_v6}), dnl
>  action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> -match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == $lr_lb_ip4), dnl
> +match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == \$${lb_as_v4}), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
>  match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == 43.43.43.1 && arp.spa == 43.43.43.0/24), dnl
> @@ -1758,7 +1761,7 @@ action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */
>  match=(inport == "lrp-public" && ip6.dst == {fe80::200:ff:fe00:100, ff02::1:ff00:100} && nd_ns && nd.target == fe80::200:ff:fe00:100), dnl
>  action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> -match=(inport == "lrp-public" && nd_ns && nd.target == $lr_lb_ip6), dnl
> +match=(inport == "lrp-public" && nd_ns && nd.target == \$${lb_as_v6}), dnl
>  action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
>  ])
>
> @@ -1792,7 +1795,7 @@ action=(xreg0[[0..47]] = 00:00:00:00:01:00; next;)
>  # Ingress router port is used for ARP reply/NA in lr_in_ip_input.
>  # xxreg0[0..47] is used unless external_mac is set.
>  # Priority 90 flows (per router).
> -AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
> +AT_CHECK_UNQUOTED([ovn-sbctl lflow-list | grep -E "lr_in_ip_input.*priority=90" | grep "arp\|nd" | sort], [0], [dnl
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
>  match=(arp.op == 1 && arp.tpa == 43.43.43.150), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
> @@ -1806,7 +1809,7 @@ action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */
>  match=(arp.op == 1 && arp.tpa == 43.43.43.4), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> -match=(inport == "lrp" && arp.op == 1 && arp.tpa == $lr_lb_ip4), dnl
> +match=(inport == "lrp" && arp.op == 1 && arp.tpa == \$${lb_as_v4}), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
>  match=(inport == "lrp" && arp.op == 1 && arp.tpa == 42.42.42.1 && arp.spa == 42.42.42.0/24), dnl
> @@ -1815,10 +1818,10 @@ action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */
>  match=(inport == "lrp" && ip6.dst == {fe80::200:ff:fe00:1, ff02::1:ff00:1} && nd_ns && nd.target == fe80::200:ff:fe00:1), dnl
>  action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> -match=(inport == "lrp" && nd_ns && nd.target == $lr_lb_ip6), dnl
> +match=(inport == "lrp" && nd_ns && nd.target == \$${lb_as_v6}), dnl
>  action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> -match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == $lr_lb_ip4 && is_chassis_resident("cr-lrp-public")), dnl
> +match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == \$${lb_as_v4} && is_chassis_resident("cr-lrp-public")), dnl
>  action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
>  match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == 43.43.43.1 && arp.spa == 43.43.43.0/24), dnl
> @@ -1827,7 +1830,7 @@ action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */
>  match=(inport == "lrp-public" && ip6.dst == {fe80::200:ff:fe00:100, ff02::1:ff00:100} && nd_ns && nd.target == fe80::200:ff:fe00:100 && is_chassis_resident("cr-lrp-public")), dnl
>  action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
>    table=3 (lr_in_ip_input     ), priority=90   , dnl
> -match=(inport == "lrp-public" && nd_ns && nd.target == $lr_lb_ip6 && is_chassis_resident("cr-lrp-public")), dnl
> +match=(inport == "lrp-public" && nd_ns && nd.target == \$${lb_as_v6} && is_chassis_resident("cr-lrp-public")), dnl
>  action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
>  ])
>
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list