Support multiple external IPs in blueprint zone type by bnaecker · Pull Request #11236 · oxidecomputer/omicron

bnaecker · 2026-09-03T21:42:59Z

Add support for multiple EIPs to the blueprint zone types for Nexus, External DNS, and Boundary NTP zones.
Add some newtypes and wrappers to support lists of these or up to 2 of them for the SNAT case of Boundary NTP.
Add a test that the full blueprint with multiple addresses round-trips through the database.
Planner still emits exactly one address in all these cases, this is only the structural change to support multiple addresses.
Update lockstep OpenAPI docs

- Add support for multiple EIPs to the blueprint zone types for Nexus, External DNS, and Boundary NTP zones. - Add some newtypes and wrappers to support lists of these or up to 2 of them for the SNAT case of Boundary NTP. - Add a test that the full blueprint with multiple addresses round-trips through the database. - Planner still emits exactly one address in all these cases, this is only the structural change to support multiple addresses. - Update lockstep OpenAPI docs - Closes #9288

- Check uniqueness of external IP UUIDs too in container types - Ensure entire set of IPs roundtrips to DB, not just the new one added later. - Check any number of already-allocated EIPs for a zone when checking if it's already been allocated, rather than bailing if there is more than one. - Clean up conversion between blueprint -> inventory types, and simplify proptests - Misc comment cleanup, clarity, typos, etc, regenerate OpenAPI doc

- Fixup test catching bad reallocations of the same EIP by ID - Support multiple IPs per zone in the `OmicronZoneNetworkResources` type. Loosen one of the internal maps from a three-key to two-key, enforcing only uniqueness of the IP and its ID, so that we can have multiple entries per zone. Add an explicit unit test for that.

Have the sled-agent populate all the private IP addresses an External DNS zone needs in the sled-agent, to back all of its external IPs.

jgallagher

This all LGTM. I'm okay approving, but would like to give Dave a chance to look at this if time allows. I'm a little nervous I'm missing something that may bite us when we go to use this, but (a) I don't have anything specific to point to and (b) if we did miss anything I think we can probably fix it when we discover it.

With the changes on just this branch, could we add or extend any of the existing reconfigurator tests to exercise multiple IPs? Or do we need more changes to the planner get to that point?

bnaecker · 2026-09-11T15:41:19Z

With the changes on just this branch, could we add or extend any of the existing reconfigurator tests to exercise multiple IPs? Or do we need more changes to the planner get to that point?

I think we need to change the planner. I have the next branch in my chain lined up, in which for_new_nexus() returns multiple IPs for a Nexus zone. I think that's the minimum required to test things at the planner, but I'd rather keep that separate given this is already too big.

bnaecker · 2026-09-11T16:24:29Z

I'm on the fence, leaning toward I don't think a newtype just for the vec would be worth it here. OTOH this method is already returning a tuple - maybe we could return a struct instead, something like

I like this idea. It's not much more work than this, and will be a good bit safer. I'll take it for a spin.

davepacheco · 2026-09-11T15:59:16Z

                        "dns_address",
                        "astring",
-                        private_dns_address,
+                        private_dns_addresses,


Reasoning out loud:

This is fine if there's only one IP address, regardless of whether the DNS image predates or postdates Listen on multiple addresses in the DNS server(s) #11207.

This is fine if there's multiple addresses as long as the DNS image postdates Listen on multiple addresses in the DNS server(s) #11207.

I'm not sure what this would do if there's multiple IPs and the image predates Listen on multiple addresses in the DNS server(s) #11207. I imagine we think that's impossible because in order to have multiple IPs set, you need to have updated to a target release that supports that, and that's necessarily post-11207?

I can't think of a way for this to go wrong (that's not pretty out-of-bounds, like a backwards MUPdate), but it does make me nervous. I don't have any suggestions though!

This was my conclusion. You could only have multiple addresses here if Nexus has been updated to include this PR, which is after 11207. So all the zones have to be updated first, and in that case, even if they're the old zone image that supports 1 IP and the sled-agent supports multiple, the actual zone configuration can only have one IP in the plan.

I agree that we should figure out how to minimize these inter-zone dependencies. I think Nexus does a pretty good job in this regard, since it has a configuration file we deserialize into a defined Rust type. Using these CLI arguments, especially through SMF properties, feels pretty fraught to me.

davepacheco · 2026-09-11T17:35:46Z

+    }
+}
+
+impl std::cmp::Ord for OmicronZoneExternalFloatingIps {


It does seem kind of weird to define an ordering between two sets of things. I'd be inclined to remove it but I guess it depends on what consumer needs it.

bnaecker · 2026-09-11T18:33:49Z

It does seem kind of weird to define an ordering between two sets of things. I'd be inclined to remove it but I guess it depends on what consumer needs it.

The parent container blueprint_zone_type::Nexus is Ord, so we need it on this type. It's also required on a whole chain of parent types, e.g., BlueprintZoneType the enum and BlueprintZoneConfig. I think this is all basically because we want to stuff many of these types into IdOrdMaps, such as the field BlueprintSledConfig.zones.

I agree it's a bit odd. I also think we need it if we want to (1) internally use an IdOrdMap in this type itself, and (2) continue to use IdOrdMaps in anything transitively containing this.

- Use richer type for returning a zone's external networking data, update callers - Drop unnecessary Cow - Comment cleanup

bnaecker · 2026-09-11T18:51:15Z

Thanks @davepacheco and @jgallagher. I think I've addressed everything except for the implementation of Ord. I think it's required, but I could be missing something. LMK if y'all have more thoughts!

…blueprint-zone-types

davepacheco · 2026-09-11T22:23:03Z

It does seem kind of weird to define an ordering between two sets of things. I'd be inclined to remove it but I guess it depends on what consumer needs it.

The parent container blueprint_zone_type::Nexus is Ord, so we need it on this type. It's also required on a whole chain of parent types, e.g., BlueprintZoneType the enum and BlueprintZoneConfig. I think this is all basically because we want to stuff many of these types into IdOrdMaps, such as the field BlueprintSledConfig.zones.

I agree it's a bit odd. I also think we need it if we want to (1) internally use an IdOrdMap in this type itself, and (2) continue to use IdOrdMaps in anything transitively containing this.

For IdOrdMap, you only need the entries to impl IdOrdItem, which only requires that the key is Ord. If the key is the zone id, that's easy for BlueprintZoneConfig without the whole type being Ord. I wonder if we could get away with that instead of impl'ing Ord all the way down. This is outside the scope of this PR, for sure.

bnaecker · 2026-09-11T22:49:34Z

Thanks @davepacheco, that makes sense. I can make a follow-up issue, if that's helpful? And LMK if you have other thoughts on the PR at this point.

bnaecker force-pushed the ben/multiple-eips-in-blueprint-zone-types branch 3 times, most recently from 36eb2df to 3a6bc8d Compare September 7, 2026 02:46

bnaecker force-pushed the ben/multiple-eips-in-blueprint-zone-types branch from 3a6bc8d to 9c66fa5 Compare September 7, 2026 14:05

bnaecker requested review from davepacheco and jgallagher September 9, 2026 03:44

jgallagher reviewed Sep 10, 2026

View reviewed changes

bnaecker added 4 commits September 10, 2026 16:19

Merge branch 'main' into ben/multiple-eips-in-blueprint-zone-types

daddca4

Review feedback

ae7a552

Have the sled-agent populate all the private IP addresses an External DNS zone needs in the sled-agent, to back all of its external IPs.

bnaecker requested a review from jgallagher September 11, 2026 01:12

jgallagher reviewed Sep 11, 2026

View reviewed changes

Comment thread nexus/types/src/deployment/network_resources.rs Outdated

Comment thread sled-agent/src/services.rs Outdated

Comment thread nexus/types/src/deployment/zone_type.rs Outdated

davepacheco reviewed Sep 11, 2026

View reviewed changes

bnaecker mentioned this pull request Sep 11, 2026

Want types for representing non-empty collections #11280

Open

Review feedback

628f05d

- Use richer type for returning a zone's external networking data, update callers - Drop unnecessary Cow - Comment cleanup

bnaecker requested review from davepacheco and jgallagher September 11, 2026 18:51

Fixup missed test callsites

784a056

jgallagher approved these changes Sep 11, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into ben/multiple-eips-in-…

b3dab2e

…blueprint-zone-types

davepacheco mentioned this pull request Sep 11, 2026

BlueprintZoneConfig and friends should not have to impl Ord/PartialOrd #11284

Open

Uh oh!

Conversation

bnaecker commented Sep 3, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnaecker commented Sep 11, 2026

Uh oh!

bnaecker commented Sep 11, 2026

Uh oh!

Uh oh!

davepacheco Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

bnaecker Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davepacheco Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bnaecker commented Sep 11, 2026

Uh oh!

bnaecker commented Sep 11, 2026

Uh oh!

davepacheco commented Sep 11, 2026

Uh oh!

bnaecker commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants