inetaf/netaddr

make IPPrefix opaque, smaller?

bradfitz opened this issue · 6 comments

bradfitz commented

IPPrefix is currently defined as:

type IPPrefix struct {
	IP   IP
	Bits uint8
}

That means it takes 32 bytes (25 bytes + padding: https://play.golang.org/p/4XZ9LQWEwj4).

And it also means callers need to create them with keyed struct literals, lest they anger go vet's cross-package-unkeyed-struct-literal check:

 ipp := netaddr.IPPrefix{IP: ..., Bits: ....}

We extolled the virtues of opaque representations in https://tailscale.com/blog/netaddr-new-ip-type-for-go/, but our IPPrefix type isn't.

We could instead do:

type IPPrefix struct {
     addr uint128
     isv6 bool
     bits uint8
}

And we'd be at 24 bytes after padding: https://play.golang.org/p/oS6tg8RtycQ

We'd then need methods:

func (p IPPrefix) IP() IP
func (p IPPrefix) Bits() uint8

But those would be alloc-free.

We'd then need a constructor. I can't think of a great. NewFoo should return a pointer to Foo, so NewIPPrefix isn't great. Make isn't great. Maybe:

  • func Prefix(ip IP, bits uint8) IPPrefix ?
  • func IPBits(ip IP, bits uint8) IPPrefix ?

/cc @danderson @josharian @mdlayher @crawshaw

bradfitz commented

I forgot to mention: this all assumes an IPPrefix of an IPv6 zone doesn't make sense. Is that correct?

danderson commented

I'm not sure if a v6+zone prefix makes sense. My mind immediately jumped to v6 link-local prefixes, which would implicitly be per-interface things, but I don't know if they conventionally use the addr zone to represent that scoping.

In theory, what you propose seems fine, although I think I might regret the opacity, because I'm used to being able to easily construct prefixes and extract their component parts.

Along the same lines: should we be doing that to IPPort and friends as well? If we believe in the value of opaque types, should all types in this package be opaque? Makes them a little uglier to use imho, but maybe worth it?

josharian commented

If we need ipv6 zones + prefixes, we can intern the zone and bits together. (May need new package intern api to avoid allocating.)

mdlayher commented

+1 for making the type opaque (and sure, why not do the others too) and I like func Prefix(ip IP, bits uint8) IPPrefix.

I also can't think of any legitimate use for IPv6 zone + prefix. I've never seen notation like fe80::/10%eth0 or similar and I think you'd be hard pressed to find any software that accepts that sort of syntax.

josharian commented

I spent a bit of time today looking at the impact of making IPPort opaque. (Whatever we decide for IPPort we'll probably do for IPPrefix, and vice versa.)

First, we need a way to construct IPPort values. Two possible API styles:

// WithPort returns an IPPort with IP ip and port port.
func (ip IP) WithPort(port uint16) IPPort

// NewIPPort returns an IPPort with IP ip and port port.
func NewIPPort(ip IP, port uint16) IPPort

You can see what those look like at call sites within Tailscale at tailscale/tailscale@920145a and tailscale/tailscale@eec74d4. I'm inclined to the IP method, even though it is perhaps a bit unusual and not great in godoc, because the call sites look simple and concise.

Second, we need accessors for IP and Port. Those are pretty clear. It's unfortunate that they collide with existing field names, because it means we can't easily use automated type-safe refactoring tools to make the required changes (requires multiple steps), but there's not much to do about that.

Third, we need to deal with code that sets the IP and Port fields, which we had a surprising amount of in Tailscale. This is generally used for incrementally constructing and then modifying an IPPort. We don't want setters (it's a value type), but replacement via constructing a new value is pretty ugly. Options I see here include:

  • Do nothing. Have call sites construct a new value as needed.

  • Provide field replacement methods that construct a new value out of an existing value and a "field" to change:

// WithIP returns an IPPort with IP ip and port p.Port().
func (p IPPort) WithIP(ip IP) IPPort
// WithPort returns an IPPort with ip p.IP and port port.
func (p IPPort) WithPort(port uint16) IPPort
  • Use separate IP and Port fields in the callers. I'd be tempted by this, given the complexity and verbosity of the fixes involved, but it's kind of sad.

I'm inclined towards "do nothing" (and let callers do what they will), and figure we can add field replacement methods later if really needed.

The code changes here are pretty minimal, and I'm happy to make them. Feedback on API requested. This is a blocker for any attempt to use netaddr in std, so it'd be nice to make progress on it soonish.

bradfitz commented

Decision from meeting:

func IPPrefixFrom
func IPPortFrom
func IPRangeFrom
func (p IPPort) WithIP(ip IP) IPPort
func (p IPPort) WithPort(port uint16) IPPort

And make all three opaque.