FRRouting / frr

The FRRouting Protocol Suite
https://frrouting.org/
Other
3.33k stars 1.25k forks source link

`extcommunity-list expanded x delete` matches against all extended community values making it impossible to delete only a single extcommunity by regex #17081

Open rudis opened 1 week ago

rudis commented 1 week ago

Description

I'm trying to delete one type of extended community with any value (e.g. all "RT:x:y", no matter x and y) but not extended communities with other types.

The following config removes all extended community entries, even non RT types.

bgp extcommunity-list expanded exp seq 5 permit RT:
route-map test permit 10
 set extended-comm-list exp delete
exit

Removing a single RT value works (only RT:65002:9001 is removed):

bgp extcommunity-list standard std seq 10 permit rt 65002:9001
route-map test permit 10
 set extended-comm-list std delete
exit

The problem seems to be that ecommunity_list_match_delete passes ecom to ecommunity_regexp_match and not local_ecom. So the full string of the ecommunity is compared in each iteration. Fixing this should be as simple as passing local_ecom, but I didn't test it.

From a quick look it the same issue affects community_list_match_delete and lcommunity_list_match_delete as well. Or is this expected behavior?

Version

10.1.1, also present in current git

How to reproduce

See above

Expected behavior

extcommunity-list expanded matches against a single ecommunity value.

Actual behavior

extcommunity-list expanded matches against the whole ecommunity value.

Additional context

No response

Checklist

ton31337 commented 1 week ago

Well, "RT:" is too simple to be evaluated strictly, so it matches everything that has RT:*. (expected). The idea of how it works is, that we convert all the extended communities into a string (e.g. RT:1.1.1.1:1 RT 2.2.2.2:2), then apply RT: which matches a string.

If you specify something like bgp extcommunity-list expanded r1-rt seq 5 permit RT:1.1.1.1:1$ it won't match because the end ($) is with 2.2.2.2:2.

rudis commented 6 days ago

The problem is that it's matched against the whole string which includes attributes besides RT.

Consider the following community without any filtering:

 Extended Community: RT:65002:9001 ET:8 Rmac:52:6f:d6:bd:f2:fa

When I apply bgp extcommunity-list expanded test1 permit RT:65002:9001$ I get the community unchanged:

 Extended Community: RT:65002:9001 ET:8 Rmac:52:6f:d6:bd:f2:fa

When I apply bgp extcommunity-list expanded test2 permit Rmac:52:6f:d6:bd:f2:fa$ the community is completely removed.

Both of these behaviors work as you describe (and as currently implemented in the code) but it doesn't permit the kind of control I'd like to have.

I couldn't find a way to strip only RT:65002:9001 but leave ET and Rmac unchanged.

Or using your example, how would I remove just RT:1.1.1.1:1 but keep RT:2.2.2.2:2?

ton31337 commented 6 days ago

I couldn't find a way to strip only RT:65002:9001 but leave ET and Rmac unchanged.

With regexp (extended), that's not possible right now.

rudis commented 6 days ago

Ok. I guess changing the behavior to match individual values (as mentioned in my original report) is not possible because it would break existing configurations.

Besides the backwards compatibility, are there cases when one does not want to match against the individual values (my suggestion) and use the whole string (as it's currently implemented)?

ton31337 commented 6 days ago

Besides the backwards compatibility, are there cases when one does not want to match against the individual values (my suggestion) and use the whole string (as it's currently implemented)?

I'm not aware of this mixed use cases.

rudis commented 6 days ago

My use case was to remove internal EVPN extended communities to a CE but to keep the possibility to add other extended attributes later just in case. But at the moment it's only a lab setup, so feel free to close this bug as wontfix.

Thank you for your quick and competent replies!

ton31337 commented 6 days ago

You should be able to filter out arbitrary extended communities with standard (not extended), did you try it?

rudis commented 6 days ago

Yes, that works. But from my understanding I have to match explicit values (e.g. Rmac:52:6f:d6:bd:f2:fa) so it's easy to forgot adding new ones. A regex would make that safer.