-
Notifications
You must be signed in to change notification settings - Fork 59
Support IPv6 VPC-private addresses for network interfaces #9269
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
Conversation
4222a41 to
189254f
Compare
- Make more explicit how we assign IP addresses for NICs, by adding a full configuration type. That includes details of whether an address is auto assigned or explicitly requested, and the corresponding transit IPs. Also adds the notion of single-stack IPv4 or IPv6 NICs, or explicitly dual-stack. - Restructure some of the internals of the network interface `InsertQuery` to more clearly generate SQL based on the assignment preferences - Add `NextIpv6Address` type and use it internally to create VPC-private IPv6 addresses for interfaces. Add a few basic tests for the new behavior. - Rework callers of `IncompleteNetworkInterface` to use new IP configuration enum. - Closes #9243 - Closes #9244
189254f to
d1f5170
Compare
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.
Thanks @bnaecker. Comments follow
| ), | ||
| }, | ||
| None, //Request IP address assignment | ||
| IpConfig::auto_ipv4(), |
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.
Not for this PR but a general thought on this. I think what we'll want as a default is an IpConfig::auto_dual_stack()? Same for instances.
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, that sounds reasonable to me. There's no real reason to prevent VPC-private IPv6 addressing, since the address space is so huge.
| InsertError::IpAddressNotAvailable(ip) | ||
| let Some(ipv4) = interface.ip_config.ipv4_addr() else { | ||
| let err = Error::internal_error(&format!( | ||
| "Violated constraint that ensures unique \ |
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.
Does this mean auto assignment failed?
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.
Not quite, or at least that's not what I was going for. This means someone tried to insert an explicit IPv4 address, we filtered it out because it's already been used, and so we tried to insert NULL for both the IPv4 and IPv6 addresses.
So it's really "you requested a specific address, but it was already used".
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.
To put a finer point on it, this could be an assert. We made a programming error somewhere.
- IP-version specific first / last address functions - Fixup some names and typos - Allow transit IPs on an instance NIC - Add default dual-stack constructor
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.
LGTM!
InsertQueryto more clearly generate SQL based on the assignment preferencesNextIpv6Addresstype (needs tests)IncompleteNetworkInterfaceto use new assignment-request enum