From 389206fc2ac2a7a08b77c3d0251b628109b81859 Mon Sep 17 00:00:00 2001 From: David Crespo <david-crespo@users.noreply.github.com> Date: Wed, 20 Mar 2024 08:39:21 -0500 Subject: [PATCH] IP range add can only be IPv4 (#5107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #5085 - ~~Change range add request body from `IpRange` to `Ipv4Range`, see what breaks~~ - [x] Error on Ipv6 ranges in the service layer - [x] Update tests and add new tests - ~~May change range remove body to `Ipv4Range` for consistency even though it is technically fine as long as add is restricted to `Ipv4` — need to think about it~~ decided not to so we can still remove any existing IPv6 ranges through the API --- nexus/src/app/ip_pool.rs | 26 ++++ nexus/src/external_api/http_entrypoints.rs | 4 + nexus/tests/integration_tests/ip_pools.rs | 151 +++++++++++---------- openapi/nexus.json | 2 + 4 files changed, 108 insertions(+), 75 deletions(-) diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 87a7d98c91..fd73a18355 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -27,6 +27,7 @@ use omicron_common::api::external::NameOrId; use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; use ref_cast::RefCast; +use std::matches; use uuid::Uuid; /// Helper to make it easier to 404 on attempts to manipulate internal pools @@ -291,6 +292,19 @@ impl super::Nexus { return Err(not_found_from_lookup(pool_lookup)); } + // Disallow V6 ranges until IPv6 is fully supported by the networking + // subsystem. Instead of changing the API to reflect that (making this + // endpoint inconsistent with the rest) and changing it back when we + // add support, we accept them at the API layer and error here. It + // would be nice if we could do it in the datastore layer, but we'd + // have no way of creating IPv6 ranges for the purpose of testing IP + // pool utilization. + if matches!(range, IpRange::V6(_)) { + return Err(Error::invalid_request( + "IPv6 ranges are not allowed yet", + )); + } + self.db_datastore.ip_pool_add_range(opctx, &authz_pool, range).await } @@ -347,6 +361,18 @@ impl super::Nexus { let (authz_pool, ..) = self.db_datastore.ip_pools_service_lookup(opctx).await?; opctx.authorize(authz::Action::Modify, &authz_pool).await?; + // Disallow V6 ranges until IPv6 is fully supported by the networking + // subsystem. Instead of changing the API to reflect that (making this + // endpoint inconsistent with the rest) and changing it back when we + // add support, we accept them at the API layer and error here. It + // would be nice if we could do it in the datastore layer, but we'd + // have no way of creating IPv6 ranges for the purpose of testing IP + // pool utilization. + if matches!(range, IpRange::V6(_)) { + return Err(Error::invalid_request( + "IPv6 ranges are not allowed yet", + )); + } self.db_datastore.ip_pool_add_range(opctx, &authz_pool, range).await } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 94cd35519b..41e5bce08a 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1789,6 +1789,8 @@ async fn ip_pool_range_list( } /// Add range to IP pool +/// +/// IPv6 ranges are not allowed yet. #[endpoint { method = POST, path = "/v1/system/ip-pools/{pool}/ranges/add", @@ -1880,6 +1882,8 @@ async fn ip_pool_service_range_list( } /// Add IP range to Oxide service pool +/// +/// IPv6 ranges are not allowed yet. #[endpoint { method = POST, path = "/v1/system/ip-pools-service/ranges/add", diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index c8390e8ce0..cb5eade735 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -9,6 +9,8 @@ use dropshot::HttpErrorResponseBody; use dropshot::ResultsPage; use http::method::Method; use http::StatusCode; +use nexus_db_queries::authz; +use nexus_db_queries::context::OpContext; use nexus_db_queries::db::datastore::SERVICE_IP_POOL_NAME; use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO; use nexus_db_queries::db::fixed_data::silo::INTERNAL_SILO_ID; @@ -38,7 +40,6 @@ use nexus_types::external_api::params::IpPoolSiloUpdate; use nexus_types::external_api::params::IpPoolUpdate; use nexus_types::external_api::shared::IpRange; use nexus_types::external_api::shared::Ipv4Range; -use nexus_types::external_api::shared::Ipv6Range; use nexus_types::external_api::shared::SiloIdentityMode; use nexus_types::external_api::views::IpPool; use nexus_types::external_api::views::IpPoolRange; @@ -46,7 +47,9 @@ use nexus_types::external_api::views::IpPoolSiloLink; use nexus_types::external_api::views::Silo; use nexus_types::external_api::views::SiloIpPool; use nexus_types::identity::Resource; +use omicron_common::address::Ipv6Range; use omicron_common::api::external::IdentityMetadataUpdateParams; +use omicron_common::api::external::LookupType; use omicron_common::api::external::NameOrId; use omicron_common::api::external::SimpleIdentity; use omicron_common::api::external::{IdentityMetadataCreateParams, Name}; @@ -765,7 +768,7 @@ async fn create_pool(client: &ClientTestContext, name: &str) -> IpPool { async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; - create_pool(client, "p0").await; + let pool = create_pool(client, "p0").await; assert_ip_pool_utilization(client, "p0", 0, 0, 0, 0).await; @@ -783,20 +786,36 @@ async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { assert_ip_pool_utilization(client, "p0", 0, 5, 0, 0).await; - // now let's add a gigantic range just for fun + // Now let's add a gigantic range. This requires direct datastore + // shenanigans because adding IPv6 ranges through the API is currently not + // allowed. It's worth doing because we want this code to correctly handle + // IPv6 ranges when they are allowed again. + + let nexus = &cptestctx.server.apictx().nexus; + let datastore = nexus.datastore(); + let log = cptestctx.logctx.log.new(o!()); + let opctx = OpContext::for_tests(log, datastore.clone()); + let authz_pool = authz::IpPool::new( + authz::FLEET, + pool.identity.id, + LookupType::ByName("p0".to_string()), + ); + let big_range = IpRange::V6( Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1), + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), std::net::Ipv6Addr::new( - 0xfd00, 0, 0, 0, 0xffff, 0xfff, 0xffff, 0xffff, + 0xfd00, 0, 0, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, ), ) .unwrap(), ); - object_create::<IpRange, IpPoolRange>(client, &add_url, &big_range).await; + datastore + .ip_pool_add_range(&opctx, &authz_pool, &big_range) + .await + .expect("could not add range"); - assert_ip_pool_utilization(client, "p0", 0, 5, 0, 18446480190918885375) - .await; + assert_ip_pool_utilization(client, "p0", 0, 5, 0, 2u128.pow(80)).await; } // Data for testing overlapping IP ranges @@ -895,59 +914,9 @@ async fn test_ip_pool_range_overlapping_ranges_fails( }; test_bad_ip_ranges(client, &ip_pool_add_range_url, &ipv4_range).await; - // Test data for IPv6 ranges that should fail due to overlap - let ipv6_range = TestRange { - base_range: IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 20), - ) - .unwrap(), - ), - bad_ranges: vec![ - // The exact same range - IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 20), - ) - .unwrap(), - ), - // Overlaps below - IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 5), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 15), - ) - .unwrap(), - ), - // Overlaps above - IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 15), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 25), - ) - .unwrap(), - ), - // Contains the base range - IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 100), - ) - .unwrap(), - ), - // Contained by the base range - IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 12), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 13), - ) - .unwrap(), - ), - ], - }; - test_bad_ip_ranges(client, &ip_pool_add_range_url, &ipv6_range).await; + // IPv6 tests removed along with support for IPv6 ranges in + // https://github.com/oxidecomputer/omicron/pull/5107 + // Put them back when IPv6 ranges are supported again. } async fn test_bad_ip_ranges( @@ -994,6 +963,38 @@ async fn test_bad_ip_ranges( } } +// Support for IPv6 ranges removed in +// https://github.com/oxidecomputer/omicron/pull/5107 +// Delete this test when we support IPv6 again. +#[nexus_test] +async fn test_ip_pool_range_rejects_v6(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + + create_ip_pool(client, "p0", None).await; + + let range = IpRange::V6( + Ipv6Range::new( + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 20), + ) + .unwrap(), + ); + + let add_url = "/v1/system/ip-pools/p0/ranges/add"; + let error = + object_create_error(client, add_url, &range, StatusCode::BAD_REQUEST) + .await; + + assert_eq!(error.message, "IPv6 ranges are not allowed yet"); + + // same deal with service pool + let add_url = "/v1/system/ip-pools-service/ranges/add"; + let error = + object_create_error(client, add_url, &range, StatusCode::BAD_REQUEST) + .await; + assert_eq!(error.message, "IPv6 ranges are not allowed yet"); +} + #[nexus_test] async fn test_ip_pool_range_pagination(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; @@ -1026,17 +1027,17 @@ async fn test_ip_pool_range_pagination(cptestctx: &ControlPlaneTestContext) { // address, which sorts all IPv4 before IPv6, then within protocol versions // by their first address. let ranges = [ - IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 11), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 20), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 3), + std::net::Ipv4Addr::new(10, 0, 0, 4), ) .unwrap(), ), - IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 5), + std::net::Ipv4Addr::new(10, 0, 0, 6), ) .unwrap(), ), @@ -1304,15 +1305,15 @@ async fn test_ip_pool_service(cptestctx: &ControlPlaneTestContext) { let ranges = [ IpRange::V4( Ipv4Range::new( - std::net::Ipv4Addr::new(10, 0, 0, 1), - std::net::Ipv4Addr::new(10, 0, 0, 2), + std::net::Ipv4Addr::new(10, 0, 0, 3), + std::net::Ipv4Addr::new(10, 0, 0, 4), ) .unwrap(), ), - IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 2), ) .unwrap(), ), diff --git a/openapi/nexus.json b/openapi/nexus.json index 54a2d3250f..5ea1a48d8b 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5432,6 +5432,7 @@ "system/networking" ], "summary": "Add range to IP pool", + "description": "IPv6 ranges are not allowed yet.", "operationId": "ip_pool_range_add", "parameters": [ { @@ -5847,6 +5848,7 @@ "system/networking" ], "summary": "Add IP range to Oxide service pool", + "description": "IPv6 ranges are not allowed yet.", "operationId": "ip_pool_service_range_add", "requestBody": { "content": {