Start unifying un/numbered bgp peer internals (#790)

* Add mgd-unnumbered falcon-lab topology and test

Add simple p2p toplogy with two mgd nodes running BGP unnumbered. This
test captures the issue where outbound connect() attempts fail due to
an invalid destination TCP port being used (0), as observed in
maghemite#782.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* mgd: use BGP_PORT in unnumbered dummy sockaddr

Fixes: #782

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* Convert BGP_PORT from u16 to NonZeroU16

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* Unify internals for un/numbered peers

Instead of using a placeholder SocketAddr to represent the (peer, port),
use a PeerId and a NonZeruU16. This removes the need for unnumbered
peers to store a placeholder SocketAddr (which was only disambiguated by
scope_id anyway), since we have a proper way to represent them. This
provides the basis for further consolidation of un/numbered peers:
- merge new_session/new_unnumbered_session and
  ensure_session/ensure_unnumbered_session into single functions taking
  unnumbered_manager: Option<...>; drop the redundant peer_id arg from
  new_session_locked
- fold update_unnumbered_session into update_session
- widen Error::UnknownPeer from IpAddr to PeerId
- route reset_unnumbered_neighbor through the unified get_session,
  deleting UnnumberedManagerNdp::get_neighbor_session and its now-dead
  routers table

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* bgp: fix PeerId variant in unnumbered tests

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* bgp: bind unnumbered peers to ifindex

We had been binding unnumbered interfaces to a source SocketAddr with
the scope_id set, but this is not always sufficient for socket lookups
to ensure link-local traffic is delivered consistently (See #780). This
adds a setsockopt call to bind the socket to the unnumbered interface
(via a socket2 wrapper). There haven't been any issues observed/reported
with BgpConnection sockets so far, but it's better to be proactive.
Also refactors try_resolve_connect_addr to centralize logging in the
caller and give better context around the resolution failure by
returning a Result with a new Error type ResolvePeerError.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* buildomat: run mgd-unnumbered falcon-lab test

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* falcon-lab: add juniper, promote trio to quartet

Add support for JunOS in falcon-lab, integrating it into existing BFD
and BGP unnumbered tests (renaming trio to quartet). Adds a JuniperNode
type with full diagnostic collection.

JunOS images are expected to be prebuilt (this one was build using the
experimental voxel-image process) with a handful of prerequisites:
1) installed packages: docker, jq
2) crpd docker container installed
3) systemd service to manage creation of cargo-bay mount
4) systemd service to apply license and config to crpd

This allows falcon-lab to use cargo-bay as a transparent mount point so
it doesn't have to be in charge of managing its items. In particular,
the JunOS license must be staged there but falcon-lab is not responsible
for populating it. This is done either by the user or by CI.

Also refactors the linux diagnostics to be centralized so any other node
riding atop a linux base can make use of them (e.g. frr, eos, juniper).

Also updates the diagnostic collection logic to unpause nodes before
trying to grab diag info... can't really query a paused node, can you?

Also adds a README for falcon-lab.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* falcon-lab: cleanup old state, per protocol diag

Adds logic to cleanup old guest configs on startup.
Adds logic to CI to call cleanup on test topologies after each run.
Splits diag collection by protocol (e.g. BGP/BFD info only collected in
tests when those protocols are in use).

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* PR feedback

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

---------

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

Author: taspelund

.github/buildomat/jobs/falcon-lab.sh

@@ -54,6 +54,14 @@ mkdir -p cargo-bay
 mv mgd cargo-bay/
 mv ddmd cargo-bay/
 
+# Juniper/cRPD images require a runtime license. Fetch it on the CI runner,
+# which has catacomb access, and pass it to the guest by file via cargo-bay.
+# The license contents must never be printed or committed.
+curl -sSfL --retry 10 --retry-all-errors \
+	-o cargo-bay/falcon-juniper-license.key \
+	http://catacomb.eng.oxide.computer:12346/falcon/jl
+chmod 0600 cargo-bay/falcon-juniper-license.key
+
 export EXT_INTERFACE=${EXT_INTERFACE:-igb0}
 
 first=$(bmat address ls -f extra -Ho first)
@@ -62,8 +70,15 @@ gw=$(bmat address ls -f extra -Ho gateway)
 server=$(ipadm show-addr "${EXT_INTERFACE}"/dhcp -po ADDR | sed 's#/.*##g')
 pfexec ./dhcp-server "${first}" "${last}" "${gw}" "${server}" &> /work/dhcp-server.log &
 
-RUST_LOG=debug pfexec ./falcon-lab run \
-	trio-unnumbered
+run_test() {
+	local test=$1
+	local status=0
+
+	RUST_LOG=debug pfexec ./falcon-lab run "${test}" || status=$?
+	pfexec ./falcon-lab cleanup "${test}" || true
+	return "${status}"
+}
 
-RUST_LOG=debug pfexec ./falcon-lab run \
-	trio-bfd-static-routing
+run_test mgd-unnumbered
+run_test quartet-unnumbered
+run_test quartet-bfd-static-routing

bgp/src/config.rs

@@ -2,20 +2,23 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
+use crate::BGP_PORT;
 use mg_api_types::bgp::config::{
     BgpPeerParameters, Neighbor, UnnumberedNeighbor,
 };
+use mg_api_types::bgp::peer::PeerId;
 use mg_api_types_versions::v1;
 use rdb::Asn;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
-use std::net::{SocketAddr, SocketAddrV6};
+use std::num::NonZeroU16;
 
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
 pub struct PeerConfig {
     pub name: String,
     pub group: String,
-    pub host: SocketAddr,
+    pub id: PeerId,
+    pub port: NonZeroU16,
     pub hold_time: u64,
     pub idle_hold_time: u64,
     pub delay_open: u64,
@@ -77,7 +80,8 @@ impl From<Neighbor> for PeerConfig {
         Self {
             name,
             group,
-            host: *host,
+            id: PeerId::Ip(host.ip()),
+            port: NonZeroU16::new(host.port()).unwrap_or(BGP_PORT),
             hold_time,
             idle_hold_time,
             delay_open,
@@ -119,7 +123,8 @@ impl From<v1::bgp::config::Neighbor> for PeerConfig {
         Self {
             name,
             group,
-            host,
+            id: PeerId::Ip(host.ip()),
+            port: NonZeroU16::new(host.port()).unwrap_or(BGP_PORT),
             hold_time,
             idle_hold_time,
             delay_open,
@@ -131,17 +136,15 @@ impl From<v1::bgp::config::Neighbor> for PeerConfig {
 }
 
 impl PeerConfig {
-    /// Construct a `PeerConfig` from an `UnnumberedNeighbor` (uses the supplied
-    /// IPv6 link-local socket address as the connection target).
-    pub fn from_unnumbered_neighbor(
-        n: &UnnumberedNeighbor,
-        addr: SocketAddrV6,
-    ) -> Self {
+    /// Construct a `PeerConfig` from an `UnnumberedNeighbor`. The peer is
+    /// identified by its interface; the connection target is resolved via NDP
+    /// at connect time, and the port is always the well-known BGP port.
+    pub fn from_unnumbered_neighbor(n: &UnnumberedNeighbor) -> Self {
         let UnnumberedNeighbor {
             asn: _,
             name,
             group,
-            interface: _,
+            interface,
             act_as_a_default_ipv6_router: _,
             parameters,
         } = n.clone();
@@ -171,8 +174,9 @@ impl PeerConfig {
         } = parameters;
         Self {
             name,
-            host: addr.into(),
             group,
+            id: PeerId::Interface(interface),
+            port: BGP_PORT,
             hold_time,
             idle_hold_time,
             delay_open,

bgp/src/connection_channel.rs

@@ -256,7 +256,7 @@ impl BgpListener<BgpConnectionChannel> for BgpListenerChannel {
         let runner = lock!(sessions)
             .get(&key)
             .cloned()
-            .ok_or(Error::UnknownPeer(peer.ip()))?;
+            .ok_or_else(|| Error::UnknownPeer(key.clone()))?;
 
         let config = lock!(runner.session);
         Ok(BgpConnectionChannel::with_conn(

bgp/src/connection_tcp.rs

@@ -30,6 +30,7 @@ use std::{
     io::Read,
     io::Write,
     net::{SocketAddr, TcpListener, TcpStream, ToSocketAddrs},
+    num::NonZeroU32,
     sync::atomic::AtomicBool,
     sync::{Arc, Mutex, atomic::Ordering, mpsc::Sender},
     thread::{JoinHandle, sleep},
@@ -172,7 +173,7 @@ impl BgpListener<BgpConnectionTcp> for BgpListenerTcp {
                     let runner = lock!(sessions)
                         .get(&key)
                         .cloned()
-                        .ok_or(Error::UnknownPeer(ip))?;
+                        .ok_or_else(|| Error::UnknownPeer(key.clone()))?;
 
                     let config = lock!(runner.session);
                     return BgpConnectionTcp::with_conn(
@@ -316,6 +317,22 @@ impl BgpConnector<BgpConnectionTcp> for BgpConnectorTcp {
                     "timeout" => timeout.as_millis()
                 );
 
+                // Bind the socket to the destination's interface for scoped
+                // (link-local / unnumbered) peers.
+                if let SocketAddr::V6(v6) = peer
+                    && let Some(idx) = NonZeroU32::new(v6.scope_id())
+                    && let Err(e) = s.bind_device_by_index_v6(Some(idx))
+                {
+                    connection_log_lite!(log,
+                        warn,
+                        "failed to bind device index {idx} for {peer}: {e}";
+                        "direction" => ConnectionDirection::Outbound,
+                        "peer" => format!("{peer}"),
+                        "error" => format!("{e}")
+                    );
+                    return;
+                }
+
                 // Bind to source address/port if specified
                 if let Some(src) = config.bind_addr {
                     let ba: socket2::SockAddr = src.into();
@@ -1382,7 +1399,7 @@ fn get_md5_source_addrs(peer_ip: IpAddr) -> Result<Vec<SocketAddr>, Error> {
 
     Ok(sources
         .iter()
-        .map(|x| SocketAddr::new(*x, crate::BGP_PORT))
+        .map(|x| SocketAddr::new(*x, crate::BGP_PORT.get()))
         .collect())
 }
 
@@ -1520,7 +1537,7 @@ fn setup_outbound_md5(
 
     let local: Vec<SocketAddr> = sources
         .iter()
-        .map(|x| SocketAddr::new(*x, crate::BGP_PORT))
+        .map(|x| SocketAddr::new(*x, crate::BGP_PORT.get()))
         .collect();
 
     init_md5_associations(fd, key, local.clone(), peer)?;

bgp/src/error.rs

@@ -7,6 +7,7 @@ use std::{
     net::{IpAddr, Ipv4Addr},
 };
 
+use mg_api_types::bgp::peer::PeerId;
 use num_enum::TryFromPrimitiveError;
 
 #[derive(thiserror::Error, Debug)]
@@ -122,7 +123,7 @@ pub enum Error {
     NotConnected,
 
     #[error("Connection attempt from unknown peer: {0}")]
-    UnknownPeer(IpAddr),
+    UnknownPeer(PeerId),
 
     #[error("Session for peer already exists")]
     PeerExists,

bgp/src/lib.rs

@@ -16,6 +16,8 @@ pub mod router;
 pub mod session;
 pub mod unnumbered;
 
+use std::num::NonZeroU16;
+
 mod rhai_integration;
 
 #[cfg(test)]
@@ -34,7 +36,7 @@ pub mod connection_channel;
 #[cfg(test)]
 pub mod unnumbered_mock;
 
-pub const BGP_PORT: u16 = 179;
+pub const BGP_PORT: NonZeroU16 = NonZeroU16::new(179).unwrap();
 pub const BGP_VERSION: u8 = 4;
 pub const COMPONENT_BGP: &str = "bgp";
 pub const MOD_ROUTER: &str = "router";

bgp/src/router.rs

@@ -305,104 +305,57 @@ impl<Cnx: BgpConnection + 'static> Router<Cnx> {
         }
     }
 
+    #[allow(clippy::too_many_arguments)]
     pub fn ensure_session(
         self: &Arc<Self>,
         peer: PeerConfig,
         bind_addr: Option<SocketAddr>,
         event_tx: Sender<FsmEvent<Cnx>>,
         event_rx: Receiver<FsmEvent<Cnx>>,
         info: SessionInfo,
+        unnumbered_manager: Option<Arc<dyn UnnumberedManager>>,
     ) -> Result<EnsureSessionResult<Cnx>, Error> {
         let sessions = lock!(self.sessions);
-        let key = PeerId::Ip(peer.host.ip());
-        if sessions.contains_key(&key) {
+        if sessions.contains_key(&peer.id) {
             drop(sessions);
             Ok(EnsureSessionResult::Updated(
                 self.update_session(peer, info)?,
             ))
-        } else {
-            Ok(EnsureSessionResult::New(self.new_session_locked(
-                sessions, key, peer, bind_addr, event_tx, event_rx, info, None,
-            )?))
-        }
-    }
-
-    #[allow(clippy::too_many_arguments)]
-    pub fn ensure_unnumbered_session(
-        self: &Arc<Self>,
-        interface: String,
-        peer: PeerConfig,
-        bind_addr: Option<SocketAddr>,
-        event_tx: Sender<FsmEvent<Cnx>>,
-        event_rx: Receiver<FsmEvent<Cnx>>,
-        info: SessionInfo,
-        unnumbered_manager: Arc<dyn UnnumberedManager>,
-    ) -> Result<EnsureSessionResult<Cnx>, Error> {
-        let sessions = lock!(self.sessions);
-        let key = PeerId::Interface(interface.clone());
-        if sessions.contains_key(&key) {
-            drop(sessions);
-            Ok(EnsureSessionResult::Updated(
-                self.update_unnumbered_session(&interface, peer, info)?,
-            ))
         } else {
             Ok(EnsureSessionResult::New(self.new_session_locked(
                 sessions,
-                key,
                 peer,
                 bind_addr,
                 event_tx,
                 event_rx,
                 info,
-                Some(unnumbered_manager),
+                unnumbered_manager,
             )?))
         }
     }
 
-    pub fn new_session(
-        self: &Arc<Self>,
-        peer: PeerConfig,
-        bind_addr: Option<SocketAddr>,
-        event_tx: Sender<FsmEvent<Cnx>>,
-        event_rx: Receiver<FsmEvent<Cnx>>,
-        info: SessionInfo,
-    ) -> Result<Arc<SessionRunner<Cnx>>, Error> {
-        let sessions = lock!(self.sessions);
-        let key = PeerId::Ip(peer.host.ip());
-        if sessions.contains_key(&key) {
-            Err(Error::PeerExists)
-        } else {
-            self.new_session_locked(
-                sessions, key, peer, bind_addr, event_tx, event_rx, info, None,
-            )
-        }
-    }
-
     #[allow(clippy::too_many_arguments)]
-    pub fn new_unnumbered_session(
+    pub fn new_session(
         self: &Arc<Self>,
-        interface: String,
         peer: PeerConfig,
         bind_addr: Option<SocketAddr>,
         event_tx: Sender<FsmEvent<Cnx>>,
         event_rx: Receiver<FsmEvent<Cnx>>,
         info: SessionInfo,
-        unnumbered_manager: Arc<dyn UnnumberedManager>,
+        unnumbered_manager: Option<Arc<dyn UnnumberedManager>>,
     ) -> Result<Arc<SessionRunner<Cnx>>, Error> {
         let sessions = lock!(self.sessions);
-        let key = PeerId::Interface(interface);
-        if sessions.contains_key(&key) {
+        if sessions.contains_key(&peer.id) {
             Err(Error::PeerExists)
         } else {
             self.new_session_locked(
                 sessions,
-                key,
                 peer,
                 bind_addr,
                 event_tx,
                 event_rx,
                 info,
-                Some(unnumbered_manager),
+                unnumbered_manager,
             )
         }
     }
@@ -411,7 +364,6 @@ impl<Cnx: BgpConnection + 'static> Router<Cnx> {
     fn new_session_locked(
         self: &Arc<Self>,
         mut sessions: MutexGuard<SessionMap<Cnx>>,
-        peer_id: PeerId,
         peer: PeerConfig,
         bind_addr: Option<SocketAddr>,
         event_tx: Sender<FsmEvent<Cnx>>,
@@ -434,8 +386,8 @@ impl<Cnx: BgpConnection + 'static> Router<Cnx> {
         let neighbor = NeighborInfo {
             name: Arc::new(Mutex::new(peer.name.clone())),
             peer_group: peer.group.clone(),
-            peer: peer_id,
-            port: peer.host.port(),
+            peer: peer.id.clone(),
+            port: peer.port,
         };
 
         let runner = Arc::new(SessionRunner::new(
@@ -460,33 +412,9 @@ impl<Cnx: BgpConnection + 'static> Router<Cnx> {
         peer: PeerConfig,
         info: SessionInfo,
     ) -> Result<Arc<SessionRunner<Cnx>>, Error> {
-        // Use PeerId::Ip for numbered sessions
-        let key = PeerId::Ip(peer.host.ip());
-        let session = match lock!(self.sessions).get(&key) {
-            None => return Err(Error::UnknownPeer(peer.host.ip())),
-            Some(s) => s.clone(),
-        };
-
-        session.update_session_parameters(peer, info)?;
-
-        Ok(session)
-    }
-
-    pub fn update_unnumbered_session(
-        self: &Arc<Self>,
-        interface: &str,
-        peer: PeerConfig,
-        info: SessionInfo,
-    ) -> Result<Arc<SessionRunner<Cnx>>, Error> {
-        // Use PeerId::Interface for unnumbered sessions
-        let key = PeerId::Interface(interface.to_string());
+        let key = peer.id.clone();
         let session = match lock!(self.sessions).get(&key) {
-            None => {
-                return Err(Error::InternalCommunication(format!(
-                    "unnumbered session not found for interface: {}",
-                    interface
-                )));
-            }
+            None => return Err(Error::UnknownPeer(key)),
             Some(s) => s.clone(),
         };