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. ic-route-filter-tag will take precedence over this config. 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
