Don't add routes or advertise tunnel exit for down links (#514)

Author: rcgoodfellow

mg-lower/src/ddm.rs

@@ -4,48 +4,11 @@
 
 use ddm_admin_client::types::TunnelOrigin;
 use ddm_admin_client::Client;
-use oxnet::{IpNet, Ipv6Net};
-use rdb::db::Rib;
-use rdb::{Prefix, Prefix4, Prefix6, DEFAULT_ROUTE_PRIORITY};
+use oxnet::Ipv6Net;
 use slog::{error, info, Logger};
-use std::{collections::HashSet, net::Ipv6Addr, sync::Arc};
+use std::{net::Ipv6Addr, sync::Arc};
 
-use crate::dendrite::RouteHash;
-
-const BOUNDARY_SERVICES_VNI: u32 = 99;
-
-pub(crate) fn update_tunnel_endpoints(
-    tep: Ipv6Addr, // tunnel endpoint address
-    client: &Client,
-    routes: &Rib,
-    rt: Arc<tokio::runtime::Handle>,
-    log: &Logger,
-) {
-    let current: HashSet<TunnelOrigin> = match rt
-        .block_on(async { client.get_originated_tunnel_endpoints().await })
-        .map(|x| x.into_inner())
-    {
-        Ok(x) => x,
-        Err(e) => {
-            error!(log, "get originated tunnel endpoints: {e}");
-            return;
-        }
-    }
-    .into_iter()
-    .collect();
-
-    let target: HashSet<TunnelOrigin> = routes
-        .iter()
-        .filter(|(_prefix, path)| !path.is_empty())
-        .map(|(prefix, _path)| route_to_tunnel(tep, prefix))
-        .collect();
-
-    let to_add = target.difference(&current);
-    let to_remove = current.difference(&target);
-
-    add_tunnel_endpoints(tep, client, to_add.into_iter(), &rt, log);
-    remove_tunnel_endpoints(client, to_remove.into_iter(), &rt, log);
-}
+pub(crate) const BOUNDARY_SERVICES_VNI: u32 = 99;
 
 fn ensure_tep_underlay_origin(
     client: &Client,
@@ -79,60 +42,7 @@ fn ensure_tep_underlay_origin(
     };
 }
 
-fn route_to_tunnel(tep: Ipv6Addr, prefix: &Prefix) -> TunnelOrigin {
-    match prefix {
-        Prefix::V4(p) => {
-            TunnelOrigin {
-                overlay_prefix: oxnet::Ipv4Net::new(p.value, p.length)
-                    .unwrap()
-                    .into(),
-                boundary_addr: tep,
-                vni: BOUNDARY_SERVICES_VNI,     //TODO?
-                metric: DEFAULT_ROUTE_PRIORITY, //TODO
-            }
-        }
-        Prefix::V6(p) => {
-            TunnelOrigin {
-                overlay_prefix: oxnet::Ipv6Net::new(p.value, p.length)
-                    .unwrap()
-                    .into(),
-                boundary_addr: tep,
-                vni: BOUNDARY_SERVICES_VNI,     //TODO?
-                metric: DEFAULT_ROUTE_PRIORITY, //TODO
-            }
-        }
-    }
-}
-
-pub(crate) fn add_tunnel_routes(
-    tep: Ipv6Addr, // tunnel endpoint address
-    client: &Client,
-    routes: &HashSet<RouteHash>,
-    rt: Arc<tokio::runtime::Handle>,
-    log: &Logger,
-) {
-    let teps: Vec<TunnelOrigin> = routes
-        .iter()
-        .map(|rt| {
-            let pfx = match rt.cidr {
-                IpNet::V4(p) => Prefix4 {
-                    value: p.prefix(),
-                    length: p.width(),
-                }
-                .into(),
-                IpNet::V6(p) => Prefix6 {
-                    value: p.prefix(),
-                    length: p.width(),
-                }
-                .into(),
-            };
-            route_to_tunnel(tep, &pfx)
-        })
-        .collect();
-    add_tunnel_endpoints(tep, client, teps.iter(), &rt, log)
-}
-
-pub(crate) fn add_tunnel_endpoints<'a, I: Iterator<Item = &'a TunnelOrigin>>(
+pub(crate) fn add_tunnel_routes<'a, I: Iterator<Item = &'a TunnelOrigin>>(
     tep: Ipv6Addr, // tunnel endpoint address
     client: &Client,
     routes: I,
@@ -151,38 +61,7 @@ pub(crate) fn add_tunnel_endpoints<'a, I: Iterator<Item = &'a TunnelOrigin>>(
     }
 }
 
-pub(crate) fn remove_tunnel_routes(
-    tep: Ipv6Addr, // tunnel endpoint address
-    client: &Client,
-    routes: &HashSet<RouteHash>,
-    rt: Arc<tokio::runtime::Handle>,
-    log: &Logger,
-) {
-    let teps: Vec<TunnelOrigin> = routes
-        .iter()
-        .map(|rt| {
-            let pfx = match rt.cidr {
-                IpNet::V4(p) => Prefix4 {
-                    value: p.prefix(),
-                    length: p.width(),
-                }
-                .into(),
-                IpNet::V6(p) => Prefix6 {
-                    value: p.prefix(),
-                    length: p.width(),
-                }
-                .into(),
-            };
-            route_to_tunnel(tep, &pfx)
-        })
-        .collect();
-    remove_tunnel_endpoints(client, teps.iter(), &rt, log)
-}
-
-pub(crate) fn remove_tunnel_endpoints<
-    'a,
-    I: Iterator<Item = &'a TunnelOrigin>,
->(
+pub(crate) fn remove_tunnel_routes<'a, I: Iterator<Item = &'a TunnelOrigin>>(
     client: &Client,
     routes: I,
     rt: &Arc<tokio::runtime::Handle>,

mg-lower/src/dendrite.rs

@@ -374,31 +374,6 @@ pub(crate) fn get_routes_for_prefix(
                 if r.tag != MG_LOWER_TAG {
                     continue;
                 }
-                match link_is_up(dpd, &r.port_id, &r.link_id, &rt) {
-                    Err(e) => {
-                        error!(
-                            log,
-                            "nexthop: {} failed to get link state for {:?}/{:?}: {e}",
-                            r.tgt_ip,
-                            r.port_id,
-                            r.link_id
-                        );
-                        continue;
-                    }
-                    Ok(false) => {
-                        warn!(
-                            log,
-                            "nexthop: {} link {:?}/{:?} is not up, \
-                            not installing route for {:?}",
-                            r.tgt_ip,
-                            r.port_id,
-                            r.link_id,
-                            prefix,
-                        );
-                        continue;
-                    }
-                    Ok(true) => {}
-                }
                 match RouteHash::new(
                     cidr.into(),
                     r.port_id.clone(),

mg-lower/src/error.rs

@@ -7,6 +7,9 @@ pub enum Error {
     #[error("dpd error {0}")]
     Dpd(#[from] dpd_client::Error<dpd_client::types::Error>),
 
+    #[error("ddm error {0}")]
+    Ddm(#[from] ddm_admin_client::Error<ddm_admin_client::types::Error>),
+
     #[error("tfport error {0}")]
     Tfport(String),
 

mg-lower/src/lib.rs

@@ -12,15 +12,16 @@ use crate::dendrite::{
 use crate::error::Error;
 use ddm::{
     add_tunnel_routes, new_ddm_client, remove_tunnel_routes,
-    update_tunnel_endpoints,
+    BOUNDARY_SERVICES_VNI,
 };
+use ddm_admin_client::types::TunnelOrigin;
 use ddm_admin_client::Client as DdmClient;
-use dendrite::ensure_tep_addr;
+use dendrite::{ensure_tep_addr, link_is_up};
 use dpd_client::Client as DpdClient;
 use mg_common::stats::MgLowerStats as Stats;
 use rdb::db::Rib;
-use rdb::{Db, Prefix, PrefixChangeNotification};
-use slog::{error, info, Logger};
+use rdb::{Db, Prefix, PrefixChangeNotification, DEFAULT_ROUTE_PRIORITY};
+use slog::{error, info, warn, Logger};
 use std::collections::HashSet;
 use std::net::Ipv6Addr;
 use std::sync::mpsc::{channel, RecvTimeoutError};
@@ -129,13 +130,10 @@ fn full_sync(
     // Make sure our tunnel endpoint address is on the switch ASIC
     ensure_tep_addr(tep, dpd, rt.clone(), log);
 
-    // Announce tunnel endpoints via ddm
-    update_tunnel_endpoints(tep, ddm, &rib, rt.clone(), log);
-
     // Compute the bestpath for each prefix and synchronize the ASIC routing
     // tables with the chosen paths.
     for (prefix, _paths) in rib.iter() {
-        sync_prefix(tep, db.loc_rib(), prefix, dpd, ddm, log, &rt)?;
+        sync_prefix(tep, &db.loc_rib(), prefix, dpd, ddm, log, &rt)?;
     }
 
     Ok(())
@@ -152,23 +150,31 @@ fn handle_change(
     rt: Arc<tokio::runtime::Handle>,
 ) -> Result<(), Error> {
     for prefix in notification.changed.iter() {
-        sync_prefix(tep, db.loc_rib(), prefix, dpd, ddm, log, &rt)?;
+        sync_prefix(tep, &db.loc_rib(), prefix, dpd, ddm, log, &rt)?;
     }
 
     Ok(())
 }
 
 fn sync_prefix(
     tep: Ipv6Addr,
-    rib_loc: Rib,
+    rib_loc: &Rib,
     prefix: &Prefix,
     dpd: &DpdClient,
     ddm: &DdmClient,
     log: &Logger,
     rt: &Arc<tokio::runtime::Handle>,
 ) -> Result<(), Error> {
     // The current routes that are on the ASIC.
-    let current = get_routes_for_prefix(dpd, prefix, rt.clone(), log.clone())?;
+    let dpd_current =
+        get_routes_for_prefix(dpd, prefix, rt.clone(), log.clone())?;
+
+    // The current tunnel routes in ddm
+    let ddm_current = rt
+        .block_on(async { ddm.get_originated_tunnel_endpoints().await })?
+        .into_inner()
+        .into_iter()
+        .collect::<HashSet<_>>();
 
     // The best routes in the RIB
     let mut best: HashSet<RouteHash> = HashSet::new();
@@ -178,18 +184,74 @@ fn sync_prefix(
         }
     }
 
+    // Remove paths for which the link is down.
+    best.retain(|x| match link_is_up(dpd, &x.port_id, &x.link_id, rt) {
+        Err(e) => {
+            error!(
+                log,
+                "sync_prefix: failed to get link state for {:?}/{:?} \
+                    not installing route {:?} -> {:?}: {e}",
+                x.port_id,
+                x.link_id,
+                x.cidr,
+                x.nexthop,
+            );
+            false
+        }
+        Ok(false) => {
+            warn!(
+                log,
+                "sync_prefix: link {:?}/{:?} is not up, \
+                    not installing route {:?} -> {:?}",
+                x.port_id,
+                x.link_id,
+                x.cidr,
+                x.nexthop,
+            );
+            false
+        }
+        Ok(true) => true,
+    });
+
+    //
+    // Update the ASIC routing tables
+    //
+
     // Routes that are in the best set but not on the asic should be added.
-    let add: HashSet<RouteHash> = best.difference(&current).cloned().collect();
+    let add: HashSet<RouteHash> =
+        best.difference(&dpd_current).cloned().collect();
 
     // Routes that are on the asic but not in the best set should be removed.
-    let del: HashSet<RouteHash> = current.difference(&best).cloned().collect();
+    let del: HashSet<RouteHash> =
+        dpd_current.difference(&best).cloned().collect();
 
-    // Update DDM tunnel routing
-    add_tunnel_routes(tep, ddm, &add, rt.clone(), log);
-    remove_tunnel_routes(tep, ddm, &del, rt.clone(), log);
-
-    // Update the ASIC routing tables
     update_dendrite(add.iter(), del.iter(), dpd, rt.clone(), log)?;
 
+    //
+    // Update the ddm tunnel advertisements
+    //
+
+    let best_tunnel = best
+        .clone()
+        .into_iter()
+        .map(|x| TunnelOrigin {
+            boundary_addr: tep,
+            overlay_prefix: x.cidr,
+            metric: DEFAULT_ROUTE_PRIORITY,
+            vni: BOUNDARY_SERVICES_VNI,
+        })
+        .collect::<HashSet<_>>();
+
+    // Routes that are in the best set but not in ddm should be added.
+    let add: HashSet<TunnelOrigin> =
+        best_tunnel.difference(&ddm_current).cloned().collect();
+
+    // Routes that are in ddm but not in the best set should be removed.
+    let del: HashSet<TunnelOrigin> =
+        ddm_current.difference(&best_tunnel).cloned().collect();
+
+    add_tunnel_routes(tep, ddm, add.iter(), rt, log);
+    remove_tunnel_routes(ddm, del.iter(), rt, log);
+
     Ok(())
 }