[floating-ip,fix,api] Fix floating IP creation API to distinguish explicit IP from pool selection #9687

zeeshanlakhani · 2026-01-20T19:49:08Z

This work clears up ambiguity.

AddressAllocator::Explicit now requires only an IP field, with the pool inferred from the address (since IP pools cannot have overlapping ranges). Pool-based allocation moves to AddressAllocator::Auto with PoolSelector::Explicit { pool }.

Includes:

Simplifies AddressAllocator::Explicit to only require ip as a field (pool inferred)
Moves explicit pool selection to Auto { pool_selector: PoolSelector::Explicit { pool } }
Adds API version 2026012200 (FLOATING_IP_ALLOCATOR_UPDATE)
Adds ip_pool_fetch_containing_address() to resolve pool from explicit IP address correctly
- Consolidates multicast pool resolution to use ip_pool_fetch_containing_address() too
Adds versioned types in v2026011600.rs and updates v2026011501.rs delegation chain
Adds unit tests for wire format compatibility and version conversions
Adds FloatingIpAllocation enum at datastore layer (type-safe, mirrors multicast pattern)
Adds Display impl for consistent pool selection logging (inferred/explicit/default/auto)
Converts NotFound to InvalidRequest for an IP not in any configured pool
Adds an index on ip_pool_range.ip_pool_id (schema version 223) for optimizing ip_pool_fetch_containing_address() and other queries

Current params JSON:

Explicit IP (pool inferred from address):

{"type": "explicit", "ip": "10.0.0.1"}

Auto with explicit pool:

{"type": "auto", "pool_selector": {"type": "explicit", "pool": "my-pool"}}

Auto with default pool (specific IP version)

{"type": "auto", "pool_selector": {"type": "auto", "ip_version": "v4"}}

Auto with default pool (silo default, works only if there's a single
default pool)

{"type": "auto", "pool_selector": {"type": "auto"}}

Auto with all defaults (works only if there's a single default pool)

{"type": "auto"}

Example FloatingIpCreate request body:

{
  "name": "my-floating-ip",
  "description": "Example floating IP",
  "address_allocator": {"type": "explicit", "ip": "10.0.0.1"}
}

Or with pool selection:

{
  "name": "my-floating-ip",
  "description": "Example floating IP",
  "address_allocator": {
    "type": "auto",
    "pool_selector": {"type": "explicit", "pool": "my-pool"}
  }
}

Note: There's some multicast cleanup here to make sure of the same functionality, which just removes unnecessary code.

… explicit allocation Fixes #9680. The `AddressAllocator::Explicit` variant now accepts just a `pool` field without requiring `ip`, allowing users to auto-allocate from a specific pool. Additionally, `AddressAllocator::Auto` now rejects unknown fields like `pool` instead of silently ignoring them. Includes: - Add `ExplicitAllocation` newtype with validation (at least one of `ip` or `pool` required) - Add `deny_unknown_fields` to `AddressAllocator` enum - Add API version 2026012000 (`ADDRESS_ALLOCATOR_OPTIONAL_IP`) - Add versioned types in v2026011600.rs and update v2026011501.rs delegation chain - Add unit tests for both bug cases and wire format compatibility

nexus/types/src/external_api/params.rs

mergeconflict · 2026-01-21T00:24:50Z

The AddressAllocator::Explicit variant now accepts just a pool field without requiring ip, allowing users to auto-allocate from a specific pool. Additionally, AddressAllocator::Auto now rejects unknown fields like pool instead of silently ignoring them.

My mental model so far (which might be totally wrong) is that Explicit is referring to an explicit IP address. I guess IP pools can't have any overlapping ranges with each other, so specifying an address should imply at most one pool?

zeeshanlakhani · 2026-01-21T01:31:05Z

The AddressAllocator::Explicit variant now accepts just a pool field without requiring ip, allowing users to auto-allocate from a specific pool. Additionally, AddressAllocator::Auto now rejects unknown fields like pool instead of silently ignoring them.

My mental model so far (which might be totally wrong) is that Explicit is referring to an explicit IP address. I guess IP pools can't have any overlapping ranges with each other, so specifying an address should imply at most one pool?

Yeah, I think moving to #9687 (comment) simplifies this from what we had before anyhow.

…icit) The previous API had two issues: - Auto variant silently ignored the pool field - Explicit IP allocation failed for addresses not in the default pool Updates: - Moves explicit pool selection firmly to `Auto { pool_selector: PoolSelector::Explicit }` - Explicit variant in floating IP creation now only requires ip (pool inferred from address) - Adds deny_unknown_fields to reject invalid requests like `{\"type\": \"auto\", \"pool\": \"...\"}` - Adds `ip_pool_fetch_containing_address()` to resolve pool from explicit IP address correctly - Consolidates multicast pool resolution to use `ip_pool_fetch_containing_address()` too - Adds FloatingIpAllocation enum at datastore layer (type-safe, mirrors multicast pattern) - Adds Display impl for consistent pool selection logging (inferred/explicit/default) - Converts NotFound to InvalidRequest for IP not in any configured pool - Adds test_floating_ip_create_ip_not_in_pool integration test

zeeshanlakhani · 2026-01-21T06:20:04Z

@bnaecker @mergeconflict updated everything to be more "explicit" for explicit. The multicast changes are mainly here for cleanup and reuse.

mergeconflict · 2026-01-21T12:37:28Z

nexus/db-queries/src/db/datastore/multicast/groups.rs

+    Auto {
+        /// IP version for default pool selection.
+        /// Required if both IPv4 and IPv6 default multicast pools exist.
+        ip_version: Option<IpVersion>,
+    },


I'm not familiar with the db-queries code yet. Does this need something analogous to a PoolSelector like in external_api::params::AddressAllocator?

It didn't have it previously. And, there is a difference in type (post authz). This just makes it explicit within the datastore code as well, vs tuple matching.

In terms of multicast, joins happen via members, and you can't specify a pool. It's a bit different in terms of API, and purposely so, i.e. you join a group by ip, uuid, name of the group, which is more common for mcast APIs.

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

mergeconflict · 2026-01-21T12:50:50Z

nexus/types/src/external_api/params.rs

-        /// The pool containing this address. If not specified, the default
-        /// pool for the address's IP version is used.
-        pool: Option<NameOrId>,


Shoot... Does this mean I should remove pool from ExternalSubnetAllocator::Explicit too?

Yeah, I think being more direct is the way to go semantically, right? Being explicit with explicit (haha) does seem better overall.

@mergeconflict #9578 has a version of the subnet allocator type that follows this pattern, although I think I called it by the older term "selector". You're welcome to crib from that if you need.

mergeconflict · 2026-01-21T12:52:27Z

nexus/types/src/external_api/params.rs

 /// Specify how to allocate a floating IP address.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
-#[serde(tag = "type", rename_all = "snake_case")]
+#[serde(tag = "type", rename_all = "snake_case", deny_unknown_fields)]


Do I need deny_unknown_fields for ExternalSubnetAllocator as well?

Probably so, as someone could mistakenly pass pool and it would be silently ignored, right?

I don't believe we do this in general for API types, although I'm not sure there's a good reason for that. @ahl might have an opinion here too.

Yeah, I added it here due to #9680.

I think this comes about just due to the change in API semantics from what was expected? Of course we have versioned APIs now, but the issue showcased trying to treat the new API like an old one.

For this case, the {"type": "auto"} possibility may make it worthwhile? @ahl?

I lean against including deny_unknown_fields.

Like I said in my top level review, I think the reason we don't do it is we wanted to be generous and avoid blowing up when we don't have to. On the other hand, in most other spots when it comes up, we do tend to prefer erroring out and giving detailed explanations as to why. So maybe we should think about adding deny_unknown_fields on more request bodies.

However, a stronger reason might be this, from the serde docs:

Note: this attribute is not supported in combination with flatten, neither on the outer struct nor on the flattened field.

We do use flatten in create bodies:

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

Lines 557 to 561 in 5eb1337

/// Create-time parameters for a `Silo`

#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]

pub struct SiloCreate {

#[serde(flatten)]

pub identity: IdentityMetadataCreateParams,

The strongest argument specific to this case, though, is that while pool was present in FloatingIpCreate in v17, we're talking about AddressAllocator here, not FloatingIpCreate. If I understand correctly, this actually won't error in the most likely user error situation: a leftover pool param in the FloatingIpCreate body in an out of date client library or CLI script. This will only error if you try to do

{ "address_allocator": { "pool": "abc" } }

But since AddressAllocator is new in v18 and the pool param in there was only there for like a week, there is no existing customer code or out of date client code that expects to be able to stick pool in there. So in the name of consistency with all the other structs, I would leave it out.

That's good reasoning.

@askfongjojo note this thread for your testing btw.

mergeconflict · 2026-01-21T13:03:00Z

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

-                ip_version,
-            )
-            .await?;
+        let (authz_pool, explicit_ip) = match &allocation {


Do we also need to get the pool's rcgen to avoid concurrent modifications in the write that happens after this?

This follows the same pattern as resolve_pool_for_allocation, as neither checks pool rcgen. The protection happens in NextExternalIp allocation which atomically allocates the IP and bumps the range rcgen in one CTE.

The lookup just identifies which pool to allocate from, while the actual allocation is atomic itself. If the pool/range changes between lookup and allocation, NextExternalIp will fail.

I guess I'm wondering what happens if (for example) the authz_pool is unlinked from the silo after this block but before we allocate an IP from it. I think the range won't have changed, but the write shouldn't succeed. WDYT?

@mergeconflict unrelated to this PR, but you have hit on something that's already a TODO/issue:

/// Look up whether the given pool is available to users in the current /// silo, i.e., whether there is an entry in the association table linking /// the pool with that silo // // TODO-correctness: This seems difficult to use without TOCTOU issues. It's // currently used to ensure there's a link between a Silo and an IP Pool // when allocating an external address for an instance in that Silo. But // that works by checking that the link exists, and then in a separate // query, allocating an address out of it. Suppose the silo was unlinked // after this check, but before the external address allocation query ran. // Then one could end up with an address from an unlinked IP Pool, which // seems wrong. // // See https://github.com/oxidecomputer/omicron/issues/8992 pub async fn ip_pool_fetch_link( ...

As per the issue, there's a series of these to make more robust.

I was in this code a few months ago, which is where this comment and issue came from. There are lot of opportunities for races like this. I don't think we hit them in practice because linking / unlinking isn't that common, but we should definitely fix these things long term.

bnaecker

Looks good, thanks for fixing this.

david-crespo

API and help comments look great, and thanks for including all the example JSON bodies, that was very helpful. Just taking a look at the deny_unknown_fields question. Generally we want to be generous in what we accept unless it's very misleading to do so (which in this case it may be).

david-crespo · 2026-01-22T00:13:44Z

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

+            .select(IpPool::as_select())
+            .first_async::<IpPool>(
+                &*self.pool_connection_authorized(opctx).await?,
+            )


This is fun. Simpler than I pictured.

I asked Claude if we are missing any indexes that would help this query. It sounds quite plausible, but I'm not 100% sure -- key claim is that the join from ip_pool_range to ip_pool on pool ID would benefit from an index on ip_pool_id for the range table. Take it or leave it. It's a low-cost optimization.

The query in ip_pool_fetch_containing_address joins ip_pool_range → ip_pool → ip_pool_resource with these key filters:

ip_pool_resource.resource_id = silo_id (highly selective)

ip_pool.pool_type = pool_type

ip_pool_range.first_address <= ip AND last_address >= ip

The query planner could start from ip_pool_resource (highly selective by silo_id) and work toward ip_pool_range, but there's no index on ip_pool_range.ip_pool_id. This means when joining from ip_pool to ip_pool_range, CockroachDB must either:

Do a full scan of ip_pool_range (filtering by address afterward), or

Scan one of the address indexes and filter by ip_pool_id

The existing address indexes (lookup_pool_range_by_first_address, lookup_pool_range_by_last_address) were designed for overlap detection, not for range containment lookups.

A useful index would be:

CREATE INDEX IF NOT EXISTS ip_pool_range_by_pool_id ON omicron.public.ip_pool_range ( ip_pool_id ) WHERE time_deleted IS NULL;

This would allow the planner to:

Start from ip_pool_resource (filtered by silo_id)

Join to ip_pool (filtered by pool_type)

Use the new index to efficiently find ranges for those specific pools

Filter by address containment

The number of ranges per pool is typically small, so filtering by address within that subset is cheap.

zeeshanlakhani requested review from askfongjojo, bnaecker and david-crespo January 20, 2026 19:50

zeeshanlakhani self-assigned this Jan 20, 2026

zeeshanlakhani force-pushed the zl/fix-9680-floating-ip-allocation branch from e6ee9c3 to 48948b2 Compare January 20, 2026 20:04

bnaecker reviewed Jan 20, 2026

View reviewed changes

nexus/types/src/external_api/params.rs Outdated Show resolved Hide resolved

nexus/types/src/external_api/params.rs Show resolved Hide resolved

zeeshanlakhani requested a review from mergeconflict January 21, 2026 04:44

zeeshanlakhani changed the title ~~[floating-ip,fix,api] Fix floating IP creation API to allow pool-only explicit allocation~~ [floating-ip,fix,api] Fix floating IP creation API to distinguish explicit IP from pool selection Jan 21, 2026

zeeshanlakhani requested a review from bnaecker January 21, 2026 06:16

merge main into fix-9680

1114b75

zeeshanlakhani mentioned this pull request Jan 21, 2026

[multicast] Drop (m)vlan from multicast groups #9451

Open

mergeconflict reviewed Jan 21, 2026

View reviewed changes

bnaecker approved these changes Jan 21, 2026

View reviewed changes

david-crespo approved these changes Jan 21, 2026

View reviewed changes

david-crespo reviewed Jan 22, 2026

View reviewed changes

zeeshanlakhani added 2 commits January 22, 2026 00:55

[review] updates

042ae52

merge main into fix-9680

a0ae22a

zeeshanlakhani force-pushed the zl/fix-9680-floating-ip-allocation branch from 495b227 to a0ae22a Compare January 22, 2026 04:10

zeeshanlakhani enabled auto-merge (squash) January 22, 2026 04:22

zeeshanlakhani disabled auto-merge January 22, 2026 04:24

zeeshanlakhani enabled auto-merge (squash) January 22, 2026 04:28

zeeshanlakhani merged commit 3b27679 into main Jan 22, 2026
17 checks passed

zeeshanlakhani deleted the zl/fix-9680-floating-ip-allocation branch January 22, 2026 11:57

	/// Create-time parameters for a `Silo`
	#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
	pub struct SiloCreate {
	#[serde(flatten)]
	pub identity: IdentityMetadataCreateParams,

[floating-ip,fix,api] Fix floating IP creation API to distinguish explicit IP from pool selection #9687

[floating-ip,fix,api] Fix floating IP creation API to distinguish explicit IP from pool selection #9687

Uh oh!

Conversation

zeeshanlakhani commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergeconflict commented Jan 21, 2026

Uh oh!

zeeshanlakhani commented Jan 21, 2026

Uh oh!

zeeshanlakhani commented Jan 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeeshanlakhani Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeeshanlakhani Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeeshanlakhani Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeeshanlakhani Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnaecker left a comment

Choose a reason for hiding this comment

Uh oh!

david-crespo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zeeshanlakhani commented Jan 20, 2026 •

edited

Loading

zeeshanlakhani Jan 21, 2026 •

edited

Loading

zeeshanlakhani Jan 21, 2026 •

edited

Loading

zeeshanlakhani Jan 21, 2026 •

edited

Loading

zeeshanlakhani Jan 21, 2026 •

edited

Loading

david-crespo Jan 22, 2026 •

edited

Loading