proposal: net/netaddr: add new IP address type, netaddr package (discussion)

[rsc]

This discussion is for capturing more structured discussion of proposal #46518. The current API additions are:

package net

func (r *Resolver) LookupNetIP(ctx context.Context, network, host string) ([]netip.Addr, error)

func TCPAddrPort(addr netip.AddrPort) *TCPAddr
func (a *TCPAddr) AddrPort() netip.AddrPort

func UDPAddrPort(addr netip.AddrPort) *UDPAddr
func (a *UDPAddr) AddrPort() netip.AddrPort
 
func (c *UDPConn) ReadMsgUDPAddrPort(b, oob []byte) (n, oobn, flags int, addr netip.AddrPort, err error)
func (c *UDPConn) WriteToUDPAddrPort(b []byte, addr netip.AddrPort) (int, error)
func (c *UDPConn) WriteMsgUDPAddrPort(b, oob []byte, addr netip.AddrPort) (n, oobn int, err error)

package netip

type Addr struct { ... unexported ... }

func AddrFrom4(addr [4]byte) Addr
func AddrFrom16(addr [16]byte) Addr  // "raw", no unmapping
func AddrFromSlice(std []byte) (ip Addr, ok bool)  // "raw", no unmapping
func IPv6LinkLocalAllNodes() Addr
func IPv6Unspecified() Addr
func MustParseAddr(s string) Addr
func ParseAddr(s string) (Addr, error)

func (ip Addr) AppendTo(b []byte) []byte
func (ip Addr) As16() [16]byte
func (ip Addr) As4() [4]byte
func (ip Addr) BitLen() int
func (ip Addr) Compare(ip2 Addr) int
func (ip Addr) Is4() bool
func (ip Addr) Is4in6() bool
func (ip Addr) Is6() bool
func (ip Addr) IsGlobalUnicast() bool
func (ip Addr) IsInterfaceLocalMulticast() bool
func (ip Addr) IsLinkLocalMulticast() bool
func (ip Addr) IsLinkLocalUnicast() bool
func (ip Addr) IsLoopback() bool
func (ip Addr) IsMulticast() bool
func (ip Addr) IsPrivate() bool
func (ip Addr) IsUnspecified() bool
func (ip Addr) IsValid() bool
func (ip Addr) Less(ip2 Addr) bool
func (ip Addr) MarshalBinary() ([]byte, error)
func (ip Addr) MarshalText() ([]byte, error)
func (ip Addr) Next() Addr
func (ip Addr) Prefix(bits int) (Prefix, error)
func (ip Addr) Prev() Addr
func (ip Addr) String() string
func (ip Addr) StringExpanded() string
func (ip Addr) Unmap() Addr
func (ip *Addr) UnmarshalBinary(b []byte) error
func (ip *Addr) UnmarshalText(text []byte) error
func (ip Addr) WithZone(zone string) Addr
func (ip Addr) Zone() string

type AddrPort struct { ... unexported ... }

func AddrPortFrom(ip Addr, port uint16) AddrPort
func ParseAddrPort(s string) (AddrPort, error)
func MustParseAddrPort(s string) AddrPort

func (p AddrPort) AppendTo(b []byte) []byte
func (p AddrPort) Addr() Addr
func (p AddrPort) IsValid() bool
func (p AddrPort) MarshalText() ([]byte, error)
func (p AddrPort) Port() uint16
func (p AddrPort) String() string
func (p *AddrPort) UnmarshalText(text []byte) error

type Prefix struct { ... unexported ... }

func PrefixFrom(ip Addr, bits int) Prefix
func MustParsePrefix(s string) Prefix
func ParsePrefix(s string) (Prefix, error)

func (p Prefix) AppendTo(b []byte) []byte
func (p Prefix) Bits() int
func (p Prefix) Contains(ip Addr) bool
func (p Prefix) Addr() Addr
func (p Prefix) IsSingleIP() bool
func (p Prefix) IsValid() bool
func (p Prefix) MarshalText() ([]byte, error)
func (p Prefix) Masked() Prefix
func (p Prefix) Overlaps(o Prefix) bool
func (p Prefix) String() string
func (p *Prefix) UnmarshalText(text []byte) error

[rsc]

@zephyr suggested net/ip instead of net/netaddr, which leads to names like ip.Addr, ip.Port, ip.Prefix, and so on.

[josharian]

ip.Addr, ip.Range, and ip.Set are all clear. ip.Port doesn’t seem as clear; I’d expect it to be just a port, not an IP and a port. Perhaps not a showstopper.

[bcmills]

@cespare, another way to avoid stomping on the useful ip variable name is to reduplicate the net prefix: net/netip instead of net/ip. (There is some precedent in path/filepath, which otherwise could have been named path/file.)

[bcmills]

That gives names like netip.Addr, which is at least a little bit less redundant than ipaddr.Addr, but also scans a bit awkwardly as English (“knee-tip” instead of “net-eye-p”).

[mdlayher]

There's also precedent with net/http/http{test,trace,util}. net/netip or the more descriptive net/ipaddr would also be fine by me.

[cespare]

I like net/netip. That's reasonably short, it satisfies @rsc's preference for the package to be scoped to IP only, and as @bcmills points out netip.Addr is less redundant than ipaddr.Addr. Then I propose the following names:

netip.Addr
netip.AddrPort       // per @josharian's point, netip.Port doesn't sound like a pair
netip.Prefix
netip.Range
netip.Set
netip.SetBuilder

[cespare]

The API surface proposed is (not including deprecated functions):

• type IP: 10 constructor functions, 33 methods
• type IPPort: 4 constructor functions, 13 methods
• type IPPrefix: 4 constructor functions, 14 methods
• type IPRange: 3 constructor functions, 13 methods
• type IPSet: 10 methods
• type IPSetBuilder: 12 methods
• Total: 21 constructor functions, 95 methods

It has been tweaked a bit since #46518 was originally proposed as well.

Unless this is a "meta-proposal" and there are going to be more detailed proposals for different parts of the API, I think it would help for the proposal to list every single type, function, and method along with doc comments, and also update that list in response to feedback (same as we've been doing for e.g. the slices proposal).

[beoran]

If #47331 is accepted, I think we can drop IPset and possibly also IPSetBuilder completely and use generic sets of {type IP, IPPrefix} in stead.

[beoran]

Furthermore,
func IPFrom16(addr [16]byte) IP
seems to be redundant with
func IPv6Raw(addr [16]byte) IP

And
func IPFrom4(addr [4]byte) IP
Seems to be redundant with
func IPv4(a, b, c, d uint8) IP

[rsc]

@beoran, I don't see how generics helps replace IPSet. A generic set will not have "contains specific IP" as a method, unless it is a set[IP], which will be a remarkably memory-intensive way to represent, say, 0.0.0.0/0, not to mention [::0]/0.

[rsc]

As for IPFrom16 etc, let's not worry about those yet. We need to figure out the general direction before we wordsmith specific details.

[beoran]

Ok, apparently i don't understand the difference between an IPSet and an IPRange, my bad, I guess.
I agree we should probably discuss the general direction first.

[cespare]

[Status: change accepted]

I think that functions with "bits" parameters should use int rather than uint8:

• IP.BitLen
• IP.Prefix
• IPPrefixFrom
• IPPrefix.Bits

[Merovius]

ISTM a uint8 can also be out of range, no? So this problem already exists.

[cespare]

For comparison, net already uses int for this sort of thing:

func CIDRMask(ones, bits int) IPMask

[beoran]

In another discussion here I read the long standing design decision of Go is to always use int for lengths, capacities an counts. I guess this also applies here ?

[bradfitz]

ISTM a uint8 can also be out of range, no? So this problem already exists.

Good point. So I suppose the constructor just cap it at 255 at report false on IsValid.

The uint8 mattered when the struct had exported fields but once we closed all of them (to match IP) then we could've only exposed int at the edges.

[cespare]

@rsc I still believe we should change these four parameters from uint8 to int.

@bradfitz it sounds like you're okay with it based on your last comment?

[cespare]

[Status: retracted]

I don't think we need IP.Compare given that we have IP.Less.

[bradfitz]

That arose from porting code that was doing bytes.Compare on net.IP []byte values, like:

// routeLess reports whether ri should sort before rj.
// The actual sort order doesn't appear to matter. The caller just
// wants them sorted to be able to de-dup.
func routeLess(ri, rj *winipcfg.RouteData) bool {
        if v := bytes.Compare(ri.Destination.IP, rj.Destination.IP); v != 0 {
                return v == -1
        }
        if v := bytes.Compare(ri.Destination.Mask, rj.Destination.Mask); v != 0 {
                // Narrower masks first
                return v == 1
        }
        if ri.Metric != rj.Metric {
                // Lower metrics first
                return ri.Metric < rj.Metric
        }
        if v := bytes.Compare(ri.NextHop, rj.NextHop); v != 0 {
                // No nexthop before non-empty nexthop.
                return v == -1
        }
        return false
}

It came up often enough that we added the accessor. (None of the methods were added preemptively)

[cespare]

OK, fair enough. I'll retract this suggestion.

[cespare]

[Status: change accepted]

Instead of Next/Prior, I think it should be Next/Prev or Next/Previous.
(For...ahem, prior art, consider that there is no Prior method in the standard library today, but container/list and container/ring both have Prev methods.)

[cespare]

@rsc looks like you took this change?

[cespare]

What's the purpose of IPPrefix.Masked? It's not clear to me when I'd need it. It also seems easy enough to write p.IP().Prefix(p.Bits()).

[bradfitz]

Masked returns 1 value, but Prefix(bits) is 2 value comma-ok, so it's not as nice to use.

Current usages in our tree at least:

bradfitz@tsdev:~/src/tailscale.com$ git grep -B 2 -A 1 -F '.Masked('
cmd/tailscale/cli/up.go-                                return nil, fmt.Errorf("%q is not a valid IP address or CIDR prefix", s)
cmd/tailscale/cli/up.go-                        }
cmd/tailscale/cli/up.go:                        if ipp != ipp.Masked() {
cmd/tailscale/cli/up.go:                                return nil, fmt.Errorf("%s has non-address bits set; expected %s", ipp, ipp.Masked())
cmd/tailscale/cli/up.go-                        }
--
wgengine/filter/tailcfg.go-                     return nil, err
wgengine/filter/tailcfg.go-             }
wgengine/filter/tailcfg.go:             if pfx != pfx.Masked() {
wgengine/filter/tailcfg.go-                     return nil, fmt.Errorf("%v contains non-network bits set", pfx)
--
wgengine/router/router_linux.go-// of the IP address zeroed out.
wgengine/router/router_linux.go-func normalizeCIDR(cidr netaddr.IPPrefix) string {
wgengine/router/router_linux.go:        return cidr.Masked().String()
wgengine/router/router_linux.go-}

[cespare]

@rsc looks like you dropped Prefix.Masked?

[mdlayher]

In addition to the convenience of p != p.Masked() to check if an address contains any bits set beyond the prefix length, I use this API to quickly check for unique prefixes when pulling addresses directly from a network interface:

https://play.golang.org/p/rSOjAvH6Mwy

The alternative would be:

https://play.golang.org/p/0gfInYF7WOD

I still think Masked is useful especially when working with IP addresses and their associated subnet masks derived from operating system network interfaces.

[mdlayher]

Seems like an oversight? This is present in Brad's CL: https://go-review.googlesource.com/c/go/+/339309/.

[cespare]

It's surprising that IPRangeFrom accepts invalid ranges, especially given that ParseIPRange does not. Perhaps it should return an error?

[bradfitz]

See inetaf/netaddr#178 ("Why do IPPrefixFrom and IPRangeFrom permit constructing invalid values?")

[cespare]

The IPRange and IPSet/IPSetBuilder types feel a bit specialized for inclusion in the standard library to me.

The proposal says:

We wouldn't need to import all of it, at least not right away, but I think it'd be nice if we did... the package types work together very well.

"Working together well" seems like a necessary-but-not-sufficient reason for adding something to the standard library. Could you say more about why these types are important to add?

Additionally, these types are built on top of the other ones, so it would be easy enough to leave them out initially and add them in later.

[rsc]

I've pasted the current API from Brad's CL into the top comment on this discussion.
There are two different packages we could add here.
The fundamental question seems to be whether to add a package with
(1) lighter-weight address implementations for package net or
(2) a general-purpose IP address arithmetic package.
Either could easily be justified for the standard library.
We just have to decide the direction we want.

If we decide to do (1), then it looks like only IP and IPPort are needed,
and net/netaddr may be a fine name. It might in that case also make
sense to move net.Addr, net.UnixAddr, net.TCPAddr, and net.UDPAddr in,
so that from here on the rule would be that you'd use net/netaddr for address stuff.
Or it might not.

If we decide to do (2), then the whole set IP, IPPort, IPPrefix, IPRange, IPSet, IPSetBuilder makes some sense.
I would be reluctant to call this net/netaddr at that point, because it is providing things
that really aren't needed by package net's API
(in retrospect, adding net.IPNet was probably a mistake for the same reason),
and also it is not providing non-IP addresses (Unix socket addresses, MAC addresses, etc).
It would help to have it clearly scoped to IP addresses, so it is clear what belongs and what does not.

If we do (2), then net/ip or net/netip still seems like a properly scoped name,
with IP, IPPort (not just Port, to make clear it is an IP+port pair), Prefix, Range, Set, SetBuilder.
(or, as suggested elsewhere, Addr, AddrPort, Prefix, Range, Set, SetBuilder; unimportant right now).

I suppose there is an alternative (3) a general purpose all-address package,
which would have all the IP stuff but also MAC address parsing and other addresses too,
but that seems strictly worse than either of the other two.

The argument in favor of (1) is that it keeps things small.
The argument in favor of (2) is that these are fundamental operations on IP addresses,
and we might as well provide a single, well-designed API in a single, canonical place.
The alternative is putting half the routines here and the other half elsewhere.

[beoran]

I am in favor of (1). The option (2) seems to fall under https://golang.org/doc/faq#x_in_std.

If i compare just the proposed API on top, with the API of the ebiten game library, then the amount of functions/methods is almost the same. From my point of view, a game library is much more useful than an IP address calculation package, but i won't suggest we integrate ebiten in the standard library. ;-)

I also don't see why the Go developers want to take on this seemingly heavy maintenance burden and loose the flexibility of a separate release cycle.

I would add a simpler package with non-allocating addresses in it, and leave the IP address range calculation as a separate package that uses the (1) but can evolve independently.

[rsc]

Independent of the answer to the general approach, we have to figure out the terminology to use for the new versus old IP addresses and explain clearly when to use each. The current netaddr package calls them “standard IP addresses” (FromStd) but we are talking about making the new representation “standard” too.

I spent a while thinking about alternatives (legacy, old, etc), and I had an aha! moment on “slice”. It's just the slice form of the address.

For example:

package net

// IP is a single IP address represented as a slice of bytes.
// A 4-byte slice is an IPv4 address, and a 16-byte is an IPv6 address.
// IP was originally the only IP address representation in this package.
// In new code, it is usually more efficient and convenient to use netip.Addr instead.
type IP []byte

and in net/netip (assuming we go that way):

// FromSlice converts a 4-byte or 16-byte slice into an Addr.
// If slice's length is not 4 or 16, FromSlice returns Addr{}, false.
// Note that a net.IP can be passed to this function without conversion.
func FromSlice(slice []byte) (a Addr, ok bool)

This would be instead of FromStdIP and FromStdIPRaw, with the “raw” (no unmapping) semantics.
If people want unmapping, they can always call Unmap on the result.

@bradfitz, do you have any numbers on how often code needed FromStdIP and couldn't have used FromStdIPRaw instead?

[earthboundkid]

You could go further an rename net.IP to net.IPSlice with a type alias for the deprecated name. That would make the docs pretty clear about what to use and why without breaking anyone.

[bcmills]

@carlmjohnson as a minor nit, I would call the type SliceIP instead of IPSlice: it is a single IP represented as a slice, not a “slice of IPs” as I (and, I assume, at least some others) would assume from the IPSlice name.

[rsc]

To try to move forward, I suggest we answer #47323 by saying (2) a general IP manipulation package, imported as net/netip, and have it contain:

type Addr
type AddrPort
type Prefix
type Range

but for now leave out Set and SetBuilder, which can be provided in x/net/ipset or something like that.

(With the various adapters in package net to use them, as before.)

[beoran]

Well splitting off Set and SetBuilder already trims it down considerably, so it's a step in the right direction.

[cespare]

@rsc the current API at the top contains

func (p Prefix) Range() AddrRange

which should also be dropped.

[rsc]

Thanks, fixed.

[rsc]

Seems like we are getting close. Based on chat with Brad, I made the following changes in the API atop the discussion:

• Resolver.LookupNetAddr -> Resolver.LookupNetIP
• Dropped the IsZero methods; let people use IsValid or something else.
• Note: Unmarshal methods should allow overwriting any value.
• From4 takes [4]byte to match As4.
• Renamed From4/From16/FromSlice to AddrFrom4, AddrFrom16, AddrFromSlice.
• Dropped AddrPort's WithIP and WithPort.
• Changed two uint8 to int.

Any remaining objections?

[cespare]

Can you clarify whether Prefix.Masked was dropped intentionally or unintentionally, per #47323 (comment)?

(Side note: it has not been easy to follow the changes made based on this discussion because there has been no explicit resolution for suggestions. For example, these two suggestions of mine [1, 2] seem to have been accepted, but I had to notice the edits and mark them as accepted myself. Or this suggestion which got feedback from Brad but no explicit rejection. This is at least partly a shortcoming of Discussions.)

[rsc]

It's true, we haven't been marking individual comments. I'll go through and do that. Thanks.

[rsc]

Prefix.Masked was not dropped intentionally. It's back.

[iangcarroll]

Not sure if this is the best place for this comment, but I recently used this in my implementation of a security control to prevent server-side request forgery (migrating from this solution).

Unfortunately, while doing so I shot myself in the foot by solely using .IsPrivate(), instead of .IsPrivate() || .IsLoopback() to prevent local IPs as well. While this technical distinction might be correct, it seems hard to securely use this new package for a common use-case (i.e. blocking internal requests). It's not even clear to me if both of these is completely sufficient and doesn't make me feel very confident in this API.

[ydnar]

Would an IsPublic method help?

[iangcarroll]

I think IsPublic would have been great given the previous implementation I linked is very similar (provides an IsPublic interface). I fear I might have still made the mistake without an explicit comment visible in the IDE, though, perhaps it could become something like:

// IsPrivate reports whether ip is a private address, according to
// RFC 1918 (IPv4 addresses) and RFC 4193 (IPv6 addresses).
// This does not cover all local addresses, including
// loopback addresses (see IsLoopback). 
func (ip IP) IsPrivate() bool {

(or a similar mention for IsPublic if it was implemented)

[ydnar]

Do you imagine something like this?

func (ip IP) IsPublic() bool {
    return !ip.IsPrivate() && ip.IsGlobalUnicast()
}

[iangcarroll]

Yeah, that seems to catch a decent number of cases. I'm not sure if IsGlobalUnicast covers every case of the previous implementation I was using, but perhaps they are not necessary in practice.