FromStdIP has surprising behavior with a v4 net.IP represented at 16 bytes

[danderson]

Just encountered this in Tailscale, converting from net.IP into netaddr.IP for one of our libraries.

If you give FromStdIP an IPv4 address that currently happens to be in its 16-byte internal representation, FromStdIP will create a v6 mapped v4 netaddr.IP. Because of net.IP's whacky treatment of pure v4 vs. mapped v4, this requires a fairly clunky pattern to use FromStdIP when you want the intuitive behavior:

stdip := getAv4NetIPFromSomewhere()
if stdip4 := stdip.To4(); stdip4 != nil {
    stdip = stdip4
}
return netaddr.FromStdIP(stdip)

Maybe FromStdIP should canonicalize the net.IP before converting it? That would prevent people from deliberately creating a v6 mapped v4 netaddr.IP from a net.IP, but that seems like a bit of a corner case - and one we could address with helpers like netaddr.To4in6(), for cases where you deliberately want a v6 address representing a v4 IP.

cc @mdlayher @bradfitz thoughts?

[danderson]

This is actually especially bad when using FromStdIPNet, because if the net.IPNet's IP is in the wrong form, you have to do a ton of legwork to pull apart the net.IPNet, fix up the IPs and mask, then reconstruct the net.IPNet and feed that to FromStdIPNet :(

[mdlayher]

Unfortunately I think this might be the expected behavior. While it is a nuisance that the stdlib defaults to 16 bytes for e.g. net.IPv4(192, 0, 2, 1), I suppose I would expect that netaddr uses the same logical representation when passed a 16-byte IPv4 address.

The caller is able to explicitly pass net.IPv4(192, 0, 2, 1).To4() if they want though.

That said, I am not opposed to assuming the caller wants the standard 4-byte representation for IPv4 and requiring an explicit method call to opt in to the mapped representation. We'd just want to document it well.

[mdlayher]

One clarification question: I assume Tailscale folks are optimizing for minimal memory use in constrained environments?

I am focused more on API ergonomics since I am running this on pretty much all amd64 machines.

But I would be in favor of picking the smallest representation as the default and allowing the caller to opt in to the larger representation if need be.

[danderson]

In this case, I was optimizing for not breaking Tailscale :) If you pass a v6 mapped v4 into netlink, it gets configured as an IPv6 address, not an IPv4 address, and your routing, uh, stops working. Prior to the conversion to netaddr, we were passing around ipv4 addresses, but the way I had to apply the translation into netaddr accidentally created a v6 mapped address instead.

This is the impedance mismatch between the stdlib and netaddr rearing its head: if you .String() a v6-mapped v4 net.IP, it gets printed as IPv4. If you .String() a v6-mapped v4 netaddr.IP, you get the v6-mapped representation. This is as-expected for netaddr, since we're treating v4 and v6-mapped v4 as separate entities, but it means converting from net.IP requires picking whether you intended the "stdlib-ish" behavior, or the "netaddr-ish" behavior.

I don't think we care explicitly about memory usage, although obviously we'd prefer to use fewer bytes for IPv4. But the way netaddr handles v6-mapped v4 means that it's really not an IPv4 at all without careful explicit handling, and the FromStd* helpers currently implement the more surprising behavior (imho).

I think I'd be okay with FromStd* not creating mapped IPs, and having a different way to request a mapping that includes mapped v4 addresses. It's not ideal, but we need a way to resolve the ambiguity created by the net.IP type.

[bradfitz]

@danderson, I think you missed IP.Unmap: https://godoc.org/inet.af/netaddr#IP.Unmap

return netaddr.FromStdIP(stdip).Unmap()

We could add an Unmap method on IPPort too.

[danderson]

btw, the code and bug I introduced are open source in this case:

• In tailscale/tailscale@b01db10 , I converted a package from wgcfg.* to netaddr.*, and did a conversion in the parent package that goes through net.* types.
• In tailscale/tailscale@ccbd093 , fixed the fact that v6-mapped v4 breaks all our networking.

[danderson]

Indeed, I missed Unmap. AFAICT, I will have to use .FromStdIP().Unmap() every time I use FromStdIP(), because the v6-mapped address is almost never what you want. Not great that the simpler code is for the uncommon case :/

[danderson]

Adjusted our conversion code: tailscale/tailscale@48b1e85...e16f7e4

Another possible API: FromStdIP always coerces to the base family (i.e. v6-mapped v4 into v4). If you want to guarantee you have a v6 address, call Map to forcibly make the IP an IPv6, mapping v4 if necessary.

Now the conversion I'm doing in the tailscale code is just FromStdIP. Folks who care about the v4 vs. 4in6 distinction can test what the net.IP encoding is, and call Map as needed after converting. The common case is the least amount of code, the uncommon case is a bit more verbose but still possible.

Thoughts?

[bradfitz]

I'm fine changing FromStdIP to automatically Unmap, as long as we retain some way to go from a std IP faithfully without losing information (even if it's usually useless/misleading/wrong information).

I'm thinking something like FromStdIPRaw, which is uglier so you're unlikely to want to use it, but it sorts just after FromStdIP in godoc.

Other possible names:

• FromStdIPPreserve
• FromStdIPVerbatim

Also, there is no Map (yet?). See #7 for some discussion.