[ovs-dev,v4,1/2] ovn-northd: Refactor parsing of *_force_snat_ip.

Avoid reparsing the *_force_snat_ip addresses.

Acked-by: Han Zhou <hzhou@ovn.org>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 lib/ovn-util.c      |    6 ++++
 lib/ovn-util.h      |    2 +
 northd/ovn-northd.c |   69 ++++++++++++++++++++++++---------------------------
 tests/ovn-northd.at |   19 ++++++++++++++
 4 files changed, 59 insertions(+), 37 deletions(-)

On Mon, Sep 28, 2020 at 3:39 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> Avoid reparsing the *_force_snat_ip addresses.
>
> Acked-by: Han Zhou <hzhou@ovn.org>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Thanks Dumitru. I applied this patch to master.

Numan

> ---
>  lib/ovn-util.c      |    6 ++++
>  lib/ovn-util.h      |    2 +
>  northd/ovn-northd.c |   69 ++++++++++++++++++++++++---------------------------
>  tests/ovn-northd.at |   19 ++++++++++++++
>  4 files changed, 59 insertions(+), 37 deletions(-)
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 47c7f57..1daf665 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -320,6 +320,12 @@ extract_sbrec_binding_first_mac(const struct sbrec_port_binding *binding,
>      return ret;
>  }
>
> +bool
> +lport_addresses_is_empty(struct lport_addresses *laddrs)
> +{
> +    return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
> +}
> +
>  void
>  destroy_lport_addresses(struct lport_addresses *laddrs)
>  {
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index a597efb..1d22a69 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -75,7 +75,7 @@ bool extract_lrp_networks(const struct nbrec_logical_router_port *,
>                            struct lport_addresses *);
>  bool extract_sbrec_binding_first_mac(const struct sbrec_port_binding *binding,
>                                       struct eth_addr *ea);
> -
> +bool lport_addresses_is_empty(struct lport_addresses *);
>  void destroy_lport_addresses(struct lport_addresses *);
>
>  char *alloc_nat_zone_key(const struct uuid *key, const char *type);
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 3324c9e..5fddc5e 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -625,6 +625,8 @@ struct ovn_datapath {
>
>      /* SNAT IPs used by the router. */
>      struct sset snat_ips;
> +    struct lport_addresses dnat_force_snat_addrs;
> +    struct lport_addresses lb_force_snat_addrs;
>
>      struct ovn_port **localnet_ports;
>      size_t n_localnet_ports;
> @@ -670,32 +672,31 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry)
>  static void
>  init_nat_entries(struct ovn_datapath *od)
>  {
> -    struct lport_addresses snat_addrs;
> -
>      if (!od->nbr) {
>          return;
>      }
>
>      sset_init(&od->snat_ips);
> -    if (get_force_snat_ip(od, "dnat", &snat_addrs)) {
> -        if (snat_addrs.n_ipv4_addrs) {
> -            sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
> +    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);
>          }
> -        if (snat_addrs.n_ipv6_addrs) {
> -            sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
> +        if (od->dnat_force_snat_addrs.n_ipv6_addrs) {
> +            sset_add(&od->snat_ips,
> +                     od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s);
>          }
> -        destroy_lport_addresses(&snat_addrs);
>      }
>
> -    memset(&snat_addrs, 0, sizeof(snat_addrs));
> -    if (get_force_snat_ip(od, "lb", &snat_addrs)) {
> -        if (snat_addrs.n_ipv4_addrs) {
> -            sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
> +    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);
>          }
> -        if (snat_addrs.n_ipv6_addrs) {
> -            sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
> +        if (od->lb_force_snat_addrs.n_ipv6_addrs) {
> +            sset_add(&od->snat_ips,
> +                     od->lb_force_snat_addrs.ipv6_addrs[0].addr_s);
>          }
> -        destroy_lport_addresses(&snat_addrs);
>      }
>
>      if (!od->nbr->n_nat) {
> @@ -736,6 +737,9 @@ destroy_nat_entries(struct ovn_datapath *od)
>      }
>
>      sset_destroy(&od->snat_ips);
> +    destroy_lport_addresses(&od->dnat_force_snat_addrs);
> +    destroy_lport_addresses(&od->lb_force_snat_addrs);
> +
>      for (size_t i = 0; i < od->nbr->n_nat; i++) {
>          destroy_lport_addresses(&od->nat_entries[i].ext_addrs);
>      }
> @@ -9306,12 +9310,10 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>
>          struct sset nat_entries = SSET_INITIALIZER(&nat_entries);
>
> -        struct lport_addresses dnat_force_snat_addrs;
> -        struct lport_addresses lb_force_snat_addrs;
> -        bool dnat_force_snat_ip = get_force_snat_ip(od, "dnat",
> -                                                    &dnat_force_snat_addrs);
> -        bool lb_force_snat_ip = get_force_snat_ip(od, "lb",
> -                                                  &lb_force_snat_addrs);
> +        bool dnat_force_snat_ip =
> +            !lport_addresses_is_empty(&od->dnat_force_snat_addrs);
> +        bool lb_force_snat_ip =
> +            !lport_addresses_is_empty(&od->lb_force_snat_addrs);
>
>          for (int i = 0; i < od->nbr->n_nat; i++) {
>              const struct nbrec_nat *nat;
> @@ -9811,23 +9813,25 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>          /* Handle force SNAT options set in the gateway router. */
>          if (!od->l3dgw_port) {
>              if (dnat_force_snat_ip) {
> -                if (dnat_force_snat_addrs.n_ipv4_addrs) {
> +                if (od->dnat_force_snat_addrs.n_ipv4_addrs) {
>                      build_lrouter_force_snat_flows(lflows, od, "4",
> -                        dnat_force_snat_addrs.ipv4_addrs[0].addr_s, "dnat");
> +                        od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
> +                        "dnat");
>                  }
> -                if (dnat_force_snat_addrs.n_ipv6_addrs) {
> +                if (od->dnat_force_snat_addrs.n_ipv6_addrs) {
>                      build_lrouter_force_snat_flows(lflows, od, "6",
> -                        dnat_force_snat_addrs.ipv6_addrs[0].addr_s, "dnat");
> +                        od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
> +                        "dnat");
>                  }
>              }
>              if (lb_force_snat_ip) {
> -                if (lb_force_snat_addrs.n_ipv4_addrs) {
> +                if (od->lb_force_snat_addrs.n_ipv4_addrs) {
>                      build_lrouter_force_snat_flows(lflows, od, "4",
> -                        lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb");
> +                        od->lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb");
>                  }
> -                if (lb_force_snat_addrs.n_ipv6_addrs) {
> +                if (od->lb_force_snat_addrs.n_ipv6_addrs) {
>                      build_lrouter_force_snat_flows(lflows, od, "6",
> -                        lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb");
> +                        od->lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb");
>                  }
>              }
>
> @@ -9844,13 +9848,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                            "ip", "flags.loopback = 1; ct_dnat;");
>          }
>
> -        if (dnat_force_snat_ip) {
> -            destroy_lport_addresses(&dnat_force_snat_addrs);
> -        }
> -        if (lb_force_snat_ip) {
> -            destroy_lport_addresses(&lb_force_snat_addrs);
> -        }
> -
>          /* Load balancing and packet defrag are only valid on
>           * Gateway routers or router with gateway port. */
>          if (!smap_get(&od->nbr->options, "chassis") && !od->l3dgw_port) {
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 99a9204..c36c580 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1603,6 +1603,25 @@ grep "ip4 && ip4.dst == 192.168.2.6 && tcp && tcp.dst == 8080" -c) ])
>
>  AT_CLEANUP
>
> +AT_SETUP([ovn -- DNAT force snat IP])
> +ovn_start
> +
> +ovn-nbctl lr-add lr0
> +ovn-nbctl lrp-add lr0 lr0-public 00:00:01:01:02:04 192.168.2.1/24
> +ovn-nbctl lrp-add lr0 lr0-join 00:00:01:01:02:04 10.10.0.1/24
> +
> +ovn-nbctl set logical_router lr0 options:chassis=ch1
> +ovn-nbctl lr-nat-add lr0 dnat 192.168.2.2 10.0.0.5
> +ovn-nbctl set logical_router lr0 options:dnat_force_snat_ip=192.168.2.3
> +ovn-nbctl --wait=sb sync
> +
> +AT_CHECK([ovn-sbctl lflow-list lr0 | grep lr_in_unsnat], [0], [dnl
> +  table=5 (lr_in_unsnat       ), priority=110  , match=(ip4 && ip4.dst == 192.168.2.3), action=(ct_snat;)
> +  table=5 (lr_in_unsnat       ), priority=0    , match=(1), action=(next;)
> +])
> +
> +AT_CLEANUP
> +
>  AT_SETUP([ovn -- check reconcile stale Datapath_Binding])
>  ovn_start
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>