-
Notifications
You must be signed in to change notification settings - Fork 62
Adds basic types to represent VPCs, subnets, and NICs. #163
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e5d5317
Adds basic types to represent VPCs, subnets, and NICs.
bnaecker 8e3dc47
Splits IP v4 and v6 subnet types
bnaecker 63b51d4
Use STRING in MAC addr DB type for uniformity, and add TODO about NIC…
bnaecker cef9ecc
Rename VNIC -> NetworkInterface
bnaecker 7a4021b
Add serialization derives to NetworkInterface
bnaecker 22d12f8
put project_id on VPC model, remove TODO on subnet and NIC
david-crespo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack - IMO it would still be handy to clarify this is the IP address "inside the VM'.
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.