android: fix local endpoint parsing and add regression ifaceparse_test

[kari-ts]

Android reports interface addresses in CIDR form. Recent changes (see 9c933a0#diff-1d627686c31972e04ef60d7d301e8a2a93714c60096a50055a6bbe9aa041ca8fL105, 9c933a0#diff-1d627686c31972e04ef60d7d301e8a2a93714c60096a50055a6bbe9aa041ca8fL105) parsed addresses as CIDR prefixes, resulting in prefix base being advertised as a local endpoint. This change:

• instead of parsing CIDR prefixes, the parser strips the /NN portion and uses netip.ParseAddr to parse the host IP only
• extracts the interface parsing into a helper in a new package to make it testable on non-Android platforms
• adds a unit test to verify that host addresses are present and prefix-base ones are not

Fixes tailscale/tailscale#16836

[raggi]

Do you know if that string interface is stable?

I wonder if we should be using GetInterfaces() and building the structure that we want to pass to go?

[kari-ts]

It's internally stable, but brittle - we're relying on the number of fields and the order staying the same, etc. We're also relying on InterfaceAddress.toString(), which might not work across OEMs,
We could pass a JSON across the Android/go boundary. I'll work on this approach and rerequest review when it's ready.

[sfllaw]

@kari-ts Thanks for trying to fix this properly. I have a few concerns because I’m unfamiliar with the code, but I also don’t want to block tailscale/tailscale#16836 which seems like it is pretty annoying. I am OK if you agree with my concerns but want to address them in a separate PR.

[sfllaw]

@kari-ts The refactor looks very promising. Sorry to be a bit picky about this, but it looks like this is a brittle piece of code and it would be good to fix it properly. Is there any need to keep the old string parsing code if we can reliably marshal as JSON?

[sfllaw]

⛏️ Nit (non-blocking): It seems weird to be counting lines when really we are processing interfaces. I think you’re encoding JSON in a compact form, so there aren’t any line breaks anyway?

[kari-ts]

I renamed "lines" to "interfaces" since lines no longer maps to anything meaningful

[sfllaw]

@kari-ts Approved, because this is basically done. I only have a couple style concerns, but they are not super important. You can merge whether you want to change them or not.

[sfllaw]

⚠️ Note: this restores the old behaviour, from before #706, for code like: https://github.com/tailscale/tailscale/blob/6c67deff3805e7c90894c9aced1b594854747b87/net/netmon/state.go#L196-L207

for _, a := range addrs {
	switch v := a.(type) {
	case *net.IPNet:
		if pfx, ok := netaddr.FromStdIPNet(v); ok {
			fn(iface, pfx)
		}
	case *net.IPAddr:
		if ip, ok := netip.AddrFromSlice(v.IP); ok {
			fn(iface, netip.PrefixFrom(ip, ip.BitLen()))
		}
	}
}

[kari-ts]

I think this is ok; the pre-#706 implementation appended the *net.IPNet directly, and here we're setting IP to the host IP, so we don't reintroduce that previous behavior where we were just using the prefix base

[sfllaw]

Agreed! Sorry for the confusion. I was just leaving a note here for our future selves to explain why we moved away from IPNet in #706 and then restored it in this PR.

[sfllaw]

✍️ Style (non-blocking): I admit that I haven’t ever encountered this danger before, can you help me understand why the obvious implementation isn’t safe? And if we really want to clone the slice, have you seen slices.Clone?

Suggested change

	ip := net.IP(append([]byte(nil), na.AsSlice()...)) // copy to be safe
	ip := net.IP(na.AsSlice())

[kari-ts]

hm, my fear was aliasing, but I don't know how big of a risk that is here. Updated to use slices.Clone - thanks!

[sfllaw]

I’m 99% sure that netip.Addr.AsSlice will never be changed to reuse memory. That would probably break so many pieces of software. But there’s no harm with cloning and it’s not like this is a hot loop or anything.