Hi Rosemarie, Thanks for your review. Em ter., 21 de jul. de 2026 às 17:22, Rosemarie O'Riorden < [email protected]> escreveu:
> 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. > > Do you think ic-route-allow-tag would be better? > 2. You could combine your other patch converting the ic-route-filter-tag > to accept lists with this one into one patch set. > > > I agree > 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. > > We already have something like that in ovn-ic (ic-route-deny-adv and ic-route-filter-adv). 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. > > I agree. It doesn't make sense at the same time, but I think scenarios where it could be more easy to configure with allow and others with block. Let me explain my scenario: I have 5 VPCs, lets say: vpc1, vpc2, vpc3, vpc4, vpc5 vpc1 must connect with all. (No options configure) vpc2 must connect with vpc1,vpc3. (allow vpc1,vpc3) vpc3 must connect with vpc1,vpc2. (allow vpc1,vpc2) vpc4 must connect with vpc1,vpc5. (allow vpc1,vpc5) vpc5 must connect with vpc1,vpc4. (allow vpc1,vpc4) I know that I can block all except some VPCs , however for each new VPC connected in TS, I must update all existing configurations to deny the new VPC. > 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. > I add in NEWS, "ic-route-filter-tag" still takes precedence over this allowlist. I'll adjust the test and code to be easier to understand. > What do you think? > > I think you have good points. I tried to explain my points on each topic. > > --- > > 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. > > I agree. > > > > 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. > I agree. > > > 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 ? > I agree. > > > + 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. > > I agree Regards, Lucas > > + > > OVN_CLEANUP_IC([az1], [az2]) > > > > AT_CLEANUP > > -- > Rosemarie O'Riorden > Lowell, MA, United States > [email protected] > > -- _‘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
