IP pools: change/remove default

[david-crespo]

In #3985, IP pools gained an explicit property is_default, which can be set at create time. However, it cannot be changed through the IP pool update endpoint.

omicron/nexus/types/src/external_api/params.rs

Lines 729 to 752 in 01e730a

	/// Create-time parameters for an `IpPool`
	#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
	pub struct IpPoolCreate {
	#[serde(flatten)]
	pub identity: IdentityMetadataCreateParams,
	/// If an IP pool is associated with a silo, instance IP allocations in that
	/// silo can draw from that pool.
	pub silo: Option<NameOrId>,
	/// Whether the IP pool is considered a default pool for its scope (fleet,
	/// or silo). If a pool is marked default and is associated with a silo,
	/// instances created in that silo will draw IPs from that pool unless
	/// another pool is specified at instance create time.
	#[serde(default)]
	pub is_default: bool,
	}
	/// Parameters for updating an IP Pool
	#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
	pub struct IpPoolUpdate {
	#[serde(flatten)]
	pub identity: IdentityMetadataUpdateParams,
	}

Because there can be at most one default for a given scope (fleet-wide or silo-specific),

omicron/schema/crdb/dbinit.sql

Lines 1497 to 1505 in 01e730a

	/*
	* Ensure there can only be one default pool for the fleet or a given silo.
	* Coalesce is needed because otherwise different nulls are considered to be
	* distinct from each other.
	*/
	CREATE UNIQUE INDEX IF NOT EXISTS one_default_pool_per_scope ON omicron.public.ip_pool (
	COALESCE(silo_id, '00000000-0000-0000-0000-000000000000'::uuid)
	) WHERE
	is_default = true AND time_deleted IS NULL;

Proposal

I don't think it makes sense to add is_default to the update params. Instead, I think we should have a make default endpoint, sort of analogous to image promotion and demotion. This endpoint would

• set is_default = false on the current default pool (if there is one)
• set is_default = true on the target pool

We probably also want a make-not-default endpoint, but it should probably only work on silo-scoped pools, otherwise instance create could fail due to there being no default pool.

[karencfv]

For things like Terraform it's preferable to have the update endpoint as similar to the create one as much as possible, because you can only do a single action per create/read/update/delete per resource.

Are there any strong reasons to have a separate endpoint that only sets a single IP Pool to default?

[david-crespo]

My thinking is that because there is only one default for the fleet or a silo, making one pool default makes another pool not default, and I would find it strange for a PUT /v1/system/ip-pools/my-pool to modify my-other-pool.

Another way to handle this, which honestly did not occur to me while I was hurrying to do #3895, is to have the association between pools and silos be a separate join table that's not considered a property of the pool or the silo at all. In other words, you can list your pools, and you can set a default pool for a silo, but those don't intrinsically have anything do with each other. I am definitely open to changing the model to something more flexible and less surprising.

However, I think from your point of view the result is the same — setting a default pool for silo would still be a separate endpoint.

[karencfv]

My thinking is that because there is only one default for the fleet or a silo, making one pool default makes another pool not default, and I would find it strange for a PUT /v1/system/ip-pools/my-pool to modify my-other-pool.

Hm yeah, that makes sense.

Another way to handle this, which honestly did not occur to me while I was hurrying to do #3895, is to have the association between pools and silos be a separate join table that's not considered a property of the pool or the silo at all. In other words, you can list your pools, and you can set a default pool for a silo, but those don't intrinsically have anything do with each other. I am definitely open to changing the model to something more flexible and less surprising.

I'm assuming this means the is_default field is removed altogether from the create endpoint? I think I like this option more. This means that I don't have to worry about syncing all IP pools to show which is the default one.

Another option might be to add a default_ip_pool to the silo update endpoint, this way with a silo read you'll know which IP Pool is the default one. This correlation makes more sense to me? What do you think?