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

Dumitru Ceara dceara at redhat.com
Tue Nov 2 19:31:03 UTC 2021


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>
(cherry picked from commit beed00c9206d439c89da3bcaf924163abf606215)
---
 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 c1d83548e..d237f693d 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)
 {
@@ -12040,7 +12071,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);
@@ -12056,7 +12087,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);
@@ -13687,7 +13718,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,
@@ -13697,7 +13728,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 166723502..3f21edee9 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; };)
 ])
 



More information about the dev mailing list