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? 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). 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 >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
