Simplify shared external IP configuration type#10148
Conversation
- Changes the `ExternalIpConfig` type from a complicated enum to a struct with two optional fields. This lets us more simply and cleanly represent the IP stacks we support now, and also paves the way for supporting external IP configurations that have an Ephemeral or Floating IP, but without an SNAT address. - Keep the original type around in omicron common for backwards compatibility, using it in the previous versions of the sled-agent API. Add a new sled-agent API version and entrypoints for converting. - Removes a bunch of code that's now unnecessary, since we no longer need to enforce the complicated type's invariants. We can remove the custom deserialization logic and the entire `ExternalIpConfigBuilder` type. - Fixes #9839
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
This looks great - just a handful of questions / style nits.
| #[serde(remote = "ExternalIps")] | ||
| struct ExternalIpsShadow<T> | ||
| // Private type only used for deriving the JSON schema for `ExternalIps`. | ||
| #[derive(JsonSchema)] |
There was a problem hiding this comment.
I feel like this is a dumb question but - why do we need this type / what's stopping us from deriving JsonSchema directly on ExternalIps?
There was a problem hiding this comment.
Only the fact that the derived name of the schema is terrible. It's something like ExternalIps_for_Ipv4, or equally bad. I wanted the schema to have the same name as the type alias, and I wasn't sure how to do that otherwise.
| .ipv6_config() | ||
| .v6 | ||
| .as_ref() | ||
| .map(|v6| build_external_ipv6_config(Some(v6))); |
There was a problem hiding this comment.
Tiny nit - could these two be
let external_ips_v4 =
build_external_ipv4_config(external_ips.v4.as_ref());
let external_ips_v6 =
build_external_ipv6_config(external_ips.v6.as_ref());instead? At a glance that means they'd be non-optional-but-possibly-empty ExternalIpCfgs instead of Option<ExternalIpCfg>, but maybe that's a sign there's something else here that's a little off type wise? E.g., what's the difference between None and Some(external_ip_cfg) where external_ip_cfg is whatever's returned by build_external_ipvX_config(None)?
There was a problem hiding this comment.
There's not really a difference, but it makes the detection of a completely-empty configuration a bit easier. That said, I think I kept it most for historical reasons, and that it might be cleaner to remove it entirely. I'll either implement the suggestion you have, or clean it up more substantially. Thanks.
There was a problem hiding this comment.
Ah, actually we can't quite do that. The fields of the the SetExternalIpsReq below are optional, so we need to return an actual Option<ExternalIpCfg<_>>. build_external_ipvX_config() returns an ExternalIpCfg<_> directly. I realize this is all kind of murky, but the short answer is: OPTE still has the same kind of representation as the one this PR removes. It uses Option<ExternalIpCfg>, where None means there is no IP stack of that version, and Some(_) means there is an IP stack, although it looks like it could be empty. The comment just above this block also describes this.
Hopefully that makes sense, and I'll try to clean this up as much as I can.
There was a problem hiding this comment.
I left this as-is in c390700. I think it's about as clean as it can get already, and the mismatch here is between OPTE's and Omicron's mechanisms for representing "no IP stack of a particular version". LMK if you have other things you'd suggest here though.
There was a problem hiding this comment.
Ah, I didn't realize pushing this farther would require changes to OPTE. Yeah, agreed this is a perfectly reasonable stopping point.
- DCE - Add some useful helpers - Make list of floating IPs into a set. We have to add "frozen" versions of the original types to the older v1 to make this work, which is a little more code but worth it.
| .ipv6_config() | ||
| .v6 | ||
| .as_ref() | ||
| .map(|v6| build_external_ipv6_config(Some(v6))); |
There was a problem hiding this comment.
Ah, I didn't realize pushing this farther would require changes to OPTE. Yeah, agreed this is a perfectly reasonable stopping point.
2 participants
ExternalIpConfigtype from a complicated enum to a struct with two optional fields. This lets us more simply and cleanly represent the IP stacks we support now, and also paves the way for supporting external IP configurations that have an Ephemeral or Floating IP, but without an SNAT address.ExternalIpConfigBuildertype.ExternalIpConfigis too complicated #9839