diff mbox series

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

Message ID 20211102193101.615.23638.stgit@dceara.remote.csb
State Accepted
Headers show
Series Improve Load Balancer performance. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Dumitru Ceara Nov. 2, 2021, 7:31 p.m. UTC 
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@redhat.com>
(cherry picked from commit beed00c9206d439c89da3bcaf924163abf606215)
---
 northd/northd.c     |   39 +++++++++++++++++++++++++++++++++++----
 tests/ovn-northd.at |   27 +++++++++++++++------------
 2 files changed, 50 insertions(+), 16 deletions(-)
diff mbox series

Patch

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; };)
 ])