From 673920d48bfda3af3e97e483437628c4d1bb05c1 Mon Sep 17 00:00:00 2001 From: Rayhaan Jaufeerally Date: Tue, 6 Oct 2026 11:35:12 +0200 Subject: [PATCH] Names that said the same thing twice, or two things once - PathAttribute::UnknownPathAttribute and Capability::UnknownCapability are PathAttribute::Unknown and Capability::Unknown: the enum already says what kind of thing is unknown. - The capability entry types are all named XxxEntry now (ExtendedNextHopEntry, LongLivedGracefulRestartEntry), rather than two of them sharing a name with the variant that holds a list of them; and MultiLabelCompat is MultiLabel, since nothing explained the "Compat". - Two different things were both called Reflection: what a session needs to reflect routes (export.rs, still Reflection) and the identifiers a reflected route must not carry back to us (policy.rs, now ReflectorIds). - well_known_allow's peer_is_internal is ibgp, as everywhere else. Co-Authored-By: Claude Fable 5.1 --- crates/packet/src/attributes.rs | 10 +++--- crates/packet/src/message.rs | 4 +-- crates/packet/src/open.rs | 54 +++++++++++++++--------------- crates/packet/src/update_decode.rs | 6 ++-- crates/packet/tests/properties.rs | 26 +++++++------- crates/server/src/config.rs | 6 ++-- crates/server/src/export.rs | 8 ++--- crates/server/src/fsm.rs | 6 ++-- crates/server/src/marks.rs | 4 +-- crates/server/src/policy.rs | 6 ++-- crates/web/src/api.rs | 4 +-- 11 files changed, 68 insertions(+), 66 deletions(-) diff --git a/crates/packet/src/attributes.rs b/crates/packet/src/attributes.rs index 2a0198f..b971703 100644 --- a/crates/packet/src/attributes.rs +++ b/crates/packet/src/attributes.rs @@ -86,7 +86,7 @@ pub enum PathAttribute { ClusterList(Vec), /// An attribute this implementation does not interpret, kept as it /// came so that it can be passed on. - UnknownPathAttribute { + Unknown { flags: PathAttributeFlags, type_code: u8, payload: Vec, @@ -198,7 +198,7 @@ impl PathAttribute { Self::OnlyToCustomer(_) => ONLY_TO_CUSTOMER, Self::OriginatorId(_) => ORIGINATOR_ID, Self::ClusterList(_) => CLUSTER_LIST, - Self::UnknownPathAttribute { type_code, .. } => *type_code, + Self::Unknown { type_code, .. } => *type_code, } } @@ -207,7 +207,7 @@ impl PathAttribute { fn flags(&self) -> PathAttributeFlags { match self { // As they came: the Partial bit has to survive (RFC 4271 §5). - Self::UnknownPathAttribute { flags, .. } => flags.clone(), + Self::Unknown { flags, .. } => flags.clone(), known => { let (optional, transitive) = defined_flags(known.type_code()) .expect("every attribute with a variant has defined flags"); @@ -304,7 +304,7 @@ impl PathAttribute { .map(|id| Ipv4Addr::from(*id)) .collect(), ), - _ => Self::UnknownPathAttribute { + _ => Self::Unknown { flags, type_code, payload: value.to_vec(), @@ -362,7 +362,7 @@ impl PathAttribute { out.put_slice(&id.octets()); } } - Self::UnknownPathAttribute { payload, .. } => out.put_slice(payload), + Self::Unknown { payload, .. } => out.put_slice(payload), } Ok(()) } diff --git a/crates/packet/src/message.rs b/crates/packet/src/message.rs index bd7ad02..285d253 100644 --- a/crates/packet/src/message.rs +++ b/crates/packet/src/message.rs @@ -639,7 +639,7 @@ mod tests { test_path_attribute_roundtrip!( test_unknown_path_attribute, &[0x80, 0x63, 0x04, 0xde, 0xad, 0xbe, 0xef], - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { flags: PathAttributeFlags::new( /*optional=*/ true, /*transitive=*/ false, /*partial=*/ false, /*extended_length=*/ false, @@ -780,7 +780,7 @@ mod tests { ], UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![PathAttribute::UnknownPathAttribute { + path_attributes: vec![PathAttribute::Unknown { flags: PathAttributeFlags::new(true, false, false, false), type_code: 0x63, payload: vec![0xde, 0xad, 0xbe, 0xef], diff --git a/crates/packet/src/open.rs b/crates/packet/src/open.rs index 4df38a2..f9fe16b 100644 --- a/crates/packet/src/open.rs +++ b/crates/packet/src/open.rs @@ -237,7 +237,7 @@ pub enum Capability { OutboundRouteFilter(Vec), /// Extended Next Hop encoding (RFC 8950). ExtendedNextHop { - entries: Vec, + entries: Vec, }, /// Extended Message (RFC 8654). ExtendedMessage, @@ -248,7 +248,7 @@ pub enum Capability { }, /// Multiple labels (RFC 8277 §2.1): for each address family, how many /// labels the speaker can handle in one NLRI. - MultiLabelCompat { + MultiLabel { entries: Vec, }, BgpRole { @@ -280,10 +280,10 @@ pub enum Capability { /// Enhanced Route Refresh (RFC 7313). EnhancedRouteRefresh, /// Long Lived Graceful Restart (RFC 9494). - LongLivedGracefulRestart(Vec), + LongLivedGracefulRestart(Vec), /// A capability this implementation has no definition for, kept as it /// came (RFC 5492 §3: what is not understood is ignored). - UnknownCapability { + Unknown { type_code: u8, value: Vec, }, @@ -308,14 +308,14 @@ impl Capability { Self::ExtendedNextHop { .. } => EXTENDED_NEXT_HOP, Self::ExtendedMessage => EXTENDED_MESSAGE, Self::BgpSec { .. } => BGP_SEC, - Self::MultiLabelCompat { .. } => MULTI_LABEL, + Self::MultiLabel { .. } => MULTI_LABEL, Self::BgpRole { .. } => BGP_ROLE, Self::GracefulRestart { .. } => GRACEFUL_RESTART, Self::FourByteAsn { .. } => FOUR_BYTE_ASN, Self::AddPath { .. } => ADD_PATH, Self::EnhancedRouteRefresh => ENHANCED_ROUTE_REFRESH, Self::LongLivedGracefulRestart(_) => LONG_LIVED_GRACEFUL_RESTART, - Self::UnknownCapability { type_code, .. } => *type_code, + Self::Unknown { type_code, .. } => *type_code, } } @@ -366,7 +366,7 @@ impl Capability { .map_err(|_| malformed(format!("SAFI {safi} does not fit in one octet")))?; Ok(( buf, - ExtendedNextHop { + ExtendedNextHopEntry { afi: Afi(afi), safi, nh_afi: Afi(nh_afi), @@ -399,7 +399,7 @@ impl Capability { if entries.is_empty() { return Err(malformed("Multiple Labels capability with no entries")); } - (rest, Self::MultiLabelCompat { entries }) + (rest, Self::MultiLabel { entries }) } BGP_ROLE => { let (rest, role) = be_u8(value)?; @@ -466,7 +466,7 @@ impl Capability { let (buf, stale_time) = be_u24(buf)?; Ok(( buf, - LongLivedGracefulRestart { + LongLivedGracefulRestartEntry { afi, safi, flags, @@ -478,7 +478,7 @@ impl Capability { } type_code => ( &[][..], - Self::UnknownCapability { + Self::Unknown { type_code, value: value.to_vec(), }, @@ -524,7 +524,7 @@ impl Capability { out.put_u8(header.0); out.put_u16(afi.0); } - Capability::MultiLabelCompat { entries } => { + Capability::MultiLabel { entries } => { for entry in entries { put_afi_safi(AfiSafi::new(entry.afi, entry.safi), out); out.put_u8(entry.count); @@ -566,7 +566,7 @@ impl Capability { out.put_slice(&entry.stale_time.to_be_bytes()[1..]); } } - Capability::UnknownCapability { value, .. } => out.put_slice(value), + Capability::Unknown { value, .. } => out.put_slice(value), } Ok(()) } @@ -692,11 +692,11 @@ impl AddPathEntry { } } -/// `ExtendedNextHop` allows for the nexthop to be for a different address family +/// `ExtendedNextHopEntry` allows for the nexthop to be for a different address family /// than the one being used to advertise prefixes for. E.g. it allows for sending /// IPv4 prefixes with an IPv6 nexthop. #[derive(Debug, Clone, Serialize, PartialEq, Eq)] -pub struct ExtendedNextHop { +pub struct ExtendedNextHopEntry { /// The Address Family to update the nexthop format for. pub afi: Afi, /// The Subsequent Address Family to update the nexthop format for. @@ -743,7 +743,7 @@ impl From<&BgpRole> for u8 { } #[derive(Debug, Clone, Serialize, PartialEq, Eq)] -pub struct LongLivedGracefulRestart { +pub struct LongLivedGracefulRestartEntry { pub afi: Afi, pub safi: Safi, pub flags: u8, @@ -780,7 +780,7 @@ mod tests { use eyre::Result; use crate::open::{ - AddPathEntry, AddPathSendRecv, Capability, ExtendedNextHop, GracefulRestartEntry, + AddPathEntry, AddPathSendRecv, Capability, ExtendedNextHopEntry, GracefulRestartEntry, MultiLabelEntry, OpenMessage, OpenOption, OrfFamily, OrfSendRecv, OrfType, OutboundRouteFilterEntry, }; @@ -816,7 +816,7 @@ mod tests { test_capability_roundtrip!( test_route_refresh_cisco_unsupported, &[0x80, 0x00], - Capability::UnknownCapability { + Capability::Unknown { type_code: 0x80, value: vec![] } @@ -868,17 +868,17 @@ mod tests { ], Capability::ExtendedNextHop { entries: vec![ - ExtendedNextHop { + ExtendedNextHopEntry { afi: Afi::IPV4, safi: Safi::UNICAST, nh_afi: Afi::IPV6, }, - ExtendedNextHop { + ExtendedNextHopEntry { afi: Afi::IPV4, safi: Safi::MULTICAST, nh_afi: Afi::IPV6, }, - ExtendedNextHop { + ExtendedNextHopEntry { afi: Afi::IPV4, safi: Safi::MPLS_VPN, nh_afi: Afi::IPV6 @@ -906,7 +906,7 @@ mod tests { afi: Afi::IPV4, safi: Safi::UNICAST, }]), - OpenOption::Capabilities(vec![Capability::UnknownCapability { + OpenOption::Capabilities(vec![Capability::Unknown { type_code: 0x80, value: vec![], }]), @@ -924,17 +924,17 @@ mod tests { }]), OpenOption::Capabilities(vec![Capability::ExtendedNextHop { entries: vec![ - ExtendedNextHop { + ExtendedNextHopEntry { afi: Afi::IPV4, safi: Safi::UNICAST, nh_afi: Afi::IPV6, }, - ExtendedNextHop { + ExtendedNextHopEntry { afi: Afi::IPV4, safi: Safi::MULTICAST, nh_afi: Afi::IPV6, }, - ExtendedNextHop { + ExtendedNextHopEntry { afi: Afi::IPV4, safi: Safi::MPLS_VPN, nh_afi: Afi::IPV6, @@ -962,7 +962,7 @@ mod tests { assert!(rest.is_empty()); assert_eq!( cap, - Capability::MultiLabelCompat { + Capability::MultiLabel { entries: vec![ MultiLabelEntry { afi: Afi::IPV4, @@ -1136,7 +1136,7 @@ mod tests { #[test] fn long_optional_parameters_use_two_octet_lengths() { let capabilities: Vec = (0..60) - .map(|i| Capability::UnknownCapability { + .map(|i| Capability::Unknown { type_code: 200, value: vec![i; 4], }) @@ -1194,7 +1194,7 @@ mod tests { /// used to be written with the length wrapped around. #[test] fn a_capability_too_long_for_its_length_octet_is_refused() { - let entry = ExtendedNextHop { + let entry = ExtendedNextHopEntry { afi: Afi::IPV4, safi: Safi::UNICAST, nh_afi: Afi::IPV6, diff --git a/crates/packet/src/update_decode.rs b/crates/packet/src/update_decode.rs index 61d83bf..191bc99 100644 --- a/crates/packet/src/update_decode.rs +++ b/crates/packet/src/update_decode.rs @@ -421,7 +421,7 @@ impl<'a> AttributeReader<'a> { /// The attribute kept as bytes. fn opaque(raw: &RawAttribute<'_>) -> PathAttribute { - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { flags: raw.flags.clone(), type_code: raw.type_code, payload: raw.value.to_vec(), @@ -1000,7 +1000,7 @@ mod tests { assert!(decoded.errors.is_empty()); assert_eq!( decoded.update.path_attributes.last(), - Some(&PathAttribute::UnknownPathAttribute { + Some(&PathAttribute::Unknown { flags: PathAttributeFlags(OPTIONAL_TRANSITIVE), type_code: AS4_AGGREGATOR, payload: value.to_vec(), @@ -1204,7 +1204,7 @@ mod tests { assert!(decoded.errors.is_empty()); assert_eq!( decoded.update.path_attributes[3], - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { flags: PathAttributeFlags(0xe0), type_code: 99, payload: vec![1, 2, 3], diff --git a/crates/packet/tests/properties.rs b/crates/packet/tests/properties.rs index 4f4528b..c584431 100644 --- a/crates/packet/tests/properties.rs +++ b/crates/packet/tests/properties.rs @@ -25,8 +25,8 @@ use bgp_packet::message::{ PathAttributeFlags, RouteRefreshMessage, UpdateMessage, }; use bgp_packet::open::{ - AddPathEntry, AddPathSendRecv, BgpRole, BgpSecHeader, Capability, ExtendedNextHop, - GracefulRestartEntry, LongLivedGracefulRestart, MultiLabelEntry, OpenMessage, OpenOption, + AddPathEntry, AddPathSendRecv, BgpRole, BgpSecHeader, Capability, ExtendedNextHopEntry, + GracefulRestartEntry, LongLivedGracefulRestartEntry, MultiLabelEntry, OpenMessage, OpenOption, OrfFamily, OrfSendRecv, OrfType, OutboundRouteFilterEntry, }; use bgp_primitives::constants::{Afi, AfiSafi, IpVersion, Safi}; @@ -220,7 +220,7 @@ fn unknown_attributes() -> impl Strategy> { if payload.len() > 255 { flags |= 0x10; } - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { flags: PathAttributeFlags(flags), type_code, payload, @@ -402,7 +402,7 @@ fn capability() -> impl Strategy { vec((afi(), safi(), afi()), 1..4).prop_map(|entries| Capability::ExtendedNextHop { entries: entries .into_iter() - .map(|(afi, safi, nh_afi)| ExtendedNextHop { afi, safi, nh_afi }) + .map(|(afi, safi, nh_afi)| ExtendedNextHopEntry { afi, safi, nh_afi }) .collect(), }), Just(Capability::ExtendedMessage), @@ -411,7 +411,7 @@ fn capability() -> impl Strategy { afi, }), vec((afi(), safi(), any::()), 1..4).prop_map(|entries| { - Capability::MultiLabelCompat { + Capability::MultiLabel { entries: entries .into_iter() .map(|(afi, safi, count)| MultiLabelEntry { afi, safi, count }) @@ -458,12 +458,14 @@ fn capability() -> impl Strategy { Capability::LongLivedGracefulRestart( entries .into_iter() - .map(|(afi, safi, flags, stale_time)| LongLivedGracefulRestart { - afi, - safi, - flags, - stale_time, - }) + .map( + |(afi, safi, flags, stale_time)| LongLivedGracefulRestartEntry { + afi, + safi, + flags, + stale_time, + }, + ) .collect(), ) }), @@ -473,7 +475,7 @@ fn capability() -> impl Strategy { }), vec(any::(), 0..20) ) - .prop_map(|(type_code, value)| Capability::UnknownCapability { type_code, value }), + .prop_map(|(type_code, value)| Capability::Unknown { type_code, value }), ] } diff --git a/crates/server/src/config.rs b/crates/server/src/config.rs index c52b3b4..eeeddb7 100644 --- a/crates/server/src/config.rs +++ b/crates/server/src/config.rs @@ -17,7 +17,7 @@ use crate::error::{ConfigError, ExportProblem, ImportProblem, OriginateProblem, use crate::family::FamilySet; use crate::marks::{Control, Marks, Patterns, Scrub}; use crate::policy::{ - ExportLimits, ExportPolicy, ImportPolicy, ImportRule, PeerRole, PrefixSet, Reflection, + ExportLimits, ExportPolicy, ImportPolicy, ImportRule, PeerRole, PrefixSet, ReflectorIds, }; use crate::proto::config::{self as proto, BgpConfig, OriginatedRoute, Peer}; use crate::restart::GracefulRestartConfig; @@ -341,7 +341,7 @@ impl Ours { for peer in peers { if peer.remote_asn == self.local_asn { peer.import.originated = Arc::clone(&originated); - peer.import.reflection = Some(Reflection { + peer.import.reflection = Some(ReflectorIds { router_id: self.router_id, cluster_id: self.cluster_id, }); @@ -836,7 +836,7 @@ mod tests { assert!(config.peers[0].reflector_client); assert_eq!( config.peers[0].import.reflection, - Some(Reflection { + Some(ReflectorIds { router_id: config.router_id, cluster_id: config.router_id }) diff --git a/crates/server/src/export.rs b/crates/server/src/export.rs index 8acc6e3..ebf48fb 100644 --- a/crates/server/src/export.rs +++ b/crates/server/src/export.rs @@ -260,7 +260,7 @@ impl Egress<'_> { // What we do not understand goes on if it says it may, marked // as having passed a router that did not look at it; and no // further if not (RFC 4271 §5). - PathAttribute::UnknownPathAttribute { flags, .. } => { + PathAttribute::Unknown { flags, .. } => { let passes = flags.optional() && flags.transitive(); if passes { flags.set_partial(true); @@ -619,12 +619,12 @@ mod tests { communities(&[(3356, 3)]), // Understood by nobody here: one that may go on, one that may // not. - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { flags: PathAttributeFlags::new(true, true, false, false), type_code: 200, payload: vec![1], }, - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { flags: PathAttributeFlags::new(true, false, false, false), type_code: 201, payload: vec![2], @@ -641,7 +641,7 @@ mod tests { PathAttribute::Origin(OriginPathAttribute::IGP), AsPathAttribute::from_asns(vec![LOCAL, 3356, 64700]), communities(&[(3356, 3)]), - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { // Marked as having passed a router that did not know it. flags: PathAttributeFlags::new(true, true, true, false), type_code: 200, diff --git a/crates/server/src/fsm.rs b/crates/server/src/fsm.rs index e803c38..981c104 100644 --- a/crates/server/src/fsm.rs +++ b/crates/server/src/fsm.rs @@ -17,7 +17,7 @@ use std::time::Duration; use bgp_packet::errors::{BgpError, CeaseSubcode, DecodeError, OpenMsgSubcode}; use bgp_packet::message::{AS_TRANS, BGP4_VERSION, Message, NotificationMessage, UpdateMessage}; use bgp_packet::open::{ - AddPathEntry, AddPathSendRecv, Capability, ExtendedNextHop, OpenMessage, OpenOption, + AddPathEntry, AddPathSendRecv, Capability, ExtendedNextHopEntry, OpenMessage, OpenOption, }; use bgp_primitives::constants::{Afi, AfiSafi, Safi}; @@ -555,7 +555,7 @@ impl Fsm { /// IPv4 unicast routes with next hops that are IPv6 addresses, as the /// Extended Next Hop capability names it (RFC 8950 §3). -const IPV4_WITH_IPV6_NEXT_HOP: ExtendedNextHop = ExtendedNextHop { +const IPV4_WITH_IPV6_NEXT_HOP: ExtendedNextHopEntry = ExtendedNextHopEntry { afi: Afi::IPV4, safi: Safi::UNICAST, nh_afi: Afi::IPV6, @@ -1066,7 +1066,7 @@ mod tests { // Offered for something else: IPv6 routes with IPv4 next hops, // say. let other = Capability::ExtendedNextHop { - entries: vec![ExtendedNextHop { + entries: vec![ExtendedNextHopEntry { afi: Afi::IPV6, safi: Safi::UNICAST, nh_afi: Afi::IPV4, diff --git a/crates/server/src/marks.rs b/crates/server/src/marks.rs index 01a2261..f327abb 100644 --- a/crates/server/src/marks.rs +++ b/crates/server/src/marks.rs @@ -295,10 +295,10 @@ const NO_EXPORT_SUBCONFED: (u16, u16) = (0xFFFF, 0xFF03); /// all, as far as the well-known ones say: NO_ADVERTISE keeps it from /// everyone, NO_EXPORT from other networks. (There are no confederations /// here, so NO_EXPORT_SUBCONFED is the same as NO_EXPORT.) -pub fn well_known_allow(communities: &[(u16, u16)], peer_is_internal: bool) -> bool { +pub fn well_known_allow(communities: &[(u16, u16)], ibgp: bool) -> bool { !communities.iter().any(|community| { *community == NO_ADVERTISE - || (!peer_is_internal && matches!(*community, NO_EXPORT | NO_EXPORT_SUBCONFED)) + || (!ibgp && matches!(*community, NO_EXPORT | NO_EXPORT_SUBCONFED)) }) } diff --git a/crates/server/src/policy.rs b/crates/server/src/policy.rs index 27f6bf6..23fd3fd 100644 --- a/crates/server/src/policy.rs +++ b/crates/server/src/policy.rs @@ -123,7 +123,7 @@ pub struct ImportPolicy { /// For a peer in our own AS, what a route passed on inside the AS /// must not say it has already been through (RFC 4456 §8); `None` for /// a peer in another AS, whose routes carry no such thing. - pub reflection: Option, + pub reflection: Option, } /// Who we are to routes passed on from one router of our AS to another: @@ -131,7 +131,7 @@ pub struct ImportPolicy { /// whose CLUSTER_LIST holds our cluster ID has been passed on by us /// already. Either has gone round in a circle, and is not taken. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct Reflection { +pub struct ReflectorIds { pub router_id: Ipv4Addr, pub cluster_id: Ipv4Addr, } @@ -1068,7 +1068,7 @@ mod tests { let us = Ipv4Addr::new(10, 0, 0, 1); let cluster = Ipv4Addr::new(10, 0, 0, 9); let policy = ImportPolicy { - reflection: Some(Reflection { + reflection: Some(ReflectorIds { router_id: us, cluster_id: cluster, }), diff --git a/crates/web/src/api.rs b/crates/web/src/api.rs index 61d6b9e..0fd2f36 100644 --- a/crates/web/src/api.rs +++ b/crates/web/src/api.rs @@ -390,7 +390,7 @@ fn other_attribute(attribute: &PathAttribute) -> Option { "cluster list", list(ids.iter().map(Ipv4Addr::to_string).collect(), " "), ), - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { flags, type_code, payload, @@ -1412,7 +1412,7 @@ mod tests { Ipv4Addr::new(10, 0, 0, 1), Ipv4Addr::new(10, 0, 0, 2), ]), - PathAttribute::UnknownPathAttribute { + PathAttribute::Unknown { flags: PathAttributeFlags::new(true, true, false, false), type_code: 99, payload: vec![0xab, 0x01],