Link all default IP Pools to VPC's internet gateway on creation#10144
Link all default IP Pools to VPC's internet gateway on creation#10144
Conversation
bnaecker
commented
Mar 24, 2026
bnaecker
commented
- Remove the footgunny method for fetching "the" default IP Pool for a silo, which is hard to use correctly without specifying an IP version. Also fix call sites.
- Add method for returning all the default IP Pools for a Silo, of which there are at most 2.
- In the VPC creation saga, ensure that we create an internet gateway pool for all the IP Pools currently linked to the silo as a default.
- Add a data migration that renames any existing "default" gateway IP Pools as "default-v4" or "default-v6", as we're now specifying the version.
- Add regression test for Newly created VPC does not have IP pool attached to internet gateway #10117.
- This fixes Newly created VPC does not have IP pool attached to internet gateway #10117.
- Remove the footgunny method for fetching "the" default IP Pool for a silo, which is hard to use correctly without specifying an IP version. Also fix call sites. - Add method for returning all the default IP Pools for a Silo, of which there are at most 2. - In the VPC creation saga, ensure that we create an internet gateway pool for all the IP Pools currently linked to the silo as a default. - Add a data migration that renames any existing "default" gateway IP Pools as "default-v4" or "default-v6", as we're now specifying the version. - Add regression test for #10117. - This fixes #10117.
bnaecker
force-pushed
the
ben/link-all-default-pools-to-default-internet-gateway
branch
from
34a886b to
26086b6
Compare
FelixMcFelix
left a comment
FelixMcFelix
left a comment
There was a problem hiding this comment.
One optional nit but I think this looks good. Thanks for putting this together quickly Ben.
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
LGTM, just a questions.
|
|
||
| UPDATE omicron.public.internet_gateway_ip_pool AS igw_pool | ||
| SET | ||
| name = 'default-' || ip_pool.ip_version::STRING, |
There was a problem hiding this comment.
Could someone have manually created pools named default-v4 or default-v6?
There was a problem hiding this comment.
Yeah, I think that could happen. I waffled on this honestly, because there is no unique index on the name, nor do we ever really look for the name at all AFAICT. I only thought it would be confusing to have two pools named the same thing, that only differed in the version.
There was a problem hiding this comment.
Do you think it's better to avoid the migration entirely, given this?
There was a problem hiding this comment.
I'm not sure! Do we have deployed systems that already have default pools? If so, are those names both named default unless someone has explicitly changed them?
My gut feeling is default-v4 + default-v6 is clearer than default + default. I'm just a little sketchy on unconditionally renaming a thing that an operator might have already named (or renamed?) themselves, or renaming things in a way that might collide with other pools that already exist. Sorry, I'm not sure I'm being helpful at all - might be worth asking somewhat more broadly, if this is worth getting consensus on.
There was a problem hiding this comment.
No, I get it. I think there are a few parts:
- There's no unique index on the name, so while having duplicates is probably confusing, it won't cause hard errors
- It's possible that someone created a gateway IP pool with the name
"default-v{4,6}", but not renamed this default to that. We don't have an update endpoint for this type.
I'm happy to get more feedback, but my sense is this is an improvement and unlikely to break things.
- simpler ordering - Add error message if no default pools on VPC creation - fix mismerge with main
…pools-to-default-internet-gateway
bnaecker
deleted the
ben/link-all-default-pools-to-default-internet-gateway
branch
3 participants
