On Tue, Feb 28, 2023 at 01:11:14PM +0100, Claudio Jeker wrote: > When I implemented RFC9234 support I was a bit to conservative and only > enabled the loop detection if the capability was enabled. This is > how every other capability works but RFC9234 is special. > On top of this with the addition of ASPA support the way BGP roles are > handled changed and the code turned up a bit strange. > Considering all of this and rereading RFC9234 I came up with this diff: > > - add role output to bgpctl, also adjust the capability output. > Note, this changes the JSON output of neighbors a bit. > - adjust the config parser to enable the RFC9234 role capability when > there is a role set. iBGP and sessions with no role will not announce > the role capability. > - adjust the role capability announcement to be only on sessions that > use either AFI IPv4 or IPv6 and SAFI 1 (AID_INET, AID_INET6). If a > session has neither of the two then do not announce the role (even if > set) > - if there is an OPEN notification indicating that the role capability > is bad only disable the capability if it is not enforced. If enforced > do nothing and let the session fail (again and again). > - Adjust capability negotiation, store remote_role on the peer since > the neighbors role is no longer needed. > - inject the OTC attribute on ingress only for AID_INET and AID_INET6. > For other AIDs clear the F_ATTR_OTC_LOOP flag since OTC loop detection > must be skipped for other address families. > - Adjust the role logic in the RDE and use the peer->role (local role of > the system) for all checks. Also remove the check if the role capability > was present (removal of peer_has_open_policy()). > This change switches all role checks from raw capability codes to > enum role and at the same time flips the logic since the view of the > checks is changed from remote system to local system. > The roles change like this: rs <-> rs-client, customer <-> provider, > peer <-> peer > - In prefix_eligible() check also if the F_ATTR_OTC_LOOP flag is set. > The RFC requires that prefixes must be considered ineligible (and not > treat as withdraw) > - When generating an UPDATE include the OTC attribute unless the AID is > neither AID_INET or AID_INET6 or the roles do not require the addtion. > No longer depend on peer_has_open_policy() there. > > Please test and check that my logic is actually correct and following the > RFC.
This all looks good to me. I could not spot anything wrong with it. ok tb
