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. >> >> >> 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
