Hi Lucas, Paulo, Numan, On 3/13/25 7:39 PM, Lucas Vargas Dias (Dev - Cloud IaaS Network R&D) via dev wrote: > Em qua., 5 de mar. de 2025 às 12:04, Numan Siddique <[email protected]> > escreveu: > >> On Fri, Feb 14, 2025 at 7:56 AM Paulo Guilherme da Silva via dev >> <[email protected]> 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 <[email protected]> >>> Signed-off-by: Lucas Vargas Dias <[email protected]> >>> Signed-off-by: Paulo Guilherme da Silva <[email protected]> >>> --- >>> ic/ovn-ic.c | 119 +++++++++++++++++++++++++++++++++++++++++----- >>> ovn-nb.xml | 44 +++++++++++++++++ >>> tests/ovn-ic.at | 122 ++++++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 274 insertions(+), 11 deletions(-) >>> >>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c >>> index ea68b1c12..f0df8bf8d 100644 >>> --- a/ic/ovn-ic.c >>> +++ b/ic/ovn-ic.c >>> @@ -1069,6 +1069,86 @@ 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); >> >> nit: Fix the indentation of "filter_direction" param to match with >> the first param. >> >> > Ok, we will fix it. > > >> >>> + 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); >> >> nit: Same as above. >> >>> + if (lr_route_filter) { >>> + ds_put_format(&filter_list, "%s,", lr_route_filter); >>> + } >>> + >>> + if (!filter_list.length) { >>> + ds_destroy(&filter_list); >>> + return true; >>> + } >>> + >>> + struct in6_addr bl_prefix; >>> + unsigned int bl_plen; >>> + char *cur, *next, *start; >>> + next = start = xstrdup(ds_cstr(&filter_list)); >>> + bool matched = false; >> >> nit: Reverse christmas tree >> >> Can you reorder like below ? >> >> > Yes, we can. > > >> ------------------ >> struct in6_addr bl_prefix; >> char *cur, *next, *start; >> unsigned int bl_plen; >> bool matched = false; >> >> next = start = xstrdup(ds_cstr(&filter_list)); >> ... >> --------------------- >> >>> + 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 logical router/logical >> router" >>> + "port options: %s: %s. CIDR expected.", >>> + filter_direction, cur); >>> + matched = true; >> >> Why are we setting matched = true here and returning ? >> Does it mean all the cidrs in the option will be ignored ? >> Why not just ignore this particular cidr ? >> >> > > > Because If the user configures just one value and it is an invalid CIDR, > none prefix will be advertised or learned. > Yes, in this patch the other cidrs will be ignored. This point I think we > can discuss. > What would be better considering if the user just configures an invalid > value in CIDR ? >
In my opinion, it's fine either way. However, if we go for the option in which all CIDRs are ignored if one is invalid then we should at least make it explicit to the user that we're ignoring everything (that is change the WARN above to reflect that). I'm moving this patch to "Changes Requested" in patchwork as it needs a v4 to address Numan's review comments. https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Regards, Dumitru > > >>> + break; >>> + } >>> + >>> + 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; >>> + } >>> + >>> + 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 mask = ipv6_create_mask(plen); >>> + /* First calculate the difference between bl_prefix and >> prefix, so >>> + * use the bl mask to ensure prefixes are correctly >> validated. >>> + * e.g.: 2005:1734:5678::/50 is a subnet of 2005:1234::/21 >> */ >>> + struct in6_addr m_prefixes = ipv6_addr_bitand(prefix, >> &bl_prefix); >>> + struct in6_addr m_prefix = ipv6_addr_bitand(&m_prefixes, >> &mask); >>> + struct in6_addr m_bl_prefix = ipv6_addr_bitand(&bl_prefix, >> &mask); >>> + if (!ipv6_addr_equals(&m_prefix, &m_bl_prefix)) { >>> + continue; >>> + } >>> + } >>> + matched = true; >>> + break; >>> + } >>> + free(start); >>> + ds_destroy(&filter_list); >>> + return matched; >>> +} >>> + >> >> I'd avoid duplicating the code if possible since the same code is used >> in prefix_is_deny_listed(). >> Is it possible to refactor ? I see there is one difference in >> handling the wrong cidr format. >> >> Let me know if it's hard to refactor. In which case we can live with it. >> >> Thanks >> Numan >> >> > > I think it's possible to refactor. > > >>> static bool >>> prefix_is_deny_listed(const struct smap *nb_options, >>> struct in6_addr *prefix, >>> @@ -1134,7 +1214,9 @@ 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; >>> @@ -1156,6 +1238,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; >>> } >>> >>> @@ -1210,7 +1297,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; >>> @@ -1219,7 +1307,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; >>> } >>> >>> @@ -1260,7 +1349,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; >>> @@ -1268,7 +1358,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; >>> @@ -1322,7 +1413,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; >>> @@ -1341,6 +1433,10 @@ 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); >>> @@ -1470,7 +1566,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; >>> } >>> >>> @@ -1656,7 +1752,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; >>> >>> @@ -1679,7 +1776,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); >>> } >>> } >>> >>> @@ -1691,7 +1788,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. */ >>> @@ -1755,7 +1852,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/ovn-nb.xml b/ovn-nb.xml >>> index 20d30dd58..550495cea 100644 >>> --- a/ovn-nb.xml >>> +++ b/ovn-nb.xml >>> @@ -3024,6 +3024,28 @@ or >>> derived from OvS default datapath limit. >>> </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. 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. 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> >>> + >>> <column name="options" key="dynamic-routing" type='{"type": >> "boolean"}'> >>> If set to <code>true</code> then this <ref >> table="Logical_Router"/> >>> can participate in dynamic routing with components outside of >> OVN. >>> @@ -3884,6 +3906,28 @@ or >>> </p> >>> </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> >>> + >>> <column name="options" key="requested-chassis"> >>> <p> >>> If set, identifies a specific chassis (by name or hostname) >> that >>> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at >>> index 02191f13a..0fc955ce5 100644 >>> --- a/tests/ovn-ic.at >>> +++ b/tests/ovn-ic.at >>> @@ -2651,6 +2651,128 @@ OVN_CLEANUP_IC([az1], [az2]) >>> 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 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 >>> + >>> +# 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" >>> + >>> +# 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" >>> + >>> +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 all routes >>> +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 >>> +192.168.2.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 >>> +]) >>> + >>> OVN_FOR_EACH_NORTHD([ >>> AT_SETUP([spine-leaf: 2 AZs, 2 HVs, 2 LSs, connected via transit spine >> switch]) >>> AT_KEYWORDS([spine leaf]) >>> -- >>> 2.34.1 >>> >>> >>> -- >>> >>> >>> >>> >>> _'Esta mensagem é direcionada apenas para os endereços constantes no >>> cabeçalho inicial. Se você não está listado nos endereços constantes no >>> cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa >>> mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas >> estão >>> imediatamente anuladas e proibidas'._ >>> >>> >>> * **'Apesar do Magazine Luiza tomar >>> todas as precauções razoáveis para assegurar que nenhum vírus esteja >>> presente nesse e-mail, a empresa não poderá aceitar a responsabilidade >> por >>> quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.* >>> >>> >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
