Em sex., 14 de ago. de 2026 às 09:24, Dumitru Ceara <[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]> > > 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 > >> > >> 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
