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 >>> "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
