Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API for no/default/custom NICs on instance create #1003

Open
david-crespo opened this issue May 3, 2022 · 5 comments
Open

API for no/default/custom NICs on instance create #1003

david-crespo opened this issue May 3, 2022 · 5 comments
Labels
api Related to the API. networking Related to the networking. nexus Related to nexus

Comments

@david-crespo
Copy link
Contributor

Moved from #960 (comment)

There's a related point here, which may deserve its own issue. The current API for creating NICs in Nexus allows clients to specify one of three kinds of data when creating an instance. That's defined here, where the variants have the following interpretations:

  • None: Give the instance no NICs at all
  • Default: Give the instance exactly on interface, the primary. It will be in the VPC and VPC Subnet both named "default", and it will have an auto-assigned IP.
  • Custom: Specify zero or more items of this type, giving full control over all the parameters of each NIC.

I originally made it this way when I thought we'd also support attaching an existing NIC, which is no longer the case. And the issue is that the last variant actually subsumes the others.

For example, if you want no NICs, use a list of size zero. We can use default values on the fields of the NetworkInterfaceCreate struct to make specifying the "default case" of getting just one primary interface pretty straightforward. I'm not sure if this is worth it though, so I'd love some feedback from others on whether this is worth pursuing. cc @david-crespo @zephraph Wondering if y'all have initial thoughts here?

@david-crespo david-crespo added api Related to the API. networking Related to the networking. nexus Related to nexus labels May 3, 2022
@david-crespo
Copy link
Contributor Author

@bnaecker:

Not sure where we came down on no interfaces. Seemed like @rmustacc tried hard to come up with a potential use case but couldn't quite bring himself to say it was important. For me that's a Won't Do unless we get it for free, which could happen depending on how the API is designed.

We can use default values on the fields of the NetworkInterfaceCreate struct to make specifying the "default case" of getting just one primary interface pretty straightforward.

Could you say more about what this looks like? I like having the clean Default enum variant that doesn't require constructing an attachment object in a particular way, but that also might prevent us from handling the case where someone wants the default NIC plus some set of custom ones. So maybe instead of the current API:

type NetworkInterfaceCreate = {
  description: string
  ip?: string | null
  name: Name
  subnetName: Name
  vpcName: Name
}

type InstanceNetworkInterfaceAttachment =
  | { type: 'Create', params: NetworkInterfaceCreate[] }
  | { type: 'Default' }
  | { type: 'None' }

type InstanceCreate = {
  ...
  networkInterfaces?: InstanceNetworkInterfaceAttachment | null
  ...
}

it would look more like

type InstanceNetworkInterfaceAttachment =
  | { type: 'Custom'; params: NetworkInterfaceCreate  }
  | { type: 'Default' }

type InstanceCreate = {
  ...
  networkInterfaces?: InstanceNetworkInterfaceAttachment[]
  ...
}

Is that something like what you had in mind? That covers all the cases neatly:

  • No interface (pass empty array)
  • Just default (pass [{ type: 'Default' }])
  • Default + custom (pass [{ type: 'Default' }, { type: 'Custom', ... }])
  • Just custom (pass [{ type: 'Custom', ... }, { type: 'Custom', ... }])

I'm less clear on whether we should allow networkInterfaces: null or leaving the key out of the post body, and if we do allow it, what it would mean. The fact that you could either expect it to mean no interface or default interface makes me lean toward disallowing it, though on the other hand if we think that 99.9% of the time people will want the default interface and no others, handling that with networkInterfaces: null (or left out) might be justifiable. I just don't love the idea of there being two ways to get the default interface.

@rmustacc
Copy link

rmustacc commented May 4, 2022

@david-crespo I thought I had at least a couple use cases that made more sense for no nics. Whether it's worth it, harder to say, but that was a lot more plausible to me than the no disks case. I would like to have it if possible, but there's a lot of things I'd like and not sure where the energy makes the most sense.

That said, the default stuff still bothers me a little bit because of the rest of the API support here or really more importantly a lack there of. When the idea of the "default" project, VPC, and VPC Subnet was introduced in RFD 21 it was to aid getting started and making that as painless as possible. However, it's also just as likely that customers will end up creating a blank project/vpc without the default VPC set up allowing them to create their own. At which point the choose the thing named "default" doesn't really make sense (to me). I don't think we want to define this in terms of a thing with a specific name, but rather it feels like a top-level project thing of if you don't select an image, VPC/VPC Subnet, instance type information, etc. this is what you get. Throwing out a project as it seems kind of a higher-level thing that you want all in one place rather than a part of the VPC itself.

I like the use of a type here a bit, that does seem better. Presumably it's illegal to pass default twice? Is there any value in trying to make this look similar for disks?

@david-crespo
Copy link
Contributor Author

david-crespo commented May 4, 2022

Sorry, I meant you tried and succeeded at coming up with some use cases.

Correct about two default NICs, I figured [{ type: 'Default' }, { type: 'Default' }] would just 400.

The interface for disks at instance create is already pretty similar to my suggestion. Maybe that's where I got the idea. The disks key is an array of these:

type InstanceDiskAttachment =
  | {
      description: string
      diskSource: DiskSource
      name: Name
      size: ByteCount
      type: 'create'
    }
  | {
      name: Name
      type: 'attach'
    }

(We should standardize on casing for type but that's a superficial change.)

GCP does something interesting with the default interface that fits with what you're saying about how it relates to the default VPC. Their default interface explicitly refers to the default VPC, and in order to add any other NICs you have to add another VPC. In our system, if you delete the default VPC and add another one instead (or worse, two), then { type: 'Default' } doesn't necessarily tell the API enough to make a NIC. If there are two VPCs, which one should it be associated with? But even if there's one custom VPC, it might be sort of surprising that we can just pick the one that's there and pick a subnet on it. I think in that case, any request to use { type: 'Default' } would have to 400, right?

Network interfaces section in GCP instance create UI image

@rmustacc
Copy link

rmustacc commented May 4, 2022

In our system, if you delete the default VPC and add another one instead (or worse, two), then { type: 'Default' } doesn't necessarily tell the API enough to make a NIC. If there are two VPCs, which one should it be associated with? But even if there's one custom VPC, it might be sort of surprising that we can just pick the one that's there and pick a subnet on it. I think in that case, any request to use { type: 'Default' } would have to 400, right?

Yeah, in our current system as defined, I think that's right. And the 400s in those different cases are all kind of unfortunate. I think this gets to the heart of my issue with the scheme. We're really conflating two different things:

  • A setup time default (the stuff that RFD 21 calls out for a "Default" VPC and a "Default" VPC Subnet.
  • A provision (instance create) time default that is used here.

Ideally our setup time default would set something on them that indicates that they're the thing for the provision time default. This would allow folks to set up their own defaults in a project that make sense. Right now the relationship between the provision time default is just by finding something called "Default", but in some ways, it feels worse to have provision time defaults that you can't set. So that's why I think we want to have something that nominates this, e.g. a /project/defaults endpoint that you can get/set or something. I think centralizing at the project level makes more sense, dunno. Thoughts?

@bnaecker
Copy link
Collaborator

bnaecker commented May 4, 2022

@rmustacc I agree that having a VPC and VPC Subnet made special just by their name is not great. I think the idea was always to eventually add the database table(s) and CRUD to manipulate those defaults. That's a good bit of work, though. Nothing hard, just takes time. It also brings up a few questions that I've been wrestling with. I'll try to spell those out here, with my attempts at answers for them.

What if a user opts to have no default VPC for a Project?

We should allow this. The implicit behavior may be undesirable in some cases, and this would require that the VPC and VPC Subnet be specified by name for any NIC that's created, either at instance provision or at a later time.

Are users allowed to delete the default VPC?

We should allow this, assuming the VPC could normally be deleted (e.g., that it's empty). In that case, the likely result would be to "unset" the default, so that there is no default for the Project.

How do we specify the default at Project-creation time?

RFD 21 talks about this a bit, but the idea is to have a setup_defaults boolean parameter in the Project creation request. If true, a default VPC, VPC Subnet, and set of default firewall rules will be created. (Side note, was this intended to exclude the default routes @rmustacc?) If false, no VPC or VPC Subnet will be created automatically. The properties of the default VPC and VPC Subnet can't be specified in this request. If the user wants something else, they should set setup_defaults=false, and create their desired defaults separately. The parameters are always:

  1. The /48 IPv6 prefix for the VPC is random.
  2. The /64 IPv6 prefix for the VPC Subnet is random.
  3. The IPv4 subnet for the VPC Subnet is 172.30.0.0/22.
  4. The names for the VPC and VPC Subnet are the literal string "default". I don't have a better alternative, and I expect users will find this pretty familiar if they've used something like AWS before.

How do we make the most common NIC option easy?

The most common choice users will likely make when creating a NIC for an instance is to have a single primary interface, with an automatically assigned IP address in the default VPC and VPC Subnet. If possible, the request that achieves this should have basically nothing in it referring to networking.

It should still be possible to create an instance with multiple interfaces, and with none at all, though these could be more awkward to specify in the request. One way to achieve this would look like this, in Rust:

pub enum InstanceNetworkInterfaceAttachment {
    DefaultPrimaryInterface,
    CustomInterfaces(Vec<NetworkInterfaceCreate>),
}

pub struct NetworkInterfaceCreate {
    pub name: String,
    pub description: Option<String>,
    pub vpc_name: Option<String>,
    pub subnet_name: Option<String>,
    pub ip: Option<std::net::IpAddr>,
}

As David pointed out, the second enum variant encompasses both zero and multiple NICs. If the user supplies DefaultPrimaryInterface, there's no control over any of the values, which I think prevents the possibility of requesting that default interface more than once. If there is no default VPC and/or VPC Subnet for the Project, 400 is returned, no matter which variant is used. The only non-obvious implicit behavior here is that the first element in the list inside the CustomInterfaces variant is used as the primary interface. I think that's pretty reasonable.

@rmustacc @david-crespo Let me know what y'all think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the API. networking Related to the networking. nexus Related to nexus
Projects
None yet
Development

No branches or pull requests

3 participants