Add IP Pools and contained IP ranges

[bnaecker]

• Adds basic CRUD API for IP Pools, as named, global resources
• Adds table and database operations for IP Pools
• Adds API for operating on IP ranges within an IP Pool. These are
  expressed as a first/last IP address pair, and describe a range of
  addresses to add or remove from a pool. Currently, all ranges must be
  non-overlapping, which is checked at insertion time with a custom
  query. Deleting a range currently requires specifying the exact same
  first/last address used to create it. Both of these could be relaxed
  in the future without changing the API, for example to support
  splitting or coalescing ranges.
• Adds paginated endpoint for listing the ranges within a pool, ordered
  by the first address in the range.
• Adds integration tests for new endpoints.
• Adds unauthorized coverage checks.

[smklein]

Looks great, nice job getting this integrated!

[smklein]

When it comes time to associate external IPs with instances (or services, like Nexus), do we expect to just use this table as-is, or create a new representation of "assigned" IP addresses?

[bnaecker]

I'm not entirely sure what that'll look like, but I've had some thoughts. TL;DR: I'm expecting to add the used IPs into things like the network_interface table.

So the IP Pools that we've implemented here are a bit of a conflation of two things: IP Pools as defined by RFD 21 and "address sets" defined by RFD 267. The former are addresses used for instance NAT. The latter are more general -- they are a superset of IP Pools, but also used for things like services (e.g, Nexus, DNS) and customer network integration (such as prefixes advertised in some routing protocol like BGP). The work here unfortunately merges these, because we need to get something and it's not entirely clear what's required for address sets yet.

That said, I'd expect that the address set table(s) will track what each set is for, and which will direct us to another table, say the ip_pool table, for finding the ranges within that set. Specifically for IP Pools in the sense of instance NAT, I do expect that we'll update the network_interface table (or add a new table) to include the IPs from this range that are currently in use. It all gets pretty hairy here in terms of the database operations, since now there are at least 3 tables to consider for certain operations, such as deleting an IP Pool resource.

[david-crespo]

Thanks for fixing the OpenAPI schema thing. Running this through the TS generator, there's no way to distinguish between IPv4 and IPv6 because of TS's structural typing. Usually we use a discriminator property to discriminate, like adding type: "ipv4" and type: "ipv6" respectively. I don't think there's anything you need to change here, because adding such a discriminator would not make sense in the Rust code. I will mull over whether I think Dropshot's OpenAPI spec generator should try to handle this, or maybe it's something we can automatically do in the TS generator whenever we see enums of a certain kind.

[bnaecker]

@david-crespo Sure thing, thanks for flagging it. What does the TS generator do for IpNet now? That works the same way, as an externally-tagged enum, e.g. {"V4": {"first": .., "last":, .... }}.

[david-crespo]

😁

type IpNet = Ipv4Net | Ipv6Net

type Ipv4Net = string

type Ipv6Net = string

So far it has not been a problem. I'll keep an eye on it. We also pull in the regex pattern for validating each one at runtime, but the types don't know anything about that.

[bnaecker]

Welp, that's a bummer 😆 . We can make them internally-tagged, e.g., the type/value keys that you referred to earlier. I don't really have a horse in the race -- it's all the same once we deserialize into Rust, so whatever makes the clients work better. @ahl Any suggestions here?

[ahl]

As I recall, we drew inspiration from std::net::IpAddr which serializes to a string: https://docs.serde.rs/src/serde/ser/impls.rs.html#654-675

And the JsonSchema for it is just: I'm a string but with a format = ip
https://docs.rs/schemars/latest/src/schemars/json_schema_impls/primitives.rs.html#52

For IpNet we had the actual regex for v4/v6 so included them in the schema (@bnaecker I think you did this work). It's probably more than is strictly necessary.

omicron/common/src/api/external/mod.rs

Lines 1206 to 1232 in 3690806

	impl JsonSchema for IpNet {
	fn schema_name() -> String {
	"IpNet".to_string()
	}
	fn json_schema(
	gen: &mut schemars::gen::SchemaGenerator,
	) -> schemars::schema::Schema {
	schemars::schema::SchemaObject {
	metadata: Some(
	schemars::schema::Metadata { ..Default::default() }.into(),
	),
	subschemas: Some(
	schemars::schema::SubschemaValidation {
	one_of: Some(vec![
	label_schema("v4", gen.subschema_for::<Ipv4Net>()),
	label_schema("v6", gen.subschema_for::<Ipv6Net>()),
	]),
	..Default::default()
	}
	.into(),
	),
	..Default::default()
	}
	.into()
	}
	}

As an aside, progenitor recovers the the type pretty well, but doesn't properly include the custom regex for deserialization:
https://github.com/oxidecomputer/progenitor/blob/main/progenitor-impl/tests/output/nexus.out#L566-L571

    #[derive(Serialize, Deserialize, Debug, Clone)]
    #[serde(untagged)]
    pub enum IpNet {
        V4(Ipv4Net),
        V6(Ipv6Net),
    }
    #[derive(Serialize, Deserialize, Debug, Clone)]
    pub struct Ipv4Net(pub String);

Part of the motivation here is to let consumers of the API simply say:

{
    "ip": "127.0.0.1"
}

Rather than

{
    "ip": {
        "v4": "127.0.0.1"
    }
}

... or something like that.

I think ideally a generator is going to either distinguish types based on these regexes or do a "deep collapse" i.e. saying

enum IpRange {
    IpV4Range {
        first: String,
        last: String,
    },
    IpV6Range {
        first: String,
        last: String,
    },
}

should be collapsed into

struct IpRange {
    first: String,
    last: String,
}

So either we should:

• drop all the fancy regex stuff and just say that these types are strings
• update the generators to either handle the regexes all the way or reduce structurally equivalent types

[ahl]

Also for context (at least for myself):
@smklein proposed switching to ipnet from ipnetwork #877 and to that end he added JsonSchema support krisprice/ipnet#41. Support for JsonSchema in ipnetwork appears to be busted achanda/ipnetwork#155 (I am going to investigate and file a bug).

[david-crespo]

From TS's POV it is already collapsed because the following are equivalent:

type X = { first: string; last: string } | { first: string; last: string }

type Y = { first: string, last: string }

[bnaecker]

Thanks Adam, that's helpful. I would argue we don't use the regular expressions for determining types. There has already been at least one bug, and I'm betting there are more. The server will ultimately parse the request data anyway, rather than validate they match the regex, so those errors will just be annoying for clients.

One option here is to not have an enum, but use a generic (@davepacheco proposed this above). For the range, it'd be something like IpRange<Ipv4Addr> and IpRange<Ipv6Addr>. The Nexus API would take IpRange<T>, although I don't know if that works with Dropshot's endpoint macro.

[bnaecker]

@smklein @davepacheco @david-crespo Thanks all for the helpful input! Let me know if y'all have any further comments.