Skip to content

Adds basic support for IPv6 #255

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 3 commits into from
Sep 19, 2022
Merged

Adds basic support for IPv6 #255

merged 3 commits into from
Sep 19, 2022

Conversation

bnaecker
Copy link
Contributor

  • Adds IPv6 support to the OPTE API and main engine types. This includes fleshing out some missing edges for IPv6 addresses and CIDRs, and adding support for specifying IPv6 addresses in router entries, etc. The main type expanded here is the VpcCfg, which now supports an IpCfg that specifies all L3 information. That supports exactly one IPv4 or IPv6, or one of each, for private addresses. An optional SNAT and external address for each are also supported.
  • Updates the opte-ioctl and opteadm crates to support IPv6, and to use a VpcCfg as the argument, rather than a bunch of disparate arguments. Fleshes out handling for IPv6 in router entries, port info and printing, and layer / rule printing.
  • Adds a few niceties to the D scripts for pretty-printing IPv6
  • Renames a lot of IPv4 specific types, such as Dhcp4Reply to DhcpReply. Types without a prefix will be assumed to refer to IPv4, and IPv6 will always have a version number.
  • Adds an icmpv6 layer to opte and the oxide-vpc, and inserts it in the configuration created by the xde driver. This supports ICMPv6 echo requests from the guest to the gateway only. An integration test verifying the hairpinned echo reply is also here.
  • Updates the API version check script to compare all commits relative to the master branch, rather than the last.

- Adds IPv6 support to the OPTE API and main engine types. This includes
  fleshing out some missing edges for IPv6 addresses and CIDRs, and
  adding support for specifying IPv6 addresses in router entries, etc.
  The main type expanded here is the `VpcCfg`, which now supports an
  `IpCfg` that specifies all L3 information. That supports exactly one
  IPv4 or IPv6, or one of each, for private addresses. An optional SNAT
  and external address for each are also supported.
- Updates the `opte-ioctl` and `opteadm` crates to support IPv6, and to
  use a `VpcCfg` as the argument, rather than a bunch of disparate
  arguments. Fleshes out handling for IPv6 in router entries, port info
  and printing, and layer / rule printing.
- Adds a few niceties to the D scripts for pretty-printing IPv6
- Renames a lot of IPv4 specific types, such as `Dhcp4Reply` to
  `DhcpReply`. Types without a prefix will be assumed to refer to IPv4,
  and IPv6 will always have a version number.
- Adds an `icmpv6` layer to `opte` and the `oxide-vpc`, and inserts it
  in the configuration created by the `xde` driver. This supports ICMPv6
  echo requests from the guest to the gateway only. An integration test
  verifying the hairpinned echo reply is also here.
- Updates the API version check script to compare all commits relative
  to the `master` branch, rather than the last.
@bnaecker bnaecker requested a review from rzezeski September 16, 2022 22:09
@bnaecker
Copy link
Contributor Author

In addition the tests in the repo, I've also tested we can actually launch guests with Omicron using the updated xde. IPv4 support appears unchanged -- guests can still talk to each other and reach the internet. I've also verified that we can ping6 the gateway from the guest. I have a modified Omicron which hardcodes an address of fd00:abcd:abcd:abcd::5 in the guest, which I added manually in the guest with ip -6 addr add. Also, note that NDP isn't implemented, so you'd need to add the OPTE gateway as a neighbor with: sudo ip -6 neigh add <IPv6> lladdr a8:40:25:ff:77:77 dev <iface>. Once that's done, from the guest we see:

debian@myinst:~$ ping6 fd00:abcd:abcd:abcd::1 -c 1
PING fd00:abcd:abcd:abcd::1(fd00:abcd:abcd:abcd::1) 56 data bytes
64 bytes from fd00:abcd:abcd:abcd::1: icmp_seq=1 ttl=255 time=0.126 ms

--- fd00:abcd:abcd:abcd::1 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.126/0.126/0.126/0.000 ms
debian@myinst:~$

OPTE is actually hairpinning the packet:

bnaecker@feldspar : ~/opte $ pfexec ./dtrace/opte-trace opte-rule-match
<SNIP>
PORT     LAYER        MATCH  DIR FLOW                                        ACTION
opte0    icmpv6       YES    out ICMPv6,[fd00:abcd:abcd:abcd::5]:0,[fd00:abcd:abcd:abcd::1]:0 HAIRPIN: ICMPv6 Echo Reply (A8:40:25:FF:77:77,fd00:abcd:abcd:abcd::1) => (A8:40:25:FA:4E:42,fd00:abcd:abcd:abcd::5)
^C

Copy link
Contributor

@rzezeski rzezeski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of great work. It feels good to see various IPv6 todo!s eliminated! I think most of my comments are pretty straightforward and shouldn't require much modification. This is pretty much how I would have done all this work; though I don't think I would have thought of some of the nice simplifications you made in various places.

@bnaecker
Copy link
Contributor Author

bnaecker commented Sep 19, 2022

The opte-xde check is failing on a format-check, though I really don't like how cargo fmt wants to format the block. This might be some interaction between a line-length limit and the wrapping logic, but this in particular is horrible:

1437                         } else {
1438                             // This should be unreachable!(). We have an IP header,
1439                             // but neither `ip4()` nor `ip6()` returned something.
1440                             opte::engine::dbg(format!(
1441                             "Packet contains IP header, but neither `ip4()` \
1442                                 nor `ip6()` returned `Some`. IP header: {:?}",
1443                             ip_hdr,
1444                         ));
1445                             false
1446                         }

That's putting the closing parenthesis for the function call at the same indention as the surrounding if-else bracing, which is extremely confusing. I'm going to play with the line length a bit, see if I can't convince cargo fmt to...not suck.

- Better router error message
- Better error messages when parsing IpAddr / IpCidr
- Better comments throughout, some better type names
- DCE
- Fix ARP handling to unconditionally drop outbound requests for
  anything other than the gateway, and all inbound requests.
@rzezeski
Copy link
Contributor

No worries on cargo fmt, it is what it is, I wouldn't spent too much time trying to get it to heel.

- Renamed `public_ip` -> `external_ip` fields on NAT-related types. This
  is important because the "outside" IP address for NAT need not
  actually be an address that's routable on the public Internet. It can
  be any address in any network on the other side of the NAT node.
- Fix location of Copy derive
@bnaecker
Copy link
Contributor Author

Got the fmt resolved. It was line length, I just split the lines into shorter pieces and it did the right thing here.

@bnaecker
Copy link
Contributor Author

bnaecker commented Sep 19, 2022

Thanks for the detailed review @rzezeski!

@bnaecker bnaecker merged commit 0023d5a into master Sep 19, 2022
@bnaecker bnaecker deleted the icmpv6-support branch September 19, 2022 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants