diff mbox series

[ovs-dev,v4,ovn,4/5] ovn-northd: Refactor NAT address parsing.

Message ID 20200703183434.22921.95770.stgit@dceara.remote.csb
State Accepted
Headers show
Series Reduce number of flows in IN_IP_INPUT table for DNAT. | expand

Commit Message

Dumitru Ceara July 3, 2020, 6:34 p.m. UTC 
Store NAT entries pointers in ovn_datapath and pre-parse the external IP
addresses. This simplifies the code and makes it easier to reuse the parsed
external IP and solicited-node address without reparsing.

Acked-by: Han Zhou <hzhou@ovn.org>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 northd/ovn-northd.c |  119 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 89 insertions(+), 30 deletions(-)

Comments

Numan Siddique July 6, 2020, 10:56 a.m. UTC | #1 
On Sat, Jul 4, 2020 at 12:05 AM Dumitru Ceara <dceara@redhat.com> wrote:

> Store NAT entries pointers in ovn_datapath and pre-parse the external IP
> addresses. This simplifies the code and makes it easier to reuse the parsed
> external IP and solicited-node address without reparsing.
>
> Acked-by: Han Zhou <hzhou@ovn.org>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>

Hi Dumitru for the patches and Han for the Acks.

I applied the first 4 patches of this series to master with the below small
change in patch 3.
I swapped the last 2 params of the
functions build_lrouter_arp_flow/build_lrouter_nd_flow.

I think you need to resubmit p5 of this series after resolving the
conflicts.

********
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 14e121593..cbe6a1771 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -7986,7 +7986,8 @@ static void
 build_lrouter_arp_flow(struct ovn_datapath *od, struct ovn_port *op,
                        const char *ip_address, const char *eth_addr,
                        struct ds *extra_match, uint16_t priority,
-                       struct hmap *lflows, const struct ovsdb_idl_row
*hint)
+                       const struct ovsdb_idl_row *hint,
+                       struct hmap *lflows)
 {
     struct ds match = DS_EMPTY_INITIALIZER;
     struct ds actions = DS_EMPTY_INITIALIZER;
@@ -8033,8 +8034,8 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct
ovn_port *op,
                       const char *action, const char *ip_address,
                       const char *sn_ip_address, const char *eth_addr,
                       struct ds *extra_match, uint16_t priority,
-                      struct hmap *lflows,
-                      const struct ovsdb_idl_row *hint)
+                      const struct ovsdb_idl_row *hint,
+                      struct hmap *lflows)
 {
     struct ds match = DS_EMPTY_INITIALIZER;
     struct ds actions = DS_EMPTY_INITIALIZER;
@@ -8408,8 +8409,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,

             build_lrouter_arp_flow(op->od, op,
                                    op->lrp_networks.ipv4_addrs[i].addr_s,
-                                   REG_INPORT_ETH_ADDR, &match, 90, lflows,
-                                   &op->nbrp->header_);
+                                   REG_INPORT_ETH_ADDR, &match, 90,
+                                   &op->nbrp->header_, lflows);
         }

         /* A set to hold all load-balancer vips that need ARP responses. */
@@ -8427,7 +8428,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,

             build_lrouter_arp_flow(op->od, op,
                                    ip_address, REG_INPORT_ETH_ADDR,
-                                   &match, 90, lflows, NULL);
+                                   &match, 90, NULL, lflows);
         }

         SSET_FOR_EACH (ip_address, &all_ips_v6) {
@@ -8439,7 +8440,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,

             build_lrouter_nd_flow(op->od, op, "nd_na",
                                   ip_address, NULL, REG_INPORT_ETH_ADDR,
-                                  &match, 90, lflows, NULL);
+                                  &match, 90, NULL, lflows);
         }

         sset_destroy(&all_ips_v4);
@@ -8543,11 +8544,11 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
                 build_lrouter_nd_flow(op->od, op, "nd_na",
                                        nat->external_ip, sn_addr_s,
                                        mac_s, &match, 90,
-                                       lflows, &nat->header_);
+                                       &nat->header_, lflows);
             } else {
                 build_lrouter_arp_flow(op->od, op,
                                        nat->external_ip, mac_s, &match, 90,
-                                       lflows, &nat->header_);
+                                       &nat->header_, lflows);
             }
         }

@@ -8734,8 +8735,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
             build_lrouter_nd_flow(op->od, op, "nd_na_router",
                                   op->lrp_networks.ipv6_addrs[i].addr_s,
                                   op->lrp_networks.ipv6_addrs[i].sn_addr_s,
-                                  REG_INPORT_ETH_ADDR, &match, 90, lflows,
-                                  &op->nbrp->header_);
+                                  REG_INPORT_ETH_ADDR, &match, 90,
+                                  &op->nbrp->header_, lflows);
         }

         /* UDP/TCP port unreachable */

**************

Thanks
Numan

---
>  northd/ovn-northd.c |  119
> ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 89 insertions(+), 30 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 14e1215..2dc4e20 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -607,6 +607,9 @@ struct ovn_datapath {
>       * the "redirect-chassis". */
>      struct ovn_port *l3redirect_port;
>
> +    /* NAT entries configured on the router. */
> +    struct ovn_nat *nat_entries;
> +
>      struct ovn_port **localnet_ports;
>      size_t n_localnet_ports;
>
> @@ -619,6 +622,69 @@ struct ovn_datapath {
>      struct hmap nb_pgs;
>  };
>
> +/* Contains a NAT entry with the external addresses pre-parsed. */
> +struct ovn_nat {
> +    const struct nbrec_nat *nb;
> +    struct lport_addresses ext_addrs;
> +};
> +
> +/* Returns true if a 'nat_entry' is valid, i.e.:
> + * - parsing was successful.
> + * - the string yielded exactly one IPv4 address or exactly one IPv6
> address.
> + */
> +static bool
> +nat_entry_is_valid(const struct ovn_nat *nat_entry)
> +{
> +    const struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> +
> +    return (ext_addrs->n_ipv4_addrs == 1 && ext_addrs->n_ipv6_addrs == 0)
> ||
> +        (ext_addrs->n_ipv4_addrs == 0 && ext_addrs->n_ipv6_addrs == 1);
> +}
> +
> +static bool
> +nat_entry_is_v6(const struct ovn_nat *nat_entry)
> +{
> +    return nat_entry->ext_addrs.n_ipv6_addrs > 0;
> +}
> +
> +static void
> +init_nat_entries(struct ovn_datapath *od)
> +{
> +    if (!od->nbr || od->nbr->n_nat == 0) {
> +        return;
> +    }
> +
> +    od->nat_entries = xmalloc(od->nbr->n_nat * sizeof *od->nat_entries);
> +
> +    for (size_t i = 0; i < od->nbr->n_nat; i++) {
> +        const struct nbrec_nat *nat = od->nbr->nat[i];
> +        struct ovn_nat *nat_entry = &od->nat_entries[i];
> +
> +        nat_entry->nb = nat;
> +        if (!extract_ip_addresses(nat->external_ip,
> +                                  &nat_entry->ext_addrs) ||
> +                !nat_entry_is_valid(nat_entry)) {
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +
> +            VLOG_WARN_RL(&rl,
> +                         "Bad ip address %s in nat configuration "
> +                         "for router %s", nat->external_ip,
> od->nbr->name);
> +        }
> +    }
> +}
> +
> +static void
> +destroy_nat_entries(struct ovn_datapath *od)
> +{
> +    if (!od->nbr) {
> +        return;
> +    }
> +
> +    for (size_t i = 0; i < od->nbr->n_nat; i++) {
> +        destroy_lport_addresses(&od->nat_entries[i].ext_addrs);
> +    }
> +}
> +
>  /* A group of logical router datapaths which are connected - either
>   * directly or indirectly.
>   * Each logical router can belong to only one group. */
> @@ -676,6 +742,8 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
> ovn_datapath *od)
>          ovn_destroy_tnlids(&od->port_tnlids);
>          bitmap_free(od->ipam_info.allocated_ipv4s);
>          free(od->router_ports);
> +        destroy_nat_entries(od);
> +        free(od->nat_entries);
>          free(od->localnet_ports);
>          ovn_ls_port_group_destroy(&od->nb_pgs);
>          destroy_mcast_info_for_datapath(od);
> @@ -1104,6 +1172,7 @@ join_datapaths(struct northd_context *ctx, struct
> hmap *datapaths,
>              ovs_list_push_back(nb_only, &od->list);
>          }
>          init_mcast_info_for_datapath(od);
> +        init_nat_entries(od);
>          ovs_list_push_back(lr_list, &od->lr_list);
>      }
>  }
> @@ -8465,30 +8534,24 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>              snat_ips[n_snat_ips++] = snat_ip;
>          }
>
> -        for (int i = 0; i < op->od->nbr->n_nat; i++) {
> -            const struct nbrec_nat *nat;
> -
> -            nat = op->od->nbr->nat[i];
> +        for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> +            struct ovn_nat *nat_entry = &op->od->nat_entries[i];
> +            const struct nbrec_nat *nat = nat_entry->nb;
>
> -            ovs_be32 ip;
> -            struct in6_addr ipv6;
> -            bool is_v6 = false;
> -            if (!ip_parse(nat->external_ip, &ip) || !ip) {
> -                if (!ipv6_parse(nat->external_ip, &ipv6)) {
> -                    static struct vlog_rate_limit rl =
> -                        VLOG_RATE_LIMIT_INIT(5, 1);
> -                    VLOG_WARN_RL(&rl, "bad ip address %s in nat
> configuration "
> -                                 "for router %s", nat->external_ip,
> op->key);
> -                    continue;
> -                }
> -                is_v6 = true;
> +            /* Skip entries we failed to parse. */
> +            if (!nat_entry_is_valid(nat_entry)) {
> +                continue;
>              }
>
>              if (!strcmp(nat->type, "snat")) {
> -                if (is_v6) {
> +                if (nat_entry_is_v6(nat_entry)) {
> +                    struct in6_addr *ipv6 =
> +                        &nat_entry->ext_addrs.ipv6_addrs[0].addr;
> +
>                      snat_ips[n_snat_ips].family = AF_INET6;
> -                    snat_ips[n_snat_ips++].ipv6 = ipv6;
> +                    snat_ips[n_snat_ips++].ipv6 = *ipv6;
>                  } else {
> +                    ovs_be32 ip = nat_entry->ext_addrs.ipv4_addrs[0].addr;
>                      snat_ips[n_snat_ips].family = AF_INET;
>                      snat_ips[n_snat_ips++].ipv4 = ip;
>                  }
> @@ -8531,22 +8594,18 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                      }
>                  }
>              }
> -            if (is_v6) {
> -                /* For ND solicitations, we need to listen for both the
> -                 * unicast IPv6 address and its all-nodes multicast
> address,
> -                 * but always respond with the unicast IPv6 address. */
> -                char sn_addr_s[INET6_ADDRSTRLEN + 1];
> -                struct in6_addr sn_addr;
> -                in6_addr_solicited_node(&sn_addr, &ipv6);
> -                ipv6_string_mapped(sn_addr_s, &sn_addr);
>
> +            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> +            if (nat_entry_is_v6(nat_entry)) {
>                  build_lrouter_nd_flow(op->od, op, "nd_na",
> -                                       nat->external_ip, sn_addr_s,
> -                                       mac_s, &match, 90,
> -                                       lflows, &nat->header_);
> +                                      ext_addrs->ipv6_addrs[0].addr_s,
> +                                      ext_addrs->ipv6_addrs[0].sn_addr_s,
> +                                      mac_s, &match, 90,
> +                                      lflows, &nat->header_);
>              } else {
>                  build_lrouter_arp_flow(op->od, op,
> -                                       nat->external_ip, mac_s, &match,
> 90,
> +                                       ext_addrs->ipv4_addrs[0].addr_s,
> +                                       mac_s, &match, 90,
>                                         lflows, &nat->header_);
>              }
>          }
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Dumitru Ceara July 6, 2020, 11:33 a.m. UTC | #2 
On 7/6/20 12:56 PM, Numan Siddique wrote:
> 
> 
> On Sat, Jul 4, 2020 at 12:05 AM Dumitru Ceara <dceara@redhat.com
> <mailto:dceara@redhat.com>> wrote:
> 
>     Store NAT entries pointers in ovn_datapath and pre-parse the external IP
>     addresses. This simplifies the code and makes it easier to reuse the
>     parsed
>     external IP and solicited-node address without reparsing.
> 
>     Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>>
>     Signed-off-by: Dumitru Ceara <dceara@redhat.com
>     <mailto:dceara@redhat.com>>
> 
> 
> Hi Dumitru for the patches and Han for the Acks.
> 
> I applied the first 4 patches of this series to master with the below
> small change in patch 3.
> I swapped the last 2 params of the
> functions build_lrouter_arp_flow/build_lrouter_nd_flow.
> 
> I think you need to resubmit p5 of this series after resolving the
> conflicts.
> 

Thanks Han, Numan!

I sent a v5 with patch 5 here:
https://patchwork.ozlabs.org/project/openvswitch/patch/1594035141-21430-1-git-send-email-dceara@redhat.com/

Thanks,
Dumitru
diff mbox series

Patch

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 14e1215..2dc4e20 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -607,6 +607,9 @@  struct ovn_datapath {
      * the "redirect-chassis". */
     struct ovn_port *l3redirect_port;
 
+    /* NAT entries configured on the router. */
+    struct ovn_nat *nat_entries;
+
     struct ovn_port **localnet_ports;
     size_t n_localnet_ports;
 
@@ -619,6 +622,69 @@  struct ovn_datapath {
     struct hmap nb_pgs;
 };
 
+/* Contains a NAT entry with the external addresses pre-parsed. */
+struct ovn_nat {
+    const struct nbrec_nat *nb;
+    struct lport_addresses ext_addrs;
+};
+
+/* Returns true if a 'nat_entry' is valid, i.e.:
+ * - parsing was successful.
+ * - the string yielded exactly one IPv4 address or exactly one IPv6 address.
+ */
+static bool
+nat_entry_is_valid(const struct ovn_nat *nat_entry)
+{
+    const struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
+
+    return (ext_addrs->n_ipv4_addrs == 1 && ext_addrs->n_ipv6_addrs == 0) ||
+        (ext_addrs->n_ipv4_addrs == 0 && ext_addrs->n_ipv6_addrs == 1);
+}
+
+static bool
+nat_entry_is_v6(const struct ovn_nat *nat_entry)
+{
+    return nat_entry->ext_addrs.n_ipv6_addrs > 0;
+}
+
+static void
+init_nat_entries(struct ovn_datapath *od)
+{
+    if (!od->nbr || od->nbr->n_nat == 0) {
+        return;
+    }
+
+    od->nat_entries = xmalloc(od->nbr->n_nat * sizeof *od->nat_entries);
+
+    for (size_t i = 0; i < od->nbr->n_nat; i++) {
+        const struct nbrec_nat *nat = od->nbr->nat[i];
+        struct ovn_nat *nat_entry = &od->nat_entries[i];
+
+        nat_entry->nb = nat;
+        if (!extract_ip_addresses(nat->external_ip,
+                                  &nat_entry->ext_addrs) ||
+                !nat_entry_is_valid(nat_entry)) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+
+            VLOG_WARN_RL(&rl,
+                         "Bad ip address %s in nat configuration "
+                         "for router %s", nat->external_ip, od->nbr->name);
+        }
+    }
+}
+
+static void
+destroy_nat_entries(struct ovn_datapath *od)
+{
+    if (!od->nbr) {
+        return;
+    }
+
+    for (size_t i = 0; i < od->nbr->n_nat; i++) {
+        destroy_lport_addresses(&od->nat_entries[i].ext_addrs);
+    }
+}
+
 /* A group of logical router datapaths which are connected - either
  * directly or indirectly.
  * Each logical router can belong to only one group. */
@@ -676,6 +742,8 @@  ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od)
         ovn_destroy_tnlids(&od->port_tnlids);
         bitmap_free(od->ipam_info.allocated_ipv4s);
         free(od->router_ports);
+        destroy_nat_entries(od);
+        free(od->nat_entries);
         free(od->localnet_ports);
         ovn_ls_port_group_destroy(&od->nb_pgs);
         destroy_mcast_info_for_datapath(od);
@@ -1104,6 +1172,7 @@  join_datapaths(struct northd_context *ctx, struct hmap *datapaths,
             ovs_list_push_back(nb_only, &od->list);
         }
         init_mcast_info_for_datapath(od);
+        init_nat_entries(od);
         ovs_list_push_back(lr_list, &od->lr_list);
     }
 }
@@ -8465,30 +8534,24 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
             snat_ips[n_snat_ips++] = snat_ip;
         }
 
-        for (int i = 0; i < op->od->nbr->n_nat; i++) {
-            const struct nbrec_nat *nat;
-
-            nat = op->od->nbr->nat[i];
+        for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
+            struct ovn_nat *nat_entry = &op->od->nat_entries[i];
+            const struct nbrec_nat *nat = nat_entry->nb;
 
-            ovs_be32 ip;
-            struct in6_addr ipv6;
-            bool is_v6 = false;
-            if (!ip_parse(nat->external_ip, &ip) || !ip) {
-                if (!ipv6_parse(nat->external_ip, &ipv6)) {
-                    static struct vlog_rate_limit rl =
-                        VLOG_RATE_LIMIT_INIT(5, 1);
-                    VLOG_WARN_RL(&rl, "bad ip address %s in nat configuration "
-                                 "for router %s", nat->external_ip, op->key);
-                    continue;
-                }
-                is_v6 = true;
+            /* Skip entries we failed to parse. */
+            if (!nat_entry_is_valid(nat_entry)) {
+                continue;
             }
 
             if (!strcmp(nat->type, "snat")) {
-                if (is_v6) {
+                if (nat_entry_is_v6(nat_entry)) {
+                    struct in6_addr *ipv6 =
+                        &nat_entry->ext_addrs.ipv6_addrs[0].addr;
+
                     snat_ips[n_snat_ips].family = AF_INET6;
-                    snat_ips[n_snat_ips++].ipv6 = ipv6;
+                    snat_ips[n_snat_ips++].ipv6 = *ipv6;
                 } else {
+                    ovs_be32 ip = nat_entry->ext_addrs.ipv4_addrs[0].addr;
                     snat_ips[n_snat_ips].family = AF_INET;
                     snat_ips[n_snat_ips++].ipv4 = ip;
                 }
@@ -8531,22 +8594,18 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                     }
                 }
             }
-            if (is_v6) {
-                /* For ND solicitations, we need to listen for both the
-                 * unicast IPv6 address and its all-nodes multicast address,
-                 * but always respond with the unicast IPv6 address. */
-                char sn_addr_s[INET6_ADDRSTRLEN + 1];
-                struct in6_addr sn_addr;
-                in6_addr_solicited_node(&sn_addr, &ipv6);
-                ipv6_string_mapped(sn_addr_s, &sn_addr);
 
+            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
+            if (nat_entry_is_v6(nat_entry)) {
                 build_lrouter_nd_flow(op->od, op, "nd_na",
-                                       nat->external_ip, sn_addr_s,
-                                       mac_s, &match, 90,
-                                       lflows, &nat->header_);
+                                      ext_addrs->ipv6_addrs[0].addr_s,
+                                      ext_addrs->ipv6_addrs[0].sn_addr_s,
+                                      mac_s, &match, 90,
+                                      lflows, &nat->header_);
             } else {
                 build_lrouter_arp_flow(op->od, op,
-                                       nat->external_ip, mac_s, &match, 90,
+                                       ext_addrs->ipv4_addrs[0].addr_s,
+                                       mac_s, &match, 90,
                                        lflows, &nat->header_);
             }
         }