Updated ipHash to calculate over the actual IP

[magapp]

The ipHash() function generates a hash from the resolved IP.

Because net.IP is a 16 bytes array, sometimes the array have leading 0 (depending if the target is an IP or hostname). Also, an IPv4 address could be represented as 0.0.0.0.0.0.0.0.0.0.255.255.8.8.8.8 in the byte array or just 8.8.8.8.0.0.0.0.0.0.0.0.0.0.0.0. This makes the hash unreliable.

Example:

host www.whatsmyip.org
www.whatsmyip.org has address 208.79.209.138

curl -s "localhost:9115/probe?target=www.whatsmyip.org&module=icmp&debug=true" | grep ^probe_ip_addr_hash
probe_ip_addr_hash 3.248520427e+09

curl -s "localhost:9115/probe?target=208.79.209.138&module=icmp&debug=true" | grep ^probe_ip_addr_hash
probe_ip_addr_hash 6.13293073e+08

Simply using String() and calculate checksum over the actual IP-string instead will fix this. Also, as a side effect, it is easier to precalculate hash and compare it (for example, I calculate the hash in Python that create metrics that is later compared to see if address resolves correct or not).

[dgl]

I can't reproduce this, for me it looks like in both cases I see the 16 byte version. Given the shorter form is coming from DNS it could be many differences in the path through the DNS libraries you end up taking (I tried with Go 1.17.3 on darwin/arm64 and Go 1.16.5 on linux/amd64).

Because net.IP is a 16 bytes array

Not always, see the description of the IP type at https://pkg.go.dev/net#IP -- it's either a 4 byte or 16 byte slice, if it's 16 byte it's in the IPv4 mapped form (with 0xff in there).

I think you're along the right lines though, this is varying when it's a 4-byte vs 16-byte slice, but like I said I can't reproduce seeing the 4 byte slice.

Entirely changing the hash seems a bit drastic though. I think a better approach would be to always hash the 16 byte address, i.e. h.Write(ip.To16()) in ipHash, regardless of the form of the input.

[I'm not a maintainer of this, but if you're willing to update your PR to use To16() I think that would be reasonable, please also follow the DCO process to add the Signed-off-by lines.]

[magapp]

My first thought was to use To16(), but the hash differ. This is because the IP can be either one of these two string:

8.8.8.8.0.0.0.0.0.0.0.0.0.0.0.0.
0.0.0.0.0.0.0.0.0.0.255.255.8.8.8.8

Agree, that it could be caused by dns libraries. However, you should be able to reproduce it using the linux compiled binary. Have you ipv6 enabled in your setup?

Please also test with both "preferred_ip_ protocol ip4", "preferred_ip_ protocol ip6" (and with IP or hostname).

[SuperQ]

I agree with @dgl, we don't want to String() this.

[SuperQ]

I did some testing, I can reproduce this issue. The fix is this:

Suggested change

	h.Write([]byte(ip.String()))
	if ip.To4() != nil {
	h.Write(ip.To4())
	} else {
	h.Write(ip.To16())
	}

[electron0zero]

Q: can we close this in favor of #863?

[SuperQ]

I was trying to give @magapp some time to respond to the feedback. Maybe in a couple days we merge #863 and close this one.

[SuperQ]

Closing in favor of #863.

Thanks for finding the bug.