IPPort and IPPrefix MarshalText doesn't round trip

[josharian]

I'm puzzling over how IPPort and IPPrefix's MarshalText methods should work when their IP is the zero IP but they have a non-zero port/bits.

Currently we return an empty slice, but that doesn't round trip. If you do IPPortFrom(IP{}, 80), MarshalText, and then UnmarshalText, you get a zero IPPort. That is, we lose the 80. Similarly so for IPPrefix.

Options:

1. Make MarshalText return an error when there is a zero IP and a non-zero port/prefix. We then lose the nice property that MarshalText never returns an error.
2. Make MarshalText return something like :80 (or invalid IP:80?), and then teach UnmarshalText to handle those inputs. We then lose the nice property that UnmarshalText is identical to ParseIPPort except in its handling of an empty input.
3. Do the previous option, and also change ParseIPPort to match UnmarshalText. We then lose the nice property that there is no way to get a zero IP by parsing a textual input, only by constructing one in code.

I'm inclined towards 1, but don't feel strongly. Opinions?

cc @danderson @maisem @bradfitz @mdlayher

[mdlayher]

Option 1 seems the most straightforward to me.

[danderson]

Having some way of representing and manipulating the equivalent of :80 would be nice, because that's a common way to express "listen on all interfaces". Linux sort-of expressed this with an explicit [::]:80, but really that's more working around a shortcoming of the BSD sockets API.

I confused myself a couple times saying "zero IP", so for the rest of this text I specifically mean the Go zero value, i.e. the moral equivalent of a nil. I specifically do not mean the "all bits zero" IP address of a particular family.

My gut feeling is that an IPPort with a zero IP would be useful if we ever end up with netaddr-aware dial/listen APIs (not in this package, I mean another package that would implement sockets using netaddr), because it's so common to want to express "bind to this port on all interfaces", and our Go API should allow you to express that without resorting to the platform-specific hack of "some IPs have magic meanings that span families".

Based on that, I think I'm biased towards option 3: make :80 a valid, defined IPPort, and handle it as such consistently everywhere.

I also don't feel very strongly, but I do think option 3 is better than the alternatives, because it's the only one that affords complete consistency without introducing panics or rejecting meaningful and useful values like :80. If we go with something else, I would like to favor consistency across APIs: either everything accepts the idea of :80, or none accept it.

[josharian]

That works for me. Should we do the same for /32 to mean IP{} with 32 bits?

[danderson]

I don't think prefixes with a zero IP make sense, because the Bits field's validity depends on the address family. /32 might mean either a single IPv4 address, or a very large IPv6 subnet, and I'm not seeing any benefit to permitting the expression of that concept.

[josharian]

Oops, #174 wasn't supposed to close this issue.

[josharian]

I'm not seeing any benefit to permitting the expression of that concept.

As I mentioned over at #178, the benefit is that the mental model is simple. If I constructed an IPPrefix with a zero IP and n bits, and then ask for n back, I should get n. And that should survive a serialization round trip. It also makes IPPrefix like IPPort, and the more parallel they are, the easier they are to understand. So I guess I'm advocating that we do option 3 for IPPrefix as well.

[danderson]

Fundamentally, it comes down to whether we favor absolute consistency, or whether we permit some variation to only allow things that make sense. I think /32 cannot ever make sense, so I would oppose making it unmarshalable. OTOH, :80 is sensical and has a useful application. I worry that focusing on complete consistency, rather than "what makes sense for each type, with a common default we only depart from when justified", will produce a library that successfully parses garbage into useless values.

[josharian]

I'm not so sure it is garbage, or that the values are useless. It is easy to imagine viewing IPPrefix as a composite type that you build incrementally, in which case /32 has a perfectly sensible meaning: It's the first 32 bits of the IP that I'm going to finish filling out later.

(Tailscale does this in places with IPPorts; we have IPPorts with zero IPs that mean "IP TBD" not "all interfaces". It's true that we don't serialize those, but if we were communicating them back and forth between the front end and the back end, we easily could.)

[danderson]

That implies that "I'm going to fill it out later" means "so much later that I need to marshal it over the network, or to disk." The cases we have in Tailscale, afaik, happen within a single function, while we're incrementally building a value.

I think "/32" is sufficiently esoteric that allowing it will just mask bugs, in which people accidentally transmit partially initialized values rather than notice rapidly that they forgot to finish building it.

From an API POV, I think the consistent API I'd want is: "any value that is Valid() can be marshaled and roundtripped. Values that are not Valid() cannot." Then we define different valid states for different types, based on whether they're representing what we think is a complete value. In IPPort's case, just a port has a reasonable meaning that's not just "I haven't finished building this value yet", so it's Valid. In IPPrefix's case, just Bits probably indicates you're about to transmit a bug over the network, so it's not Valid.

(this feels like a conversation that would be easier as an interactive chat, maybe Monday?)

[josharian]

(this feels like a conversation that would be easier as an interactive chat, maybe Monday?)

I was going to say the same thing. Yes please!

[josharian]

Another data point: Calling WithZone on an IPv4 address currently ignores the newly requested zone (i.e. silently fixes the input as opposed to panicking or returning an error).

I fear our Mondays are on track to not overlap, but hopefully we can sort this all out tomorrow, so we can start working on a proposal to upstream.

[danderson]

We did not get to discussing this issue in our Monday chat. Looks like we might need another.

The conclusions we reached in our last chat don't translate to an obviously correct way forward here.