diff mbox series

[ovs-dev,v8,5/6] northd: Add options to automatically add routes for NATs and LBs.

Message ID 20210603184931.1425441-6-mmichels@redhat.com
State Changes Requested
Headers show
Series ARP and Floating IP Fixes | expand

Commit Message

Mark Michelson June 3, 2021, 6:49 p.m. UTC
Load_Balancer and NAT entries have a new option, "add_route" that can be
set to automatically add routes to those addresses to neighbor routers,
therefore eliminating the need to create static routes.

Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
 northd/ovn-northd.c   | 29 +++++++++++++++++++++--------
 northd/ovn_northd.dl  | 23 ++++++++++++++++-------
 ovn-nb.xml            | 29 ++++++++++++++++++++++++++++-
 tests/ovn-nbctl.at    |  3 +++
 tests/ovn-northd.at   | 40 ++++++++++++++++++++++++++++++++++++----
 utilities/ovn-nbctl.c | 25 +++++++++++++++++++------
 6 files changed, 123 insertions(+), 26 deletions(-)

Comments

0-day Robot June 3, 2021, 7:04 p.m. UTC | #1
Bleep bloop.  Greetings Mark Michelson, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line has trailing whitespace
#239 FILE: ovn-nb.xml:2051:
          load balancer IP addresses to the appropriate MAC address. Setting 

WARNING: Line lacks whitespace around operator
#365 FILE: utilities/ovn-nbctl.c:370:
  [--add-route]\n\

Lines checked: 448, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Dumitru Ceara June 24, 2021, 2:42 p.m. UTC | #2
On 6/3/21 8:49 PM, Mark Michelson wrote:
> Load_Balancer and NAT entries have a new option, "add_route" that can be
> set to automatically add routes to those addresses to neighbor routers,
> therefore eliminating the need to create static routes.
> 
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> ---

Hi Mark,

Looks like this series needs a rebase because it's conflicting with the
newly added:

  d27509de940a ("northd: Precompute load balancer IP sets.")

Sorry about that.

I think there are ways to keep precomputing the load balancer IPs and
not use get_router_load_balancer_ips().  Probably the quickest is to
store two sets of load balancer IPs per datapath, "all-ips" and
"ips-from-load-balancers-with-add-route-set".  There might be
alternatives though.

Regards,
Dumitru
diff mbox series

Patch

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3b9cad80b..414bf9c48 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -1454,13 +1454,14 @@  destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
     free(ra->laddrs);
 }
 
-static char **get_nat_addresses(const struct ovn_port *op, size_t *n);
+static char **get_nat_addresses(const struct ovn_port *op, size_t *n,
+                                bool routable_only);
 
 static void
 assign_routable_addresses(struct ovn_port *op)
 {
     size_t n;
-    char **nats = get_nat_addresses(op, &n);
+    char **nats = get_nat_addresses(op, &n, true);
 
     if (!nats) {
         return;
@@ -2510,7 +2511,7 @@  join_logical_ports(struct northd_context *ctx,
 }
 
 static void
-get_router_load_balancer_ips(const struct ovn_datapath *od,
+get_router_load_balancer_ips(const struct ovn_datapath *od, bool routable_only,
                              struct sset *all_ips_v4, struct sset *all_ips_v6)
 {
     if (!od->nbr) {
@@ -2522,6 +2523,11 @@  get_router_load_balancer_ips(const struct ovn_datapath *od,
         struct smap *vips = &lb->vips;
         struct smap_node *node;
 
+        if (routable_only &&
+            !smap_get_bool(&lb->options, "add_route", false)) {
+            continue;
+        }
+
         SMAP_FOR_EACH (node, vips) {
             /* node->key contains IP:port or just IP. */
             char *ip_address;
@@ -2560,7 +2566,7 @@  get_router_load_balancer_ips(const struct ovn_datapath *od,
  * The caller must free each of the n returned strings with free(),
  * and must free the returned array when it is no longer needed. */
 static char **
-get_nat_addresses(const struct ovn_port *op, size_t *n)
+get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only)
 {
     size_t n_nats = 0;
     struct eth_addr mac;
@@ -2583,6 +2589,12 @@  get_nat_addresses(const struct ovn_port *op, size_t *n)
         const struct nbrec_nat *nat = op->od->nbr->nat[i];
         ovs_be32 ip, mask;
 
+        if (routable_only &&
+            (!strcmp(nat->type, "snat") ||
+             !smap_get_bool(&nat->options, "add_route", false))) {
+            continue;
+        }
+
         char *error = ip_parse_masked(nat->external_ip, &ip, &mask);
         if (error || mask != OVS_BE32_MAX) {
             free(error);
@@ -2636,7 +2648,8 @@  get_nat_addresses(const struct ovn_port *op, size_t *n)
     /* Two sets to hold all load-balancer vips. */
     struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4);
     struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6);
-    get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6);
+    get_router_load_balancer_ips(op->od, routable_only,
+                                 &all_ips_v4, &all_ips_v6);
 
     const char *ip_address;
     SSET_FOR_EACH (ip_address, &all_ips_v4) {
@@ -3159,7 +3172,7 @@  ovn_port_update_sbrec(struct northd_context *ctx,
             if (nat_addresses && !strcmp(nat_addresses, "router")) {
                 if (op->peer && op->peer->od
                     && (chassis || op->peer->od->l3redirect_port)) {
-                    nats = get_nat_addresses(op->peer, &n_nats);
+                    nats = get_nat_addresses(op->peer, &n_nats, false);
                 }
             /* Only accept manual specification of ethernet address
              * followed by IPv4 addresses on type "l3gateway" ports. */
@@ -6615,7 +6628,7 @@  build_lswitch_rport_arp_req_flows(struct ovn_port *op,
     struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4);
     struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6);
 
-    get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6);
+    get_router_load_balancer_ips(op->od, false, &all_ips_v4, &all_ips_v6);
 
     const char *ip_addr;
     const char *ip_addr_next;
@@ -11177,7 +11190,7 @@  build_lrouter_ipv4_ip_input(struct ovn_port *op,
         /* A set to hold all load-balancer vips that need ARP responses. */
         struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4);
         struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6);
-        get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6);
+        get_router_load_balancer_ips(op->od, false, &all_ips_v4, &all_ips_v6);
 
         const char *ip_address;
         if (sset_count(&all_ips_v4)) {
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 610bfbfb2..794b86ee1 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -201,7 +201,7 @@  OutProxy_Port_Binding(._uuid              = lsp._uuid,
             Some{"router"} -> match ((l3dgw_port, opt_chassis, peer)) {
                                  (None, None, _) -> set_empty(),
                                  (_, _, None) -> set_empty(),
-                                 (_, _, Some{rport}) -> get_nat_addresses(rport)
+                                 (_, _, Some{rport}) -> get_nat_addresses(rport, false)
                               },
             Some{nat_addresses} -> {
                 /* Only accept manual specification of ethernet address
@@ -298,12 +298,16 @@  OutProxy_Port_Binding(._uuid              = lrp._uuid,
     }.
 /*
 */
-function get_router_load_balancer_ips(router: Intern<Router>) :
+function get_router_load_balancer_ips(router: Intern<Router>,
+                                      routable_only: bool) :
     (Set<string>, Set<string>) =
 {
     var all_ips_v4 = set_empty();
     var all_ips_v6 = set_empty();
     for (lb in router.lbs) {
+        if (routable_only and not lb.options.get_bool_def("add_route", false)) {
+            continue;
+        };
         for (kv in lb.vips) {
             (var vip, _) = kv;
             /* node->key contains IP:port or just IP. */
@@ -325,7 +329,7 @@  function get_router_load_balancer_ips(router: Intern<Router>) :
  * external IP addresses of all NAT rules defined on that router, and all
  * of the IP addresses used in load balancer VIPs defined on that router.
  */
-function get_nat_addresses(rport: Intern<RouterPort>): Set<string> =
+function get_nat_addresses(rport: Intern<RouterPort>, routable_only: bool): Set<string> =
 {
     var addresses = set_empty();
     var has_redirect = rport.router.l3dgw_port.is_some();
@@ -337,6 +341,11 @@  function get_nat_addresses(rport: Intern<RouterPort>): Set<string> =
 
             /* Get NAT IP addresses. */
             for (nat in rport.router.nats) {
+                if (routable_only and
+                    (nat.nat.__type == "snat" or
+                     not nat.nat.options.get_bool_def("add_route", false))) {
+                    continue;
+                };
                 /* Determine whether this NAT rule satisfies the conditions for
                  * distributed NAT processing. */
                 if (has_redirect and nat.nat.__type == "dnat_and_snat" and
@@ -379,7 +388,7 @@  function get_nat_addresses(rport: Intern<RouterPort>): Set<string> =
             };
 
             /* A set to hold all load-balancer vips. */
-            (var all_ips_v4, var all_ips_v6) = get_router_load_balancer_ips(rport.router);
+            (var all_ips_v4, var all_ips_v6) = get_router_load_balancer_ips(rport.router, routable_only);
 
             for (ip_address in set_union(all_ips_v4, all_ips_v6)) {
                 c_addresses = c_addresses ++ " ${ip_address}";
@@ -4099,7 +4108,7 @@  function get_arp_forward_ips(rp: Intern<RouterPort>): (Set<string>, Set<string>)
     var all_ips_v6 = set_empty();
 
     (var lb_ips_v4, var lb_ips_v6)
-        = get_router_load_balancer_ips(rp.router);
+        = get_router_load_balancer_ips(rp.router, false);
     for (a in lb_ips_v4) {
         /* Check if the ovn port has a network configured on which we could
          * expect ARP requests for the LB VIP.
@@ -5091,7 +5100,7 @@  var residence_check = match (is_redirect) {
     true -> Some{"is_chassis_resident(${router.redirect_port_name})"},
     false -> None
 } in {
-    (var all_ips_v4, _) = get_router_load_balancer_ips(router) in {
+    (var all_ips_v4, _) = get_router_load_balancer_ips(router, false) in {
         if (not all_ips_v4.is_empty()) {
             LogicalRouterArpFlow(.lr = router,
                                  .lrp = Some{lrp},
@@ -6505,7 +6514,7 @@  relation RouterPortRoutableAddresses(
     addresses: Set<lport_addresses>)
 RouterPortRoutableAddresses(port.lrp._uuid, addresses) :-
     port in &RouterPort(.is_redirect = true),
-    var addresses = get_nat_addresses(port).filter_map(extract_addresses),
+    var addresses = get_nat_addresses(port, true).filter_map(extract_addresses),
     addresses != set_empty().
 
 /* Return a vector of pairs (1, set[0]), ... (n, set[n - 1]). */
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 47f25eac1..6edf62d98 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -1697,6 +1697,16 @@ 
         option, the force_snat_for_lb option configured for the router
         pipeline will not be applied for this load balancer.
       </column>
+
+      <column name="options" key="add_route">
+        If set to <code>true</code>, then neighbor routers will have logical
+        flows added that will allow for routing to the VIP IP. It also will
+        have ARP resolution logical flows added. By setting this option, it
+        means there is no reason to create a
+        <ref table="Logical_Router_Static_Route"/> from neighbor routers to
+        this NAT address. It also means that no ARP request is required for
+        neighbor routers to learn the IP-MAC mapping for this VIP IP.
+      </column>
     </group>
   </table>
 
@@ -2034,7 +2044,13 @@ 
           <code>false</code> by default.  It is recommended to set to
           <code>true</code> when a large number of logical routers are
           connected to the same logical switch but most of them never need to
-          send traffic between each other.
+          send traffic between each other. By default, ovn-northd does not
+          create mappings to NAT and load balancer addresess. However, for NAT
+          and load balancer addresses that have the <code>add_route</code>
+          option added, ovn-northd will create logical flows that map NAT and
+          load balancer IP addresses to the appropriate MAC address. Setting 
+          <var>dynamic_neigh_routers</var> to <code>true</code> will prevent
+          the automatic creation of these logical flows.
         </p>
       </column>
       <column name="options" key="always_learn_from_arp_request" type='{"type": "boolean"}'>
@@ -3007,6 +3023,17 @@ 
       tracking state or not.
     </column>
 
+    <column name="options" key="add_route">
+      If set to <code>true</code>, then neighbor routers will have logical
+      flows added that will allow for routing to the NAT address. It also will
+      have ARP resolution logical flows added. By setting this option, it means
+      there is no reason to create a <ref table="Logical_Router_Static_Route"/>
+      from neighbor routers to this NAT address. It also means that no ARP
+      request is required for neighbor routers to learn the IP-MAC mapping for
+      this NAT address. This option only applies to NATs of type
+      <code>dnat</code> and <code>dnat_and_snat</code>.
+    </column>
+
     <group title="Common Columns">
       <column name="external_ids">
         See <em>External IDs</em> at the beginning of this document.
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 1058d418a..828777b82 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -465,6 +465,9 @@  AT_CHECK([ovn-nbctl lsp-add ls0 lp0])
 AT_CHECK([ovn-nbctl lr-nat-add lr0 dnat_and_snat 30.0.0.2 192.168.1.2 lp0 00:00:00:01:02], [1], [],
 [ovn-nbctl: invalid mac address 00:00:00:01:02.
 ])
+AT_CHECK([ovn-nbctl --add-route lr-nat-add lr0 snat 30.0.0.2 192.168.1.2], [1], [],
+[ovn-nbctl: routes cannot be added for snat types.
+])
 
 dnl Add snat and dnat
 AT_CHECK([ovn-nbctl lr-nat-add lr0 snat 30.0.0.1 192.168.1.0/24])
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 12a8e47b4..9e4f2ca62 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3694,11 +3694,20 @@  check ovn-nbctl lr-nat-add ro2 dnat 20.0.0.100 192.168.2.100
 
 check_lflows 0
 
-AS_BOX([Checking that NAT flows are installed for gateway routers])
+AS_BOX([Checking that non-routable NAT flows are not installed for gateway routers])
 
 check ovn-nbctl lrp-set-gateway-chassis ro1-sw hv1 100
 check ovn-nbctl --wait=sb lrp-set-gateway-chassis ro2-sw hv2 100
 
+check_lflows 0
+
+AS_BOX([Checking that routable NAT flows are installed when gateway chassis exists])
+
+check ovn-nbctl lr-nat-del ro1
+check ovn-nbctl lr-nat-del ro2
+check ovn-nbctl --add-route lr-nat-add ro1 dnat 10.0.0.100 192.168.1.100
+check ovn-nbctl --add-route lr-nat-add ro2 dnat 20.0.0.100 192.168.2.100
+
 check_lflows 1
 
 AS_BOX([Checking that NAT flows are not installed for routers with gateway chassis removed])
@@ -3732,11 +3741,20 @@  check ovn-nbctl lr-nat-add ro2 dnat_and_snat 20.0.0.100 192.168.2.2 vm2 00:00:00
 
 check_lflows 0
 
-AS_BOX([Checking that Floating IP NAT flows are installed for gateway routers])
+AS_BOX([Checking that non-routable Floating IP NAT flows are not installed for gateway routers])
 
 check ovn-nbctl lrp-set-gateway-chassis ro1-sw hv1 100
 check ovn-nbctl --wait=sb lrp-set-gateway-chassis ro2-sw hv2 100
 
+check_lflows 0
+
+AS_BOX([Checking that routable Floating IP NAT flows are installed for gateway routers])
+check ovn-nbctl lr-nat-del ro1
+check ovn-nbctl lr-nat-del ro2
+
+check ovn-nbctl --add-route lr-nat-add ro1 dnat_and_snat 10.0.0.100 192.168.1.2 vm1 00:00:00:00:00:01
+check ovn-nbctl --add-route lr-nat-add ro2 dnat_and_snat 20.0.0.100 192.168.2.2 vm2 00:00:00:00:00:02
+
 check_lflows 1
 
 AS_BOX([Checking that Floating IP NAT flows are not installed for routers with gateway chassis removed])
@@ -3772,15 +3790,29 @@  check ovn-nbctl lb-add lb1 10.0.0.100 192.168.1.2
 check ovn-nbctl lr-lb-add ro1 lb1
 
 check ovn-nbctl lb-add lb2 20.0.0.100 192.168.2.2
-check ovn-nbctl lr-lb-add ro2 lb2
+check ovn-nbctl --wait=sb lr-lb-add ro2 lb2
 
 check_lflows 0
 
-AS_BOX([Checking that Load Balancer VIP flows are installed for gateway routers])
+AS_BOX([Checking that non-routable Load Balancer VIP flows are not installed for gateway routers])
 
 check ovn-nbctl lrp-set-gateway-chassis ro1-sw hv1 100
 check ovn-nbctl --wait=sb lrp-set-gateway-chassis ro2-sw hv2 100
 
+check_lflows 0
+
+AS_BOX([Checking that routable Load Balancer VIP flows are installed for gateway routers])
+
+check ovn-nbctl lr-lb-del ro1 lb1
+check ovn-nbctl lr-lb-del ro2 lb2
+check ovn-nbctl lb-del lb1
+check ovn-nbctl lb-del lb2
+
+check ovn-nbctl --add-route lb-add lb1 10.0.0.100 192.168.1.2
+check ovn-nbctl --add-route lb-add lb2 20.0.0.100 192.168.2.2
+check ovn-nbctl lr-lb-add ro1 lb1
+check ovn-nbctl --wait=sb lr-lb-add ro2 lb2
+
 check_lflows 1
 
 AS_BOX([Checking that Load Balancer VIP flows are not installed for routers with gateway chassis removed])
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index dc13fa9ca..f4a859495 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -367,6 +367,7 @@  Policy commands:\n\
 NAT commands:\n\
   [--stateless]\n\
   [--portrange]\n\
+  [--add-route]\n\
   lr-nat-add ROUTER TYPE EXTERNAL_IP LOGICAL_IP [LOGICAL_PORT EXTERNAL_MAC]\n\
                             [EXTERNAL_PORT_RANGE]\n\
                             add a NAT to ROUTER\n\
@@ -2703,6 +2704,7 @@  nbctl_lb_add(struct ctl_context *ctx)
     bool add_duplicate = shash_find(&ctx->options, "--add-duplicate") != NULL;
     bool empty_backend_rej = shash_find(&ctx->options, "--reject") != NULL;
     bool empty_backend_event = shash_find(&ctx->options, "--event") != NULL;
+    bool add_route = shash_find(&ctx->options, "--add-route") != NULL;
 
     if (empty_backend_event && empty_backend_rej) {
             ctl_error(ctx,
@@ -2822,14 +2824,18 @@  nbctl_lb_add(struct ctl_context *ctx)
     smap_add(CONST_CAST(struct smap *, &lb->vips),
             lb_vip_normalized, ds_cstr(&lb_ips_new));
     nbrec_load_balancer_set_vips(lb, &lb->vips);
+    struct smap options = SMAP_INITIALIZER(&options);
     if (empty_backend_rej) {
-        const struct smap options = SMAP_CONST1(&options, "reject", "true");
-        nbrec_load_balancer_set_options(lb, &options);
+        smap_add(&options, "reject", "true");
     }
     if (empty_backend_event) {
-        const struct smap options = SMAP_CONST1(&options, "event", "true");
-        nbrec_load_balancer_set_options(lb, &options);
+        smap_add(&options, "event", "true");
+    }
+    if (add_route) {
+        smap_add(&options, "add_route", "true");
     }
+    nbrec_load_balancer_set_options(lb, &options);
+    smap_destroy(&options);
 out:
     ds_destroy(&lb_ips_new);
 
@@ -4400,6 +4406,7 @@  nbctl_lr_nat_add(struct ctl_context *ctx)
     char *new_logical_ip = NULL;
     char *new_external_ip = NULL;
     bool is_portrange = shash_find(&ctx->options, "--portrange") != NULL;
+    bool add_route = shash_find(&ctx->options, "--add-route") != NULL;
 
     char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
     if (error) {
@@ -4516,6 +4523,11 @@  nbctl_lr_nat_add(struct ctl_context *ctx)
     }
 
     int is_snat = !strcmp("snat", nat_type);
+
+    if (is_snat && add_route) {
+        ctl_error(ctx, "routes cannot be added for snat types.");
+        goto cleanup;
+    }
     for (size_t i = 0; i < lr->n_nat; i++) {
         const struct nbrec_nat *nat = lr->nat[i];
 
@@ -4596,6 +4608,7 @@  nbctl_lr_nat_add(struct ctl_context *ctx)
     }
 
     smap_add(&nat_options, "stateless", stateless ? "true":"false");
+    smap_add(&nat_options, "add_route", add_route ? "true": "false");
     nbrec_nat_set_options(nat, &nat_options);
 
     smap_destroy(&nat_options);
@@ -6714,7 +6727,7 @@  static const struct ctl_command_syntax nbctl_commands[] = {
       "ROUTER TYPE EXTERNAL_IP LOGICAL_IP"
       "[LOGICAL_PORT EXTERNAL_MAC] [EXTERNAL_PORT_RANGE]",
       nbctl_pre_lr_nat_add, nbctl_lr_nat_add,
-      NULL, "--may-exist,--stateless,--portrange", RW },
+      NULL, "--may-exist,--stateless,--portrange,--add-route", RW },
     { "lr-nat-del", 1, 3, "ROUTER [TYPE [IP]]",
       nbctl_pre_lr_nat_del, nbctl_lr_nat_del, NULL, "--if-exists", RW },
     { "lr-nat-list", 1, 1, "ROUTER", nbctl_pre_lr_nat_list,
@@ -6725,7 +6738,7 @@  static const struct ctl_command_syntax nbctl_commands[] = {
     /* load balancer commands. */
     { "lb-add", 3, 4, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]",
       nbctl_pre_lb_add, nbctl_lb_add, NULL,
-      "--may-exist,--add-duplicate,--reject,--event", RW },
+      "--may-exist,--add-duplicate,--reject,--event,--add-route", RW },
     { "lb-del", 1, 2, "LB [VIP]", nbctl_pre_lb_del, nbctl_lb_del, NULL,
         "--if-exists", RW },
     { "lb-list", 0, 1, "[LB]", nbctl_pre_lb_list, nbctl_lb_list, NULL, "", RO },