[ovs-dev,v2] northd: introduce nat-only-router for nat-addresses option

The CMS can configure the same lbs on multiple gw routers so the
same IPs/mac bindings will be adverised by multiple routers
through GARPs.
In order to fix the issue, introduce nat-only-router flag for
nat-addresses option in order to advertise in GARPs packets all
SNAT/DNAT configured in the connected logical router but skip
all configured load_balancers.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2054394
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2053013
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- improve commit message
- rebase on top of ovn master
---
 northd/northd.c | 51 +++++++++++++++++++++++++++----------------------
 ovn-nb.xml      | 21 ++++++++++++++++++++
 tests/ovn.at    | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 23 deletions(-)

On Thu, Feb 24, 2022 at 5:23 PM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> The CMS can configure the same lbs on multiple gw routers so the
> same IPs/mac bindings will be adverised by multiple routers
> through GARPs.
> In order to fix the issue, introduce nat-only-router flag for
> nat-addresses option in order to advertise in GARPs packets all
> SNAT/DNAT configured in the connected logical router but skip
> all configured load_balancers.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2054394
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2053013
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Hi Lorenzo,

Thanks for updating the commit message and for the rebase.

I see one problem with this approach.  We will break the functionality of garps
if CMS uses the new value "nat-only-router", and if ovn-northd is not updated
to the version which has this support.

Although we could argue that it is the problem of CMS,  I think it's
better we avoid
this in OVN.

Instead I would suggest adding another option -
"exclude-lb-vips-from-garp=true".
If CMS sets this option in the options column of logical_switch_port, ovn-northd
can exclude the load balancer vips from the garps.

Let me know what you think.  Can you please also add a test case in
ovn-northd.at
which checks that the lb vips are not set in the port_bindings options
column if this option is set.

Can you please also add an entry in the NEWS item for this.,

Also one small typo below.

Thanks
Numan

> ---
> Changes since v1:
> - improve commit message
> - rebase on top of ovn master
> ---
>  northd/northd.c | 51 +++++++++++++++++++++++++++----------------------
>  ovn-nb.xml      | 21 ++++++++++++++++++++
>  tests/ovn.at    | 38 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+), 23 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 219398f96..0d0fb7b95 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -1641,13 +1641,13 @@ destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
>  }
>
>  static char **get_nat_addresses(const struct ovn_port *op, size_t *n,
> -                                bool routable_only);
> +                                bool routable_only, bool include_lb_ips);
>
>  static void
>  assign_routable_addresses(struct ovn_port *op)
>  {
>      size_t n;
> -    char **nats = get_nat_addresses(op, &n, true);
> +    char **nats = get_nat_addresses(op, &n, true, true);
>
>      if (!nats) {
>          return;
> @@ -2711,7 +2711,8 @@ join_logical_ports(struct northd_input *input_data,
>   * 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, bool routable_only)
> +get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only,
> +                  bool include_lb_ips)
>  {
>      size_t n_nats = 0;
>      struct eth_addr mac;
> @@ -2791,24 +2792,26 @@ get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only)
>          }
>      }
>
> -    const char *ip_address;
> -    if (routable_only) {
> -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4_routable) {
> -            ds_put_format(&c_addresses, " %s", ip_address);
> -            central_ip_address = true;
> -        }
> -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6_routable) {
> -            ds_put_format(&c_addresses, " %s", ip_address);
> -            central_ip_address = true;
> -        }
> -    } else {
> -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) {
> -            ds_put_format(&c_addresses, " %s", ip_address);
> -            central_ip_address = true;
> -        }
> -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) {
> -            ds_put_format(&c_addresses, " %s", ip_address);
> -            central_ip_address = true;
> +    if (include_lb_ips) {
> +        const char *ip_address;
> +        if (routable_only) {
> +            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4_routable) {
> +                ds_put_format(&c_addresses, " %s", ip_address);
> +                central_ip_address = true;
> +            }
> +            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6_routable) {
> +                ds_put_format(&c_addresses, " %s", ip_address);
> +                central_ip_address = true;
> +            }
> +        } else {
> +            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) {
> +                ds_put_format(&c_addresses, " %s", ip_address);
> +                central_ip_address = true;
> +            }
> +            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) {
> +                ds_put_format(&c_addresses, " %s", ip_address);
> +                central_ip_address = true;
> +            }
>          }
>      }
>
> @@ -3374,10 +3377,12 @@ ovn_port_update_sbrec(struct northd_input *input_data,
>                                             "nat-addresses");
>              size_t n_nats = 0;
>              char **nats = NULL;
> -            if (nat_addresses && !strcmp(nat_addresses, "router")) {
> +            if (nat_addresses && (!strcmp(nat_addresses, "nat-only-router") ||
> +                                  !strcmp(nat_addresses, "router"))) {
>                  if (op->peer && op->peer->od
>                      && (chassis || op->peer->od->n_l3dgw_ports)) {
> -                    nats = get_nat_addresses(op->peer, &n_nats, false);
> +                    nats = get_nat_addresses(op->peer, &n_nats, false,
> +                                             !strcmp(nat_addresses, "router"));
>                  }
>              /* Only accept manual specification of ethernet address
>               * followed by IPv4 addresses on type "l3gateway" ports. */
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 642e91845..2d8d6ed4b 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -913,6 +913,27 @@
>                </p>
>              </dd>
>
> +            <dt><code>nat-only-router</code></dt>
> +            <dd>
> +              <p>
> +                Gratuitous ARPs will be sent for all SNAT and DNAT external IP
> +                addresses defined on the <ref column="options"
> +                key="router-port"/>'s logical router, using the
> +                <ref column="options" key="router-port"/>'s MAC address.
> +              </p>
> +
> +              <p>
> +                This form of <ref column="options" key="nat-addresses"/> is
> +                valid for logical switch ports where <ref column="options"
> +                key="router-port"/> is the name of a port on a gateway router,
> +                or the name of a distributed gateway port.
> +              </p>
> +
> +              <p>
> +                Supported only in OVN 2.21.12.1 and later.
s/2.21.12.1/21.12.1

Although I'd say we can probably skip this in the documentation since
the NEWS entry would cover it.

Thanks
Numan

> +              </p>
> +            </dd>
> +
>              <dt><code>Ethernet address followed by one or more IPv4 addresses</code></dt>
>              <dd>
>                <p>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index fa0e0a4a2..7f85aa6b3 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -8651,6 +8651,44 @@ expected="fffffffffffff0000000000108060001080006040001f00000000001c0a80003000000
>  echo $expected >> expout
>  AT_CHECK([sort packets], [0], [expout])
>
> +# Temporarily remove nat-addresses option to avoid race conditions
> +# due to GARP backoff
> +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses=""
> +
> +reset_pcap_file() {
> +    local iface=$1
> +    local pcap_file=$2
> +    ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \
> +options:rxq_pcap=dummy-rx.pcap
> +    rm -f ${pcap_file}*.pcap
> +    ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \
> +options:rxq_pcap=${pcap_file}-rx.pcap
> +}
> +
> +as hv1 reset_pcap_file snoopvif hv1/snoopvif
> +
> +# Re-add nat-addresses option
> +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses="nat-only-router"
> +
> +# Wait for packets to be received.
> +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 250])
> +trim_zeros() {
> +    sed 's/\(00\)\{1,\}$//'
> +}
> +
> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros > packets
> +g0="fffffffffffff0000000000108060001080006040001f00000000001c0a80001000000000000c0a80001"
> +echo $g0 > expout
> +g1="fffffffffffff0000000000108060001080006040001f00000000001c0a80002000000000000c0a80002"
> +echo $g1 >> expout
> +
> +grep $g0 packets | head -1 > exp
> +grep $g1 packets | head -1 >> exp
> +AT_CHECK([cat exp], [0], [expout])
> +
> +g3="fffffffffffff0000000000108060001080006040001f00000000001c0a80003000000000000c0a80003"
> +AT_CHECK([grep -q $g3 packets], [1])
> +
>  OVN_CLEANUP([hv1])
>
>  AT_CLEANUP
> --
> 2.35.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Feb 24, Numan Siddique wrote:
> On Thu, Feb 24, 2022 at 5:23 PM Lorenzo Bianconi
> <lorenzo.bianconi@redhat.com> wrote:
> >
> > The CMS can configure the same lbs on multiple gw routers so the
> > same IPs/mac bindings will be adverised by multiple routers
> > through GARPs.
> > In order to fix the issue, introduce nat-only-router flag for
> > nat-addresses option in order to advertise in GARPs packets all
> > SNAT/DNAT configured in the connected logical router but skip
> > all configured load_balancers.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2054394
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2053013
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Hi Lorenzo,

Hi Numan,

> 
> Thanks for updating the commit message and for the rebase.
> 
> I see one problem with this approach.  We will break the functionality of garps
> if CMS uses the new value "nat-only-router", and if ovn-northd is not updated
> to the version which has this support.
> 
> Although we could argue that it is the problem of CMS,  I think it's
> better we avoid
> this in OVN.

if ovn-northd is not updated and the CMS set nat-only-router in nat-addresses,
we should continue sending garp for all ips (including lbs)? If so, are still
having the issue of sending garps for lbs for multiple ports?

> 
> Instead I would suggest adding another option -
> "exclude-lb-vips-from-garp=true".
> If CMS sets this option in the options column of logical_switch_port, ovn-northd
> can exclude the load balancer vips from the garps.

I guess it is more clear to keep the info in nat-address option but I can live
with it :)

> 
> Let me know what you think.  Can you please also add a test case in
> ovn-northd.at

ack, will do.

> which checks that the lb vips are not set in the port_bindings options
> column if this option is set.
> 
> Can you please also add an entry in the NEWS item for this.,

ack, will do

> 
> Also one small typo below.

ack, I will fix it.

Regards,
Lorenzo

> 
> Thanks
> Numan
> 
> > ---
> > Changes since v1:
> > - improve commit message
> > - rebase on top of ovn master
> > ---
> >  northd/northd.c | 51 +++++++++++++++++++++++++++----------------------
> >  ovn-nb.xml      | 21 ++++++++++++++++++++
> >  tests/ovn.at    | 38 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 87 insertions(+), 23 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 219398f96..0d0fb7b95 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -1641,13 +1641,13 @@ destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
> >  }
> >
> >  static char **get_nat_addresses(const struct ovn_port *op, size_t *n,
> > -                                bool routable_only);
> > +                                bool routable_only, bool include_lb_ips);
> >
> >  static void
> >  assign_routable_addresses(struct ovn_port *op)
> >  {
> >      size_t n;
> > -    char **nats = get_nat_addresses(op, &n, true);
> > +    char **nats = get_nat_addresses(op, &n, true, true);
> >
> >      if (!nats) {
> >          return;
> > @@ -2711,7 +2711,8 @@ join_logical_ports(struct northd_input *input_data,
> >   * 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, bool routable_only)
> > +get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only,
> > +                  bool include_lb_ips)
> >  {
> >      size_t n_nats = 0;
> >      struct eth_addr mac;
> > @@ -2791,24 +2792,26 @@ get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only)
> >          }
> >      }
> >
> > -    const char *ip_address;
> > -    if (routable_only) {
> > -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4_routable) {
> > -            ds_put_format(&c_addresses, " %s", ip_address);
> > -            central_ip_address = true;
> > -        }
> > -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6_routable) {
> > -            ds_put_format(&c_addresses, " %s", ip_address);
> > -            central_ip_address = true;
> > -        }
> > -    } else {
> > -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) {
> > -            ds_put_format(&c_addresses, " %s", ip_address);
> > -            central_ip_address = true;
> > -        }
> > -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) {
> > -            ds_put_format(&c_addresses, " %s", ip_address);
> > -            central_ip_address = true;
> > +    if (include_lb_ips) {
> > +        const char *ip_address;
> > +        if (routable_only) {
> > +            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4_routable) {
> > +                ds_put_format(&c_addresses, " %s", ip_address);
> > +                central_ip_address = true;
> > +            }
> > +            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6_routable) {
> > +                ds_put_format(&c_addresses, " %s", ip_address);
> > +                central_ip_address = true;
> > +            }
> > +        } else {
> > +            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) {
> > +                ds_put_format(&c_addresses, " %s", ip_address);
> > +                central_ip_address = true;
> > +            }
> > +            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) {
> > +                ds_put_format(&c_addresses, " %s", ip_address);
> > +                central_ip_address = true;
> > +            }
> >          }
> >      }
> >
> > @@ -3374,10 +3377,12 @@ ovn_port_update_sbrec(struct northd_input *input_data,
> >                                             "nat-addresses");
> >              size_t n_nats = 0;
> >              char **nats = NULL;
> > -            if (nat_addresses && !strcmp(nat_addresses, "router")) {
> > +            if (nat_addresses && (!strcmp(nat_addresses, "nat-only-router") ||
> > +                                  !strcmp(nat_addresses, "router"))) {
> >                  if (op->peer && op->peer->od
> >                      && (chassis || op->peer->od->n_l3dgw_ports)) {
> > -                    nats = get_nat_addresses(op->peer, &n_nats, false);
> > +                    nats = get_nat_addresses(op->peer, &n_nats, false,
> > +                                             !strcmp(nat_addresses, "router"));
> >                  }
> >              /* Only accept manual specification of ethernet address
> >               * followed by IPv4 addresses on type "l3gateway" ports. */
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 642e91845..2d8d6ed4b 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -913,6 +913,27 @@
> >                </p>
> >              </dd>
> >
> > +            <dt><code>nat-only-router</code></dt>
> > +            <dd>
> > +              <p>
> > +                Gratuitous ARPs will be sent for all SNAT and DNAT external IP
> > +                addresses defined on the <ref column="options"
> > +                key="router-port"/>'s logical router, using the
> > +                <ref column="options" key="router-port"/>'s MAC address.
> > +              </p>
> > +
> > +              <p>
> > +                This form of <ref column="options" key="nat-addresses"/> is
> > +                valid for logical switch ports where <ref column="options"
> > +                key="router-port"/> is the name of a port on a gateway router,
> > +                or the name of a distributed gateway port.
> > +              </p>
> > +
> > +              <p>
> > +                Supported only in OVN 2.21.12.1 and later.
> s/2.21.12.1/21.12.1
> 
> Although I'd say we can probably skip this in the documentation since
> the NEWS entry would cover it.
> 
> Thanks
> Numan
> 
> > +              </p>
> > +            </dd>
> > +
> >              <dt><code>Ethernet address followed by one or more IPv4 addresses</code></dt>
> >              <dd>
> >                <p>
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index fa0e0a4a2..7f85aa6b3 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -8651,6 +8651,44 @@ expected="fffffffffffff0000000000108060001080006040001f00000000001c0a80003000000
> >  echo $expected >> expout
> >  AT_CHECK([sort packets], [0], [expout])
> >
> > +# Temporarily remove nat-addresses option to avoid race conditions
> > +# due to GARP backoff
> > +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses=""
> > +
> > +reset_pcap_file() {
> > +    local iface=$1
> > +    local pcap_file=$2
> > +    ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \
> > +options:rxq_pcap=dummy-rx.pcap
> > +    rm -f ${pcap_file}*.pcap
> > +    ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \
> > +options:rxq_pcap=${pcap_file}-rx.pcap
> > +}
> > +
> > +as hv1 reset_pcap_file snoopvif hv1/snoopvif
> > +
> > +# Re-add nat-addresses option
> > +ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses="nat-only-router"
> > +
> > +# Wait for packets to be received.
> > +OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 250])
> > +trim_zeros() {
> > +    sed 's/\(00\)\{1,\}$//'
> > +}
> > +
> > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros > packets
> > +g0="fffffffffffff0000000000108060001080006040001f00000000001c0a80001000000000000c0a80001"
> > +echo $g0 > expout
> > +g1="fffffffffffff0000000000108060001080006040001f00000000001c0a80002000000000000c0a80002"
> > +echo $g1 >> expout
> > +
> > +grep $g0 packets | head -1 > exp
> > +grep $g1 packets | head -1 >> exp
> > +AT_CHECK([cat exp], [0], [expout])
> > +
> > +g3="fffffffffffff0000000000108060001080006040001f00000000001c0a80003000000000000c0a80003"
> > +AT_CHECK([grep -q $g3 packets], [1])
> > +
> >  OVN_CLEANUP([hv1])
> >
> >  AT_CLEANUP
> > --
> > 2.35.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>