Routing sanities

routing/src/cli/display.rs

@@ -123,6 +123,9 @@ impl Display for Nhop {
         if self.invalid.get() {
             write!(f, " (INVALID)")?;
         }
+        if self.is_unresolved() {
+            write!(f, " (unresolved)")?;
+        }
         fmt_nhop_resolvers(f, self, 2)
     }
 }
@@ -138,6 +141,9 @@ fn fmt_nhop_resolvers(f: &mut std::fmt::Formatter<'_>, rc: &Nhop, depth: u8) ->
         for r in resolvers.iter() {
             if let Some(r) = r.upgrade().as_ref() {
                 write!(f, "\n{indent} {}", r.key)?;
+                if r.is_unresolved() {
+                    write!(f, " (UNRESOLVED)")?;
+                }
                 fmt_nhop_resolvers(f, r, depth + 1)?;
             }
         }
@@ -167,14 +173,18 @@ fn fmt_nhop_rec(f: &mut std::fmt::Formatter<'_>, rc: &Rc<Nhop>, depth: u8) -> st
     let indent = " ".repeat(tab);
 
     let sym = if depth == 0 { "NH" } else { "ref" };
-    writeln!(
+    write!(
         f,
         "{} ({}) {} = {}",
         indent,
         Rc::strong_count(rc),
         sym,
         rc.key
     )?;
+    if rc.is_unresolved() {
+        write!(f, " (UNRESOLVED)")?;
+    }
+    writeln!(f)?;
     //    fmt_nhop_instruction(f, rc)?;
 
     let Ok(resolvers) = rc.resolvers.try_borrow() else {

routing/src/errors.rs

@@ -54,4 +54,7 @@ pub enum RouterError {
 
     #[error("Invalid interface address: {0}")]
     IfAddressError(#[from] IfAddrError),
+
+    #[error("Invalid next-hop: {0}")]
+    InvalidNexthop(&'static str),
 }

routing/src/rib/nexthop.rs

@@ -22,7 +22,7 @@ use std::cell::RefCell;
 use std::rc::{Rc, Weak};
 #[cfg(test)]
 use std::str::FromStr;
-use tracing::{debug, error};
+use tracing::{debug, error, warn};
 
 use tracectl::trace_target;
 trace_target!("next-hops", LevelFilter::WARN, &["routing-full"]);
@@ -192,7 +192,7 @@ impl Nhop {
     fn resolves_with(&self, checked: &Nhop) -> bool {
         // resolve to oneself is forbidden
         if self.key == checked.key {
-            error!("Loop detected!: {} resolves with {}", checked.key, self.key);
+            error!("Loop detected for next-hop {}!", self.key);
             return true;
         }
         // resolvers should not refer back to the checked next-hop
@@ -203,26 +203,63 @@ impl Nhop {
             .any(|res| res.resolves_with(checked))
     }
 
-    /// Resolve a next-hop with a VRF, non-recursively, assuming that its resolvers are resolved already
-    pub fn lazy_resolve(&self, vrf: &Vrf) {
-        if self.key.ifindex.is_some() || self.key.fwaction == FwAction::Drop {
-            return;
+    /// Tell if a next-hop requires resolution
+    pub(super) fn must_be_resolved(&self) -> bool {
+        self.key.ifindex.is_none() && self.key.fwaction != FwAction::Drop
+    }
+
+    /// Tell if a next-hop requires resolution but could not be resolved
+    #[must_use]
+    pub(crate) fn is_unresolved(&self) -> bool {
+        self.must_be_resolved()
+            && self
+                .resolvers
+                .try_borrow()
+                .is_ok_and(|resolvers| resolvers.is_empty())
+    }
+
+    /// Tell if a next hop requires resolution and if that's possible. If so,
+    /// return the address to resolve
+    fn needs_resolution(&self) -> Option<IpAddr> {
+        if !self.must_be_resolved() {
+            debug!("Nhop {self} requires no resolution");
+            return None;
         }
         let Some(a) = self.key.address else {
-            error!("Got forwarding nexthop with neither address nor ifindex!: {self}");
+            error!("Found nexthop with neither address nor ifindex!: {self}");
+            return None;
+        };
+        Some(a)
+    }
+
+    /// Resolve a next-hop with a VRF, non-recursively; i.e. without caring whether
+    /// the next-hops that a next-hop resolve to are resolved
+    pub fn lazy_resolve(&self, vrf: &Vrf) {
+        let name = &vrf.name;
+        let Some(target) = self.needs_resolution() else {
             return;
         };
-        debug!("Resolving {a} with vrf '{}'...", vrf.name);
-        let (prefix, route) = vrf.lpm(a);
-        debug!("Address {a} resolves with route to {prefix}");
+        let (prefix, route) = vrf.lpm(target);
+        debug!("Address {target} resolves with route to {prefix} in vrf {name}");
+
+        // collect resolvers
         let mut resolvers = Vec::with_capacity(route.s_nhops.len());
-        for nh in &route.s_nhops {
-            if !nh.rc.resolves_with(self) {
-                debug!(" -> {}", nh.rc);
-                resolvers.push(Rc::downgrade(&nh.rc));
+        for nhop in &route.s_nhops {
+            let resolver = &nhop.rc;
+            if !resolver.resolves_with(self) {
+                debug!(" {target} -> {resolver}");
+                resolvers.push(Rc::downgrade(resolver));
             }
         }
-        // update resolvers
+        // warn if we got no valid resolver for the next-hop
+        if resolvers.is_empty() {
+            warn!(
+                "Cannot resolve address {target} with vrf {name}: {} route to {prefix} has no usable next-hop",
+                route.origin
+            );
+        }
+
+        // update resolvers (N.B: resolvers may be empty)
         self.resolvers.replace(resolvers);
     }
 
@@ -361,24 +398,35 @@ impl NhopStore {
     }
 
     /// Rebuild the instructions for each next-hop
-    pub fn resolve_nhop_instructions(&self, rstore: &RmacStore) {
+    pub fn rebuild_nhop_instructions(&self, rstore: &RmacStore) {
         for nhop in self.iter() {
             nhop.build_nhop_instructions(rstore);
         }
     }
 
-    /// Lazily resolve all next-hops in this store.
+    /// Flush all resolution state of all next-hops
+    fn flush_resolvers(&self) {
+        for nhop in self.iter() {
+            nhop.resolvers.borrow_mut().clear();
+        }
+    }
+
+    /// Flush all resolution state and lazily re-resolve all next-hops.
     pub fn lazy_resolve_all(&self, vrf: &Vrf) {
+        self.flush_resolvers();
         self.iter().for_each(|nhop| nhop.lazy_resolve(vrf));
     }
 
     /// Rebuild the fibgroup for every next-hop. This method visits every next-hop and
     /// rebuilds its fibgroup. It returns a vector with only those next-hops whose
     /// fibgroup changed. We return a Vector and not an iterator to force the rebuild
-    /// of the fibgroups.
-    pub fn rebuild_fibgroups(&self, rstore: &RmacStore) -> Vec<&Rc<Nhop>> {
+    /// of the fibgroups. N.B. we hand out weak references and not owning ones so as to
+    /// not alter the strong count of the next-hops, which tells how many routes use them
+    /// and determines if a next-hop can be removed (see `NhopStore::del_nhop()`).
+    pub fn rebuild_fibgroups(&self, rstore: &RmacStore) -> Vec<Weak<Nhop>> {
         self.iter()
             .filter(|nhop| nhop.set_fibgroup(rstore))
+            .map(Rc::downgrade)
             .collect()
     }
 }
@@ -392,15 +440,6 @@ impl NhopStore {
         self.0.contains(&nh)
     }
 
-    /// Flush all resolution state of all next-hops
-    pub(crate) fn flush_resolvers(&self) {
-        for nhop in self.iter() {
-            nhop.resolvers.borrow_mut().clear();
-            nhop.instructions.borrow_mut().clear();
-            nhop.fibgroup.take();
-        }
-    }
-
     /// Resolve a next-hop by address. If no next-hop
     /// exists for that address, returns None. Otherwise, it returns the
     /// result of `quick_resolve()` on the next-hop found.
@@ -418,6 +457,8 @@ impl NhopStore {
 
 #[cfg(test)]
 mod tests {
+    use crate::evpn::RmacStore;
+    use crate::fib::fibobjects::{FibEntry, PktInstruction};
     use crate::rib::nexthop::*;
     use std::rc::Rc;
     use tracing_test::traced_test;
@@ -814,6 +855,67 @@ mod tests {
         println!("{res:#?}");
     }
 
+    #[cfg_attr(not(emulated), traced_test)]
+    #[test]
+    fn test_must_be_resolved() {
+        // a drop next-hop requires no resolution
+        let nhop = Nhop::from_key(&NhopKey::with_drop());
+        assert!(!nhop.must_be_resolved());
+
+        // a next-hop with only address requires resolution
+        let nhop = Nhop::from_key(&NhopKey::from_address("7.0.0.1"));
+        assert!(nhop.must_be_resolved());
+
+        // a next-hop with address and ifindex does not require resolution
+        let nhop = Nhop::from_key(&NhopKey::with_addr_ifindex("7.0.0.1", 13));
+        assert!(!nhop.must_be_resolved());
+    }
+
+    #[cfg_attr(not(emulated), traced_test)]
+    #[test]
+    /// An unresolved next-hop (requiring resolution) produces a drop `FibGroup`
+    fn test_unresolved_nhop_drops_traffic() {
+        let store = build_test_nhop_store_with_drop_nexthop();
+
+        // 8.0.0.1 resolves to nothing */
+        let key = NhopKey::from_address("8.0.0.1");
+        let nhop = store.get_nhop(&key).expect("Next-hop should be there");
+        assert!(nhop.must_be_resolved());
+        assert!(nhop.is_unresolved());
+
+        let fibgroup = nhop.build_nhop_fibgroup();
+        assert_eq!(fibgroup.len(), 1, "Should get a single fib entry");
+        assert_eq!(
+            fibgroup.entries()[0],
+            FibEntry::drop_fibentry(),
+            "Traffic to an unresolved next-hop must be dropped"
+        );
+    }
+
+    #[cfg_attr(not(emulated), traced_test)]
+    #[test]
+    fn test_unresolved_nhop_is_fib_ignored() {
+        let mut store = NhopStore::new();
+
+        // 7.0.0.1 resolves over interface 1 and 9.9.9.9 (unresolved) */
+        let i1 = store.add_nhop(&NhopKey::with_ifindex(1));
+        let unresolved = store.add_nhop(&NhopKey::from_address("9.9.9.9"));
+        let key = NhopKey::from_address("7.0.0.1");
+        let nhop = store.add_nhop(&key);
+        nhop.add_resolver(&i1).add_resolver(&unresolved);
+        store.rebuild_nhop_instructions(&RmacStore::new());
+        store.dump();
+
+        // Fibgroup gets only one entry over interface
+        let fibgroup = nhop.build_nhop_fibgroup();
+        assert_eq!(fibgroup.len(), 1, "Only the usable path should be there");
+        let entry = &fibgroup.entries()[0];
+        assert!(matches!(
+            entry.iter().next().expect("Should have an instruction"),
+            PktInstruction::Egress(_)
+        ));
+    }
+
     #[cfg_attr(not(emulated), traced_test)]
     #[test]
     fn test_loop_prevention() {

routing/src/rib/rib2fib.rs

@@ -77,9 +77,7 @@ impl Nhop {
     }
 
     //////////////////////////////////////////////////////////////////////
-    /// Given a next-hop, build its packet instructions and resolve them
-    /// In this implementation, the next-hop owns the packet instructions
-    /// So, they are not shared and have to be resolved per next-hop.
+    /// Given a next-hop, build its packet instructions and attach them to it
     //////////////////////////////////////////////////////////////////////
     pub(crate) fn build_nhop_instructions(&self, rstore: &RmacStore) {
         // build new instruction vector for the next-hop
@@ -98,12 +96,19 @@ impl Nhop {
         let instructions = self.instructions.borrow().clone();
         entry.extend_from_slice(&instructions);
 
-        // check the instructions of the resolving next-hops
+        // check the instructions of the resolving next-hops, if any
         let Ok(resolvers) = self.resolvers.try_borrow() else {
             warn!("Warning, try-borrow failed!!!");
             return;
         };
         if resolvers.is_empty() {
+            if self.must_be_resolved() {
+                // Nhop has no resolver. This should only happen if: 1) we forgot to
+                // resolve it (BUG) or 2) we attempted resolution, but stopped it because
+                // we detected a loop.
+                warn!("Next-hop {self} is unresolved: will not use it");
+                return;
+            }
             entry.squash(); /* squash entry before committing it to the group */
             fibgroup.add(entry); /* add fib entry to group */
         } else {
@@ -114,12 +119,19 @@ impl Nhop {
     }
 
     //////////////////////////////////////////////////////////////////
-    /// Build a [`FibGroup`] for an [`Nhop`]
+    /// Build a [`FibGroup`] for an [`Nhop`]. If the next-hop cannot be
+    /// resolved and the fibgroup would be empty, artificially inject
+    /// an entry with action DROP so that packets hitting the route
+    /// don't get misrouted.
     //////////////////////////////////////////////////////////////////////
     pub(crate) fn build_nhop_fibgroup(&self) -> FibGroup {
-        let mut out = FibGroup::new();
-        self.build_nhop_fibgroup_rec(&mut out, FibEntry::new());
-        out
+        let mut fibgroup = FibGroup::new();
+        self.build_nhop_fibgroup_rec(&mut fibgroup, FibEntry::new());
+        if fibgroup.is_empty() {
+            warn!("Next-hop {self} has no usable path: will add DROP fibgroup");
+            fibgroup.add(FibEntry::drop_fibentry());
+        }
+        fibgroup
     }
 
     //////////////////////////////////////////////////////////////////////