On 7/21/26 10:38 PM, Rosemarie O'Riorden wrote:
> On 7/17/26 7:52 PM, Lucas Vargas Dias wrote:
>> The 'ic-route-filter-tag' option on a Logical_Router_Port used to accept
>> only a single route-tag: the learned route's tag was matched against the
>> option value with a plain strcmp(), so a comma-separated value would
>> never match any real tag and the filter would silently do nothing.
>>
>> Parse the option as a comma-separated list instead, building an sset of
>> tags and matching each learned route's tag against it, mirroring the
>> behavior already used by the 'ic-route-filter-adv' and
>> 'ic-route-filter-learn' prefix filters. A single tag keeps working
>> exactly as before.
>>
>> The documentation is updated to describe the list form and the test in
>> tests/ovn-ic.at is extended to verify that a route is filtered when its
>> tag is one of several listed tags, and learned again when it is not.
>>
>> Signed-off-by: Lucas Vargas Dias <[email protected]>
>
> Hi Lucas!
>
Hi Lucas, Rosemarie,
Thanks a lot for the patch and for the review!
> I think this should be part of a patch set along with your patch
> "ic: Add allowlist filter for learned IC route tags."
>
Yes, grouping related patches together in sets makes reviewer's lives
easier.
>> ---
>> ic/ovn-ic.c | 9 +++++++--
>> ovn-nb.xml | 7 ++++---
>> tests/ovn-ic.at | 23 +++++++++++++++++++++++
>> 3 files changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>> index f07e74866..ae77990e7 100644
>> --- a/ic/ovn-ic.c
>> +++ b/ic/ovn-ic.c
>> @@ -2370,6 +2370,10 @@ sync_learned_routes(struct ic_context *ctx,
>> route_filter_tag = "";
>> }
>>
>> + /* The filter tag option accepts a comma-separated list of tags. */
>> + struct sset filter_tags = SSET_INITIALIZER(&filter_tags);
>> + sset_from_delimited_string(&filter_tags, route_filter_tag, ",");
>
> The name of the variable route_filter_tag should probably be updated now
> that it's a delimited string intended to hold a list. "route_tag_filter"
> actually works fine instead.
>
>> +
>> 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);
>> @@ -2393,9 +2397,9 @@ sync_learned_routes(struct ic_context *ctx,
>>
>> const char *isb_route_tag = smap_get(&isb_route->external_ids,
>> "ic-route-tag");
>> - if (isb_route_tag && !strcmp(isb_route_tag, route_filter_tag))
>> {
>> + if (isb_route_tag && sset_contains(&filter_tags,
>> isb_route_tag)) {
>> VLOG_DBG("Skip learning route %s -> %s as its route tag "
>> - "[%s] is filtered by the filter tag [%s] of TS LRP
>> ",
>> + "[%s] is filtered by the filter tags [%s] of TS
>> LRP ",
>> isb_route->ip_prefix, isb_route->nexthop,
>> isb_route_tag, route_filter_tag);
>> continue;
>> @@ -2465,6 +2469,7 @@ sync_learned_routes(struct ic_context *ctx,
>> }
>> }
>> icsbrec_route_index_destroy_row(isb_route_key);
>> + sset_destroy(&filter_tags);
>> }
>>
>> /* Delete extra learned routes. */
>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>> index 33a6dc676..a13c2083c 100644
>> --- a/ovn-nb.xml
>> +++ b/ovn-nb.xml
>> @@ -4573,9 +4573,10 @@ or
>> <column name="options" key="ic-route-filter-tag"
>> type='{"type": "string"}'>
>> <p>
>> - This option expects a name of a filtered route-tag that's present
>> - in the Logical Router Port. If set, it causes any route learned by
>> - the Logical Router Port with the <code>route-tag</code> present in
>> + This option expects a comma-separated list of filtered route-tags
>> + that's present in the Logical Router Port. If set, it causes any
>> + route learned by the Logical Router Port with a
>> + <code>route-tag</code> matching one of the listed tags, present in
>
> It might be a good idea to change the name of this key in the database.
> As I mentioned earlier, "ic-route-filter-tag" implies one tag, and maybe
> "ic-route-tag-filter" would be better.
>
> However this would cause backwards-compatibility issues and both would
> need to be kept for some time, so it's difficult to say which option is
> better. What do you think? Maybe @Dumitru has some input.
>
We've done that in the past, supporting two versions of config keys for
a while for backwards compatibility.
In this case though, I'd be OK with keeping the "ic-route-filter-tag"
key. I understand Rosemarie's point of it being slightly
(grammatically) incorrect but we have a bunch of other places in our
config where we do that. E.g., in the NB schema:
"Load_Balancer_Group": {
"columns": {
"name": {"type": "string"},
"load_balancer": {"type": {"key": {"type": "uuid",
"refTable": "Load_Balancer",
"refType": "weak"},
"min": 0,
"max": "unlimited"}}},
For maintenance ease, I'd just reuse the current key. I won't oppose a
new key either if you guys agree to do that but I don't agree with
"ic-route-tag-filter" being necessarily better, we have a bunch of other
"ic-route-filter-*" options already.
>> the external_ids register of the advertised route entry in the
>> <ref table="Route" db="OVN_IC_Southbound"/> table of the
>> <ref db="OVN_IC_Southbound"/> database, will be filtered and not
>> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
>> index f9fceac8d..f3bdc816f 100644
>> --- a/tests/ovn-ic.at
>> +++ b/tests/ovn-ic.at
>> @@ -3299,6 +3299,29 @@ 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
>> ])
>>
>> +# Filter using a comma-separated list of tags that includes vpc1.
>> +# The vpc1-tagged route (169.254.103.12) must be filtered out.
>> +ovn_as az1 ovn-nbctl set logical_router_port lrp-lr11-tspeer
>> options:ic-route-filter-tag=vpc0,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.1.0/24 169.254.103.22
>> +])
>> +
>> +# Change the filter to a list that does not include vpc1.
>> +# The vpc1-tagged route (169.254.103.12) must be learned again.
>> +ovn_as az1 ovn-nbctl set logical_router_port lrp-lr11-tspeer
>> options:ic-route-filter-tag=vpc0,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
>> +])
>> +
>> OVN_CLEANUP_IC([az1], [az2])
>>
>> AT_CLEANUP
>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
