Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need API endpoint for listing IP Pools scoped to a project #2148

Closed
bnaecker opened this issue Jan 10, 2023 · 6 comments · Fixed by #4261
Closed

Need API endpoint for listing IP Pools scoped to a project #2148

bnaecker opened this issue Jan 10, 2023 · 6 comments · Fixed by #4261
Assignees
Labels
known issue To include in customer documentation and training mvp networking Related to the networking.
Milestone

Comments

@bnaecker
Copy link
Collaborator

Need a way to list the IP pools available to resources within a project. That means any IP Pool not reserved to another project, or reserved for a specific project.

@bnaecker bnaecker added networking Related to the networking. mvp labels Jan 10, 2023
@smklein
Copy link
Collaborator

smklein commented Jan 10, 2023

Related: #2055

I thought there were not "IP Pools scoped to a project", but rather, "IP Pools which are available to a project". I think that's a subtle distinction, but also a matter of ownership.

@bnaecker
Copy link
Collaborator Author

Yeah, I was rushing here. I meant that the endpoint should be project-scoped, and that should return the IP Pools available to a project.

So ../projects/{project_name}/ip-pools returns all IP Pools that can be used by Project project_name.

@bnaecker
Copy link
Collaborator Author

I'm catching back up to a bunch of work I was only vaguely aware of in the last few months. This issue is possibly still relevant, but much simpler after #2056. There is no project ID at all on IP Pools anymore, so this just needs to return all non-internal pools. That logic may change in the future as we flesh out the scoping of pools, but for now, it's just anything that any developer can use. I.e., all projects will return the same list now.

@smklein smklein added this to the MVP milestone Jan 20, 2023
@lifning
Copy link
Contributor

lifning commented May 11, 2023

just to clarify -- for the time being, this would literally be a duplicate of /v1/system/ip-pools's current behavior (which already does .filter(dsl::internal.eq(false))), with the purpose being that /v1/projects/{}/ip-pools would be subject to potentially change in the future?

@rmustacc
Copy link

Yes, I believe that this is correct. Let me try to provide some surrounding context here.

One of the main things that we know that we're going to need, both from prior experience and from customer feedback, is that they want a way to have a group of IPs that can be used for provisioning that is eventually restricted to a subset of users, whether that ACL is eventually associated with a project, a silo, or some other more complex thing (but critically not based on user id). So the idea is that really if you're using the CLI or any other thing, the question of what images, IP pools, or other resources can I use is ultimately specific to the project and therefore by using a project-scoped URL you'll get what you can use.

So today the only ACL that really exists is the internal-only bit. I think we want to look at what it means to update all the user and terraform tooling to use this endpoint. The fact that the system IP pools interface filters out internal things raises a question for me because it makes me ask how does an operator change that.

Hopefully this helps a bit. Thanks for picking this one up @lifning.

@zephraph
Copy link
Contributor

Sounds good.

I think the shape of the API is likely going to need to be different though.

/v1/projects/{project}/ip-pools only fits with our API if all you can do is list a resource. If you need to be able to get a specific resource from the collection or access sub resources (like ip pool ranges) then you'll want to use /v1/ip-pools with project being a query selector. I'm happy to chat with you about what that should look like.

@lifning lifning self-assigned this Jun 6, 2023
lifning added a commit that referenced this issue Jun 7, 2023
… (#3132)

Per discussion in #2148, this is a future-looking API that's functionally similar to the `/v1/system/`-prefixed IP-pool-viewing routes we already have, but providing read-only access to less-privileged users, with room to grow into project-level scoping when it's implemented.
@morlandi7 morlandi7 added the known issue To include in customer documentation and training label Dec 18, 2023
david-crespo added a commit that referenced this issue 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
known issue To include in customer documentation and training mvp networking Related to the networking.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants