Adds basic types to represent VPCs, subnets, and NICs. #163
Conversation
smklein
left a comment
smklein
left a comment
There was a problem hiding this comment.
Thanks for putting this together - my comments are (currently) mostly restricted to the model.rs API
- Adds the `VPC`, `VPCSubnet` and `VNIC` types, including internal control plane representations, client views, and database types. - Implements some required traits for generating JSON schema and serializing to/from PostgreSQL wire formats. Note that the MAC address type is serialized in the database as a string, as CockroachDB doesn't currently support the PostgreSQL MACADDR type.
bnaecker
force-pushed
the
nexus/basic-networking-types
branch
from
9d60010 to
e5d5317
Compare
|
@smklein @davepacheco Updated in response to offline chat with Sean, let me know if either of y'all have other issues! |
omicron-nexus/src/db/schema.rs
Outdated
| "time_created", | ||
| "time_modified", | ||
| "time_deleted", | ||
| // TODO: Add project-scoping: "project_id", |
There was a problem hiding this comment.
Is there anything surprising required here? Based on instances and disks, it looks like you just add project_id in a few spots: here, dbinit.sql, model.rs, model_db.rs. If it's straightforward, I could make an attempt.
There was a problem hiding this comment.
I think the original reason to not do this up front was that it would allow us to hardcode a single VPC and VPC Subnet in the populate script. I think we're already creating a new VPC and VPC Subnet at project-create time, in which case yeah, we may as well just put the project_id in it.
There was a problem hiding this comment.
If VPCs are project-scoped and subnets are VPC-scoped, do we need to make subnets explicitly project-scoped? Similar question for NICs I guess.
There was a problem hiding this comment.
My assumption is that these would be implicitly project-scoped, not explicitly -- just for the usual database normalization reasons. If we want to list the VPC Subnets in a Project, we'd do that with a join.
| /** The IP address assigned to this interface. */ | ||
| pub ip: IpAddr, |
There was a problem hiding this comment.
After seeing @Nieuwejaar 's presentation in the control plane sync today, IMO we should probably be explicit this is an "internal" IP address.
Sounds like Nexus is also going to be responsible for assigning + telling the switch/OPTE about the external IP address too, so we can do effective translation.
Should we add an optional (I assume it would be optional - seems possible to have internal-only NICs) "external IP address" field here too?
(also FYI @rzezeski for feedback)
There was a problem hiding this comment.
After seeing @Nieuwejaar 's presentation in the control plane sync today, IMO we should probably be explicit this is an "internal" IP address.
More precisely, I think this is an IP address from the associated VPC Subnet. I'm not sure it's always "internal", depending on how you mean that: a customer may eventually have two VPC Subnets with routes between them, and this address would be visible on that other network, right?
There may be two layers of abstraction here: the VNIC that we store in the database presumably knows about VPC Subnets and knows this is an IP address from that subnet. By the time we pass this information to Sled Agent, I'm not sure it needs to know that -- it's just an IP address, wherever it came from.
Should we add an optional (I assume it would be optional - seems possible to have internal-only NICs) "external IP address" field here too?
Hmm. I've been assuming that when we implement Floating IPs and Ephemeral IPs, these would have separate records in the database for those. That is, we'd create a row in a FloatingIp table that contains the VnicId and the public IP address. This makes it easy to see which have been allocated, attached where, etc. And it makes it easy to have different kinds of public IPs on the VNIC or even more than one of them (e.g., more than one Floating IP). I'm not sure if the latter is something we intend to support in v1 but it seems reasonable. Thoughts?
There was a problem hiding this comment.
More precisely, I think this is an IP address from the associated VPC Subnet. I'm not sure it's always "internal", depending on how you mean that: a customer may eventually have two VPC Subnets with routes between them, and this address would be visible on that other network, right?
Yeah, you're right - I came at this with the framing of "internal to the VPC networks" vs "external to the outside world" mindset, but I agree that those frames are relative. Perhaps just clarification that "IP address allocated within the VPC subnet" would be enough to distinguish it from "the other one". (this admittedly may be inferred from context, but I like being as explicit as we can be, since there so many dang layers at play)
There may be two layers of abstraction here: the VNIC that we store in the database presumably knows about VPC Subnets and knows this is an IP address from that subnet. By the time we pass this information to Sled Agent, I'm not sure it needs to know that -- it's just an IP address, wherever it came from.
I'm not so certain - from the point of view of Propolis, yes, it can just act on the VPC subnet, but there may be information that needs ferrying to OPTE to transform packets, which would presumably be proxied through the sled agent.
Hmm. I've been assuming that when we implement Floating IPs and Ephemeral IPs, these would have separate records in the database for those. That is, we'd create a row in a FloatingIp table that contains the VnicId and the public IP address. This makes it easy to see which have been allocated, attached where, etc. And it makes it easy to have different kinds of public IPs on the VNIC or even more than one of them (e.g., more than one Floating IP). I'm not sure if the latter is something we intend to support in v1 but it seems reasonable. Thoughts?
I think this may be another case where "db representation" != "internal API representation".
From a storage perspective I absolutely agree with you, that makes sense - use a different object, reference this NIC by UUID, do querying that way, etc.
From an API perspective, I think that public IP would also need transferring to OPTE running on the Sled - IMO it makes sense to put it in the structure being passed as a part of the "instance_ensure" invocation.
There was a problem hiding this comment.
More precisely, I think this is an IP address from the associated VPC Subnet. I'm not sure it's always "internal", depending on how you mean that: a customer may eventually have two VPC Subnets with routes between them, and this address would be visible on that other network, right?
Yeah, you're right - I came at this with the framing of "internal to the VPC networks" vs "external to the outside world" mindset, but I agree that those frames are relative. Perhaps just clarification that "IP address allocated within the VPC subnet" would be enough to distinguish it from "the other one". (this admittedly may be inferred from context, but I like being as explicit as we can be, since there so many dang layers at play)
I think the way I would frame this distinction is actually what addresses are on objects that are visible from inside of the VM on IP interfaces. That is, does a user see this when they run ifconfig or ip addr or ipconfig /all.
Otherwise all the other addresses we've talked about, ephemeral and floating, are not seen on interfaces inside of the guest. The reason I like phrasing it this way is that pretty much that's the thing that really impacts a bit of who and what is receiving traffic and what rules are required where.
There may be two layers of abstraction here: the VNIC that we store in the database presumably knows about VPC Subnets and knows this is an IP address from that subnet. By the time we pass this information to Sled Agent, I'm not sure it needs to know that -- it's just an IP address, wherever it came from.
I'm not so certain - from the point of view of Propolis, yes, it can just act on the VPC subnet, but there may be information that needs ferrying to OPTE to transform packets, which would presumably be proxied through the sled agent.
Information may be proxied through sled-agent, but I'd probably be careful to distinguish between sled-agent being basically just being a transport or it actually needing to know a bunch of stuff.
There was a problem hiding this comment.
I think the way I would frame this distinction is actually what addresses are on objects that are visible from inside of the VM on IP interfaces. That is, does a user see this when they run
ifconfigorip addroripconfig /all.Otherwise all the other addresses we've talked about, ephemeral and floating, are not seen on interfaces inside of the guest. The reason I like phrasing it this way is that pretty much that's the thing that really impacts a bit of who and what is receiving traffic and what rules are required where.
Ack - IMO it would still be handy to clarify this is the IP address "inside the VM'.
Information may be proxied through sled-agent, but I'd probably be careful to distinguish between sled-agent being basically just being a transport or it actually needing to know a bunch of stuff.
I think we're on the same page here - it just needs to get to OPTE somehow, but sled-agent is doing little else with the info.
VPC,VPCSubnetandVNICtypes, including internalcontrol plane representations, client views, and database types.
serializing to/from PostgreSQL wire formats. Note that the MAC address
type is serialized in the database as a string, as CockroachDB doesn't
currently support the PostgreSQL MACADDR type.