Skip to content

Commit

Permalink
[nexus] Remove project_id, rack_id from IP pools (#2056)
Browse files Browse the repository at this point in the history
## Before this PR

- IP Pools could exist in at most one project. IP allocation during
instance creation occurred by [either by requesting an IP pool belonging
to a project, or by "just looking for any unreserved IP
Pool"](https://github.com/oxidecomputer/omicron/blob/79765a4e3b39a29bc9940c0e4a49c4364fbcc9e3/nexus/src/db/queries/external_ip.rs#L186-L212).
As discussed in #2055 ,
our intention is for IP pools to be used across multiple projects, and
for projects to be able to use multiple IP pools.
- "Service" IP pools were indexed by rack ID, though (as documented in
#1276 ), they should
probably be accessed by AZ instead.

## This PR

- Adds a default IP pool named `default`, which is used for address
allocation unless a more specific IP pool is provided
- Removes "project ID" from IP pools (and external IP addresses)
- Removes "rack ID" from IP pool API and DB representation

## In the future

- This PR doesn't provide the many-to-many connection between projects
and IP pools that we eventually want, where projects can be configured
to use different IP pools for different purposes. However, by removing
the not-quite-accurate relationship that an IP pool must belong to a
*single* project, the API moves closer towards this direction.
- We probably should access the `service_ip_pool` API with the AZ UUID
used for the query, but since AZs don't exist in the API yet, this has
been omitted.

Part of #2055
  • Loading branch information
smklein authored Dec 19, 2022
1 parent 3593342 commit f47aefc
Show file tree
Hide file tree
Showing 32 changed files with 330 additions and 913 deletions.
51 changes: 3 additions & 48 deletions common/src/sql/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1112,16 +1112,9 @@ CREATE TABLE omicron.public.ip_pool (
time_modified TIMESTAMPTZ NOT NULL,
time_deleted TIMESTAMPTZ,

/* Optional ID of the project for which this pool is reserved. */
project_id UUID,

/*
* Optional rack ID, indicating this is a reserved pool for internal
* services on a specific rack.
* TODO(https://github.com/oxidecomputer/omicron/issues/1276): This
* should probably point to an AZ or fleet, not a rack.
*/
rack_id UUID,
/* Identifies if the IP Pool is dedicated to Control Plane services */
internal BOOL NOT NULL,

/* The collection's child-resource generation number */
rcgen INT8 NOT NULL
Expand All @@ -1135,15 +1128,6 @@ CREATE UNIQUE INDEX ON omicron.public.ip_pool (
) WHERE
time_deleted IS NULL;

/*
* Index ensuring uniqueness of IP pools by rack ID
*/
CREATE UNIQUE INDEX ON omicron.public.ip_pool (
rack_id
) WHERE
rack_id IS NOT NULL AND
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
Expand All @@ -1158,20 +1142,11 @@ CREATE TABLE omicron.public.ip_pool_range (
/* The range is inclusive of the last address. */
last_address INET NOT NULL,
ip_pool_id UUID NOT NULL,
/* Optional ID of the project for which this range is reserved.
*
* NOTE: This denormalizes the tables a bit, since the project_id is
* duplicated here and in the parent `ip_pool` table. We're allowing this
* for now, since it reduces the complexity of the already-bad IP allocation
* query, but we may want to revisit that, and JOIN with the parent table
* instead.
*/
project_id UUID,
/* Tracks child resources, IP addresses allocated out of this range. */
rcgen INT8 NOT NULL
);

/*
/*
* 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.
Expand All @@ -1187,14 +1162,6 @@ CREATE UNIQUE INDEX ON omicron.public.ip_pool_range (
STORING (first_address)
WHERE time_deleted IS NULL;

/*
* Index supporting allocation of IPs out of a Pool reserved for a project.
*/
CREATE INDEX ON omicron.public.ip_pool_range (
project_id
) WHERE
time_deleted IS NULL;


/* The kind of external IP address. */
CREATE TYPE omicron.public.ip_kind AS ENUM (
Expand Down Expand Up @@ -1245,9 +1212,6 @@ CREATE TABLE omicron.public.external_ip (
/* FK to the `ip_pool_range` table. */
ip_pool_range_id UUID NOT NULL,

/* FK to the `project` table. */
project_id UUID,

/* FK to the `instance` table. See the constraints below. */
instance_id UUID,

Expand All @@ -1263,15 +1227,6 @@ CREATE TABLE omicron.public.external_ip (
/* The last port in the allowed range, also inclusive. */
last_port INT4 NOT NULL,

/*
* The project can only be NULL for service IPs.
* Additionally, the project MUST be NULL for service IPs.
*/
CONSTRAINT null_project CHECK(
(kind != 'service' AND project_id IS NOT NULL) OR
(kind = 'service' AND project_id IS NULL)
),

/* The name must be non-NULL iff this is a floating IP. */
CONSTRAINT null_fip_name CHECK (
(kind != 'floating' AND name IS NULL) OR
Expand Down
16 changes: 2 additions & 14 deletions end-to-end-tests/src/bin/bootstrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use end_to_end_tests::helpers::ctx::{build_client, Context};
use end_to_end_tests::helpers::{generate_name, get_system_ip_pool};
use omicron_test_utils::dev::poll::{wait_for_condition, CondCheckError};
use oxide_client::types::{
ByteCount, DiskCreate, DiskSource, IpPoolCreate, IpRange, Ipv4Range,
ByteCount, DiskCreate, DiskSource, IpRange, Ipv4Range,
};
use oxide_client::{ClientDisksExt, ClientOrganizationsExt, ClientSystemExt};
use std::time::Duration;
Expand All @@ -30,21 +30,9 @@ async fn main() -> Result<()> {
// ===== CREATE IP POOL ===== //
eprintln!("creating IP pool...");
let (first, last) = get_system_ip_pool()?;
let pool_name = client
.ip_pool_create()
.body(IpPoolCreate {
name: generate_name("ip-pool")?,
description: String::new(),
organization: None,
project: None,
})
.send()
.await?
.name
.clone();
client
.ip_pool_range_add()
.pool_name(pool_name)
.pool_name("default")
.body(IpRange::V4(Ipv4Range { first, last }))
.send()
.await?;
Expand Down
40 changes: 10 additions & 30 deletions nexus/db-model/src/external_ip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ pub struct ExternalIp {
pub time_deleted: Option<DateTime<Utc>>,
pub ip_pool_id: Uuid,
pub ip_pool_range_id: Uuid,
pub project_id: Option<Uuid>,
// This is Some(_) for:
// - all instance SNAT IPs
// - all ephemeral IPs
Expand All @@ -80,18 +79,6 @@ impl From<ExternalIp> for sled_agent_client::types::SourceNatConfig {
}
}

/// Describes where the IP candidates for allocation come from: either
/// from an IP pool, or from a project.
///
/// This ensures that a source is always specified, and a caller cannot
/// request an external IP allocation without providing at least one of
/// these options.
#[derive(Debug, Clone, Copy)]
pub enum IpSource {
Instance { project_id: Uuid, pool_id: Option<Uuid> },
Service { pool_id: Uuid },
}

/// An incomplete external IP, used to store state required for issuing the
/// database query that selects an available IP and stores the resulting record.
#[derive(Debug, Clone)]
Expand All @@ -102,15 +89,14 @@ pub struct IncompleteExternalIp {
time_created: DateTime<Utc>,
kind: IpKind,
instance_id: Option<Uuid>,
source: IpSource,
pool_id: Uuid,
}

impl IncompleteExternalIp {
pub fn for_instance_source_nat(
id: Uuid,
project_id: Uuid,
instance_id: Uuid,
pool_id: Option<Uuid>,
pool_id: Uuid,
) -> Self {
Self {
id,
Expand All @@ -119,33 +105,27 @@ impl IncompleteExternalIp {
time_created: Utc::now(),
kind: IpKind::SNat,
instance_id: Some(instance_id),
source: IpSource::Instance { project_id, pool_id },
pool_id,
}
}

pub fn for_ephemeral(
id: Uuid,
project_id: Uuid,
instance_id: Uuid,
pool_id: Option<Uuid>,
) -> Self {
pub fn for_ephemeral(id: Uuid, instance_id: Uuid, pool_id: Uuid) -> Self {
Self {
id,
name: None,
description: None,
time_created: Utc::now(),
kind: IpKind::Ephemeral,
instance_id: Some(instance_id),
source: IpSource::Instance { project_id, pool_id },
pool_id,
}
}

pub fn for_floating(
id: Uuid,
name: &Name,
description: &str,
project_id: Uuid,
pool_id: Option<Uuid>,
pool_id: Uuid,
) -> Self {
Self {
id,
Expand All @@ -154,7 +134,7 @@ impl IncompleteExternalIp {
time_created: Utc::now(),
kind: IpKind::Floating,
instance_id: None,
source: IpSource::Instance { project_id, pool_id },
pool_id,
}
}

Expand All @@ -166,7 +146,7 @@ impl IncompleteExternalIp {
time_created: Utc::now(),
kind: IpKind::Service,
instance_id: None,
source: IpSource::Service { pool_id },
pool_id,
}
}

Expand Down Expand Up @@ -194,8 +174,8 @@ impl IncompleteExternalIp {
&self.instance_id
}

pub fn source(&self) -> &IpSource {
&self.source
pub fn pool_id(&self) -> &Uuid {
&self.pool_id
}
}

Expand Down
30 changes: 9 additions & 21 deletions nexus/db-model/src/ip_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,11 @@ pub struct IpPool {
#[diesel(embed)]
pub identity: IpPoolIdentity,

/// An optional ID of the project for which this pool is reserved.
pub project_id: Option<Uuid>,

/// An optional ID of the rack for which this pool is reserved.
// TODO(https://github.com/oxidecomputer/omicron/issues/1276): This
// should probably point to an AZ or fleet, not a rack.
pub rack_id: Option<Uuid>,
/// If true, identifies that this IP pool is dedicated to "Control-Plane
/// Services", such as Nexus.
///
/// Otherwise, this IP pool is intended for usage by customer VMs.
pub internal: bool,

/// Child resource generation number, for optimistic concurrency control of
/// the contained ranges.
Expand All @@ -44,24 +42,22 @@ pub struct IpPool {
impl IpPool {
pub fn new(
pool_identity: &external::IdentityMetadataCreateParams,
project_id: Option<Uuid>,
rack_id: Option<Uuid>,
internal: bool,
) -> Self {
Self {
identity: IpPoolIdentity::new(
Uuid::new_v4(),
pool_identity.clone(),
),
project_id,
rack_id,
internal,
rcgen: 0,
}
}
}

impl From<IpPool> for views::IpPool {
fn from(pool: IpPool) -> Self {
Self { identity: pool.identity(), project_id: pool.project_id }
Self { identity: pool.identity() }
}
}

Expand Down Expand Up @@ -98,20 +94,13 @@ pub struct IpPoolRange {
pub last_address: IpNetwork,
/// Foreign-key to the `ip_pool` table with the parent pool for this range
pub ip_pool_id: Uuid,
/// Foreign-key to the `project` table, with the Project to which this range
/// is restricted, if any (derived from the `ip_pool` table).
pub project_id: Option<Uuid>,
/// The child resource generation number, tracking IP addresses allocated or
/// used from this range.
pub rcgen: i64,
}

impl IpPoolRange {
pub fn new(
range: &IpRange,
ip_pool_id: Uuid,
project_id: Option<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();
Expand All @@ -129,7 +118,6 @@ impl IpPoolRange {
first_address: IpNetwork::from(range.first_address()),
last_address: IpNetwork::from(range.last_address()),
ip_pool_id,
project_id,
rcgen: 0,
}
}
Expand Down
28 changes: 2 additions & 26 deletions nexus/db-model/src/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,9 @@
// 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/.

use super::{
Disk, ExternalIp, Generation, Image, Instance, IpPool, Name, Snapshot, Vpc,
};
use super::{Disk, Generation, Image, Instance, Name, Snapshot, Vpc};
use crate::collection::DatastoreCollectionConfig;
use crate::schema::{
disk, external_ip, image, instance, ip_pool, project, snapshot, vpc,
};
use crate::schema::{disk, image, instance, project, snapshot, vpc};
use chrono::{DateTime, Utc};
use db_macros::Resource;
use nexus_types::external_api::params;
Expand Down Expand Up @@ -83,26 +79,6 @@ impl DatastoreCollectionConfig<Vpc> for Project {
type CollectionIdColumn = vpc::dsl::project_id;
}

// NOTE: "IpPoolRange" also contains a reference to "project_id", but
// ranges should only exist within IP Pools.
impl DatastoreCollectionConfig<IpPool> for Project {
type CollectionId = Uuid;
type GenerationNumberColumn = project::dsl::rcgen;
type CollectionTimeDeletedColumn = project::dsl::time_deleted;
type CollectionIdColumn = ip_pool::dsl::project_id;
}

// TODO(https://github.com/oxidecomputer/omicron/issues/1482): Not yet utilized,
// but needed for project deletion safety.
// TODO(https://github.com/oxidecomputer/omicron/issues/1334): Cannot be
// utilized until floating IPs are implemented.
impl DatastoreCollectionConfig<ExternalIp> for Project {
type CollectionId = Uuid;
type GenerationNumberColumn = project::dsl::rcgen;
type CollectionTimeDeletedColumn = project::dsl::time_deleted;
type CollectionIdColumn = external_ip::dsl::project_id;
}

/// Describes a set of updates for the [`Project`] model.
#[derive(AsChangeset)]
#[diesel(table_name = project)]
Expand Down
5 changes: 1 addition & 4 deletions nexus/db-model/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,7 @@ table! {
time_created -> Timestamptz,
time_modified -> Timestamptz,
time_deleted -> Nullable<Timestamptz>,
project_id -> Nullable<Uuid>,
rack_id -> Nullable<Uuid>,
internal -> Bool,
rcgen -> Int8,
}
}
Expand All @@ -164,7 +163,6 @@ table! {
first_address -> Inet,
last_address -> Inet,
ip_pool_id -> Uuid,
project_id -> Nullable<Uuid>,
rcgen -> Int8,
}
}
Expand All @@ -179,7 +177,6 @@ table! {
time_deleted -> Nullable<Timestamptz>,
ip_pool_id -> Uuid,
ip_pool_range_id -> Uuid,
project_id -> Nullable<Uuid>,
instance_id -> Nullable<Uuid>,
kind -> crate::IpKindEnum,
ip -> Inet,
Expand Down
Loading

0 comments on commit f47aefc

Please sign in to comment.