maxminddb-golang

maxminddb-golang copied to clipboard

Go 1.18 includes the new net/netip package with value types related to IP addresses and networks.

Perhaps, new methods should be added, for example Reader.LookupAddr(ip netip.Addr, result any) and Reader.Prefixes with a corresponding iterator, that would encourage the use of the new IP types. I think, it could also reduce some of the boilerplate such as:

https://github.com/oschwald/maxminddb-golang/blob/ccd731c09b8d2ce944741bd627e8589c5c5f0654/reader.go#L247-L250

and a lot of len(ip) == net.IPv4len and ip.To4() == nil.

I'd also like to see this. That said, I am planning on waiting until 1.19 is released and 1.17 has been end-of-lifed.

Now that 1.19 is out and 1.17 is EOL, can we reconsider this? :pray:

Is there a particular use case beyond avoiding an AsSlice() call in your code? Internally, we will still need to convert the address to a byte slice, meaning that a net/netip version will likely cause an extra allocation over the current lookup methods. The number of lookup methods is also somewhat unwieldy already and I am not sure I want to add a net/netip variant of each.

Is there a particular use case beyond avoiding an AsSlice() call in your code?

Not really. I just had a cursory look at the code and naively expected the migration to be useful in a few cases. But nothing concrete.

The number of lookup methods is also somewhat unwieldy already and I am not sure I want to add a net/netip variant of each.

Totally understandable :+1:

@oschwald, sorry for the long delay.

Is there a particular use case beyond avoiding an AsSlice() call in your code?

Not just AsSlice, but FromSlice as well. As well as additional calls for netip.Prefix handling.

And it's not actually as easy, see golang/go#53607 and the related issues. In short, net.IP doesn't distinguish between IPv4 and IPv6-mapped-IPv4 addresses, while netip.Addr does, so a naïve conversion will result in a netip.Addr for which Is6 returns true despite the fact that it's an IPv4 address. Since the package already has the information about the correct protocol, it returning the correct addresses and subnets would be greatly appreciated.

Internally, we will still need to convert the address to a byte slice, meaning that a net/netip version will likely cause an extra allocation over the current lookup methods.

It's hard to judge the real impact here without benchmarks, but since netip.Addr is essentially a value type, the number of allocations shouldn't become that much higher. In particular, I feel like if netip.Addr is made the primary internal type, and the conversion is only done when accepting and returning net.IP and friends, the number of allocations should stay approximately the same.

netip.Addr does not make sense as the primary internal type. As mentioned, we would need to immediately convert it to a slice so that we can access the individual bits. Whether that slice is a net.IP or a []byte is somewhat immaterial. As such, the netip.Addr path would require an extra allocation over the net.IP path as a conversion to a slice would be necessary.

Also, net.IP not distinguishing between IPv4 and IPv4-mapped IPv6 addresses, that is actually an advantage in this case as the MMDB format is similar.

As far as *net.IPNet requiring extra work, that is a fair point. However, I believe those methods are less frequently used.

netip.Addr does not make sense as the primary internal type. As mentioned, we would need to immediately convert it to a slice so that we can access the individual bits.

Maybe we can use As4() and As16() and create a slice from the returned array, if the compiler is smart enough to figure out that the slice won't have to be heap allocated. Or just use [16]byte as the primary internal type.