oxidecomputer / oxidecomputer/omicron

OmicronZoneType variants don't describe all the addresses in use by those zones

Open
#6,796 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

OmicronZoneType::InternalDns contains two socket addresses because it's effectively running two services (a dropshot server for administration, and a DNS server): https://github.com/oxidecomputer/omicron/blob/f3ab3387b640de27875502209a55211087e9b23d/nexus-sled-agent-shared/src/inventory.rs#L232-L233

We have several other variants that are running multiple services but which only contain a single socket address:

  • OmicronZoneType::Cockroach has the address for cockroach itself, but does not describe the address for the cockroach-admin server running alongside
  • Similarly, OmicronZoneType::{Clickhouse,ClickhouseServer,ClickhouseKeeper} have the address for clickhouse itself, but do not describe the address for the clickhouse-admin server

sled-agent (and DNS, probably?) is forced to combine the IP from one address (e.g., Cockroach.address) with a hard-coded port number (COCKROACH_ADMIN_PORT) in order to configure these supplementary services.

This doesn't seem particularly urgent or terrible, as constant ports are all we use today, but is somewhat inconsistent. #6794 removed OmicronZoneConfig::underlay_address (the parent struct of OmicronZoneType) to remove some duplication; I think there are at least two possible ways to address this issue (one of which would undo #6794, and both of which would require some care to support compatibility across upgrades):

  • Add new SocketAddrV6 fields to the relevant variants for their supplementary services
  • Restore OmicronZoneConfig::underlay_address, and replace the existing OmicronZoneType SocketAddrV6 fields with just port numbers (and add port numbers for the missing services while we're there)

These aren't equivalent: the first (which matches how our DNS zone types are described) allows different services in the zone to run on different IPs; the second would require all services to use the same IP. We've sprinkled assumptions that a given Omicron service zone only has one underlay IP throughout the codebase, but I'm not sure whether that was an intentional design choice or something we may want to change in the future.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with OmicronZoneType in nexus-sled-agent-shared/src/inventory.rs, especially InternalDns, Cockroach, Clickhouse, ClickhouseServer, and ClickhouseKeeper, then trace how sled-agent derives supplementary service addresses. Compare adding SocketAddrV6 fields with restoring OmicronZoneConfig::underlay_address, including compatibility across upgrades; done means the chosen design describes all service addresses without hard-coded ports.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, networking
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.