| Message ID | 00d7b6da2c63cabbb4cad719786bdd8c86755214.1645119140.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] northd: introduce nat-only-router for nat-addresses option | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
On Thu, Feb 17, 2022 at 12:34 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > 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 > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Hi Lorenzo, This patch needs a rebase. Looking into the linked BZ, it seems like an issue from ovn-kubernetes point of view. Can you please update the commit message with the issue faced. Maybe this can be a candidate for the backport. > --- > northd/northd.c | 53 ++++++++++++++++++++++++++++--------------------- > ovn-nb.xml | 21 ++++++++++++++++++++ > tests/ovn.at | 38 +++++++++++++++++++++++++++++++++++ > 3 files changed, 89 insertions(+), 23 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 4f9c10648..e6483658e 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 nat_only); > > 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, false); > > 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 nat_only) The argument 'nat_only' is used more as a negation below. So I'd suggest changing the name as - "include_lb_ips" or a better name if you've in mind. Otherwise the patch LGTM. Thanks Numan > { > 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 (!nat_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; > + } > } > } > > @@ -3375,10 +3378,14 @@ ovn_port_update_sbrec(struct northd_input *input_data, > char **nats = NULL; > bool l3dgw_ports = op->peer && op->peer->od && > op->peer->od->n_l3dgw_ports; > - if (nat_addresses && !strcmp(nat_addresses, "router")) { > + if (nat_addresses && > + (!strcmp(nat_addresses, "router") || > + !strcmp(nat_addresses, "nat-only-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, > + "nat-only-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 e8aa8b863..e5947ac63 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. > + </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 184594831..b80ab803f 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 >
diff --git a/northd/northd.c b/northd/northd.c index 4f9c10648..e6483658e 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 nat_only); 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, false); 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 nat_only) { 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 (!nat_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; + } } } @@ -3375,10 +3378,14 @@ ovn_port_update_sbrec(struct northd_input *input_data, char **nats = NULL; bool l3dgw_ports = op->peer && op->peer->od && op->peer->od->n_l3dgw_ports; - if (nat_addresses && !strcmp(nat_addresses, "router")) { + if (nat_addresses && + (!strcmp(nat_addresses, "router") || + !strcmp(nat_addresses, "nat-only-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, + "nat-only-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 e8aa8b863..e5947ac63 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. + </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 184594831..b80ab803f 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
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 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/northd.c | 53 ++++++++++++++++++++++++++++--------------------- ovn-nb.xml | 21 ++++++++++++++++++++ tests/ovn.at | 38 +++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 23 deletions(-)