On 4/4/25 7:00 PM, Paulo Guilherme da Silva via dev wrote: > This commit adds the ability for ovn-ic to filter routes between VPCs. > In use cases where the user is using vpc-peering(route tag) handling > multiple points of redistribution, when we connect more than one TS in > the same Logical Router it is essential that we have control over which > routes will be filtered or advertised by transit switch. > > Co-authored-by: Lucas Vargas Dias <lucas.vd...@luizalabs.com> > Signed-off-by: Lucas Vargas Dias <lucas.vd...@luizalabs.com> > Signed-off-by: Paulo Guilherme da Silva <guilherme.pa...@luizalabs.com> > ---
Hi Paulo, Sorry for the delay in reviewing v5. > NEWS | 3 ++ > ic/ovn-ic.c | 124 ++++++++++++++++++++++++++--------------------- > lib/ovn-util.c | 53 ++++++++++++++++++++ > lib/ovn-util.h | 4 ++ > ovn-nb.xml | 44 ++++++++++++++++- > tests/ovn-ic.at | 126 ++++++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 299 insertions(+), 55 deletions(-) > > diff --git a/NEWS b/NEWS > index f7813184e..4476aa32c 100644 > --- a/NEWS > +++ b/NEWS > @@ -17,6 +17,9 @@ Post v25.03.0 > - SSL/TLS: > * Support for deprecated TLSv1 and TLSv1.1 protocols on OpenFlow and > database connections is now removed. > + - Added "ic-route-filter-adv" and "ic-route-filter-learn" options to > + the Logical_Router/Logical_Router_Port tables to allow users to > + filter advertised/learned IC routes. > > OVN v25.03.0 - 07 Mar 2025 > -------------------------- > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index c8796680b..53bf1dfc9 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -1069,69 +1069,69 @@ get_nexthop_from_lport_addresses(bool is_v4, > return true; > } > > +static bool > +prefix_is_filtered(struct in6_addr *prefix, > + unsigned int plen, > + const struct nbrec_logical_router *nb_lr, > + const struct nbrec_logical_router_port *ts_lrp, > + bool is_advertisement) > +{ > + struct ds filter_list = DS_EMPTY_INITIALIZER; > + const char *filter_direction = is_advertisement ? "ic-route-filter-adv" : > + > "ic-route-filter-learn"; > + if (ts_lrp) { > + const char *lrp_route_filter = smap_get(&ts_lrp->options, > + filter_direction); > + if (lrp_route_filter) { > + ds_put_format(&filter_list, "%s,", lrp_route_filter); > + } > + } > + const char *lr_route_filter = smap_get(&nb_lr->options, > + filter_direction); > + if (lr_route_filter) { > + ds_put_format(&filter_list, "%s,", lr_route_filter); > + } > + > + struct sset prefix_list = SSET_INITIALIZER(&prefix_list); > + sset_from_delimited_string(&prefix_list, ds_cstr(&filter_list), ","); > + > + bool matched = find_prefix_in_list(prefix, plen, &prefix_list, > + filter_direction); > + ds_destroy(&filter_list); > + sset_destroy(&prefix_list); > + return matched; > +} > + > static bool > prefix_is_deny_listed(const struct smap *nb_options, > struct in6_addr *prefix, > unsigned int plen) > { > - const char *denylist = smap_get(nb_options, "ic-route-denylist"); > + const char *filter_name = "ic-route-denylist"; > + const char *denylist = smap_get(nb_options, filter_name); > if (!denylist || !denylist[0]) { > denylist = smap_get(nb_options, "ic-route-blacklist"); > if (!denylist || !denylist[0]) { > return false; > } > } > - struct in6_addr bl_prefix; > - unsigned int bl_plen; > - char *cur, *next, *start; > - next = start = xstrdup(denylist); > - bool matched = false; > - while ((cur = strsep(&next, ",")) && *cur) { > - if (!ip46_parse_cidr(cur, &bl_prefix, &bl_plen)) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_WARN_RL(&rl, "Bad format in nb_global options:" > - "ic-route-denylist: %s. CIDR expected.", cur); > - continue; > - } > > - if (IN6_IS_ADDR_V4MAPPED(&bl_prefix) != > IN6_IS_ADDR_V4MAPPED(prefix)) { > - continue; > - } > - > - /* 192.168.0.0/16 does not belong to 192.168.0.0/17 */ > - if (plen < bl_plen) { > - continue; > - } > + struct sset prefix_list = SSET_INITIALIZER(&prefix_list); > + sset_from_delimited_string(&prefix_list, denylist, ","); > > - if (IN6_IS_ADDR_V4MAPPED(prefix)) { > - ovs_be32 bl_prefix_v4 = in6_addr_get_mapped_ipv4(&bl_prefix); > - ovs_be32 prefix_v4 = in6_addr_get_mapped_ipv4(prefix); > - ovs_be32 mask = be32_prefix_mask(bl_plen); > - > - if ((prefix_v4 & mask) != (bl_prefix_v4 & mask)) { > - continue; > - } > - } else { > - struct in6_addr bl_mask = ipv6_create_mask(bl_plen); > - struct in6_addr m_prefix = ipv6_addr_bitand(prefix, &bl_mask); > - struct in6_addr m_bl_prefix = ipv6_addr_bitand(&bl_prefix, > - &bl_mask); > - if (!ipv6_addr_equals(&m_prefix, &m_bl_prefix)) { > - continue; > - } > - } > - matched = true; > - break; > - } > - free(start); > - return matched; > + bool denied = find_prefix_in_list(prefix, plen, &prefix_list, > + filter_name); > + sset_destroy(&prefix_list); > + return denied; > } > > static bool > route_need_advertise(const char *policy, > struct in6_addr *prefix, > unsigned int plen, > - const struct smap *nb_options) > + const struct smap *nb_options, > + const struct nbrec_logical_router *nb_lr, > + const struct nbrec_logical_router_port *ts_lrp) > { > if (!smap_get_bool(nb_options, "ic-route-adv", false)) { > return false; > @@ -1153,6 +1153,11 @@ route_need_advertise(const char *policy, > if (prefix_is_deny_listed(nb_options, prefix, plen)) { > return false; > } > + > + if (!prefix_is_filtered(prefix, plen, nb_lr, ts_lrp, true)) { > + return false; > + } > + > return true; > } > > @@ -1207,7 +1212,8 @@ add_static_to_routes_ad( > const struct nbrec_logical_router *nb_lr, > const struct lport_addresses *nexthop_addresses, > const struct smap *nb_options, > - const char *route_tag) > + const char *route_tag, > + const struct nbrec_logical_router_port *ts_lrp) > { > struct in6_addr prefix, nexthop; > unsigned int plen; > @@ -1216,7 +1222,8 @@ add_static_to_routes_ad( > return; > } > > - if (!route_need_advertise(nb_route->policy, &prefix, plen, nb_options)) { > + if (!route_need_advertise(nb_route->policy, &prefix, plen, nb_options, > + nb_lr, ts_lrp)) { > return; > } > > @@ -1257,7 +1264,8 @@ add_network_to_routes_ad(struct hmap *routes_ad, const > char *network, > const struct lport_addresses *nexthop_addresses, > const struct smap *nb_options, > const struct nbrec_logical_router *nb_lr, > - const char *route_tag) > + const char *route_tag, > + const struct nbrec_logical_router_port *ts_lrp) > { > struct in6_addr prefix, nexthop; > unsigned int plen; > @@ -1265,7 +1273,8 @@ add_network_to_routes_ad(struct hmap *routes_ad, const > char *network, > return; > } > > - if (!route_need_advertise(NULL, &prefix, plen, nb_options)) { > + if (!route_need_advertise(NULL, &prefix, plen, nb_options, > + nb_lr, ts_lrp)) { > VLOG_DBG("Route ad: skip network %s of lrp %s.", > network, nb_lrp->name); > return; > @@ -1319,7 +1328,8 @@ static bool > route_need_learn(const struct nbrec_logical_router *lr, > const struct icsbrec_route *isb_route, > struct in6_addr *prefix, unsigned int plen, > - const struct smap *nb_options) > + const struct smap *nb_options, > + const struct nbrec_logical_router_port *ts_lrp) > { > if (!smap_get_bool(nb_options, "ic-route-learn", false)) { > return false; > @@ -1338,6 +1348,11 @@ route_need_learn(const struct nbrec_logical_router *lr, > return false; > } > > + if (!prefix_is_filtered(prefix, plen, lr, ts_lrp, false)) { > + return false; > + } > + > + > if (route_has_local_gw(lr, isb_route->route_table, > isb_route->ip_prefix)) { > VLOG_DBG("Skip learning %s (rtb:%s) route, as we've got one with " > "local GW", isb_route->ip_prefix, isb_route->route_table); > @@ -1467,7 +1482,7 @@ sync_learned_routes(struct ic_context *ctx, > continue; > } > if (!route_need_learn(ic_lr->lr, isb_route, &prefix, plen, > - &nb_global->options)) { > + &nb_global->options, lrp)) { > continue; > } > > @@ -1653,7 +1668,8 @@ build_ts_routes_to_adv(struct ic_context *ctx, > struct lport_addresses *ts_port_addrs, > const struct nbrec_nb_global *nb_global, > const char *ts_route_table, > - const char *route_tag) > + const char *route_tag, > + const struct nbrec_logical_router_port *ts_lrp) > { > const struct nbrec_logical_router *lr = ic_lr->lr; > > @@ -1676,7 +1692,7 @@ build_ts_routes_to_adv(struct ic_context *ctx, > } else if (!strcmp(ts_route_table, nb_route->route_table)) { > /* It may be a route to be advertised */ > add_static_to_routes_ad(routes_ad, nb_route, lr, ts_port_addrs, > - &nb_global->options, route_tag); > + &nb_global->options, route_tag, ts_lrp); > } > } > > @@ -1688,7 +1704,7 @@ build_ts_routes_to_adv(struct ic_context *ctx, > add_network_to_routes_ad(routes_ad, lrp->networks[j], lrp, > ts_port_addrs, > &nb_global->options, > - lr, route_tag); > + lr, route_tag, ts_lrp); > } > } else { > /* The router port of the TS port is ignored. */ > @@ -1752,7 +1768,7 @@ collect_lr_routes(struct ic_context *ctx, > route_tag = ""; > } > build_ts_routes_to_adv(ctx, ic_lr, routes_ad, &ts_port_addrs, > - nb_global, route_table, route_tag); > + nb_global, route_table, route_tag, lrp); > destroy_lport_addresses(&ts_port_addrs); > } > } > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index f406114db..dae3403fd 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -1383,6 +1383,59 @@ prefix_is_link_local(const struct in6_addr *prefix, > unsigned int plen) > ((prefix->s6_addr[1] & 0xc0) == 0x80)); > } > > +bool > +find_prefix_in_list(const struct in6_addr *prefix, unsigned int plen, > + const struct sset *prefix_list, const char *filter_name) > +{ > + if (sset_is_empty(prefix_list)) { > + return true; This seems a bit weird. If the set is empty then no prefix can be found in it, including 'prefix'. This is supposed to be a generic utility. If you need semantics like "by default, if the prefix list is empty, assume the filter matches " then maybe you should move the check for empty sset in the caller. > + } > + > + > + bool matched = false; > + struct in6_addr lt_prefix; > + const char *cur_prefix; > + unsigned int lt_plen; > + > + SSET_FOR_EACH (cur_prefix, prefix_list) { > + if (!ip46_parse_cidr(cur_prefix, <_prefix, <_plen)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "Bad format in LR, LRP or nb_global options:" > + "%s: %s. CIDR expected.", filter_name, cur_prefix); Again, this is a generic utility, we shouldn't reference LR/LRP/nb_global in the warning log. What if in the future we call it from other places. Let's just log something like: VLOG_WARN_RL(&rl, "Bad prefix (%s) format: %s. CIDR expected.", filter_name, cur_prefix); > + continue; > + } > + > + if (IN6_IS_ADDR_V4MAPPED(<_prefix) != > IN6_IS_ADDR_V4MAPPED(prefix)) { > + continue; > + } > + > + /* 192.168.0.0/16 does not belong to 192.168.0.0/17 */ > + if (plen < lt_plen) { > + continue; > + } > + > + if (IN6_IS_ADDR_V4MAPPED(prefix)) { > + ovs_be32 bl_prefix_v4 = in6_addr_get_mapped_ipv4(<_prefix); > + ovs_be32 prefix_v4 = in6_addr_get_mapped_ipv4(prefix); > + ovs_be32 mask = be32_prefix_mask(lt_plen); > + > + if ((prefix_v4 & mask) != (bl_prefix_v4 & mask)) { > + continue; > + } > + } else { > + struct in6_addr bl_mask = ipv6_create_mask(lt_plen); > + struct in6_addr m_prefix = ipv6_addr_bitand(prefix, &bl_mask); > + struct in6_addr m_bl_prefix = ipv6_addr_bitand(<_prefix, > + &bl_mask); > + if (!ipv6_addr_equals(&m_prefix, &m_bl_prefix)) { > + continue; > + } > + } > + matched = true; Just "return true;" here. Then we don't need the 'matched' variable anymore. > + break; > + } > + return matched; > +} > const struct sbrec_port_binding * > lport_lookup_by_name(struct ovsdb_idl_index *sbrec_port_binding_by_name, > const char *name) > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 0fff9b463..f2143e247 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -490,6 +490,10 @@ bool ovn_update_swconn_at(struct rconn *swconn, const > char *target, > > bool prefix_is_link_local(const struct in6_addr *prefix, unsigned int plen); > > +bool find_prefix_in_list(const struct in6_addr *prefix, unsigned int plen, > + const struct sset *prefix_list, > + const char *filter_name); > + > void ovn_debug_commands_register(void); > > const struct sbrec_port_binding *lport_lookup_by_name( > diff --git a/ovn-nb.xml b/ovn-nb.xml > index bf1f1628b..a6dc7cc58 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -3167,8 +3167,29 @@ or > for established sessions as we will commit everything in addition > to already existing stateful NATs and LBs. > </column> > - </group> > > + <column name="options" key="ic-route-filter-adv"> > + <p> > + This option expects list of CIDRs delimited by "," that's present > + in the Logical Router. A route will not be advertised if the > + route's prefix belongs to any of the CIDRs listed. > + > + This allows to filter CIDR prefixes in the process of advertising > + routes in <code>ovn-ic</code> daemon. > + </p> > + </column> > + > + <column name="options" key="ic-route-filter-learn"> > + <p> > + This option expects list of CIDRs delimited by "," that's present > + in the Logical Router. A route will not be learned if the > + route's prefix belongs to any of the CIDRs listed. > + > + This allows to filter CIDR prefixes in the process of learning > + routes in <code>ovn-ic</code> daemon. > + </p> > + </column> > + </group> > <group title="Common Columns"> > <column name="external_ids"> > See <em>External IDs</em> at the beginning of this document. > @@ -4103,6 +4124,27 @@ or > instead of the autodiscovery. Also it is then irrelevant if the port > is bound locally. > </column> > + <column name="options" key="ic-route-filter-adv"> > + <p> > + This option expects list of CIDRs delimited by "," that's present > + in the Logical Router Port. A route will be advertised if the > + route's prefix belongs to any of the CIDRs listed. > + > + This allows to filter CIDR prefixes in the process of advertising > + routes in <code>ovn-ic</code> daemon. > + </p> > + </column> > + > + <column name="options" key="ic-route-filter-learn"> > + <p> > + This option expects list of CIDRs delimited by "," that's present > + in the Logical Router Port. A route will be learned if the > + route's prefix belongs to any of the CIDRs listed. > + > + This allows to filter CIDR prefixes in the process of learning > + routes in <code>ovn-ic</code> daemon. > + </p> > + </column> > </group> > > <group title="Attachment"> > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > index c8276189c..94fd29122 100644 > --- a/tests/ovn-ic.at > +++ b/tests/ovn-ic.at > @@ -3002,3 +3002,129 @@ OVN_CHECK_PACKETS([hv3/vif5-tx.pcap], [expected-vif5]) > OVN_CLEANUP_IC([az1], [az2], [az3]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-ic -- prefix filter -- filtering routes]) > + > +ovn_init_ic_db > +ovn-ic-nbctl ts-add ts1 > + > +for i in 1 2; do > + ovn_start az$i > + ovn_as az$i > + > + # Enable route learning at AZ level > + check ovn-nbctl set nb_global . options:ic-route-learn=true > + # Enable route advertising at AZ level > + check ovn-nbctl set nb_global . options:ic-route-adv=true > +done > + > +# Create new transit switche and LRs. Test topology is next: > +# logical router (lr11) - transit switch (ts1) - logical router (lr12) > + > + > +# create lr11, lr12 and connect them to ts1 For all the comments in this new test: please write comments as sentences (starting with capital letter and ending with period). > +for i in 1 2; do > + ovn_as az$i > + > + lr=lr1$i > + check ovn-nbctl lr-add $lr > + > + lrp=lrp-$lr-ts1 > + lsp=lsp-ts1-$lr > + # Create LRP and connect to TS > + check ovn-nbctl lrp-add $lr $lrp aa:aa:aa:aa:ab:0$i 169.254.101.$i/24 > fe80:10::$i/64 > + check ovn-nbctl lsp-add ts1 $lsp \ > + -- lsp-set-addresses $lsp router \ > + -- lsp-set-type $lsp router \ > + -- lsp-set-options $lsp router-port=$lrp > +done > + > + > + Nit: let's remove two of these empty lines. > +# Create directly-connected route in lr12 > + > +ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12-1 aa:aa:aa:aa:bb:01 > "192.168.12.1/24" "2001:db12::1/64" > +ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12-2 aa:aa:aa:aa:bb:02 > "192.168.2.1/24" "2001:db22::1/64" > + Missing call to "check"? > + > +# Create directly-connected route in lr11 > +ovn_as az1 ovn-nbctl --wait=sb lrp-add lr11 lrp-lr11 aa:aa:aa:aa:cc:01 > "192.168.11.1/24" "2001:db11::1/64" Here too. > + > +check ovn-ic-nbctl --wait=sb sync > + > +# Test direct routes from lr12 were learned to lr11. > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +192.168.12.0/24 169.254.101.2 > +192.168.2.0/24 169.254.101.2 > +]) > + > +# Test direct routes from lr11 were learned to lr12. > +OVS_WAIT_FOR_OUTPUT([ovn_as az2 ovn-nbctl lr-route-list lr12 | grep 192.168 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +192.168.11.0/24 169.254.101.1 > +]) > + > +ovn_as az2 check ovn-nbctl set logical_router_port lrp-lr12-ts1 > options:ic-route-filter-adv=192.168.12.0/24 > +# Test direct routes from lr12 were learned to lr11. > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +192.168.12.0/24 169.254.101.2 > +]) > + > +# Set ic-route-filter-adv with invalid CIDR to advertise just the valid > route. > +ovn_as az2 check ovn-nbctl set logical_router_port lrp-lr12-ts1 > options:ic-route-filter-adv="192.168.12.0/24,invalid" > + > +# Test direct routes from lr12 were learned to lr11 > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +192.168.12.0/24 169.254.101.2 > +]) > + > +ovn_as az2 check ovn-nbctl remove logical_router_port lrp-lr12-ts1 options > ic-route-filter-adv > +ovn_as az1 check ovn-nbctl set logical_router_port lrp-lr11-ts1 > options:ic-route-filter-learn=192.168.2.0/24 > + > +# Test direct routes from lr12 were learned to lr11. > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +192.168.2.0/24 169.254.101.2 > +]) > + > + > +ovn_as az1 check ovn-nbctl remove logical_router_port lrp-lr11-ts1 options > ic-route-filter-learn > + > +# Test direct routes from lr12 were learned to lr11. > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 2001 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +2001:db12::/64 fe80:10::2 > +2001:db22::/64 fe80:10::2 > +]) > + > +ovn_as az2 check ovn-nbctl set logical_router_port lrp-lr12-ts1 > options:ic-route-filter-adv=2001:db22::/64 > + > +# Test direct routes from lr12 were learned to lr11. > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 2001 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +2001:db22::/64 fe80:10::2 > +]) > + > +ovn_as az2 check ovn-nbctl remove logical_router_port lrp-lr12-ts1 options > ic-route-filter-adv > +# Test direct routes from lr12 were learned to lr11. > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 2001 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +2001:db12::/64 fe80:10::2 > +2001:db22::/64 fe80:10::2 > +]) > + > +ovn_as az1 check ovn-nbctl set logical_router_port lrp-lr11-ts1 > options:ic-route-filter-learn=2001:db12::/64 > +# Test direct routes from lr12 were learned to lr11. > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 2001 | > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > +2001:db12::/64 fe80:10::2 > +]) > + > +OVN_CLEANUP_IC([az1], [az2]) > + > +AT_CLEANUP > +]) Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev