When "connected-as-host" was configured for option
"dynamic-routing-redistribute" OVN would attempt to advertise also
NATs and LBs connected to that LRP. Prevent that by advertising only
the LRP networks. This also prevents commit fail loop when
"connected-as-host" and "nat" are used at the same time. Also fix
the case when there is "conflict" between LRP IP and SNAT. In
that case honor the LRP IP advertised route.
Fixes: 93f541f342f9 ("northd: Allow announcing individual host routes.")
Reported-at: https://issues.redhat.com/browse/FDP-1564
Signed-off-by: Ales Musil <amu...@redhat.com>
---
v2: Rebase on top of latest main.
Make sure the tracked_port is properly set back if needed.
---
northd/en-advertised-route-sync.c | 87 ++++++++++++-------------------
tests/ovn-northd.at | 16 +++---
tests/system-ovn.at | 4 +-
3 files changed, 42 insertions(+), 65 deletions(-)
diff --git a/northd/en-advertised-route-sync.c
b/northd/en-advertised-route-sync.c
index 6c653529b..e75ab15c5 100644
--- a/northd/en-advertised-route-sync.c
+++ b/northd/en-advertised-route-sync.c
@@ -127,7 +127,6 @@ static void
advertised_route_table_sync(
struct ovsdb_idl_txn *ovnsb_txn,
const struct sbrec_advertised_route_table *sbrec_advertised_route_table,
- const struct lr_stateful_table *lr_stateful_table,
const struct hmap *routes,
const struct hmap *dynamic_routes,
struct advertised_route_sync_data *data);
@@ -245,14 +244,11 @@ en_advertised_route_sync_run(struct engine_node *node,
void *data OVS_UNUSED)
const struct engine_context *eng_ctx = engine_get_context();
const struct sbrec_advertised_route_table *sbrec_advertised_route_table =
EN_OVSDB_GET(engine_get_input("SB_advertised_route", node));
- struct ed_type_lr_stateful *lr_stateful_data =
- engine_get_input_data("lr_stateful", node);
stopwatch_start(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME, time_msec());
advertised_route_table_sync(eng_ctx->ovnsb_idl_txn,
sbrec_advertised_route_table,
- &lr_stateful_data->table,
&routes_data->parsed_routes,
&dynamic_routes_data->routes,
routes_sync_data);
@@ -564,40 +560,10 @@ publish_lport_addresses(struct hmap *sync_routes,
}
}
-/* Collect all IP addresses connected via this LRP. */
-static void
-publish_host_routes_lrp(struct hmap *sync_routes,
- const struct lr_stateful_table *lr_stateful_table,
- const struct ovn_datapath *advertising_od,
- const struct ovn_port *advertising_op,
- struct advertised_route_sync_data *data,
- const struct ovn_port *lrp)
-{
- /* This is a LSP connected to an LRP */
- const struct lport_addresses *addresses = &lrp->lrp_networks;
- publish_lport_addresses(sync_routes, advertising_od,
- advertising_op, addresses, lrp);
-
- const struct lr_stateful_record *lr_stateful_rec;
- lr_stateful_rec =
- lr_stateful_table_find_by_index(lr_stateful_table, lrp->od->index);
- /* We also need to track this LR as we need to recompute when
- * any of its IPs change. */
- uuidset_insert(&data->nb_lr, &lr_stateful_rec->nbr_uuid);
- struct ovn_port_routable_addresses addrs =
- get_op_addresses(lrp, lr_stateful_rec, false);
- for (size_t i = 0; i < addrs.n_addrs; i++) {
- publish_lport_addresses(sync_routes, advertising_od,
- advertising_op, &addrs.laddrs[i], lrp);
- }
- destroy_routable_addresses(&addrs);
-}
-
/* Collect all IP addresses connected to the out_port of a route.
* This traverses all LSPs on the LS connected to the out_port. */
static void
publish_host_routes(struct hmap *sync_routes,
- const struct lr_stateful_table *lr_stateful_table,
const struct ovn_datapath *advertising_od,
const struct ovn_port *advertising_op,
struct advertised_route_sync_data *data)
@@ -613,8 +579,9 @@ publish_host_routes(struct hmap *sync_routes,
if (peer_od->nbr) {
/* This is a LRP directly connected to another LRP. */
- publish_host_routes_lrp(sync_routes, lr_stateful_table, advertising_od,
- advertising_op, data, advertising_op->peer);
+ const struct ovn_port *lrp = advertising_op->peer;
+ publish_lport_addresses(sync_routes, advertising_od,
+ advertising_op, &lrp->lrp_networks, lrp);
return;
}
@@ -626,9 +593,9 @@ publish_host_routes(struct hmap *sync_routes,
HMAP_FOR_EACH (port, dp_node, &peer_od->ports) {
if (port->peer && port->peer->nbrp) {
/* This is a LSP connected to an LRP */
- publish_host_routes_lrp(sync_routes, lr_stateful_table,
- advertising_od, advertising_op,
- data, port->peer);
+ const struct ovn_port *lrp = port->peer;
+ publish_lport_addresses(sync_routes, advertising_od,
+ advertising_op, &lrp->lrp_networks, lrp);
} else {
/* This is just a plain LSP */
for (size_t i = 0; i < port->n_lsp_addrs; i++) {
@@ -688,7 +655,6 @@ should_advertise_route(const struct uuidset
*host_route_lrps,
*/
static bool
process_prereqs_advertise_route(
- const struct lr_stateful_table *lr_stateful_table,
struct advertised_route_sync_data *data,
struct uuidset *host_route_lrps,
struct hmap *sync_routes,
@@ -705,8 +671,7 @@ process_prereqs_advertise_route(
if (drr_mode_CONNECTED_AS_HOST_is_set(drr)) {
const struct uuid *lrp_uuid = &advertising_op->nbrp->header_.uuid;
uuidset_insert(host_route_lrps, lrp_uuid);
- publish_host_routes(sync_routes, lr_stateful_table,
- advertising_od, advertising_op,
+ publish_host_routes(sync_routes, advertising_od, advertising_op,
data);
return true;
}
@@ -748,7 +713,6 @@ static void
advertised_route_table_sync(
struct ovsdb_idl_txn *ovnsb_txn,
const struct sbrec_advertised_route_table *sbrec_advertised_route_table,
- const struct lr_stateful_table *lr_stateful_table,
const struct hmap *routes,
const struct hmap *dynamic_routes,
struct advertised_route_sync_data *data)
@@ -772,9 +736,9 @@ advertised_route_table_sync(
continue;
}
- if (process_prereqs_advertise_route(lr_stateful_table, data,
- &host_route_lrps, &sync_routes,
- route->od, route->out_port,
+ if (process_prereqs_advertise_route(data, &host_route_lrps,
+ &sync_routes, route->od,
+ route->out_port,
route->tracked_port,
route->source)) {
continue;
@@ -794,14 +758,22 @@ advertised_route_table_sync(
continue;
}
- if (process_prereqs_advertise_route(lr_stateful_table, data,
- &host_route_lrps, &sync_routes,
- route_e->od, route_e->op,
- route_e->tracked_port,
+ if (process_prereqs_advertise_route(data, &host_route_lrps,
+ &sync_routes, route_e->od,
+ route_e->op, route_e->tracked_port,
route_e->source)) {
continue;
}
+ const struct sbrec_port_binding *tracked_pb =
+ route_e->tracked_port ? route_e->tracked_port->sb : NULL;
+ if (ar_entry_find(&sync_routes, route_e->od->sb, route_e->op->sb,
+ route_e->ip_prefix, tracked_pb)) {
+ /* We could already have advertised route entry for LRP IP that
+ * corresponds to "snat" when "connected-as-host" is combined
+ * with "nat". Skip it. */
+ continue;
+ }
ar_entry_add(&sync_routes, route_e->od, route_e->op,
route_e->ip_prefix, route_e->tracked_port,
route_e->source);
@@ -814,12 +786,17 @@ advertised_route_table_sync(
route_e = ar_entry_find(&sync_routes, sb_route->datapath,
sb_route->logical_port, sb_route->ip_prefix,
sb_route->tracked_port);
- if (route_e) {
- hmap_remove(&sync_routes, &route_e->hmap_node);
- ar_entry_free(route_e);
- } else {
- sbrec_advertised_route_delete(sb_route);
+ if (!route_e) {
+ sbrec_advertised_route_delete(sb_route);
+ continue;
}
+
+ if (route_e->tracked_port && !sb_route->tracked_port) {
+ sbrec_advertised_route_set_tracked_port(
+ sb_route, route_e->tracked_port->sb);
+ }
+ hmap_remove(&sync_routes, &route_e->hmap_node);
+ ar_entry_free(route_e);
}
HMAP_FOR_EACH_POP (route_e, hmap_node, &sync_routes) {
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 5ddb15587..ec7009b28 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -15545,7 +15545,7 @@ datapath=$(fetch_column datapath_binding _uuid
external_ids:name=lr0)
# Configuring the LRP lr0-sw0 to send host routes.
# As sw0 is quite empty we will only see the addresses of lr0-sw0.
check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 \
- options:dynamic-routing-redistribute="connected-as-host,static"
+ options:dynamic-routing-redistribute="connected-as-host,static,nat"
check_row_count Advertised_Route 2
check_column 10.0.0.1 Advertised_Route ip_prefix \
datapath=$datapath logical_port=$sw0
@@ -15577,14 +15577,14 @@ check_column $lr1 Advertised_Route tracked_port \
# Adding a NAT rule to lr1 will advertise it as well.
check ovn-nbctl --wait=sb lr-nat-add lr1 dnat_and_snat 10.0.0.100 192.168.0.1
check_row_count Advertised_Route 5
-check_row_count Advertised_Route 4 tracked_port!='[[]]'
-check_column $lr1 Advertised_Route tracked_port \
+check_row_count Advertised_Route 3 tracked_port!='[[]]'
+check_column '' Advertised_Route tracked_port \
datapath=$datapath logical_port=$sw0 ip_prefix=10.0.0.100
# Adding a static route to lr1 will be advertised just normally.
check ovn-nbctl --wait=sb lr-route-add lr0 172.16.0.0/24 10.0.0.200
check_row_count Advertised_Route 6
-check_row_count Advertised_Route 4 tracked_port!='[[]]'
+check_row_count Advertised_Route 3 tracked_port!='[[]]'
check_row_count Advertised_Route 1 datapath=$datapath logical_port=$sw0 \
ip_prefix=172.16.0.0/24
@@ -15593,16 +15593,16 @@ check_row_count Advertised_Route 1 datapath=$datapath
logical_port=$sw0 \
check ovn-nbctl lr-add lr2
check ovn-nbctl lrp-add lr0 lr0-lr2 00:00:00:02:ff:01 10.10.0.1/24 peer=lr2-lr0
check ovn-nbctl set Logical_Router_Port lr0-lr2 \
- options:dynamic-routing-redistribute="connected-as-host,static"
+ options:dynamic-routing-redistribute="connected-as-host,static,nat"
lr0lr2=$(fetch_column port_binding _uuid logical_port=lr0-lr2)
check ovn-nbctl lrp-add lr2 lr2-lr0 00:00:00:02:ff:02 10.10.0.2/24 peer=lr0-lr2
lr2lr0=$(fetch_column port_binding _uuid logical_port=lr2-lr0)
check ovn-nbctl lrp-add lr2 lr2-sw1 00:00:00:02:ff:03 10.11.0.1/24
check ovn-nbctl --wait=sb lr-nat-add lr2 dnat_and_snat 10.10.0.20 10.20.0.123
check_row_count Advertised_Route 8
-check_row_count Advertised_Route 6 tracked_port!='[[]]'
+check_row_count Advertised_Route 4 tracked_port!='[[]]'
check_row_count Advertised_Route 1 datapath=$datapath logical_port=$lr0lr2 \
- ip_prefix=10.10.0.20 tracked_port=$lr2lr0
+ ip_prefix=10.10.0.20 tracked_port='[[]]'
# Add a directly connected switch and make sure that northd is not confused.
# The IP on a switch-switch port should be advertised.
@@ -15615,7 +15615,7 @@ check ovn-nbctl --wait=sb lsp-set-addresses sw0-sw1 \
"11:aa:bb:cc:dd:ee 10.0.0.42"
sw0sw1=$(fetch_column port_binding _uuid logical_port=sw0-sw1)
check_row_count Advertised_Route 9
-check_row_count Advertised_Route 7 tracked_port!='[[]]'
+check_row_count Advertised_Route 5 tracked_port!='[[]]'
check_column $sw0sw1 Advertised_Route tracked_port \
datapath=$datapath logical_port=$sw0 ip_prefix=10.0.0.42
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index e0407383a..e99cf6f3d 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -16125,7 +16125,7 @@ blackhole 198.51.100.0/24 proto ovn metric 1000])
# The last 3 of them are local to the current chassis so we expect a better
# prio.
check ovn-nbctl --wait=hv set Logical_Router_Port internet-public \
- options:dynamic-routing-redistribute="connected-as-host,static"
+ options:dynamic-routing-redistribute="connected-as-host,static,nat"
OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
blackhole 192.0.2.1 proto ovn metric 1000
@@ -16485,7 +16485,7 @@ blackhole 198.51.100.0/24 proto ovn metric 1000])
# The last 3 of them are local to the current chassis so we expect a better
# prio.
check ovn-nbctl --wait=hv set Logical_Router_Port internet-public \
- options:dynamic-routing-redistribute="connected-as-host,static"
+ options:dynamic-routing-redistribute="connected-as-host,static,nat"
OVN_ROUTE_EQUAL([ovnvrf1337], [dnl
blackhole 192.0.2.1 proto ovn metric 100
--
2.50.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
