[ovs-dev,ovn] manage ARP process locally in a DVR scenario

OVN currently performs L2 address resolution and IP buffering on the
gw node. If the system relies on FIPs, OVN will re-inject the buffered
IP packets on the gw node, while following packets will go though
the localnet port on the compute node resulting in a ToR switch
misconfiguration. This patch addresses the issue managing ARP
and IP buffering locally if FIPs are configured on the node

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 northd/ovn-northd.8.xml | 31 ++++++++++++++++++++++++++
 northd/ovn-northd.c     | 48 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 1 deletion(-)

On 2/21/20 6:40 PM, Lorenzo Bianconi wrote:
> OVN currently performs L2 address resolution and IP buffering on the
> gw node. If the system relies on FIPs, OVN will re-inject the buffered
> IP packets on the gw node, while following packets will go though
> the localnet port on the compute node resulting in a ToR switch
> misconfiguration. This patch addresses the issue managing ARP
> and IP buffering locally if FIPs are configured on the node
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Hi Lorenzo,

Overall, the code looks ok to me. Just a few comments/questions inline.

Thanks,
Dumitru

> ---
>  northd/ovn-northd.8.xml | 31 ++++++++++++++++++++++++++
>  northd/ovn-northd.c     | 48 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index a27dfa951..23b385377 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2405,6 +2405,37 @@ output;
>          </p>
>        </li>
>  
> +      <li>
> +        <p>
> +          For distributed logical routers where one of the logical router ports
> +          specifies a <code>redirect-chassis</code>, a priority-400 logical
> +          flow for each <code>dnat_and_snat</code> NAT rules configured.
> +          These flows will allow to properly forward traffic to the external
> +          connections if available and avoid sending it through the tunnel.
> +          Assuming the following NAT rule has been configured:
> +        </p>
> +
> +        <pre>
> +external_ip = <var>A</var>;
> +external_mac = <var>B</var>;
> +logical_ip = <var>C</var>;
> +        </pre>
> +
> +        <p>
> +          the following action will be applied:
> +        </p>
> +
> +        <pre>
> +ip.ttl--;
> +reg0 = <var>ip.dst</var>;
> +reg1 = <var>A</var>;
> +eth.src = <var>B</var>;
> +outport = <var>router-port</var>;
> +next;
> +        </pre>
> +
> +      </li>
> +
>        <li>
>          <p>
>            IPv4 routing table.  For each route to IPv4 network <var>N</var> with
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 721cb05ce..8cecc98ca 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -7338,6 +7338,42 @@ build_ecmp_route_flow(struct hmap *lflows, struct ovn_datapath *od,
>      ds_destroy(&actions);
>  }
>  
> +#define DROUTE_PRIO 400

Can we move this where the route related structure definitions are? I
guess it would fit better before the definition of "struct
parsed_route". Also a comment describing why we need it would be useful.

> +static void
> +add_distributed_routes(struct hmap *lflows, struct ovn_datapath *od)
> +{
> +    struct ds actions = DS_EMPTY_INITIALIZER;
> +    struct ds match = DS_EMPTY_INITIALIZER;
> +
> +    if (!od->l3dgw_port || !od->nbr) {
> +        return;
> +    }
> +
> +    for (size_t i = 0; i < od->nbr->n_nat; i++) {
> +        const struct nbrec_nat *nat = od->nbr->nat[i];
> +
> +        if (strcmp(nat->type, "dnat_and_snat") ||
> +            !nat->external_mac  || !nat->external_ip) {

Please remove the extra whitespace after "!nat->external_mac".

> +            continue;
> +        }
> +
> +        bool is_ipv4 = strchr(nat->logical_ip, '.') ? true : false;
> +        ds_put_format(&match, "ip%s.src == %s && is_chassis_resident(\"%s\")",
> +                      is_ipv4 ? "4" : "6", nat->logical_ip,
> +                      nat->logical_port);
> +        char *prefix = is_ipv4 ? "" : "xx";
> +        ds_put_format(&actions, "outport = %s; eth.src = %s; "
> +                      "%sreg0 = ip%s.dst; %sreg1 = %s; next;",
> +                      od->l3dgw_port->json_key, nat->external_mac,
> +                      prefix, is_ipv4 ? "4" : "6",
> +                      prefix, nat->external_ip);
> +        ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, DROUTE_PRIO,
> +                      ds_cstr(&match), ds_cstr(&actions));
> +        ds_clear(&match);
> +        ds_clear(&actions);
> +    }
> +}
> +
>  static void
>  add_route(struct hmap *lflows, const struct ovn_port *op,
>            const char *lrp_addr_s, const char *network_s, int plen,
> @@ -7359,6 +7395,9 @@ add_route(struct hmap *lflows, const struct ovn_port *op,
>      }
>      build_route_match(op_inport, network_s, plen, is_src_route, is_ipv4,
>                        &match, &priority);

I'm a bit confused here. Why do we need to bump priority only for some
router ports while in add_distributed_routes we add flows with priority
400 for the whole datapath? Shouldn't the condition here match with the
condition that needs to be true in add_distributed_routes in order to
add the flows for nat rules?

In any case, I think a comment would be nice helpful.

> +    if (op->nbrp && !op->nbrp->n_gateway_chassis) {
> +        priority += DROUTE_PRIO;
> +    }
>  
>      struct ds actions = DS_EMPTY_INITIALIZER;
>      ds_put_format(&actions, "ip.ttl--; "REG_ECMP_GROUP_ID" = 0; %sreg0 = ",
> @@ -8927,7 +8966,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                                nat->logical_ip,
>                                od->l3dgw_port->json_key);
>                  ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT,
> -                                        100, ds_cstr(&match), "next;",
> +                                        200, ds_cstr(&match), "next;",
>                                          &nat->header_);
>              }
>  
> @@ -9208,6 +9247,13 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>          ovn_lflow_add(lflows, od, S_ROUTER_IN_ND_RA_RESPONSE, 0, "1", "next;");
>      }
>  
> +    /* Logical router ingress table IP_ROUTING - IP routing for distributed
> +     * logical router
> +     */
> +    HMAP_FOR_EACH (od, key_node, datapaths) {
> +        add_distributed_routes(lflows, od);
> +    }
> +
>      /* Logical router ingress table IP_ROUTING & IP_ROUTING_ECMP: IP Routing.
>       *
>       * A packet that arrives at this table is an IP packet that should be
>