diff --git a/config/src/converters/k8s/config/bgp.rs b/config/src/converters/k8s/config/bgp.rs index 83d3d6e473..a9913c09a4 100644 --- a/config/src/converters/k8s/config/bgp.rs +++ b/config/src/converters/k8s/config/bgp.rs @@ -11,21 +11,41 @@ use crate::internal::routing::bgp::{BgpNeighType, BgpNeighbor, BgpUpdateSource}; impl TryFrom<&GatewayAgentGatewayNeighbors> for BgpNeighbor { type Error = FromK8sConversionError; + /// Decode one CRD neighbor entry into the internal model. + /// + /// An entry with an address is a numbered peer whose `source` is the local + /// update-source. An entry without one is BGP unnumbered, and `source` then + /// names the interface to peer over instead. Whether that interface exists, + /// and whether its addressing agrees with the neighbor's, is checked in + /// `Underlay::validate` rather than here. + /// + /// # Errors + /// + /// Returns [`FromK8sConversionError`] if the remote ASN is missing, if the + /// address is present but unparseable, or if an address-less entry names no + /// source interface and so identifies no peer at all. fn try_from(neighbor: &GatewayAgentGatewayNeighbors) -> Result { - let neighbor_addr = match neighbor.ip.as_ref() { - Some(ip) => ip.parse::().map_err(|e| { - FromK8sConversionError::InvalidData(format!("neighbor address {ip}: {e}")) - })?, - None => { - return Err(FromK8sConversionError::MissingData(format!( - "Missing neighbor address in BGP neighbor with ASN {}", - neighbor.asn.ok_or(FromK8sConversionError::MissingData( - "Missing neighbor address and ASN in BGP neighbor".to_string() - ))? - ))); - } + // A neighbor with no address is BGP unnumbered. Pick source here + let Some(ip) = neighbor.ip.as_ref() else { + let ifname = neighbor.source.as_ref().ok_or_else(|| { + FromK8sConversionError::MissingData( + "BGP neighbor has neither an address nor a source interface: an unnumbered \ + neighbor must name the interface to peer over" + .to_string(), + ) + })?; + let remote_as = neighbor + .asn + .ok_or(FromK8sConversionError::MissingData(format!( + "Missing ASN in unnumbered BGP neighbor on interface {ifname}" + )))?; + return Ok(BgpNeighbor::new_interface(ifname).set_remote_as(remote_as)); }; + let neighbor_addr = ip.parse::().map_err(|e| { + FromK8sConversionError::InvalidData(format!("neighbor address {ip}: {e}")) + })?; + // Parse remote ASN let remote_as = neighbor .asn @@ -49,10 +69,33 @@ impl TryFrom<&GatewayAgentGatewayNeighbors> for BgpNeighbor { impl TryFrom<&BgpNeighbor> for GatewayAgentGatewayNeighbors { type Error = ToK8sConversionError; + /// Encode an internal BGP neighbor back into a CRD entry. + /// + /// A numbered peer keeps its address, with `source` carrying an interface + /// update-source if it has one. An unnumbered peer has no address and maps + /// back to its interface name in `source`, mirroring the decode above. + /// + /// # Errors + /// + /// Returns [`ToK8sConversionError`] for neighbors the CRD cannot express: + /// peer groups, a neighbor with no type set, an address-valued + /// update-source, or a neighbor with no remote ASN. fn try_from(neighbor: &BgpNeighbor) -> Result { - // Get neighbor address safely - let ip = match &neighbor.ntype { - BgpNeighType::Host(addr) => addr.to_string(), + let (ip, source) = match &neighbor.ntype { + BgpNeighType::Host(addr) => { + let source = neighbor + .update_source + .as_ref() + .map(|source| match source { + BgpUpdateSource::Interface(intf) => Ok(intf.clone()), + BgpUpdateSource::Address(_) => Err(ToK8sConversionError::Unsupported( + "Unsupported BgpUpdateSource type".to_string(), + )), + }) + .transpose()?; + (Some(addr.to_string()), source) + } + BgpNeighType::Interface(ifname) => (None, Some(ifname.clone())), BgpNeighType::PeerGroup(name) => { return Err(ToK8sConversionError::Unsupported(format!( "Peer group type not supported in CRD: {name}" @@ -70,20 +113,9 @@ impl TryFrom<&BgpNeighbor> for GatewayAgentGatewayNeighbors { ToK8sConversionError::MissingData("Missing remote ASN for BGP neighbor".to_string()) })?; - let source = neighbor - .update_source - .as_ref() - .map(|source| match source { - BgpUpdateSource::Interface(intf) => Ok(intf.clone()), - BgpUpdateSource::Address(_) => Err(ToK8sConversionError::Unsupported( - "Unsupported BgpUpdateSource type".to_string(), - )), - }) - .transpose()?; - Ok(GatewayAgentGatewayNeighbors { asn: Some(*asn), - ip: Some(ip), + ip, source, }) } @@ -107,4 +139,54 @@ mod tests { assert_eq!(neighbor.normalize(), converted_neighbor); }); } + + /// A neighbor with no `ip` is the BGP-unnumbered case: it becomes an + /// interface peer over `source`, and `source` is *not* reused as an + /// update-source. + #[test] + fn test_unnumbered_neighbor_conversion() { + let crd = GatewayAgentGatewayNeighbors { + asn: Some(65100), + ip: None, + source: Some("enp2s1np0".to_string()), + }; + + let neigh = BgpNeighbor::try_from(&crd).expect("unnumbered neighbor should convert"); + assert!(matches!(&neigh.ntype, BgpNeighType::Interface(i) if i == "enp2s1np0")); + assert_eq!(neigh.remote_as, Some(65100)); + assert!(neigh.update_source.is_none()); + + // and it round-trips back to the same CRD shape + let back = GatewayAgentGatewayNeighbors::try_from(&neigh).expect("should convert back"); + assert_eq!(back, crd); + } + + /// A neighbor with an `ip` keeps the numbered behaviour: `source` is the + /// update-source, not the peering interface. + #[test] + fn test_numbered_neighbor_keeps_update_source() { + let crd = GatewayAgentGatewayNeighbors { + asn: Some(65100), + ip: Some("172.30.128.22".to_string()), + source: Some("enp2s1np0".to_string()), + }; + + let neigh = BgpNeighbor::try_from(&crd).expect("numbered neighbor should convert"); + assert!(matches!(neigh.ntype, BgpNeighType::Host(_))); + assert!(matches!( + &neigh.update_source, + Some(BgpUpdateSource::Interface(i)) if i == "enp2s1np0" + )); + } + + /// Neither an address nor a source interface leaves nothing to peer with. + #[test] + fn test_neighbor_without_ip_or_source_is_rejected() { + let crd = GatewayAgentGatewayNeighbors { + asn: Some(65100), + ip: None, + source: None, + }; + assert!(BgpNeighbor::try_from(&crd).is_err()); + } } diff --git a/config/src/external/underlay/mod.rs b/config/src/external/underlay/mod.rs index 080c1f5204..7c8e52a09e 100644 --- a/config/src/external/underlay/mod.rs +++ b/config/src/external/underlay/mod.rs @@ -3,10 +3,11 @@ //! Underlay configuration -use crate::ConfigError; use crate::internal::interfaces::interface::{InterfaceConfig, InterfaceType}; +use crate::internal::routing::bgp::{BgpNeighType, BgpUpdateSource}; use crate::internal::routing::evpn::VtepConfig; use crate::internal::routing::vrf::VrfConfig; +use crate::{ConfigError, ConfigResult}; use net::eth::mac::SourceMac; use net::ipv4::UnicastIpv4Addr; @@ -72,11 +73,52 @@ impl Underlay { } } + /// Check that every BGP neighbor agrees with the addressing of the interface + /// it names. + fn validate_bgp_neighbor_addressing(&self) -> ConfigResult { + let Some(bgp) = &self.vrf.bgp else { + return Ok(()); + }; + + for neigh in &bgp.neighbors { + match &neigh.ntype { + BgpNeighType::Interface(ifname) => { + let iface = self.vrf.interfaces.get(ifname).ok_or_else(|| { + ConfigError::Invalid(format!( + "BGP neighbor peers over interface '{ifname}', which is not configured" + )) + })?; + if iface.has_ipv4_address() { + return Err(ConfigError::Invalid(format!( + "BGP neighbor over interface '{ifname}' has no address, requesting \ + BGP unnumbered, but '{ifname}' has an IPv4 address: unnumbered \ + requires an interface with no IPv4 addressing" + ))); + } + } + BgpNeighType::Host(addr) => { + if let Some(BgpUpdateSource::Interface(ifname)) = &neigh.update_source + && let Some(iface) = self.vrf.interfaces.get(ifname) + && !iface.has_ipv4_address() + { + return Err(ConfigError::Invalid(format!( + "BGP neighbor {addr} is sourced from interface '{ifname}', which has \ + no IPv4 address" + ))); + } + } + BgpNeighType::PeerGroup(_) | BgpNeighType::Unset => {} + } + } + Ok(()) + } + /// Validate the underlay configuration. /// /// # Errors /// - /// Returns an error if any interface is invalid or VTEP configuration is wrong. + /// Returns an error if any interface is invalid, VTEP configuration is wrong, + /// or a BGP neighbor disagrees with the addressing of the interface it names. pub fn validate(&self) -> Result { debug!("Validating underlay configuration..."); @@ -86,6 +128,8 @@ impl Underlay { .values() .try_for_each(InterfaceConfig::validate)?; + self.validate_bgp_neighbor_addressing()?; + Ok(Self { vrf: self.vrf.clone(), // set vtep information if a vtep interface has been specified in the config @@ -93,3 +137,146 @@ impl Underlay { }) } } + +#[cfg(test)] +mod tests { + use super::*; + + use crate::internal::interfaces::interface::{IfEthConfig, InterfaceConfig}; + use crate::internal::routing::bgp::{BgpConfig, BgpNeighbor}; + use std::net::IpAddr; + use std::str::FromStr; + + /// An underlay with the given ethernet interfaces (name, addresses) and BGP + /// neighbors. + fn underlay_with(ifaces: &[(&str, &[&str])], neighs: Vec) -> Underlay { + let mut vrf = VrfConfig::new("default", None, true); + + for (name, ips) in ifaces { + let mut iface = InterfaceConfig::new( + name, + InterfaceType::Ethernet(IfEthConfig { mac: None }), + false, + ); + for ip in *ips { + let (addr, len) = ip.split_once('/').expect("test address needs a mask"); + iface = iface.add_address( + IpAddr::from_str(addr).expect("bad test address"), + len.parse().expect("bad test mask"), + ); + } + vrf.add_interface_config(iface); + } + + let mut bgp = BgpConfig::new(65000); + for neigh in neighs { + bgp.add_neighbor(neigh); + } + vrf.set_bgp(bgp); + + Underlay { vrf, vtep: None } + } + + /// Parse a neighbor address written as a plain literal in a test. + fn host(addr: &str) -> IpAddr { + IpAddr::from_str(addr).expect("bad test address") + } + + /// The valid unnumbered shape: no neighbor address, no IPv4 on the link. + #[test] + fn test_unnumbered_over_unaddressed_interface_is_valid() { + let underlay = underlay_with( + &[("enp2s1np0", &[])], + vec![BgpNeighbor::new_interface("enp2s1np0").set_remote_as(65100)], + ); + assert!(underlay.validate().is_ok()); + } + + /// IPv6 on the link does not interfere: FRR's IPv4 peer derivation only looks + /// at `AF_INET`, so link-local peering still happens. + #[test] + fn test_unnumbered_over_ipv6_only_interface_is_valid() { + let underlay = underlay_with( + &[("enp2s1np0", &["2001:db8::1/64"])], + vec![BgpNeighbor::new_interface("enp2s1np0").set_remote_as(65100)], + ); + assert!(underlay.validate().is_ok()); + } + + /// A /31 on the link would make FRR derive the far end and peer over IPv4 + /// rather than link-local, so the combination is refused. + #[test] + fn test_unnumbered_over_ipv4_interface_is_rejected() { + let underlay = underlay_with( + &[("enp2s1np0", &["172.30.128.23/31"])], + vec![BgpNeighbor::new_interface("enp2s1np0").set_remote_as(65100)], + ); + let err = underlay + .validate() + .expect_err("IPv4 on an unnumbered link must be rejected"); + assert!( + err.to_string().contains("enp2s1np0"), + "error should name the interface: {err}" + ); + } + + /// Any IPv4 prefix length is refused, not just the /30 and /31 FRR would + /// derive a peer from: the rule is "no IPv4 on an unnumbered link". + #[test] + fn test_unnumbered_over_non_p2p_ipv4_interface_is_rejected() { + let underlay = underlay_with( + &[("enp2s1np0", &["10.0.0.1/24"])], + vec![BgpNeighbor::new_interface("enp2s1np0").set_remote_as(65100)], + ); + assert!(underlay.validate().is_err()); + } + + /// An unnumbered peer must name an interface that exists, since that name is + /// what gets rendered as `neighbor interface`. + #[test] + fn test_unnumbered_over_unknown_interface_is_rejected() { + let underlay = underlay_with( + &[("enp2s1np0", &[])], + vec![BgpNeighbor::new_interface("eth0").set_remote_as(65100)], + ); + assert!(underlay.validate().is_err()); + } + + /// The ordinary fabric case: numbered neighbor, /31 on the link. + #[test] + fn test_numbered_over_ipv4_interface_is_valid() { + let neigh = BgpNeighbor::new_host(host("172.30.128.22")) + .set_remote_as(65100) + .set_update_source_interface("enp2s1np0"); + let underlay = underlay_with(&[("enp2s1np0", &["172.30.128.23/31"])], vec![neigh]); + assert!(underlay.validate().is_ok()); + } + + /// The mirror rule: a session to an explicit address cannot be sourced from + /// an interface with no IPv4 address. + #[test] + fn test_numbered_over_unaddressed_interface_is_rejected() { + let neigh = BgpNeighbor::new_host(host("172.30.128.22")) + .set_remote_as(65100) + .set_update_source_interface("enp2s1np0"); + let underlay = underlay_with(&[("enp2s1np0", &[])], vec![neigh]); + let err = underlay + .validate() + .expect_err("an unaddressed update-source must be rejected"); + assert!( + err.to_string().contains("enp2s1np0"), + "error should name the interface: {err}" + ); + } + + /// An update-source naming an interface this VRF does not hold is left alone: + /// it may be one created elsewhere, such as the `lo` carrying the VTEP address. + #[test] + fn test_numbered_with_foreign_update_source_is_left_alone() { + let neigh = BgpNeighbor::new_host(host("172.30.128.22")) + .set_remote_as(65100) + .set_update_source_interface("lo"); + let underlay = underlay_with(&[("enp2s1np0", &["172.30.128.23/31"])], vec![neigh]); + assert!(underlay.validate().is_ok()); + } +} diff --git a/config/src/internal/interfaces/interface.rs b/config/src/internal/interfaces/interface.rs index 78e3b8c653..1491c9b857 100644 --- a/config/src/internal/interfaces/interface.rs +++ b/config/src/internal/interfaces/interface.rs @@ -167,6 +167,12 @@ impl InterfaceConfig { } Ok(()) } + + /// Whether any IPv4 address is configured on this interface. + #[must_use] + pub fn has_ipv4_address(&self) -> bool { + self.addresses.iter().any(|a| a.address.is_ipv4()) + } } impl InterfaceConfigTable { @@ -177,6 +183,12 @@ impl InterfaceConfigTable { pub fn add_interface_config(&mut self, cfg: InterfaceConfig) { self.0.insert(cfg.name.clone(), cfg); } + /// Look up an interface by name, or `None` if the table holds no such + /// interface. + #[must_use] + pub fn get(&self, name: &str) -> Option<&InterfaceConfig> { + self.0.get(name) + } pub fn values(&self) -> impl Iterator { self.0.values() } diff --git a/config/src/internal/routing/bgp.rs b/config/src/internal/routing/bgp.rs index 1dd3e07a1e..acf7083869 100644 --- a/config/src/internal/routing/bgp.rs +++ b/config/src/internal/routing/bgp.rs @@ -95,6 +95,10 @@ pub enum BgpNeighType { Unset, Host(IpAddr), PeerGroup(String), + /// An unnumbered (interface) peer: the session is established over the + /// named interface, with FRR discovering the peer from its IPv6 + /// link-local router advertisements instead of a configured address. + Interface(String), } #[derive(Clone, Debug, Default)] @@ -396,10 +400,24 @@ impl BgpNeighbor { ..Default::default() } } + /// Build an unnumbered (interface) neighbor peering over `ifname`. + #[must_use] + pub fn new_interface(ifname: &str) -> Self { + Self { + ntype: BgpNeighType::Interface(ifname.to_owned()), + ..Default::default() + } + } #[must_use] pub fn is_peer_group(&self) -> bool { matches!(self.ntype, BgpNeighType::PeerGroup(_)) } + /// Whether this is an unnumbered peer, i.e. one identified by the interface + /// it peers over rather than by an address. + #[must_use] + pub fn is_interface(&self) -> bool { + matches!(self.ntype, BgpNeighType::Interface(_)) + } /* capabilities */ #[must_use] diff --git a/k8s-intf/src/bolero/bgp.rs b/k8s-intf/src/bolero/bgp.rs index 4014c2cb37..a4a49893a2 100644 --- a/k8s-intf/src/bolero/bgp.rs +++ b/k8s-intf/src/bolero/bgp.rs @@ -12,15 +12,26 @@ use crate::bolero::{LegalValue, Normalize}; use crate::gateway_agent_crd::GatewayAgentGatewayNeighbors; impl TypeGenerator for LegalValue { + /// Generate a BGP neighbor entry the conversion will accept: a non-zero ASN, + /// an interface name in `source`, and either an IPv4 address (a numbered + /// peer) or none at all (an unnumbered peer peering over `source`). fn generate(d: &mut D) -> Option { let asn = d.gen_u32(Bound::Included(&1), Bound::Included(&u32::MAX))?; - let ip = d.produce::()?; let source = d.produce::()?.to_string(); + // An address-less entry is BGP unnumbered, and `source` then names the + // interface to peer over rather than the update-source. Generate both + // shapes. + let ip = if d.produce::()? { + None + } else { + Some(d.produce::()?.to_string()) + }; + Some(LegalValue(GatewayAgentGatewayNeighbors { asn: Some(asn), - ip: Some(ip.to_string()), + ip, source: Some(source), })) } diff --git a/routing/src/frr/renderer/bgp.rs b/routing/src/frr/renderer/bgp.rs index 0a48a7f846..d24f9466bb 100644 --- a/routing/src/frr/renderer/bgp.rs +++ b/routing/src/frr/renderer/bgp.rs @@ -16,11 +16,21 @@ use config::internal::routing::bmp::{BmpOptions, BmpSource}; /* impl Display */ impl Rendered for BgpNeighType { + /// The name FRR keys this neighbor by: an address for a numbered peer, the + /// group name for a peer group, and the interface name for an unnumbered + /// peer. Every per-neighbor line uses it, both under `router bgp` and in the + /// `neighbor activate` lines of each address family. + /// + /// # Panics + /// + /// Panics on [`BgpNeighType::Unset`], which is a neighbor that was never + /// given a type and so cannot be rendered at all. fn rendered(&self) -> String { match self { BgpNeighType::Unset => panic!("Bgp neighbor without type"), BgpNeighType::Host(address) => format!("{address}"), BgpNeighType::PeerGroup(group) => group.clone(), + BgpNeighType::Interface(ifname) => ifname.clone(), } } } @@ -117,19 +127,42 @@ impl Render for BmpOptions { } /* utils to render BGP neighbor configs */ + +/// Render the line that defines a neighbor, which every other `neighbor +/// ...` line then refines. One of: +/// +/// ```text +/// neighbor SPINES peer-group +/// neighbor 172.30.128.22 remote-as 65100 +/// neighbor 172.30.128.22 peer-group SPINES +/// neighbor enp2s1np0 interface remote-as 65100 +/// ``` +/// +/// The `interface` keyword marks an unnumbered peer and must sit between the +/// name and the `remote-as`/`peer-group` clause, which is why this cannot be +/// built from the same string as the other per-neighbor lines. +/// +/// # Panics +/// +/// Panics if a neighbor that is not a peer group has neither a peer group nor a +/// remote ASN, leaving nothing to peer with. fn bgp_neigh_minimal(neigh: &BgpNeighbor, name: &str) -> String { - let mut out; if neigh.is_peer_group() { - out = format!(" neighbor {name} peer-group"); + return format!(" neighbor {name} peer-group"); + } + let mut out = format!(" neighbor {name}"); + /* unnumbered peers: FRR's grammar puts `interface` between the peer name + (an interface name here) and the peer-group / remote-as clause: + `neighbor interface remote-as ` */ + if neigh.is_interface() { + out += " interface"; + } + if let Some(peer_group) = &neigh.peer_group { + out += format!(" peer-group {peer_group}").as_str(); + } else if let Some(remote_as) = neigh.remote_as { + out += format!(" remote-as {remote_as}").as_str(); } else { - out = format!(" neighbor {name}"); - if let Some(peer_group) = &neigh.peer_group { - out += format!(" peer-group {peer_group}").as_str(); - } else if let Some(remote_as) = neigh.remote_as { - out += format!(" remote-as {remote_as}").as_str(); - } else { - panic!("Missing peer-group or ASN"); - } + panic!("Missing peer-group or ASN"); } out } @@ -607,6 +640,71 @@ pub mod tests { use std::str::FromStr; use std::time::Duration; + /// An unnumbered neighbor must render as `neighbor interface + /// remote-as `: the `interface` keyword is what tells FRR to peer over + /// the interface's IPv6 link-local rather than treating the name as an + /// address. Every other per-neighbor line keys on the bare interface name, + /// exactly as it keys on the address for a numbered peer. + #[test] + fn test_bgp_render_unnumbered_neighbor() { + let mut bgp = BgpConfig::new(65000); + + let mut neigh = BgpNeighbor::new_interface("eth0") + .set_remote_as(65100) + .set_description("Unnumbered fabric peer") + .set_bfd(true); + neigh.ipv4_unicast_activate(BgpNeighAF::default()); + neigh.l2vpn_evpn_activate(BgpNeighAF::with_rmap_in("RM-EVPN-IN")); + bgp.add_neighbor(neigh); + bgp.set_af_ipv4unicast(AfIpv4Ucast::new()); + bgp.set_af_l2vpn_evpn(AfL2vpnEvpn::new()); + + let rendered = bgp.render(&()).to_string(); + let lines: Vec<&str> = rendered.lines().collect(); + + assert!( + lines.contains(&" neighbor eth0 interface remote-as 65100"), + "missing unnumbered neighbor line in:\n{rendered}" + ); + assert!(lines.contains(&" neighbor eth0 description Unnumbered fabric peer")); + assert!(lines.contains(&" neighbor eth0 bfd")); + assert!(lines.contains(&" neighbor eth0 activate")); + assert!(lines.contains(&" neighbor eth0 route-map RM-EVPN-IN in")); + /* the `interface` keyword belongs on the defining line only */ + assert_eq!( + lines + .iter() + .filter(|l| l.contains(" interface")) + .collect::>(), + vec![&" neighbor eth0 interface remote-as 65100"], + "`interface` leaked onto a non-defining line in:\n{rendered}" + ); + /* unnumbered peers get no update-source: `interface` already binds it */ + assert!(!rendered.contains("update-source")); + } + + /// A numbered neighbor must keep rendering exactly as before, with no + /// `interface` keyword. + #[test] + fn test_bgp_render_numbered_neighbor_unchanged() { + let mut bgp = BgpConfig::new(65000); + bgp.add_neighbor( + BgpNeighbor::new_host(IpAddr::from_str("172.30.128.1").expect("Bad address")) + .set_remote_as(65100) + .set_update_source_interface("eth0"), + ); + + let rendered = bgp.render(&()).to_string(); + let lines: Vec<&str> = rendered.lines().collect(); + + assert!(lines.contains(&" neighbor 172.30.128.1 remote-as 65100")); + assert!(lines.contains(&" neighbor 172.30.128.1 update-source eth0")); + assert!( + !rendered.contains("interface remote-as"), + "numbered peer must not be rendered as unnumbered:\n{rendered}" + ); + } + #[test] #[allow(clippy::too_many_lines)] fn test_bgp_render() {