On Thu, Sep 4, 2025 at 2:57 PM Dumitru Ceara via dev <
[email protected]> wrote:
> Commit f2deb24c5c43 ("northd: Sync Advertised_Route to sb.") correctly
> skipped over "link-local connected networks" not creating
> Advertised_Route records for the link-local subnet. However, when
> implementing the connected-as-host semantics it shouldn't have skipped
> over the (non-LLA) IPs configured on routers/switches connected to said
> interfaces.
>
> It introduced an inconsistency between the case (a) when a router port is
> configured with an explicit IP (and network) and the case (b) when the
> router port is only configured with MAC address (just a link-local IPv6
> address). In (a), if the router port is configured to advertise
> "connected-as-host" ovn-northd creates Advertised_Route records for all
> IPs known to be configured on the peer switch/router. In (b), however
> it does not.
>
> This forces CMS to always configure an explicit IP on the router port
> even though that's not necessary for any of the routing decisions.
>
> Fix it by advertising the connected-as-host routes for all connected
> routes generated for a router port.
>
> NOTE: we don't try to verify that advertised host routes are actually
> reachable (there's a route prefix on the router that matches on them).
> This was already the ovn-northd behavior.
>
> Reported-at: https://issues.redhat.com/browse/FDP-1563
> Reported-by: Jakub Libosvar <[email protected]>
> Fixes: f2deb24c5c43 ("northd: Sync Advertised_Route to sb.")
> Signed-off-by: Dumitru Ceara <[email protected]>
> ---
> northd/en-advertised-route-sync.c | 72 +++++++++++++++++--------------
> tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++
> 2 files changed, 112 insertions(+), 32 deletions(-)
>
> diff --git a/northd/en-advertised-route-sync.c
> b/northd/en-advertised-route-sync.c
> index dfa9f9c447..35def9d227 100644
> --- a/northd/en-advertised-route-sync.c
> +++ b/northd/en-advertised-route-sync.c
> @@ -649,36 +649,44 @@ should_advertise_route(const struct uuidset
> *host_route_lrps,
> }
> }
>
> -/* Returns true if the route has already been fully processed for
> - * advertising (e.g., for connected routes when connected-as-host is set).
> - * Returns false otherwise, in which case the a new 'ar_entry' needs to
> - * be created for this route.
> - */
> +/* Returns true if the connected route was advertised as a set of host
> routes
> + * (/32 for IPv4 and /128 for IPv6), one for each individual IP known to
> be
> + * reachable in the connected route's subnet. Returns false otherwise. */
> static bool
> -process_prereqs_advertise_route(
> +advertise_routes_as_host_prefix(
> struct advertised_route_sync_data *data,
> struct uuidset *host_route_lrps,
> struct hmap *sync_routes,
> const struct ovn_datapath *advertising_od,
> const struct ovn_port *advertising_op,
> - const struct ovn_port *tracked_op,
> - enum route_source source)
> + enum route_source source
> +)
> {
> + if (source != ROUTE_SOURCE_CONNECTED) {
> + return false;
> + }
> +
> enum dynamic_routing_redistribute_mode drr =
> advertising_op->dynamic_routing_redistribute;
> + if (!drr_mode_CONNECTED_AS_HOST_is_set(drr)) {
> + return false;
> + }
> +
> + uuidset_insert(host_route_lrps, &advertising_op->nbrp->header_.uuid);
> + publish_host_routes(sync_routes, advertising_od, advertising_op,
> data);
> + return true;
> +}
>
> +/* Track datapaths (routers/switches) whose changes should trigger
> + * the set of advertised routes. That includes NAT and LB related
> + * advertised routes. */
> +static void
> +advertise_route_track_od(struct advertised_route_sync_data *data,
> + const struct ovn_datapath *advertising_od,
> + const struct ovn_port *tracked_op,
> + enum route_source source)
> +{
> switch (source) {
> - case ROUTE_SOURCE_CONNECTED:
> - 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, advertising_od,
> advertising_op,
> - data);
> - return true;
> - }
> - break;
> - case ROUTE_SOURCE_STATIC:
> - break;
> case ROUTE_SOURCE_NAT:
> /* If NAT route tracks port on a different DP than the one that
> * advertises the route, we need to watch for changes on that DP
> as
> @@ -702,12 +710,14 @@ process_prereqs_advertise_route(
> &tracked_op->od->nbr->header_.uuid);
> }
> break;
> + case ROUTE_SOURCE_CONNECTED:
> + case ROUTE_SOURCE_STATIC:
> + break;
> case ROUTE_SOURCE_LEARNED:
> OVS_NOT_REACHED();
> default:
> OVS_NOT_REACHED();
> }
> - return false;
> }
>
> static void
> @@ -728,23 +738,25 @@ advertised_route_table_sync(
> continue;
> }
>
> - if (prefix_is_link_local(&route->prefix, route->plen)) {
> - continue;
> - }
> -
> if (!should_advertise_route(&host_route_lrps, route->od,
> route->out_port, route->source)) {
> continue;
> }
>
> - if (process_prereqs_advertise_route(data, &host_route_lrps,
> + if (advertise_routes_as_host_prefix(data, &host_route_lrps,
> &sync_routes, route->od,
> route->out_port,
> - route->tracked_port,
> route->source)) {
> continue;
> }
>
> + if (prefix_is_link_local(&route->prefix, route->plen)) {
> + continue;
> + }
> +
> + advertise_route_track_od(data, route->od, route->tracked_port,
> + route->source);
> +
> ar_entry_add_nocopy(&sync_routes, route->od, route->out_port,
> normalize_v46_prefix(&route->prefix,
> route->plen),
> route->tracked_port,
> @@ -759,12 +771,8 @@ advertised_route_table_sync(
> continue;
> }
>
> - 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;
> - }
> + advertise_route_track_od(data, route_e->od, route_e->tracked_port,
> + route_e->source);
>
> const struct sbrec_port_binding *tracked_pb =
> route_e->tracked_port ? route_e->tracked_port->sb : NULL;
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 3d27f10cde..208bef20dc 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -15591,6 +15591,78 @@ AT_CHECK([fetch_column sb:Port_Binding options
> logical_port=lr0-sw0 | grep -q 'd
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([dynamic-routing - host routes - unnumbered LRP interfaces])
> +AT_KEYWORDS([dynamic-routing])
> +ovn_start
> +
> +dnl Logical topology:
> +dnl
> +dnl internal-lr
> +dnl |
> +dnl |
> +dnl internal-ls
> +dnl |
> +dnl +-------+----+
> +dnl | |
> +dnl | +--- ovs-bridge
> +dnl | |
> +dnl | +----+
> +dnl | |
> +dnl | public-ls
> +dnl | |
> +dnl | |
> +dnl | | (explicit IP)
> +dnl (no explicit IP) +-- public-lr (BGP enabled)
> +dnl (just LLA) |
> +dnl |
> +dnl <fabric>
> +dnl
> +dnl When configured to advertise connected-routes "as-host" prefixes
> +dnl router ports should also include connected router/switch IPs reachable
> +dnl via unnumbered router ports (that only have LLA configured).
> +
> +check ovn-nbctl --wait=sb \
> + -- lr-add internal-lr \
> + -- lrp-add internal-lr internal-lrp 00:00:00:00:00:01 1.1.1.1/24 \
> + -- ls-add internal-ls \
> + -- lsp-add internal-ls internal-ls-lrp \
> + -- lsp-set-type internal-ls-lrp router \
> + -- lsp-set-options internal-ls-lrp router-port=internal-lrp \
> + -- lsp-set-addresses internal-ls-lrp router \
> + -- lsp-add internal-ls internal-ls-lla-lrp \
> + -- lsp-set-type internal-ls-lla-lrp router \
> + -- lsp-set-options internal-ls-lla-lrp router-port=lla-lrp \
> + -- lsp-set-addresses internal-ls-lla-lrp router \
> + -- lsp-add internal-ls internal-ls-localnet \
> + -- lsp-set-type internal-ls-localnet localnet \
> + -- ls-add public-ls \
> + -- lsp-add public-ls public-ls-lrp \
> + -- lsp-set-type public-ls-lrp router \
> + -- lsp-set-options public-ls-lrp router-port=public-lrp \
> + -- lsp-set-addresses public-ls-lrp router \
> + -- lsp-add public-ls public-ls-localnet \
> + -- lsp-set-type public-ls-localnet localnet \
> + -- lr-add public-lr \
> + -- lrp-add public-lr public-lrp 00:00:00:00:00:02 1.1.1.2/24 \
> + -- lrp-add public-lr lla-lrp 00:00:00:00:00:03 \
> + -- lrp-add public-lr fab-lrp 00:00:00:00:00:04 2.2.2.1/24 \
> + -- set Logical_Router public-lr option:dynamic-routing=true \
> + -- set Logical_Router_Port lla-lrp \
> + options:dynamic-routing-redistribute="connected-as-host"
> +
> +pub_lr_dp=$(fetch_column Datapath_Binding _uuid
> external_ids:name=public-lr)
> +lla_lrp_pb=$(fetch_column Port_Binding _uuid logical_port=lla-lrp)
> +int_lrp_pb=$(fetch_column Port_Binding _uuid logical_port=internal-lrp)
> +check_column 1.1.1.1 Advertised_Route ip_prefix \
> + datapath=$pub_lr_dp \
> + logical_port=$lla_lrp_pb \
> + tracked_port=$int_lrp_pb
> +check_row_count Advertised_Route 1
> +
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD_NO_HV([
> AT_SETUP([dynamic-routing incremental processing])
> AT_KEYWORDS([dynamic-routing])
> --
> 2.50.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.
Acked-by: Ales Musil <[email protected]>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
