diff --git a/crates/api-core/src/dhcp/expire.rs b/crates/api-core/src/dhcp/expire.rs index e403bb3b60..a5c0d69602 100644 --- a/crates/api-core/src/dhcp/expire.rs +++ b/crates/api-core/src/dhcp/expire.rs @@ -66,23 +66,33 @@ pub async fn expire_dhcp_lease( // pair. Otherwise, just call the address-only variant, which would // be something we would see from an admin-cli call used for deleting // a specific IP allocation. - let deleted = match mac_address { + // Determine which interfaces need a hostname resync after the delete. The + // mac-scoped variant targets a single (ip, mac) row, so resync the interface + // looked up above; the address-only variant can match multiple rows, so + // resync every owner it reports rather than dropping all but one. + let (deleted, resync_targets) = match mac_address { Some(mac) => { - db::machine_interface_address::delete_by_address_and_mac( + let deleted = db::machine_interface_address::delete_by_address_and_mac( &mut txn, ip_address, mac, model::allocation_type::AllocationType::Dhcp, ) - .await? + .await?; + let targets = match (deleted, &interface) { + (true, Some(iface)) => vec![iface.id], + _ => Vec::new(), + }; + (deleted, targets) } None => { - db::machine_interface_address::delete_by_address( + let owners = db::machine_interface_address::delete_by_address( &mut txn, ip_address, model::allocation_type::AllocationType::Dhcp, ) - .await? + .await?; + (!owners.is_empty(), owners) } }; @@ -90,8 +100,8 @@ pub async fn expire_dhcp_lease( // consistent: the IP style re-derives (and re-derives again from the next // allocated IP on rediscovery); the other styles keep their names and only // drop out of DNS while addressless. - if deleted && let Some(iface) = interface { - db::machine_interface::sync_hostname_after_address_change(&mut txn, iface.id).await?; + for iface_id in &resync_targets { + db::machine_interface::sync_hostname_after_address_change(&mut txn, *iface_id).await?; } txn.commit().await?; diff --git a/crates/api-core/src/handlers/machine_interface_address.rs b/crates/api-core/src/handlers/machine_interface_address.rs index 2f197ee187..4fd9c6876a 100644 --- a/crates/api-core/src/handlers/machine_interface_address.rs +++ b/crates/api-core/src/handlers/machine_interface_address.rs @@ -151,6 +151,17 @@ pub async fn assign_static_address( ); } + // Keep the interface's IP-derived hostname/domain consistent with the new + // address, matching every other assignment path. Skipping this leaves a + // stale hostname that encodes a now-freed IP and collides with the + // fqdn_must_be_unique constraint when the allocator reuses that IP. + db::machine_interface::sync_hostname_after_address_assignment( + &mut txn, + interface_id, + target_segment.config.subdomain_id, + ) + .await?; + txn.commit().await?; let status: rpc::AssignStaticAddressStatus = result.into(); @@ -174,15 +185,24 @@ pub async fn remove_static_address( let ip_address: std::net::IpAddr = req.ip_address.parse()?; let mut txn = api.txn_begin().await?; - let deleted = db::machine_interface_address::delete_by_address( + let removed_owners = db::machine_interface_address::delete_by_address( &mut txn, ip_address, AllocationType::Static, ) .await?; + + // Re-derive the hostname/domain of every interface that actually owned a + // removed address, matching the DHCP lease-expiry path. delete_by_address + // deletes by IP, so the owner may differ from the request's interface_id; + // resyncing the wrong one would leave the real owner with a stale hostname. + for owner_id in &removed_owners { + db::machine_interface::sync_hostname_after_address_change(&mut txn, *owner_id).await?; + } + txn.commit().await?; - let status = if deleted { + let status = if !removed_owners.is_empty() { tracing::info!(%interface_id, %ip_address, "Removed static address"); rpc::RemoveStaticAddressStatus::Removed } else { diff --git a/crates/api-core/tests/integration/static_address_management.rs b/crates/api-core/tests/integration/static_address_management.rs index 371ad5c65d..1ef9474f98 100644 --- a/crates/api-core/tests/integration/static_address_management.rs +++ b/crates/api-core/tests/integration/static_address_management.rs @@ -452,6 +452,155 @@ async fn test_assign_remove_then_dhcp_reallocates( Ok(()) } +/// Regression for #3383: assign-address and remove-address must keep the +/// interface's IP-derived hostname consistent with its address, like every +/// other allocation path. A stale hostname encoding a now-freed IP collides +/// with the fqdn_must_be_unique constraint and wedges DHCP allocation on the +/// whole segment. +#[sqlx_test] +async fn test_assign_and_remove_resync_hostname( + pool: PgPool, +) -> Result<(), Box> { + let StaticAddressTestEnv { + env, admin_segment, .. + } = init(pool).await; + let mac = MacAddress::from_str("aa:bb:cc:dd:ee:17").unwrap(); + let mac_str = mac.to_string(); + + // Let the interface get a DHCP address; its hostname is derived from it. + let discover_resp = env + .api() + .discover_dhcp( + DhcpDiscovery::builder(&mac_str, admin_segment.relay_address).tonic_request(), + ) + .await? + .into_inner(); + let dhcp_ip = discover_resp.address.clone(); + let interface_id = discover_resp.machine_interface_id.unwrap(); + + let mut txn = env.db_txn().await; + let iface = db::machine_interface::find_one(&mut *txn, interface_id).await?; + assert_eq!( + iface.hostname, + dhcp_ip.replace('.', "-"), + "hostname should be derived from the DHCP address" + ); + txn.commit().await?; + + // Move it to a different static IP (same family, so it replaces the DHCP + // one). The hostname must follow the new address, not stay pinned to the + // now-freed DHCP IP. + let static_ip = "192.0.2.217"; + env.api() + .assign_static_address(Request::new(AssignStaticAddressRequest { + interface_id: Some(interface_id), + ip_address: static_ip.to_string(), + })) + .await?; + + let mut txn = env.db_txn().await; + let iface_after_assign = db::machine_interface::find_one(&mut *txn, interface_id).await?; + assert_eq!( + iface_after_assign.hostname, + static_ip.replace('.', "-"), + "hostname should resync to the newly assigned static address" + ); + txn.commit().await?; + + // Remove the static address; with no addresses left the hostname must reset + // to the dormant placeholder rather than keep encoding the freed IP. + env.api() + .remove_static_address(Request::new(RemoveStaticAddressRequest { + interface_id: Some(interface_id), + ip_address: static_ip.to_string(), + })) + .await?; + + let mut txn = env.db_txn().await; + let iface_after_remove = db::machine_interface::find_one(&mut *txn, interface_id).await?; + assert!( + iface_after_remove + .hostname + .to_lowercase() + .starts_with("noip"), + "hostname should reset to dormant format after removal, got: {}", + iface_after_remove.hostname, + ); + txn.commit().await?; + + Ok(()) +} + +/// Regression for #3383 (review follow-up): remove-address deletes by IP, so the +/// interface that owned the removed address may differ from the caller-supplied +/// interface_id. The resync must target the actual owner, otherwise the real +/// owner keeps a hostname pinned to the freed IP. +#[sqlx_test] +async fn test_remove_resyncs_the_address_owner_not_the_request_id( + pool: PgPool, +) -> Result<(), Box> { + let StaticAddressTestEnv { + env, admin_segment, .. + } = init(pool).await; + + // Owner interface: give it a static address so its hostname is IP-derived. + let owner_mac = MacAddress::from_str("aa:bb:cc:dd:ee:18") + .unwrap() + .to_string(); + let owner = env + .api() + .discover_dhcp( + DhcpDiscovery::builder(&owner_mac, admin_segment.relay_address).tonic_request(), + ) + .await? + .into_inner(); + let owner_id = owner.machine_interface_id.unwrap(); + let owned_ip = "192.0.2.218"; + env.api() + .assign_static_address(Request::new(AssignStaticAddressRequest { + interface_id: Some(owner_id), + ip_address: owned_ip.to_string(), + })) + .await?; + + // A second, unrelated interface whose id we will (incorrectly) pass to + // remove-address. + let other_mac = MacAddress::from_str("aa:bb:cc:dd:ee:19") + .unwrap() + .to_string(); + let other = env + .api() + .discover_dhcp( + DhcpDiscovery::builder(&other_mac, admin_segment.relay_address).tonic_request(), + ) + .await? + .into_inner(); + let other_id = other.machine_interface_id.unwrap(); + + // Remove the owner's IP but pass the *other* interface's id. + let resp = env + .api() + .remove_static_address(Request::new(RemoveStaticAddressRequest { + interface_id: Some(other_id), + ip_address: owned_ip.to_string(), + })) + .await? + .into_inner(); + assert_eq!(resp.status(), RemoveStaticAddressStatus::Removed); + + // The owner (which actually lost the address) must be resynced to dormant. + let mut txn = env.db_txn().await; + let owner_after = db::machine_interface::find_one(&mut *txn, owner_id).await?; + assert!( + owner_after.hostname.to_lowercase().starts_with("noip"), + "the address owner should be resynced to dormant, got: {}", + owner_after.hostname, + ); + txn.commit().await?; + + Ok(()) +} + /// When assigning a static IP that's within a managed segment's prefix, /// the interface's segment_id should be updated to that segment. #[sqlx_test] diff --git a/crates/api-db/src/machine_interface/tests.rs b/crates/api-db/src/machine_interface/tests.rs index 44e19b8605..678b4c0cf9 100644 --- a/crates/api-db/src/machine_interface/tests.rs +++ b/crates/api-db/src/machine_interface/tests.rs @@ -303,7 +303,7 @@ async fn test_retain_bmc_address_pins_dhcp_and_survives_expiry( crate::machine_interface_address::find_for_interface(txn.as_pgconn(), interface_id).await?; txn.commit().await?; assert!( - !deleted, + deleted.is_empty(), "DHCP-scoped expiry delete should not match a Static address" ); assert_eq!( diff --git a/crates/api-db/src/machine_interface_address.rs b/crates/api-db/src/machine_interface_address.rs index b38b6f9d6d..eaf7e463b5 100644 --- a/crates/api-db/src/machine_interface_address.rs +++ b/crates/api-db/src/machine_interface_address.rs @@ -203,21 +203,22 @@ pub async fn assign_static( Ok(result) } -/// Delete an address allocation of the given type. Returns true if a -/// matching allocation was found and deleted, false otherwise. +/// Delete an address allocation of the given type. Returns the interfaces that +/// owned the deleted addresses (normally one, empty if nothing matched) so +/// callers can resync each one's hostname rather than guessing the owner. The +/// delete removes every matching row, so all owners are returned — not just the +/// first — since `(address, allocation_type)` is not unique on its own. pub async fn delete_by_address( txn: &mut PgConnection, address: IpAddr, allocation_type: AllocationType, -) -> Result { - let query = - "DELETE FROM machine_interface_addresses WHERE address = $1::inet AND allocation_type = $2"; - sqlx::query(query) +) -> Result, DatabaseError> { + let query = "DELETE FROM machine_interface_addresses WHERE address = $1::inet AND allocation_type = $2 RETURNING interface_id"; + sqlx::query_scalar(query) .bind(address) .bind(allocation_type) - .execute(txn) + .fetch_all(txn) .await - .map(|r| r.rows_affected() > 0) .map_err(|e| DatabaseError::query(query, e)) }