From b2db78fb260f2d9ad470fc9bdfd9004dff7bba5d Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Mon, 6 Jun 2022 18:41:20 +0000 Subject: [PATCH 1/6] Add IP Pools and contained IP ranges - Adds basic CRUD API for IP Pools, as named, global resources - Adds table and database operations for IP Pools - Adds API for operating on IP ranges within an IP Pool. These are expressed as a first/last IP address pair, and describe a range of addresses to add or remove from a pool. Currently, all ranges must be non-overlapping, which is checked at insertion time with a custom query. Deleting a range currently requires specifying the exact same first/last address used to create it. Both of these could be relaxed in the future without changing the API, for example to support splitting or coalescing ranges. - Adds paginated endpoint for listing the ranges within a pool, ordered by the first address in the range. - Adds integration tests for new endpoints. - Adds unauthorized coverage checks. --- common/src/api/external/mod.rs | 204 +++++++ common/src/sql/dbinit.sql | 113 +++- nexus/src/app/ip_pool.rs | 128 +++++ nexus/src/app/mod.rs | 1 + nexus/src/authz/api_resources.rs | 80 +++ nexus/src/authz/omicron.polar | 19 + nexus/src/authz/oso_generic.rs | 2 + nexus/src/db/datastore.rs | 275 +++++++++ nexus/src/db/lookup.rs | 23 + nexus/src/db/model/ip_pool.rs | 124 +++++ nexus/src/db/model/mod.rs | 2 + nexus/src/db/multi_table_object.rs | 212 +++++++ nexus/src/db/queries/ip_pool.rs | 288 ++++++++++ nexus/src/db/queries/mod.rs | 1 + nexus/src/db/schema.rs | 27 + nexus/src/external_api/http_entrypoints.rs | 256 +++++++++ nexus/src/external_api/params.rs | 44 ++ nexus/src/external_api/tag-config.json | 6 + nexus/src/external_api/views.rs | 42 ++ nexus/tests/integration_tests/endpoints.rs | 84 +++ nexus/tests/integration_tests/ip_pools.rs | 520 ++++++++++++++++++ nexus/tests/integration_tests/mod.rs | 1 + nexus/tests/integration_tests/unauthorized.rs | 5 + nexus/tests/output/nexus_tags.txt | 11 + openapi/nexus.json | 503 +++++++++++++++++ 25 files changed, 2945 insertions(+), 26 deletions(-) create mode 100644 nexus/src/app/ip_pool.rs create mode 100644 nexus/src/db/model/ip_pool.rs create mode 100644 nexus/src/db/multi_table_object.rs create mode 100644 nexus/src/db/queries/ip_pool.rs create mode 100644 nexus/tests/integration_tests/ip_pools.rs diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index dbd99f2137..a0cb5fa794 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -525,6 +525,7 @@ pub enum ResourceType { Disk, Image, Instance, + IpPool, NetworkInterface, Rack, Service, @@ -1123,6 +1124,51 @@ pub enum IpNet { V6(Ipv6Net), } +impl IpNet { + /// Return the first address in this subnet + pub fn first_address(&self) -> IpAddr { + match self { + IpNet::V4(inner) => IpAddr::from(inner.iter().next().unwrap()), + IpNet::V6(inner) => IpAddr::from(inner.iter().next().unwrap()), + } + } + + /// Return the last address in this subnet. + /// + /// For a subnet of size 1, e.g., a /32, this is the same as the first + /// address. + // NOTE: This is a workaround for the fact that the `ipnetwork` crate's + // iterator provides only the `Iterator::next()` method. That means that + // finding the last address is linear in the size of the subnet, which is + // completely untenable and totally avoidable with some addition. In the + // long term, we should either put up a patch to the `ipnetwork` crate or + // move the `ipnet` crate, which does provide an efficient iterator + // implementation. + pub fn last_address(&self) -> IpAddr { + match self { + IpNet::V4(inner) => { + let base: u32 = inner.network().into(); + let size = inner.size() - 1; + std::net::IpAddr::V4(std::net::Ipv4Addr::from(base + size)) + } + IpNet::V6(inner) => { + let base: u128 = inner.network().into(); + let size = inner.size() - 1; + std::net::IpAddr::V6(std::net::Ipv6Addr::from(base + size)) + } + } + } +} + +impl From for IpNet { + fn from(n: ipnetwork::IpNetwork) -> Self { + match n { + ipnetwork::IpNetwork::V4(v4) => IpNet::V4(Ipv4Net(v4)), + ipnetwork::IpNetwork::V6(v6) => IpNet::V6(Ipv6Net(v6)), + } + } +} + impl From for IpNet { fn from(n: Ipv4Net) -> IpNet { IpNet::V4(n) @@ -1258,6 +1304,116 @@ fn label_schema( .into() } +/// An IP Range is a contiguous range of IP addresses, usually within an IP +/// Pool. +#[derive(Clone, Copy, Debug, Deserialize, Serialize)] +pub enum IpRange { + V4(Ipv4Range), + V6(Ipv6Range), +} + +impl JsonSchema for IpRange { + fn schema_name() -> String { + "IpRange".to_string() + } + + fn json_schema( + gen: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + schemars::schema::SchemaObject { + metadata: Some( + schemars::schema::Metadata { ..Default::default() }.into(), + ), + subschemas: Some( + schemars::schema::SubschemaValidation { + one_of: Some(vec![ + label_schema("v4", gen.subschema_for::()), + label_schema("v6", gen.subschema_for::()), + ]), + ..Default::default() + } + .into(), + ), + ..Default::default() + } + .into() + } +} + +impl IpRange { + pub fn first_address(&self) -> IpAddr { + match self { + IpRange::V4(inner) => IpAddr::from(inner.first), + IpRange::V6(inner) => IpAddr::from(inner.first), + } + } + + pub fn last_address(&self) -> IpAddr { + match self { + IpRange::V4(inner) => IpAddr::from(inner.last), + IpRange::V6(inner) => IpAddr::from(inner.last), + } + } +} + +/// A non-decreasing IPv4 address range, inclusive of both ends. +/// +/// The first address must be less than or equal to the last address. +#[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] +#[serde(try_from = "AnyIpv4Range")] +pub struct Ipv4Range { + pub first: Ipv4Addr, + pub last: Ipv4Addr, +} + +#[derive(Clone, Copy, Debug, Deserialize)] +struct AnyIpv4Range { + first: Ipv4Addr, + last: Ipv4Addr, +} + +impl TryFrom for Ipv4Range { + type Error = Error; + fn try_from(r: AnyIpv4Range) -> Result { + if r.first <= r.last { + Ok(Self { first: r.first, last: r.last }) + } else { + Err(Error::invalid_request( + "IP address ranges must be non-decreasing", + )) + } + } +} + +/// A non-decreasing IPv6 address range, inclusive of both ends. +/// +/// The first address must be less than or equal to the last address. +#[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] +#[serde(try_from = "AnyIpv6Range")] +pub struct Ipv6Range { + pub first: Ipv6Addr, + pub last: Ipv6Addr, +} + +#[derive(Clone, Copy, Debug, Deserialize)] +struct AnyIpv6Range { + first: Ipv6Addr, + last: Ipv6Addr, +} + +impl TryFrom for Ipv6Range { + type Error = Error; + fn try_from(r: AnyIpv6Range) -> Result { + if r.first <= r.last { + Ok(Self { first: r.first, last: r.last }) + } else { + Err(Error::invalid_request( + "IP address ranges must be non-decreasing", + )) + } + } +} + /// A `RouteTarget` describes the possible locations that traffic matching a /// route destination can be sent. #[derive( @@ -2449,4 +2605,52 @@ mod test { let net_des = serde_json::from_str::(&ser).unwrap(); assert_eq!(net, net_des); } + + #[test] + fn test_ipnet_first_last_address() { + use std::net::IpAddr; + use std::net::Ipv4Addr; + use std::net::Ipv6Addr; + let net: IpNet = "fd00::/128".parse().unwrap(); + assert_eq!( + net.first_address(), + IpAddr::from(Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0)), + ); + assert_eq!( + net.last_address(), + IpAddr::from(Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0)), + ); + + let net: IpNet = "fd00::/64".parse().unwrap(); + assert_eq!( + net.first_address(), + IpAddr::from(Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0)), + ); + assert_eq!( + net.last_address(), + IpAddr::from(Ipv6Addr::new( + 0xfd00, 0, 0, 0, 0xffff, 0xffff, 0xffff, 0xffff + )), + ); + + let net: IpNet = "10.0.0.0/16".parse().unwrap(); + assert_eq!( + net.first_address(), + IpAddr::from(Ipv4Addr::new(10, 0, 0, 0)), + ); + assert_eq!( + net.last_address(), + IpAddr::from(Ipv4Addr::new(10, 0, 255, 255)), + ); + + let net: IpNet = "10.0.0.0/32".parse().unwrap(); + assert_eq!( + net.first_address(), + IpAddr::from(Ipv4Addr::new(10, 0, 0, 0)), + ); + assert_eq!( + net.last_address(), + IpAddr::from(Ipv4Addr::new(10, 0, 0, 0)), + ); + } } diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 3944b3fd46..39ab59a7c8 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -827,32 +827,6 @@ STORING (vpc_id, subnet_id, is_primary) WHERE time_deleted IS NULL; - -CREATE TYPE omicron.public.vpc_router_kind AS ENUM ( - 'system', - 'custom' -); - -CREATE TABLE omicron.public.vpc_router ( - /* Identity metadata (resource) */ - id UUID PRIMARY KEY, - name STRING(63) NOT NULL, - description STRING(512) NOT NULL, - time_created TIMESTAMPTZ NOT NULL, - time_modified TIMESTAMPTZ NOT NULL, - /* Indicates that the object has been deleted */ - time_deleted TIMESTAMPTZ, - kind omicron.public.vpc_router_kind NOT NULL, - vpc_id UUID NOT NULL, - rcgen INT NOT NULL -); - -CREATE UNIQUE INDEX ON omicron.public.vpc_router ( - vpc_id, - name -) WHERE - time_deleted IS NULL; - CREATE TYPE omicron.public.vpc_firewall_rule_status AS ENUM ( 'disabled', 'enabled' @@ -904,6 +878,31 @@ CREATE UNIQUE INDEX ON omicron.public.vpc_firewall_rule ( ) WHERE time_deleted IS NULL; +CREATE TYPE omicron.public.vpc_router_kind AS ENUM ( + 'system', + 'custom' +); + +CREATE TABLE omicron.public.vpc_router ( + /* Identity metadata (resource) */ + id UUID PRIMARY KEY, + name STRING(63) NOT NULL, + description STRING(512) NOT NULL, + time_created TIMESTAMPTZ NOT NULL, + time_modified TIMESTAMPTZ NOT NULL, + /* Indicates that the object has been deleted */ + time_deleted TIMESTAMPTZ, + kind omicron.public.vpc_router_kind NOT NULL, + vpc_id UUID NOT NULL, + rcgen INT NOT NULL +); + +CREATE UNIQUE INDEX ON omicron.public.vpc_router ( + vpc_id, + name +) WHERE + time_deleted IS NULL; + CREATE TYPE omicron.public.router_route_kind AS ENUM ( 'default', 'vpc_subnet', @@ -933,6 +932,68 @@ CREATE UNIQUE INDEX ON omicron.public.router_route ( ) WHERE time_deleted IS NULL; +/* + * An IP Pool, a collection of zero or more IP ranges for external IPs. + */ +CREATE TABLE omicron.public.ip_pool ( + /* Resource identity metadata */ + id UUID PRIMARY KEY, + name STRING(63) NOT NULL, + description STRING(512) NOT NULL, + time_created TIMESTAMPTZ NOT NULL, + time_modified TIMESTAMPTZ NOT NULL, + time_deleted TIMESTAMPTZ, + + /* The collection's child-resource generation number */ + rcgen INT8 NOT NULL + + /* + * TODO-completeness: These will need some enum describing what the pool can + * be used for. That can currently be: + * - Routing + * - Oxide public use, e.g., Nexus's public API or the console + * - Virtual machine instance NAT + */ +); + +/* + * Index ensuring uniqueness of IP Pool names, globally. + */ +CREATE UNIQUE INDEX ON omicron.public.ip_pool ( + name +) WHERE + time_deleted IS NULL; + +/* + * IP Pools are made up of a set of IP ranges, which are start/stop addresses. + * Note that these need not be CIDR blocks or well-behaved subnets with a + * specific netmask. + */ +CREATE TABLE omicron.public.ip_pool_range ( + id UUID PRIMARY KEY, + time_created TIMESTAMPTZ NOT NULL, + time_modified TIMESTAMPTZ NOT NULL, + time_deleted TIMESTAMPTZ, + first_address INET NOT NULL, + last_address INET NOT NULL, + ip_pool_id UUID NOT NULL +); + +/* + * These indexes are currently used to enforce that the ranges within an IP Pool + * do not overlap with any other ranges. + */ +CREATE UNIQUE INDEX ON omicron.public.ip_pool_range ( + first_address +) +STORING (last_address) +WHERE time_deleted IS NULL; +CREATE UNIQUE INDEX ON omicron.public.ip_pool_range ( + last_address +) +STORING (first_address) +WHERE time_deleted IS NULL; + /*******************************************************************/ /* diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs new file mode 100644 index 0000000000..956a7396ce --- /dev/null +++ b/nexus/src/app/ip_pool.rs @@ -0,0 +1,128 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! IP Pools, collections of external IP addresses for guest instances + +use crate::authz; +use crate::context::OpContext; +use crate::db; +use crate::db::lookup::LookupPath; +use crate::db::model::Name; +use crate::external_api::params; +use omicron_common::api::external::CreateResult; +use omicron_common::api::external::DataPageParams; +use omicron_common::api::external::DeleteResult; +use omicron_common::api::external::IpRange; +use omicron_common::api::external::ListResultVec; +use omicron_common::api::external::LookupResult; +use omicron_common::api::external::UpdateResult; +use std::net::IpAddr; +use uuid::Uuid; + +impl super::Nexus { + pub async fn ip_pool_create( + &self, + opctx: &OpContext, + new_pool: ¶ms::IpPoolCreate, + ) -> CreateResult { + self.db_datastore.ip_pool_create(opctx, new_pool).await + } + + pub async fn ip_pools_list_by_name( + &self, + opctx: &OpContext, + pagparams: &DataPageParams<'_, Name>, + ) -> ListResultVec { + self.db_datastore.ip_pools_list_by_name(opctx, pagparams).await + } + + pub async fn ip_pools_list_by_id( + &self, + opctx: &OpContext, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + self.db_datastore.ip_pools_list_by_id(opctx, pagparams).await + } + + pub async fn ip_pool_fetch( + &self, + opctx: &OpContext, + pool_name: &Name, + ) -> LookupResult { + let (.., db_pool) = LookupPath::new(opctx, &self.db_datastore) + .ip_pool_name(pool_name) + .fetch() + .await?; + Ok(db_pool) + } + + pub async fn ip_pool_delete( + &self, + opctx: &OpContext, + pool_name: &Name, + ) -> DeleteResult { + let (.., authz_pool, db_pool) = + LookupPath::new(opctx, &self.db_datastore) + .ip_pool_name(pool_name) + .fetch_for(authz::Action::Delete) + .await?; + self.db_datastore.ip_pool_delete(opctx, &authz_pool, &db_pool).await + } + + pub async fn ip_pool_update( + &self, + opctx: &OpContext, + pool_name: &Name, + updates: ¶ms::IpPoolUpdate, + ) -> UpdateResult { + let (.., authz_pool) = LookupPath::new(opctx, &self.db_datastore) + .ip_pool_name(pool_name) + .lookup_for(authz::Action::Modify) + .await?; + self.db_datastore + .ip_pool_update(opctx, &authz_pool, updates.clone().into()) + .await + } + + pub async fn ip_pool_list_ranges( + &self, + opctx: &OpContext, + pool_name: &Name, + pagparams: &DataPageParams<'_, IpAddr>, + ) -> ListResultVec { + let (.., authz_pool) = LookupPath::new(opctx, &self.db_datastore) + .ip_pool_name(pool_name) + .lookup_for(authz::Action::ListChildren) + .await?; + self.db_datastore + .ip_pool_list_ranges(opctx, &authz_pool, pagparams) + .await + } + + pub async fn ip_pool_add_range( + &self, + opctx: &OpContext, + pool_name: &Name, + range: &IpRange, + ) -> UpdateResult { + let (.., authz_pool) = LookupPath::new(opctx, &self.db_datastore) + .ip_pool_name(pool_name) + .lookup_for(authz::Action::Modify) + .await?; + self.db_datastore.ip_pool_add_range(opctx, &authz_pool, range).await + } + + pub async fn ip_pool_delete_range( + &self, + opctx: &OpContext, + pool_name: &Name, + range: &IpRange, + ) -> DeleteResult { + let (.., authz_pool) = LookupPath::new(opctx, &self.db_datastore) + .ip_pool_name(pool_name) + .lookup_for(authz::Action::Modify) + .await?; + self.db_datastore.ip_pool_delete_range(opctx, &authz_pool, range).await + } +} diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index 1cb1f6b6ff..0407c91d74 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -24,6 +24,7 @@ mod disk; mod iam; mod image; mod instance; +mod ip_pool; mod organization; mod oximeter; mod project; diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index 486414dd1c..3383b0897f 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -359,6 +359,68 @@ impl AuthorizedResource for GlobalImageList { } } +#[derive(Clone, Copy, Debug)] +pub struct IpPoolList; + +/// Singleton representing the [`IpPoolList`] itself for authz purposes +pub const IP_POOL_LIST: IpPoolList = IpPoolList; + +impl Eq for IpPoolList {} + +impl PartialEq for IpPoolList { + fn eq(&self, _: &Self) -> bool { + true + } +} + +impl oso::PolarClass for IpPoolList { + fn get_polar_class_builder() -> oso::ClassBuilder { + oso::Class::builder() + .with_equality_check() + .add_attribute_getter("fleet", |_: &IpPoolList| FLEET) + } +} + +impl AuthorizedResource for IpPoolList { + fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>( + &'a self, + opctx: &'b OpContext, + datastore: &'c DataStore, + authn: &'d authn::Context, + roleset: &'e mut RoleSet, + ) -> futures::future::BoxFuture<'f, Result<(), Error>> + where + 'a: 'f, + 'b: 'f, + 'c: 'f, + 'd: 'f, + 'e: 'f, + { + // There are no roles on the IpPoolList, only permissions. But we still + // need to load the Fleet-related roles to verify that the actor has the + // "admin" role on the Fleet. + load_roles_for_resource( + opctx, + datastore, + authn, + ResourceType::Fleet, + *FLEET_ID, + roleset, + ) + .boxed() + } + + fn on_unauthorized( + &self, + _: &Authz, + error: Error, + _: AnyActor, + _: Action, + ) -> Error { + error + } +} + // Main resource hierarchy: Organizations, Projects, and their resources authz_resource! { @@ -674,6 +736,24 @@ authz_resource! { polar_snippet = FleetChild, } +authz_resource! { + name = "IpPool", + parent = "Fleet", + primary_key = Uuid, + roles_allowed = false, + polar_snippet = FleetChild, +} + +/* +authz_resource! { + name = "IpPoolCidrBlock", + parent = "IpPool", + primary_key = Uuid, + roles_allowed = false, + polar_snippet = Custom, +} +*/ + #[cfg(test)] mod test { use super::FleetRole; diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 5c862517aa..a3db822b8c 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -304,6 +304,25 @@ has_relation(silo: Silo, "parent_silo", saml_identity_provider: SamlIdentityProv # Fleet. None of these resources defines their own roles. # +# Describes the policy for accessing "/ip-pools" in the API +resource IpPoolList { + permissions = [ + "list_children", + "modify", + "create_child", + ]; + + # Fleet Administrators can create or modify the IP Pools list. + relations = { parent_fleet: Fleet }; + "modify" if "admin" on "parent_fleet"; + "create_child" if "admin" on "parent_fleet"; + + # Fleet Viewers can list IP Pools + "list_children" if "viewer" on "parent_fleet"; +} +has_relation(fleet: Fleet, "parent_fleet", ip_pool_list: IpPoolList) + if ip_pool_list.fleet = fleet; + # Describes the policy for accessing "/images" (in the API) resource GlobalImageList { permissions = [ diff --git a/nexus/src/authz/oso_generic.rs b/nexus/src/authz/oso_generic.rs index 3c814b8ceb..e0abc0283b 100644 --- a/nexus/src/authz/oso_generic.rs +++ b/nexus/src/authz/oso_generic.rs @@ -43,6 +43,7 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result { AuthenticatedActor::get_polar_class(), Database::get_polar_class(), Fleet::get_polar_class(), + IpPoolList::get_polar_class(), GlobalImageList::get_polar_class(), ConsoleSessionList::get_polar_class(), ]; @@ -57,6 +58,7 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result { Project::init(), Disk::init(), Instance::init(), + IpPool::init(), NetworkInterface::init(), Vpc::init(), VpcRouter::init(), diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 499eee458b..29af343da3 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -39,8 +39,12 @@ use crate::db::fixed_data::silo::DEFAULT_SILO; use crate::db::lookup::LookupPath; use crate::db::model::DatabaseString; use crate::db::model::IncompleteVpc; +use crate::db::model::IpPool; +use crate::db::model::IpPoolRange; +use crate::db::model::IpPoolUpdate; use crate::db::model::NetworkInterfaceUpdate; use crate::db::model::Vpc; +use crate::db::queries::ip_pool::FilterOverlappingIpRanges; use crate::db::queries::network_interface; use crate::db::queries::vpc::InsertVpcQuery; use crate::db::queries::vpc_subnet::FilterConflictingVpcSubnetRangesQuery; @@ -81,6 +85,7 @@ use omicron_common::api::external; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; +use omicron_common::api::external::IpRange; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::LookupType; @@ -1028,6 +1033,276 @@ impl DataStore { }) } + // IP Pools + + /// List IP Pools by their name + pub async fn ip_pools_list_by_name( + &self, + opctx: &OpContext, + pagparams: &DataPageParams<'_, Name>, + ) -> ListResultVec { + use db::schema::ip_pool::dsl; + opctx + .authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST) + .await?; + paginated(dsl::ip_pool, dsl::name, pagparams) + .filter(dsl::time_deleted.is_null()) + .select(db::model::IpPool::as_select()) + .get_results_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + } + + /// List IP Pools by their IDs + pub async fn ip_pools_list_by_id( + &self, + opctx: &OpContext, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + use db::schema::ip_pool::dsl; + opctx + .authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST) + .await?; + paginated(dsl::ip_pool, dsl::id, pagparams) + .filter(dsl::time_deleted.is_null()) + .select(db::model::IpPool::as_select()) + .get_results_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + } + + pub async fn ip_pool_create( + &self, + opctx: &OpContext, + new_pool: ¶ms::IpPoolCreate, + ) -> CreateResult { + use db::schema::ip_pool::dsl; + opctx + .authorize(authz::Action::CreateChild, &authz::IP_POOL_LIST) + .await?; + let pool = IpPool::new(&new_pool.identity); + let pool_name = pool.name().as_str().to_string(); + diesel::insert_into(dsl::ip_pool) + .values(pool) + .on_conflict(dsl::id) + .do_nothing() + .returning(IpPool::as_returning()) + .get_result_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::Conflict(ResourceType::IpPool, &pool_name), + ) + }) + } + + pub async fn ip_pool_delete( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + db_pool: &IpPool, + ) -> DeleteResult { + use db::schema::ip_pool::dsl; + use db::schema::ip_pool_range; + opctx.authorize(authz::Action::Delete, authz_pool).await?; + + // Verify there are no IP ranges still in this pool + let range = diesel_pool_result_optional( + ip_pool_range::dsl::ip_pool_range + .filter(ip_pool_range::dsl::ip_pool_id.eq(authz_pool.id())) + .filter(ip_pool_range::dsl::time_deleted.is_null()) + .select(ip_pool_range::dsl::id) + .limit(1) + .first_async::(self.pool_authorized(opctx).await?) + .await, + ) + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))?; + if range.is_some() { + return Err(Error::InvalidRequest { + message: + "IP Pool cannot be deleted while it contains IP ranges" + .to_string(), + }); + } + + // Delete the pool, conditional on the rcgen not having changed. This + // protects the delete from occuring if clients created a new IP range + // in between the above check for children and this query. + let now = Utc::now(); + let updated_rows = diesel::update(dsl::ip_pool) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::id.eq(authz_pool.id())) + .filter(dsl::rcgen.eq(db_pool.rcgen)) + .set(dsl::time_deleted.eq(now)) + .execute_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByResource(authz_pool), + ) + })?; + + if updated_rows == 0 { + return Err(Error::InvalidRequest { + message: "deletion failed due to concurrent modification" + .to_string(), + }); + } + Ok(()) + } + + pub async fn ip_pool_update( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + updates: IpPoolUpdate, + ) -> UpdateResult { + use db::schema::ip_pool::dsl; + opctx.authorize(authz::Action::Modify, authz_pool).await?; + diesel::update(dsl::ip_pool) + .filter(dsl::id.eq(authz_pool.id())) + .filter(dsl::time_deleted.is_null()) + .set(updates) + .returning(IpPool::as_returning()) + .get_result_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByResource(authz_pool), + ) + }) + } + + pub async fn ip_pool_list_ranges( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + pag_params: &DataPageParams<'_, std::net::IpAddr>, + ) -> ListResultVec { + use db::schema::ip_pool_range::dsl; + opctx.authorize(authz::Action::ListChildren, authz_pool).await?; + let limit = pag_params.limit.get().into(); + let res = if let Some(last_seen_addr) = pag_params.marker { + let addr = ipnetwork::IpNetwork::from(*last_seen_addr); + dsl::ip_pool_range + .filter(dsl::ip_pool_id.eq(authz_pool.id())) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::first_address.gt(addr)) + .order(dsl::first_address) + .limit(limit) + .select(IpPoolRange::as_select()) + .get_results_async(self.pool_authorized(opctx).await?) + } else { + dsl::ip_pool_range + .filter(dsl::ip_pool_id.eq(authz_pool.id())) + .filter(dsl::time_deleted.is_null()) + .order(dsl::first_address) + .limit(limit) + .select(IpPoolRange::as_select()) + .get_results_async(self.pool_authorized(opctx).await?) + }; + res.await.map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByResource(authz_pool), + ) + }) + } + + pub async fn ip_pool_add_range( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + range: &IpRange, + ) -> CreateResult { + use db::schema::ip_pool_range::dsl; + opctx.authorize(authz::Action::CreateChild, authz_pool).await?; + let pool_id = authz_pool.id(); + let new_range = IpPoolRange::new(range, pool_id); + let filter_subquery = FilterOverlappingIpRanges { range: new_range }; + let insert_query = + diesel::insert_into(dsl::ip_pool_range).values(filter_subquery); + IpPool::insert_resource(pool_id, insert_query) + .insert_and_get_result_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| { + use async_bb8_diesel::ConnectionError::Query; + use async_bb8_diesel::PoolError::Connection; + use diesel::result::Error::NotFound; + + match e { + AsyncInsertError::DatabaseError(Connection(Query( + NotFound, + ))) => { + // We've filtered out the IP addresses the client provided, + // i.e., there's some overlap with existing addresses. + Error::invalid_request( + format!( + "The provided IP range {}-{} overlaps with \ + an existing range", + range.first_address(), + range.last_address(), + ) + .as_str(), + ) + } + AsyncInsertError::CollectionNotFound => { + Error::ObjectNotFound { + type_name: ResourceType::IpPool, + lookup_type: LookupType::ById(pool_id), + } + } + AsyncInsertError::DatabaseError(err) => { + public_error_from_diesel_pool(err, ErrorHandler::Server) + } + } + }) + } + + pub async fn ip_pool_delete_range( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + range: &IpRange, + ) -> DeleteResult { + use db::schema::ip_pool_range::dsl; + opctx.authorize(authz::Action::Modify, authz_pool).await?; + let now = Utc::now(); + + // We can just delete the range, provided that it the exact first/last + // address pair exists. We are guaranteed that concurrent modifications + // don't affect this, since this query will be serialized with any to + // insert a new range, which must be non-overlapping. + let first_address = range.first_address(); + let last_address = range.last_address(); + let first_net = ipnetwork::IpNetwork::from(first_address); + let last_net = ipnetwork::IpNetwork::from(last_address); + let updated_rows = diesel::update(dsl::ip_pool_range) + .filter(dsl::first_address.eq(first_net)) + .filter(dsl::last_address.eq(last_net)) + .filter(dsl::time_deleted.is_null()) + .set(dsl::time_deleted.eq(now)) + .execute_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel_pool(e, ErrorHandler::Server) + })?; + if updated_rows == 1 { + Ok(()) + } else { + Err(Error::invalid_request( + format!( + "The provided range {}-{} does not exist", + first_address, last_address, + ) + .as_str(), + )) + } + } + // Instances /// Idempotently insert a database record for an Instance diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 22f3feb5e8..7b6df46900 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -226,6 +226,20 @@ impl<'a> LookupPath<'a> { } } + /// Select a resource of type IpPool, identified by its name + pub fn ip_pool_name<'b, 'c>(self, name: &'b Name) -> IpPool<'c> + where + 'a: 'c, + 'b: 'c, + { + IpPool { key: IpPoolKey::Name(Root { lookup_root: self }, name) } + } + + /// Select a resource of type IpPool, identified by its id + pub fn ip_pool_id(self, id: Uuid) -> IpPool<'a> { + IpPool { key: IpPoolKey::PrimaryKey(Root { lookup_root: self }, id) } + } + /// Select a resource of type Disk, identified by its id pub fn disk_id(self, id: Uuid) -> Disk<'a> { Disk { key: DiskKey::PrimaryKey(Root { lookup_root: self }, id) } @@ -429,6 +443,15 @@ lookup_resource! { ] } +lookup_resource! { + name = "IpPool", + ancestors = [], + children = [], + lookup_by_name = true, + soft_deletes = true, + primary_key_columns = [ { column_name = "id", rust_type = Uuid }] +} + lookup_resource! { name = "SamlIdentityProvider", ancestors = [ "Silo" ], diff --git a/nexus/src/db/model/ip_pool.rs b/nexus/src/db/model/ip_pool.rs new file mode 100644 index 0000000000..aa793464fb --- /dev/null +++ b/nexus/src/db/model/ip_pool.rs @@ -0,0 +1,124 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Model types for IP Pools and the CIDR blocks therein. + +use crate::db::collection_insert::DatastoreCollection; +use crate::db::model::Name; +use crate::db::schema::ip_pool; +use crate::db::schema::ip_pool_range; +use crate::external_api::params; +use chrono::DateTime; +use chrono::Utc; +use db_macros::Resource; +use diesel::Selectable; +use ipnetwork::IpNetwork; +use omicron_common::api::external; +use std::net::IpAddr; +use uuid::Uuid; + +/// An IP Pool is a collection of IP addresses external to the rack. +#[derive(Queryable, Insertable, Selectable, Clone, Debug, Resource)] +#[diesel(table_name = ip_pool)] +pub struct IpPool { + #[diesel(embed)] + pub identity: IpPoolIdentity, + + /// Child resource generation number, for optimistic concurrency control of + /// the contained ranges. + pub rcgen: i64, +} + +impl IpPool { + pub fn new(pool_identity: &external::IdentityMetadataCreateParams) -> Self { + Self { + identity: IpPoolIdentity::new( + Uuid::new_v4(), + pool_identity.clone(), + ), + rcgen: 0, + } + } +} + +/// A set of updates to an IP Pool +#[derive(AsChangeset)] +#[diesel(table_name = ip_pool)] +pub struct IpPoolUpdate { + pub name: Option, + pub description: Option, + pub time_modified: DateTime, +} + +impl From for IpPoolUpdate { + fn from(params: params::IpPoolUpdate) -> Self { + Self { + name: params.identity.name.map(|n| n.into()), + description: params.identity.description, + time_modified: Utc::now(), + } + } +} + +/// A range of IP addresses for an IP Pool. +#[derive(Queryable, Insertable, Selectable, Clone, Debug)] +#[diesel(table_name = ip_pool_range)] +pub struct IpPoolRange { + pub id: Uuid, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, + pub first_address: IpNetwork, + pub last_address: IpNetwork, + pub ip_pool_id: Uuid, +} + +impl IpPoolRange { + pub fn new(range: &external::IpRange, ip_pool_id: Uuid) -> Self { + let now = Utc::now(); + let first_address = range.first_address(); + let last_address = range.last_address(); + assert!( + last_address >= first_address, + "Address ranges must be non-decreasing" + ); + Self { + id: Uuid::new_v4(), + time_created: now, + time_modified: now, + time_deleted: None, + first_address: IpNetwork::from(range.first_address()), + last_address: IpNetwork::from(range.last_address()), + ip_pool_id, + } + } +} + +impl Into for IpPoolRange { + fn into(self) -> external::IpRange { + match (self.first_address.ip(), self.last_address.ip()) { + (IpAddr::V4(first), IpAddr::V4(last)) => { + external::IpRange::V4(external::Ipv4Range { first, last }) + } + (IpAddr::V6(first), IpAddr::V6(last)) => { + external::IpRange::V6(external::Ipv6Range { first, last }) + } + (first, last) => { + unreachable!( + "Expected first/last address of an IP range to \ + both be of the same protocol version, but first = {:?} \ + and last = {:?}", + first, last + ); + } + } + } +} + +impl DatastoreCollection for IpPool { + type CollectionId = uuid::Uuid; + type GenerationNumberColumn = ip_pool::dsl::rcgen; + type CollectionTimeDeletedColumn = ip_pool::dsl::time_deleted; + type CollectionIdColumn = ip_pool_range::dsl::ip_pool_id; +} diff --git a/nexus/src/db/model/mod.rs b/nexus/src/db/model/mod.rs index 03cad918d2..c5785c120c 100644 --- a/nexus/src/db/model/mod.rs +++ b/nexus/src/db/model/mod.rs @@ -19,6 +19,7 @@ mod image; mod instance; mod instance_cpu_count; mod instance_state; +mod ip_pool; mod ipv4net; mod ipv6net; mod l4_port_range; @@ -69,6 +70,7 @@ pub use image::*; pub use instance::*; pub use instance_cpu_count::*; pub use instance_state::*; +pub use ip_pool::*; pub use ipv4net::*; pub use ipv6net::*; pub use l4_port_range::*; diff --git a/nexus/src/db/multi_table_object.rs b/nexus/src/db/multi_table_object.rs new file mode 100644 index 0000000000..08bc188449 --- /dev/null +++ b/nexus/src/db/multi_table_object.rs @@ -0,0 +1,212 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// Copyright 2022 Oxide Computer Company + +//! Tools for working with a multi-table object in an atomic fashion. +//! +//! Database normalization is the process of structuring tables to reduce +//! duplication and increase integrity or validity checking. A very common +//! example is something like an `Employee` table, with a `Manager` column. In +//! the case where multiple people report to the same manager, those rows will +//! contain the same name or entry in the `Manager` column. Normalizing these +//! tables involes creating a new `Manager` table, and referring to the primary +//! key of that table in the `Employee` table. This enables storing all the +//! information about the manager in its own table, and referring to all that +//! data from the employee, by foreign key. +//! +//! There are several objects in the Oxide API where such a representation is +//! important. A good example is a VPC Firewall. This resource has its own ID, +//! name, and other metadata. However, it also refers to a potentially variable +//! number of firewall _rules_. One might store those inline in the firewall +//! table, as an array of objects for example. +//! +//! This has several downsides. First, it dramatically limits the indexing and +//! search operations that can be efficiently performed. Imagine we wanted to +//! search for a particular firewall rule. One either needs to create a full +//! inverted index on the rule column, or perform a table scan _and_ a +//! potentially linear search within the array of items. The former is very +//! space-inefficient, the latter time-inefficient. +//! +//! Instead, we could normalize these tables, putting the rules in a separate +//! table. That enables efficient querying with more standard indexes, such as +//! B-trees. +//! +//! There are problems here, however. We've split the "real" firewall object +//! into multiple tables. The firewall is not really a "collection" of rules, it +//! _is_ the rules. It has no independent identity beyond the list of rules that +//! make it up. Put another way, the primitive operation on a firewall is a +//! `PUT`, i.e., a complete replacement of all rules within it. One could do +//! that inside an interactive transaction, or with joins or other more +//! complicated operations such as common table expressions. +//! +//! This module provides tools for working with such normalized, multi-table +//! objects, in an atomic fashion using common table expressions. + +use diesel::Table; +use diesel::associations::BelongsTo; + +pub struct MultiTableObject { + parent: ParentModel, + children: Vec, +} + +/* +#[derive(Debug, Clone, Copy)] +pub struct MultiTableObject< + Model, + ParentTable + ParentModel, + ChildTable, + ChildModel +> { + parent_table: ParentTable, + parent_model: ParentModel, + child_table: ChildTable, + child_model: ChildModel, +} +*/ + +/* +// ChildTable: BelongsTo +// +// Want to basically have ability to CRUD this one object, and have that do the +// right operation on both tables. +// +// insert -> (insert parent, insert all children) +// list -> (join parent, children, grouped by id or similar) +// get -> (join parent, children with particular parent id) +// update -> (remove all children, insert all children, update parent rcgen) +// delete -> (remove all children, remove parent) +// +// +// So we'd have +// VpcFirewallParent +// VpcFirewallRule +// +// VpcFirewall is MultiTableObject; +// +// +// Problems: +// +// How do you list all of these? It looks like we'd need do use `grouped_by` to +// get a vec of these fuckers. Or generate an actual GROUP BY and HAVING +// + +impl MultiTableObject +where + Parent: Table + Default, + Child: Table + BelongsTo + Default, +{ + pub fn new() -> Self { + Self { parent: Parent::default(), child: Child::default() } + } + + pub fn insert(self, conn: &DbConnection) -> CreateResult { + /* + WITH + inserted_parent + AS ( + INSERT + INTO + p (id) + VALUES + ('cd9963c3-3507-c29e-baea-a79faf887122') + RETURNING + * + ), + inserted_children + AS ( + INSERT + INTO + c (id, p_id) + VALUES + ( + '90f1ab21-502e-edd6-b655-c165e504a85b', + 'cd9963c3-3507-c29e-baea-a79faf887122' + ), + ( + '04c64cb5-48dc-c430-ed58-a8fc8d77b2ea', + 'cd9963c3-3507-c29e-baea-a79faf887122' + ) + RETURNING + * + ) + SELECT + * + FROM + inserted_parent, inserted_children + WHERE + inserted_parent.id = inserted_children.p_id + AND inserted_parent.id + = 'cd9963c3-3507-c29e-baea-a79faf887122' + */ + todo!() + } + + pub fn list(conn: &DbConnection) -> ListResultVec { + todo!() + } + + pub fn get(conn: &DbConnection) -> ListResult { + /* + * SELECT * FROM + * parent, child + * WHERE + * parent.id = child.parent_id AND + * parent.id = AND + * parent.time_deleted IS NOT NULL; + */ + todo!() + } + + pub fn update(self, conn: &DbConnection) -> UpdateResult { + /* + * WITH + * updated_children AS ( + * UPDATE child SET = + * where parent.id = + * returning * + * ), + * updated_parent AS ( + * UPDATE parent SET + * (time_modified, rcgen) = (NOW(), rcgen + 1) + * where id = + * returning * + * ) + * select * from updated_parent, updated_children + * where updated_parent.id = updated_children.parent_id + * and updated_parent.id = + */ + todo!() + } + + pub fn delete(self, conn: &DbConnection) -> DeleteResult { + /* Delete children, delete parent, return...count? */ + todo!() + } +} +*/ + +macro_rules! multi_table_object { + ($model:ident, $parent:ty, $child:ty) => { + pub struct $model { + mto: crate::db::multi_table_object::MultiTableObject< + <$parent as ::diesel::associations::HasTable>::Table, + <$child as ::diesel::associations::HasTable>::Table, + >, + parent: $parent, + children: Vec<$child>, + } + + impl $model { + pub fn new(parent: $parent, children: &[$child]) -> Self { + assert!(!children.is_empty()); + let children = children.to_vec(); + let mto = crate::db::multi_table_object::MultiTableObject::new(&parent, &children); + Self { mto, parent, children } + } + } + } +} diff --git a/nexus/src/db/queries/ip_pool.rs b/nexus/src/db/queries/ip_pool.rs new file mode 100644 index 0000000000..539946b5c7 --- /dev/null +++ b/nexus/src/db/queries/ip_pool.rs @@ -0,0 +1,288 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Implementation of queries for operating on IP Pools. + +use crate::db::model::IpPoolRange; +use crate::db::schema::ip_pool_range::dsl; +use chrono::DateTime; +use chrono::Utc; +use diesel::pg::Pg; +use diesel::query_builder::AstPass; +use diesel::query_builder::QueryFragment; +use diesel::query_builder::QueryId; +use diesel::sql_types; +use diesel::Column; +use diesel::Insertable; +use diesel::QueryResult; +use ipnetwork::IpNetwork; +use uuid::Uuid; + +/// A query for filtering out candidate IP ranges that overlap with any +/// existing ranges. +/// +/// This query is used when inserting a new IP range into an existing IP Pool. +/// Those ranges must currently be unique globally, across all pools. This query +/// selects the candidate range, _if_ it does not overlap with any existing +/// range. I.e., it filters out the candidate if it overlaps. The query looks +/// like +/// +/// ```sql +/// SELECT +/// +/// WHERE +/// -- Check for ranges that contain the candidate first address +/// NOT EXISTS( +/// SELECT +/// id +/// FROM +/// ip_pool_range +/// WHERE +/// >= first_address AND +/// <= last_address AND +/// time_deleted IS NULL +/// LIMIT 1 +/// ) +/// AND +/// -- Check for ranges that contain the candidate last address +/// NOT EXISTS( +/// SELECT +/// id +/// FROM +/// ip_pool_range +/// WHERE +/// >= first_address AND +/// <= last_address AND +/// time_deleted IS NULL +/// LIMIT 1 +/// ) +/// AND +/// -- Check for ranges whose first address is contained by the candidate +/// -- range +/// NOT EXISTS( +/// SELECT +/// id +/// FROM +/// ip_pool_range +/// WHERE +/// first_address >= AND +/// first_address <= AND +/// time_deleted IS NULL +/// LIMIT 1 +/// ) +/// AND +/// -- Check for ranges whose last address is contained by the candidate +/// -- range +/// NOT EXISTS( +/// SELECT +/// id +/// FROM +/// ip_pool_range +/// WHERE +/// last_address >= AND +/// last_address <= AND +/// time_deleted IS NULL +/// LIMIT 1 +/// ) +/// ``` +/// +/// That's a lot of duplication, but it's to help with the scalability of the +/// query. Collapsing those different `EXISTS()` subqueries into one set of +/// `WHERE` clauses would require an `OR`. For example: +/// +/// ```sql +/// WHERE +/// ( +/// >= first_address AND +/// <= last_address +/// ) +/// OR +/// ( +/// >= first_address AND +/// <= last_address +/// ) +/// AND +/// time_deleted IS NULL +/// ``` +/// +/// That `OR` means the database cannot use the indexes we've supplied on the +/// `first_address` and `last_address` columns, and must resort to a full table +/// scan. +#[derive(Debug, Clone)] +pub struct FilterOverlappingIpRanges { + pub range: IpPoolRange, +} + +impl QueryId for FilterOverlappingIpRanges { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +// Push the subquery finding any existing record that contains a candidate's +// address (first or last): +// +// ```sql +// SELECT +// id +// FROM +// ip_pool_range +// WHERE +//
>= first_address AND +//
<= last_address AND +// time_deleted IS NULL +// LIMIT 1 +// ``` +fn push_record_contains_candidate_subquery<'a>( + mut out: AstPass<'_, 'a, Pg>, + address: &'a IpNetwork, +) -> QueryResult<()> { + out.push_sql("SELECT "); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(" FROM "); + IP_POOL_RANGE_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_bind_param::(address)?; + out.push_sql(" >= "); + out.push_identifier(dsl::first_address::NAME)?; + out.push_sql(" AND "); + out.push_bind_param::(address)?; + out.push_sql(" <= "); + out.push_identifier(dsl::last_address::NAME)?; + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL LIMIT 1"); + Ok(()) +} + +// Push the subquery that finds any records with an address contained within the +// provided candidate range. +// +// ```sql +// SELECT +// id +// FROM +// ip_pool_range +// WHERE +// >= AND +// <= AND +// time_deleted IS NULL +// LIMIT 1 +// ``` +fn push_candidate_contains_record_subquery<'a, C>( + mut out: AstPass<'_, 'a, Pg>, + first_address: &'a IpNetwork, + last_address: &'a IpNetwork, +) -> QueryResult<()> +where + C: Column, +{ + out.push_sql("SELECT "); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(" FROM "); + IP_POOL_RANGE_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(C::NAME)?; + out.push_sql(" >= "); + out.push_bind_param::(first_address)?; + out.push_sql(" AND "); + out.push_identifier(C::NAME)?; + out.push_sql(" <= "); + out.push_bind_param::(last_address)?; + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL LIMIT 1"); + Ok(()) +} + +impl QueryFragment for FilterOverlappingIpRanges { + fn walk_ast<'a>(&'a self, mut out: AstPass<'_, 'a, Pg>) -> QueryResult<()> { + out.unsafe_to_cache_prepared(); + out.push_sql("SELECT "); + out.push_bind_param::(&self.range.id)?; + out.push_sql(", "); + out.push_bind_param::>( + &self.range.time_created, + )?; + out.push_sql(", "); + out.push_bind_param::>( + &self.range.time_modified, + )?; + out.push_sql(", "); + out.push_bind_param::< sql_types::Nullable, Option>>(&self.range.time_deleted)?; + out.push_sql(", "); + out.push_bind_param::( + &self.range.first_address, + )?; + out.push_sql(", "); + out.push_bind_param::( + &self.range.last_address, + )?; + out.push_sql(", "); + out.push_bind_param::(&self.range.ip_pool_id)?; + + out.push_sql(" WHERE NOT EXISTS("); + push_candidate_contains_record_subquery::( + out.reborrow(), + &self.range.first_address, + &self.range.last_address, + )?; + out.push_sql(") AND NOT EXISTS("); + push_candidate_contains_record_subquery::( + out.reborrow(), + &self.range.first_address, + &self.range.last_address, + )?; + out.push_sql(") AND NOT EXISTS("); + push_record_contains_candidate_subquery( + out.reborrow(), + &self.range.first_address, + )?; + out.push_sql(") AND NOT EXISTS("); + push_record_contains_candidate_subquery( + out.reborrow(), + &self.range.last_address, + )?; + out.push_sql(")"); + Ok(()) + } +} + +impl Insertable for FilterOverlappingIpRanges { + type Values = FilterOverlappingIpRangesValues; + + fn values(self) -> Self::Values { + FilterOverlappingIpRangesValues(self) + } +} + +#[derive(Debug, Clone)] +pub struct FilterOverlappingIpRangesValues(pub FilterOverlappingIpRanges); + +impl QueryId for FilterOverlappingIpRangesValues { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl diesel::insertable::CanInsertInSingleQuery + for FilterOverlappingIpRangesValues +{ + fn rows_to_insert(&self) -> Option { + Some(1) + } +} + +impl QueryFragment for FilterOverlappingIpRangesValues { + fn walk_ast<'a>( + &'a self, + mut out: AstPass<'_, 'a, Pg>, + ) -> diesel::QueryResult<()> { + self.0.walk_ast(out.reborrow()) + } +} + +type FromClause = + diesel::internal::table_macro::StaticQueryFragmentInstance; +type IpPoolRangeFromClause = FromClause; +const IP_POOL_RANGE_FROM_CLAUSE: IpPoolRangeFromClause = + IpPoolRangeFromClause::new(); diff --git a/nexus/src/db/queries/mod.rs b/nexus/src/db/queries/mod.rs index 11736e4a95..d23ecf583e 100644 --- a/nexus/src/db/queries/mod.rs +++ b/nexus/src/db/queries/mod.rs @@ -5,6 +5,7 @@ //! Specialized queries for inserting database records, usually to maintain //! complex invariants that are most accurately expressed in a single query. +pub mod ip_pool; #[macro_use] mod next_item; pub mod network_interface; diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index a6d281d987..439cdecf69 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -135,6 +135,30 @@ table! { } } +table! { + ip_pool (id) { + id -> Uuid, + name -> Text, + description -> Text, + time_created -> Timestamptz, + time_modified -> Timestamptz, + time_deleted -> Nullable, + rcgen -> Int8, + } +} + +table! { + ip_pool_range (id) { + id -> Uuid, + time_created -> Timestamptz, + time_modified -> Timestamptz, + time_deleted -> Nullable, + first_address -> Inet, + last_address -> Inet, + ip_pool_id -> Uuid, + } +} + table! { silo (id) { id -> Uuid, @@ -507,6 +531,9 @@ table! { } } +allow_tables_to_appear_in_same_query!(ip_pool_range, ip_pool); +joinable!(ip_pool_range -> ip_pool (ip_pool_id)); + allow_tables_to_appear_in_same_query!( dataset, disk, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index b47e6ecf12..96f7380d40 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -4,6 +4,8 @@ //! Handler functions (entrypoints) for external HTTP APIs +use super::views::IpPool; +use super::views::IpRange; use super::{ console_api, params, views, views::{ @@ -34,6 +36,7 @@ use dropshot::RequestContext; use dropshot::ResultsPage; use dropshot::TypedBody; use dropshot::WhichPage; +use omicron_common::api::external; use omicron_common::api::external::http_pagination::data_page_params_nameid_id; use omicron_common::api::external::http_pagination::data_page_params_nameid_name; use omicron_common::api::external::http_pagination::pagination_field_for_scan_params; @@ -103,6 +106,23 @@ pub fn external_api() -> NexusApiDescription { api.register(organization_projects_get_project_policy)?; api.register(organization_projects_put_project_policy)?; + api.register(ip_pools_get)?; + api.register(ip_pools_post)?; + api.register(ip_pools_get_ip_pool)?; + api.register(ip_pools_delete_ip_pool)?; + api.register(ip_pools_put_ip_pool)?; + + api.register(ip_pool_ranges_get)?; + api.register(ip_pool_ranges_add)?; + api.register(ip_pool_ranges_delete)?; + + /* + api.register(ip_pool_cidr_blocks_get)?; + api.register(ip_pool_cidr_blocks_post)?; + api.register(ip_pool_cidr_blocks_get_cidr_block)?; + api.register(ip_pool_cidr_blocks_delete_cidr_block)?; + */ + api.register(project_disks_get)?; api.register(project_disks_post)?; api.register(project_disks_get_disk)?; @@ -1010,6 +1030,242 @@ async fn organization_projects_put_project_policy( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +// IP Pools + +#[derive(Deserialize, JsonSchema)] +pub struct IpPoolPathParam { + pub pool_name: Name, +} + +#[derive(Deserialize, JsonSchema)] +pub struct IpPoolCidrBlockPathParam { + pub pool_name: Name, + pub block_name: Name, +} + +/// List IP Pools. +#[endpoint { + method = GET, + path = "/ip-pools", + tags = ["ip-pools"], +}] +async fn ip_pools_get( + rqctx: Arc>>, + query_params: Query, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let query = query_params.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let params = ScanByNameOrId::from_query(&query)?; + let field = pagination_field_for_scan_params(params); + let pools = match field { + PagField::Id => { + let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + nexus.ip_pools_list_by_id(&opctx, &page_selector).await? + } + PagField::Name => { + let page_selector = + data_page_params_nameid_name(&rqctx, &query)? + .map_name(|n| Name::ref_cast(n)); + nexus.ip_pools_list_by_name(&opctx, &page_selector).await? + } + } + .into_iter() + .map(IpPool::from) + .collect(); + Ok(HttpResponseOk(ScanByNameOrId::results_page(&query, pools)?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Create a new IP Pool. +#[endpoint { + method = POST, + path = "/ip-pools", + tags = ["ip-pools"], +}] +async fn ip_pools_post( + rqctx: Arc>>, + pool_params: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let pool_params = pool_params.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let pool = nexus.ip_pool_create(&opctx, &pool_params).await?; + Ok(HttpResponseCreated(IpPool::from(pool))) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Fetch a single IP Pool. +#[endpoint { + method = GET, + path = "/ip-pools/{pool_name}", + tags = ["ip-pools"], +}] +async fn ip_pools_get_ip_pool( + rqctx: Arc>>, + path_params: Path, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let pool_name = &path.pool_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let pool = nexus.ip_pool_fetch(&opctx, pool_name).await?; + Ok(HttpResponseOk(IpPool::from(pool))) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Delete an IP Pool. +#[endpoint { + method = DELETE, + path = "/ip-pools/{pool_name}", + tags = ["ip-pools"], +}] +async fn ip_pools_delete_ip_pool( + rqctx: Arc>>, + path_params: Path, +) -> Result { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let pool_name = &path.pool_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + nexus.ip_pool_delete(&opctx, pool_name).await?; + Ok(HttpResponseDeleted()) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Update an IP Pool. +#[endpoint { + method = PUT, + path = "/ip-pools/{pool_name}", + tags = ["ip-pools"], +}] +async fn ip_pools_put_ip_pool( + rqctx: Arc>>, + path_params: Path, + updates: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let pool_name = &path.pool_name; + let updates = updates.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let pool = nexus.ip_pool_update(&opctx, pool_name, &updates).await?; + Ok(HttpResponseOk(pool.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +pub type IpPoolRangePaginationParams = + PaginationParams; + +/// List the ranges of IP addresses within an existing IP Pool. +/// +/// Note that ranges are listed sorted by their first address. +#[endpoint { + method = GET, + path = "/ip-pools/{pool_name}/ranges", + tags = ["ip-pools"], +}] +async fn ip_pool_ranges_get( + rqctx: Arc>>, + path_params: Path, + // TODO(ben): Figure out how to paginate this + query_params: Query, + //) -> Result>, HttpError> { +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let query = query_params.into_inner(); + let path = path_params.into_inner(); + let pool_name = &path.pool_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let marker = match query.page { + WhichPage::First(_) => None, + WhichPage::Next(ref last_address) => Some(last_address), + }; + let pag_params = DataPageParams { + limit: rqctx.page_limit(&query)?, + direction: PaginationOrder::Ascending, + marker, + }; + let ranges = nexus + .ip_pool_list_ranges(&opctx, pool_name, &pag_params) + .await? + .into_iter() + .map(|range| range.into()) + .collect(); + Ok(HttpResponseOk(ResultsPage::new( + ranges, + &EmptyScanParams {}, + |range: &IpRange, _| range.range.first_address(), + )?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Add a new range to an existing IP Pool. +#[endpoint { + method = POST, + path = "/ip-pools/{pool_name}/ranges/add", + tags = ["ip-pools"], +}] +async fn ip_pool_ranges_add( + rqctx: Arc>>, + path_params: Path, + range_params: TypedBody, +) -> Result, HttpError> { + let apictx = &rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let pool_name = &path.pool_name; + let range = range_params.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let out = nexus.ip_pool_add_range(&opctx, pool_name, &range).await?; + Ok(HttpResponseCreated(out.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Remove a range from an existing IP Pool. +#[endpoint { + method = POST, + path = "/ip-pools/{pool_name}/ranges/delete", + tags = ["ip-pools"], +}] +async fn ip_pool_ranges_delete( + rqctx: Arc>>, + path_params: Path, + range_params: TypedBody, +) -> Result { + let apictx = &rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let pool_name = &path.pool_name; + let range = range_params.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + nexus.ip_pool_delete_range(&opctx, pool_name, &range).await?; + Ok(HttpResponseUpdatedNoContent()) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + // Disks /// List disks in a project. diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index e5dcf46ba7..94852cfc95 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -4,6 +4,8 @@ //! Params define the request bodies of API endpoints for creating or updating resources. +use omicron_common::api::external::IpNet; +use omicron_common::api::external::IpRange; use omicron_common::api::external::{ ByteCount, IdentityMetadataCreateParams, IdentityMetadataUpdateParams, InstanceCpuCount, Ipv4Net, Ipv6Net, Name, @@ -310,6 +312,48 @@ pub struct NetworkInterfaceUpdate { pub make_primary: bool, } +// IP POOLS + +/// Create-time parameters for an IP Pool. +/// +/// See [`IpPool`](omicron_nexus::external_api::views::IpPool) +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct IpPoolCreate { + #[serde(flatten)] + pub identity: IdentityMetadataCreateParams, +} + +/// Parameters for updating an IP Pool +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct IpPoolUpdate { + #[serde(flatten)] + pub identity: IdentityMetadataUpdateParams, +} + +/// Parameters for adding a new IP range to an IP Pool +#[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] +pub enum IpRangeAdd { + Network(IpNet), + Range(IpRange), +} + +/// Parameters for creating a new CIDR block within an existing IP Pool. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct IpPoolCidrBlockCreate { + #[serde(flatten)] + pub identity: IdentityMetadataCreateParams, + + /// The IP subnet, expressed as a CIDR block. + pub cidr_block: IpNet, +} + +/// Parameters for updating an existing CIDR block within an IP Pool. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct IpPoolCidrBlockUpdate { + #[serde(flatten)] + pub identity: IdentityMetadataUpdateParams, +} + // INSTANCES pub const MIN_MEMORY_SIZE_BYTES: u32 = 1 << 30; // 1 GiB diff --git a/nexus/src/external_api/tag-config.json b/nexus/src/external_api/tag-config.json index 592b7095ff..1e451e7d9b 100644 --- a/nexus/src/external_api/tag-config.json +++ b/nexus/src/external_api/tag-config.json @@ -38,6 +38,12 @@ "url": "http://oxide.computer/docs/#xxx" } }, + "ip-pools": { + "description": "IP Pools contain subnets used for assigning external IP addresses to virtual machine instances.", + "external_docs": { + "url": "http://oxide.computer/docs/#xxx" + } + }, "login": { "description": "Authentication endpoints", "external_docs": { diff --git a/nexus/src/external_api/views.rs b/nexus/src/external_api/views.rs index f758e8f4e1..37916e5f92 100644 --- a/nexus/src/external_api/views.rs +++ b/nexus/src/external_api/views.rs @@ -8,12 +8,16 @@ use crate::authn; use crate::db::identity::{Asset, Resource}; use crate::db::model; use api_identity::ObjectIdentity; +use chrono::DateTime; +use chrono::Utc; +use omicron_common::api::external; use omicron_common::api::external::{ ByteCount, Digest, IdentityMetadata, Ipv4Net, Ipv6Net, Name, ObjectIdentity, RoleName, }; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use std::net::IpAddr; use std::net::SocketAddrV6; use uuid::Uuid; @@ -315,6 +319,44 @@ impl From for VpcRouter { } } +#[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct IpPool { + #[serde(flatten)] + pub identity: IdentityMetadata, +} + +impl From for IpPool { + fn from(pool: model::IpPool) -> Self { + Self { identity: pool.identity() } + } +} + +#[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] +pub struct IpRange { + pub id: Uuid, + pub time_created: DateTime, + pub range: external::IpRange, +} + +impl From for IpRange { + fn from(range: model::IpPoolRange) -> Self { + let r = match (range.first_address.ip(), range.last_address.ip()) { + (IpAddr::V4(first), IpAddr::V4(last)) => { + external::IpRange::V4(external::Ipv4Range { first, last }) + } + (IpAddr::V6(first), IpAddr::V6(last)) => { + external::IpRange::V6(external::Ipv6Range { first, last }) + } + (first, last) => unreachable!( + "Expected first and last address from the same IP \ + protocol versions, found {:?} and {:?}", + first, last + ), + }; + Self { id: range.id, time_created: range.time_created, range: r } + } +} + // RACKS /// Client view of an [`Rack`] diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 577d3c7988..17d7bf44e9 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -16,7 +16,9 @@ use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::InstanceCpuCount; +use omicron_common::api::external::IpRange; use omicron_common::api::external::Ipv4Net; +use omicron_common::api::external::Ipv4Range; use omicron_common::api::external::Name; use omicron_common::api::external::RouteDestination; use omicron_common::api::external::RouteTarget; @@ -272,6 +274,32 @@ lazy_static! { block_size: params::BlockSize::try_from(4096).unwrap(), }; + // IP Pools + pub static ref DEMO_IP_POOLS_URL: &'static str = "/ip-pools"; + pub static ref DEMO_IP_POOL_NAME: Name = "pool0".parse().unwrap(); + pub static ref DEMO_IP_POOL_CREATE: params::IpPoolCreate = + params::IpPoolCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_IP_POOL_NAME.clone(), + description: String::from("an IP pool"), + }, + }; + pub static ref DEMO_IP_POOL_URL: String = format!("/ip-pools/{}", *DEMO_IP_POOL_NAME); + pub static ref DEMO_IP_POOL_UPDATE: params::IpPoolUpdate = + params::IpPoolUpdate { + identity: IdentityMetadataUpdateParams { + name: None, + description: Some(String::from("a new IP pool")), + }, + }; + pub static ref DEMO_IP_POOL_RANGE: IpRange = IpRange::V4(Ipv4Range { + first: std::net::Ipv4Addr::new(10, 0, 0, 1), + last: std::net::Ipv4Addr::new(10, 0, 0, 2), + }); + pub static ref DEMO_IP_POOL_RANGES_URL: String = format!("{}/ranges", *DEMO_IP_POOL_URL); + pub static ref DEMO_IP_POOL_RANGES_ADD_URL: String = format!("{}/add", *DEMO_IP_POOL_RANGES_URL); + pub static ref DEMO_IP_POOL_RANGES_DEL_URL: String = format!("{}/delete", *DEMO_IP_POOL_RANGES_URL); + // Snapshots pub static ref DEMO_SNAPSHOT_NAME: Name = "demo-snapshot".parse().unwrap(); pub static ref DEMO_SNAPSHOT_URL: String = @@ -447,6 +475,62 @@ lazy_static! { ], }, + // IP Pools top-level endpoint + VerifyEndpoint { + url: *DEMO_IP_POOLS_URL, + visibility: Visibility::Public, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_IP_POOL_CREATE).unwrap() + ), + ], + }, + + // Single IP Pool endpoint + VerifyEndpoint { + url: &*DEMO_IP_POOL_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Put( + serde_json::to_value(&*DEMO_IP_POOL_UPDATE).unwrap() + ), + AllowedMethod::Delete, + ], + }, + + // IP Pool ranges endpoint + VerifyEndpoint { + url: &*DEMO_IP_POOL_RANGES_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get + ], + }, + + // IP Pool ranges/add endpoint + VerifyEndpoint { + url: &*DEMO_IP_POOL_RANGES_ADD_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Post( + serde_json::to_value(&*DEMO_IP_POOL_RANGE).unwrap() + ), + ], + }, + + // IP Pool ranges/delete endpoint + VerifyEndpoint { + url: &*DEMO_IP_POOL_RANGES_DEL_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Post( + serde_json::to_value(&*DEMO_IP_POOL_RANGE).unwrap() + ), + ], + }, + /* Silos */ VerifyEndpoint { url: "/silos", diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs new file mode 100644 index 0000000000..f02f364c4d --- /dev/null +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -0,0 +1,520 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Integration tests for operating on IP Pools + +use dropshot::test_util::ClientTestContext; +use dropshot::HttpErrorResponseBody; +use http::method::Method; +use http::StatusCode; +use nexus_test_utils::http_testing::AuthnMode; +use nexus_test_utils::http_testing::NexusRequest; +use nexus_test_utils::http_testing::RequestBuilder; +use nexus_test_utils::resource_helpers::objects_list_page_authz; +use nexus_test_utils::ControlPlaneTestContext; +use nexus_test_utils_macros::nexus_test; +use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::IdentityMetadataUpdateParams; +use omicron_common::api::external::IpRange; +use omicron_common::api::external::Ipv4Range; +use omicron_common::api::external::Ipv6Range; +use omicron_nexus::external_api::params::IpPoolCreate; +use omicron_nexus::external_api::params::IpPoolUpdate; +use omicron_nexus::external_api::views::IpPool; +use omicron_nexus::external_api::views::IpRange as IpRangeView; + +// Basic test verifying CRUD behavior on the IP Pool itself. +#[nexus_test] +async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let ip_pools_url = "/ip-pools"; + let pool_name = "p0"; + let description = "an ip pool"; + let ip_pool_url = format!("{}/{}", ip_pools_url, pool_name); + let ip_pool_ranges_url = format!("{}/ranges", ip_pool_url); + let ip_pool_add_range_url = format!("{}/add", ip_pool_ranges_url); + + // Verify the list of IP pools is empty + assert_eq!( + NexusRequest::iter_collection_authn::( + client, + ip_pools_url, + "", + None + ) + .await + .expect("Failed to list IP Pools") + .all_items + .len(), + 0, + "Expected a list of zero IP pools" + ); + + // Verify 404 if the pool doesn't exist yet, both for creating or deleting + let error: HttpErrorResponseBody = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + &ip_pool_url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + error.message, + format!("not found: ip-pool with name \"{}\"", pool_name), + ); + let error: HttpErrorResponseBody = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::DELETE, + &ip_pool_url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + error.message, + format!("not found: ip-pool with name \"{}\"", pool_name), + ); + + // Create the pool, verify we can get it back by either listing or fetching + // directly + let params = IpPoolCreate { + identity: IdentityMetadataCreateParams { + name: String::from(pool_name).parse().unwrap(), + description: String::from(description), + }, + }; + let created_pool: IpPool = + NexusRequest::objects_post(client, ip_pools_url, ¶ms) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!(created_pool.identity.name, pool_name); + assert_eq!(created_pool.identity.description, description); + + let list = NexusRequest::iter_collection_authn::( + client, + ip_pools_url, + "", + None, + ) + .await + .expect("Failed to list IP Pools") + .all_items; + assert_eq!(list.len(), 1, "Expected exactly one IP pool"); + assert_pools_eq(&created_pool, &list[0]); + + let fetched_pool: IpPool = NexusRequest::object_get(client, &ip_pool_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_pools_eq(&created_pool, &fetched_pool); + + // Verify we get a conflict error if we insert it again + let error: HttpErrorResponseBody = NexusRequest::new( + RequestBuilder::new(client, Method::POST, ip_pools_url) + .body(Some(¶ms)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + error.message, + format!("already exists: ip-pool \"{}\"", pool_name) + ); + + // Add a range, verify that we can't delete the Pool + let range = IpRange::V4(Ipv4Range { + first: "10.0.0.1".parse().unwrap(), + last: "10.0.0.5".parse().unwrap(), + }); + let created_range: IpRangeView = + NexusRequest::objects_post(client, &ip_pool_add_range_url, &range) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!(range.first_address(), created_range.range.first_address()); + assert_eq!(range.last_address(), created_range.range.last_address()); + let error: HttpErrorResponseBody = NexusRequest::expect_failure( + client, + StatusCode::BAD_REQUEST, + Method::DELETE, + &ip_pool_url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + error.message, + "IP Pool cannot be deleted while it contains IP ranges", + ); + + // Rename the pool. + // + // Ensure we can fetch the pool under the new name, and not the old, and + // that the modification time has changed. + let new_pool_name = "p1"; + let new_ip_pool_url = format!("{}/{}", ip_pools_url, new_pool_name); + let new_ip_pool_del_range_url = + format!("{}/ranges/delete", new_ip_pool_url); + let updates = IpPoolUpdate { + identity: IdentityMetadataUpdateParams { + name: Some(String::from(new_pool_name).parse().unwrap()), + description: None, + }, + }; + let modified_pool: IpPool = + NexusRequest::object_put(client, &ip_pool_url, Some(&updates)) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!(modified_pool.identity.name, new_pool_name); + assert_eq!(modified_pool.identity.id, created_pool.identity.id); + assert_eq!( + modified_pool.identity.description, + created_pool.identity.description + ); + assert_eq!( + modified_pool.identity.time_created, + created_pool.identity.time_created + ); + assert!( + modified_pool.identity.time_modified + > created_pool.identity.time_modified + ); + + let fetched_modified_pool: IpPool = + NexusRequest::object_get(client, &new_ip_pool_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_pools_eq(&modified_pool, &fetched_modified_pool); + + let error: HttpErrorResponseBody = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + &ip_pool_url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + error.message, + format!("not found: ip-pool with name \"{}\"", pool_name), + ); + + // Delete the range, then verify we can delete the pool and everything looks + // gravy. + /* + * Most clients should expect 201, which isn't really what we're doing here. + NexusRequest::objects_post(client, &new_ip_pool_del_range_url, &range) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to delete IP range within pool"); + */ + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &new_ip_pool_del_range_url) + .body(Some(&range)) + .expect_status(Some(StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to delete IP range from a pool"); + NexusRequest::object_delete(client, &new_ip_pool_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Expected to be able to delete an empty IP Pool"); +} + +// Data for testing overlapping IP ranges +struct TestRange { + // A starting IP range that should be inserted correctly + base_range: IpRange, + // Ranges that should fail for various reasons of overlap + bad_ranges: Vec, +} + +// Integration test verifying the uniqueness of IP ranges when inserted / +// deleted across multiple pools +#[nexus_test] +async fn test_ip_pool_range_overlapping_ranges_fails( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let ip_pools_url = "/ip-pools"; + let pool_name = "p0"; + let description = "an ip pool"; + let ip_pool_url = format!("{}/{}", ip_pools_url, pool_name); + let ip_pool_ranges_url = format!("{}/ranges", ip_pool_url); + let ip_pool_add_range_url = format!("{}/add", ip_pool_ranges_url); + + // Create the pool, verify basic properties + let params = IpPoolCreate { + identity: IdentityMetadataCreateParams { + name: String::from(pool_name).parse().unwrap(), + description: String::from(description), + }, + }; + let created_pool: IpPool = + NexusRequest::objects_post(client, ip_pools_url, ¶ms) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!(created_pool.identity.name, pool_name); + assert_eq!(created_pool.identity.description, description); + + // Test data for IPv4 ranges that should fail due to overlap + let ipv4_range = TestRange { + base_range: IpRange::V4(Ipv4Range { + first: "10.0.0.2".parse().unwrap(), + last: "10.0.0.5".parse().unwrap(), + }), + bad_ranges: vec![ + // The exact same range + IpRange::V4(Ipv4Range { + first: "10.0.0.2".parse().unwrap(), + last: "10.0.0.5".parse().unwrap(), + }), + // Overlaps below + IpRange::V4(Ipv4Range { + first: "10.0.0.1".parse().unwrap(), + last: "10.0.0.2".parse().unwrap(), + }), + // Overlaps above + IpRange::V4(Ipv4Range { + first: "10.0.0.5".parse().unwrap(), + last: "10.0.0.6".parse().unwrap(), + }), + // Contains the base range + IpRange::V4(Ipv4Range { + first: "10.0.0.1".parse().unwrap(), + last: "10.0.0.6".parse().unwrap(), + }), + // Contained by the base range + IpRange::V4(Ipv4Range { + first: "10.0.0.1".parse().unwrap(), + last: "10.0.0.6".parse().unwrap(), + }), + ], + }; + 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 { + first: "fd00::10".parse().unwrap(), + last: "fd00::20".parse().unwrap(), + }), + bad_ranges: vec![ + // The exact same range + IpRange::V6(Ipv6Range { + first: "fd00::10".parse().unwrap(), + last: "fd00::20".parse().unwrap(), + }), + // Overlaps below + IpRange::V6(Ipv6Range { + first: "fd00::5".parse().unwrap(), + last: "fd00::10".parse().unwrap(), + }), + // Overlaps above + IpRange::V6(Ipv6Range { + first: "fd00::15".parse().unwrap(), + last: "fd00::25".parse().unwrap(), + }), + // Contains the base range + IpRange::V6(Ipv6Range { + first: "fd00::".parse().unwrap(), + last: "fd00::100".parse().unwrap(), + }), + // Contained by the base range + IpRange::V6(Ipv6Range { + first: "fd00::12".parse().unwrap(), + last: "fd00::12".parse().unwrap(), + }), + ], + }; + test_bad_ip_ranges(client, &ip_pool_add_range_url, &ipv6_range).await; +} + +async fn test_bad_ip_ranges( + client: &ClientTestContext, + url: &str, + ranges: &TestRange, +) { + let created_range: IpRangeView = + NexusRequest::objects_post(client, url, &ranges.base_range) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + ranges.base_range.first_address(), + created_range.range.first_address() + ); + assert_eq!( + ranges.base_range.last_address(), + created_range.range.last_address() + ); + + // Everything else should fail + for bad_range in ranges.bad_ranges.iter() { + let error: HttpErrorResponseBody = NexusRequest::new( + RequestBuilder::new(client, Method::POST, url) + .body(Some(bad_range)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + let expected_message = format!( + "The provided IP range {}-{} overlaps with an existing range", + bad_range.first_address(), + bad_range.last_address(), + ); + assert_eq!(error.message, expected_message); + } +} + +#[nexus_test] +async fn test_ip_pool_range_pagination(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let ip_pools_url = "/ip-pools"; + let pool_name = "p0"; + let description = "an ip pool"; + let ip_pool_url = format!("{}/{}", ip_pools_url, pool_name); + let ip_pool_ranges_url = format!("{}/ranges", ip_pool_url); + let ip_pool_add_range_url = format!("{}/add", ip_pool_ranges_url); + + // Create the pool, verify basic properties + let params = IpPoolCreate { + identity: IdentityMetadataCreateParams { + name: String::from(pool_name).parse().unwrap(), + description: String::from(description), + }, + }; + let created_pool: IpPool = + NexusRequest::objects_post(client, ip_pools_url, ¶ms) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!(created_pool.identity.name, pool_name); + assert_eq!(created_pool.identity.description, description); + + // Add some ranges, out of order. These will be paginated by their first + // address, which sorts all IPv4 before IPv6, then within protocol versions + // by their first address. + let ranges = [ + IpRange::V6(Ipv6Range { + first: "fd00::11".parse().unwrap(), + last: "fd00::20".parse().unwrap(), + }), + IpRange::V6(Ipv6Range { + first: "fd00::".parse().unwrap(), + last: "fd00::10".parse().unwrap(), + }), + IpRange::V4(Ipv4Range { + first: "10.0.0.1".parse().unwrap(), + last: "10.0.0.2".parse().unwrap(), + }), + ]; + + let mut expected_ranges = Vec::with_capacity(ranges.len()); + for range in ranges.iter() { + let created_range: IpRangeView = + NexusRequest::objects_post(client, &ip_pool_add_range_url, &range) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!(range.first_address(), created_range.range.first_address()); + assert_eq!(range.last_address(), created_range.range.last_address()); + expected_ranges.push(created_range); + } + expected_ranges + .sort_by(|a, b| a.range.first_address().cmp(&b.range.first_address())); + + // List the first 2 results, then the last. These should appear sorted by + // their first address. + let first_page_url = format!("{}?limit=2", ip_pool_ranges_url); + let first_page = + objects_list_page_authz::(client, &first_page_url).await; + assert_eq!(first_page.items.len(), 2); + + let second_page_url = format!( + "{}&page_token={}", + first_page_url, + first_page.next_page.unwrap() + ); + let second_page = + objects_list_page_authz::(client, &second_page_url).await; + assert_eq!(second_page.items.len(), 1); + + let actual_ranges = first_page.items.iter().chain(second_page.items.iter()); + for (expected_range, actual_range) in + expected_ranges.iter().zip(actual_ranges) + { + assert_ranges_eq(expected_range, actual_range); + } +} + +fn assert_pools_eq(first: &IpPool, second: &IpPool) { + assert_eq!(first.identity, second.identity); +} + +fn assert_ranges_eq(first: &IpRangeView, second: &IpRangeView) { + assert_eq!(first.id, second.id); + assert_eq!(first.time_created, second.time_created); + assert_eq!(first.range.first_address(), second.range.first_address()); + assert_eq!(first.range.last_address(), second.range.last_address()); +} diff --git a/nexus/tests/integration_tests/mod.rs b/nexus/tests/integration_tests/mod.rs index de5de9679b..639c754d84 100644 --- a/nexus/tests/integration_tests/mod.rs +++ b/nexus/tests/integration_tests/mod.rs @@ -11,6 +11,7 @@ mod datasets; mod disks; mod images; mod instances; +mod ip_pools; mod organizations; mod oximeter; mod projects; diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 45e8d67073..36a15ebb79 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -206,6 +206,11 @@ lazy_static! { url: &*SAML_IDENTITY_PROVIDERS_URL, body: serde_json::to_value(&*SAML_IDENTITY_PROVIDER).unwrap(), }, + // Create an IP pool + SetupReq { + url: &*DEMO_IP_POOLS_URL, + body: serde_json::to_value(&*DEMO_IP_POOL_CREATE).unwrap(), + } ]; } diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index b470210dda..25aadfb9e8 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -50,6 +50,17 @@ project_instances_instance_stop /organizations/{organization_name}/proj project_instances_migrate_instance /organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/migrate project_instances_post /organizations/{organization_name}/projects/{project_name}/instances +API operations found with tag "ip-pools" +OPERATION ID URL PATH +ip_pool_ranges_add /ip-pools/{pool_name}/ranges/add +ip_pool_ranges_delete /ip-pools/{pool_name}/ranges/delete +ip_pool_ranges_get /ip-pools/{pool_name}/ranges +ip_pools_delete_ip_pool /ip-pools/{pool_name} +ip_pools_get /ip-pools +ip_pools_get_ip_pool /ip-pools/{pool_name} +ip_pools_post /ip-pools +ip_pools_put_ip_pool /ip-pools/{pool_name} + API operations found with tag "login" OPERATION ID URL PATH consume_credentials /login/{silo_name}/{provider_name} diff --git a/openapi/nexus.json b/openapi/nexus.json index 3a9c4e7106..286d54b607 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -376,6 +376,365 @@ } } }, + "/ip-pools": { + "get": { + "tags": [ + "ip-pools" + ], + "summary": "List IP Pools.", + "operationId": "ip_pools_get", + "parameters": [ + { + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + }, + "style": "form" + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retrieve the subsequent page", + "schema": { + "nullable": true, + "type": "string" + }, + "style": "form" + }, + { + "in": "query", + "name": "sort_by", + "schema": { + "$ref": "#/components/schemas/NameOrIdSortMode" + }, + "style": "form" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpPoolResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": true + }, + "post": { + "tags": [ + "ip-pools" + ], + "summary": "Create a new IP Pool.", + "operationId": "ip_pools_post", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpPoolCreate" + } + } + }, + "required": true + }, + "responses": { + "201": { + "description": "successful creation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpPool" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/ip-pools/{pool_name}": { + "get": { + "tags": [ + "ip-pools" + ], + "summary": "Fetch a single IP Pool.", + "operationId": "ip_pools_get_ip_pool", + "parameters": [ + { + "in": "path", + "name": "pool_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpPool" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "tags": [ + "ip-pools" + ], + "summary": "Update an IP Pool.", + "operationId": "ip_pools_put_ip_pool", + "parameters": [ + { + "in": "path", + "name": "pool_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpPoolUpdate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpPool" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "delete": { + "tags": [ + "ip-pools" + ], + "summary": "Delete an IP Pool.", + "operationId": "ip_pools_delete_ip_pool", + "parameters": [ + { + "in": "path", + "name": "pool_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "responses": { + "204": { + "description": "successful deletion" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/ip-pools/{pool_name}/ranges": { + "get": { + "tags": [ + "ip-pools" + ], + "summary": "List the ranges of IP addresses within an existing IP Pool.", + "description": "Note that ranges are listed sorted by their first address.", + "operationId": "ip_pool_ranges_get", + "parameters": [ + { + "in": "path", + "name": "pool_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + }, + "style": "form" + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retrieve the subsequent page", + "schema": { + "nullable": true, + "type": "string" + }, + "style": "form" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpRangeResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": true + } + }, + "/ip-pools/{pool_name}/ranges/add": { + "post": { + "tags": [ + "ip-pools" + ], + "summary": "Add a new range to an existing IP Pool.", + "operationId": "ip_pool_ranges_add", + "parameters": [ + { + "in": "path", + "name": "pool_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpRange" + } + } + }, + "required": true + }, + "responses": { + "201": { + "description": "successful creation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpRange" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/ip-pools/{pool_name}/ranges/delete": { + "post": { + "tags": [ + "ip-pools" + ], + "summary": "Remove a range from an existing IP Pool.", + "operationId": "ip_pool_ranges_delete", + "parameters": [ + { + "in": "path", + "name": "pool_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpRange" + } + } + }, + "required": true + }, + "responses": { + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/login": { "post": { "tags": [ @@ -7031,6 +7390,143 @@ } ] }, + "IpPool": { + "description": "Identity-related metadata that's included in nearly all public API objects", + "type": "object", + "properties": { + "description": { + "description": "human-readable free-form text about a resource", + "type": "string" + }, + "id": { + "description": "unique, immutable, system-controlled identifier for each resource", + "type": "string", + "format": "uuid" + }, + "name": { + "description": "unique, mutable, user-controlled identifier for each resource", + "allOf": [ + { + "$ref": "#/components/schemas/Name" + } + ] + }, + "time_created": { + "description": "timestamp when this resource was created", + "type": "string", + "format": "date-time" + }, + "time_modified": { + "description": "timestamp when this resource was last modified", + "type": "string", + "format": "date-time" + } + }, + "required": [ + "description", + "id", + "name", + "time_created", + "time_modified" + ] + }, + "IpPoolCreate": { + "description": "Create-time parameters for an IP Pool.\n\nSee [`IpPool`](omicron_nexus::external_api::views::IpPool)", + "type": "object", + "properties": { + "description": { + "type": "string" + }, + "name": { + "$ref": "#/components/schemas/Name" + } + }, + "required": [ + "description", + "name" + ] + }, + "IpPoolResultsPage": { + "description": "A single page of results", + "type": "object", + "properties": { + "items": { + "description": "list of items on this page of results", + "type": "array", + "items": { + "$ref": "#/components/schemas/IpPool" + } + }, + "next_page": { + "nullable": true, + "description": "token used to fetch the next page of results (if any)", + "type": "string" + } + }, + "required": [ + "items" + ] + }, + "IpPoolUpdate": { + "description": "Parameters for updating an IP Pool", + "type": "object", + "properties": { + "description": { + "nullable": true, + "type": "string" + }, + "name": { + "nullable": true, + "allOf": [ + { + "$ref": "#/components/schemas/Name" + } + ] + } + } + }, + "IpRange": { + "type": "object", + "properties": { + "id": { + "type": "string", + "format": "uuid" + }, + "range": { + "$ref": "#/components/schemas/IpRange" + }, + "time_created": { + "type": "string", + "format": "date-time" + } + }, + "required": [ + "id", + "range", + "time_created" + ] + }, + "IpRangeResultsPage": { + "description": "A single page of results", + "type": "object", + "properties": { + "items": { + "description": "list of items on this page of results", + "type": "array", + "items": { + "$ref": "#/components/schemas/IpRange" + } + }, + "next_page": { + "nullable": true, + "description": "token used to fetch the next page of results (if any)", + "type": "string" + } + }, + "required": [ + "items" + ] + }, "Ipv4Net": { "example": "192.168.1.0/24", "title": "An IPv4 subnet", @@ -9715,6 +10211,13 @@ "url": "http://oxide.computer/docs/#xxx" } }, + { + "name": "ip-pools", + "description": "IP Pools contain subnets used for assigning external IP addresses to virtual machine instances.", + "externalDocs": { + "url": "http://oxide.computer/docs/#xxx" + } + }, { "name": "login", "description": "Authentication endpoints", From cc89d65aa4b886e36e8e426d26572bf168c63519 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Thu, 23 Jun 2022 18:06:22 +0000 Subject: [PATCH 2/6] Review feedback round 1 - Constructors and getters for IP range types to ensure they are non-decreasing - Tests of IP range types - Better conversions between model / view / public IP range types - Remove lots of dead code, some comment cleanup - Rename some types to avoid OpenAPI confusion --- common/src/api/external/mod.rs | 147 ++++++++++++-- nexus/src/db/datastore.rs | 4 +- nexus/src/db/model/ip_pool.rs | 35 ++-- nexus/src/db/multi_table_object.rs | 212 --------------------- nexus/src/external_api/http_entrypoints.rs | 23 +-- nexus/src/external_api/views.rs | 24 +-- nexus/tests/integration_tests/endpoints.rs | 8 +- nexus/tests/integration_tests/ip_pools.rs | 198 +++++++++++-------- openapi/nexus.json | 134 +++++++++---- 9 files changed, 376 insertions(+), 409 deletions(-) delete mode 100644 nexus/src/db/multi_table_object.rs diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index a0cb5fa794..63fa874b8d 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1306,7 +1306,7 @@ fn label_schema( /// An IP Range is a contiguous range of IP addresses, usually within an IP /// Pool. -#[derive(Clone, Copy, Debug, Deserialize, Serialize)] +#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize)] pub enum IpRange { V4(Ipv4Range), V6(Ipv6Range), @@ -1356,14 +1356,48 @@ impl IpRange { } } +impl TryFrom<(Ipv4Addr, Ipv4Addr)> for IpRange { + type Error = String; + + fn try_from(pair: (Ipv4Addr, Ipv4Addr)) -> Result { + Ipv4Range::new(pair.0, pair.1).map(IpRange::V4) + } +} + +impl TryFrom<(Ipv6Addr, Ipv6Addr)> for IpRange { + type Error = String; + + fn try_from(pair: (Ipv6Addr, Ipv6Addr)) -> Result { + Ipv6Range::new(pair.0, pair.1).map(IpRange::V6) + } +} + /// A non-decreasing IPv4 address range, inclusive of both ends. /// /// The first address must be less than or equal to the last address. -#[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] +#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] #[serde(try_from = "AnyIpv4Range")] pub struct Ipv4Range { - pub first: Ipv4Addr, - pub last: Ipv4Addr, + first: Ipv4Addr, + last: Ipv4Addr, +} + +impl Ipv4Range { + pub fn new(first: Ipv4Addr, last: Ipv4Addr) -> Result { + if first <= last { + Ok(Self { first, last }) + } else { + Err(String::from("IP address ranges must be non-decreasing")) + } + } + + pub fn first_address(&self) -> Ipv4Addr { + self.first + } + + pub fn last_address(&self) -> Ipv4Addr { + self.last + } } #[derive(Clone, Copy, Debug, Deserialize)] @@ -1375,24 +1409,37 @@ struct AnyIpv4Range { impl TryFrom for Ipv4Range { type Error = Error; fn try_from(r: AnyIpv4Range) -> Result { - if r.first <= r.last { - Ok(Self { first: r.first, last: r.last }) - } else { - Err(Error::invalid_request( - "IP address ranges must be non-decreasing", - )) - } + Ipv4Range::new(r.first, r.last) + .map_err(|msg| Error::invalid_request(msg.as_str())) } } /// A non-decreasing IPv6 address range, inclusive of both ends. /// /// The first address must be less than or equal to the last address. -#[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] +#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] #[serde(try_from = "AnyIpv6Range")] pub struct Ipv6Range { - pub first: Ipv6Addr, - pub last: Ipv6Addr, + first: Ipv6Addr, + last: Ipv6Addr, +} + +impl Ipv6Range { + pub fn new(first: Ipv6Addr, last: Ipv6Addr) -> Result { + if first <= last { + Ok(Self { first, last }) + } else { + Err(String::from("IP address ranges must be non-decreasing")) + } + } + + pub fn first_address(&self) -> Ipv6Addr { + self.first + } + + pub fn last_address(&self) -> Ipv6Addr { + self.last + } } #[derive(Clone, Copy, Debug, Deserialize)] @@ -1404,13 +1451,8 @@ struct AnyIpv6Range { impl TryFrom for Ipv6Range { type Error = Error; fn try_from(r: AnyIpv6Range) -> Result { - if r.first <= r.last { - Ok(Self { first: r.first, last: r.last }) - } else { - Err(Error::invalid_request( - "IP address ranges must be non-decreasing", - )) - } + Ipv6Range::new(r.first, r.last) + .map_err(|msg| Error::invalid_request(msg.as_str())) } } @@ -2054,6 +2096,9 @@ impl std::fmt::Display for Digest { #[cfg(test)] mod test { use super::IpNet; + use super::IpRange; + use super::Ipv4Range; + use super::Ipv6Range; use super::RouteDestination; use super::RouteTarget; use super::VpcFirewallRuleHostFilter; @@ -2068,6 +2113,8 @@ mod test { use crate::api::external::Error; use crate::api::external::ResourceType; use std::convert::TryFrom; + use std::net::Ipv4Addr; + use std::net::Ipv6Addr; use std::str::FromStr; #[test] @@ -2653,4 +2700,62 @@ mod test { IpAddr::from(Ipv4Addr::new(10, 0, 0, 0)), ); } + + #[test] + fn test_ip_range_checks_non_decreasing() { + let lo = Ipv4Addr::new(10, 0, 0, 1); + let hi = Ipv4Addr::new(10, 0, 0, 3); + assert!(Ipv4Range::new(lo, hi).is_ok()); + assert!(Ipv4Range::new(lo, lo).is_ok()); + assert!(Ipv4Range::new(hi, lo).is_err()); + + let lo = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1); + let hi = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3); + assert!(Ipv6Range::new(lo, hi).is_ok()); + assert!(Ipv6Range::new(lo, lo).is_ok()); + assert!(Ipv6Range::new(hi, lo).is_err()); + } + + #[test] + fn test_ip_range_enum_deserialization() { + let data = r#"{"V4": {"first": "10.0.0.1", "last": "10.0.0.3"}}"#; + let expected = IpRange::V4( + Ipv4Range::new( + Ipv4Addr::new(10, 0, 0, 1), + Ipv4Addr::new(10, 0, 0, 3), + ) + .unwrap(), + ); + assert_eq!(expected, serde_json::from_str(data).unwrap()); + + let data = r#"{"V6": {"first": "fd00::", "last": "fd00::3"}}"#; + let expected = IpRange::V6( + Ipv6Range::new( + Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), + Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3), + ) + .unwrap(), + ); + assert_eq!(expected, serde_json::from_str(data).unwrap()); + + let data = r#"{"V6": {"first": "fd00::3", "last": "fd00::"}}"#; + assert!( + serde_json::from_str::(data).is_err(), + "Expected an error deserializing an IP range with first address \ + greater than last address", + ); + } + + #[test] + fn test_ip_range_try_from() { + let lo = Ipv4Addr::new(10, 0, 0, 1); + let hi = Ipv4Addr::new(10, 0, 0, 3); + assert!(IpRange::try_from((lo, hi)).is_ok()); + assert!(IpRange::try_from((hi, lo)).is_err()); + + let lo = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1); + let hi = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3); + assert!(IpRange::try_from((lo, hi)).is_ok()); + assert!(IpRange::try_from((hi, lo)).is_err()); + } } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 29af343da3..9ba4f6502a 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1274,8 +1274,8 @@ impl DataStore { // We can just delete the range, provided that it the exact first/last // address pair exists. We are guaranteed that concurrent modifications - // don't affect this, since this query will be serialized with any to - // insert a new range, which must be non-overlapping. + // don't affect this, since this query will be serialized with any + // requests to insert a new range, which must be non-overlapping. let first_address = range.first_address(); let last_address = range.last_address(); let first_net = ipnetwork::IpNetwork::from(first_address); diff --git a/nexus/src/db/model/ip_pool.rs b/nexus/src/db/model/ip_pool.rs index aa793464fb..ab721561ee 100644 --- a/nexus/src/db/model/ip_pool.rs +++ b/nexus/src/db/model/ip_pool.rs @@ -95,24 +95,27 @@ impl IpPoolRange { } } -impl Into for IpPoolRange { - fn into(self) -> external::IpRange { - match (self.first_address.ip(), self.last_address.ip()) { - (IpAddr::V4(first), IpAddr::V4(last)) => { - external::IpRange::V4(external::Ipv4Range { first, last }) - } - (IpAddr::V6(first), IpAddr::V6(last)) => { - external::IpRange::V6(external::Ipv6Range { first, last }) - } - (first, last) => { - unreachable!( - "Expected first/last address of an IP range to \ +impl From<&IpPoolRange> for external::IpRange { + fn from(range: &IpPoolRange) -> Self { + let maybe_range = + match (range.first_address.ip(), range.last_address.ip()) { + (IpAddr::V4(first), IpAddr::V4(last)) => { + external::IpRange::try_from((first, last)) + } + (IpAddr::V6(first), IpAddr::V6(last)) => { + external::IpRange::try_from((first, last)) + } + (first, last) => { + unreachable!( + "Expected first/last address of an IP range to \ both be of the same protocol version, but first = {:?} \ and last = {:?}", - first, last - ); - } - } + first, last, + ); + } + }; + maybe_range + .expect("Retrieved an out-of-order IP range pair from the database") } } diff --git a/nexus/src/db/multi_table_object.rs b/nexus/src/db/multi_table_object.rs deleted file mode 100644 index 08bc188449..0000000000 --- a/nexus/src/db/multi_table_object.rs +++ /dev/null @@ -1,212 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// Copyright 2022 Oxide Computer Company - -//! Tools for working with a multi-table object in an atomic fashion. -//! -//! Database normalization is the process of structuring tables to reduce -//! duplication and increase integrity or validity checking. A very common -//! example is something like an `Employee` table, with a `Manager` column. In -//! the case where multiple people report to the same manager, those rows will -//! contain the same name or entry in the `Manager` column. Normalizing these -//! tables involes creating a new `Manager` table, and referring to the primary -//! key of that table in the `Employee` table. This enables storing all the -//! information about the manager in its own table, and referring to all that -//! data from the employee, by foreign key. -//! -//! There are several objects in the Oxide API where such a representation is -//! important. A good example is a VPC Firewall. This resource has its own ID, -//! name, and other metadata. However, it also refers to a potentially variable -//! number of firewall _rules_. One might store those inline in the firewall -//! table, as an array of objects for example. -//! -//! This has several downsides. First, it dramatically limits the indexing and -//! search operations that can be efficiently performed. Imagine we wanted to -//! search for a particular firewall rule. One either needs to create a full -//! inverted index on the rule column, or perform a table scan _and_ a -//! potentially linear search within the array of items. The former is very -//! space-inefficient, the latter time-inefficient. -//! -//! Instead, we could normalize these tables, putting the rules in a separate -//! table. That enables efficient querying with more standard indexes, such as -//! B-trees. -//! -//! There are problems here, however. We've split the "real" firewall object -//! into multiple tables. The firewall is not really a "collection" of rules, it -//! _is_ the rules. It has no independent identity beyond the list of rules that -//! make it up. Put another way, the primitive operation on a firewall is a -//! `PUT`, i.e., a complete replacement of all rules within it. One could do -//! that inside an interactive transaction, or with joins or other more -//! complicated operations such as common table expressions. -//! -//! This module provides tools for working with such normalized, multi-table -//! objects, in an atomic fashion using common table expressions. - -use diesel::Table; -use diesel::associations::BelongsTo; - -pub struct MultiTableObject { - parent: ParentModel, - children: Vec, -} - -/* -#[derive(Debug, Clone, Copy)] -pub struct MultiTableObject< - Model, - ParentTable - ParentModel, - ChildTable, - ChildModel -> { - parent_table: ParentTable, - parent_model: ParentModel, - child_table: ChildTable, - child_model: ChildModel, -} -*/ - -/* -// ChildTable: BelongsTo -// -// Want to basically have ability to CRUD this one object, and have that do the -// right operation on both tables. -// -// insert -> (insert parent, insert all children) -// list -> (join parent, children, grouped by id or similar) -// get -> (join parent, children with particular parent id) -// update -> (remove all children, insert all children, update parent rcgen) -// delete -> (remove all children, remove parent) -// -// -// So we'd have -// VpcFirewallParent -// VpcFirewallRule -// -// VpcFirewall is MultiTableObject; -// -// -// Problems: -// -// How do you list all of these? It looks like we'd need do use `grouped_by` to -// get a vec of these fuckers. Or generate an actual GROUP BY and HAVING -// - -impl MultiTableObject -where - Parent: Table + Default, - Child: Table + BelongsTo + Default, -{ - pub fn new() -> Self { - Self { parent: Parent::default(), child: Child::default() } - } - - pub fn insert(self, conn: &DbConnection) -> CreateResult { - /* - WITH - inserted_parent - AS ( - INSERT - INTO - p (id) - VALUES - ('cd9963c3-3507-c29e-baea-a79faf887122') - RETURNING - * - ), - inserted_children - AS ( - INSERT - INTO - c (id, p_id) - VALUES - ( - '90f1ab21-502e-edd6-b655-c165e504a85b', - 'cd9963c3-3507-c29e-baea-a79faf887122' - ), - ( - '04c64cb5-48dc-c430-ed58-a8fc8d77b2ea', - 'cd9963c3-3507-c29e-baea-a79faf887122' - ) - RETURNING - * - ) - SELECT - * - FROM - inserted_parent, inserted_children - WHERE - inserted_parent.id = inserted_children.p_id - AND inserted_parent.id - = 'cd9963c3-3507-c29e-baea-a79faf887122' - */ - todo!() - } - - pub fn list(conn: &DbConnection) -> ListResultVec { - todo!() - } - - pub fn get(conn: &DbConnection) -> ListResult { - /* - * SELECT * FROM - * parent, child - * WHERE - * parent.id = child.parent_id AND - * parent.id = AND - * parent.time_deleted IS NOT NULL; - */ - todo!() - } - - pub fn update(self, conn: &DbConnection) -> UpdateResult { - /* - * WITH - * updated_children AS ( - * UPDATE child SET = - * where parent.id = - * returning * - * ), - * updated_parent AS ( - * UPDATE parent SET - * (time_modified, rcgen) = (NOW(), rcgen + 1) - * where id = - * returning * - * ) - * select * from updated_parent, updated_children - * where updated_parent.id = updated_children.parent_id - * and updated_parent.id = - */ - todo!() - } - - pub fn delete(self, conn: &DbConnection) -> DeleteResult { - /* Delete children, delete parent, return...count? */ - todo!() - } -} -*/ - -macro_rules! multi_table_object { - ($model:ident, $parent:ty, $child:ty) => { - pub struct $model { - mto: crate::db::multi_table_object::MultiTableObject< - <$parent as ::diesel::associations::HasTable>::Table, - <$child as ::diesel::associations::HasTable>::Table, - >, - parent: $parent, - children: Vec<$child>, - } - - impl $model { - pub fn new(parent: $parent, children: &[$child]) -> Self { - assert!(!children.is_empty()); - let children = children.to_vec(); - let mto = crate::db::multi_table_object::MultiTableObject::new(&parent, &children); - Self { mto, parent, children } - } - } - } -} diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 96f7380d40..7ed0c06558 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -5,7 +5,7 @@ //! Handler functions (entrypoints) for external HTTP APIs use super::views::IpPool; -use super::views::IpRange; +use super::views::IpPoolRange; use super::{ console_api, params, views, views::{ @@ -116,13 +116,6 @@ pub fn external_api() -> NexusApiDescription { api.register(ip_pool_ranges_add)?; api.register(ip_pool_ranges_delete)?; - /* - api.register(ip_pool_cidr_blocks_get)?; - api.register(ip_pool_cidr_blocks_post)?; - api.register(ip_pool_cidr_blocks_get_cidr_block)?; - api.register(ip_pool_cidr_blocks_delete_cidr_block)?; - */ - api.register(project_disks_get)?; api.register(project_disks_post)?; api.register(project_disks_get_disk)?; @@ -1037,12 +1030,6 @@ pub struct IpPoolPathParam { pub pool_name: Name, } -#[derive(Deserialize, JsonSchema)] -pub struct IpPoolCidrBlockPathParam { - pub pool_name: Name, - pub block_name: Name, -} - /// List IP Pools. #[endpoint { method = GET, @@ -1183,10 +1170,8 @@ pub type IpPoolRangePaginationParams = async fn ip_pool_ranges_get( rqctx: Arc>>, path_params: Path, - // TODO(ben): Figure out how to paginate this query_params: Query, - //) -> Result>, HttpError> { -) -> Result>, HttpError> { +) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); @@ -1212,7 +1197,7 @@ async fn ip_pool_ranges_get( Ok(HttpResponseOk(ResultsPage::new( ranges, &EmptyScanParams {}, - |range: &IpRange, _| range.range.first_address(), + |range: &IpPoolRange, _| range.range.first_address(), )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -1228,7 +1213,7 @@ async fn ip_pool_ranges_add( rqctx: Arc>>, path_params: Path, range_params: TypedBody, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = &rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); diff --git a/nexus/src/external_api/views.rs b/nexus/src/external_api/views.rs index 37916e5f92..abd1ad9855 100644 --- a/nexus/src/external_api/views.rs +++ b/nexus/src/external_api/views.rs @@ -17,7 +17,6 @@ use omicron_common::api::external::{ }; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use std::net::IpAddr; use std::net::SocketAddrV6; use uuid::Uuid; @@ -332,28 +331,19 @@ impl From for IpPool { } #[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] -pub struct IpRange { +pub struct IpPoolRange { pub id: Uuid, pub time_created: DateTime, pub range: external::IpRange, } -impl From for IpRange { +impl From for IpPoolRange { fn from(range: model::IpPoolRange) -> Self { - let r = match (range.first_address.ip(), range.last_address.ip()) { - (IpAddr::V4(first), IpAddr::V4(last)) => { - external::IpRange::V4(external::Ipv4Range { first, last }) - } - (IpAddr::V6(first), IpAddr::V6(last)) => { - external::IpRange::V6(external::Ipv6Range { first, last }) - } - (first, last) => unreachable!( - "Expected first and last address from the same IP \ - protocol versions, found {:?} and {:?}", - first, last - ), - }; - Self { id: range.id, time_created: range.time_created, range: r } + Self { + id: range.id, + time_created: range.time_created, + range: external::IpRange::from(&range), + } } } diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 17d7bf44e9..98925ca637 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -292,10 +292,10 @@ lazy_static! { description: Some(String::from("a new IP pool")), }, }; - pub static ref DEMO_IP_POOL_RANGE: IpRange = IpRange::V4(Ipv4Range { - first: std::net::Ipv4Addr::new(10, 0, 0, 1), - last: std::net::Ipv4Addr::new(10, 0, 0, 2), - }); + pub static ref DEMO_IP_POOL_RANGE: IpRange = IpRange::V4(Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 2), + ).unwrap()); pub static ref DEMO_IP_POOL_RANGES_URL: String = format!("{}/ranges", *DEMO_IP_POOL_URL); pub static ref DEMO_IP_POOL_RANGES_ADD_URL: String = format!("{}/add", *DEMO_IP_POOL_RANGES_URL); pub static ref DEMO_IP_POOL_RANGES_DEL_URL: String = format!("{}/delete", *DEMO_IP_POOL_RANGES_URL); diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index f02f364c4d..bbb39c85ce 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -22,7 +22,7 @@ use omicron_common::api::external::Ipv6Range; use omicron_nexus::external_api::params::IpPoolCreate; use omicron_nexus::external_api::params::IpPoolUpdate; use omicron_nexus::external_api::views::IpPool; -use omicron_nexus::external_api::views::IpRange as IpRangeView; +use omicron_nexus::external_api::views::IpPoolRange; // Basic test verifying CRUD behavior on the IP Pool itself. #[nexus_test] @@ -143,11 +143,14 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) { ); // Add a range, verify that we can't delete the Pool - let range = IpRange::V4(Ipv4Range { - first: "10.0.0.1".parse().unwrap(), - last: "10.0.0.5".parse().unwrap(), - }); - let created_range: IpRangeView = + let range = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 5), + ) + .unwrap(), + ); + let created_range: IpPoolRange = NexusRequest::objects_post(client, &ip_pool_add_range_url, &range) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -240,14 +243,6 @@ async fn test_ip_pool_basic_crud(cptestctx: &ControlPlaneTestContext) { // Delete the range, then verify we can delete the pool and everything looks // gravy. - /* - * Most clients should expect 201, which isn't really what we're doing here. - NexusRequest::objects_post(client, &new_ip_pool_del_range_url, &range) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("Failed to delete IP range within pool"); - */ NexusRequest::new( RequestBuilder::new(client, Method::POST, &new_ip_pool_del_range_url) .body(Some(&range)) @@ -306,72 +301,108 @@ async fn test_ip_pool_range_overlapping_ranges_fails( // Test data for IPv4 ranges that should fail due to overlap let ipv4_range = TestRange { - base_range: IpRange::V4(Ipv4Range { - first: "10.0.0.2".parse().unwrap(), - last: "10.0.0.5".parse().unwrap(), - }), + base_range: IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 2), + std::net::Ipv4Addr::new(10, 0, 0, 5), + ) + .unwrap(), + ), bad_ranges: vec![ // The exact same range - IpRange::V4(Ipv4Range { - first: "10.0.0.2".parse().unwrap(), - last: "10.0.0.5".parse().unwrap(), - }), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 2), + std::net::Ipv4Addr::new(10, 0, 0, 5), + ) + .unwrap(), + ), // Overlaps below - IpRange::V4(Ipv4Range { - first: "10.0.0.1".parse().unwrap(), - last: "10.0.0.2".parse().unwrap(), - }), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 2), + ) + .unwrap(), + ), // Overlaps above - IpRange::V4(Ipv4Range { - first: "10.0.0.5".parse().unwrap(), - last: "10.0.0.6".parse().unwrap(), - }), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 5), + std::net::Ipv4Addr::new(10, 0, 0, 6), + ) + .unwrap(), + ), // Contains the base range - IpRange::V4(Ipv4Range { - first: "10.0.0.1".parse().unwrap(), - last: "10.0.0.6".parse().unwrap(), - }), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 6), + ) + .unwrap(), + ), // Contained by the base range - IpRange::V4(Ipv4Range { - first: "10.0.0.1".parse().unwrap(), - last: "10.0.0.6".parse().unwrap(), - }), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 3), + std::net::Ipv4Addr::new(10, 0, 0, 4), + ) + .unwrap(), + ), ], }; 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 { - first: "fd00::10".parse().unwrap(), - last: "fd00::20".parse().unwrap(), - }), + 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 { - first: "fd00::10".parse().unwrap(), - last: "fd00::20".parse().unwrap(), - }), + 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 { - first: "fd00::5".parse().unwrap(), - last: "fd00::10".parse().unwrap(), - }), + 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 { - first: "fd00::15".parse().unwrap(), - last: "fd00::25".parse().unwrap(), - }), + 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 { - first: "fd00::".parse().unwrap(), - last: "fd00::100".parse().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, 100), + ) + .unwrap(), + ), // Contained by the base range - IpRange::V6(Ipv6Range { - first: "fd00::12".parse().unwrap(), - last: "fd00::12".parse().unwrap(), - }), + 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; @@ -382,7 +413,7 @@ async fn test_bad_ip_ranges( url: &str, ranges: &TestRange, ) { - let created_range: IpRangeView = + let created_range: IpPoolRange = NexusRequest::objects_post(client, url, &ranges.base_range) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -453,23 +484,32 @@ 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 { - first: "fd00::11".parse().unwrap(), - last: "fd00::20".parse().unwrap(), - }), - IpRange::V6(Ipv6Range { - first: "fd00::".parse().unwrap(), - last: "fd00::10".parse().unwrap(), - }), - IpRange::V4(Ipv4Range { - first: "10.0.0.1".parse().unwrap(), - last: "10.0.0.2".parse().unwrap(), - }), + 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), + ) + .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), + ) + .unwrap(), + ), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 2), + ) + .unwrap(), + ), ]; let mut expected_ranges = Vec::with_capacity(ranges.len()); for range in ranges.iter() { - let created_range: IpRangeView = + let created_range: IpPoolRange = NexusRequest::objects_post(client, &ip_pool_add_range_url, &range) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -488,7 +528,7 @@ async fn test_ip_pool_range_pagination(cptestctx: &ControlPlaneTestContext) { // their first address. let first_page_url = format!("{}?limit=2", ip_pool_ranges_url); let first_page = - objects_list_page_authz::(client, &first_page_url).await; + objects_list_page_authz::(client, &first_page_url).await; assert_eq!(first_page.items.len(), 2); let second_page_url = format!( @@ -497,7 +537,7 @@ async fn test_ip_pool_range_pagination(cptestctx: &ControlPlaneTestContext) { first_page.next_page.unwrap() ); let second_page = - objects_list_page_authz::(client, &second_page_url).await; + objects_list_page_authz::(client, &second_page_url).await; assert_eq!(second_page.items.len(), 1); let actual_ranges = first_page.items.iter().chain(second_page.items.iter()); @@ -512,7 +552,7 @@ fn assert_pools_eq(first: &IpPool, second: &IpPool) { assert_eq!(first.identity, second.identity); } -fn assert_ranges_eq(first: &IpRangeView, second: &IpRangeView) { +fn assert_ranges_eq(first: &IpPoolRange, second: &IpPoolRange) { assert_eq!(first.id, second.id); assert_eq!(first.time_created, second.time_created); assert_eq!(first.range.first_address(), second.range.first_address()); diff --git a/openapi/nexus.json b/openapi/nexus.json index 286d54b607..5ee0ed2016 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -631,7 +631,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/IpRangeResultsPage" + "$ref": "#/components/schemas/IpPoolRangeResultsPage" } } } @@ -680,7 +680,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/IpRange" + "$ref": "#/components/schemas/IpPoolRange" } } } @@ -7446,6 +7446,48 @@ "name" ] }, + "IpPoolRange": { + "type": "object", + "properties": { + "id": { + "type": "string", + "format": "uuid" + }, + "range": { + "$ref": "#/components/schemas/IpRange" + }, + "time_created": { + "type": "string", + "format": "date-time" + } + }, + "required": [ + "id", + "range", + "time_created" + ] + }, + "IpPoolRangeResultsPage": { + "description": "A single page of results", + "type": "object", + "properties": { + "items": { + "description": "list of items on this page of results", + "type": "array", + "items": { + "$ref": "#/components/schemas/IpPoolRange" + } + }, + "next_page": { + "nullable": true, + "description": "token used to fetch the next page of results (if any)", + "type": "string" + } + }, + "required": [ + "items" + ] + }, "IpPoolResultsPage": { "description": "A single page of results", "type": "object", @@ -7486,45 +7528,23 @@ } }, "IpRange": { - "type": "object", - "properties": { - "id": { - "type": "string", - "format": "uuid" - }, - "range": { - "$ref": "#/components/schemas/IpRange" - }, - "time_created": { - "type": "string", - "format": "date-time" - } - }, - "required": [ - "id", - "range", - "time_created" - ] - }, - "IpRangeResultsPage": { - "description": "A single page of results", - "type": "object", - "properties": { - "items": { - "description": "list of items on this page of results", - "type": "array", - "items": { - "$ref": "#/components/schemas/IpRange" - } + "oneOf": [ + { + "title": "v4", + "allOf": [ + { + "$ref": "#/components/schemas/Ipv4Range" + } + ] }, - "next_page": { - "nullable": true, - "description": "token used to fetch the next page of results (if any)", - "type": "string" + { + "title": "v6", + "allOf": [ + { + "$ref": "#/components/schemas/Ipv6Range" + } + ] } - }, - "required": [ - "items" ] }, "Ipv4Net": { @@ -7535,6 +7555,24 @@ "pattern": "(^(10\\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9]\\.){2}(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])/(1[0-9]|2[0-8]|[8-9]))$)|(^(172\\.16\\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])\\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])/(1[2-9]|2[0-8]))$)|(^(192\\.168\\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])\\.(25[0-5]|[1-2][0-4][0-9]|[1-9][0-9]|[0-9])/(1[6-9]|2[0-8]))$)", "maxLength": 18 }, + "Ipv4Range": { + "description": "A non-decreasing IPv4 address range, inclusive of both ends.\n\nThe first address must be less than or equal to the last address.", + "type": "object", + "properties": { + "first": { + "type": "string", + "format": "ipv4" + }, + "last": { + "type": "string", + "format": "ipv4" + } + }, + "required": [ + "first", + "last" + ] + }, "Ipv6Net": { "example": "fd12:3456::/64", "title": "An IPv6 subnet", @@ -7543,6 +7581,24 @@ "pattern": "^(fd|FD)[0-9a-fA-F]{2}:((([0-9a-fA-F]{1,4}\\:){6}[0-9a-fA-F]{1,4})|(([0-9a-fA-F]{1,4}:){1,6}:))/(6[4-9]|[7-9][0-9]|1[0-1][0-9]|12[0-6])$", "maxLength": 43 }, + "Ipv6Range": { + "description": "A non-decreasing IPv6 address range, inclusive of both ends.\n\nThe first address must be less than or equal to the last address.", + "type": "object", + "properties": { + "first": { + "type": "string", + "format": "ipv6" + }, + "last": { + "type": "string", + "format": "ipv6" + } + }, + "required": [ + "first", + "last" + ] + }, "L4PortRange": { "example": "22", "title": "A range of IP ports", From a20d28f44abd5750e7fcd9201dbc009e21e2d8b9 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Thu, 23 Jun 2022 20:26:01 +0000 Subject: [PATCH 3/6] Better pagination for listing IP Pool ranges --- nexus/src/app/ip_pool.rs | 4 +-- nexus/src/db/datastore.rs | 41 ++++++++-------------- nexus/src/external_api/http_entrypoints.rs | 39 +++++++++++++++----- openapi/nexus.json | 16 +++++++++ 4 files changed, 63 insertions(+), 37 deletions(-) diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 956a7396ce..d9bb5e39da 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -10,6 +10,7 @@ use crate::db; use crate::db::lookup::LookupPath; use crate::db::model::Name; use crate::external_api::params; +use ipnetwork::IpNetwork; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; @@ -17,7 +18,6 @@ use omicron_common::api::external::IpRange; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::UpdateResult; -use std::net::IpAddr; use uuid::Uuid; impl super::Nexus { @@ -89,7 +89,7 @@ impl super::Nexus { &self, opctx: &OpContext, pool_name: &Name, - pagparams: &DataPageParams<'_, IpAddr>, + pagparams: &DataPageParams<'_, IpNetwork>, ) -> ListResultVec { let (.., authz_pool) = LookupPath::new(opctx, &self.db_datastore) .ip_pool_name(pool_name) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 9ba4f6502a..ceefdee75a 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -80,6 +80,7 @@ use diesel::query_builder::{QueryFragment, QueryId}; use diesel::query_dsl::methods::LoadQuery; use diesel::upsert::excluded; use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; +use ipnetwork::IpNetwork; use omicron_common::api; use omicron_common::api::external; use omicron_common::api::external::DataPageParams; @@ -1180,36 +1181,22 @@ impl DataStore { &self, opctx: &OpContext, authz_pool: &authz::IpPool, - pag_params: &DataPageParams<'_, std::net::IpAddr>, + pag_params: &DataPageParams<'_, IpNetwork>, ) -> ListResultVec { use db::schema::ip_pool_range::dsl; opctx.authorize(authz::Action::ListChildren, authz_pool).await?; - let limit = pag_params.limit.get().into(); - let res = if let Some(last_seen_addr) = pag_params.marker { - let addr = ipnetwork::IpNetwork::from(*last_seen_addr); - dsl::ip_pool_range - .filter(dsl::ip_pool_id.eq(authz_pool.id())) - .filter(dsl::time_deleted.is_null()) - .filter(dsl::first_address.gt(addr)) - .order(dsl::first_address) - .limit(limit) - .select(IpPoolRange::as_select()) - .get_results_async(self.pool_authorized(opctx).await?) - } else { - dsl::ip_pool_range - .filter(dsl::ip_pool_id.eq(authz_pool.id())) - .filter(dsl::time_deleted.is_null()) - .order(dsl::first_address) - .limit(limit) - .select(IpPoolRange::as_select()) - .get_results_async(self.pool_authorized(opctx).await?) - }; - res.await.map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByResource(authz_pool), - ) - }) + paginated(dsl::ip_pool_range, dsl::first_address, pag_params) + .filter(dsl::ip_pool_id.eq(authz_pool.id())) + .filter(dsl::time_deleted.is_null()) + .select(IpPoolRange::as_select()) + .get_results_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByResource(authz_pool), + ) + }) } pub async fn ip_pool_add_range( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 7ed0c06558..9f886d2912 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -36,6 +36,7 @@ use dropshot::RequestContext; use dropshot::ResultsPage; use dropshot::TypedBody; use dropshot::WhichPage; +use ipnetwork::IpNetwork; use omicron_common::api::external; use omicron_common::api::external::http_pagination::data_page_params_nameid_id; use omicron_common::api::external::http_pagination::data_page_params_nameid_name; @@ -1156,8 +1157,25 @@ async fn ip_pools_put_ip_pool( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -pub type IpPoolRangePaginationParams = - PaginationParams; +/// Parameters for controlling pagination of IP Pool ranges +#[derive(Debug, Clone, Copy, Serialize, Deserialize, JsonSchema)] +pub struct IpPoolRangeScanParams { + #[serde(default = "default_ip_pool_range_order")] + direction: PaginationOrder, +} + +fn default_ip_pool_range_order() -> PaginationOrder { + PaginationOrder::Ascending +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize)] +struct IpPoolRangePageSelector { + scan_params: IpPoolRangeScanParams, + last_seen_address: IpNetwork, +} + +type IpPoolRangePaginationParams = + PaginationParams; /// List the ranges of IP addresses within an existing IP Pool. /// @@ -1179,13 +1197,15 @@ async fn ip_pool_ranges_get( let pool_name = &path.pool_name; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let marker = match query.page { - WhichPage::First(_) => None, - WhichPage::Next(ref last_address) => Some(last_address), + let (scan_params, marker) = match query.page { + WhichPage::First(ref scan_params) => (scan_params, None), + WhichPage::Next(ref selector) => { + (&selector.scan_params, Some(&selector.last_seen_address)) + } }; let pag_params = DataPageParams { limit: rqctx.page_limit(&query)?, - direction: PaginationOrder::Ascending, + direction: scan_params.direction, marker, }; let ranges = nexus @@ -1196,8 +1216,11 @@ async fn ip_pool_ranges_get( .collect(); Ok(HttpResponseOk(ResultsPage::new( ranges, - &EmptyScanParams {}, - |range: &IpPoolRange, _| range.range.first_address(), + scan_params, + |range: &IpPoolRange, scan_params| IpPoolRangePageSelector { + scan_params: scan_params.clone(), + last_seen_address: IpNetwork::from(range.range.first_address()), + }, )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/openapi/nexus.json b/openapi/nexus.json index 5ee0ed2016..9c980d28e0 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -602,6 +602,14 @@ }, "style": "simple" }, + { + "in": "query", + "name": "direction", + "schema": { + "$ref": "#/components/schemas/PaginationOrder" + }, + "style": "form" + }, { "in": "query", "name": "limit", @@ -10221,6 +10229,14 @@ "name_descending", "id_ascending" ] + }, + "PaginationOrder": { + "description": "The order in which the client wants to page through the requested collection", + "type": "string", + "enum": [ + "ascending", + "descending" + ] } } }, From 2f5733db1f64f9b508144d8d0957d81603207cc5 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Thu, 23 Jun 2022 20:29:32 +0000 Subject: [PATCH 4/6] appease clippy --- nexus/src/external_api/http_entrypoints.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 9f886d2912..d9a976c6f8 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1218,7 +1218,7 @@ async fn ip_pool_ranges_get( ranges, scan_params, |range: &IpPoolRange, scan_params| IpPoolRangePageSelector { - scan_params: scan_params.clone(), + scan_params: *scan_params, last_seen_address: IpNetwork::from(range.range.first_address()), }, )?)) From e9850d742fddd8de1fb4e67a18934fe7ec11d3b4 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Thu, 23 Jun 2022 23:25:51 +0000 Subject: [PATCH 5/6] Review feedback round 2 - Move IpRange types to shared Nexus crate - Remove more dead code - Cleanup some comments --- common/src/api/external/mod.rs | 220 +------------------- common/src/sql/dbinit.sql | 14 +- nexus/src/app/ip_pool.rs | 2 +- nexus/src/authz/api_resources.rs | 10 - nexus/src/db/datastore.rs | 4 +- nexus/src/db/model/ip_pool.rs | 11 +- nexus/src/external_api/http_entrypoints.rs | 5 +- nexus/src/external_api/params.rs | 26 --- nexus/src/external_api/shared.rs | 227 +++++++++++++++++++++ nexus/src/external_api/tag-config.json | 2 +- nexus/src/external_api/views.rs | 6 +- nexus/tests/integration_tests/endpoints.rs | 4 +- nexus/tests/integration_tests/ip_pools.rs | 6 +- openapi/nexus.json | 2 +- 14 files changed, 256 insertions(+), 283 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 63fa874b8d..de2f5d4401 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1280,7 +1280,10 @@ impl JsonSchema for IpNet { /// Insert another level of schema indirection in order to provide an /// additional title for a subschema. This allows generators to infer a better /// variant name for an "untagged" enum. -fn label_schema( +// TODO-cleanup: We should move IpNet and this to +// `omicron_nexus::external_api::shared`. It's public now because `IpRange`, +// which is defined there, uses it. +pub fn label_schema( label: &str, schema: schemars::schema::Schema, ) -> schemars::schema::Schema { @@ -1304,158 +1307,6 @@ fn label_schema( .into() } -/// An IP Range is a contiguous range of IP addresses, usually within an IP -/// Pool. -#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize)] -pub enum IpRange { - V4(Ipv4Range), - V6(Ipv6Range), -} - -impl JsonSchema for IpRange { - fn schema_name() -> String { - "IpRange".to_string() - } - - fn json_schema( - gen: &mut schemars::gen::SchemaGenerator, - ) -> schemars::schema::Schema { - schemars::schema::SchemaObject { - metadata: Some( - schemars::schema::Metadata { ..Default::default() }.into(), - ), - subschemas: Some( - schemars::schema::SubschemaValidation { - one_of: Some(vec![ - label_schema("v4", gen.subschema_for::()), - label_schema("v6", gen.subschema_for::()), - ]), - ..Default::default() - } - .into(), - ), - ..Default::default() - } - .into() - } -} - -impl IpRange { - pub fn first_address(&self) -> IpAddr { - match self { - IpRange::V4(inner) => IpAddr::from(inner.first), - IpRange::V6(inner) => IpAddr::from(inner.first), - } - } - - pub fn last_address(&self) -> IpAddr { - match self { - IpRange::V4(inner) => IpAddr::from(inner.last), - IpRange::V6(inner) => IpAddr::from(inner.last), - } - } -} - -impl TryFrom<(Ipv4Addr, Ipv4Addr)> for IpRange { - type Error = String; - - fn try_from(pair: (Ipv4Addr, Ipv4Addr)) -> Result { - Ipv4Range::new(pair.0, pair.1).map(IpRange::V4) - } -} - -impl TryFrom<(Ipv6Addr, Ipv6Addr)> for IpRange { - type Error = String; - - fn try_from(pair: (Ipv6Addr, Ipv6Addr)) -> Result { - Ipv6Range::new(pair.0, pair.1).map(IpRange::V6) - } -} - -/// A non-decreasing IPv4 address range, inclusive of both ends. -/// -/// The first address must be less than or equal to the last address. -#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] -#[serde(try_from = "AnyIpv4Range")] -pub struct Ipv4Range { - first: Ipv4Addr, - last: Ipv4Addr, -} - -impl Ipv4Range { - pub fn new(first: Ipv4Addr, last: Ipv4Addr) -> Result { - if first <= last { - Ok(Self { first, last }) - } else { - Err(String::from("IP address ranges must be non-decreasing")) - } - } - - pub fn first_address(&self) -> Ipv4Addr { - self.first - } - - pub fn last_address(&self) -> Ipv4Addr { - self.last - } -} - -#[derive(Clone, Copy, Debug, Deserialize)] -struct AnyIpv4Range { - first: Ipv4Addr, - last: Ipv4Addr, -} - -impl TryFrom for Ipv4Range { - type Error = Error; - fn try_from(r: AnyIpv4Range) -> Result { - Ipv4Range::new(r.first, r.last) - .map_err(|msg| Error::invalid_request(msg.as_str())) - } -} - -/// A non-decreasing IPv6 address range, inclusive of both ends. -/// -/// The first address must be less than or equal to the last address. -#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] -#[serde(try_from = "AnyIpv6Range")] -pub struct Ipv6Range { - first: Ipv6Addr, - last: Ipv6Addr, -} - -impl Ipv6Range { - pub fn new(first: Ipv6Addr, last: Ipv6Addr) -> Result { - if first <= last { - Ok(Self { first, last }) - } else { - Err(String::from("IP address ranges must be non-decreasing")) - } - } - - pub fn first_address(&self) -> Ipv6Addr { - self.first - } - - pub fn last_address(&self) -> Ipv6Addr { - self.last - } -} - -#[derive(Clone, Copy, Debug, Deserialize)] -struct AnyIpv6Range { - first: Ipv6Addr, - last: Ipv6Addr, -} - -impl TryFrom for Ipv6Range { - type Error = Error; - fn try_from(r: AnyIpv6Range) -> Result { - Ipv6Range::new(r.first, r.last) - .map_err(|msg| Error::invalid_request(msg.as_str())) - } -} - /// A `RouteTarget` describes the possible locations that traffic matching a /// route destination can be sent. #[derive( @@ -2096,9 +1947,6 @@ impl std::fmt::Display for Digest { #[cfg(test)] mod test { use super::IpNet; - use super::IpRange; - use super::Ipv4Range; - use super::Ipv6Range; use super::RouteDestination; use super::RouteTarget; use super::VpcFirewallRuleHostFilter; @@ -2113,8 +1961,6 @@ mod test { use crate::api::external::Error; use crate::api::external::ResourceType; use std::convert::TryFrom; - use std::net::Ipv4Addr; - use std::net::Ipv6Addr; use std::str::FromStr; #[test] @@ -2700,62 +2546,4 @@ mod test { IpAddr::from(Ipv4Addr::new(10, 0, 0, 0)), ); } - - #[test] - fn test_ip_range_checks_non_decreasing() { - let lo = Ipv4Addr::new(10, 0, 0, 1); - let hi = Ipv4Addr::new(10, 0, 0, 3); - assert!(Ipv4Range::new(lo, hi).is_ok()); - assert!(Ipv4Range::new(lo, lo).is_ok()); - assert!(Ipv4Range::new(hi, lo).is_err()); - - let lo = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1); - let hi = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3); - assert!(Ipv6Range::new(lo, hi).is_ok()); - assert!(Ipv6Range::new(lo, lo).is_ok()); - assert!(Ipv6Range::new(hi, lo).is_err()); - } - - #[test] - fn test_ip_range_enum_deserialization() { - let data = r#"{"V4": {"first": "10.0.0.1", "last": "10.0.0.3"}}"#; - let expected = IpRange::V4( - Ipv4Range::new( - Ipv4Addr::new(10, 0, 0, 1), - Ipv4Addr::new(10, 0, 0, 3), - ) - .unwrap(), - ); - assert_eq!(expected, serde_json::from_str(data).unwrap()); - - let data = r#"{"V6": {"first": "fd00::", "last": "fd00::3"}}"#; - let expected = IpRange::V6( - Ipv6Range::new( - Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), - Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3), - ) - .unwrap(), - ); - assert_eq!(expected, serde_json::from_str(data).unwrap()); - - let data = r#"{"V6": {"first": "fd00::3", "last": "fd00::"}}"#; - assert!( - serde_json::from_str::(data).is_err(), - "Expected an error deserializing an IP range with first address \ - greater than last address", - ); - } - - #[test] - fn test_ip_range_try_from() { - let lo = Ipv4Addr::new(10, 0, 0, 1); - let hi = Ipv4Addr::new(10, 0, 0, 3); - assert!(IpRange::try_from((lo, hi)).is_ok()); - assert!(IpRange::try_from((hi, lo)).is_err()); - - let lo = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1); - let hi = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3); - assert!(IpRange::try_from((lo, hi)).is_ok()); - assert!(IpRange::try_from((hi, lo)).is_err()); - } } diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 39ab59a7c8..9471a71392 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -946,14 +946,6 @@ CREATE TABLE omicron.public.ip_pool ( /* The collection's child-resource generation number */ rcgen INT8 NOT NULL - - /* - * TODO-completeness: These will need some enum describing what the pool can - * be used for. That can currently be: - * - Routing - * - Oxide public use, e.g., Nexus's public API or the console - * - Virtual machine instance NAT - */ ); /* @@ -975,13 +967,15 @@ CREATE TABLE omicron.public.ip_pool_range ( time_modified TIMESTAMPTZ NOT NULL, time_deleted TIMESTAMPTZ, first_address INET NOT NULL, + /* The range is inclusive of the last address. */ last_address INET NOT NULL, ip_pool_id UUID NOT NULL ); /* - * These indexes are currently used to enforce that the ranges within an IP Pool - * do not overlap with any other ranges. + * These help Nexus enforce that the ranges within an IP Pool do not overlap + * with any other ranges. See `nexus/src/db/queries/ip_pool.rs` for the actual + * query which does that. */ CREATE UNIQUE INDEX ON omicron.public.ip_pool_range ( first_address diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index d9bb5e39da..2e7fd8e147 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -10,11 +10,11 @@ use crate::db; use crate::db::lookup::LookupPath; use crate::db::model::Name; use crate::external_api::params; +use crate::external_api::shared::IpRange; use ipnetwork::IpNetwork; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; -use omicron_common::api::external::IpRange; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::UpdateResult; diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index 3383b0897f..a17568a5b7 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -744,16 +744,6 @@ authz_resource! { polar_snippet = FleetChild, } -/* -authz_resource! { - name = "IpPoolCidrBlock", - parent = "IpPool", - primary_key = Uuid, - roles_allowed = false, - polar_snippet = Custom, -} -*/ - #[cfg(test)] mod test { use super::FleetRole; diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index ceefdee75a..d945046447 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -70,6 +70,7 @@ use crate::db::{ pagination::paginated_multicolumn, update_and_check::{UpdateAndCheck, UpdateStatus}, }; +use crate::external_api::shared::IpRange; use crate::external_api::{params, shared}; use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl, ConnectionManager}; use chrono::Utc; @@ -86,7 +87,6 @@ use omicron_common::api::external; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; -use omicron_common::api::external::IpRange; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::LookupType; @@ -1085,8 +1085,6 @@ impl DataStore { let pool_name = pool.name().as_str().to_string(); diesel::insert_into(dsl::ip_pool) .values(pool) - .on_conflict(dsl::id) - .do_nothing() .returning(IpPool::as_returning()) .get_result_async(self.pool_authorized(opctx).await?) .await diff --git a/nexus/src/db/model/ip_pool.rs b/nexus/src/db/model/ip_pool.rs index ab721561ee..a0a9353196 100644 --- a/nexus/src/db/model/ip_pool.rs +++ b/nexus/src/db/model/ip_pool.rs @@ -9,6 +9,7 @@ use crate::db::model::Name; use crate::db::schema::ip_pool; use crate::db::schema::ip_pool_range; use crate::external_api::params; +use crate::external_api::shared::IpRange; use chrono::DateTime; use chrono::Utc; use db_macros::Resource; @@ -75,10 +76,12 @@ pub struct IpPoolRange { } impl IpPoolRange { - pub fn new(range: &external::IpRange, ip_pool_id: Uuid) -> Self { + pub fn new(range: &IpRange, ip_pool_id: Uuid) -> Self { let now = Utc::now(); let first_address = range.first_address(); let last_address = range.last_address(); + // `range` has already been validated to have first address no greater + // than last address. assert!( last_address >= first_address, "Address ranges must be non-decreasing" @@ -95,15 +98,15 @@ impl IpPoolRange { } } -impl From<&IpPoolRange> for external::IpRange { +impl From<&IpPoolRange> for IpRange { fn from(range: &IpPoolRange) -> Self { let maybe_range = match (range.first_address.ip(), range.last_address.ip()) { (IpAddr::V4(first), IpAddr::V4(last)) => { - external::IpRange::try_from((first, last)) + IpRange::try_from((first, last)) } (IpAddr::V6(first), IpAddr::V6(last)) => { - external::IpRange::try_from((first, last)) + IpRange::try_from((first, last)) } (first, last) => { unreachable!( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d9a976c6f8..c868371c83 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -37,7 +37,6 @@ use dropshot::ResultsPage; use dropshot::TypedBody; use dropshot::WhichPage; use ipnetwork::IpNetwork; -use omicron_common::api::external; use omicron_common::api::external::http_pagination::data_page_params_nameid_id; use omicron_common::api::external::http_pagination::data_page_params_nameid_name; use omicron_common::api::external::http_pagination::pagination_field_for_scan_params; @@ -1235,7 +1234,7 @@ async fn ip_pool_ranges_get( async fn ip_pool_ranges_add( rqctx: Arc>>, path_params: Path, - range_params: TypedBody, + range_params: TypedBody, ) -> Result, HttpError> { let apictx = &rqctx.context(); let nexus = &apictx.nexus; @@ -1259,7 +1258,7 @@ async fn ip_pool_ranges_add( async fn ip_pool_ranges_delete( rqctx: Arc>>, path_params: Path, - range_params: TypedBody, + range_params: TypedBody, ) -> Result { let apictx = &rqctx.context(); let nexus = &apictx.nexus; diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 94852cfc95..e3863cba87 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -4,8 +4,6 @@ //! Params define the request bodies of API endpoints for creating or updating resources. -use omicron_common::api::external::IpNet; -use omicron_common::api::external::IpRange; use omicron_common::api::external::{ ByteCount, IdentityMetadataCreateParams, IdentityMetadataUpdateParams, InstanceCpuCount, Ipv4Net, Ipv6Net, Name, @@ -330,30 +328,6 @@ pub struct IpPoolUpdate { pub identity: IdentityMetadataUpdateParams, } -/// Parameters for adding a new IP range to an IP Pool -#[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] -pub enum IpRangeAdd { - Network(IpNet), - Range(IpRange), -} - -/// Parameters for creating a new CIDR block within an existing IP Pool. -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct IpPoolCidrBlockCreate { - #[serde(flatten)] - pub identity: IdentityMetadataCreateParams, - - /// The IP subnet, expressed as a CIDR block. - pub cidr_block: IpNet, -} - -/// Parameters for updating an existing CIDR block within an IP Pool. -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct IpPoolCidrBlockUpdate { - #[serde(flatten)] - pub identity: IdentityMetadataUpdateParams, -} - // INSTANCES pub const MIN_MEMORY_SIZE_BYTES: u32 = 1 << 30; // 1 GiB diff --git a/nexus/src/external_api/shared.rs b/nexus/src/external_api/shared.rs index 62a0f35169..833596bbc5 100644 --- a/nexus/src/external_api/shared.rs +++ b/nexus/src/external_api/shared.rs @@ -12,6 +12,9 @@ use serde::de::Error as _; use serde::Deserialize; use serde::Deserializer; use serde::Serialize; +use std::net::IpAddr; +use std::net::Ipv4Addr; +use std::net::Ipv6Addr; use uuid::Uuid; /// Maximum number of role assignments allowed on any one resource @@ -132,6 +135,167 @@ impl TryFrom for IdentityType { } } +/// An IP Range is a contiguous range of IP addresses, usually within an IP +/// Pool. +/// +/// The first address in the range is guaranteed to be no greater than the last +/// address. +#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize)] +pub enum IpRange { + V4(Ipv4Range), + V6(Ipv6Range), +} + +impl JsonSchema for IpRange { + fn schema_name() -> String { + "IpRange".to_string() + } + + fn json_schema( + gen: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + schemars::schema::SchemaObject { + metadata: Some( + schemars::schema::Metadata { ..Default::default() }.into(), + ), + subschemas: Some( + schemars::schema::SubschemaValidation { + one_of: Some(vec![ + omicron_common::api::external::label_schema( + "v4", + gen.subschema_for::(), + ), + omicron_common::api::external::label_schema( + "v6", + gen.subschema_for::(), + ), + ]), + ..Default::default() + } + .into(), + ), + ..Default::default() + } + .into() + } +} + +impl IpRange { + pub fn first_address(&self) -> IpAddr { + match self { + IpRange::V4(inner) => IpAddr::from(inner.first), + IpRange::V6(inner) => IpAddr::from(inner.first), + } + } + + pub fn last_address(&self) -> IpAddr { + match self { + IpRange::V4(inner) => IpAddr::from(inner.last), + IpRange::V6(inner) => IpAddr::from(inner.last), + } + } +} + +impl TryFrom<(Ipv4Addr, Ipv4Addr)> for IpRange { + type Error = String; + + fn try_from(pair: (Ipv4Addr, Ipv4Addr)) -> Result { + Ipv4Range::new(pair.0, pair.1).map(IpRange::V4) + } +} + +impl TryFrom<(Ipv6Addr, Ipv6Addr)> for IpRange { + type Error = String; + + fn try_from(pair: (Ipv6Addr, Ipv6Addr)) -> Result { + Ipv6Range::new(pair.0, pair.1).map(IpRange::V6) + } +} + +/// A non-decreasing IPv4 address range, inclusive of both ends. +/// +/// The first address must be less than or equal to the last address. +#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] +#[serde(try_from = "AnyIpv4Range")] +pub struct Ipv4Range { + first: Ipv4Addr, + last: Ipv4Addr, +} + +impl Ipv4Range { + pub fn new(first: Ipv4Addr, last: Ipv4Addr) -> Result { + if first <= last { + Ok(Self { first, last }) + } else { + Err(String::from("IP address ranges must be non-decreasing")) + } + } + + pub fn first_address(&self) -> Ipv4Addr { + self.first + } + + pub fn last_address(&self) -> Ipv4Addr { + self.last + } +} + +#[derive(Clone, Copy, Debug, Deserialize)] +struct AnyIpv4Range { + first: Ipv4Addr, + last: Ipv4Addr, +} + +impl TryFrom for Ipv4Range { + type Error = Error; + fn try_from(r: AnyIpv4Range) -> Result { + Ipv4Range::new(r.first, r.last) + .map_err(|msg| Error::invalid_request(msg.as_str())) + } +} + +/// A non-decreasing IPv6 address range, inclusive of both ends. +/// +/// The first address must be less than or equal to the last address. +#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] +#[serde(try_from = "AnyIpv6Range")] +pub struct Ipv6Range { + first: Ipv6Addr, + last: Ipv6Addr, +} + +impl Ipv6Range { + pub fn new(first: Ipv6Addr, last: Ipv6Addr) -> Result { + if first <= last { + Ok(Self { first, last }) + } else { + Err(String::from("IP address ranges must be non-decreasing")) + } + } + + pub fn first_address(&self) -> Ipv6Addr { + self.first + } + + pub fn last_address(&self) -> Ipv6Addr { + self.last + } +} + +#[derive(Clone, Copy, Debug, Deserialize)] +struct AnyIpv6Range { + first: Ipv6Addr, + last: Ipv6Addr, +} + +impl TryFrom for Ipv6Range { + type Error = Error; + fn try_from(r: AnyIpv6Range) -> Result { + Ipv6Range::new(r.first, r.last) + .map_err(|msg| Error::invalid_request(msg.as_str())) + } +} + #[cfg(test)] mod test { use super::IdentityType; @@ -139,10 +303,15 @@ mod test { use super::MAX_ROLE_ASSIGNMENTS_PER_RESOURCE; use crate::db; use crate::external_api::shared; + use crate::external_api::shared::IpRange; + use crate::external_api::shared::Ipv4Range; + use crate::external_api::shared::Ipv6Range; use anyhow::anyhow; use omicron_common::api::external::Error; use omicron_common::api::external::ResourceType; use serde::Deserialize; + use std::net::Ipv4Addr; + use std::net::Ipv6Addr; #[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq)] #[serde(rename_all = "kebab-case")] @@ -265,4 +434,62 @@ mod test { assert_eq!(success.identity_id, identity_id); assert_eq!(success.role_name, DummyRoles::Bogus); } + + #[test] + fn test_ip_range_checks_non_decreasing() { + let lo = Ipv4Addr::new(10, 0, 0, 1); + let hi = Ipv4Addr::new(10, 0, 0, 3); + assert!(Ipv4Range::new(lo, hi).is_ok()); + assert!(Ipv4Range::new(lo, lo).is_ok()); + assert!(Ipv4Range::new(hi, lo).is_err()); + + let lo = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1); + let hi = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3); + assert!(Ipv6Range::new(lo, hi).is_ok()); + assert!(Ipv6Range::new(lo, lo).is_ok()); + assert!(Ipv6Range::new(hi, lo).is_err()); + } + + #[test] + fn test_ip_range_enum_deserialization() { + let data = r#"{"V4": {"first": "10.0.0.1", "last": "10.0.0.3"}}"#; + let expected = IpRange::V4( + Ipv4Range::new( + Ipv4Addr::new(10, 0, 0, 1), + Ipv4Addr::new(10, 0, 0, 3), + ) + .unwrap(), + ); + assert_eq!(expected, serde_json::from_str(data).unwrap()); + + let data = r#"{"V6": {"first": "fd00::", "last": "fd00::3"}}"#; + let expected = IpRange::V6( + Ipv6Range::new( + Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), + Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3), + ) + .unwrap(), + ); + assert_eq!(expected, serde_json::from_str(data).unwrap()); + + let data = r#"{"V6": {"first": "fd00::3", "last": "fd00::"}}"#; + assert!( + serde_json::from_str::(data).is_err(), + "Expected an error deserializing an IP range with first address \ + greater than last address", + ); + } + + #[test] + fn test_ip_range_try_from() { + let lo = Ipv4Addr::new(10, 0, 0, 1); + let hi = Ipv4Addr::new(10, 0, 0, 3); + assert!(IpRange::try_from((lo, hi)).is_ok()); + assert!(IpRange::try_from((hi, lo)).is_err()); + + let lo = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1); + let hi = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 3); + assert!(IpRange::try_from((lo, hi)).is_ok()); + assert!(IpRange::try_from((hi, lo)).is_err()); + } } diff --git a/nexus/src/external_api/tag-config.json b/nexus/src/external_api/tag-config.json index 1e451e7d9b..e36542da9a 100644 --- a/nexus/src/external_api/tag-config.json +++ b/nexus/src/external_api/tag-config.json @@ -39,7 +39,7 @@ } }, "ip-pools": { - "description": "IP Pools contain subnets used for assigning external IP addresses to virtual machine instances.", + "description": "IP Pools contain external IP addresses that can be assigned to virtual machine Instances.", "external_docs": { "url": "http://oxide.computer/docs/#xxx" } diff --git a/nexus/src/external_api/views.rs b/nexus/src/external_api/views.rs index abd1ad9855..46cc535a38 100644 --- a/nexus/src/external_api/views.rs +++ b/nexus/src/external_api/views.rs @@ -7,10 +7,10 @@ use crate::authn; use crate::db::identity::{Asset, Resource}; use crate::db::model; +use crate::external_api::shared::IpRange; use api_identity::ObjectIdentity; use chrono::DateTime; use chrono::Utc; -use omicron_common::api::external; use omicron_common::api::external::{ ByteCount, Digest, IdentityMetadata, Ipv4Net, Ipv6Net, Name, ObjectIdentity, RoleName, @@ -334,7 +334,7 @@ impl From for IpPool { pub struct IpPoolRange { pub id: Uuid, pub time_created: DateTime, - pub range: external::IpRange, + pub range: IpRange, } impl From for IpPoolRange { @@ -342,7 +342,7 @@ impl From for IpPoolRange { Self { id: range.id, time_created: range.time_created, - range: external::IpRange::from(&range), + range: IpRange::from(&range), } } } diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 98925ca637..3045890c5a 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -16,9 +16,7 @@ use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::InstanceCpuCount; -use omicron_common::api::external::IpRange; use omicron_common::api::external::Ipv4Net; -use omicron_common::api::external::Ipv4Range; use omicron_common::api::external::Name; use omicron_common::api::external::RouteDestination; use omicron_common::api::external::RouteTarget; @@ -29,6 +27,8 @@ use omicron_nexus::authn; use omicron_nexus::authz; use omicron_nexus::external_api::params; use omicron_nexus::external_api::shared; +use omicron_nexus::external_api::shared::IpRange; +use omicron_nexus::external_api::shared::Ipv4Range; use std::net::IpAddr; use std::net::Ipv4Addr; diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index bbb39c85ce..884d5676e0 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -16,11 +16,11 @@ use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; -use omicron_common::api::external::IpRange; -use omicron_common::api::external::Ipv4Range; -use omicron_common::api::external::Ipv6Range; use omicron_nexus::external_api::params::IpPoolCreate; use omicron_nexus::external_api::params::IpPoolUpdate; +use omicron_nexus::external_api::shared::IpRange; +use omicron_nexus::external_api::shared::Ipv4Range; +use omicron_nexus::external_api::shared::Ipv6Range; use omicron_nexus::external_api::views::IpPool; use omicron_nexus::external_api::views::IpPoolRange; diff --git a/openapi/nexus.json b/openapi/nexus.json index 9c980d28e0..b6a6e18ea8 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -10285,7 +10285,7 @@ }, { "name": "ip-pools", - "description": "IP Pools contain subnets used for assigning external IP addresses to virtual machine instances.", + "description": "IP Pools contain external IP addresses that can be assigned to virtual machine Instances.", "externalDocs": { "url": "http://oxide.computer/docs/#xxx" } From 23c402f89bc5ce975ee31e3288dc6604fb3088c2 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Fri, 24 Jun 2022 00:07:56 +0000 Subject: [PATCH 6/6] More review feedback - Remove unnecessary pagination order from ip pool ranges - Remove more dead code - Untagged enum repr for IpRange type --- nexus/src/external_api/http_entrypoints.rs | 37 +++++----------------- nexus/src/external_api/shared.rs | 11 +++++-- openapi/nexus.json | 16 ---------- 3 files changed, 16 insertions(+), 48 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index c868371c83..47e9c53058 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1156,25 +1156,7 @@ async fn ip_pools_put_ip_pool( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/// Parameters for controlling pagination of IP Pool ranges -#[derive(Debug, Clone, Copy, Serialize, Deserialize, JsonSchema)] -pub struct IpPoolRangeScanParams { - #[serde(default = "default_ip_pool_range_order")] - direction: PaginationOrder, -} - -fn default_ip_pool_range_order() -> PaginationOrder { - PaginationOrder::Ascending -} - -#[derive(Debug, Clone, Copy, Serialize, Deserialize)] -struct IpPoolRangePageSelector { - scan_params: IpPoolRangeScanParams, - last_seen_address: IpNetwork, -} - -type IpPoolRangePaginationParams = - PaginationParams; +type IpPoolRangePaginationParams = PaginationParams; /// List the ranges of IP addresses within an existing IP Pool. /// @@ -1196,15 +1178,13 @@ async fn ip_pool_ranges_get( let pool_name = &path.pool_name; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let (scan_params, marker) = match query.page { - WhichPage::First(ref scan_params) => (scan_params, None), - WhichPage::Next(ref selector) => { - (&selector.scan_params, Some(&selector.last_seen_address)) - } + let marker = match query.page { + WhichPage::First(_) => None, + WhichPage::Next(ref addr) => Some(addr), }; let pag_params = DataPageParams { limit: rqctx.page_limit(&query)?, - direction: scan_params.direction, + direction: PaginationOrder::Ascending, marker, }; let ranges = nexus @@ -1215,10 +1195,9 @@ async fn ip_pool_ranges_get( .collect(); Ok(HttpResponseOk(ResultsPage::new( ranges, - scan_params, - |range: &IpPoolRange, scan_params| IpPoolRangePageSelector { - scan_params: *scan_params, - last_seen_address: IpNetwork::from(range.range.first_address()), + &EmptyScanParams {}, + |range: &IpPoolRange, _| { + IpNetwork::from(range.range.first_address()) }, )?)) }; diff --git a/nexus/src/external_api/shared.rs b/nexus/src/external_api/shared.rs index 833596bbc5..9f275837a5 100644 --- a/nexus/src/external_api/shared.rs +++ b/nexus/src/external_api/shared.rs @@ -141,11 +141,16 @@ impl TryFrom for IdentityType { /// The first address in the range is guaranteed to be no greater than the last /// address. #[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize)] +#[serde(untagged)] pub enum IpRange { V4(Ipv4Range), V6(Ipv6Range), } +// NOTE: We don't derive JsonSchema. That's intended so that we can use an +// untagged enum for `IpRange`, and use this method to annotate schemars output +// for client-generators (e.g., progenitor) to use in generating a better +// client. impl JsonSchema for IpRange { fn schema_name() -> String { "IpRange".to_string() @@ -452,7 +457,7 @@ mod test { #[test] fn test_ip_range_enum_deserialization() { - let data = r#"{"V4": {"first": "10.0.0.1", "last": "10.0.0.3"}}"#; + let data = r#"{"first": "10.0.0.1", "last": "10.0.0.3"}"#; let expected = IpRange::V4( Ipv4Range::new( Ipv4Addr::new(10, 0, 0, 1), @@ -462,7 +467,7 @@ mod test { ); assert_eq!(expected, serde_json::from_str(data).unwrap()); - let data = r#"{"V6": {"first": "fd00::", "last": "fd00::3"}}"#; + let data = r#"{"first": "fd00::", "last": "fd00::3"}"#; let expected = IpRange::V6( Ipv6Range::new( Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), @@ -472,7 +477,7 @@ mod test { ); assert_eq!(expected, serde_json::from_str(data).unwrap()); - let data = r#"{"V6": {"first": "fd00::3", "last": "fd00::"}}"#; + let data = r#"{"first": "fd00::3", "last": "fd00::"}"#; assert!( serde_json::from_str::(data).is_err(), "Expected an error deserializing an IP range with first address \ diff --git a/openapi/nexus.json b/openapi/nexus.json index b6a6e18ea8..8650c29531 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -602,14 +602,6 @@ }, "style": "simple" }, - { - "in": "query", - "name": "direction", - "schema": { - "$ref": "#/components/schemas/PaginationOrder" - }, - "style": "form" - }, { "in": "query", "name": "limit", @@ -10229,14 +10221,6 @@ "name_descending", "id_ascending" ] - }, - "PaginationOrder": { - "description": "The order in which the client wants to page through the requested collection", - "type": "string", - "enum": [ - "ascending", - "descending" - ] } } },