| Message ID | 183edfc446633c5c38d7d7361089d34432c527dd.1645793899.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v3] northd: introduce exclude-lb-vips-from-garp option for lsp | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Fri, Feb 25, 2022 at 9:25 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > The CMS can configure the same lbs on multiple gw routers so the > same VIPs will be adverised by multiple routers through GARPs. > In order to fix the issue, introduce exclude-lb-vips-from-garp > option in the logical_switch_port table in order to advertise in > GARPs packets all SNAT/DNAT configured in the connected logical > router but skip all configured load_balancers if nat-addresses > option is set to router. > > 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> Thanks. I applied to the main branch and also backported to branch-21.12. Numan > --- > Changes since v2: > - add exclude-lb-vips-from-garp option instead of nat-only-router one > - add ovn-northd unit-test > > Changes since v1: > - improve commit message > - rebase on top of ovn master > --- > NEWS | 2 ++ > northd/northd.c | 50 +++++++++++++++++++++++++-------------------- > ovn-nb.xml | 9 ++++++++ > tests/ovn-northd.at | 32 +++++++++++++++++++++++++++++ > tests/ovn.at | 38 ++++++++++++++++++++++++++++++++++ > 5 files changed, 109 insertions(+), 22 deletions(-) > > diff --git a/NEWS b/NEWS > index a309fe8df..ebc6ec353 100644 > --- a/NEWS > +++ b/NEWS > @@ -12,6 +12,8 @@ Post v21.12.0 > ovn-controller-vtep will not process any DB changes. > - When configured to log packtes matching ACLs, log the direction (logical > pipeline) too. > + - Introduce exclude-lb-vips-from-garp in logical_switch_port column in order > + to not advertise lbs VIPs in GARPs sent by the logical router. > > OVN v21.12.0 - 22 Dec 2021 > -------------------------- > diff --git a/northd/northd.c b/northd/northd.c > index 219398f96..dc7f5d122 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; > + } > } > } > > @@ -3377,7 +3380,10 @@ ovn_port_update_sbrec(struct northd_input *input_data, > if (nat_addresses && !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); > + bool exclude_lb_vips = smap_get_bool(&op->nbsp->options, > + "exclude-lb-vips-from-garp", false); > + nats = get_nat_addresses(op->peer, &n_nats, false, > + !exclude_lb_vips); > } > /* 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..9af1ea2eb 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -931,6 +931,15 @@ > </dl> > </column> > > + <column name="options" key="exclude-lb-vips-from-garp"> > + If <ref column="options" key="nat-addresses"/> is set to > + <code>router</code>, 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, > + not cosidering configured load balancers. > + </column> > + > <column name="options" key="arp_proxy"> > Optional. A list of IPv4 addresses that this > logical switch <code>router</code> port will reply to ARP requests. > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 170d015b2..106e08d76 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -5966,3 +5966,35 @@ AT_CHECK([grep -e "(lr_in_ip_routing ).*outport" lr0flows | sed 's/table=../ta > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([check exclude-lb-vips-from-garp option]) > +ovn_start > + > +ovn-nbctl lr-add R1 > +ovn-nbctl set logical_router R1 options:chassis=hv1 > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24 > + > +ovn-nbctl ls-add S1 > +ovn-nbctl lsp-add S1 S1-R1 > +ovn-nbctl lsp-set-type S1-R1 router > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01 > +ovn-nbctl --wait=sb lsp-set-options S1-R1 router-port=R1-S1 nat-addresses="router" > + > +ovn-nbctl lr-nat-add R1 snat 172.16.1.1 10.0.0.0/24 > +ovn-nbctl lr-nat-add R1 dnat 172.16.1.2 10.0.0.1 > +# Add load balancers > +ovn-nbctl lb-add lb0 172.16.1.10:80 10.0.0.1:80 > +ovn-nbctl lr-lb-add R1 lb0 > +ovn-nbctl lb-add lb1 172.16.1.10:8080 10.0.0.1:8080 > +ovn-nbctl lr-lb-add R1 lb1 > + > +AT_CHECK([ovn-sbctl get Port_Binding S1-R1 nat_addresses |grep -q 172.16.1.10], [0]) > + > +ovn-nbctl --wait=sb lsp-set-options S1-R1 router-port=R1-S1 nat-addresses="router" \ > + exclude-lb-vips-from-garp="true" > + > +AT_CHECK([ovn-sbctl get Port_Binding S1-R1 nat_addresses |grep -q 172.16.1.10], [1]) > + > +AT_CLEANUP > +]) > diff --git a/tests/ovn.at b/tests/ovn.at > index fa0e0a4a2..c1c06a5e4 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="router" exclude-lb-vips-from-garp="true" > + > +# 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/NEWS b/NEWS index a309fe8df..ebc6ec353 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,8 @@ Post v21.12.0 ovn-controller-vtep will not process any DB changes. - When configured to log packtes matching ACLs, log the direction (logical pipeline) too. + - Introduce exclude-lb-vips-from-garp in logical_switch_port column in order + to not advertise lbs VIPs in GARPs sent by the logical router. OVN v21.12.0 - 22 Dec 2021 -------------------------- diff --git a/northd/northd.c b/northd/northd.c index 219398f96..dc7f5d122 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; + } } } @@ -3377,7 +3380,10 @@ ovn_port_update_sbrec(struct northd_input *input_data, if (nat_addresses && !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); + bool exclude_lb_vips = smap_get_bool(&op->nbsp->options, + "exclude-lb-vips-from-garp", false); + nats = get_nat_addresses(op->peer, &n_nats, false, + !exclude_lb_vips); } /* 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..9af1ea2eb 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -931,6 +931,15 @@ </dl> </column> + <column name="options" key="exclude-lb-vips-from-garp"> + If <ref column="options" key="nat-addresses"/> is set to + <code>router</code>, 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, + not cosidering configured load balancers. + </column> + <column name="options" key="arp_proxy"> Optional. A list of IPv4 addresses that this logical switch <code>router</code> port will reply to ARP requests. diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 170d015b2..106e08d76 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -5966,3 +5966,35 @@ AT_CHECK([grep -e "(lr_in_ip_routing ).*outport" lr0flows | sed 's/table=../ta AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([check exclude-lb-vips-from-garp option]) +ovn_start + +ovn-nbctl lr-add R1 +ovn-nbctl set logical_router R1 options:chassis=hv1 +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24 + +ovn-nbctl ls-add S1 +ovn-nbctl lsp-add S1 S1-R1 +ovn-nbctl lsp-set-type S1-R1 router +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01 +ovn-nbctl --wait=sb lsp-set-options S1-R1 router-port=R1-S1 nat-addresses="router" + +ovn-nbctl lr-nat-add R1 snat 172.16.1.1 10.0.0.0/24 +ovn-nbctl lr-nat-add R1 dnat 172.16.1.2 10.0.0.1 +# Add load balancers +ovn-nbctl lb-add lb0 172.16.1.10:80 10.0.0.1:80 +ovn-nbctl lr-lb-add R1 lb0 +ovn-nbctl lb-add lb1 172.16.1.10:8080 10.0.0.1:8080 +ovn-nbctl lr-lb-add R1 lb1 + +AT_CHECK([ovn-sbctl get Port_Binding S1-R1 nat_addresses |grep -q 172.16.1.10], [0]) + +ovn-nbctl --wait=sb lsp-set-options S1-R1 router-port=R1-S1 nat-addresses="router" \ + exclude-lb-vips-from-garp="true" + +AT_CHECK([ovn-sbctl get Port_Binding S1-R1 nat_addresses |grep -q 172.16.1.10], [1]) + +AT_CLEANUP +]) diff --git a/tests/ovn.at b/tests/ovn.at index fa0e0a4a2..c1c06a5e4 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="router" exclude-lb-vips-from-garp="true" + +# 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
The CMS can configure the same lbs on multiple gw routers so the same VIPs will be adverised by multiple routers through GARPs. In order to fix the issue, introduce exclude-lb-vips-from-garp option in the logical_switch_port table in order to advertise in GARPs packets all SNAT/DNAT configured in the connected logical router but skip all configured load_balancers if nat-addresses option is set to router. 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 v2: - add exclude-lb-vips-from-garp option instead of nat-only-router one - add ovn-northd unit-test Changes since v1: - improve commit message - rebase on top of ovn master --- NEWS | 2 ++ northd/northd.c | 50 +++++++++++++++++++++++++-------------------- ovn-nb.xml | 9 ++++++++ tests/ovn-northd.at | 32 +++++++++++++++++++++++++++++ tests/ovn.at | 38 ++++++++++++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 22 deletions(-)