On 7/21/26 8:56 AM, Lucas Vargas Dias wrote:
> The "ic-route-filter-tag" option on a Logical_Router_Port acts as a
> blocklist: routes whose "ic-route-tag" matches the configured tag are
> not learned. There was no way to express the opposite - "learn only
> routes carrying one of these tags" - which is useful when a router port
> should import routes from a known set of VPCs and drop everything else.
>
> Add an "ic-route-filter-allow-tag" option that takes a comma-separated
> list of route tags. When set, only IC routes whose "ic-route-tag"
> matches one of the listed tags are learned; every other route,
> including untagged ones, is filtered out. The existing
> "ic-route-filter-tag" blocklist still takes precedence, so a route
> whose tag is filtered is skipped even if the same tag is allowlisted.
>
> Assisted-by: Claude Opus 4.8, Claude Code
> Signed-off-by: Lucas Vargas Dias <[email protected]>
Hi Lucas, thanks for the patch.
This seems like a useful feature overall! I just have a few concerns:
1. The naming isn't ideal IMHO. "Filter" implies a block-list already,
because with how this option already works, we are filtering tags out
(rather than filtering FOR certain tags), so adding a "filter-allow"
option is kind of paradoxical.
2. You could combine your other patch converting the ic-route-filter-tag
to accept lists with this one into one patch set.
3. I think it's too complex to have two dependent lists configured in
separate options. It just seems like this could lead to confusion for
users, and adding another option unnecessarily.
The thing is allow and block do not make sense be used at the same time,
because allowing any tag(s) blocks all other tags, and blocking any
tag(s) allows all other tags. So this means that only one list should be
in use at any given time, meaning that maybe it makes sense to just have
one, and also all options in said list can be interpreted as either
allow or block.
One idea is to do this with a list like "*vpc1,*vpc2,*vpc3" to allow all
of those tags, and then "vpc1,vpc2,vpc3" to block all of them.
Similarly, "*vpc1,vpc2,vpc3" could mean the whole list is allow, since
it's only used for just block or allow at one time.
Or to be more explicit, "allow:vpc1,vpc2,vpc3" or "block:vpc1,vpc2,vpc3"
4. The "precedence" that the filter-tag takes is kind of confusing,
because what happens for non-specified tags if you have both the
filter-tag and allow-tag configured at once?
For example: vpc1 allowed, vpc2 blocked, what happens to vpc3? I guess
it's blocked because of the blocklist's precedence only applying to
specified tags, but this isn't easy to understand.
What do you think?
> ---
> NEWS | 6 ++++++
> ic/ovn-ic.c | 24 ++++++++++++++++++++++++
> ovn-nb.xml | 24 ++++++++++++++++++++++++
> tests/ovn-ic.at | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 99 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index c7cdcc5a4..09357f3ab 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -82,6 +82,12 @@ Post v26.03.0
> as an override, to Logical_Router_Port) to advertise a router's IPv4
> routes through ovn-ic using the interconnect port's IPv6 address as next
> hop ("IPv4 over IPv6").
> + - Added "ic-route-filter-allow-tag" option to the Logical_Router_Port
> + table, an allowlist counterpart to "ic-route-filter-tag": it accepts a
> + comma-separated list of route tags and only IC routes whose
> + "ic-route-tag" matches one of them are learned. Untagged routes and
> + routes tagged otherwise are not learned; "ic-route-filter-tag" still
> + takes precedence over this allowlist.
This seems too long. The NEWS entry should be a super concise overview
of the new feature, not configuration details. Those should be in the
documentation so the user can find them there.
>
> OVN v26.03.0 - xxx xx xxxx
> --------------------------
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index 26173a5f6..a6af5d73e 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -2556,6 +2556,7 @@ sync_learned_routes(struct ic_context *ctx,
> ovs_assert(nb_global);
>
> const char *lrp_name, *ts_route_table, *route_filter_tag;
> + const char *route_filter_allow_tags;
Nit: try to keep variables in reverse christmas tree order; also putting
the filter variables together might be good.
> const struct icsbrec_port_binding *isb_pb;
> const struct nbrec_logical_router_port *lrp;
> VECTOR_FOR_EACH (&ic_lr->isb_pbs, isb_pb) {
> @@ -2568,11 +2569,21 @@ sync_learned_routes(struct ic_context *ctx,
> ts_route_table = smap_get_def(&lrp->options, "route_table", "");
> route_filter_tag = smap_get_def(&lrp->options,
> "ic-route-filter-tag", "");
> + route_filter_allow_tags = smap_get_def(&lrp->options,
> + "ic-route-filter-allow-tag", "");
> } else {
> ts_route_table = "";
> route_filter_tag = "";
> + route_filter_allow_tags = "";
> }
>
> + /* Allowlist of route tags to learn. When non-empty, only routes
> + * whose "ic-route-tag" is in this set are learned; every other
> + * route (including untagged ones) is filtered out. */
> + struct sset allow_tag_set = SSET_INITIALIZER(&allow_tag_set);
> + sset_from_delimited_string(&allow_tag_set, route_filter_allow_tags,
> + ",");
> +
> isb_route_key =
> icsbrec_route_index_init_row(ctx->icsbrec_route_by_ts);
> icsbrec_route_index_set_transit_switch(isb_route_key,
> isb_pb->transit_switch);
> @@ -2596,6 +2607,7 @@ sync_learned_routes(struct ic_context *ctx,
>
> const char *isb_route_tag = smap_get(&isb_route->external_ids,
> "ic-route-tag");
> + /* Blocklist takes precedence over the allowlist below. */
> if (isb_route_tag && !strcmp(isb_route_tag, route_filter_tag)) {
> VLOG_DBG("Skip learning route %s -> %s as its route tag "
> "[%s] is filtered by the filter tag [%s] of TS LRP
> ",
> @@ -2604,6 +2616,17 @@ sync_learned_routes(struct ic_context *ctx,
> continue;
> }
>
> + if (!sset_is_empty(&allow_tag_set) &&
> + (!isb_route_tag ||
> + !sset_contains(&allow_tag_set, isb_route_tag))) {
This is logically sound but kind of hard to read, maybe split it into a
nested-if and add a comment explaining this ?
> + VLOG_DBG("Skip learning route %s -> %s as its route tag "
> + "[%s] is not in the allow tag list [%s] of TS LRP ",
> + isb_route->ip_prefix, isb_route->nexthop,
> + isb_route_tag ? isb_route_tag : "(none)",
> + route_filter_allow_tags);
> + continue;
> + }
> +
> if (isb_route->route_table[0] &&
> strcmp(isb_route->route_table, ts_route_table)) {
> if (VLOG_IS_DBG_ENABLED()) {
> @@ -2668,6 +2691,7 @@ sync_learned_routes(struct ic_context *ctx,
> }
> }
> icsbrec_route_index_destroy_row(isb_route_key);
> + sset_destroy(&allow_tag_set);
> }
>
> /* Delete extra learned routes. */
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 1901bbf26..42f887798 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -4645,6 +4645,30 @@ or
> </p>
> </column>
>
> + <column name="options" key="ic-route-filter-allow-tag"
> + type='{"type": "string"}'>
> + <p>
> + This option expects a comma-separated list of allowed route-tags,
> + for example <code>vpc1,vpc2</code>. When set on the Logical Router
> + Port, it acts as an allowlist for the route learning process: only
> + routes whose <code>ic-route-tag</code> (present in the
> + <code>external_ids</code> register of the advertised route entry in
> + the <ref table="Route" db="OVN_IC_Southbound"/> table of the
> + <ref db="OVN_IC_Southbound"/> database) matches one of the listed
> + tags are learned by the <code>ovn-ic</code> daemon. Every other
> + route, including routes that carry no <code>ic-route-tag</code> at
> + all, is filtered and not learned.
> + </p>
> +
> + <p>
> + When both this option and <ref column="options"
> + key="ic-route-filter-tag"/> are set, the blocklist behaviour of
> + <ref column="options" key="ic-route-filter-tag"/> takes precedence:
> + a route whose tag matches the filter tag is skipped even if the
> + same tag is present in this allowlist.
> + </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 d884de282..57e4a9f7f 100644
> --- a/tests/ovn-ic.at
> +++ b/tests/ovn-ic.at
> @@ -3683,6 +3683,51 @@ OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl
> lr-route-list lr11 | grep 192.168 |
> 192.168.1.0/24 169.254.103.22
> ])
>
> +# Allowlist: only learn tspeer routes tagged vpc1 on lrp-lr11-tspeer.
> +# The tspeer route from lr22 (169.254.103.22) carries no tag and is thus
> +# filtered by the strict allowlist. Routes learned through ts11/ts12 are
> +# not affected as the option is set on the tspeer LRP only.
> +ovn_as az1 ovn-nbctl set logical_router_port lrp-lr11-tspeer
> options:ic-route-filter-allow-tag=vpc1
> +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.0.0/24 169.254.101.2
> +192.168.0.0/24 169.254.102.2
> +192.168.0.0/24 169.254.103.12
> +])
> +
> +# Tag lr22's advertised route with vpc2 and allow a list of tags [vpc1,vpc2].
> +# The previously untagged route is now tagged vpc2 and learned again.
> +ovn_as az2 ovn-nbctl set logical_router_port lrp-lr22-tspeer
> options:ic-route-tag=vpc2
> +ovn_as az1 ovn-nbctl set logical_router_port lrp-lr11-tspeer
> options:ic-route-filter-allow-tag=vpc1,vpc2
> +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.0.0/24 169.254.101.2
> +192.168.0.0/24 169.254.102.2
> +192.168.0.0/24 169.254.103.12
> +192.168.1.0/24 169.254.103.22
> +])
> +
> +# Blocklist takes precedence over the allowlist: filtering vpc1 drops the
> +# vpc1 tspeer route (169.254.103.12) even though vpc1 is in the allow list.
> +ovn_as az1 ovn-nbctl set logical_router_port lrp-lr11-tspeer
> options:ic-route-filter-tag=vpc1
> +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.0.0/24 169.254.101.2
> +192.168.0.0/24 169.254.102.2
> +192.168.1.0/24 169.254.103.22
> +])
> +
> +# Remove both tag options: all routes are learned again.
> +ovn_as az1 ovn-nbctl remove logical_router_port lrp-lr11-tspeer options
> ic-route-filter-allow-tag=vpc1,vpc2
> +ovn_as az1 ovn-nbctl remove logical_router_port lrp-lr11-tspeer options
> ic-route-filter-tag=vpc1
> +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.0.0/24 169.254.101.2
> +192.168.0.0/24 169.254.102.2
> +192.168.0.0/24 169.254.103.12
> +192.168.1.0/24 169.254.103.22
> +])
There needs to be a test that verifies the blockage of a non-specified
tag, like I mentioned in point 4 at the top.
> +
> OVN_CLEANUP_IC([az1], [az2])
>
> AT_CLEANUP
--
Rosemarie O'Riorden
Lowell, MA, United States
[email protected]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
