chore(bgp): Add BGP unnumbered

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<Self, Self::Error> {
-        let neighbor_addr = match neighbor.ip.as_ref() {
-            Some(ip) => ip.parse::<IpAddr>().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::<IpAddr>().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<Self, Self::Error> {
-        // 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());
+    }
 }

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<Self, ConfigError> {
         debug!("Validating underlay configuration...");
 
@@ -86,10 +128,155 @@ 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
             vtep: self.get_vtep_info()?,
         })
     }
 }
+
+#[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<BgpNeighbor>) -> 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 <ifname> 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());
+    }
+}