On Tue, Jun 28, 2022 at 12:37:08PM +0300, Eugene Bogomazov wrote: > Hello everyone, > > The current changes were sufficient to add the described AFI/SAFI check. > Role/OTC rules now only apply to the unicast session. I've checked this on > a stand with a mixture of multicast and unicast sessions and it seems to > work well in all cases. > > As a result, all the necessary basic functionality for roles has been > achieved. > > I decided to collect all my patches into one to make patching easier and > I'll attach it to this post. This can be applied to the latest master > version.
Hello Finally did some thorough review and prepared it for merging. Found some bugs: bgp_update_attrs(): bgp_set_attr_u32(&attrs, pool, BA_ONLY_TO_CUSTOMER, 0, p->local_as); Here should be p->public_as, to handle case of AS confederation boundary, where local_as is internal member-AS, while public_as is a confederation ID. bgp_read_capabilities(): caps->role = BGP_ROLE_UNDEFINED; This should be set when caps is allocated (caps = mb_allocz()), as it is possible to call bgp_read_capabilities() multiple times (if capabilities are split to multiple OPEN option). There should also be a check for received capability value equal to BGP_ROLE_UNDEFINED, in which case it should fail. bgp_format_role_name(): if (role <= 5) Here should be (role < 5)? Also did some minor changes: - renamed 'strict mode' to 'require roles' - removed conn->neighbor_role, just used caps->role - simplified error handling in bgp_read_capabilities() and bgp_finish_attrs() - improve error messages in case of route leaks The final patch is here: https://gitlab.nic.cz/labs/bird/-/commit/c73b5d2d3d94204d2a81d93efd02c4c115859353 Are you OK with these changes? If so, i will merge it to the master branch. I am also working on some test setup for our CI. There are still two remaining issues related to BGP roles (which could be targetted in following patches): 1) As confederation handling It does not really work properly. RFC says: Also, on egress from the AS Confederation, an UPDATE MUST NOT contain an OTC Attribute with a value corresponding to any Member-AS Number other than the AS Confederation Identifier. That is not done and cannot be done based on local information. That is why AS_PATH has separate AS_SEQUENCE and AS_CONFED_SEQUENCE, so internal ASes can be stripped on confederation boundary without knowledge of all internal ASes. This is something that is underestimated in the RFC 9234. We could either just disallow roles on intra-confederation EBGP links, or just put big fat warning when configured. 2) Filter support for bgp_otc attribute I think there should be filter support, although it could be controversial considering RFC says: Once the OTC Attribute has been set, it MUST be preserved unchanged (this also applies to an AS Confederation). The operator MUST NOT have the ability to modify the procedures defined in this section. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
