[ovs-dev] [PATCH v4 ovn 2/2] ovn-northd: Refactor processing of SNAT IPs.

Numan Siddique numans at ovn.org
Tue Sep 29 08:48:34 UTC 2020


On Mon, Sep 28, 2020 at 3:39 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> Instead of building string sets every time we need to generate logical flows
> for unique SNAT IPs we now prebuild the set of unique SNAT IPs and store the
> list of NAT entries that refer it.
>
> Acked-by: Han Zhou <hzhou at ovn.org>
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
>  northd/ovn-northd.c |  322 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 174 insertions(+), 148 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 5fddc5e..a39cb0e 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -623,8 +623,9 @@ struct ovn_datapath {
>      /* NAT entries configured on the router. */
>      struct ovn_nat *nat_entries;
>
> -    /* SNAT IPs used by the router. */
> -    struct sset snat_ips;
> +    /* SNAT IPs owned by the router (shash of 'struct ovn_snat_ip'). */
> +    struct shash snat_ips;
> +
>      struct lport_addresses dnat_force_snat_addrs;
>      struct lport_addresses lb_force_snat_addrs;
>
> @@ -644,6 +645,18 @@ struct ovn_datapath {
>  struct ovn_nat {
>      const struct nbrec_nat *nb;
>      struct lport_addresses ext_addrs;
> +    struct ovs_list ext_addr_list_node; /* Linkage in the per-external IP
> +                                         * list of nat entries. Currently
> +                                         * only used for SNAT.
> +                                         */
> +};
> +
> +/* Stores the list of SNAT entries referencing a unique SNAT IP address.
> + * The 'snat_entries' list will be empty if the SNAT IP is used only for
> + * dnat_force_snat_ip or lb_force_snat_ip.
> + */
> +struct ovn_snat_ip {
> +    struct ovs_list snat_entries;
>  };
>
>  static bool
> @@ -670,32 +683,49 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry)
>  }
>
>  static void
> +snat_ip_add(struct ovn_datapath *od, const char *ip, struct ovn_nat *nat_entry)
> +{
> +    struct ovn_snat_ip *snat_ip = shash_find_data(&od->snat_ips, ip);
> +
> +    if (!snat_ip) {
> +        snat_ip = xzalloc(sizeof *snat_ip);
> +        ovs_list_init(&snat_ip->snat_entries);
> +        shash_add(&od->snat_ips, ip, snat_ip);
> +    }
> +
> +    if (nat_entry) {
> +        ovs_list_push_back(&snat_ip->snat_entries,
> +                           &nat_entry->ext_addr_list_node);
> +    }
> +}
> +
> +static void
>  init_nat_entries(struct ovn_datapath *od)
>  {
>      if (!od->nbr) {
>          return;
>      }
>
> -    sset_init(&od->snat_ips);
> +    shash_init(&od->snat_ips);
>      if (get_force_snat_ip(od, "dnat", &od->dnat_force_snat_addrs)) {
>          if (od->dnat_force_snat_addrs.n_ipv4_addrs) {
> -            sset_add(&od->snat_ips,
> -                     od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s);
> +            snat_ip_add(od, od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
> +                        NULL);
>          }
>          if (od->dnat_force_snat_addrs.n_ipv6_addrs) {
> -            sset_add(&od->snat_ips,
> -                     od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s);
> +            snat_ip_add(od, od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
> +                        NULL);
>          }
>      }
>
>      if (get_force_snat_ip(od, "lb", &od->lb_force_snat_addrs)) {
>          if (od->lb_force_snat_addrs.n_ipv4_addrs) {
> -            sset_add(&od->snat_ips,
> -                     od->lb_force_snat_addrs.ipv4_addrs[0].addr_s);
> +            snat_ip_add(od, od->lb_force_snat_addrs.ipv4_addrs[0].addr_s,
> +                        NULL);
>          }
>          if (od->lb_force_snat_addrs.n_ipv6_addrs) {
> -            sset_add(&od->snat_ips,
> -                     od->lb_force_snat_addrs.ipv6_addrs[0].addr_s);
> +            snat_ip_add(od, od->lb_force_snat_addrs.ipv6_addrs[0].addr_s,
> +                        NULL);
>          }
>      }
>
> @@ -721,10 +751,15 @@ init_nat_entries(struct ovn_datapath *od)
>              continue;
>          }
>
> -        if (!nat_entry_is_v6(nat_entry)) {
> -            sset_add(&od->snat_ips, nat_entry->ext_addrs.ipv4_addrs[0].addr_s);
> -        } else {
> -            sset_add(&od->snat_ips, nat_entry->ext_addrs.ipv6_addrs[0].addr_s);
> +        /* If this is a SNAT rule add the IP to the set of unique SNAT IPs. */
> +        if (!strcmp(nat->type, "snat")) {
> +            if (!nat_entry_is_v6(nat_entry)) {
> +                snat_ip_add(od, nat_entry->ext_addrs.ipv4_addrs[0].addr_s,
> +                            nat_entry);
> +            } else {
> +                snat_ip_add(od, nat_entry->ext_addrs.ipv6_addrs[0].addr_s,
> +                            nat_entry);
> +            }
>          }
>      }
>  }
> @@ -736,7 +771,7 @@ destroy_nat_entries(struct ovn_datapath *od)
>          return;
>      }
>
> -    sset_destroy(&od->snat_ips);
> +    shash_destroy_free_data(&od->snat_ips);
>      destroy_lport_addresses(&od->dnat_force_snat_addrs);
>      destroy_lport_addresses(&od->lb_force_snat_addrs);
>
> @@ -8728,6 +8763,65 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op,
>  }
>
>  static void
> +build_lrouter_drop_own_dest(struct ovn_port *op, enum ovn_stage stage,
> +                            uint16_t priority, bool drop_snat,
> +                            struct hmap *lflows)
> +{
> +    struct ds match_ips = DS_EMPTY_INITIALIZER;
> +
> +    if (op->lrp_networks.n_ipv4_addrs) {
> +        for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> +            const char *ip = op->lrp_networks.ipv4_addrs[i].addr_s;
> +
> +            if (drop_snat && !shash_find(&op->od->snat_ips, ip)) {
> +                continue;
> +            } else if (!drop_snat && shash_find(&op->od->snat_ips, ip)) {
> +                continue;
> +            }
> +            ds_put_format(&match_ips, "%s, ", ip);

Hi Dumitru,

The patch LGTM with a few minor comments.

I find the above if/else if and the continue a bit confusing. We could
also avoid calling shash_find twice.

How about the below changes on top of your patch. Does that sound good ?

******************************************************
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index a39cb0ebe..91da31941 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -8764,7 +8764,7 @@ build_lrouter_port_nat_arp_nd_flow(struct ovn_port *op,

 static void
 build_lrouter_drop_own_dest(struct ovn_port *op, enum ovn_stage stage,
-                            uint16_t priority, bool drop_snat,
+                            uint16_t priority, bool drop_snat_ip,
                             struct hmap *lflows)
 {
     struct ds match_ips = DS_EMPTY_INITIALIZER;
@@ -8773,12 +8773,12 @@ build_lrouter_drop_own_dest(struct ovn_port
*op, enum ovn_stage stage,
         for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
             const char *ip = op->lrp_networks.ipv4_addrs[i].addr_s;

-            if (drop_snat && !shash_find(&op->od->snat_ips, ip)) {
-                continue;
-            } else if (!drop_snat && shash_find(&op->od->snat_ips, ip)) {
-                continue;
+            bool router_ip_in_snat_ips = !!shash_find(&op->od->snat_ips, ip);
+            bool drop_router_ip = (drop_snat_ip == router_ip_in_snat_ips);
+
+            if (drop_router_ip) {
+                ds_put_format(&match_ips, "%s, ", ip);
             }
-            ds_put_format(&match_ips, "%s, ", ip);
         }

         if (ds_last(&match_ips) != EOF) {
@@ -8799,12 +8799,12 @@ build_lrouter_drop_own_dest(struct ovn_port
*op, enum ovn_stage stage,
         for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
             const char *ip = op->lrp_networks.ipv6_addrs[i].addr_s;

-            if (drop_snat && !shash_find(&op->od->snat_ips, ip)) {
-                continue;
-            } else if (!drop_snat && shash_find(&op->od->snat_ips, ip)) {
-                continue;
+            bool router_ip_in_snat_ips = !!shash_find(&op->od->snat_ips, ip);
+            bool drop_router_ip = (drop_snat_ip == router_ip_in_snat_ips);
+
+            if (drop_router_ip) {
+                ds_put_format(&match_ips, "%s, ", ip);
             }
-            ds_put_format(&match_ips, "%s, ", ip);
         }

         if (ds_last(&match_ips) != EOF) {
**********************************************************

Thanks
Numan

> +        }
> +
> +        if (ds_last(&match_ips) != EOF) {
> +            ds_chomp(&match_ips, ' ');
> +            ds_chomp(&match_ips, ',');
> +
> +            char *match = xasprintf("ip4.dst == {%s}", ds_cstr(&match_ips));
> +            ovn_lflow_add_with_hint(lflows, op->od, stage, priority,
> +                                    match, "drop;",
> +                                    &op->nbrp->header_);
> +            free(match);
> +        }
> +    }
> +
> +    if (op->lrp_networks.n_ipv6_addrs) {
> +        ds_clear(&match_ips);
> +
> +        for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> +            const char *ip = op->lrp_networks.ipv6_addrs[i].addr_s;
> +
> +            if (drop_snat && !shash_find(&op->od->snat_ips, ip)) {
> +                continue;
> +            } else if (!drop_snat && shash_find(&op->od->snat_ips, ip)) {
> +                continue;
> +            }
> +            ds_put_format(&match_ips, "%s, ", ip);
> +        }
> +
> +        if (ds_last(&match_ips) != EOF) {
> +            ds_chomp(&match_ips, ' ');
> +            ds_chomp(&match_ips, ',');
> +
> +            char *match = xasprintf("ip6.dst == {%s}", ds_cstr(&match_ips));
> +            ovn_lflow_add_with_hint(lflows, op->od, stage, priority,
> +                                    match, "drop;",
> +                                    &op->nbrp->header_);
> +            free(match);
> +        }
> +    }
> +    ds_destroy(&match_ips);
> +}
> +
> +static void
>  build_lrouter_force_snat_flows(struct hmap *lflows, struct ovn_datapath *od,
>                                 const char *ip_version, const char *ip_addr,
>                                 const char *context)
> @@ -8897,68 +8991,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                  op, lflows, &match, &actions);
>      }
>
> -    /* Drop IP traffic destined to router owned IPs. Part of it is dropped
> -     * in stage "lr_in_ip_input" but traffic that could have been unSNATed
> -     * but didn't match any existing session might still end up here.
> -     */
> -    HMAP_FOR_EACH (op, key_node, ports) {
> -        if (!op->nbrp) {
> -            continue;
> -        }
> -
> -        if (op->lrp_networks.n_ipv4_addrs) {
> -            ds_clear(&match);
> -            for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> -                if (!sset_find(&op->od->snat_ips,
> -                               op->lrp_networks.ipv4_addrs[i].addr_s)) {
> -                    continue;
> -                }
> -                ds_put_format(&match, "%s, ",
> -                              op->lrp_networks.ipv4_addrs[i].addr_s);
> -            }
> -
> -            if (ds_last(&match) != EOF) {
> -                ds_chomp(&match, ' ');
> -                ds_chomp(&match, ',');
> -
> -                char *drop_match = xasprintf("ip4.dst == {%s}",
> -                                             ds_cstr(&match));
> -                /* Drop traffic with IP.dest == router-ip. */
> -                ovn_lflow_add_with_hint(lflows, op->od,
> -                                        S_ROUTER_IN_ARP_RESOLVE, 1,
> -                                        drop_match, "drop;",
> -                                        &op->nbrp->header_);
> -                free(drop_match);
> -            }
> -        }
> -
> -        if (op->lrp_networks.n_ipv6_addrs) {
> -            ds_clear(&match);
> -            for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> -                if (!sset_find(&op->od->snat_ips,
> -                               op->lrp_networks.ipv6_addrs[i].addr_s)) {
> -                    continue;
> -                }
> -                ds_put_format(&match, "%s, ",
> -                              op->lrp_networks.ipv6_addrs[i].addr_s);
> -            }
> -
> -            if (ds_last(&match) != EOF) {
> -                ds_chomp(&match, ' ');
> -                ds_chomp(&match, ',');
> -
> -                char *drop_match = xasprintf("ip6.dst == {%s}",
> -                                             ds_cstr(&match));
> -                /* Drop traffic with IP.dest == router-ip. */
> -                ovn_lflow_add_with_hint(lflows, op->od,
> -                                        S_ROUTER_IN_ARP_RESOLVE, 1,
> -                                        drop_match, "drop;",
> -                                        &op->nbrp->header_);
> -                free(drop_match);
> -            }
> -        }
> -    }
> -
>      HMAP_FOR_EACH (od, key_node, datapaths) {
>          if (!od->nbr) {
>              continue;
> @@ -8972,29 +9004,37 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>           * port to handle the special cases. In case we get the packet
>           * on a regular port, just reply with the port's ETH address.
>           */
> -        struct sset snat_ips = SSET_INITIALIZER(&snat_ips);
>          for (int i = 0; i < od->nbr->n_nat; i++) {
>              struct ovn_nat *nat_entry = &od->nat_entries[i];
> -            const struct nbrec_nat *nat = nat_entry->nb;
>
>              /* Skip entries we failed to parse. */
>              if (!nat_entry_is_valid(nat_entry)) {
>                  continue;
>              }
>
> -            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> -            char *ext_addr = nat_entry_is_v6(nat_entry) ?
> -                             ext_addrs->ipv6_addrs[0].addr_s :
> -                             ext_addrs->ipv4_addrs[0].addr_s;
> +            /* Skip SNAT entries for now, we handle unique SNAT IPs separately
> +             * below.
> +             */
> +            if (!strcmp(nat_entry->nb->type, "snat")) {
> +                continue;
> +            }
> +            build_lrouter_nat_arp_nd_flow(od, nat_entry, lflows);
> +        }
>
> -            if (!strcmp(nat->type, "snat")) {
> -                if (!sset_add(&snat_ips, ext_addr)) {
> -                    continue;
> -                }
> +        /* Now handle SNAT entries too, one per unique SNAT IP. */
> +        struct shash_node *snat_snode;
> +        SHASH_FOR_EACH (snat_snode, &od->snat_ips) {
> +            struct ovn_snat_ip *snat_ip = snat_snode->data;
> +
> +            if (ovs_list_is_empty(&snat_ip->snat_entries)) {
> +                continue;
>              }
> +
> +            struct ovn_nat *nat_entry =
> +                CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries),
> +                             struct ovn_nat, ext_addr_list_node);
>              build_lrouter_nat_arp_nd_flow(od, nat_entry, lflows);
>          }
> -        sset_destroy(&snat_ips);
>      }
>
>      HMAP_FOR_EACH (od, key_node, datapaths) {
> @@ -9193,82 +9233,59 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>              }
>          }
>
> -        /* A gateway router can have 4 SNAT IP addresses to force DNATed and
> -         * LBed traffic respectively to be SNATed. In addition, there can be
> -         * a number of SNAT rules in the NAT table.
> -         * Skip all of them for drop flows. */
> -        ds_clear(&match);
> -        ds_put_cstr(&match, "ip4.dst == {");
> -        bool has_drop_ips = false;
> -        for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> -            if (sset_find(&op->od->snat_ips,
> -                          op->lrp_networks.ipv4_addrs[i].addr_s)) {
> -                continue;
> -            }
> -            ds_put_format(&match, "%s, ",
> -                          op->lrp_networks.ipv4_addrs[i].addr_s);
> -            has_drop_ips = true;
> -        }
> -        if (has_drop_ips) {
> -            ds_chomp(&match, ' ');
> -            ds_chomp(&match, ',');
> -            ds_put_cstr(&match, "} || ip6.dst == {");
> -        } else {
> -            ds_clear(&match);
> -            ds_put_cstr(&match, "ip6.dst == {");
> -        }
> -
> -        for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> -            if (sset_find(&op->od->snat_ips,
> -                          op->lrp_networks.ipv6_addrs[i].addr_s)) {
> -                continue;
> -            }
> -            ds_put_format(&match, "%s, ",
> -                          op->lrp_networks.ipv6_addrs[i].addr_s);
> -            has_drop_ips = true;
> -        }
> -
> -        ds_chomp(&match, ' ');
> -        ds_chomp(&match, ',');
> -        ds_put_cstr(&match, "}");
> -
> -        if (has_drop_ips) {
> -            /* Drop IP traffic to this router. */
> -            ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_IP_INPUT, 60,
> -                                    ds_cstr(&match), "drop;",
> -                                    &op->nbrp->header_);
> -        }
> +        /* Drop IP traffic destined to router owned IPs except if the IP is
> +         * also a SNAT IP. Those are dropped later, in stage
> +         * "lr_in_arp_resolve", if unSNAT was unsuccessful.
> +         *
> +         * Priority 60.
> +         */
> +        build_lrouter_drop_own_dest(op, S_ROUTER_IN_IP_INPUT, 60, false,
> +                                    lflows);
>
> -        /* ARP/NS packets are taken care of per router. The only exception
> -         * is on the l3dgw_port where we might need to use a different
> -         * ETH address.
> +        /* ARP / ND handling for external IP addresses.
> +         *
> +         * DNAT and SNAT IP addresses are external IP addresses that need ARP
> +         * handling.
> +         *
> +         * These are already taken care globally, per router. The only
> +         * exception is on the l3dgw_port where we might need to use a
> +         * different ETH address.
>           */
>          if (op != op->od->l3dgw_port) {
>              continue;
>          }
>
> -        struct sset sset_snat_ips = SSET_INITIALIZER(&sset_snat_ips);
>          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;
>
>              /* Skip entries we failed to parse. */
>              if (!nat_entry_is_valid(nat_entry)) {
>                  continue;
>              }
>
> -            struct lport_addresses *ext_addrs = &nat_entry->ext_addrs;
> -            char *ext_addr = (nat_entry_is_v6(nat_entry)
> -                              ? ext_addrs->ipv6_addrs[0].addr_s
> -                              : ext_addrs->ipv4_addrs[0].addr_s);
> -            if (!strcmp(nat->type, "snat")) {
> -                if (!sset_add(&sset_snat_ips, ext_addr)) {
> -                    continue;
> -                }
> +            /* Skip SNAT entries for now, we handle unique SNAT IPs separately
> +             * below.
> +             */
> +            if (!strcmp(nat_entry->nb->type, "snat")) {
> +                continue;
> +            }
> +            build_lrouter_port_nat_arp_nd_flow(op, nat_entry, lflows);
> +        }
> +
> +        /* Now handle SNAT entries too, one per unique SNAT IP. */
> +        struct shash_node *snat_snode;
> +        SHASH_FOR_EACH (snat_snode, &op->od->snat_ips) {
> +            struct ovn_snat_ip *snat_ip = snat_snode->data;
> +
> +            if (ovs_list_is_empty(&snat_ip->snat_entries)) {
> +                continue;
>              }
> +
> +            struct ovn_nat *nat_entry =
> +                CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries),
> +                             struct ovn_nat, ext_addr_list_node);
>              build_lrouter_port_nat_arp_nd_flow(op, nat_entry, lflows);
>          }
> -        sset_destroy(&sset_snat_ips);
>      }
>
>      HMAP_FOR_EACH (op, key_node, ports) {
> @@ -10642,6 +10659,15 @@ build_arp_resolve_flows_for_lrouter_port(
>                                          &op->nbrp->header_);
>              }
>          }
> +
> +        /* Drop IP traffic destined to router owned IPs. Part of it is dropped
> +         * in stage "lr_in_ip_input" but traffic that could have been unSNATed
> +         * but didn't match any existing session might still end up here.
> +         *
> +         * Priority 1.
> +         */
> +        build_lrouter_drop_own_dest(op, S_ROUTER_IN_ARP_RESOLVE, 1, true,
> +                                    lflows);
>      } else if (op->od->n_router_ports && strcmp(op->nbsp->type, "router")
>                 && strcmp(op->nbsp->type, "virtual")) {
>          /* This is a logical switch port that backs a VM or a container.
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list