Skip to content

Conversation

@david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Aug 30, 2023

Followup to #3985. The diff is not very readable, I would just read the result:

#[nexus_test]
async fn test_instance_ephemeral_ip_from_correct_pool(
cptestctx: &ControlPlaneTestContext,
) {
let client = &cptestctx.external_client;
let _ = create_project(&client, PROJECT_NAME).await;
// Create two IP pools.
//
// The first is given to the "default" pool, the provided to a distinct
// explicit pool.
let default_pool_range = IpRange::V4(
Ipv4Range::new(
std::net::Ipv4Addr::new(10, 0, 0, 1),
std::net::Ipv4Addr::new(10, 0, 0, 5),
)
.unwrap(),
);
let other_pool_range = IpRange::V4(
Ipv4Range::new(
std::net::Ipv4Addr::new(10, 1, 0, 1),
std::net::Ipv4Addr::new(10, 1, 0, 5),
)
.unwrap(),
);
populate_ip_pool(&client, "default", Some(default_pool_range)).await;
create_ip_pool(&client, "other-pool", Some(other_pool_range)).await;
// Create an instance with pool name blank, expect IP from default pool
create_instance_with_pool(client, "default-pool-inst", None).await;
let ip = fetch_instance_ephemeral_ip(client, "default-pool-inst").await;
assert!(
ip.ip >= default_pool_range.first_address()
&& ip.ip <= default_pool_range.last_address(),
"Expected ephemeral IP to come from default pool"
);
// Create an instance explicitly using the "other-pool".
create_instance_with_pool(client, "other-pool-inst", Some("other-pool"))
.await;
let ip = fetch_instance_ephemeral_ip(client, "other-pool-inst").await;
assert!(
ip.ip >= other_pool_range.first_address()
&& ip.ip <= other_pool_range.last_address(),
"Expected ephemeral IP to come from other pool"
);
// now create a third pool, a silo default, to confirm it gets used. not
// using create_ip_pool because we need to specify a silo and default: true
let pool_name = "silo-pool";
let _silo_pool: views::IpPool = object_create(
client,
"/v1/system/ip-pools",
&params::IpPoolCreate {
identity: IdentityMetadataCreateParams {
name: pool_name.parse().unwrap(),
description: String::from("an ip pool"),
},
silo: Some(NameOrId::Id(DEFAULT_SILO.id())),
is_default: true,
},
)
.await;
let silo_pool_range = IpRange::V4(
Ipv4Range::new(
std::net::Ipv4Addr::new(10, 2, 0, 1),
std::net::Ipv4Addr::new(10, 2, 0, 5),
)
.unwrap(),
);
populate_ip_pool(client, pool_name, Some(silo_pool_range)).await;
create_instance_with_pool(client, "silo-pool-inst", Some("silo-pool"))
.await;
let ip = fetch_instance_ephemeral_ip(client, "silo-pool-inst").await;
assert!(
ip.ip >= silo_pool_range.first_address()
&& ip.ip <= silo_pool_range.last_address(),
"Expected ephemeral IP to come from the silo default pool"
);
// 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"
);
}

@david-crespo david-crespo enabled auto-merge (squash) August 30, 2023 23:44
@david-crespo david-crespo merged commit 1c1c20e into main Aug 31, 2023
@david-crespo david-crespo deleted the default-silo-pool-test branch August 31, 2023 01:29
david-crespo added a commit that referenced this pull request Jan 5, 2024
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 <git@just-be.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants