fix(admin-cli): delete machine interfaces by MAC address (#3046)

crates/admin-cli/src/machine_interfaces/delete/args.rs

@@ -16,7 +16,8 @@
  */
 
 use carbide_uuid::machine::MachineInterfaceId;
-use clap::Parser;
+use clap::{ArgGroup, Parser};
+use mac_address::MacAddress;
 
 #[derive(Parser, Debug)]
 #[command(after_long_help = "\
@@ -25,8 +26,23 @@ EXAMPLES:
 Delete a machine interface by ID (redeploy kea afterward):
     $ nico-admin-cli machine-interfaces delete 12345678-1234-5678-90ab-cdef01234567
 
+Delete a leftover interface when you only have the BMC MAC (e.g. a replacement host
+whose ingestion is blocked by a stale interface record):
+    $ nico-admin-cli machine-interfaces delete --mac-address 00:11:22:33:44:55
+
 ")]
+#[clap(group(
+    ArgGroup::new("interface_selector")
+        .required(true)
+        .args(["interface_id", "mac_address"]),
+))]
 pub struct Args {
     #[clap(help = "The interface ID to delete.")]
-    pub interface_id: MachineInterfaceId,
+    pub interface_id: Option<MachineInterfaceId>,
+
+    #[clap(
+        long,
+        help = "Delete every interface carrying this MAC address instead of selecting by ID."
+    )]
+    pub mac_address: Option<MacAddress>,
 }

crates/admin-cli/src/machine_interfaces/delete/cmd.rs

@@ -15,11 +15,20 @@
  * limitations under the License.
  */
 
+use ::rpc::forge::InterfaceDeleteQuery;
+
 use super::args::Args;
 use crate::errors::CarbideCliResult;
 use crate::rpc::ApiClient;
 
 pub async fn handle_delete(args: Args, api_client: &ApiClient) -> CarbideCliResult<()> {
-    api_client.0.delete_interface(args.interface_id).await?;
+    // Clap's ArgGroup guarantees exactly one of these is set.
+    api_client
+        .0
+        .delete_interface(InterfaceDeleteQuery {
+            id: args.interface_id,
+            mac_address: args.mac_address.map(|mac| mac.to_string()),
+        })
+        .await?;
     Ok(())
 }

crates/admin-cli/src/machine_interfaces/tests.rs

@@ -85,13 +85,31 @@ fn parse_delete_variants() {
         run = |argv| {
             Cmd::try_parse_from(argv.iter().copied())
                 .map(|cmd| match cmd {
-                    Cmd::Delete(args) => args.interface_id.to_string(),
+                    Cmd::Delete(args) => (
+                        args.interface_id.map(|id| id.to_string()),
+                        args.mac_address.map(|mac| mac.to_string()),
+                    ),
                     _ => panic!("expected Delete variant"),
                 })
                 .map_err(drop)
         };
         "with an interface ID" {
-            &["machine-interface", "delete", TEST_INTERFACE_ID][..] => Yields(TEST_INTERFACE_ID.to_string()),
+            &["machine-interface", "delete", TEST_INTERFACE_ID][..]
+                => Yields((Some(TEST_INTERFACE_ID.to_string()), None)),
+        }
+
+        "with a MAC address" {
+            &["machine-interface", "delete", "--mac-address", "00:11:22:33:44:55"][..]
+                => Yields((None, Some("00:11:22:33:44:55".to_string()))),
+        }
+
+        "ID and MAC together are rejected" {
+            &["machine-interface", "delete", TEST_INTERFACE_ID, "--mac-address", "00:11:22:33:44:55"][..]
+                => Fails,
+        }
+
+        "a malformed MAC is rejected at parse time" {
+            &["machine-interface", "delete", "--mac-address", "not-a-mac"][..] => Fails,
         }
     );
 }

crates/api-core/src/handlers/machine_interface.rs

@@ -22,6 +22,7 @@ use ::rpc::forge as rpc;
 use db::WithTransaction;
 use futures_util::FutureExt;
 use itertools::Itertools;
+use mac_address::MacAddress;
 use model::machine_interface::InterfaceType;
 use tonic::{Request, Response, Status};
 
@@ -79,42 +80,118 @@ pub(crate) async fn delete_interface(
 
     let mut txn = api.txn_begin().await?;
 
-    let rpc::InterfaceDeleteQuery { id } = request.into_inner();
-    let Some(id) = id else {
-        return Err(CarbideError::MissingArgument("delete interface.interface_id").into());
-    };
+    let rpc::InterfaceDeleteQuery { id, mac_address } = request.into_inner();
 
-    let interface = db::machine_interface::find_one(&mut txn, id).await?;
+    // Resolve the interfaces to delete. Deleting by MAC exists for the case where the
+    // operator only has the BMC MAC and not the interface id; a MAC is unique per
+    // network segment, so it can match more than one interface.
+    let interfaces = match (id, mac_address) {
+        (Some(id), None) => match db::machine_interface::find_one(&mut txn, id).await {
+            Ok(interface) => vec![interface],
+            // Report an unknown id as not-found rather than letting it fall through as an
+            // internal error, matching the unknown-MAC arm below.
+            Err(db::DatabaseError::FindOneReturnedNoResultsError(_)) => {
+                return Err(CarbideError::NotFoundError {
+                    kind: "Machine Interface",
+                    id: id.to_string(),
+                }
+                .into());
+            }
+            Err(e) => return Err(e.into()),
+        },
+        (None, Some(mac_address)) => {
+            let mac = MacAddress::from_str(&mac_address).map_err(|e| {
+                CarbideError::InvalidArgument(format!("invalid MAC address {mac_address:?}: {e}"))
+            })?;
+            let mut interfaces =
+                db::machine_interface::find_by_mac_address(txn.as_pgconn(), mac).await?;
+            if interfaces.is_empty() {
+                return Err(CarbideError::NotFoundError {
+                    kind: "Machine Interface",
+                    id: mac.to_string(),
+                }
+                .into());
+            }
+            // `machine_interface::delete` retains each row's boot pair keyed by MAC, so
+            // when several rows share this MAC the last one deleted decides the retained
+            // `boot_interface_id`. The lookup has no inherent order, so sort to make that
+            // outcome deterministic rather than dependent on the query plan.
+            interfaces.sort_by_key(|interface| interface.id);
+            interfaces
+        }
+        (Some(_), Some(_)) => {
+            return Err(CarbideError::InvalidArgument(
+                "specify either id or mac_address, not both".to_string(),
+            )
+            .into());
+        }
+        (None, None) => {
+            return Err(
+                CarbideError::MissingArgument("delete interface.id or .mac_address").into(),
+            );
+        }
+    };
 
-    // There should not be any machine associated with this interface.
-    if let Some(machine_id) = interface.machine_id {
-        if interface.interface_type == InterfaceType::Bmc {
+    // Check every interface before deleting any of them, so a MAC that matches one
+    // deletable and one attached interface refuses as a whole instead of half-deleting.
+    for interface in &interfaces {
+        // There should not be any machine associated with this interface.
+        if let Some(machine_id) = interface.machine_id {
+            if interface.interface_type == InterfaceType::Bmc {
+                return Err(CarbideError::InvalidArgument(format!(
+                    "this looks like a BMC interface and attached with machine: {machine_id}. delete that first"
+                ))
+                .into());
+            }
             return Err(CarbideError::InvalidArgument(format!(
-                "this looks like a BMC interface and attached with machine: {machine_id}. delete that first"
+                "already a machine {machine_id} is attached to this interface. delete that first"
             ))
             .into());
         }
-        return Err(CarbideError::InvalidArgument(format!(
-            "already a machine {machine_id} is attached to this interface. delete that first"
-        ))
-        .into());
-    }
 
-    // There should not be any BMC information associated with any machine.
-    for address in interface.addresses.iter() {
-        let machine_id =
-            db::machine_topology::find_machine_id_by_bmc_ip(txn.as_pgconn(), &address.to_string())
-                .await?;
-
-        if let Some(machine_id) = machine_id {
+        // Nor any other live owner. Unlike `machine_id`, these associations do not stop
+        // the row being deleted at the database level, so they have to be refused here --
+        // selecting by MAC can match rows the caller never saw, so a switch or power
+        // shelf must not be taken out as collateral.
+        if let Some(dpu_machine_id) = interface.attached_dpu_machine_id {
+            return Err(CarbideError::InvalidArgument(format!(
+                "this interface is attached to DPU machine {dpu_machine_id}. delete that first"
+            ))
+            .into());
+        }
+        if let Some(switch_id) = interface.switch_id {
+            return Err(CarbideError::InvalidArgument(format!(
+                "this interface belongs to switch {switch_id}. delete that first"
+            ))
+            .into());
+        }
+        if let Some(power_shelf_id) = interface.power_shelf_id {
             return Err(CarbideError::InvalidArgument(format!(
-                "this looks like a BMC interface and attached with machine: {machine_id}. delete that first"
+                "this interface belongs to power shelf {power_shelf_id}. delete that first"
             ))
             .into());
         }
+
+        // There should not be any BMC information associated with any machine.
+        for address in interface.addresses.iter() {
+            let machine_id = db::machine_topology::find_machine_id_by_bmc_ip(
+                txn.as_pgconn(),
+                &address.to_string(),
+            )
+            .await?;
+
+            if let Some(machine_id) = machine_id {
+                return Err(CarbideError::InvalidArgument(format!(
+                    "this looks like a BMC interface and attached with machine: {machine_id}. delete that first"
+                ))
+                .into());
+            }
+        }
     }
 
-    db::machine_interface::delete(&interface.id, &mut txn).await?;
+    for interface in &interfaces {
+        db::machine_interface::delete(&interface.id, &mut txn).await?;
+    }
 
     txn.commit().await?;