Hi Lucas, On 8/14/26 2:52 PM, Lucas Vargas Dias wrote: > > Do you think ovn-ic could automatically replace ic-router-filter-tag by > ic-route-learn-tag-rules=block? >
I'm not sure that will be easy to do and might impact upgrades. Wouldn't it mean that all ovn-ic's should be upgraded at the same time? Regards, Dumitru > > Regards, > Lucas > > Em sex., 14 de ago. de 2026 às 09:31, Lucas Vargas Dias > <[email protected]> escreveu: > > > > Em sex., 14 de ago. de 2026 às 09:24, Dumitru Ceara > <[email protected] <mailto:[email protected]>> escreveu: > > On 8/14/26 1:57 PM, Lucas Vargas Dias wrote: > > Hi Dumitru and Rosemaria, > > Thanks for your review. > > > > Em sex., 14 de ago. de 2026 às 07:22, Dumitru Ceara > <[email protected] <mailto:[email protected]>> > > escreveu: > > > >> On 8/14/26 12:09 AM, Rosemarie O'Riorden wrote: > >>> Hi Lucas, > >>> > >> > >> Hi Lucas, Rosemarie, > >> > >> Thanks for the new version and for the reviews! > >> > >>> Sorry I didn't respond to you explaining your use-case for > having both > >>> the block and allowlist at the same time. I guess I've > already shared my > >>> thoughts on that so it would be nice if someone else could > chime in and > >>> share what they think. I'm fine with it if others are okay > with it. And > >>> that's fair that there's already similar behavior in ovn-ic. > However as > >>> a general practice I would say we shouldn't mirror less- > than-ideal > >>> behavior just because it's already in the codebase :) But as > I said I > >>> would appreciate others' opinions! > >>> > >>> I hope you can see Lucas' response to my review. It's not > showing up for > >>> me on patchwork but it's this thread: > >>> [ovs-dev] [PATCH ovn v1] ic: Add allowlist filter for > learned IC route > >> tags. > >>> > >> > >> It should be this one: > >> > >> https://patchwork.ozlabs.org/project/ovn/ > patch/[email protected]/#3734125 > <https://patchwork.ozlabs.org/project/ovn/ > patch/[email protected]/#3734125> > >> > >> Copied inline here: > >> > >>>> 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" > >> > >> Lucas, with Rosemarie's explicit single list alternative > above you could > >> do: > >> > >> vpc1: no ic-route-filter-tag > >> vpc2: ic-route-filter-tag="allow:vpc1,vpc3" > >> vpc3: ic-route-filter-tag="allow:vpc1,vpc2" > >> vpc4: ic-route-filter-tag="allow:vpc1,vpc5" > >> vpc5: ic-route-filter-tag="allow:vpc1,vpc4" > >> > >> Now if you add a vpc6 you don't need to change the > configuration of the > >> existing vpc2-6, they won't be importing routes from vpc6 > because it's > >> not listed in their allow list. > >> > >> So if we would be implementing this from scratch I think I'd > go for this > >> explicit option that Rosemarie suggested. > >> > >> There's a catch though: > >> > >> The current ic-route-filter-tag used to be a single tag, now > patch 1/2 > >> changes it to accept a list which means that if users used to > have a > >> configuration with a single tag that happens to include a comma > >> character we now break their configuration. That is backwards > >> incompatible but I guess that might be considered acceptable. > >> > >> Now, if we go for this single allow/block list option we add > even more > >> restrictions, i.e., tags cannot contain semicolons either (or > at least > >> they can't start with allow:/block:). > >> > >> With all that in mind maybe it's good to take a step back and > consider > >> Rosemarie's original suggestion of adding a new configuration > key (I > >> know I'm contradicting my reply on the v1 of patch 1/2 but I > didn't > >> realize all of the above until now). > >> > >> So what if we: > >> - keep the existing behavior > >> - add a new key 'ic-route-learn-tag-rules' which only > supports the > >> following types of values: > >> > >> EITHER > >> a. "allow:<comma-separated-tags>" > >> > >> OR > >> b. "block:<comma-separated-tags>" > >> > >> with the restriction that tags cannot contain commas. > >> > >> > > I agree about this. So, you can ignore the first patch where I > add the > > support to multiple tags in ic-route-filter-tag. > > I'll send a new version with the config 'ic-route-learn-tag- > rules' with > > possibility of allow, block. > > Cool, thank you! I'd also like to hear inputs from Rosemarie but I > guess we can move the discussion on the new version when you > post it for > review. > > > ic-route-filter-tag will take precedence over this config. > > > > Why? Why not give precedence to the new config you're adding, > instead > of preferring the one we're essentially deprecating? > > > OK, I'll take the precedence to the new config. > > > On that front, I guess your new patch version should probably > also add a > deprecation note for the old ic-route-filter-tag (see the "OVN > Deprecation plan" section in TODO.rst). > > > I'll add a deprecation note. > > Regards, > Lucas > > > > Regards, > Dumitru > > > Regards, > > Lucas > > > >>> > >>>> > >>>> 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. > >>>> > >> > >> > >> 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
