Rename static route local_pref to rib_priority (#6693)

This is the omicron half of maghemite#359. This renames the `local_pref` field of static routes to `rib_priority` to avoid overloading the name of a standardized and well-known BGP attribute (Local Preference). This change shrinks the size of the field from u32 -> u8 and unwraps the Option<T> before shipping routes to mgd, aligning with the updated API. This also brings omicron back into sync with maghemite. Fixes: #6711 Signed-off-by: Trey Aspelund <[email protected]> --------- Signed-off-by: Trey Aspelund <[email protected]> Co-authored-by: Levon Tarver <[email protected]>
oxidecomputer · Oct 7, 2024 · f3ab338 · f3ab338
1 parent f0ec01f
commit f3ab338
Show file tree

Hide file tree

Showing 30 changed files with 193 additions and 170 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -420,10 +420,8 @@ macaddr = { version = "1.0.1", features = ["serde_std"] }
 maplit = "1.0.2"
 mockall = "0.13"
 newtype_derive = "0.1.6"
-# mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "9e0fe45ca3862176dc31ad8cc83f605f8a7e1a42" }
-# ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "9e0fe45ca3862176dc31ad8cc83f605f8a7e1a42" }
-mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", branch = "hyper-v1-no-merge" }
-ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", branch = "hyper-v1-no-merge" }
+mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "056283eb02b6887fbf27f66a215662520f7c159c" }
+ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "056283eb02b6887fbf27f66a215662520f7c159c" }
 multimap = "0.10.0"
 nexus-auth = { path = "nexus/auth" }
 nexus-client = { path = "clients/nexus-client" }

diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs
@@ -2607,8 +2607,8 @@ pub struct SwitchPortRouteConfig {
  /// over an 802.1Q tagged L2 segment.
  pub vlan_id: Option<u16>,
 
- /// Local preference indicating priority within and across protocols.
- pub local_pref: Option<u32>,
+ /// RIB Priority indicating priority within and across protocols.
+ pub rib_priority: Option<u8>,
 }
 
 /*

diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs
@@ -306,9 +306,9 @@ pub struct RouteConfig {
  /// The VLAN id associated with this route.
  #[serde(default)]
  pub vlan_id: Option<u16>,
- /// The local preference associated with this route.
+ /// The RIB priority (i.e. Admin Distance) associated with this route.
  #[serde(default)]
- pub local_pref: Option<u32>,
+ pub rib_priority: Option<u8>,
 }
 
 #[derive(

diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs
@@ -188,7 +188,7 @@ table! {
  dst -> Inet,
  gw -> Inet,
  vid -> Nullable<Int4>,
- local_pref -> Nullable<Int8>,
+ local_pref -> Nullable<Int2>,
  }
 }
 

diff --git a/nexus/db-model/src/switch_port.rs b/nexus/db-model/src/switch_port.rs
@@ -559,7 +559,8 @@ pub struct SwitchPortRouteConfig {
  pub dst: IpNetwork,
  pub gw: IpNetwork,
  pub vid: Option<SqlU16>,
- pub local_pref: Option<SqlU32>,
+ #[diesel(column_name = local_pref)]
+ pub rib_priority: Option<SqlU8>,
 }
 
 impl SwitchPortRouteConfig {
@@ -569,9 +570,9 @@ impl SwitchPortRouteConfig {
  dst: IpNetwork,
  gw: IpNetwork,
  vid: Option<SqlU16>,
- local_pref: Option<SqlU32>,
+ rib_priority: Option<SqlU8>,
  ) -> Self {
- Self { port_settings_id, interface_name, dst, gw, vid, local_pref }
+ Self { port_settings_id, interface_name, dst, gw, vid, rib_priority }
  }
 }
 
@@ -583,7 +584,7 @@ impl Into<external::SwitchPortRouteConfig> for SwitchPortRouteConfig {
  dst: self.dst.into(),
  gw: self.gw.into(),
  vlan_id: self.vid.map(Into::into),
- local_pref: self.local_pref.map(Into::into),
+ rib_priority: self.rib_priority.map(Into::into),
  }
  }
 }

diff --git a/nexus/db-queries/src/db/datastore/switch_port.rs b/nexus/db-queries/src/db/datastore/switch_port.rs
@@ -1237,7 +1237,7 @@ async fn do_switch_port_settings_create(
  route.dst.into(),
  route.gw.into(),
  route.vid.map(Into::into),
- route.local_pref.map(Into::into),
+ route.rib_priority.map(Into::into),
  ));
  }
  }

diff --git a/nexus/src/app/background/tasks/sync_switch_configuration.rs b/nexus/src/app/background/tasks/sync_switch_configuration.rs
@@ -71,6 +71,10 @@ const PHY0: &str = "phy0";
 // before breaking to check for shutdown conditions.
 const BGP_SESSION_RESOLUTION: u64 = 100;
 
+// This is the default RIB Priority used for static routes. This mirrors
+// the const defined in maghemite in rdb/src/lib.rs.
+const DEFAULT_RIB_PRIORITY_STATIC: u8 = 1;
+
 pub struct SwitchPortSettingsManager {
  datastore: Arc<DataStore>,
  resolver: Resolver,
@@ -978,7 +982,7 @@ impl BackgroundTask for SwitchPortSettingsManager {
  destination: r.dst.into(),
  nexthop: r.gw.ip(),
  vlan_id: r.vid.map(|x| x.0),
- local_pref: r.local_pref.map(|x| x.0),
+ rib_priority: r.rib_priority.map(|x| x.0),
  })
  .collect(),
  switch: *location,
@@ -1497,8 +1501,7 @@ fn build_sled_agent_clients(
  sled_agent_clients
 }
 
-type SwitchStaticRoutes =
- HashSet<(Ipv4Addr, Prefix4, Option<u16>, Option<u32>)>;
+type SwitchStaticRoutes = HashSet<(Ipv4Addr, Prefix4, Option<u16>, Option<u8>)>;
 
 fn static_routes_to_del(
  current_static_routes: HashMap<SwitchLocation, SwitchStaticRoutes>,
@@ -1514,11 +1517,12 @@ fn static_routes_to_del(
  // if it's on the switch but not desired (in our db), it should be removed
  let stale_routes = routes_on_switch
  .difference(routes_wanted)
- .map(|(nexthop, prefix, vlan_id, local_pref)| StaticRoute4 {
+ .map(|(nexthop, prefix, vlan_id, rib_priority)| StaticRoute4 {
  nexthop: *nexthop,
  prefix: *prefix,
  vlan_id: *vlan_id,
- local_pref: *local_pref,
+ rib_priority: rib_priority
+ .unwrap_or(DEFAULT_RIB_PRIORITY_STATIC),
  })
  .collect::<Vec<StaticRoute4>>();
 
@@ -1532,11 +1536,12 @@ fn static_routes_to_del(
  // if no desired routes are present, all routes on this switch should be deleted
  let stale_routes = routes_on_switch
  .iter()
- .map(|(nexthop, prefix, vlan_id, local_pref)| StaticRoute4 {
+ .map(|(nexthop, prefix, vlan_id, rib_priority)| StaticRoute4 {
  nexthop: *nexthop,
  prefix: *prefix,
  vlan_id: *vlan_id,
- local_pref: *local_pref,
+ rib_priority: rib_priority
+ .unwrap_or(DEFAULT_RIB_PRIORITY_STATIC),
  })
  .collect::<Vec<StaticRoute4>>();
 
@@ -1583,11 +1588,12 @@ fn static_routes_to_add(
  };
  let missing_routes = routes_wanted
  .difference(routes_on_switch)
- .map(|(nexthop, prefix, vlan_id, local_pref)| StaticRoute4 {
+ .map(|(nexthop, prefix, vlan_id, rib_priority)| StaticRoute4 {
  nexthop: *nexthop,
  prefix: *prefix,
  vlan_id: *vlan_id,
- local_pref: *local_pref,
+ rib_priority: rib_priority
+ .unwrap_or(DEFAULT_RIB_PRIORITY_STATIC),
  })
  .collect::<Vec<StaticRoute4>>();
 
@@ -1640,7 +1646,7 @@ fn static_routes_in_db(
  nexthop,
  prefix,
  route.vid.map(|x| x.0),
- route.local_pref.map(|x| x.0),
+ route.rib_priority.map(|x| x.0),
  ));
  }
 
@@ -1819,46 +1825,49 @@ async fn static_routes_on_switch<'a>(
  let mut routes_on_switch = HashMap::new();
 
  for (location, client) in mgd_clients {
- let static_routes: SwitchStaticRoutes = match client
- .static_list_v4_routes()
- .await
- {
- Ok(routes) => {
- let mut flattened = HashSet::new();
- for (destination, paths) in routes.iter() {
- let Ok(dst) = destination.parse() else {
- error!(
- log,
- "failed to parse static route destination: \
+ let static_routes: SwitchStaticRoutes =
+ match client.static_list_v4_routes().await {
+ Ok(routes) => {
+ let mut flattened = HashSet::new();
+ for (destination, paths) in routes.iter() {
+ let Ok(dst) = destination.parse() else {
+ error!(
+ log,
+ "failed to parse static route destination: \
  {destination}"
- );
- continue;
- };
- for p in paths.iter() {
- let nh = match p.nexthop {
- IpAddr::V4(addr) => addr,
- IpAddr::V6(addr) => {
- error!(
- log,
- "ipv6 nexthops not supported: {addr}"
- );
- continue;
- }
+ );
+ continue;
  };
- flattened.insert((nh, dst, p.vlan_id, p.local_pref));
+ for p in paths.iter() {
+ let nh = match p.nexthop {
+ IpAddr::V4(addr) => addr,
+ IpAddr::V6(addr) => {
+ error!(
+ log,
+ "ipv6 nexthops not supported: {addr}"
+ );
+ continue;
+ }
+ };
+ flattened.insert((
+ nh,
+ dst,
+ p.vlan_id,
+ Some(p.rib_priority),
+ ));
+ }
  }
+ flattened
  }
- flattened
- }
- Err(_) => {
- error!(
- &log,
- "unable to retrieve routes from switch";
- "switch_location" => ?location,
- );
- continue;
- }
- };
+ Err(_) => {
+ error!(
+ &log,
+ "unable to retrieve routes from switch";
+ "switch_location" => ?location,
+ );
+ continue;
+ }
+ };
  routes_on_switch.insert(*location, static_routes);
  }
  routes_on_switch

diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs
@@ -592,7 +592,7 @@ impl super::Nexus {
  dst: r.destination,
  gw: r.nexthop,
  vid: r.vlan_id,
- local_pref: r.local_pref,
+ rib_priority: r.rib_priority,
  })
  .collect();
 

diff --git a/nexus/tests/integration_tests/switch_port.rs b/nexus/tests/integration_tests/switch_port.rs
@@ -148,7 +148,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) {
  dst: "1.2.3.0/24".parse().unwrap(),
  gw: "1.2.3.4".parse().unwrap(),
  vid: None,
- local_pref: None,
+ rib_priority: None,
  }],
  },
  );

diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs
@@ -1791,7 +1791,7 @@ pub struct Route {
 
  /// Local preference for route. Higher preference indictes precedence
  /// within and across protocols.
- pub local_pref: Option<u32>,
+ pub rib_priority: Option<u8>,
 }
 
 /// Select a BGP config by a name or id.

diff --git a/openapi/bootstrap-agent.json b/openapi/bootstrap-agent.json
@@ -1257,19 +1257,19 @@
  }
  ]
  },
- "local_pref": {
- "nullable": true,
- "description": "The local preference associated with this route.",
- "default": null,
- "type": "integer",
- "format": "uint32",
- "minimum": 0
- },
  "nexthop": {
  "description": "The nexthop/gateway address.",
  "type": "string",
  "format": "ip"
  },
+ "rib_priority": {
+ "nullable": true,
+ "description": "The RIB priority (i.e. Admin Distance) associated with this route.",
+ "default": null,
+ "type": "integer",
+ "format": "uint8",
+ "minimum": 0
+ },
  "vlan_id": {
  "nullable": true,
  "description": "The VLAN id associated with this route.",

diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json
@@ -4823,19 +4823,19 @@
  }
  ]
  },
- "local_pref": {
- "nullable": true,
- "description": "The local preference associated with this route.",
- "default": null,
- "type": "integer",
- "format": "uint32",
- "minimum": 0
- },
  "nexthop": {
  "description": "The nexthop/gateway address.",
  "type": "string",
  "format": "ip"
  },
+ "rib_priority": {
+ "nullable": true,
+ "description": "The RIB priority (i.e. Admin Distance) associated with this route.",
+ "default": null,
+ "type": "integer",
+ "format": "uint8",
+ "minimum": 0
+ },
  "vlan_id": {
  "nullable": true,
  "description": "The VLAN id associated with this route.",

diff --git a/openapi/nexus.json b/openapi/nexus.json
@@ -18431,11 +18431,11 @@
  "type": "string",
  "format": "ip"
  },
- "local_pref": {
+ "rib_priority": {
  "nullable": true,
  "description": "Local preference for route. Higher preference indictes precedence within and across protocols.",
  "type": "integer",
- "format": "uint32",
+ "format": "uint8",
  "minimum": 0
  },
  "vid": {
@@ -20496,18 +20496,18 @@
  "description": "The interface name this route configuration is assigned to.",
  "type": "string"
  },
- "local_pref": {
- "nullable": true,
- "description": "Local preference indicating priority within and across protocols.",
- "type": "integer",
- "format": "uint32",
- "minimum": 0
- },
  "port_settings_id": {
  "description": "The port settings object this route configuration belongs to.",
  "type": "string",
  "format": "uuid"
  },
+ "rib_priority": {
+ "nullable": true,
+ "description": "RIB Priority indicating priority within and across protocols.",
+ "type": "integer",
+ "format": "uint8",
+ "minimum": 0
+ },
  "vlan_id": {
  "nullable": true,
  "description": "The VLAN identifier for the route. Use this if the gateway is reachable over an 802.1Q tagged L2 segment.",