On 8/11/26 9:11 AM, Dumitru Ceara wrote:
> On 8/11/26 2:47 PM, Lucas Vargas Dias wrote:
>> Hi Dumitru, Rosemarie,
>>
>> I would like to know your opinions before I submit a new version of this
>> patch.
>>
>
> Hi Lucas, Rosemarie,
>
> Please see below.
>
>> Regards,
>> Lucas
>>
>>
>> Em seg., 3 de ago. de 2026 às 09:46, Lucas Vargas Dias
>> <[email protected]> escreveu:
>>
>>> Hi Rosemarie, Dumitru,
>>>
>>> Thanks for your reviews.
>>>
>>>
>>>
>>>
>>> Em qua., 22 de jul. de 2026 às 05:06, Dumitru Ceara <[email protected]>
>>> escreveu:
>>>
>>>> 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.
>>>>
>>>
>>> I agree.
>>>
>>>
>>>
>>>
>>>>
>>>>>> ---
>>>>>> 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.
>>>>>
>>>>
>>> I agree that "ic-route-filter-tag" implies one tag.But, I think it could
>>> confuse
>>> the user with two configurations to do the same thing.
>>> Also, in the future, "ic-route-filter-tag" could be deprecated and just
>>> the new option
>>> will be used.
>>>
>>> So, I would like to keep the same config name. I understand that's an
>>> improvement in the configuration
>>> and avoids future work.
>>>
>>>
>>> Regards,
>>> Lucas
>>>
>>>> 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
>
> As mentioned earlier, for maintenance ease and also because we have
> other similarly working configuration options (e.g.,
> Forwarding_Group.child_port, Network_Function_Group.network_function,
> Logical_Switch.load_balancer) I'd reuse the current ic-route-filter-tag key.
>
> Rosemarie, would that be acceptable for you too?
>
> Regards,
> Dumitru
Hello Lucas and Dumitru,
Yes that sounds fine to me too. I will keep an eye out for the next
version.
Take care!
>
>>>> "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
>>>>
>>>>
>>
>
--
Rosemarie O'Riorden
Lowell, MA, United States
[email protected]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
