Small IP pools fixes #4007

david-crespo · 2023-09-01T17:40:13Z

Followup to #3985, closes #4005.

Add is_default to IP pool response
Inline ip_pools_fetch into the one callsite and delete it (per discussion [nexus] Silo IP pools #3985 (comment))
Other small test tweaks suggested by @luqmana

david-crespo · 2023-09-01T17:40:45Z

nexus/db-queries/src/db/datastore/ip_pool.rs

-
-        Ok(pool)
-    }
-


This did not make sense as a function here. All the logic was related to the calling context.

david-crespo · 2023-09-01T17:43:13Z

nexus/db-queries/src/db/datastore/ip_pool.rs

-            .await
-            .expect("Failed to get fleet default IP pool");
-        assert_eq!(ip_pool.id(), fleet_default_pool.id());
-


can't do this anymore because the method is gone, but this integration test covers it.

omicron/nexus/tests/integration_tests/instances.rs

Lines 3442 to 3451 in 01e730a

// we can still specify other pool even though we now have a silo default

create_instance_with_pool(client, "other-pool-inst-2", Some("other-pool"))

.await;

let ip = fetch_instance_ephemeral_ip(client, "other-pool-inst-2").await;

assert!(

ip.ip >= other_pool_range.first_address()

&& ip.ip <= other_pool_range.last_address(),

"Expected ephemeral IP to come from the other pool"

);

@zephraph

Closes #2148 Closes #4002 Closes #4003 Closes #4006 ## Background #3985 (and followups #3998 and #4007) made it possible to associate an IP pool with a silo so that instances created in that silo would get their ephemeral IPs from said pool by default (i.e., without the user having to say anything other than "I want an ephemeral IP"). An IP pool associated with a silo was not accessible for ephemeral IP allocation from other silos — if a disallowed pool was specified by name at instance create time, the request would 404. However! That was the quick version, and the data model left much to be desired. The relation was modeled by adding a nullable `silo_id` and sort-of-not-really-nullable `is_default` column directly on the IP pool table, which has the following limitations (and there are probably more): * A given IP pool could only be associated with at most one silo, could not be shared * The concept of `default` was treated as a property of the pool itself, rather than a property of the _association_ with another resource, which is quite strange. Even if you could associate the pool with multiple silos, you could not have it be the default for one and not for the other * There is no way to create an IP pool without associating it with either the fleet or a silo * Extending this model to allow association at the project level would be inelegant — we'd have to add a `project_id` column (which I did in #3981 before removing it in #3985) More broadly (and vaguely), the idea of an IP pool "knowing" about silos or projects doesn't really make sense. Entities aren't really supposed to know about each other unless they have a parent-child relationship. ## Changes in this PR ### No such thing as fleet-scoped pool, only silo Thanks to @zephraph for encouraging me to make this change. It is dramatically easier to explain "link silo to IP pool" than it is to explain "link resource (fleet or silo) to IP pool". The way to recreate the behavior of a single default pool for the fleet is to simply associate a pool with all silos. Data migrations ensure that existing fleet-scoped pools will be associated with all silos. There can only be one default pool for a silo, so in the rare case where pool A is a fleet default and pool B is default on silo S, we associate both A and B with S, but only B is made silo default pool. ### API These endpoints are added. They're pretty self-explanatory. ``` ip_pool_silo_link POST /v1/system/ip-pools/{pool}/silos ip_pool_silo_list GET /v1/system/ip-pools/{pool}/silos ip_pool_silo_unlink DELETE /v1/system/ip-pools/{pool}/silos/{silo} ip_pool_silo_update PUT /v1/system/ip-pools/{pool}/silos/{silo} ``` The `silo_id` and `is_default` fields are removed from the `IpPool` response as they are now a property of the `IpPoolLink`, not the pool itself. I also fixed the silo-scoped IP pools list (`/v1/ip-pools`) and fetch (`/v1/ip-pools/{pool}`) endpoints, which a) did not actually filter for the current silo, allowing any user to fetch any pool, and b) took a spurious `project` query param that didn't do anything. ### DB The association between IP pools and fleet or silo (or eventually projects, but not here) is now modeled through a polymorphic join table called `ip_pool_resource`: ip_pool_id | resource_type | resource_id | is_default -- | -- | -- | -- 123 | silo | 23 | true 123 | silo | 4 | false ~~65~~ | ~~fleet~~ | ~~FLEET_ID~~ | ~~true~~ Now, instead of setting the association with a silo or fleet at IP pool create or update time, there are separate endpoints for adding and removing an association. A pool can be associated with any number of resources, but a unique index ensures that a given resource can only have one default pool. ### Default IP pool logic If an instance ephemeral IP or a floating IP is created **with a pool specified**, we simply use that pool if it exists and is linked to the user's silo. If an instance ephemeral IP or a floating IP is created **without a pool unspecified**, we look for a default pool for the current silo. If there is a pool linked with the current silo with `is_default=true`, use that. Otherwise, there is no default pool for the given scope and IP allocation will fail, which means the instance create or floating IP create request will fail. The difference introduced in this PR is that we do not fall back to fleet default if there is no silo default because we have removed the concept of a fleet-scoped pool. ### Tests and test helpers This is the source of a lot of noise in this PR. Because there can no longer be a fleet default pool, we can no longer rely on that for tests. The test setup was really confusing. We assumed a default IP pool existed, but we still had to populate it (add a range) if we had to do anything with it. Now, we don't assume it exists, we create it and add a range and associate it with a silo all in one helper. ## What do customers have to do when they upgrade? They should not _have_ to do anything at upgrade time. If they were relying on a single fleet default pool to automatically be used by new silos, when they create silos in the future they will have to manually associate each new silo with the desired pool. We are working on ways to make that easier or more automatic, but that's not in this change. It is less urgent because silo creation is an infrequent operation. If they are _not_ using the previously fleet default IP pool named `default` and do not want it to exist, they can simply delete any IP ranges it contains, unlink it from all silos and delete it. If they are not using it, there should not be any IPs allocated from it (which means they can delete it). --------- Co-authored-by: Justin Bennett <[email protected]>

david-crespo added 3 commits August 31, 2023 16:13

small fixes to #3985

d982d81

inline ip_pools_fetch in the one call site

676dca8

add is_default to views::IpPool. closes #4005

cc6c721

david-crespo commented Sep 1, 2023

View reviewed changes

david-crespo enabled auto-merge (squash) September 1, 2023 18:57

david-crespo merged commit db702a2 into main Sep 1, 2023

david-crespo deleted the small-ip-pools-fixes branch September 1, 2023 19:17

david-crespo mentioned this pull request Oct 11, 2023

IP pools data model and API rework #4261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small IP pools fixes #4007

Small IP pools fixes #4007

david-crespo commented Sep 1, 2023

david-crespo Sep 1, 2023

david-crespo Sep 1, 2023

	// we can still specify other pool even though we now have a silo default
	create_instance_with_pool(client, "other-pool-inst-2", Some("other-pool"))
	.await;

	let ip = fetch_instance_ephemeral_ip(client, "other-pool-inst-2").await;
	assert!(
	ip.ip >= other_pool_range.first_address()
	&& ip.ip <= other_pool_range.last_address(),
	"Expected ephemeral IP to come from the other pool"
	);

Small IP pools fixes #4007

Small IP pools fixes #4007

Conversation

david-crespo commented Sep 1, 2023

david-crespo Sep 1, 2023

Choose a reason for hiding this comment

david-crespo Sep 1, 2023

Choose a reason for hiding this comment