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.
> + 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 ?
------------------
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 ?
> + 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
> 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
