diff --git a/crates/api-core/src/cfg/README.md b/crates/api-core/src/cfg/README.md index 71c0ee611d..2cca868efb 100644 --- a/crates/api-core/src/cfg/README.md +++ b/crates/api-core/src/cfg/README.md @@ -42,7 +42,7 @@ Use `site_explorer.dpu_policy` instead. | `enable_route_servers` | `bool` | `false` | `networking` | Enables route server injection into DPU FRR configs for L2VPN. | | `deny_prefixes` | `Vec` | `[]` | `networking` | IPv4 and IPv6 CIDR prefixes that tenant instances are blocked from reaching. FNN generates family-specific NVUE ACL policies; all non-FNN virtualizers apply the IPv4 prefixes only. | | `site_fabric_prefixes` | `Vec` | `[]` | `networking` | IP prefixes (v4/v6) assigned for tenant use within this site. | -| `tenant_prefix_overlap_enabled` | `bool` | `false` | `networking` | Site opt-in for tenant prefix overlap admission. This setting has no effect until [#3890](https://github.com/NVIDIA/infra-controller/issues/3890) lands and does not change the existing database prefix constraints. Admission will also require site-wide `vpc_isolation_behavior = "mutual_isolation"` and participating FNN base profiles with `tenant_prefix_overlap_eligible = true`. | +| `tenant_prefix_overlap_enabled` | `bool` | `false` | `networking` | Site opt-in for [tenant prefix overlap checks](#tenant-prefix-overlap-checks). The existing `VpcPrefix` exclusion continues to prevent overlapping `VpcPrefix` persistence until the cutover tracked by [#3892](https://github.com/NVIDIA/infra-controller/issues/3892). | | `max_site_prefixes_per_tenant` | `u32` | `8` | `networking` | Maximum tenant-managed SitePrefixes retained for one tenant at this site. Prefixes awaiting removal still count against this limit and keep their CIDR reserved. | | `anycast_site_prefixes` | `Vec` | `[]` | `networking` | Aggregate IPv4 prefixes containing tenant-announced prefixes (e.g., BYOIP). **Deprecated.** Use [`routing_profiles.allowed_anycast_prefixes`](#fnnroutingprofileconfig) instead. | | `common_tenant_host_asn` | `Option` | — | `networking` | ASN that tenants use to peer with the DPU. If unset, any ASN is accepted. | @@ -705,7 +705,7 @@ client-certificate authentication is not used. | `route_target_imports` | `Option>` | — (effective `[]`) | Route targets imported into DPU VRFs for VPC routes. | | `route_targets_on_exports` | `Option>` | — (effective `[]`) | Route targets added to routes exported by the DPU. | | `internal` | `Option` | — (effective `false`) | Whether the profile uses internal VNI allocation. This property cannot be overridden on a VPC. | -| `tenant_prefix_overlap_eligible` | `bool` | `false` | Base-profile opt-in for tenant prefix overlap admission. This setting has no effect until [#3890](https://github.com/NVIDIA/infra-controller/issues/3890) lands and cannot be overridden on a VPC. | +| `tenant_prefix_overlap_eligible` | `bool` | `false` | Base routing profile opt-in for [tenant prefix overlap checks](#tenant-prefix-overlap-checks). This setting cannot be overridden on a VPC. | | `leak_default_route_from_underlay` | `Option` | — (effective `false`) | Leak the default route from the underlay/default VRF into tenant VRFs. | | `leak_tenant_host_routes_to_underlay` | `Option` | — (effective `false`) | Leak tenant host routes into the underlay/default VRF. | | `tenant_leak_communities_accepted` | `Option` | — (effective `false`) | Honor route-leak communities sent by the tenant host OS. | @@ -717,6 +717,54 @@ Unset properties retain presence information so a VPC's inline `routing_profile_overrides` can inherit them. After the named profile and VPC override are combined, properties still unset use the effective defaults above. +### Tenant prefix overlap checks + +`tenant_prefix_overlap_enabled` defaults to `false`. When set to `true`, NICo +checks whether two `VpcPrefix` records may reuse the same CIDR. It does not +permit direct `NetworkPrefix` reuse or change the database constraints. + +An overlapping `VpcPrefix` pair is eligible only when all of these conditions +are true: + +- The requested and existing CIDRs are identical, the existing `VpcPrefix` is + not deleted, and the `VpcPrefix` records belong to different VPCs and tenant + organizations. +- Both VPCs use FNN and have assigned, distinct status VNIs. +- Each `VpcPrefix` is linked to a tenant-managed, `DatacenterOnly` `SitePrefix` + owned by its VPC tenant and containing the `VpcPrefix` CIDR. The requested + `SitePrefix` must be `Ready`; the existing `SitePrefix` may be `Ready` or + `Deleting`. +- Site-wide `vpc_isolation_behavior` is `"mutual_isolation"`. +- `site_global_vpc_vni` and `common_internal_route_target` are unset, and + `additional_route_target_imports` is empty, so they cannot bridge the VPCs. +- Each resolved FNN profile, after applying its VPC overrides, has + `tenant_prefix_overlap_eligible = true` and `internal = true`; has no import + or export route targets; disables default-route leakage, tenant-host-route + leakage, and tenant leak communities; and has no accepted underlay leaks or + allowed anycast prefixes. + +The gRPC `CreateNetworkSegment` and `AttachNetworkSegmentToVpc` handlers reject +any direct prefix that overlaps a `VpcPrefix`, regardless of the site gate. The +gRPC `CreateVpcPrefix` handler considers prefixes on attached segments. It may +adopt only direct Tenant segment prefixes in the same VPC that are not already +linked to a `VpcPrefix`; every other direct `NetworkPrefix` overlap on an +attached segment is rejected. An unattached `CreateNetworkSegment` request does +not run these checks, but a later attachment does. + +These handlers do not validate changes to peering or VPC policy, or Instance +paths that retain routing state. They also do not cover startup or audit every +writer. Those checks are tracked in +[#5114](https://github.com/NVIDIA/infra-controller/issues/5114) and +[#5115](https://github.com/NVIDIA/infra-controller/issues/5115), while startup +and complete writer coverage are tracked in +[#5116](https://github.com/NVIDIA/infra-controller/issues/5116). All three must +land before the database cutover in +[#3892](https://github.com/NVIDIA/infra-controller/issues/3892). + +Even when the application accepts an eligible pair, the existing `VpcPrefix` +exclusion rejects overlapping `VpcPrefix` persistence until the cutover tracked +by [#3892](https://github.com/NVIDIA/infra-controller/issues/3892). + ### `VpcDefinition` | Field | Type | Default | Description | diff --git a/crates/api-core/src/cfg/file.rs b/crates/api-core/src/cfg/file.rs index 32100ba23a..350e139764 100644 --- a/crates/api-core/src/cfg/file.rs +++ b/crates/api-core/src/cfg/file.rs @@ -231,11 +231,11 @@ pub struct CarbideConfig { #[serde(default)] pub site_fabric_prefixes: Vec, - /// Opts this site into tenant prefix overlap admission. + /// Opts this site into exact tenant VpcPrefix reuse checks. /// - /// Defaults to `false`. Participating FNN base profiles must separately - /// set `tenant_prefix_overlap_eligible`, and configuration alone does not permit - /// duplicate prefix persistence while database exclusions remain active. + /// Defaults to `false`. The complete eligibility, rejection, and database + /// fallback contract is documented under "Tenant prefix overlap checks" + /// in `crates/api-core/src/cfg/README.md`. #[serde(default)] pub tenant_prefix_overlap_enabled: bool, @@ -2793,12 +2793,12 @@ pub struct FnnRoutingProfileConfig { #[serde(default)] pub internal: Option, - /// Opts VPCs based on this profile into future tenant prefix overlap admission. + /// Opts VPCs based on this profile into exact tenant VpcPrefix reuse checks. /// /// This base-profile setting defaults to `false` and cannot be overridden - /// by a VPC. Admission support is tracked by - /// ; this value - /// alone changes neither routing nor prefix persistence. + /// by a VPC. The complete eligibility, rejection, and database fallback + /// contract is documented under "Tenant prefix overlap checks" in + /// `crates/api-core/src/cfg/README.md`. #[serde(default)] pub tenant_prefix_overlap_eligible: bool, @@ -2845,22 +2845,27 @@ pub struct FnnRoutingProfileConfig { } impl FnnRoutingProfileConfig { - /// Returns whether this resolved profile satisfies the profile-local overlap policy. + /// `is_eligible_for_tenant_prefix_overlap` returns whether the resolved + /// profile meets every profile condition for exact prefix reuse. /// /// Evaluate the profile returned by [`FnnConfig::resolve_vpc_routing_profile`], /// not the raw base profile, so VPC overrides participate in the decision. - /// This check cannot see site-wide route targets, additional FNN imports, - /// VPC peering, or retained routing state. Callers must reject those paths - /// between overlapping VPCs and separately require the site gate and - /// site-wide `vpc_isolation_behavior = "mutual_isolation"`. - #[allow(dead_code)] // Staged for https://github.com/NVIDIA/infra-controller/issues/3890. + /// The caller adds the site-wide conditions for these prefix writers. + /// Peering and VPC policy changes are tracked in + /// , and retained + /// Instance paths are tracked in + /// . Startup and + /// complete writer coverage are tracked in + /// . All three must + /// land before the database cutover in + /// . pub(crate) fn is_eligible_for_tenant_prefix_overlap(&self) -> bool { // Keep this exhaustive so new profile fields require an explicit eligibility decision. let Self { tenant_prefix_overlap_eligible, route_target_imports, route_targets_on_exports, - // External profiles are outside the initial overlap-admission scope. + // External profiles cannot participate in exact prefix reuse. internal, leak_default_route_from_underlay, leak_tenant_host_routes_to_underlay, diff --git a/crates/api-core/src/handlers/mod.rs b/crates/api-core/src/handlers/mod.rs index b17232d5dc..9e4cbbd422 100644 --- a/crates/api-core/src/handlers/mod.rs +++ b/crates/api-core/src/handlers/mod.rs @@ -90,6 +90,7 @@ pub(super) mod switch; pub(super) mod tenant; pub(super) mod tenant_identity_config; pub(super) mod tenant_keyset; +mod tenant_prefix_overlap; pub(super) mod tpm_ca; pub(super) mod uefi; pub(super) mod uefi_credential_rotation; diff --git a/crates/api-core/src/handlers/network_segment.rs b/crates/api-core/src/handlers/network_segment.rs index bb6b0138b0..2e5755d9c7 100644 --- a/crates/api-core/src/handlers/network_segment.rs +++ b/crates/api-core/src/handlers/network_segment.rs @@ -17,6 +17,7 @@ use ::rpc::forge as rpc; use db::resource_pool::ResourcePoolDatabaseError; use db::{AnnotatedSqlxError, DatabaseError, ObjectColumnFilter, network_segment}; +use ipnetwork::IpNetwork; use model::network_segment::{ NetworkSegment, NetworkSegmentControllerState, NetworkSegmentSearchConfig, NetworkSegmentType, NewNetworkSegment, @@ -25,8 +26,8 @@ use model::vpc::VpcVirtualizationTypeCapabilities; use sqlx::{PgConnection, PgTransaction}; use tonic::{Request, Response, Status}; -use crate::CarbideError; use crate::api::{Api, log_request_data}; +use crate::{CarbideError, CarbideResult}; pub(crate) async fn find_ids( api: &Api, @@ -87,6 +88,24 @@ pub(crate) async fn find_by_ids( })) } +/// `reject_vpc_prefix_overlaps` rejects direct `NetworkPrefix` records that +/// overlap a `VpcPrefix`. +/// +/// The caller holds the overlap transaction lock from this probe through the +/// `NetworkSegment` write, so another participating `VpcPrefix` request cannot +/// commit between them. +async fn reject_vpc_prefix_overlaps( + txn: &mut PgConnection, + prefixes: &[IpNetwork], +) -> CarbideResult<()> { + for prefix in prefixes { + if !db::vpc_prefix::probe(*prefix, &mut *txn).await?.is_empty() { + return Err(super::tenant_prefix_overlap::overlap_error()); + } + } + Ok(()) +} + pub(crate) async fn create( api: &Api, request: Request, @@ -131,6 +150,9 @@ pub(crate) async fn create( } let mut txn = api.txn_begin().await?; + if new_network_segment.vpc_id.is_some() { + db::tenant_prefix_overlap::lock_checks(txn.as_mut()).await?; + } let allocate_svi_ip = if let Some(vpc_id) = new_network_segment.vpc_id { let vpcs = db::vpc::find_by( @@ -156,6 +178,15 @@ pub(crate) async fn create( false }; + if new_network_segment.vpc_id.is_some() { + let prefixes = new_network_segment + .prefixes + .iter() + .map(|prefix| prefix.prefix) + .collect::>(); + reject_vpc_prefix_overlaps(&mut txn, &prefixes).await?; + } + let network_segment = save(api, &mut txn, new_network_segment, false, allocate_svi_ip).await?; txn.commit().await?; @@ -181,6 +212,7 @@ pub(crate) async fn attach_to_vpc( let vpc_id = vpc_id.ok_or(CarbideError::MissingArgument("vpc_id"))?; let mut txn = api.txn_begin().await?; + db::tenant_prefix_overlap::lock_checks(txn.as_mut()).await?; let vpcs = db::vpc::find_by_with_lock( txn.as_mut(), @@ -228,7 +260,16 @@ pub(crate) async fn attach_to_vpc( )) .into()); } - _ => db::network_segment::attach_to_vpc(&segment, txn.as_mut(), vpc_id).await?, + _ => { + let prefixes = segment + .prefixes + .iter() + .filter(|prefix| prefix.vpc_prefix_id.is_none()) + .map(|prefix| prefix.prefix) + .collect::>(); + reject_vpc_prefix_overlaps(&mut txn, &prefixes).await?; + db::network_segment::attach_to_vpc(&segment, txn.as_mut(), vpc_id).await? + } }; txn.commit().await?; diff --git a/crates/api-core/src/handlers/tenant_prefix_overlap.rs b/crates/api-core/src/handlers/tenant_prefix_overlap.rs new file mode 100644 index 0000000000..a37ec657a5 --- /dev/null +++ b/crates/api-core/src/handlers/tenant_prefix_overlap.rs @@ -0,0 +1,462 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +//! Tenant prefix overlap policy shared by `VpcPrefix` and `NetworkSegment` +//! handlers. + +use carbide_network::virtualization::VpcVirtualizationType; +use ipnetwork::IpNetwork; +use model::site_prefix::{ + SitePrefix, SitePrefixAuthority, SitePrefixLifecycleState, SitePrefixRoutingScope, +}; +use model::vpc::Vpc; + +use crate::CarbideError; +use crate::cfg::file::VpcIsolationBehaviorType; + +const INELIGIBLE_OVERLAP: &str = + "the requested prefix overlaps address space that is not eligible for reuse"; + +/// `overlap_error` returns the common client error for an overlap that cannot +/// be reused. +/// +/// It does not identify the conflicting resource because that resource may +/// belong to another tenant. +pub(super) fn overlap_error() -> CarbideError { + CarbideError::InvalidArgument(INELIGIBLE_OVERLAP.to_string()) +} + +/// `VpcPrefixParticipant` groups the `VpcPrefix`, VPC, and `SitePrefix` facts +/// needed for one exact CIDR check. +pub(super) struct VpcPrefixParticipant<'a> { + /// Exact `VpcPrefix` CIDR under consideration. + pub(super) prefix: IpNetwork, + /// Whether the `VpcPrefix` has started deletion. + pub(super) is_deleted: bool, + /// VPC that owns the `VpcPrefix`. + pub(super) vpc: &'a Vpc, + /// `SitePrefix` referenced by the `VpcPrefix`. + pub(super) site_prefix: &'a SitePrefix, +} + +/// `contains_prefix` returns whether `parent` contains `child` in the same +/// address family. +pub(super) fn contains_prefix(parent: IpNetwork, child: IpNetwork) -> bool { + match (parent, child) { + (IpNetwork::V4(parent), IpNetwork::V4(child)) => child.is_subnet_of(parent), + (IpNetwork::V6(parent), IpNetwork::V6(child)) => child.is_subnet_of(parent), + _ => false, + } +} + +/// `site_prefix_is_eligible` checks the `SitePrefix` requirements for one +/// `VpcPrefix`. +/// +/// `allow_deleting` is true only for an existing `VpcPrefix`. Its CIDR remains +/// reserved while the `SitePrefix` is `Deleting`, but a new `VpcPrefix` +/// requires a `Ready` `SitePrefix`. +fn site_prefix_is_eligible( + site_prefix: &SitePrefix, + vpc: &Vpc, + prefix: IpNetwork, + allow_deleting: bool, +) -> bool { + site_prefix.status.authority == SitePrefixAuthority::TenantManaged + && site_prefix + .config + .tenant_organization_id + .as_ref() + .map(|id| id.as_str()) + == Some(vpc.config.tenant_organization_id.as_str()) + && site_prefix.config.routing_scope == SitePrefixRoutingScope::DatacenterOnly + && contains_prefix(site_prefix.config.prefix, prefix) + && match site_prefix.status.lifecycle_state { + SitePrefixLifecycleState::Ready => true, + SitePrefixLifecycleState::Deleting => allow_deleting, + SitePrefixLifecycleState::Provisioning | SitePrefixLifecycleState::Error => false, + } +} + +/// `pair_is_eligible` returns whether two `VpcPrefix` records may reuse one +/// exact CIDR. +/// +/// The pair eligibility checks ownership, FNN isolation, distinct VNIs, +/// `SitePrefix` state, and each VPC's resolved routing profile. Callers still +/// need `db::tenant_prefix_overlap::lock_checks` and must reject every +/// ineligible overlap. +pub(super) fn pair_is_eligible( + runtime_config: &crate::cfg::file::CarbideConfig, + candidate: VpcPrefixParticipant<'_>, + existing: VpcPrefixParticipant<'_>, +) -> bool { + if !runtime_config.tenant_prefix_overlap_enabled + || !matches!( + runtime_config.vpc_isolation_behavior, + VpcIsolationBehaviorType::MutualIsolation + ) + || runtime_config.site_global_vpc_vni.is_some() + || existing.is_deleted + || candidate.prefix != existing.prefix + || candidate.vpc.id == existing.vpc.id + || candidate.vpc.config.tenant_organization_id == existing.vpc.config.tenant_organization_id + || candidate.vpc.config.network_virtualization_type != VpcVirtualizationType::Fnn + || existing.vpc.config.network_virtualization_type != VpcVirtualizationType::Fnn + || !site_prefix_is_eligible( + candidate.site_prefix, + candidate.vpc, + candidate.prefix, + false, + ) + || !site_prefix_is_eligible(existing.site_prefix, existing.vpc, existing.prefix, true) + || !matches!( + (candidate.vpc.status.vni, existing.vpc.status.vni), + (Some(candidate_vni), Some(existing_vni)) if candidate_vni != existing_vni + ) + { + return false; + } + + let Some(fnn) = runtime_config.fnn.as_ref() else { + return false; + }; + if fnn.common_internal_route_target.is_some() || !fnn.additional_route_target_imports.is_empty() + { + return false; + } + let Ok(candidate_profile) = fnn.resolve_vpc_routing_profile(&candidate.vpc.config) else { + return false; + }; + let Ok(existing_profile) = fnn.resolve_vpc_routing_profile(&existing.vpc.config) else { + return false; + }; + + candidate_profile.is_eligible_for_tenant_prefix_overlap() + && existing_profile.is_eligible_for_tenant_prefix_overlap() +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use carbide_network::virtualization::VpcVirtualizationType; + use carbide_test_support::{Check, check_values}; + use carbide_uuid::site_prefix::SitePrefixId; + use carbide_uuid::vpc::VpcId; + use chrono::Utc; + use config_version::ConfigVersion; + use model::metadata::Metadata; + use model::site_prefix::{SitePrefixConfig, SitePrefixStatus}; + use model::vpc::{VpcConfig, VpcStatus}; + + use super::*; + use crate::cfg::file::{FnnConfig, FnnRoutingProfileConfig}; + + /// Test-specific enum that selects one eligibility rule to vary. + #[derive(Clone, Copy, Debug)] + enum Variation { + Eligible, + RetainedRootDeleting, + SiteGateDisabled, + OpenIsolation, + SiteGlobalVpcVni, + CommonInternalRouteTarget, + AdditionalRouteTargetImport, + NestedPrefix, + SameVpc, + SameTenant, + ExistingNotFnn, + CandidateOperatorRoot, + ExistingWrongTenantRoot, + ExistingRootProvisioning, + CandidateUnsafeProfile, + ExistingUnsafeProfile, + CandidateVniMissing, + ExistingVniMissing, + SameVni, + ExistingPrefixDeleting, + } + + /// Test-specific function that enables every site and profile condition required for reuse. + fn eligible_config() -> crate::cfg::file::CarbideConfig { + let mut config = crate::test_support::default_config::get(); + config.tenant_prefix_overlap_enabled = true; + config.vpc_isolation_behavior = VpcIsolationBehaviorType::MutualIsolation; + config.fnn = Some(FnnConfig { + admin_vpc: None, + common_internal_route_target: None, + additional_route_target_imports: vec![], + routing_profiles: HashMap::from([ + ( + "ELIGIBLE".to_string(), + FnnRoutingProfileConfig { + tenant_prefix_overlap_eligible: true, + internal: Some(true), + ..Default::default() + }, + ), + ( + "UNSAFE".to_string(), + FnnRoutingProfileConfig { + internal: Some(true), + ..Default::default() + }, + ), + ]), + use_vpc_vrf_loopback: false, + }); + config + } + + /// Test-specific function that builds an FNN VPC with the requested tenant and VNI. + fn vpc(tenant: &str, vni: Option) -> Vpc { + Vpc { + id: VpcId::new(), + version: ConfigVersion::initial(), + config: VpcConfig { + tenant_organization_id: tenant.to_string(), + tenant_keyset_id: None, + network_virtualization_type: VpcVirtualizationType::Fnn, + network_security_group_id: None, + default_nvlink_logical_partition_id: None, + vni: None, + routing_profile_type: Some("ELIGIBLE".to_string()), + routing_profile_overrides: None, + power_resource_group: None, + slaac_enabled: false, + }, + status: VpcStatus { vni }, + metadata: Metadata::default(), + created: Utc::now(), + updated: Utc::now(), + deleted: None, + } + } + + /// Test-specific function that builds a ready tenant-managed SitePrefix for one tenant. + fn site_prefix(tenant: &str, prefix: IpNetwork) -> SitePrefix { + SitePrefix { + id: SitePrefixId::new(), + config: SitePrefixConfig { + prefix, + tenant_organization_id: Some(tenant.parse().unwrap()), + routing_scope: SitePrefixRoutingScope::DatacenterOnly, + }, + metadata: Metadata::default(), + status: SitePrefixStatus { + authority: SitePrefixAuthority::TenantManaged, + lifecycle_state: SitePrefixLifecycleState::Ready, + }, + version: ConfigVersion::initial(), + created_at: Utc::now(), + updated_at: Utc::now(), + } + } + + /// Test-specific function that checks each pair eligibility rule from an eligible pair. + #[test] + fn exact_pair_eligibility_is_fail_closed() { + let exact: IpNetwork = "10.0.1.0/24".parse().unwrap(); + + check_values( + [ + Check { + scenario: "eligible pair", + input: Variation::Eligible, + expect: true, + }, + Check { + scenario: "existing SitePrefix is deleting", + input: Variation::RetainedRootDeleting, + expect: true, + }, + Check { + scenario: "site gate is disabled", + input: Variation::SiteGateDisabled, + expect: false, + }, + Check { + scenario: "site isolation is open", + input: Variation::OpenIsolation, + expect: false, + }, + Check { + scenario: "site uses a global VPC VNI", + input: Variation::SiteGlobalVpcVni, + expect: false, + }, + Check { + scenario: "site uses a common internal route target", + input: Variation::CommonInternalRouteTarget, + expect: false, + }, + Check { + scenario: "site adds route target imports", + input: Variation::AdditionalRouteTargetImport, + expect: false, + }, + Check { + scenario: "candidate CIDR is nested", + input: Variation::NestedPrefix, + expect: false, + }, + Check { + scenario: "prefixes belong to the same VPC", + input: Variation::SameVpc, + expect: false, + }, + Check { + scenario: "prefixes belong to the same tenant", + input: Variation::SameTenant, + expect: false, + }, + Check { + scenario: "existing VPC does not use FNN", + input: Variation::ExistingNotFnn, + expect: false, + }, + Check { + scenario: "candidate SitePrefix is operator managed", + input: Variation::CandidateOperatorRoot, + expect: false, + }, + Check { + scenario: "existing SitePrefix belongs to another tenant", + input: Variation::ExistingWrongTenantRoot, + expect: false, + }, + Check { + scenario: "existing SitePrefix is provisioning", + input: Variation::ExistingRootProvisioning, + expect: false, + }, + Check { + scenario: "candidate profile is unsafe", + input: Variation::CandidateUnsafeProfile, + expect: false, + }, + Check { + scenario: "existing profile is unsafe", + input: Variation::ExistingUnsafeProfile, + expect: false, + }, + Check { + scenario: "candidate VNI is missing", + input: Variation::CandidateVniMissing, + expect: false, + }, + Check { + scenario: "existing VNI is missing", + input: Variation::ExistingVniMissing, + expect: false, + }, + Check { + scenario: "VPCs use the same VNI", + input: Variation::SameVni, + expect: false, + }, + Check { + scenario: "existing VpcPrefix is deleting", + input: Variation::ExistingPrefixDeleting, + expect: false, + }, + ], + |variation| { + let mut config = eligible_config(); + let mut candidate_prefix = exact; + let mut candidate_vpc = vpc("tenant-a", Some(100)); + let mut existing_vpc = vpc("tenant-b", Some(200)); + let mut candidate_site_prefix = + site_prefix("tenant-a", "10.0.0.0/16".parse().unwrap()); + let mut existing_site_prefix = + site_prefix("tenant-b", "10.0.0.0/16".parse().unwrap()); + let mut existing_deleted = false; + + match variation { + Variation::Eligible => {} + Variation::RetainedRootDeleting => { + existing_site_prefix.status.lifecycle_state = + SitePrefixLifecycleState::Deleting; + } + Variation::SiteGateDisabled => config.tenant_prefix_overlap_enabled = false, + Variation::OpenIsolation => { + config.vpc_isolation_behavior = VpcIsolationBehaviorType::Open; + } + Variation::SiteGlobalVpcVni => config.site_global_vpc_vni = Some(5_000), + Variation::CommonInternalRouteTarget => { + config.fnn.as_mut().unwrap().common_internal_route_target = + Some(crate::cfg::file::RouteTargetConfig { asn: 1, vni: 2 }); + } + Variation::AdditionalRouteTargetImport => { + config.fnn.as_mut().unwrap().additional_route_target_imports = + vec![crate::cfg::file::RouteTargetConfig { asn: 3, vni: 4 }]; + } + Variation::NestedPrefix => { + candidate_prefix = "10.0.1.0/25".parse().unwrap(); + } + Variation::SameVpc => existing_vpc.id = candidate_vpc.id, + Variation::SameTenant => { + existing_vpc.config.tenant_organization_id = "tenant-a".to_string(); + existing_site_prefix.config.tenant_organization_id = + Some("tenant-a".parse().unwrap()); + } + Variation::ExistingNotFnn => { + existing_vpc.config.network_virtualization_type = + VpcVirtualizationType::Flat; + } + Variation::CandidateOperatorRoot => { + candidate_site_prefix.status.authority = + SitePrefixAuthority::OperatorManaged; + } + Variation::ExistingWrongTenantRoot => { + existing_site_prefix.config.tenant_organization_id = + Some("tenant-c".parse().unwrap()); + } + Variation::ExistingRootProvisioning => { + existing_site_prefix.status.lifecycle_state = + SitePrefixLifecycleState::Provisioning; + } + Variation::CandidateUnsafeProfile => { + candidate_vpc.config.routing_profile_type = Some("UNSAFE".to_string()); + } + Variation::ExistingUnsafeProfile => { + existing_vpc.config.routing_profile_type = Some("UNSAFE".to_string()); + } + Variation::CandidateVniMissing => candidate_vpc.status.vni = None, + Variation::ExistingVniMissing => existing_vpc.status.vni = None, + Variation::SameVni => existing_vpc.status.vni = candidate_vpc.status.vni, + Variation::ExistingPrefixDeleting => existing_deleted = true, + } + + pair_is_eligible( + &config, + VpcPrefixParticipant { + prefix: candidate_prefix, + is_deleted: false, + vpc: &candidate_vpc, + site_prefix: &candidate_site_prefix, + }, + VpcPrefixParticipant { + prefix: exact, + is_deleted: existing_deleted, + vpc: &existing_vpc, + site_prefix: &existing_site_prefix, + }, + ) + }, + ); + } +} diff --git a/crates/api-core/src/handlers/vpc_prefix.rs b/crates/api-core/src/handlers/vpc_prefix.rs index 658a013542..bc0106e53b 100644 --- a/crates/api-core/src/handlers/vpc_prefix.rs +++ b/crates/api-core/src/handlers/vpc_prefix.rs @@ -15,29 +15,27 @@ * limitations under the License. */ +use std::collections::HashMap; + use ::db::{ObjectColumnFilter, vpc_prefix as db}; use ::rpc::forge as rpc; use ::rpc::forge::PrefixMatchType; use carbide_network::virtualization::VpcVirtualizationType; +use carbide_uuid::vpc::VpcId; use ipnetwork::IpNetwork; use model::network_prefix::NetworkPrefix; +use model::network_segment::NetworkSegmentType; use model::site_prefix::{ SitePrefix, SitePrefixAuthority, SitePrefixLifecycleState, SitePrefixRoutingScope, }; use model::vpc::{Vpc, VpcVirtualizationTypeCapabilities}; use model::vpc_prefix; +use sqlx::PgConnection; use tonic::{Request, Response, Status}; -use crate::CarbideError; use crate::api::{Api, log_request_data}; - -fn contains_prefix(parent: IpNetwork, child: IpNetwork) -> bool { - match (parent, child) { - (IpNetwork::V4(parent), IpNetwork::V4(child)) => child.is_subnet_of(parent), - (IpNetwork::V6(parent), IpNetwork::V6(child)) => child.is_subnet_of(parent), - _ => false, - } -} +use crate::cfg::file::CarbideConfig; +use crate::{CarbideError, CarbideResult}; fn validate_site_prefix_attachment( site_prefix: &SitePrefix, @@ -72,7 +70,7 @@ fn validate_site_prefix_attachment( ))); } - if !contains_prefix(site_prefix.config.prefix, vpc_prefix) { + if !super::tenant_prefix_overlap::contains_prefix(site_prefix.config.prefix, vpc_prefix) { return Err(CarbideError::InvalidArgument(format!( "the VPC prefix {vpc_prefix} is not contained within SitePrefix {} ({})", site_prefix.id, site_prefix.config.prefix @@ -90,6 +88,104 @@ fn validate_site_prefix_attachment( Ok(()) } +/// `validate_vpc_prefix_overlaps` rejects `candidate` unless every existing +/// overlap passes `pair_is_eligible`. +/// +/// The caller acquires the overlap lock before reading the candidate `Vpc` and +/// any selected `SitePrefix`, so a waiting create sees every competing prefix +/// that committed first. +async fn validate_vpc_prefix_overlaps( + runtime_config: &CarbideConfig, + txn: &mut PgConnection, + candidate: &vpc_prefix::NewVpcPrefix, + candidate_vpc: &Vpc, + candidate_site_prefix: Option<&SitePrefix>, + overlaps: &[vpc_prefix::VpcPrefix], +) -> CarbideResult<()> { + if overlaps.is_empty() { + return Ok(()); + } + + let Some(candidate_site_prefix) = candidate_site_prefix else { + return Err(super::tenant_prefix_overlap::overlap_error()); + }; + let vpc_ids = overlaps + .iter() + .map(|prefix| prefix.vpc_id) + .collect::>(); + let vpcs = ::db::vpc::find_by( + &mut *txn, + ObjectColumnFilter::List(::db::vpc::IdColumn, &vpc_ids), + ) + .await? + .into_iter() + .map(|vpc| (vpc.id, vpc)) + .collect::>(); + let Some(site_prefix_ids) = overlaps + .iter() + .map(|prefix| prefix.site_prefix_id) + .collect::>>() + else { + return Err(super::tenant_prefix_overlap::overlap_error()); + }; + let site_prefixes = ::db::site_prefix::find_by_ids(&mut *txn, &site_prefix_ids) + .await? + .into_iter() + .map(|site_prefix| (site_prefix.id, site_prefix)) + .collect::>(); + + for existing in overlaps { + let Some(existing_vpc) = vpcs.get(&existing.vpc_id) else { + return Err(super::tenant_prefix_overlap::overlap_error()); + }; + let Some(existing_site_prefix) = existing + .site_prefix_id + .and_then(|id| site_prefixes.get(&id)) + else { + return Err(super::tenant_prefix_overlap::overlap_error()); + }; + if !super::tenant_prefix_overlap::pair_is_eligible( + runtime_config, + super::tenant_prefix_overlap::VpcPrefixParticipant { + prefix: candidate.config.prefix, + is_deleted: false, + vpc: candidate_vpc, + site_prefix: candidate_site_prefix, + }, + super::tenant_prefix_overlap::VpcPrefixParticipant { + prefix: existing.config.prefix, + is_deleted: existing.deleted.is_some(), + vpc: existing_vpc, + site_prefix: existing_site_prefix, + }, + ) { + return Err(super::tenant_prefix_overlap::overlap_error()); + } + } + + Ok(()) +} + +/// `adoptable_segment_prefixes` returns direct Tenant prefixes the `VpcPrefix` +/// may adopt. +/// +/// A direct prefix on another VPC or a non-Tenant segment cannot be linked to +/// this `VpcPrefix`, so the request is rejected without identifying its owner. +fn adoptable_segment_prefixes( + overlaps: Vec, + candidate_vpc_id: VpcId, +) -> CarbideResult> { + let mut adoptable = Vec::with_capacity(overlaps.len()); + for overlap in overlaps { + if overlap.vpc_id != candidate_vpc_id || overlap.segment_type != NetworkSegmentType::Tenant + { + return Err(super::tenant_prefix_overlap::overlap_error()); + } + adoptable.push(overlap.prefix); + } + Ok(adoptable) +} + pub(crate) async fn create( api: &Api, request: Request, @@ -113,6 +209,7 @@ pub(crate) async fn create( } let mut txn = api.txn_begin().await?; + ::db::tenant_prefix_overlap::lock_checks(txn.as_mut()).await?; // Resolve and lock the exact SitePrefix before locking the VPC. The shared // row lock permits concurrent child creation but conflicts with retirement @@ -195,8 +292,8 @@ pub(crate) async fn create( id: new_prefix.vpc_id.to_string(), })?; - if let Some(site_prefix) = selected_site_prefix { - validate_site_prefix_attachment(&site_prefix, vpc, new_prefix.config.prefix)?; + if let Some(ref site_prefix) = selected_site_prefix { + validate_site_prefix_attachment(site_prefix, vpc, new_prefix.config.prefix)?; new_prefix.site_prefix_id = Some(site_prefix.id); } else if let Some(ref site_prefixes) = api.eth_data.site_fabric_prefixes { // Preserve the mixed-version configured-root path. Production startup @@ -221,47 +318,18 @@ pub(crate) async fn create( let expected_vpc_version = vpc.version; let conflicting_vpc_prefixes = db::probe(new_prefix.config.prefix, &mut txn).await?; - if !conflicting_vpc_prefixes.is_empty() { - let conflicting_vpc_prefixes = conflicting_vpc_prefixes - .into_iter() - .map(|p| p.config.prefix); - let conflicting_vpc_prefixes = itertools::join(conflicting_vpc_prefixes, ", "); - let msg = format!( - "The requested VPC prefix ({vpc_prefix}) overlaps at least one \ - existing VPC prefix ({conflicting_vpc_prefixes})", - vpc_prefix = new_prefix.config.prefix, - ); - return Err(CarbideError::InvalidArgument(msg).into()); - } + validate_vpc_prefix_overlaps( + &api.runtime_config, + &mut txn, + &new_prefix, + vpc, + selected_site_prefix.as_ref(), + &conflicting_vpc_prefixes, + ) + .await?; let segment_prefixes = db::probe_segment_prefixes(new_prefix.config.prefix, &mut txn).await?; - - // Check that all the prefixes we found are on segments that belong to our - // own VPC. - let segment_prefixes: Vec = { - let (own_segment_prefixes, foreign_segment_prefixes) = segment_prefixes - .into_iter() - .partition::, _>(|(segment_vpc_id, _)| segment_vpc_id == &new_prefix.vpc_id); - - if !foreign_segment_prefixes.is_empty() { - let foreign_segment_prefixes = foreign_segment_prefixes - .into_iter() - .map(|(_, np)| np.prefix); - let foreign_segment_prefixes = itertools::join(foreign_segment_prefixes, ", "); - let msg = format!( - "The requested VPC prefix of {vpc_prefix} conflicts with at \ - least one network segment prefix ({foreign_segment_prefixes}) \ - owned by another VPC", - vpc_prefix = new_prefix.config.prefix, - ); - return Err(CarbideError::InvalidArgument(msg).into()); - } - // We don't need the associated VpcIds anymore, get rid of them. - own_segment_prefixes - .into_iter() - .map(|(_, segment_prefix)| segment_prefix) - .collect() - }; + let segment_prefixes = adoptable_segment_prefixes(segment_prefixes, new_prefix.vpc_id)?; // Check that the network segment prefixes we found can actually fit into // this new VPC prefix container. @@ -279,25 +347,6 @@ pub(crate) async fn create( return Err(CarbideError::InvalidArgument(msg).into()); } - // Check that the network segment prefixes aren't already tied to a VPC - // prefix. This is probably impossible at this point if the DB constraints - // and transactional isolation are working as intended, but better safe - // than sorry. - if let Some((associated_vpc_prefix, segment_prefix)) = segment_prefixes - .iter() - .find_map(|segment_prefix| segment_prefix.vpc_prefix.map(|p| (p, segment_prefix))) - { - let msg = format!( - "The requested VPC prefix ({vpc_prefix}) contains a network \ - segment prefix ({segment_prefix}) which is already associated with \ - another VPC prefix ({associated_vpc_prefix}). If you see this \ - error message, please file a bug!", - vpc_prefix = new_prefix.config.prefix, - segment_prefix = segment_prefix.prefix, - ); - return Err(CarbideError::InvalidArgument(msg).into()); - } - new_prefix .metadata .validate(true) diff --git a/crates/api-core/src/tests/vpc_prefix.rs b/crates/api-core/src/tests/vpc_prefix.rs index 0bb4f14542..de93f79d23 100644 --- a/crates/api-core/src/tests/vpc_prefix.rs +++ b/crates/api-core/src/tests/vpc_prefix.rs @@ -15,11 +15,19 @@ * limitations under the License. */ +use std::collections::HashMap; +use std::time::Duration; + +use carbide_uuid::network::NetworkSegmentId; use carbide_uuid::site_prefix::SitePrefixId; use carbide_uuid::vpc::{VpcId, VpcPrefixId}; use config_version::ConfigVersion; use ipnetwork::IpNetwork; use model::metadata::Metadata as ModelMetadata; +use model::network_prefix::NewNetworkPrefix; +use model::network_segment::{ + NetworkSegmentControllerState, NetworkSegmentType, NewNetworkSegment, +}; use model::site_prefix::{ NewTenantManagedSitePrefix, RetireTenantManagedSitePrefix, SitePrefixAuthority, SitePrefixLifecycleState, SitePrefixRoutingScope, @@ -31,9 +39,10 @@ use rpc::forge::{ PrefixMatchType, VpcDeletionRequest, VpcPrefixCreationRequest, VpcPrefixDeletionRequest, VpcPrefixSearchQuery, }; -use sqlx::PgPool; +use sqlx::{PgPool, PgTransaction}; use tonic::Request; +use crate::cfg::file::{FnnConfig, FnnRoutingProfileConfig, VpcIsolationBehaviorType}; use crate::network_segment::allocate::PrefixAllocator; use crate::test_support::network_segment::FIXTURE_TENANT_ORG_ID; use crate::tests::common::api_fixtures::instance::{ @@ -96,18 +105,25 @@ async fn seed_tenant_managed_site_prefix( site_prefix_id } -/// Creates an FNN VPC owned by one already-persisted tenant. -async fn create_fnn_vpc_for_tenant(env: &TestEnv, tenant: &str, name: &str) -> VpcId { +/// Test-specific function that creates an FNN VPC for an existing tenant. +async fn create_fnn_vpc_for_tenant( + env: &TestEnv, + tenant: &str, + name: &str, + routing_profile_type: Option<&str>, +) -> VpcId { + let mut request = VpcCreationRequest::builder(tenant.to_owned()) + .metadata(Metadata { + name: name.to_owned(), + ..Default::default() + }) + .network_virtualization_type(rpc::forge::VpcVirtualizationType::Fnn as i32); + if let Some(routing_profile_type) = routing_profile_type { + request = request.routing_profile_type(routing_profile_type.to_string()); + } + env.api - .create_vpc( - VpcCreationRequest::builder(tenant.to_owned()) - .metadata(Metadata { - name: name.to_owned(), - ..Default::default() - }) - .network_virtualization_type(rpc::forge::VpcVirtualizationType::Fnn as i32) - .tonic_request(), - ) + .create_vpc(request.tonic_request()) .await .expect("FNN VPC fixture should be created") .into_inner() @@ -137,6 +153,516 @@ fn site_prefix_child_request( } } +/// Test-specific function that configures overlap checks with the requested +/// site gate. +fn tenant_prefix_overlap_overrides(site_gate_enabled: bool) -> TestEnvOverrides { + let mut config = crate::test_support::default_config::get(); + config.tenant_prefix_overlap_enabled = site_gate_enabled; + config.vpc_isolation_behavior = VpcIsolationBehaviorType::MutualIsolation; + + let mut overrides = TestEnvOverrides::with_config(config).with_fnn_config(Some(FnnConfig { + admin_vpc: None, + common_internal_route_target: None, + additional_route_target_imports: vec![], + routing_profiles: HashMap::from([( + "OVERLAP".to_string(), + FnnRoutingProfileConfig { + tenant_prefix_overlap_eligible: true, + internal: Some(true), + ..Default::default() + }, + )]), + use_vpc_vrf_loopback: false, + })); + overrides.create_network_segments = Some(false); + overrides.site_prefixes = Some(vec!["10.0.0.0/8".parse().unwrap()]); + overrides +} + +/// Test-specific function that creates a tenant allowed to request the overlap profile. +async fn create_overlap_tenant(env: &TestEnv, organization_id: &str) -> Result<(), tonic::Status> { + env.api + .create_tenant(Request::new(rpc::forge::CreateTenantRequest { + organization_id: organization_id.to_string(), + routing_profile_type: Some("OVERLAP".to_string()), + metadata: Some(Metadata { + name: organization_id.to_string(), + ..Default::default() + }), + })) + .await?; + Ok(()) +} + +/// Test-specific function that persists a VpcPrefix while the returned transaction holds the lock. +async fn hold_vpc_prefix_create<'a>( + env: &'a TestEnv, + id: VpcPrefixId, + vpc_id: VpcId, + site_prefix_id: SitePrefixId, + prefix: &str, +) -> Result<(PgTransaction<'a>, i32), Box> { + let mut txn = env.pool.begin().await?; + db::tenant_prefix_overlap::lock_checks(&mut txn).await?; + let blocker_pid = sqlx::query_scalar("SELECT pg_backend_pid()") + .fetch_one(&mut *txn) + .await?; + let vpc = db::vpc::find_by_with_lock( + txn.as_mut(), + db::ObjectColumnFilter::One(db::vpc::IdColumn, &vpc_id), + db::vpc::VpcRowLock::Mutation, + ) + .await? + .pop() + .expect("VPC should exist"); + db::vpc_prefix::persist( + NewVpcPrefix { + id, + site_prefix_id: Some(site_prefix_id), + vpc_id, + config: VpcPrefixConfig { + prefix: prefix.parse()?, + }, + metadata: ModelMetadata { + name: format!("held VPC prefix {prefix}"), + ..Default::default() + }, + }, + vpc.version, + &mut txn, + ) + .await?; + Ok((txn, blocker_pid)) +} + +/// Test-specific function that persists an attached segment while the returned +/// transaction holds the lock. +async fn hold_attached_segment_create<'a>( + env: &'a TestEnv, + vpc_id: VpcId, + prefix: &str, +) -> Result<(PgTransaction<'a>, i32), Box> { + let mut txn = env.pool.begin().await?; + db::tenant_prefix_overlap::lock_checks(&mut txn).await?; + let blocker_pid = sqlx::query_scalar("SELECT pg_backend_pid()") + .fetch_one(&mut *txn) + .await?; + db::network_segment::persist( + NewNetworkSegment { + id: NetworkSegmentId::new(), + name: format!("held segment {prefix}"), + subdomain_id: None, + vpc_id: Some(vpc_id), + mtu: 1500, + prefixes: vec![NewNetworkPrefix { + prefix: prefix.parse()?, + gateway: None, + dhcpv6_link_address: None, + num_reserved: 1, + }], + vlan_id: None, + vni: None, + segment_type: NetworkSegmentType::Tenant, + can_stretch: None, + allocation_strategy: Default::default(), + infer_slaac_eui64_addresses: false, + }, + &mut txn, + NetworkSegmentControllerState::Ready, + ) + .await?; + Ok((txn, blocker_pid)) +} + +/// Test-specific function that builds an attached segment with one direct prefix. +fn attached_segment_request( + id: NetworkSegmentId, + vpc_id: VpcId, + prefix: &str, + gateway: &str, + segment_type: rpc::forge::NetworkSegmentType, +) -> rpc::forge::NetworkSegmentCreationRequest { + rpc::forge::NetworkSegmentCreationRequest { + id: Some(id), + name: format!("attached segment {prefix}"), + subdomain_id: None, + vpc_id: Some(vpc_id), + mtu: Some(1500), + prefixes: vec![rpc::forge::NetworkPrefix { + id: None, + prefix: prefix.to_string(), + gateway: Some(gateway.to_string()), + reserve_first: 1, + free_ip_count: 0, + svi_ip: None, + free_ip_count_v2: None, + free_ip_count_saturated: false, + }], + segment_type: segment_type as i32, + infer_slaac_eui64_addresses: false, + } +} + +/// Test-specific function that builds an unattached HostInband segment with one direct prefix. +fn unattached_host_inband_segment_request( + id: NetworkSegmentId, + prefix: &str, + gateway: &str, +) -> rpc::forge::NetworkSegmentCreationRequest { + rpc::forge::NetworkSegmentCreationRequest { + id: Some(id), + name: format!("unattached segment {prefix}"), + subdomain_id: None, + vpc_id: None, + mtu: Some(1500), + prefixes: vec![rpc::forge::NetworkPrefix { + id: None, + prefix: prefix.to_string(), + gateway: Some(gateway.to_string()), + reserve_first: 1, + free_ip_count: 0, + svi_ip: None, + free_ip_count_v2: None, + free_ip_count_saturated: false, + }], + segment_type: rpc::forge::NetworkSegmentType::HostInband as i32, + infer_slaac_eui64_addresses: false, + } +} + +/// Test-specific function that checks an eligible pair reaches the existing database exclusion. +#[crate::sqlx_test] +async fn eligible_exact_overlap_reaches_legacy_database_exclusion( + pool: PgPool, +) -> Result<(), Box> { + let env = create_test_env_with_overrides(pool, tenant_prefix_overlap_overrides(true)).await; + let tenant_a = "overlap-eligible-a"; + let tenant_b = "overlap-eligible-b"; + create_overlap_tenant(&env, tenant_a).await?; + create_overlap_tenant(&env, tenant_b).await?; + let vpc_a = create_fnn_vpc_for_tenant(&env, tenant_a, "overlap VPC A", Some("OVERLAP")).await; + let vpc_b = create_fnn_vpc_for_tenant(&env, tenant_b, "overlap VPC B", Some("OVERLAP")).await; + let root_a = seed_tenant_managed_site_prefix( + &env, + tenant_a, + "10.100.0.0/16", + SitePrefixLifecycleState::Ready, + ) + .await; + let root_b = seed_tenant_managed_site_prefix( + &env, + tenant_b, + "10.100.0.0/16", + SitePrefixLifecycleState::Ready, + ) + .await; + + env.api + .create_vpc_prefix(Request::new(site_prefix_child_request( + VpcPrefixId::new(), + vpc_a, + Some(root_a), + "10.100.1.0/24", + ))) + .await?; + let error = env + .api + .create_vpc_prefix(Request::new(site_prefix_child_request( + VpcPrefixId::new(), + vpc_b, + Some(root_b), + "10.100.1.0/24", + ))) + .await + .expect_err("the legacy database exclusion should still block exact reuse"); + + assert_eq!(error.code(), tonic::Code::InvalidArgument); + assert!( + error + .message() + .contains("overlaps an existing or deleting VPC prefix"), + "the pair check should accept the pair before persistence: {error}" + ); + Ok(()) +} + +/// Test-specific function that checks VpcPrefix create rereads after a concurrent commit. +#[crate::sqlx_test] +async fn vpc_prefix_create_rechecks_after_concurrent_vpc_prefix_commit( + pool: PgPool, +) -> Result<(), Box> { + let env = create_test_env_with_overrides(pool, tenant_prefix_overlap_overrides(true)).await; + let tenant_a = "serialized-vpc-prefix-a"; + let tenant_b = "serialized-vpc-prefix-b"; + create_overlap_tenant(&env, tenant_a).await?; + create_overlap_tenant(&env, tenant_b).await?; + let vpc_a = + create_fnn_vpc_for_tenant(&env, tenant_a, "serialized VPC A", Some("OVERLAP")).await; + let vpc_b = + create_fnn_vpc_for_tenant(&env, tenant_b, "serialized VPC B", Some("OVERLAP")).await; + let root_a = seed_tenant_managed_site_prefix( + &env, + tenant_a, + "10.101.0.0/16", + SitePrefixLifecycleState::Ready, + ) + .await; + let root_b = seed_tenant_managed_site_prefix( + &env, + tenant_b, + "10.101.0.0/16", + SitePrefixLifecycleState::Ready, + ) + .await; + + let first_id = VpcPrefixId::new(); + let (first_create, blocker_pid) = + hold_vpc_prefix_create(&env, first_id, vpc_a, root_a, "10.101.1.0/24").await?; + let second_id = VpcPrefixId::new(); + let second_create = env + .api + .create_vpc_prefix(Request::new(site_prefix_child_request( + second_id, + vpc_b, + Some(root_b), + "10.101.1.0/25", + ))); + let release_first = async { + wait_for_blocked_query(&env.pool, blocker_pid, "tenant_prefix_overlap:checks").await; + first_create.commit().await?; + Ok::<(), Box>(()) + }; + + let (second_result, release_result) = tokio::join!(second_create, release_first); + release_result?; + let error = second_result.expect_err("the nested overlap should be rejected after reread"); + assert_eq!(error.code(), tonic::Code::InvalidArgument); + assert!(error.message().contains("not eligible for reuse")); + assert!(!error.message().contains("10.101.1.0/24")); + assert_eq!(stored_vpc_prefix_count(&env, first_id).await, 1); + assert_eq!(stored_vpc_prefix_count(&env, second_id).await, 0); + Ok(()) +} + +/// Test-specific function that checks VpcPrefix and NetworkSegment writes in both commit orders. +#[crate::sqlx_test] +async fn vpc_prefix_and_attached_segment_recheck_both_commit_orders( + pool: PgPool, +) -> Result<(), Box> { + let env = create_test_env_with_overrides(pool, tenant_prefix_overlap_overrides(false)).await; + let tenant_a = "serialized-cross-table-a"; + let tenant_b = "serialized-cross-table-b"; + create_overlap_tenant(&env, tenant_a).await?; + create_overlap_tenant(&env, tenant_b).await?; + let vpc_a = + create_fnn_vpc_for_tenant(&env, tenant_a, "cross-table VPC A", Some("OVERLAP")).await; + let vpc_b = + create_fnn_vpc_for_tenant(&env, tenant_b, "cross-table VPC B", Some("OVERLAP")).await; + let root_a = seed_tenant_managed_site_prefix( + &env, + tenant_a, + "10.0.0.0/8", + SitePrefixLifecycleState::Ready, + ) + .await; + let root_b = seed_tenant_managed_site_prefix( + &env, + tenant_b, + "10.0.0.0/8", + SitePrefixLifecycleState::Ready, + ) + .await; + + // A committed VPC prefix must be visible when an attached segment resumes. + let (prefix_create, prefix_blocker_pid) = + hold_vpc_prefix_create(&env, VpcPrefixId::new(), vpc_a, root_a, "10.102.1.0/24").await?; + let rejected_segment_id = NetworkSegmentId::new(); + let segment_create = env + .api + .create_network_segment(Request::new(attached_segment_request( + rejected_segment_id, + vpc_b, + "10.102.1.0/24", + "10.102.1.1", + rpc::forge::NetworkSegmentType::Tenant, + ))); + let release_prefix = async { + wait_for_blocked_query( + &env.pool, + prefix_blocker_pid, + "tenant_prefix_overlap:checks", + ) + .await; + prefix_create.commit().await?; + Ok::<(), Box>(()) + }; + let (segment_result, release_result) = tokio::join!(segment_create, release_prefix); + release_result?; + let error = segment_result.expect_err("the attached segment overlap should be rejected"); + assert_eq!(error.code(), tonic::Code::InvalidArgument); + assert!(error.message().contains("not eligible for reuse")); + let segment_count: i64 = + sqlx::query_scalar("SELECT COUNT(*) FROM network_segments WHERE id = $1") + .bind(rejected_segment_id) + .fetch_one(&env.pool) + .await?; + assert_eq!(segment_count, 0); + + // A committed attached segment must be visible when a VPC prefix resumes. + let (segment_create, segment_blocker_pid) = + hold_attached_segment_create(&env, vpc_a, "10.103.1.0/24").await?; + let rejected_prefix_id = VpcPrefixId::new(); + let prefix_create = env + .api + .create_vpc_prefix(Request::new(site_prefix_child_request( + rejected_prefix_id, + vpc_b, + Some(root_b), + "10.103.1.0/24", + ))); + let release_segment = async { + wait_for_blocked_query( + &env.pool, + segment_blocker_pid, + "tenant_prefix_overlap:checks", + ) + .await; + segment_create.commit().await?; + Ok::<(), Box>(()) + }; + let (prefix_result, release_result) = tokio::join!(prefix_create, release_segment); + release_result?; + let error = prefix_result.expect_err("the VPC prefix overlap should be rejected"); + assert_eq!(error.code(), tonic::Code::InvalidArgument); + assert!(error.message().contains("not eligible for reuse")); + assert_eq!(stored_vpc_prefix_count(&env, rejected_prefix_id).await, 0); + + Ok(()) +} + +/// Test-specific function that checks unattached create skips the lock and attach rereads. +#[crate::sqlx_test] +async fn unattached_segment_bypasses_overlap_lock_and_attach_rechecks( + pool: PgPool, +) -> Result<(), Box> { + let env = create_test_env_with_overrides(pool, tenant_prefix_overlap_overrides(false)).await; + let tenant = "serialized-segment-attach"; + create_overlap_tenant(&env, tenant).await?; + let prefix_vpc = create_fnn_vpc_for_tenant(&env, tenant, "prefix VPC", Some("OVERLAP")).await; + let (attach_vpc, _) = api_fixtures::vpc::create_flat_vpc( + &env, + "segment attach VPC".to_string(), + Some(tenant.to_string()), + ) + .await; + let root = seed_tenant_managed_site_prefix( + &env, + tenant, + "10.104.0.0/16", + SitePrefixLifecycleState::Ready, + ) + .await; + let (prefix_create, blocker_pid) = + hold_vpc_prefix_create(&env, VpcPrefixId::new(), prefix_vpc, root, "10.104.1.0/24").await?; + + let segment_id = NetworkSegmentId::new(); + tokio::time::timeout( + Duration::from_secs(5), + env.api + .create_network_segment(Request::new(unattached_host_inband_segment_request( + segment_id, + "10.104.1.0/24", + "10.104.1.1", + ))), + ) + .await??; + + let attach = env.api.attach_network_segment_to_vpc(Request::new( + rpc::forge::AttachNetworkSegmentToVpcRequest { + network_segment_id: Some(segment_id), + vpc_id: Some(attach_vpc), + allow_replace: false, + }, + )); + let release_prefix = async { + wait_for_blocked_query(&env.pool, blocker_pid, "tenant_prefix_overlap:checks").await; + prefix_create.commit().await?; + Ok::<(), Box>(()) + }; + let (attach_result, release_result) = tokio::join!(attach, release_prefix); + release_result?; + let error = attach_result.expect_err("the overlapping attachment should be rejected"); + assert_eq!(error.code(), tonic::Code::InvalidArgument); + assert!(error.message().contains("not eligible for reuse")); + + let attached_vpc: Option = + sqlx::query_scalar("SELECT vpc_id FROM network_segments WHERE id = $1") + .bind(segment_id) + .fetch_one(&env.pool) + .await?; + assert_eq!(attached_vpc, None); + Ok(()) +} + +/// Test-specific function that checks soft-deleted Admin prefixes still block +/// `VpcPrefix` creation. +#[crate::sqlx_test] +async fn vpc_prefix_create_rejects_soft_deleted_admin_segment_prefix( + pool: PgPool, +) -> Result<(), Box> { + let env = create_test_env_with_overrides(pool, tenant_prefix_overlap_overrides(false)).await; + let tenant = "soft-deleted-admin-segment"; + create_overlap_tenant(&env, tenant).await?; + let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "soft-delete VPC", Some("OVERLAP")).await; + let root_id = seed_tenant_managed_site_prefix( + &env, + tenant, + "10.105.0.0/16", + SitePrefixLifecycleState::Ready, + ) + .await; + + let segment_id = NetworkSegmentId::new(); + env.api + .create_network_segment(Request::new(attached_segment_request( + segment_id, + vpc_id, + "10.105.1.0/24", + "10.105.1.1", + rpc::forge::NetworkSegmentType::Admin, + ))) + .await?; + env.api + .delete_network_segment(Request::new(rpc::forge::NetworkSegmentDeletionRequest { + id: Some(segment_id), + })) + .await?; + + let segment_is_deleted: bool = + sqlx::query_scalar("SELECT deleted IS NOT NULL FROM network_segments WHERE id = $1") + .bind(segment_id) + .fetch_one(&env.pool) + .await?; + assert!(segment_is_deleted); + + let prefix_id = VpcPrefixId::new(); + let error = env + .api + .create_vpc_prefix(Request::new(site_prefix_child_request( + prefix_id, + vpc_id, + Some(root_id), + "10.105.1.0/24", + ))) + .await + .expect_err("the soft-deleted Admin prefix should remain reserved"); + + assert_eq!(error.code(), tonic::Code::InvalidArgument); + assert!(error.message().contains("not eligible for reuse")); + assert_eq!(stored_vpc_prefix_count(&env, prefix_id).await, 0); + Ok(()) +} + #[derive(serde::Deserialize)] struct LifecycleStateJson { state: String, @@ -1139,7 +1665,7 @@ async fn exact_site_prefix_attachment_enforces_lineage_and_round_trips( let other_tenant = "site-prefix-tenant-b"; create_fixture_tenant(&env, tenant).await?; create_fixture_tenant(&env, other_tenant).await?; - let fnn_vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "tenant-root FNN VPC").await; + let fnn_vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "tenant-root FNN VPC", None).await; let ready_parent = seed_tenant_managed_site_prefix( &env, @@ -1374,7 +1900,7 @@ async fn omitted_site_prefix_id_checks_only_the_vpc_tenant( let other_tenant = "legacy-prefix-tenant-b"; create_fixture_tenant(&env, tenant).await?; create_fixture_tenant(&env, other_tenant).await?; - let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "legacy tenant-scoped VPC").await; + let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "legacy tenant-scoped VPC", None).await; seed_tenant_managed_site_prefix( &env, @@ -1447,7 +1973,7 @@ async fn omitted_site_prefix_id_serializes_with_tenant_root_creation( .await; let tenant = "legacy-prefix-creation-race"; create_fixture_tenant(&env, tenant).await?; - let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "legacy creation race VPC").await; + let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "legacy creation race VPC", None).await; let mut site_prefix_create = env.pool.begin().await?; let blocker_pid: i32 = sqlx::query_scalar("SELECT pg_backend_pid()") @@ -1497,7 +2023,7 @@ async fn omitted_site_prefix_id_serializes_with_first_operator_root_reconciliati .await; let tenant = "legacy-operator-creation-race"; create_fixture_tenant(&env, tenant).await?; - let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "legacy operator race VPC").await; + let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "legacy operator race VPC", None).await; let operator_root: IpNetwork = "198.19.0.0/16".parse()?; let mut reconciliation = env.pool.begin().await?; @@ -1532,7 +2058,7 @@ async fn vpc_prefix_create_rechecks_parent_after_concurrent_retirement( let env = create_test_env(pool).await; let tenant = "site-prefix-retirement-race"; create_fixture_tenant(&env, tenant).await?; - let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "retirement race VPC").await; + let vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "retirement race VPC", None).await; let site_prefix_id = seed_tenant_managed_site_prefix( &env, tenant, @@ -1587,7 +2113,8 @@ async fn tenant_managed_lineage_blocks_virtualization_transition_and_race( let tenant = "site-prefix-virtualization-guard"; create_fixture_tenant(&env, tenant).await?; - let existing_vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "existing lineage VPC").await; + let existing_vpc_id = + create_fnn_vpc_for_tenant(&env, tenant, "existing lineage VPC", None).await; let existing_parent_id = seed_tenant_managed_site_prefix( &env, tenant, @@ -1639,7 +2166,7 @@ async fn tenant_managed_lineage_blocks_virtualization_transition_and_race( .unwrap_err(); assert_eq!(error.code(), tonic::Code::FailedPrecondition); - let racing_vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "racing lineage VPC").await; + let racing_vpc_id = create_fnn_vpc_for_tenant(&env, tenant, "racing lineage VPC", None).await; let racing_parent_id = seed_tenant_managed_site_prefix( &env, tenant, diff --git a/crates/api-db/src/lib.rs b/crates/api-db/src/lib.rs index f523cb6637..3c1bee85b8 100644 --- a/crates/api-db/src/lib.rs +++ b/crates/api-db/src/lib.rs @@ -98,6 +98,7 @@ pub mod switch; pub mod tenant; pub mod tenant_identity_config; pub mod tenant_keyset; +pub mod tenant_prefix_overlap; pub mod trim_table; pub mod vpc; pub mod vpc_dpu_loopback; diff --git a/crates/api-db/src/tenant_prefix_overlap.rs b/crates/api-db/src/tenant_prefix_overlap.rs new file mode 100644 index 0000000000..be2af8f587 --- /dev/null +++ b/crates/api-db/src/tenant_prefix_overlap.rs @@ -0,0 +1,94 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +//! Database coordination for tenant prefix overlap checks. + +use sqlx::PgTransaction; + +use crate::{DatabaseError, DatabaseResult}; + +/// `lock_checks` serializes participating transactions that could create an +/// overlap between a `VpcPrefix` and another stored prefix. +/// +/// Callers acquire it before any resource lock, then read, validate, and write +/// in the same transaction. Otherwise, two requests can each see no overlap +/// and both commit. PostgreSQL releases the lock on commit or rollback. +pub async fn lock_checks(txn: &mut PgTransaction<'_>) -> DatabaseResult<()> { + let query = "SELECT pg_advisory_xact_lock(\ + hashtextextended('tenant_prefix_overlap:checks', 0))"; + sqlx::query(query) + .execute(&mut **txn) + .await + .map(|_| ()) + .map_err(|error| DatabaseError::query(query, error)) +} + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use sqlx::PgPool; + + use super::*; + + /// Test-specific function that checks rollback releases the overlap lock. + #[crate::sqlx_test] + async fn rollback_releases_overlap_checks_lock( + pool: PgPool, + ) -> Result<(), Box> { + let mut holder = pool.begin().await?; + lock_checks(&mut holder).await?; + let holder_pid: i32 = sqlx::query_scalar("SELECT pg_backend_pid()") + .fetch_one(&mut *holder) + .await?; + + let mut waiter = pool.begin().await?; + let waiter_pid: i32 = sqlx::query_scalar("SELECT pg_backend_pid()") + .fetch_one(&mut *waiter) + .await?; + + let wait_for_lock = async { + tokio::time::timeout(Duration::from_secs(5), lock_checks(&mut waiter)).await??; + waiter.commit().await?; + Ok::<(), Box>(()) + }; + let release_lock = async { + tokio::time::timeout(Duration::from_secs(5), async { + loop { + let blocked_by_holder: bool = + sqlx::query_scalar("SELECT $1 = ANY(pg_blocking_pids($2))") + .bind(holder_pid) + .bind(waiter_pid) + .fetch_one(&pool) + .await?; + if blocked_by_holder { + return Ok::<(), sqlx::Error>(()); + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await??; + holder.rollback().await?; + Ok::<(), Box>(()) + }; + + let (waiter_result, holder_result) = tokio::join!(wait_for_lock, release_lock); + waiter_result?; + holder_result?; + Ok(()) + } +} diff --git a/crates/api-db/src/vpc_prefix.rs b/crates/api-db/src/vpc_prefix.rs index f80b741494..ae7e5c722e 100644 --- a/crates/api-db/src/vpc_prefix.rs +++ b/crates/api-db/src/vpc_prefix.rs @@ -24,6 +24,7 @@ use ipnetwork::IpNetwork; use model::DeletedFilter; use model::controller_outcome::PersistentStateHandlerOutcome; use model::network_prefix::NetworkPrefix; +use model::network_segment::NetworkSegmentType; use model::site_prefix::SitePrefixAuthority; use model::vpc_prefix::{ DeleteVpcPrefix, NewVpcPrefix, UpdateVpcPrefix, VpcPrefix, VpcPrefixControllerState, @@ -462,25 +463,45 @@ pub async fn probe( .map_err(|e| DatabaseError::query(query, e)) } -// Given a new VPC prefix which has been not been persisted yet, find the -// network segment prefixes that overlap with it, along with the VPC ID each -// one is associated with. The caller should use this information to reject -// any problematic VPC prefixes, and to update any matching segment prefixes -// which should be adopted by the new VPC prefix. +/// `AttachedSegmentPrefix` identifies a `NetworkPrefix` linked directly to a +/// `NetworkSegment` that has a VPC. +#[derive(Debug)] +pub struct AttachedSegmentPrefix { + /// The `NetworkSegment` belongs to this VPC. + pub vpc_id: VpcId, + /// `VpcPrefix` creation uses this `NetworkSegment` type to decide whether + /// it may adopt the prefix. + pub segment_type: NetworkSegmentType, + /// This is the `NetworkPrefix` stored directly on the `NetworkSegment`. + pub prefix: NetworkPrefix, +} + +/// `probe_segment_prefixes` finds direct `NetworkPrefix` records on attached +/// `NetworkSegment` records that overlap `network`. +/// +/// Soft-deleted segments remain visible because their `NetworkPrefix` rows stay +/// in the database until final deletion. A `NetworkPrefix` linked to a +/// `VpcPrefix` is checked through that `VpcPrefix` instead, so this query does +/// not return it. pub async fn probe_segment_prefixes( network: IpNetwork, txn: &mut PgConnection, -) -> Result, DatabaseError> { - let query = "SELECT ns.vpc_id AS vpc_id, np.* FROM network_prefixes np \ +) -> Result, DatabaseError> { + let query = "SELECT ns.vpc_id AS vpc_id, ns.network_segment_type, np.* \ + FROM network_prefixes np \ INNER JOIN network_segments ns ON np.segment_id = ns.id \ - WHERE np.prefix && $1 AND ns.network_segment_type='tenant'"; + WHERE np.prefix && $1 \ + AND ns.vpc_id IS NOT NULL \ + AND np.vpc_prefix_id IS NULL"; sqlx::query(query) .bind(network) .try_map(|row| { - let vpc_id: VpcId = row.try_get("vpc_id")?; - let network_prefix = NetworkPrefix::from_row(&row)?; - Ok((vpc_id, network_prefix)) + Ok(AttachedSegmentPrefix { + vpc_id: row.try_get("vpc_id")?, + segment_type: row.try_get("network_segment_type")?, + prefix: NetworkPrefix::from_row(&row)?, + }) }) .fetch_all(txn) .await