diff --git a/nexus/src/app/vpc_router.rs b/nexus/src/app/vpc_router.rs index 40b4c1de0f..8b18a77c07 100644 --- a/nexus/src/app/vpc_router.rs +++ b/nexus/src/app/vpc_router.rs @@ -17,11 +17,15 @@ use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; +use omicron_common::api::external::IpNet; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::NameOrId; +use omicron_common::api::external::RouteDestination; +use omicron_common::api::external::RouteTarget; use omicron_common::api::external::RouterRouteKind; use omicron_common::api::external::UpdateResult; +use std::net::IpAddr; use uuid::Uuid; impl super::Nexus { @@ -191,8 +195,47 @@ impl super::Nexus { kind: &RouterRouteKind, params: ¶ms::RouterRouteCreate, ) -> CreateResult { - let (.., authz_router) = - router_lookup.lookup_for(authz::Action::CreateChild).await?; + let (.., authz_router, db_router) = + router_lookup.fetch_for(authz::Action::CreateChild).await?; + + if db_router.kind == VpcRouterKind::System { + return Err(Error::invalid_request( + "user-provided routes cannot be added to a system router", + )); + } + + // Validate route destinations/targets at this stage: + // - mixed explicit v4 and v6 are disallowed. + // - users cannot specify 'Vpc' as a custom router dest/target. + // - users cannot specify 'Subnet' as a custom router target. + // - the only internet gateway we support today is 'outbound'. + match (¶ms.destination, ¶ms.target) { + (RouteDestination::Ip(IpAddr::V4(_)), RouteTarget::Ip(IpAddr::V4(_))) + | (RouteDestination::Ip(IpAddr::V6(_)), RouteTarget::Ip(IpAddr::V6(_))) + | (RouteDestination::IpNet(IpNet::V4(_)), RouteTarget::Ip(IpAddr::V4(_))) + | (RouteDestination::IpNet(IpNet::V6(_)), RouteTarget::Ip(IpAddr::V6(_))) => {}, + + (RouteDestination::Ip(_), RouteTarget::Ip(_)) + | (RouteDestination::IpNet(_), RouteTarget::Ip(_)) + => return Err(Error::invalid_request( + "cannot mix explicit IPv4 and IPv6 addresses between destination and target" + )), + + (RouteDestination::Vpc(_), _) | (_, RouteTarget::Vpc(_)) => return Err(Error::invalid_request( + "VPCs cannot be used as a destination or target in custom routers" + )), + + (_, RouteTarget::Subnet(_)) => return Err(Error::invalid_request( + "subnets cannot be used as a target in custom routers" + )), + + (_, RouteTarget::InternetGateway(n)) if n.as_str() != "outbound" => return Err(Error::invalid_request( + "'outbound' is currently the only valid internet gateway" + )), + + _ => {}, + }; + let id = Uuid::new_v4(); let route = db::model::RouterRoute::new( id, diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 57e1113a91..1185c8b9b4 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -39,6 +39,9 @@ use omicron_common::api::external::InstanceCpuCount; use omicron_common::api::external::Ipv4Net; use omicron_common::api::external::Ipv6Net; use omicron_common::api::external::NameOrId; +use omicron_common::api::external::RouteDestination; +use omicron_common::api::external::RouteTarget; +use omicron_common::api::external::RouterRoute; use omicron_common::disk::DiskIdentity; use omicron_sled_agent::sim::SledAgent; use omicron_test_utils::dev::poll::wait_for_condition; @@ -613,6 +616,78 @@ pub async fn create_router( .unwrap() } +pub async fn create_route( + client: &ClientTestContext, + project_name: &str, + vpc_name: &str, + router_name: &str, + route_name: &str, + destination: RouteDestination, + target: RouteTarget, +) -> RouterRoute { + NexusRequest::objects_post( + &client, + format!( + "/v1/vpc-router-routes?project={}&vpc={}&router={}", + &project_name, &vpc_name, &router_name + ) + .as_str(), + ¶ms::RouterRouteCreate { + identity: IdentityMetadataCreateParams { + name: route_name.parse().unwrap(), + description: String::from("route description"), + }, + target, + destination, + }, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap() +} + +#[allow(clippy::too_many_arguments)] +pub async fn create_route_with_error( + client: &ClientTestContext, + project_name: &str, + vpc_name: &str, + router_name: &str, + route_name: &str, + destination: RouteDestination, + target: RouteTarget, + status: StatusCode, +) -> HttpErrorResponseBody { + NexusRequest::new( + RequestBuilder::new( + client, + Method::POST, + format!( + "/v1/vpc-router-routes?project={}&vpc={}&router={}", + &project_name, &vpc_name, &router_name + ) + .as_str(), + ) + .body(Some(¶ms::RouterRouteCreate { + identity: IdentityMetadataCreateParams { + name: route_name.parse().unwrap(), + description: String::from("route description"), + }, + target, + destination, + })) + .expect_status(Some(status)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap() +} + pub async fn assert_ip_pool_utilization( client: &ClientTestContext, pool_name: &str, diff --git a/nexus/tests/integration_tests/router_routes.rs b/nexus/tests/integration_tests/router_routes.rs index a13026a7fd..aa5a6f50c1 100644 --- a/nexus/tests/integration_tests/router_routes.rs +++ b/nexus/tests/integration_tests/router_routes.rs @@ -4,9 +4,12 @@ use dropshot::Method; use http::StatusCode; +use itertools::Itertools; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::identity_eq; +use nexus_test_utils::resource_helpers::create_route; +use nexus_test_utils::resource_helpers::create_route_with_error; use nexus_test_utils::resource_helpers::objects_list_page_authz; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::params; @@ -23,11 +26,15 @@ use nexus_test_utils::resource_helpers::{ create_project, create_router, create_vpc, }; +use crate::integration_tests::vpc_routers::PROJECT_NAME; + type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; #[nexus_test] -async fn test_router_routes(cptestctx: &ControlPlaneTestContext) { +async fn test_router_routes_crud_operations( + cptestctx: &ControlPlaneTestContext, +) { let client = &cptestctx.external_client; let project_name = "springfield-squidport"; @@ -212,3 +219,85 @@ async fn test_router_routes(cptestctx: &ControlPlaneTestContext) { .await .unwrap(); } + +#[nexus_test] +async fn test_router_routes_disallow_mixed_v4_v6( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let _ = create_project(&client, PROJECT_NAME).await; + + let vpc_name = "default"; + let router_name = "routy"; + let _router = + create_router(&client, PROJECT_NAME, vpc_name, router_name).await; + + // Some targets/strings refer to a mixed v4/v6 entity, e.g., + // subnet or instance. Others refer to one kind only (ipnet, ip). + // Users should not be able to mix v4 and v6 in these latter routes + // -- route resolution will ignore them, but a helpful error message + // is more useful. + let dest_set: [RouteDestination; 5] = [ + "ip:4.4.4.4".parse().unwrap(), + "ipnet:4.4.4.0/24".parse().unwrap(), + "ip:2001:4860:4860::8888".parse().unwrap(), + "ipnet:2001:4860:4860::/64".parse().unwrap(), + "subnet:named-subnet".parse().unwrap(), + ]; + + let target_set: [RouteTarget; 5] = [ + "ip:172.30.0.5".parse().unwrap(), + "ip:fd37:faf4:cc25::5".parse().unwrap(), + "instance:named-instance".parse().unwrap(), + "inetgw:outbound".parse().unwrap(), + "drop".parse().unwrap(), + ]; + + for (i, (dest, target)) in dest_set + .into_iter() + .cartesian_product(target_set.into_iter()) + .enumerate() + { + use RouteDestination as Rd; + use RouteTarget as Rt; + let allowed = match (&dest, &target) { + (Rd::Ip(IpAddr::V4(_)), Rt::Ip(IpAddr::V4(_))) + | (Rd::Ip(IpAddr::V6(_)), Rt::Ip(IpAddr::V6(_))) + | (Rd::IpNet(IpNet::V4(_)), Rt::Ip(IpAddr::V4(_))) + | (Rd::IpNet(IpNet::V6(_)), Rt::Ip(IpAddr::V6(_))) => true, + (Rd::Ip(_), Rt::Ip(_)) | (Rd::IpNet(_), Rt::Ip(_)) => false, + _ => true, + }; + + let route_name = format!("test-route-{i}"); + + if allowed { + create_route( + client, + PROJECT_NAME, + vpc_name, + router_name, + &route_name, + dest, + target, + ) + .await; + } else { + let err = create_route_with_error( + client, + PROJECT_NAME, + vpc_name, + router_name, + &route_name, + dest, + target, + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!( + err.message, + "cannot mix explicit IPv4 and IPv6 addresses between destination and target" + ); + } + } +} diff --git a/nexus/tests/integration_tests/vpc_routers.rs b/nexus/tests/integration_tests/vpc_routers.rs index ab9d620d3e..8d6bd14bf7 100644 --- a/nexus/tests/integration_tests/vpc_routers.rs +++ b/nexus/tests/integration_tests/vpc_routers.rs @@ -26,13 +26,13 @@ use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::Ipv4Net; use omicron_common::api::external::NameOrId; -const PROJECT_NAME: &str = "os-cartographers"; +pub const PROJECT_NAME: &str = "os-cartographers"; type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; #[nexus_test] -async fn test_vpc_routers(cptestctx: &ControlPlaneTestContext) { +async fn test_vpc_routers_crud_operations(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; // Create a project that we'll use for testing. @@ -46,8 +46,7 @@ async fn test_vpc_routers(cptestctx: &ControlPlaneTestContext) { format!("/v1/vpc-routers?project={}&vpc={}", PROJECT_NAME, vpc_name); // get routers should have only the system router created w/ the VPC - let routers = - objects_list_page_authz::(client, &routers_url).await.items; + let routers = list_routers(client, &vpc_name).await; assert_eq!(routers.len(), 1); assert_eq!(routers[0].kind, VpcRouterKind::System); @@ -91,7 +90,7 @@ async fn test_vpc_routers(cptestctx: &ControlPlaneTestContext) { routers_eq(&router, &same_router); // routers list should now have the one in it - let routers = objects_list_page_authz(client, &routers_url).await.items; + let routers = list_routers(client, &vpc_name).await; assert_eq!(routers.len(), 2); routers_eq(&routers[0], &router); @@ -143,8 +142,7 @@ async fn test_vpc_routers(cptestctx: &ControlPlaneTestContext) { assert_eq!(router2.kind, VpcRouterKind::Custom); // routers list should now have two custom and one system - let routers = - objects_list_page_authz::(client, &routers_url).await.items; + let routers = list_routers(client, &vpc_name).await; assert_eq!(routers.len(), 3); routers_eq(&routers[0], &router); routers_eq(&routers[1], &router2); @@ -204,8 +202,7 @@ async fn test_vpc_routers(cptestctx: &ControlPlaneTestContext) { assert_eq!(&updated_router.identity.description, "another description"); // fetching list should show updated one - let routers = - objects_list_page_authz::(client, &routers_url).await.items; + let routers = list_routers(client, &vpc_name).await; assert_eq!(routers.len(), 3); routers_eq(&routers[0], &updated_router); @@ -217,8 +214,7 @@ async fn test_vpc_routers(cptestctx: &ControlPlaneTestContext) { .unwrap(); // routers list should now have two again, one system and one custom - let routers = - objects_list_page_authz::(client, &routers_url).await.items; + let routers = list_routers(client, &vpc_name).await; assert_eq!(routers.len(), 2); routers_eq(&routers[0], &router2); @@ -266,32 +262,22 @@ async fn test_vpc_routers(cptestctx: &ControlPlaneTestContext) { async fn test_vpc_routers_attach_to_subnet( cptestctx: &ControlPlaneTestContext, ) { - // XXX: really clean this up. let client = &cptestctx.external_client; - // --- - // XX: copied from above - // - // Create a project that we'll use for testing. // This includes the vpc 'default'. let _ = create_project(&client, PROJECT_NAME).await; + let vpc_name = "default"; let subnet_name = "default"; - let routers_url = - format!("/v1/vpc-routers?project={}&vpc={}", PROJECT_NAME, vpc_name); let subnets_url = format!("/v1/vpc-subnets?project={}&vpc={}", PROJECT_NAME, vpc_name); // get routers should have only the system router created w/ the VPC - let routers = - objects_list_page_authz::(client, &routers_url).await.items; + let routers = list_routers(client, vpc_name).await; assert_eq!(routers.len(), 1); assert_eq!(routers[0].kind, VpcRouterKind::System); - // - // XX: copied from above - // --- // Create a custom router for later use. let router_name = "routy"; @@ -433,13 +419,12 @@ async fn test_vpc_routers_custom_route_at_instance( ) { let _client = &cptestctx.external_client; - // Attempting to delete a system router should fail. + // Installing a custom router onto a subnet with a live instance + // should install routes at that sled. - // Attempting to add a new route to a system router should fail. + // Swapping router should change the installed routes at that sled. - // Attempting to modify/delete a VPC subnet route should fail. - - // Modifying the target of a Default (gateway) route should succeed. + // Unsetting a router should remove affected non-system routes. todo!() } @@ -476,7 +461,7 @@ async fn test_vpc_routers_internet_gateway_target( } #[nexus_test] -async fn test_vpc_routers_disallowed_custom_targets( +async fn test_vpc_routers_disallow_custom_targets( cptestctx: &ControlPlaneTestContext, ) { let _client = &cptestctx.external_client; @@ -509,6 +494,16 @@ async fn set_custom_router( .await } +async fn list_routers( + client: &ClientTestContext, + vpc_name: &str, +) -> Vec { + let routers_url = + format!("/v1/vpc-routers?project={}&vpc={}", PROJECT_NAME, vpc_name); + let out = objects_list_page_authz::(client, &routers_url).await; + out.items +} + fn routers_eq(sn1: &VpcRouter, sn2: &VpcRouter) { identity_eq(&sn1.identity, &sn2.identity); assert_eq!(sn1.vpc_id, sn2.vpc_id);