Hi Dumitru, Rosemarie, I would like to know your opinions before I submit a new version of this patch.
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 >> "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 >> >> -- _‘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
